On 6/28/23 3:55 PM, Eric Covener wrote:
> On Wed, Jun 28, 2023 at 9:08 AM Ruediger Pluem <rpl...@apache.org> 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&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
