#44965: WordPress Core strips $_GET['error'] occasionally
----------------------------------------+------------------------------
Reporter: javorszky | Owner: (none)
Type: defect (bug) | Status: new
Priority: normal | Milestone: Awaiting Review
Component: Query | Version:
Severity: normal | Resolution:
Keywords: has-patch needs-unit-tests | Focuses:
----------------------------------------+------------------------------
Changes (by SergeyBiryukov):
* keywords: => has-patch needs-unit-tests
* component: General => Query
Old description:
> I have a plugin that is an OAuth2 consumer for integrating with Stripe
> Connect.
>
> I created a new custom endpoint by adding a query var, and a rewrite
> rule, so everything that lands on `/stripe_connect` will get dealt with
> by my plugin's code.
>
> If user denies the connection request at Stripe, they are redirected back
> to my site with roughly the following URL params in tow:
>
> `/stripe_connect?state=3__5e4e4d4c9df8e6948a33fdfb44f75c0f&error=access_denied&error_description=The+user+denied+your+request`
>
> * `state` is a custom param I set that gets replayed to me
> * `error` is `access_denied`, which is the standard that Stripe will do
> in this case, see https://stripe.com/docs/connect/oauth-reference#get-
> authorize-errors
> * `error_description` is a human readable problem
>
> However in `parse_request`, a variable by the name of `$error` gets set
> to `404` at the beginning, and as it matches the rules, if it's still 404
> (ie no other error popped up, it will then unset `$_GET['error']`.
>
> Link to code: https://core.trac.wordpress.org/browser/trunk/src/wp-
> includes/class-wp.php#L260
>
> Which is something I'd actually need to deal with.
>
> Currently the way to get around it is to use `$_REQUEST` instead of
> `$_GET`, however `$_REQUEST` also has POST variables in it, so I can't
> make sure that the `error` I'm getting is actually due to a query param.
>
> I also haven't found a ticket that had this listed as a problem.
>
> What was the reasoning for unsetting that $_GET var?
>
> I see that they were added originally in
> https://core.trac.wordpress.org/changeset/1570 (14 years ago), however is
> that still a valid reason?
New description:
I have a plugin that is an OAuth2 consumer for integrating with Stripe
Connect.
I created a new custom endpoint by adding a query var, and a rewrite rule,
so everything that lands on `/stripe_connect` will get dealt with by my
plugin's code.
If user denies the connection request at Stripe, they are redirected back
to my site with roughly the following URL params in tow:
`/stripe_connect?state=3__5e4e4d4c9df8e6948a33fdfb44f75c0f&error=access_denied&error_description=The+user+denied+your+request`
* `state` is a custom param I set that gets replayed to me
* `error` is `access_denied`, which is the standard that Stripe will do in
this case, see https://stripe.com/docs/connect/oauth-reference#get-
authorize-errors
* `error_description` is a human readable problem
However in `parse_request`, a variable by the name of `$error` gets set to
`404` at the beginning, and as it matches the rules, if it's still 404 (ie
no other error popped up, it will then unset `$_GET['error']`.
Link to code: https://core.trac.wordpress.org/browser/trunk/src/wp-
includes/class-wp.php#L260
Which is something I'd actually need to deal with.
Currently the way to get around it is to use `$_REQUEST` instead of
`$_GET`, however `$_REQUEST` also has POST variables in it, so I can't
make sure that the `error` I'm getting is actually due to a query param.
I also haven't found a ticket that had this listed as a problem.
What was the reasoning for unsetting that $_GET var?
I see that they were added originally in [1570] (14 years ago), however is
that still a valid reason?
--
Comment:
> What was the reasoning for unsetting that $_GET var?
`error` is one of the public query vars, and the further code
[source:tags/4.9.8/src/wp-includes/class-wp.php?marks=292-295#L289 checks
if any of those vars is set via $_POST or $_GET], so it seems the reason
is to make sure `$this->query_vars['error']` is not set when there's no
error.
Tthere might be a better way to do that though, something like
[attachment:44965.patch].
--
Ticket URL: <https://core.trac.wordpress.org/ticket/44965#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform