Return-Path: X-Original-To: apmail-httpd-dev-archive@www.apache.org Delivered-To: apmail-httpd-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 9C33D7395 for ; Sun, 24 Jul 2011 23:36:49 +0000 (UTC) Received: (qmail 3557 invoked by uid 500); 24 Jul 2011 23:36:48 -0000 Delivered-To: apmail-httpd-dev-archive@httpd.apache.org Received: (qmail 3187 invoked by uid 500); 24 Jul 2011 23:36:47 -0000 Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm Precedence: bulk Reply-To: dev@httpd.apache.org list-help: list-unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@httpd.apache.org Received: (qmail 3179 invoked by uid 99); 24 Jul 2011 23:36:47 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 24 Jul 2011 23:36:47 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=SINGLE_HEADER_2K,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of DRuggeri@primary.net designates 216.87.38.208 as permitted sender) Received: from [216.87.38.208] (HELO mail8.primary.net) (216.87.38.208) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 24 Jul 2011 23:36:37 +0000 Received: from home.simonrage.com ([216.114.77.126]:32866 helo=[192.168.0.2]) by mail8.primary.net with esmtpa (Exim 4.63) (envelope-from ) id 1Ql8DU-0003Eo-F9 for dev@httpd.apache.org; Sun, 24 Jul 2011 18:36:15 -0500 Message-ID: <4E2CAC43.2090505@primary.net> Date: Sun, 24 Jul 2011 18:35:31 -0500 From: Daniel Ruggeri User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0) Gecko/20110624 Thunderbird/5.0 MIME-Version: 1.0 To: dev@httpd.apache.org Subject: Re: Question and request for comments on patch References: <4E290225.5090305@primary.net> <201107240912.38759.sf@sfritsch.de> In-Reply-To: <201107240912.38759.sf@sfritsch.de> Content-Type: multipart/mixed; boundary="------------090600070700090707050609" X-ACL-Warn: X-The email account used to send this email was: DRuggeri@primary.net X-Spam-Score: -2.9 (--) X-Spam-Report: Spam detection software, running on the system "mail8.primary.net", has identified this incoming email as possible spam. The original message has been attached to this so you can view it (if it isn't spam) or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: On 7/24/2011 2:12 AM, Stefan Fritsch wrote: > On Friday 22 July 2011, Daniel Ruggeri wrote: >> Attached is the final cut of the patch including doco and MMN bump >> as you brought up. I plan to commit this on Monday, time >> permitting (and of course in the absence of objections). I'll >> cobble something together afterwards for a 2.2 backport. > +1 to the principle. This also allows to use LogLevel in .htaccess, > which I wanted to implement but never got around to. > > Some thoughts / nitpicks: > style: Include a space between an if and the opening brace. Done - looks like I only did this in a few spots some how. > We need to support C90: > core.c: In function 'set_override_list': > core.c:1626:5: error: ISO C90 forbids mixed declarations and code [- > Werror=declaration-after-statement] Done also > > When parsing the list, look in ap_config_hash if a directive exists. > If not, either log a warning or error out (not sure which is better). I may be missing something, but isn't ap_config_hash private to config.c and unavailable to core.c where the configuration directive is implemented? If I'm off, I'd definitely like to add this as well as remove the need for the 'tolower' calls by looking up the expected CamelCase of a directive as it shows up in config.c. > I think the word1,word2,... syntax is unwieldly, especially if the > list gets long. Maybe use AP_INIT_TAKE_ARGV instead (see e.g. > AllowMethods). This is far from a nitpick and makes a lot more sense. I have implemented this as well. > It should be possible to reset AllowOverrideList to an empty list in a > subdir, even if it is set in the parent dir. This is important in the > case that there is a permissive AllowOverrideList in main server scope > and an empty one for some virtual hosts. In the case of an empty > list, d->override_list should be set to NULL instead of an empty table > for better performance. Maybe one could allow "disabled" or "reset" > as alias for an empty list (like in UserDir/DirectoryIndex). I realized I did not add this about [...] Content analysis details: (-2.9 points, 10.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] X-Virus-Checked: Checked by ClamAV on apache.org This is a multi-part message in MIME format. --------------090600070700090707050609 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit On 7/24/2011 2:12 AM, Stefan Fritsch wrote: > On Friday 22 July 2011, Daniel Ruggeri wrote: >> Attached is the final cut of the patch including doco and MMN bump >> as you brought up. I plan to commit this on Monday, time >> permitting (and of course in the absence of objections). I'll >> cobble something together afterwards for a 2.2 backport. > +1 to the principle. This also allows to use LogLevel in .htaccess, > which I wanted to implement but never got around to. > > Some thoughts / nitpicks: > style: Include a space between an if and the opening brace. Done - looks like I only did this in a few spots some how. > We need to support C90: > core.c: In function 'set_override_list': > core.c:1626:5: error: ISO C90 forbids mixed declarations and code [- > Werror=declaration-after-statement] Done also > > When parsing the list, look in ap_config_hash if a directive exists. > If not, either log a warning or error out (not sure which is better). I may be missing something, but isn't ap_config_hash private to config.c and unavailable to core.c where the configuration directive is implemented? If I'm off, I'd definitely like to add this as well as remove the need for the 'tolower' calls by looking up the expected CamelCase of a directive as it shows up in config.c. > I think the word1,word2,... syntax is unwieldly, especially if the > list gets long. Maybe use AP_INIT_TAKE_ARGV instead (see e.g. > AllowMethods). This is far from a nitpick and makes a lot more sense. I have implemented this as well. > It should be possible to reset AllowOverrideList to an empty list in a > subdir, even if it is set in the parent dir. This is important in the > case that there is a permissive AllowOverrideList in main server scope > and an empty one for some virtual hosts. In the case of an empty > list, d->override_list should be set to NULL instead of an empty table > for better performance. Maybe one could allow "disabled" or "reset" > as alias for an empty list (like in UserDir/DirectoryIndex). I realized I did not add this about five minutes after sending the patch along to the list. I have added this also. Your followup email about making the list NULL complicating things is also true - I had a lot of trouble merging properly when NULL was considered "nothing in the list" as opposed to "not configured". > It may be possible that some modules react badly if a directive is > used in .htaccess that has previously not been allowed there. For > example if the module needs to do some global initialization when a > directive is used for the first time. I am not aware of such a case, > but it is something we may want to keep in mind if this is backported. > And it is definitely something that should go into the new_api_2.4 > document. Yes, very good point - I did not realize this would become a side effect. I thought there were upstream context checks before a command is invoked to prevent this. I have taken the second suggestion and added a note in new_api_2.4.xml also. Thank you for the review. Much appreciated. Attached is the patch including the suggested changes except the directive look up. -- -- Daniel Ruggeri --------------090600070700090707050609 Content-Type: text/plain; name="httpd-trunk.AllowOverrideList.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="httpd-trunk.AllowOverrideList.patch" Index: httpd-trunk.AllowOverrideList/docs/manual/developer/new_api_2_4.xml =================================================================== --- httpd-trunk.AllowOverrideList/docs/manual/developer/new_api_2_4.xml (revision 1150484) +++ httpd-trunk.AllowOverrideList/docs/manual/developer/new_api_2_4.xml (working copy) @@ -92,6 +92,13 @@

common structures for heartbeat modules (should this be public API?)

+
+ ap_parse_htaccess (changed) +

The function signature for ap_parse_htaccess has been + changed. A apr_table_t of individual directives allowed + for override must now be passed (override remains).

+
+
http_config (changed)
    Index: httpd-trunk.AllowOverrideList/docs/manual/mod/core.xml =================================================================== --- httpd-trunk.AllowOverrideList/docs/manual/mod/core.xml (revision 1150484) +++ httpd-trunk.AllowOverrideList/docs/manual/mod/core.xml (working copy) @@ -327,10 +327,11 @@ Files sections. -

    When this directive is set to None, then - .htaccess files are completely ignored. - In this case, the server will not even attempt to read - .htaccess files in the filesystem.

    +

    When this directive is set to None and AllowOverrideList is set to + None .htaccess files are + completely ignored. In this case, the server will not even attempt + to read .htaccess files in the filesystem.

    When this directive is set to All, then any directive which has the .htaccess + +AllowOverrideList +Individual directives that are allowed in +.htaccess files +AllowOverrideList None|directive +[directive-type] ... +AllowOverrideList None +directory + + +

    When the server finds an .htaccess file (as + specified by AccessFileName) + it needs to know which directives declared in that file can override + earlier configuration directives.

    + + Only available in <Directory> sections + AllowOverrideList is valid only in + Directory + sections specified without regular expressions, not in Location, DirectoryMatch or + Files sections. + + +

    When this directive is set to None and AllowOverride is set to None, + then .htaccess files are completely + ignored. In this case, the server will not even attempt to read + .htaccess files in the filesystem.

    + +

    Example:

    + + + AllowOverride None + AllowOverrideList Redirect RedirectMatch + + +

    In the example above only the Redirect and + RedirectMatch directives are allowed. All others will + cause an internal server error.

    + +

    Example:

    + + + AllowOverride AuthConfig + AllowOverrideList CookieTracking CookieName + + +

    In the example above AllowOverride + grants permission to the AuthConfig + directive grouping and AllowOverrideList grants + permission to only two directves from the FileInfo directive + grouping. All others will cause an internal server error.

    + + AccessFileName +AllowOverride Configuration Files .htaccess Files Index: httpd-trunk.AllowOverrideList/server/config.c =================================================================== --- httpd-trunk.AllowOverrideList/server/config.c (revision 1150484) +++ httpd-trunk.AllowOverrideList/server/config.c (working copy) @@ -838,10 +838,20 @@ static const char *invoke_cmd(const command_rec *cmd, cmd_parms *parms, void *mconfig, const char *args) { + int override_list_ok = 0; char *w, *w2, *w3; const char *errmsg = NULL; - if ((parms->override & cmd->req_override) == 0) + /** Have we been provided a list of acceptable directives? */ + if (parms->override_list != NULL) { + char *directivelc=apr_pstrdup(parms->pool,cmd->name); + ap_str_tolower(directivelc); + + if (apr_table_get(parms->override_list, directivelc) != NULL) + override_list_ok = 1; + } + + if ((parms->override & cmd->req_override) == 0 && !override_list_ok) return apr_pstrcat(parms->pool, cmd->name, " not allowed here", NULL); parms->info = cmd->cmd_data; @@ -1506,7 +1516,7 @@ */ static cmd_parms default_parms = -{NULL, 0, 0, -1, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL}; +{NULL, 0, 0, NULL, -1, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL}; AP_DECLARE(char *) ap_server_root_relative(apr_pool_t *p, const char *file) { @@ -2005,7 +2015,7 @@ AP_CORE_DECLARE(int) ap_parse_htaccess(ap_conf_vector_t **result, request_rec *r, int override, - int override_opts, + int override_opts, apr_table_t *override_list, const char *d, const char *access_name) { ap_configfile_t *f = NULL; @@ -2027,6 +2037,7 @@ parms = default_parms; parms.override = override; parms.override_opts = override_opts; + parms.override_list = override_list; parms.pool = r->pool; parms.temp_pool = r->pool; parms.server = r->server; Index: httpd-trunk.AllowOverrideList/server/core.c =================================================================== --- httpd-trunk.AllowOverrideList/server/core.c (revision 1150484) +++ httpd-trunk.AllowOverrideList/server/core.c (working copy) @@ -236,6 +236,10 @@ conf->override_opts = new->override_opts; } + if (conf->override_list == NULL) { + conf->override_list = new->override_list; + } + if (conf->response_code_strings == NULL) { conf->response_code_strings = new->response_code_strings; } @@ -1608,6 +1612,35 @@ return NULL; } +static const char *set_override_list(cmd_parms *cmd, void *d_, int argc, char *const argv[]) +{ + core_dir_config *d = d_; + int i; + + /* Throw a warning if we're in or */ + if (ap_check_cmd_context(cmd, NOT_IN_LOCATION | NOT_IN_FILES)) { + ap_log_error(APLOG_MARK, APLOG_WARNING, 0, cmd->server, + "Useless use of AllowOverrideList in line %d of %s.", + cmd->directive->line_num, cmd->directive->filename); + } + + d->override_list = apr_table_make(cmd->pool, 1); + + for (i=0;itemp_pool,argv[i]); + + if (!strcasecmp(directive, "None")) { + return NULL; + } + else { + ap_str_tolower(directive); + apr_table_set(d->override_list, directive, "1"); + } + } + + return NULL; +} + static const char *set_options(cmd_parms *cmd, void *d_, const char *l) { core_dir_config *d = d_; @@ -3742,6 +3775,9 @@ AP_INIT_RAW_ARGS("AllowOverride", set_override, NULL, ACCESS_CONF, "Controls what groups of directives can be configured by per-directory " "config files"), +AP_INIT_TAKE_ARGV("AllowOverrideList", set_override_list, NULL, ACCESS_CONF, + "Controls what individual directives can be configured by per-directory " + "config files"), AP_INIT_RAW_ARGS("Options", set_options, NULL, OR_OPTIONS, "Set a number of attributes for a given directory"), AP_INIT_TAKE1("DefaultType", set_default_type, NULL, OR_FILEINFO, Index: httpd-trunk.AllowOverrideList/server/request.c =================================================================== --- httpd-trunk.AllowOverrideList/server/request.c (revision 1150484) +++ httpd-trunk.AllowOverrideList/server/request.c (working copy) @@ -486,6 +486,7 @@ allow_options_t remove; overrides_t override; overrides_t override_opts; + apr_table_t *override_list; } core_opts_t; static void core_opts_merge(const ap_conf_vector_t *sec, core_opts_t *opts) @@ -513,6 +514,11 @@ opts->override = this_dir->override; opts->override_opts = this_dir->override_opts; } + + if (this_dir->override_list != NULL) { + opts->override_list = this_dir->override_list; + } + } @@ -740,6 +746,7 @@ opts.remove = this_dir->opts_remove; opts.override = this_dir->override; opts.override_opts = this_dir->override_opts; + opts.override_list = this_dir->override_list; /* Set aside path_info to merge back onto path_info later. * If r->filename is a directory, we must remerge the path_info, @@ -946,12 +953,13 @@ /* No htaccess in an incomplete root path, * nor if it's disabled */ - if (seg < startseg || !opts.override) { + if (seg < startseg || (!opts.override && opts.override_list == NULL)) { break; } + res = ap_parse_htaccess(&htaccess_conf, r, opts.override, - opts.override_opts, + opts.override_opts, opts.override_list, apr_pstrdup(r->pool, r->filename), sconf->access_name); if (res) { Index: httpd-trunk.AllowOverrideList/include/http_config.h =================================================================== --- httpd-trunk.AllowOverrideList/include/http_config.h (revision 1150484) +++ httpd-trunk.AllowOverrideList/include/http_config.h (working copy) @@ -28,6 +28,7 @@ #include "util_cfgtree.h" #include "ap_config.h" +#include "apr_tables.h" #ifdef __cplusplus extern "C" { @@ -282,6 +283,8 @@ int override; /** Which allow-override-opts bits are set */ int override_opts; + /** Table of directives allowed per AllowOverrideList */ + apr_table_t *override_list; /** Which methods are <Limit>ed */ apr_int64_t limited; /** methods which are limited */ @@ -1065,6 +1068,7 @@ * @param r The request currently being served * @param override Which overrides are active * @param override_opts Which allow-override-opts bits are set + * @param override_list Table of directives allowed for override * @param path The path to the htaccess file * @param access_name The list of possible names for .htaccess files * int The status of the current request @@ -1073,6 +1077,7 @@ request_rec *r, int override, int override_opts, + apr_table_t *override_list, const char *path, const char *access_name); Index: httpd-trunk.AllowOverrideList/include/ap_mmn.h =================================================================== --- httpd-trunk.AllowOverrideList/include/ap_mmn.h (revision 1150484) +++ httpd-trunk.AllowOverrideList/include/ap_mmn.h (working copy) @@ -342,12 +342,15 @@ * rename AP_EXPR_FLAGS_* -> AP_EXPR_FLAG_* * 20110702.1 (2.3.14-dev) Add ap_scan_script_header_err*_ex functions * 20110723.0 (2.3.14-dev) Revert addition of ap_ldap* + * 20110724.0 (2.3.14-dev) Add override_list as parameter to ap_parse_htaccess + * Add member override_list to cmd_parms_struct, + * core_dir_config and htaccess_result */ #define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */ #ifndef MODULE_MAGIC_NUMBER_MAJOR -#define MODULE_MAGIC_NUMBER_MAJOR 20110723 +#define MODULE_MAGIC_NUMBER_MAJOR 20110724 #endif #define MODULE_MAGIC_NUMBER_MINOR 0 /* 0...n */ Index: httpd-trunk.AllowOverrideList/include/httpd.h =================================================================== --- httpd-trunk.AllowOverrideList/include/httpd.h (revision 1150484) +++ httpd-trunk.AllowOverrideList/include/httpd.h (working copy) @@ -706,6 +706,8 @@ int override; /** the override options allowed for the .htaccess file */ int override_opts; + /** Table of allowed directives for override */ + apr_table_t *override_list; /** the configuration directives */ struct ap_conf_vector_t *htaccess; /** the next one, or NULL if no more; N.B. never change this */ Index: httpd-trunk.AllowOverrideList/include/http_core.h =================================================================== --- httpd-trunk.AllowOverrideList/include/http_core.h (revision 1150484) +++ httpd-trunk.AllowOverrideList/include/http_core.h (working copy) @@ -31,6 +31,7 @@ #include "apr_optional.h" #include "util_filter.h" #include "ap_expr.h" +#include "apr_tables.h" #include "http_config.h" @@ -601,6 +602,9 @@ /** per-dir log config */ struct ap_logconf *log; + /** Table of directives allowed per AllowOverrideList */ + apr_table_t *override_list; + } core_dir_config; /* macro to implement off by default behaviour */ --------------090600070700090707050609--