Re: svn commit: r1910662 - /httpd/httpd/trunk/modules/mappers/mod_rewrite.c
On 6/28/23 3:55 PM, Eric Covener wrote:
> On Wed, Jun 28, 2023 at 9:08 AM Ruediger Pluem wrote:
>>
>>
>>
>> On 6/28/23 2:37 PM, cove...@apache.org wrote:
>>> Author: covener
>>> Date: Wed Jun 28 12:37:50 2023
>>> New Revision: 1910662
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1910662=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=1910661=1910662=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
>>
>> > substitution>+ "?&&" + 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
On Wed, Jun 28, 2023 at 9:08 AM Ruediger Pluem wrote:
>
>
>
> On 6/28/23 2:37 PM, cove...@apache.org wrote:
> > Author: covener
> > Date: Wed Jun 28 12:37:50 2023
> > New Revision: 1910662
> >
> > URL: http://svn.apache.org/viewvc?rev=1910662=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=1910661=1910662=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
>
> substitution>+ "?&&" + 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
cove...@gmail.com
Re: svn commit: r1910662 - /httpd/httpd/trunk/modules/mappers/mod_rewrite.c
On 6/28/23 2:37 PM, cove...@apache.org wrote:
> Author: covener
> Date: Wed Jun 28 12:37:50 2023
> New Revision: 1910662
>
> URL: http://svn.apache.org/viewvc?rev=1910662=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=1910661=1910662=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
+ "?&&" + 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
