WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 6 months ago

#44965 accepted defect (bug)

WordPress Core strips $_GET['error'] occasionally

Reported by: javorszky Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch has-unit-tests dev-feedback
Focuses: Cc:

Description (last modified by SergeyBiryukov)

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

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?

Attachments (2)

44965.patch (1004 bytes) - added by SergeyBiryukov 12 months ago.
44965-2.patch (2.2 KB) - added by gabrielchung1128 6 months ago.
Test Case

Download all attachments as: .zip

Change History (13)

#1 @SergeyBiryukov
12 months ago

  • Component changed from General to Query
  • Description modified (diff)
  • Keywords has-patch needs-unit-tests added

What was the reasoning for unsetting that $_GET var?

error is one of the public query vars, and the further code 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.

There might be a better way to do that though, something like 44965.patch.

Last edited 12 months ago by SergeyBiryukov (previous) (diff)

#2 @SergeyBiryukov
12 months ago

  • Milestone changed from Awaiting Review to 4.9.9
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#3 @pento
12 months ago

  • Milestone changed from 4.9.9 to 5.0.1

This ticket was mentioned in Slack in #core-php by sergey. View the logs.


11 months ago

#5 @pento
9 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#6 @SergeyBiryukov
9 months ago

  • Milestone changed from 5.0.2 to 5.1

#7 @pento
8 months ago

@SergeyBiryukov: Why the unset( $this->query_vars['error'] )? It seems like the original purpose ([1570]) was that WP_Query would generate a 404 if the error query var was not an empty string. WP_Query now only generates a 404 if the error query var is set to '404'.

#8 @pento
8 months ago

  • Milestone changed from 5.1 to 5.2

#9 @gabrielchung1128
6 months ago

When I am constructing a test case for this issue, I have found that when the test case is run, the global scope is removed. It means that $_GET will be cleared. It makes it difficult to create a test case for it. Should we have a configuration variable to bypass cleaning the global scope? Thanks.

Here is the line of code of removing the global scope:
https://github.com/WordPress/wordpress-develop/blob/c62eab00a79d76ef94f01ee841cfa7708ad9811c/tests/phpunit/includes/abstract-testcase.php#L129

@gabrielchung1128
6 months ago

Test Case

#10 @gabrielchung1128
6 months ago

I have figured a way to set and store the $_GET variables.

Thanks.

#11 @pento
6 months ago

  • Keywords has-unit-tests dev-feedback added; needs-unit-tests removed
  • Milestone changed from 5.2 to Future Release
Note: See TracTickets for help on using tickets.