You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by mi...@apache.org on 2010/11/08 01:41:36 UTC
svn commit: r1032431 - in /httpd/httpd/trunk: CHANGES
modules/mappers/mod_rewrite.c
Author: minfrin
Date: Mon Nov 8 00:41:35 2010
New Revision: 1032431
URL: http://svn.apache.org/viewvc?rev=1032431&view=rev
Log:
mod_rewrite: Fix the RewriteEngine directive to work within a
location. Previously, once RewriteEngine was switched on globally,
it was impossible to switch off.
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/mappers/mod_rewrite.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1032431&r1=1032430&r2=1032431&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Mon Nov 8 00:41:35 2010
@@ -6,6 +6,10 @@ Changes with Apache 2.3.9
Fix a denial of service attack against mod_reqtimeout.
[Stefan Fritsch]
+ *) mod_rewrite: Fix the RewriteEngine directive to work within a
+ location. Previously, once RewriteEngine was switched on globally,
+ it was impossible to switch off. [Graham Leggett]
+
*) core, mod_include, mod_ssl: Move the expression parser derived from
mod_include back into mod_include. Replace ap_expr with a parser
derived from mod_ssl's parser. Make mod_ssl use the new parser. Rework
Modified: httpd/httpd/trunk/modules/mappers/mod_rewrite.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/mappers/mod_rewrite.c?rev=1032431&r1=1032430&r2=1032431&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
+++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Mon Nov 8 00:41:35 2010
@@ -310,6 +310,8 @@ typedef struct {
apr_array_header_t *rewriteconds; /* the RewriteCond entries (temp.) */
apr_array_header_t *rewriterules; /* the RewriteRule entries */
server_rec *server; /* the corresponding server indicator */
+ int state_set:1;
+ int options_set:1;
} rewrite_server_conf;
typedef struct {
@@ -319,6 +321,9 @@ typedef struct {
apr_array_header_t *rewriterules; /* the RewriteRule entries */
char *directory; /* the directory where it applies */
const char *baseurl; /* the base-URL where it applies */
+ int state_set:1;
+ int options_set:1;
+ int baseurl_set:1;
} rewrite_perdir_conf;
/* the (per-child) cache structures.
@@ -2721,8 +2726,11 @@ static void *config_server_merge(apr_poo
base = (rewrite_server_conf *)basev;
overrides = (rewrite_server_conf *)overridesv;
- a->state = overrides->state;
- a->options = overrides->options;
+ a->state = (overrides->state_set == 0) ? base->state : overrides->state;
+ a->state_set = overrides->state_set || base->state_set;
+ a->options = (overrides->options_set == 0) ? base->options : overrides->options;
+ a->options_set = overrides->options_set || base->options_set;
+
a->server = overrides->server;
if (a->options & OPTION_INHERIT) {
@@ -2787,10 +2795,14 @@ static void *config_perdir_merge(apr_poo
base = (rewrite_perdir_conf *)basev;
overrides = (rewrite_perdir_conf *)overridesv;
- a->state = overrides->state;
- a->options = overrides->options;
- a->directory = overrides->directory;
- a->baseurl = overrides->baseurl;
+ a->state = (overrides->state_set == 0) ? base->state : overrides->state;
+ a->state_set = overrides->state_set || base->state_set;
+ a->options = (overrides->options_set == 0) ? base->options : overrides->options;
+ a->options_set = overrides->options_set || base->options_set;
+ a->baseurl = (overrides->baseurl_set == 0) ? base->baseurl : overrides->baseurl;
+ a->baseurl_set = overrides->baseurl_set || base->baseurl_set;
+
+ a->directory = overrides->directory;
if (a->options & OPTION_INHERIT) {
a->rewriteconds = apr_array_append(p, overrides->rewriteconds,
@@ -2814,11 +2826,17 @@ static const char *cmd_rewriteengine(cmd
sconf = ap_get_module_config(cmd->server->module_config, &rewrite_module);
- if (cmd->path == NULL) { /* is server command */
+ /* server command? set both global scope and base directory scope */
+ if (cmd->path == NULL) {
sconf->state = (flag ? ENGINE_ENABLED : ENGINE_DISABLED);
+ sconf->state_set = 1;
+ dconf->state = sconf->state;
+ dconf->state_set = 1;
}
- else /* is per-directory command */ {
+ /* directory command? set directory scope only */
+ else {
dconf->state = (flag ? ENGINE_ENABLED : ENGINE_DISABLED);
+ dconf->state_set = 1;
}
return NULL;
@@ -2848,18 +2866,24 @@ static const char *cmd_rewriteoptions(cm
}
}
- /* put it into the appropriate config */
+ /* server command? set both global scope and base directory scope */
if (cmd->path == NULL) { /* is server command */
- rewrite_server_conf *conf =
+ rewrite_perdir_conf *dconf = in_dconf;
+ rewrite_server_conf *sconf =
ap_get_module_config(cmd->server->module_config,
&rewrite_module);
- conf->options |= options;
+ sconf->options |= options;
+ sconf->options_set = 1;
+ dconf->options |= options;
+ dconf->options_set = 1;
}
+ /* directory command? set directory scope only */
else { /* is per-directory command */
- rewrite_perdir_conf *conf = in_dconf;
+ rewrite_perdir_conf *dconf = in_dconf;
- conf->options |= options;
+ dconf->options |= options;
+ dconf->options_set = 1;
}
return NULL;
@@ -3037,6 +3061,7 @@ static const char *cmd_rewritebase(cmd_p
}
dconf->baseurl = a1;
+ dconf->baseurl_set = 1;
return NULL;
}
@@ -4295,6 +4320,7 @@ static void init_child(apr_pool_t *p, se
*/
static int hook_uri2file(request_rec *r)
{
+ rewrite_perdir_conf *dconf;
rewrite_server_conf *conf;
const char *saved_rulestatus;
const char *var;
@@ -4310,11 +4336,14 @@ static int hook_uri2file(request_rec *r)
*/
conf = ap_get_module_config(r->server->module_config, &rewrite_module);
+ dconf = (rewrite_perdir_conf *)ap_get_module_config(r->per_dir_config,
+ &rewrite_module);
+
/*
* only do something under runtime if the engine is really enabled,
* else return immediately!
*/
- if (conf->state == ENGINE_DISABLED) {
+ if (!dconf || dconf->state == ENGINE_DISABLED) {
return DECLINED;
}
@@ -4627,7 +4656,7 @@ static int hook_fixup(request_rec *r)
* only do something under runtime if the engine is really enabled,
* for this directory, else return immediately!
*/
- if (dconf->state == ENGINE_DISABLED) {
+ if (!dconf || dconf->state == ENGINE_DISABLED) {
return DECLINED;
}
Re: svn commit: r1032431 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c
Posted by Eric Covener <co...@gmail.com>.
On Thu, Oct 4, 2012 at 7:18 PM, Graham Leggett <mi...@sharp.fm> wrote:
> On 04 Oct 2012, at 9:18 PM, André Malo <nd...@perlig.de> wrote:
>
>> Hmm. The very sole concept of RewriteBase is "base for this location and
>> this location only".
>
> Exactly, which is why merging is the only sensible behaviour. "/foo/bar" is below location "/foo" at all times, regardless of whether someone has added <Location /foo/bar> to their config or not. Until someone has explicitly stated otherwise, /foo/bar should have the same RewriteBase as /foo.
No, it's not sensible. The default base URL has always been relative
to the context the rules appear in and can be overridden by the
directive. The directive should not always be set and should not be
copied from higher scopes. The copying breaks that even for containers
of Location /foo/ and Location /foo/bar/, much less traditional
Directory and htacess where RewtiteBase is generally more useful.
> Not merging properly means that the addition of a completely unrelated directive such as RewriteRule to a Location that previously had only inherited RewriteRules suddenly makes the RewriteBase vanish completely without warning.
With this particular directive, no RewriteBase is better than a bad
guess, because the default prefix of relative substitution tracks to
the context the rule is defined in.
Re: svn commit: r1032431 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 04 Oct 2012, at 9:18 PM, André Malo <nd...@perlig.de> wrote:
> Hmm. The very sole concept of RewriteBase is "base for this location and
> this location only".
Exactly, which is why merging is the only sensible behaviour. "/foo/bar" is below location "/foo" at all times, regardless of whether someone has added <Location /foo/bar> to their config or not. Until someone has explicitly stated otherwise, /foo/bar should have the same RewriteBase as /foo.
Not merging properly means that the addition of a completely unrelated directive such as RewriteRule to a Location that previously had only inherited RewriteRules suddenly makes the RewriteBase vanish completely without warning.
We should not try and second guess what the user is trying to achieve. Have a sensible consistent behaviour and let the end user make up their own mind as to how to configure it.
> So merging is most certainly a nogo. It *will* break
> stuff. mod_rewrite already has enough bad guessings built in ;)
The idea is to take out the bad guessing, not maintain it.
Regards,
Graham
--
Re: svn commit: r1032431 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c
Posted by André Malo <nd...@perlig.de>.
* Eric Covener wrote:
> On Thu, Oct 4, 2012 at 2:05 PM, Graham Leggett <mi...@sharp.fm> wrote:
> > On 4 Oct 2012, at 17:32, Eric Covener <co...@gmail.com> wrote:
> >>> URL: http://svn.apache.org/viewvc?rev=1032431&view=rev
> >>> Log:
> >>> mod_rewrite: Fix the RewriteEngine directive to work within a
> >>> location. Previously, once RewriteEngine was switched on globally,
> >>> it was impossible to switch off.
> >>>
> >>> + a->baseurl = (overrides->baseurl_set == 0) ? base->baseurl :
> >>> overrides->baseurl; + a->baseurl_set = overrides->baseurl_set ||
> >>> base->baseurl_set;
> >>
> >> PR https://issues.apache.org/bugzilla/show_bug.cgi?id=53963 points out
> >> that mergeing the base is probably not very useful and actually harms
> >> configs where no rewritebase is required in some subdirs but present
> >> in parent dirs.
> >>
> >> Any issue with dropping this part of the change?
> >
> > Having an arbitrary inconsistent behaviour from a directive in a module
> > already noted for its complexity is a really bad thing. What we should
> > support instead is the ability to turn RewriteBase off if needed, with
> > something like "RewriteBase off".
>
> "Rewritebase off" to undo the copy of base URL from the parent to the
> subdir seems wrong. I'll try to merge in a value less likely to be
> wrong.
Hmm. The very sole concept of RewriteBase is "base for this location and
this location only". So merging is most certainly a nogo. It *will* break
stuff. mod_rewrite already has enough bad guessings built in ;)
nd
--
Da fällt mir ein, wieso gibt es eigentlich in Unicode kein
"i" mit einem Herzchen als Tüpfelchen? Das wär sooo süüss!
-- Björn Höhrmann in darw
Re: svn commit: r1032431 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c
Posted by Eric Covener <co...@gmail.com>.
On Thu, Oct 4, 2012 at 2:05 PM, Graham Leggett <mi...@sharp.fm> wrote:
> On 4 Oct 2012, at 17:32, Eric Covener <co...@gmail.com> wrote:
>
>>> URL: http://svn.apache.org/viewvc?rev=1032431&view=rev
>>> Log:
>>> mod_rewrite: Fix the RewriteEngine directive to work within a
>>> location. Previously, once RewriteEngine was switched on globally,
>>> it was impossible to switch off.
>>>
>>> + a->baseurl = (overrides->baseurl_set == 0) ? base->baseurl : overrides->baseurl;
>>> + a->baseurl_set = overrides->baseurl_set || base->baseurl_set;
>>
>>
>> PR https://issues.apache.org/bugzilla/show_bug.cgi?id=53963 points out
>> that mergeing the base is probably not very useful and actually harms
>> configs where no rewritebase is required in some subdirs but present
>> in parent dirs.
>>
>> Any issue with dropping this part of the change?
>
> Having an arbitrary inconsistent behaviour from a directive in a module already noted for its complexity is a really bad thing. What we should support instead is the ability to turn RewriteBase off if needed, with something like "RewriteBase off".
"Rewritebase off" to undo the copy of base URL from the parent to the
subdir seems wrong. I'll try to merge in a value less likely to be
wrong.
Re: svn commit: r1032431 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 4 Oct 2012, at 17:32, Eric Covener <co...@gmail.com> wrote:
>> URL: http://svn.apache.org/viewvc?rev=1032431&view=rev
>> Log:
>> mod_rewrite: Fix the RewriteEngine directive to work within a
>> location. Previously, once RewriteEngine was switched on globally,
>> it was impossible to switch off.
>>
>> + a->baseurl = (overrides->baseurl_set == 0) ? base->baseurl : overrides->baseurl;
>> + a->baseurl_set = overrides->baseurl_set || base->baseurl_set;
>
>
> PR https://issues.apache.org/bugzilla/show_bug.cgi?id=53963 points out
> that mergeing the base is probably not very useful and actually harms
> configs where no rewritebase is required in some subdirs but present
> in parent dirs.
>
> Any issue with dropping this part of the change?
Having an arbitrary inconsistent behaviour from a directive in a module already noted for its complexity is a really bad thing. What we should support instead is the ability to turn RewriteBase off if needed, with something like "RewriteBase off".
Regards,
Graham
--
Re: svn commit: r1032431 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c
Posted by Eric Covener <co...@gmail.com>.
On Sun, Nov 7, 2010 at 7:41 PM, <mi...@apache.org> wrote:
> Author: minfrin
> Date: Mon Nov 8 00:41:35 2010
> New Revision: 1032431
>
> URL: http://svn.apache.org/viewvc?rev=1032431&view=rev
> Log:
> mod_rewrite: Fix the RewriteEngine directive to work within a
> location. Previously, once RewriteEngine was switched on globally,
> it was impossible to switch off.
>
> + a->baseurl = (overrides->baseurl_set == 0) ? base->baseurl : overrides->baseurl;
> + a->baseurl_set = overrides->baseurl_set || base->baseurl_set;
PR https://issues.apache.org/bugzilla/show_bug.cgi?id=53963 points out
that mergeing the base is probably not very useful and actually harms
configs where no rewritebase is required in some subdirs but present
in parent dirs.
Any issue with dropping this part of the change?
(rewritebase is needed when a relative substitution is made and the
current URL and the current URL was reached via e.g. alias and not
under the documentroot)