I have checked all the calls to ap_expr_parse_cmd() and it seems that the change in the parser itself is not expected/needed. Either ap_getword_conf() is implicit (parser called from an AP_TAKEn or AP_TAKE_ARGV directive), or the caller (AP_RAW_ARGS directive) uses it explicitly. Hence that would cause a double unquoting...
There are some special cases though: - ap_authz_providers as per this commit. - mod_log_config, mod_log_debug, mod_headers have the expr=... syntax which does not handle quoted strings; - mod_rewrite has a special handling of quoted strings, including RewriteCond expr ... (but it is consistent with all RewriteCond, so guess it should be kept as is); The patch proposed in [1] is I think the simplest way to handle the case for *all* the ap_authz_providers. Otherwise, we could introduce a new ap_expr_parse_cmd_conf() which would handle quoted strings and advance the word pointer to the next one (à la ap_getword_conf() where the word is an expression, either quoted or until the end of line). It would then be used by each ap_authz_provider's parser implementation, and also by mod_{log_config,log_debug,headers} above for the expr= case. Something like: Index: server/util_expr_eval.c =================================================================== --- server/util_expr_eval.c (revision 1674695) +++ server/util_expr_eval.c (working copy) @@ -443,20 +443,31 @@ AP_DECLARE(const char *) ap_expr_parse(apr_pool_t return NULL; } -AP_DECLARE(ap_expr_info_t*) ap_expr_parse_cmd_mi(const cmd_parms *cmd, - const char *expr, - unsigned int flags, - const char **err, - ap_expr_lookup_fn_t *lookup_fn, - int module_index) +AP_DECLARE(ap_expr_info_t*) ap_expr_parse_cmd_conf(const cmd_parms *cmd, + const char **line, + unsigned int flags, + const char **err, + ap_expr_lookup_fn_t *lookup_fn, + int module_index) { + const char *expr = *line; + apr_size_t len = strlen(expr); ap_expr_info_t *info = apr_pcalloc(cmd->pool, sizeof(ap_expr_info_t)); + info->filename = cmd->directive->filename; info->line_number = cmd->directive->line_num; info->flags = flags; info->module_index = module_index; + + if (len > 1 && (expr[0] == '"' || expr[0] == '\'') + && (expr[len - 1] == expr[0])) { + expr = ap_getword_conf(cmd->temp_pool, line); + } + else { + *line = expr + len; + } + *err = ap_expr_parse(cmd->pool, cmd->temp_pool, info, expr, lookup_fn); - if (*err) return NULL; @@ -463,6 +474,17 @@ AP_DECLARE(const char *) ap_expr_parse(apr_pool_t return info; } +AP_DECLARE(ap_expr_info_t*) ap_expr_parse_cmd_mi(const cmd_parms *cmd, + const char *expr, + unsigned int flags, + const char **err, + ap_expr_lookup_fn_t *lookup_fn, + int module_index) +{ + return ap_expr_parse_cmd_conf(cmd, &expr, flags, err, + lookup_fn, module_index); +} + ap_expr_t *ap_expr_make(ap_expr_node_op_e op, const void *a1, const void *a2, ap_expr_parse_ctx_t *ctx) { -- I prefer the patch from [1] though, because it is simpler and more centralized for the ap_authz_providers case. We could likewise use ap_getword_conf() explicitely for the expr= cases, but they do not look critical to me, and "expr=..." (with quotes around the whole) can still be used. The only advantage I see for the new function would be if we have more (and more) needs for it... [1] http://mail-archives.apache.org/mod_mbox/httpd-dev/201504.mbox/%3ccakq1svpnujfyccuk1qkcaqx6y50qjey-37kdqtkrruzb5hh...@mail.gmail.com%3E On Fri, Apr 17, 2015 at 10:45 PM, Christophe JAILLET <christophe.jail...@wanadoo.fr> wrote: > Hi, > > I would +1 for this solution which is, IMHO, much better. > However, changing the parser itself would require checking the potential > impact as it is used in many places. > > CJ > > > Le 17/04/2015 13:15, Yann Ylavic a écrit : >> >> On Fri, Apr 17, 2015 at 11:54 AM, Yann Ylavic <ylavic....@gmail.com> >> wrote: >>> >>> How about handling the case in the Require directive parser itself >>> (add_authz_provider)? >> >> Or even maybe in the expr parser itself: >> >> Index: server/util_expr_eval.c >> =================================================================== >> --- server/util_expr_eval.c (revision 1674046) >> +++ server/util_expr_eval.c (working copy) >> @@ -455,6 +455,10 @@ AP_DECLARE(ap_expr_info_t*) ap_expr_parse_cmd_mi(c >> info->line_number = cmd->directive->line_num; >> info->flags = flags; >> info->module_index = module_index; >> + if (expr && (expr[0] == '"' || expr[0] == '\'') && (expr[1] != '\0') >> + && (expr[strlen(expr) - 1] == expr[0])) { >> + expr = ap_getword_conf(cmd->temp_pool, &expr); >> + } >> *err = ap_expr_parse(cmd->pool, cmd->temp_pool, info, expr, >> lookup_fn); >> >> if (*err) >> -- >> >