Are there any implications about using wp_redirect( add_query_arg( '', '' ) )?

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

Are there any implications about using wp_redirect( add_query_arg( '', '' ) )?

Nikola Nikolov
Hi there,

I've found myself in situations where I want to reload the current URL with
PHP - for instance after processing a $_POST request - and in some cases I
would add a $_GET parameter to the URL(so add_query_arg() would be a
perfect fit there), but in others I don't need to do that. I found out that
I can use

wp_redirect( esc_url_raw( add_query_arg( '', '' ) ) );
exit;

Used that way add_query_arg() seems to just give me a path and any $_GET
parameters currently present(for instance I would get
/product/my-product/). I'm escaping the URL with esc_url_raw(), but wanted
to make sure there's nothing wrong with doing things that way.

Any thoughts?
_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Are there any implications about using wp_redirect( add_query_arg( '', '' ) )?

James DiGioia
add_query_args takes two or three parameters, depending on how you'd prefer
to use it. See here:
https://developer.wordpress.org/reference/functions/add_query_arg/

It's not automatically going to redirect using the current $_GET params;
you need to specify in the function args what those params are supposed to
be.

On Tue, Feb 16, 2016 at 5:23 AM, Nikola Nikolov <[hidden email]>
wrote:

> Hi there,
>
> I've found myself in situations where I want to reload the current URL with
> PHP - for instance after processing a $_POST request - and in some cases I
> would add a $_GET parameter to the URL(so add_query_arg() would be a
> perfect fit there), but in others I don't need to do that. I found out that
> I can use
>
> wp_redirect( esc_url_raw( add_query_arg( '', '' ) ) );
> exit;
>
> Used that way add_query_arg() seems to just give me a path and any $_GET
> parameters currently present(for instance I would get
> /product/my-product/). I'm escaping the URL with esc_url_raw(), but wanted
> to make sure there's nothing wrong with doing things that way.
>
> Any thoughts?
> _______________________________________________
> wp-hackers mailing list
> [hidden email]
> http://lists.automattic.com/mailman/listinfo/wp-hackers
>
_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Are there any implications about using wp_redirect( add_query_arg( '', '' ) )?

Nikola Nikolov
Well, it actually works fine with the example I gave.

Here, say I'm on http://example.com/page1/?a=b&c=d
Then I call add_query_arg( '', '' ) - omitting the last parameter, which
makes it use the current request as the base URL.

This returns something like this: /page1/?a=b&c=d&
That last & is there because I tried to add an empty parameter, so there's
no key, but there's a trailing &(which should not be a problem as far as
I'm aware).

My question wasn't about how to use add_query_arg() as much as it was about
whether there could be any issues by passing two empty parameters to the
function, since it technically expects those to be strings.

On Tue, 16 Feb 2016 at 14:57 James DiGioia <[hidden email]> wrote:

> add_query_args takes two or three parameters, depending on how you'd prefer
> to use it. See here:
> https://developer.wordpress.org/reference/functions/add_query_arg/
>
> It's not automatically going to redirect using the current $_GET params;
> you need to specify in the function args what those params are supposed to
> be.
>
> On Tue, Feb 16, 2016 at 5:23 AM, Nikola Nikolov <[hidden email]>
> wrote:
>
> > Hi there,
> >
> > I've found myself in situations where I want to reload the current URL
> with
> > PHP - for instance after processing a $_POST request - and in some cases
> I
> > would add a $_GET parameter to the URL(so add_query_arg() would be a
> > perfect fit there), but in others I don't need to do that. I found out
> that
> > I can use
> >
> > wp_redirect( esc_url_raw( add_query_arg( '', '' ) ) );
> > exit;
> >
> > Used that way add_query_arg() seems to just give me a path and any $_GET
> > parameters currently present(for instance I would get
> > /product/my-product/). I'm escaping the URL with esc_url_raw(), but
> wanted
> > to make sure there's nothing wrong with doing things that way.
> >
> > Any thoughts?
> > _______________________________________________
> > wp-hackers mailing list
> > [hidden email]
> > http://lists.automattic.com/mailman/listinfo/wp-hackers
> >
> _______________________________________________
> wp-hackers mailing list
> [hidden email]
> http://lists.automattic.com/mailman/listinfo/wp-hackers
>
_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Are there any implications about using wp_redirect( add_query_arg( '', '' ) )?

J.D. Grimes
You are right to question whether there might be some issues with this. There is in fact a potential for security issues when using wp_redirect(), but it has nothing to do with escaping the URL. I think it's not actually necessary to use esc_url_raw(), since wp_redirect() uses wp_sanitize_redirect() to sanitize the URL (https://developer.wordpress.org/reference/functions/wp_sanitize_redirect/ <https://developer.wordpress.org/reference/functions/wp_sanitize_redirect/>).

The issues that you would run into with wp_redirect() would be an open redirect (https://www.owasp.org/index.php/Open_redirect <https://www.owasp.org/index.php/Open_redirect>). To avoid that, you can use wp_safe_redirect() (https://developer.wordpress.org/reference/functions/wp_safe_redirect/ <https://developer.wordpress.org/reference/functions/wp_safe_redirect/>) which ensures that the redirect will only go to a host that is on the whitelist. I always use wp_safe_redirect(), just to be sure that the redirect is secure.

As for this case when using add_query_arg(), it shouldn't technically be necessary to use wp_safe_redirect(), since add_query_arg() uses the REQUEST_URI server var, which should be secure. There is always the possibility of course that some other code has fiddled with the REQUEST_URI, and in that case it may not actually end up pointing to the correct host like it should. So that's why I'd recommend that you use wp_safe_redirect(), since you know that you will always be wanting to redirect back to the same site. wp_redirect() should only be used when you specifically want to redirect to a different host, and then you should be hard-coding the URL.

As far as passing empty values to add_query_arg(), that is perfectly valid, although you might as well use $_SERVER['REQUEST_URI'] directly, instead of doing extra, unnecessary processing on the URL. There is also the wp_get_referer() function (https://developer.wordpress.org/reference/functions/wp_get_referer/ <https://developer.wordpress.org/reference/functions/wp_get_referer/>) that you could use if you want to accept more ways of determining the referrer.

-J.D.

> On Feb 16, 2016, at 8:17 AM, Nikola Nikolov <[hidden email]> wrote:
>
> Well, it actually works fine with the example I gave.
>
> Here, say I'm on http://example.com/page1/?a=b&c=d
> Then I call add_query_arg( '', '' ) - omitting the last parameter, which
> makes it use the current request as the base URL.
>
> This returns something like this: /page1/?a=b&c=d&
> That last & is there because I tried to add an empty parameter, so there's
> no key, but there's a trailing &(which should not be a problem as far as
> I'm aware).
>
> My question wasn't about how to use add_query_arg() as much as it was about
> whether there could be any issues by passing two empty parameters to the
> function, since it technically expects those to be strings.
>
> On Tue, 16 Feb 2016 at 14:57 James DiGioia <[hidden email]> wrote:
>
>> add_query_args takes two or three parameters, depending on how you'd prefer
>> to use it. See here:
>> https://developer.wordpress.org/reference/functions/add_query_arg/
>>
>> It's not automatically going to redirect using the current $_GET params;
>> you need to specify in the function args what those params are supposed to
>> be.
>>
>> On Tue, Feb 16, 2016 at 5:23 AM, Nikola Nikolov <[hidden email]>
>> wrote:
>>
>>> Hi there,
>>>
>>> I've found myself in situations where I want to reload the current URL
>> with
>>> PHP - for instance after processing a $_POST request - and in some cases
>> I
>>> would add a $_GET parameter to the URL(so add_query_arg() would be a
>>> perfect fit there), but in others I don't need to do that. I found out
>> that
>>> I can use
>>>
>>> wp_redirect( esc_url_raw( add_query_arg( '', '' ) ) );
>>> exit;
>>>
>>> Used that way add_query_arg() seems to just give me a path and any $_GET
>>> parameters currently present(for instance I would get
>>> /product/my-product/). I'm escaping the URL with esc_url_raw(), but
>> wanted
>>> to make sure there's nothing wrong with doing things that way.
>>>
>>> Any thoughts?
>>> _______________________________________________
>>> wp-hackers mailing list
>>> [hidden email]
>>> http://lists.automattic.com/mailman/listinfo/wp-hackers
>>>
>> _______________________________________________
>> wp-hackers mailing list
>> [hidden email]
>> http://lists.automattic.com/mailman/listinfo/wp-hackers
>>
> _______________________________________________
> wp-hackers mailing list
> [hidden email]
> http://lists.automattic.com/mailman/listinfo/wp-hackers

_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Are there any implications about using wp_redirect( add_query_arg( '', '' ) )?

Nikola Nikolov
I used esc_url_raw() because I did read somewhere in the comments about the
security issue of not escaping add_query_arg() with a missing parameter for
the base URL. However the case they were discussing then might have been if
you want to use it in a Location header(can't remember).

Good point on wp_safe_redirect() - even though you'll most-likely end up on
the same site, it's a good rule of thumb to use that when the destination
is expected to be the same site.

Thank you for the detailed explanation!

On Tue, 16 Feb 2016 at 16:25 J.D. Grimes <[hidden email]> wrote:

> You are right to question whether there might be some issues with this.
> There is in fact a potential for security issues when using wp_redirect(),
> but it has nothing to do with escaping the URL. I think it's not actually
> necessary to use esc_url_raw(), since wp_redirect() uses
> wp_sanitize_redirect() to sanitize the URL (
> https://developer.wordpress.org/reference/functions/wp_sanitize_redirect/
> <https://developer.wordpress.org/reference/functions/wp_sanitize_redirect/
> >).
>
> The issues that you would run into with wp_redirect() would be an open
> redirect (https://www.owasp.org/index.php/Open_redirect <
> https://www.owasp.org/index.php/Open_redirect>). To avoid that, you can
> use wp_safe_redirect() (
> https://developer.wordpress.org/reference/functions/wp_safe_redirect/ <
> https://developer.wordpress.org/reference/functions/wp_safe_redirect/>)
> which ensures that the redirect will only go to a host that is on the
> whitelist. I always use wp_safe_redirect(), just to be sure that the
> redirect is secure.
>
> As for this case when using add_query_arg(), it shouldn't technically be
> necessary to use wp_safe_redirect(), since add_query_arg() uses the
> REQUEST_URI server var, which should be secure. There is always the
> possibility of course that some other code has fiddled with the
> REQUEST_URI, and in that case it may not actually end up pointing to the
> correct host like it should. So that's why I'd recommend that you use
> wp_safe_redirect(), since you know that you will always be wanting to
> redirect back to the same site. wp_redirect() should only be used when you
> specifically want to redirect to a different host, and then you should be
> hard-coding the URL.
>
> As far as passing empty values to add_query_arg(), that is perfectly
> valid, although you might as well use $_SERVER['REQUEST_URI'] directly,
> instead of doing extra, unnecessary processing on the URL. There is also
> the wp_get_referer() function (
> https://developer.wordpress.org/reference/functions/wp_get_referer/ <
> https://developer.wordpress.org/reference/functions/wp_get_referer/>)
> that you could use if you want to accept more ways of determining the
> referrer.
>
> -J.D.
>
> > On Feb 16, 2016, at 8:17 AM, Nikola Nikolov <[hidden email]>
> wrote:
> >
> > Well, it actually works fine with the example I gave.
> >
> > Here, say I'm on http://example.com/page1/?a=b&c=d
> > Then I call add_query_arg( '', '' ) - omitting the last parameter, which
> > makes it use the current request as the base URL.
> >
> > This returns something like this: /page1/?a=b&c=d&
> > That last & is there because I tried to add an empty parameter, so
> there's
> > no key, but there's a trailing &(which should not be a problem as far as
> > I'm aware).
> >
> > My question wasn't about how to use add_query_arg() as much as it was
> about
> > whether there could be any issues by passing two empty parameters to the
> > function, since it technically expects those to be strings.
> >
> > On Tue, 16 Feb 2016 at 14:57 James DiGioia <[hidden email]>
> wrote:
> >
> >> add_query_args takes two or three parameters, depending on how you'd
> prefer
> >> to use it. See here:
> >> https://developer.wordpress.org/reference/functions/add_query_arg/
> >>
> >> It's not automatically going to redirect using the current $_GET params;
> >> you need to specify in the function args what those params are supposed
> to
> >> be.
> >>
> >> On Tue, Feb 16, 2016 at 5:23 AM, Nikola Nikolov <[hidden email]>
> >> wrote:
> >>
> >>> Hi there,
> >>>
> >>> I've found myself in situations where I want to reload the current URL
> >> with
> >>> PHP - for instance after processing a $_POST request - and in some
> cases
> >> I
> >>> would add a $_GET parameter to the URL(so add_query_arg() would be a
> >>> perfect fit there), but in others I don't need to do that. I found out
> >> that
> >>> I can use
> >>>
> >>> wp_redirect( esc_url_raw( add_query_arg( '', '' ) ) );
> >>> exit;
> >>>
> >>> Used that way add_query_arg() seems to just give me a path and any
> $_GET
> >>> parameters currently present(for instance I would get
> >>> /product/my-product/). I'm escaping the URL with esc_url_raw(), but
> >> wanted
> >>> to make sure there's nothing wrong with doing things that way.
> >>>
> >>> Any thoughts?
> >>> _______________________________________________
> >>> wp-hackers mailing list
> >>> [hidden email]
> >>> http://lists.automattic.com/mailman/listinfo/wp-hackers
> >>>
> >> _______________________________________________
> >> wp-hackers mailing list
> >> [hidden email]
> >> http://lists.automattic.com/mailman/listinfo/wp-hackers
> >>
> > _______________________________________________
> > wp-hackers mailing list
> > [hidden email]
> > http://lists.automattic.com/mailman/listinfo/wp-hackers
>
> _______________________________________________
> wp-hackers mailing list
> [hidden email]
> http://lists.automattic.com/mailman/listinfo/wp-hackers
>
_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Are there any implications about using wp_redirect( add_query_arg( '', '' ) )?

J.D. Grimes
I'm not sure if a Location header would be vulnerable to an unescaped URL or not. The most common reason to escape add_qeury_arg() is to avoid XSS.

-J.D.

> On Feb 16, 2016, at 10:16 AM, Nikola Nikolov <[hidden email]> wrote:
>
> I used esc_url_raw() because I did read somewhere in the comments about the
> security issue of not escaping add_query_arg() with a missing parameter for
> the base URL. However the case they were discussing then might have been if
> you want to use it in a Location header(can't remember).
>
> Good point on wp_safe_redirect() - even though you'll most-likely end up on
> the same site, it's a good rule of thumb to use that when the destination
> is expected to be the same site.
>
> Thank you for the detailed explanation!
>
> On Tue, 16 Feb 2016 at 16:25 J.D. Grimes <[hidden email]> wrote:
>
>> You are right to question whether there might be some issues with this.
>> There is in fact a potential for security issues when using wp_redirect(),
>> but it has nothing to do with escaping the URL. I think it's not actually
>> necessary to use esc_url_raw(), since wp_redirect() uses
>> wp_sanitize_redirect() to sanitize the URL (
>> https://developer.wordpress.org/reference/functions/wp_sanitize_redirect/
>> <https://developer.wordpress.org/reference/functions/wp_sanitize_redirect/
>>> ).
>>
>> The issues that you would run into with wp_redirect() would be an open
>> redirect (https://www.owasp.org/index.php/Open_redirect <
>> https://www.owasp.org/index.php/Open_redirect>). To avoid that, you can
>> use wp_safe_redirect() (
>> https://developer.wordpress.org/reference/functions/wp_safe_redirect/ <
>> https://developer.wordpress.org/reference/functions/wp_safe_redirect/>)
>> which ensures that the redirect will only go to a host that is on the
>> whitelist. I always use wp_safe_redirect(), just to be sure that the
>> redirect is secure.
>>
>> As for this case when using add_query_arg(), it shouldn't technically be
>> necessary to use wp_safe_redirect(), since add_query_arg() uses the
>> REQUEST_URI server var, which should be secure. There is always the
>> possibility of course that some other code has fiddled with the
>> REQUEST_URI, and in that case it may not actually end up pointing to the
>> correct host like it should. So that's why I'd recommend that you use
>> wp_safe_redirect(), since you know that you will always be wanting to
>> redirect back to the same site. wp_redirect() should only be used when you
>> specifically want to redirect to a different host, and then you should be
>> hard-coding the URL.
>>
>> As far as passing empty values to add_query_arg(), that is perfectly
>> valid, although you might as well use $_SERVER['REQUEST_URI'] directly,
>> instead of doing extra, unnecessary processing on the URL. There is also
>> the wp_get_referer() function (
>> https://developer.wordpress.org/reference/functions/wp_get_referer/ <
>> https://developer.wordpress.org/reference/functions/wp_get_referer/>)
>> that you could use if you want to accept more ways of determining the
>> referrer.
>>
>> -J.D.
>>
>>> On Feb 16, 2016, at 8:17 AM, Nikola Nikolov <[hidden email]>
>> wrote:
>>>
>>> Well, it actually works fine with the example I gave.
>>>
>>> Here, say I'm on http://example.com/page1/?a=b&c=d
>>> Then I call add_query_arg( '', '' ) - omitting the last parameter, which
>>> makes it use the current request as the base URL.
>>>
>>> This returns something like this: /page1/?a=b&c=d&
>>> That last & is there because I tried to add an empty parameter, so
>> there's
>>> no key, but there's a trailing &(which should not be a problem as far as
>>> I'm aware).
>>>
>>> My question wasn't about how to use add_query_arg() as much as it was
>> about
>>> whether there could be any issues by passing two empty parameters to the
>>> function, since it technically expects those to be strings.
>>>
>>> On Tue, 16 Feb 2016 at 14:57 James DiGioia <[hidden email]>
>> wrote:
>>>
>>>> add_query_args takes two or three parameters, depending on how you'd
>> prefer
>>>> to use it. See here:
>>>> https://developer.wordpress.org/reference/functions/add_query_arg/
>>>>
>>>> It's not automatically going to redirect using the current $_GET params;
>>>> you need to specify in the function args what those params are supposed
>> to
>>>> be.
>>>>
>>>> On Tue, Feb 16, 2016 at 5:23 AM, Nikola Nikolov <[hidden email]>
>>>> wrote:
>>>>
>>>>> Hi there,
>>>>>
>>>>> I've found myself in situations where I want to reload the current URL
>>>> with
>>>>> PHP - for instance after processing a $_POST request - and in some
>> cases
>>>> I
>>>>> would add a $_GET parameter to the URL(so add_query_arg() would be a
>>>>> perfect fit there), but in others I don't need to do that. I found out
>>>> that
>>>>> I can use
>>>>>
>>>>> wp_redirect( esc_url_raw( add_query_arg( '', '' ) ) );
>>>>> exit;
>>>>>
>>>>> Used that way add_query_arg() seems to just give me a path and any
>> $_GET
>>>>> parameters currently present(for instance I would get
>>>>> /product/my-product/). I'm escaping the URL with esc_url_raw(), but
>>>> wanted
>>>>> to make sure there's nothing wrong with doing things that way.
>>>>>
>>>>> Any thoughts?
>>>>> _______________________________________________
>>>>> wp-hackers mailing list
>>>>> [hidden email]
>>>>> http://lists.automattic.com/mailman/listinfo/wp-hackers
>>>>>
>>>> _______________________________________________
>>>> wp-hackers mailing list
>>>> [hidden email]
>>>> http://lists.automattic.com/mailman/listinfo/wp-hackers
>>>>
>>> _______________________________________________
>>> wp-hackers mailing list
>>> [hidden email]
>>> http://lists.automattic.com/mailman/listinfo/wp-hackers
>>
>> _______________________________________________
>> wp-hackers mailing list
>> [hidden email]
>> http://lists.automattic.com/mailman/listinfo/wp-hackers
>>
> _______________________________________________
> wp-hackers mailing list
> [hidden email]
> http://lists.automattic.com/mailman/listinfo/wp-hackers

_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers