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
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 @@
<p>common structures for heartbeat modules (should this be public API?)</p>
</section>
+ <section id="ap_parse_htaccess">
+ <title>ap_parse_htaccess (changed)</title>
+ <p>The function signature for <code>ap_parse_htaccess</code> has been
+ changed. A <code>apr_table_t</code> of individual directives allowed
+ for override must now be passed (override remains).</p>
+ </section>
+
<section id="http_config">
<title>http_config (changed)</title>
<ul>
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 @@
<directive type="section" module="core">Files</directive> sections.
</note>
- <p>When this directive is set to <code>None</code>, then
- <a href="#accessfilename">.htaccess</a> files are completely ignored.
- In this case, the server will not even attempt to read
- <code>.htaccess</code> files in the filesystem.</p>
+ <p>When this directive is set to <code>None</code> and <directive
+ module="core">AllowOverrideList</directive> is set to
+ <code>None</code> <a href="#accessfilename">.htaccess</a> files are
+ completely ignored. In this case, the server will not even attempt
+ to read <code>.htaccess</code> files in the filesystem.</p>
<p>When this directive is set to <code>All</code>, then any
directive which has the .htaccess <a
@@ -442,7 +443,63 @@
</note>
</usage>
+<directivesynopsis>
+<name>AllowOverrideList</name>
+<description>Individual directives that are allowed in
+<code>.htaccess</code> files</description>
+<syntax>AllowOverrideList None|<var>directive</var>
+[<var>directive-type</var>] ...</syntax>
+<default>AllowOverrideList None</default>
+<contextlist><context>directory</context></contextlist>
+
+<usage>
+ <p>When the server finds an <code>.htaccess</code> file (as
+ specified by <directive module="core">AccessFileName</directive>)
+ it needs to know which directives declared in that file can override
+ earlier configuration directives.</p>
+
+ <note><title>Only available in <Directory> sections</title>
+ <directive>AllowOverrideList</directive> is valid only in
+ <directive type="section" module="core">Directory</directive>
+ sections specified without regular expressions, not in <directive
+ type="section" module="core">Location</directive>, <directive
+ module="core" type="section">DirectoryMatch</directive> or
+ <directive type="section" module="core">Files</directive> sections.
+ </note>
+
+ <p>When this directive is set to <code>None</code> and <directive
+ module="core">AllowOverride</directive> is set to <code>None</code>,
+ then <a href="#accessfilename">.htaccess</a> files are completely
+ ignored. In this case, the server will not even attempt to read
+ <code>.htaccess</code> files in the filesystem.</p>
+
+ <p>Example:</p>
+
+ <example>
+ AllowOverride None
+ AllowOverrideList Redirect RedirectMatch
+ </example>
+
+ <p>In the example above only the <code>Redirect</code> and
+ <code>RedirectMatch</code> directives are allowed. All others will
+ cause an internal server error.</p>
+
+ <p>Example:</p>
+
+ <example>
+ AllowOverride AuthConfig
+ AllowOverrideList CookieTracking CookieName
+ </example>
+
+ <p>In the example above <directive module="core">AllowOverride
+ </directive> grants permission to the <code>AuthConfig</code>
+ directive grouping and <directive>AllowOverrideList</directive> grants
+ permission to only two directves from the <code>FileInfo</code> directive
+ grouping. All others will cause an internal server error.</p>
+</usage>
+
<seealso><directive module="core">AccessFileName</directive></seealso>
+<seealso><directive module="core">AllowOverride</directive></seealso>
<seealso><a href="../configuring.html">Configuration Files</a></seealso>
<seealso><a href="../howto/htaccess.html">.htaccess Files</a></seealso>
</directivesynopsis>
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 <Location> or <Files> */
+ 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;i<argc;i++){
+ char *directive=apr_pstrdup(cmd->temp_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 */
