Re: svn commit: r1873762 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_rewrite.xml docs/manual/rewrite/flags.xml modules/mappers/mod_rewrite.c

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1873762 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_rewrite.xml docs/manual/rewrite/flags.xml modules/mappers/mod_rewrite.c

Ruediger Pluem


On 2/8/20 2:14 AM, [hidden email] wrote:

> Author: covener
> Date: Sat Feb  8 01:14:28 2020
> New Revision: 1873762
>
> URL: http://svn.apache.org/viewvc?rev=1873762&view=rev
> Log:
> add SameSite to RewriteRule ... ... [CO]
>
>
> Modified:
>     httpd/httpd/trunk/CHANGES
>     httpd/httpd/trunk/docs/manual/mod/mod_rewrite.xml
>     httpd/httpd/trunk/docs/manual/rewrite/flags.xml
>     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=1873762&r1=1873761&r2=1873762&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
> +++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Sat Feb  8 01:14:28 2020

> @@ -2654,6 +2656,11 @@ static void add_cookie(request_rec *r, c
>                                    "; HttpOnly" : NULL,
>                                   NULL);
>  
> +            if (samesite && !strcasecmp(samesite, "0")) {

Doesn't it need to be strcmp(samesite, "0") instead of !strcasecmp(samesite, "0") ?
I mean the above will set samesite to '0' in the cookie if samesite is '0'.

> +                cookie = apr_pstrcat(rmain->pool, cookie, "; SameSite=",
> +                                     samesite, NULL);
> +            }
> +

Any particular reason why we don't accept 'false' in a case insensitive way along with 0 as the flag
not being set? This would be inline with the other flags.

I think the second apr_pstrcat can waste some memory as we nearly need the memory for the cookie twice in case samesite is set.
Is it worth converting both apr_pstrcat sections to fill an iovec struct and doing one apr_pstrcatv afterwards?

Regards

RĂ¼diger
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1873762 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_rewrite.xml docs/manual/rewrite/flags.xml modules/mappers/mod_rewrite.c

Eric Covener
On Wed, Aug 26, 2020 at 4:16 AM Ruediger Pluem <[hidden email]> wrote:

>
>
>
> On 2/8/20 2:14 AM, [hidden email] wrote:
> > Author: covener
> > Date: Sat Feb  8 01:14:28 2020
> > New Revision: 1873762
> >
> > URL: http://svn.apache.org/viewvc?rev=1873762&view=rev
> > Log:
> > add SameSite to RewriteRule ... ... [CO]
> >
> >
> > Modified:
> >     httpd/httpd/trunk/CHANGES
> >     httpd/httpd/trunk/docs/manual/mod/mod_rewrite.xml
> >     httpd/httpd/trunk/docs/manual/rewrite/flags.xml
> >     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=1873762&r1=1873761&r2=1873762&view=diff
> > ==============================================================================
> > --- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
> > +++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Sat Feb  8 01:14:28 2020
>
> > @@ -2654,6 +2656,11 @@ static void add_cookie(request_rec *r, c
> >                                    "; HttpOnly" : NULL,
> >                                   NULL);
> >
> > +            if (samesite && !strcasecmp(samesite, "0")) {
>
> Doesn't it need to be strcmp(samesite, "0") instead of !strcasecmp(samesite, "0") ?
> I mean the above will set samesite to '0' in the cookie if samesite is '0'.

Yep, I see what you mean.

>
> > +                cookie = apr_pstrcat(rmain->pool, cookie, "; SameSite=",
> > +                                     samesite, NULL);
> > +            }
> > +
>
> Any particular reason why we don't accept 'false' in a case insensitive way along with 0 as the flag
> not being set? This would be inline with the other flags.

It is certainly weird but I have no memory of it.    Added "false" w/
a test in 1881263/1881264

Thanks!

>
> I think the second apr_pstrcat can waste some memory as we nearly need the memory for the cookie twice in case samesite is set.
> Is it worth converting both apr_pstrcat sections to fill an iovec struct and doing one apr_pstrcatv afterwards?
>
> Regards
>
> RĂ¼diger



--
Eric Covener
[hidden email]