littlebizzy / slickstack

Lightning-fast WordPress on Nginx
https://slickstack.io
GNU General Public License v3.0
629 stars 112 forks source link

WP-JSON timestamp and AMP query string redirect conflict #130

Closed damiankrzyz closed 2 years ago

damiankrzyz commented 2 years ago

Hi Guys,

I have spent the last few day trying to figure out why my app with OAUTH doesn't connect to Woocommerce REST API. Each call was being 401 redirecting. Cause: The NGINX has been redirecting the API calls as they include oauth_timestAMP query. Temporary Solution: commented out
redirect ?amp queries if ($query_string ~ "^(.*)amp(.*)$") { rewrite ^(.*)$ $uri? permanent; } In the nginx config

jessuppi commented 2 years ago

Thanks for this important report @damiankrzyz

Do you have an example REST API query (with the AMP syntax) that is being redirected? Maybe we can create an exception in the Nginx server block to not redirect certain URLs related to the REST API. However, it sounds perhaps like this could just be poorly coded syntax in the app you are using... which app is that?

damiankrzyz commented 2 years ago

Hi @jessuppi, Thanks for your reply. Yes I do, Please see the below. It's for an integration with bookkeeping software QuickBooks Online. Provided by OneSaas: "GET /wp-json/wc/v2/products?page=1&per_page=50&orderby=modified&order=desc&consumer_key=ck_xxx&consumer_secret=cs_xxx&oauth_consumer_key=ck_xxx&oauth_nonce=xxx&oauth_signature_method=HMAC-SHA256&oauth_timestamp=1640962464&oauth_signature=idxxx HTTP/2.0" 200 61615 "-" "OneSaas-CloudIntegrations" It's the query oauth_timestamp that's got the amp inside it.

jessuppi commented 2 years ago

Thanks for the reply with more information. The good news is this is just a simple syntax conflict...

I'm surprised many other developers who redirect AMP queries have not blogged about this to my knowledge, but it certainly seems that the timestamp string is a common one that should be supported.

The challenge is that AMP content is generated in various ways:

So if we eliminate this particular conflict by adding a slash to the rule, we lose the redirect for non-slash cases. I'm not sure if adding an "exception" to the if statement in Nginx is possible (or even a good idea if so).

jessuppi commented 2 years ago

Okay, I think we have a solution. We have changed this:

    ## redirect ?amp queries ##
    if ($query_string ~ "^(.*)amp(.*)$") {
        rewrite ^(.*)$ $uri? permanent;
    }

...to be like this:

## redirect ?amp queries ##
if ($query_string ~ "^amp(.*)$") {
    rewrite ^(.*)$ $uri? permanent;
}

So now it will only 301 redirect AMP query strings that begin with ?amp... instead of ?WILDCARDamp... strings. Based on how most AMP implementations work these days (e.g. ?amp=1 or such) this should be enough.

Ref: https://github.com/littlebizzy/slickstack/commit/6612a9d7b5e2e6c11fbc82b5b431b546caa5dcae

jessuppi commented 2 years ago

Also related discussions on how/why AMP shifted more to query strings:

Ref: https://github.com/ampproject/amp-wp/issues/2204 Ref: https://wordpress.org/support/topic/change-amp-to-amp-and-amp-home-page-not-working/