You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by co...@apache.org on 2023/06/28 12:37:50 UTC
svn commit: r1910662 - /httpd/httpd/trunk/modules/mappers/mod_rewrite.c
Author: covener
Date: Wed Jun 28 12:37:50 2023
New Revision: 1910662
URL: http://svn.apache.org/viewvc?rev=1910662&view=rev
Log:
back to original patch in PR66672
Since QSNONE will skip splitoutqueryargs, it's where we should fixup the trailing ?
Modified:
httpd/httpd/trunk/modules/mappers/mod_rewrite.c
Modified: httpd/httpd/trunk/modules/mappers/mod_rewrite.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/mappers/mod_rewrite.c?rev=1910662&r1=1910661&r2=1910662&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
+++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Wed Jun 28 12:37:50 2023
@@ -3909,10 +3909,12 @@ static const char *cmd_rewriterule(cmd_p
}
if (*(a2_end-1) == '?') {
- *(a2_end-1) = '\0'; /* trailing ? has done its job */
/* a literal ? at the end of the unsubstituted rewrite rule */
- if (!(newrule->flags & RULEFLAG_QSAPPEND))
- {
+ if (!(newrule->flags & RULEFLAG_QSAPPEND)) {
+ /* trailing ? has done its job. with QSA, splitoutqueryargs
+ * will handle it
+ */
+ *(a2_end-1) = '\0';
newrule->flags |= RULEFLAG_QSNONE;
}
}
Re: svn commit: r1910662 - /httpd/httpd/trunk/modules/mappers/mod_rewrite.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 6/28/23 3:55 PM, Eric Covener wrote:
> On Wed, Jun 28, 2023 at 9:08 AM Ruediger Pluem <rp...@apache.org> wrote:
>>
>>
>>
>> On 6/28/23 2:37 PM, covener@apache.org wrote:
>>> Author: covener
>>> Date: Wed Jun 28 12:37:50 2023
>>> New Revision: 1910662
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1910662&view=rev
>>> Log:
>>> back to original patch in PR66672
>>>
>>> Since QSNONE will skip splitoutqueryargs, it's where we should fixup the trailing ?
>>>
>>> Modified:
>>> httpd/httpd/trunk/modules/mappers/mod_rewrite.c
>>>
>>> Modified: httpd/httpd/trunk/modules/mappers/mod_rewrite.c
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/mappers/mod_rewrite.c?rev=1910662&r1=1910661&r2=1910662&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
>>> +++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Wed Jun 28 12:37:50 2023
>>> @@ -3909,10 +3909,12 @@ static const char *cmd_rewriterule(cmd_p
>>> }
>>>
>>> if (*(a2_end-1) == '?') {
>>> - *(a2_end-1) = '\0'; /* trailing ? has done its job */
>>> /* a literal ? at the end of the unsubstituted rewrite rule */
>>> - if (!(newrule->flags & RULEFLAG_QSAPPEND))
>>> - {
>>> + if (!(newrule->flags & RULEFLAG_QSAPPEND)) {
>>> + /* trailing ? has done its job. with QSA, splitoutqueryargs
>>> + * will handle it
>>
>> I think only if QSLAST is set. Otherwise substitutions or the matching URL may place a '?' left of the terminating '?' that came
>> in as encoded '?' and thus would add the original querystring as "&&" + r->args. Hence we would end up with
>>
>> <one character after some decoded '?' somewhere in the URL or after substitution><further chars>+ "?&&" + r->args.
>>
>> Hence I think it should be:
>>
>> if (!(newrule->flags & RULEFLAG_QSAPPEND)) {
>> /* trailing ? has done its job.
>> */
>> *(a2_end-1) = '\0';
>> newrule->flags |= RULEFLAG_QSNONE;
>> }
>> else {
>> /* with QSA, splitoutqueryargs will handle it if RULEFLAG_QSLAST is set.
>> newrule->flags |= RULEFLAG_QSLAST;
>> }
>>
>
> flipping it around and refreshing some comments:
>
> if (*(a2_end-1) == '?') {
> /* a literal ? at the end of the unsubstituted rewrite rule */
> if (newrule->flags & RULEFLAG_QSAPPEND) {
> /* with QSA, splitoutqueryargs will safely handle it if
> RULEFLAG_QSLAST is set */
> newrule->flags |= RULEFLAG_QSLAST;
> }
> else {
> /* avoid getting a a query string via inadvertent capture */
> newrule->flags |= RULEFLAG_QSNONE;
> /* trailing ? has done its job, but splitoutqueryargs will
> not chop it off */
> *(a2_end-1) = '\0';
> }
> }
> else if (newrule->flags & RULEFLAG_QSDISCARD) {
> if (NULL == ap_strchr(newrule->output, '?')) {
> newrule->flags |= RULEFLAG_QSNONE;
> }
> }
>
> close?
>
Looks fine.
Regards
Rüdiger
Re: svn commit: r1910662 - /httpd/httpd/trunk/modules/mappers/mod_rewrite.c
Posted by Eric Covener <co...@gmail.com>.
On Wed, Jun 28, 2023 at 9:08 AM Ruediger Pluem <rp...@apache.org> wrote:
>
>
>
> On 6/28/23 2:37 PM, covener@apache.org wrote:
> > Author: covener
> > Date: Wed Jun 28 12:37:50 2023
> > New Revision: 1910662
> >
> > URL: http://svn.apache.org/viewvc?rev=1910662&view=rev
> > Log:
> > back to original patch in PR66672
> >
> > Since QSNONE will skip splitoutqueryargs, it's where we should fixup the trailing ?
> >
> > Modified:
> > httpd/httpd/trunk/modules/mappers/mod_rewrite.c
> >
> > Modified: httpd/httpd/trunk/modules/mappers/mod_rewrite.c
> > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/mappers/mod_rewrite.c?rev=1910662&r1=1910661&r2=1910662&view=diff
> > ==============================================================================
> > --- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
> > +++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Wed Jun 28 12:37:50 2023
> > @@ -3909,10 +3909,12 @@ static const char *cmd_rewriterule(cmd_p
> > }
> >
> > if (*(a2_end-1) == '?') {
> > - *(a2_end-1) = '\0'; /* trailing ? has done its job */
> > /* a literal ? at the end of the unsubstituted rewrite rule */
> > - if (!(newrule->flags & RULEFLAG_QSAPPEND))
> > - {
> > + if (!(newrule->flags & RULEFLAG_QSAPPEND)) {
> > + /* trailing ? has done its job. with QSA, splitoutqueryargs
> > + * will handle it
>
> I think only if QSLAST is set. Otherwise substitutions or the matching URL may place a '?' left of the terminating '?' that came
> in as encoded '?' and thus would add the original querystring as "&&" + r->args. Hence we would end up with
>
> <one character after some decoded '?' somewhere in the URL or after substitution><further chars>+ "?&&" + r->args.
>
> Hence I think it should be:
>
> if (!(newrule->flags & RULEFLAG_QSAPPEND)) {
> /* trailing ? has done its job.
> */
> *(a2_end-1) = '\0';
> newrule->flags |= RULEFLAG_QSNONE;
> }
> else {
> /* with QSA, splitoutqueryargs will handle it if RULEFLAG_QSLAST is set.
> newrule->flags |= RULEFLAG_QSLAST;
> }
>
flipping it around and refreshing some comments:
if (*(a2_end-1) == '?') {
/* a literal ? at the end of the unsubstituted rewrite rule */
if (newrule->flags & RULEFLAG_QSAPPEND) {
/* with QSA, splitoutqueryargs will safely handle it if
RULEFLAG_QSLAST is set */
newrule->flags |= RULEFLAG_QSLAST;
}
else {
/* avoid getting a a query string via inadvertent capture */
newrule->flags |= RULEFLAG_QSNONE;
/* trailing ? has done its job, but splitoutqueryargs will
not chop it off */
*(a2_end-1) = '\0';
}
}
else if (newrule->flags & RULEFLAG_QSDISCARD) {
if (NULL == ap_strchr(newrule->output, '?')) {
newrule->flags |= RULEFLAG_QSNONE;
}
}
close?
--
Eric Covener
covener@gmail.com
Re: svn commit: r1910662 - /httpd/httpd/trunk/modules/mappers/mod_rewrite.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 6/28/23 2:37 PM, covener@apache.org wrote:
> Author: covener
> Date: Wed Jun 28 12:37:50 2023
> New Revision: 1910662
>
> URL: http://svn.apache.org/viewvc?rev=1910662&view=rev
> Log:
> back to original patch in PR66672
>
> Since QSNONE will skip splitoutqueryargs, it's where we should fixup the trailing ?
>
> Modified:
> httpd/httpd/trunk/modules/mappers/mod_rewrite.c
>
> Modified: httpd/httpd/trunk/modules/mappers/mod_rewrite.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/mappers/mod_rewrite.c?rev=1910662&r1=1910661&r2=1910662&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
> +++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Wed Jun 28 12:37:50 2023
> @@ -3909,10 +3909,12 @@ static const char *cmd_rewriterule(cmd_p
> }
>
> if (*(a2_end-1) == '?') {
> - *(a2_end-1) = '\0'; /* trailing ? has done its job */
> /* a literal ? at the end of the unsubstituted rewrite rule */
> - if (!(newrule->flags & RULEFLAG_QSAPPEND))
> - {
> + if (!(newrule->flags & RULEFLAG_QSAPPEND)) {
> + /* trailing ? has done its job. with QSA, splitoutqueryargs
> + * will handle it
I think only if QSLAST is set. Otherwise substitutions or the matching URL may place a '?' left of the terminating '?' that came
in as encoded '?' and thus would add the original querystring as "&&" + r->args. Hence we would end up with
<one character after some decoded '?' somewhere in the URL or after substitution><further chars>+ "?&&" + r->args.
Hence I think it should be:
if (!(newrule->flags & RULEFLAG_QSAPPEND)) {
/* trailing ? has done its job.
*/
*(a2_end-1) = '\0';
newrule->flags |= RULEFLAG_QSNONE;
}
else {
/* with QSA, splitoutqueryargs will handle it if RULEFLAG_QSLAST is set.
newrule->flags |= RULEFLAG_QSLAST;
}
Regards
Rüdiger