WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 43 hours ago

#44965 accepted defect (bug)

WordPress Core strips $_GET['error'] occasionally

Reported by: javorszky Owned by: SergeyBiryukov
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch needs-unit-tests
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 6 months ago.
44965-2.patch (2.2 KB) - added by gabrielchung1128 43 hours ago.
Test Case

Download all attachments as: .zip

Change History (12)

#1 @SergeyBiryukov
6 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 6 months ago by SergeyBiryukov (previous) (diff)

#2 @SergeyBiryukov
6 months ago

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

#3 @pento
5 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.


5 months ago

#5 @pento
3 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#6 @SergeyBiryukov
3 months ago

  • Milestone changed from 5.0.2 to 5.1

#7 @pento
2 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
2 months ago

  • Milestone changed from 5.1 to 5.2

#9 @gabrielchung1128
2 days 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
43 hours ago

Test Case

#10 @gabrielchung1128
43 hours ago

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

Thanks.

Note: See TracTickets for help on using tickets.