Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] honor original request method even if it's called from lookup #434

Merged
merged 1 commit into from
Feb 21, 2021

Conversation

smaeda-ks
Copy link
Contributor

@smaeda-ks smaeda-ks commented Feb 19, 2021

Rate-limiting module creates a Request Settings that is turned into the below VCL code:

# Request Condition: magentomodule_rate_limiting Prio: 150
if( req.http.Rate-Limit ) {
  return(lookup);
}

This would break Magento site functionalities since Fastly Varnish has surprising behavior where any request method except POST is being overwritten to GET if return(lookup) is called for the request.

So once this rate-limiting module is enabled, request methods such as DELETE, PUT, etc no longer work as expected and the origin server receives transformed GET requests instead.

To work around this, we stash the original request method in recv and restore it in miss if rate-limit module is enabled for the request. This would cause all request methods that are not defined as cacheable in RFC to be cached, which is bad. But since we also have this snippet in fetch:

https://github.com/fastly/fastly-magento2/blob/master/etc/vcl_snippets_rate_limiting/fetch.vcl#L1-L11

only 429 response is cached and all other status codes are forced to be PASS. So we should be fine with this change.

@smaeda-ks smaeda-ks requested a review from vvuksan February 19, 2021 23:02
@smaeda-ks smaeda-ks self-assigned this Feb 19, 2021
@smaeda-ks smaeda-ks force-pushed the smaeda-ks/fix-rate-limit-module branch from 6f3cd3f to 1f5a17a Compare February 20, 2021 18:47
@smaeda-ks smaeda-ks changed the title rate-limit module should not create request settings honor original request method even if it's called from lookup Feb 20, 2021
@smaeda-ks smaeda-ks changed the title honor original request method even if it's called from lookup [Bug] honor original request method even if it's called from lookup Feb 20, 2021
@vvuksan vvuksan merged commit a6c1e8b into fastly:master Feb 21, 2021
Copy link
Contributor

@vvuksan vvuksan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@ninex
Copy link

ninex commented Mar 11, 2021

Hi,
If it's a PUT the body of the request is being discarded and an empty body sent to Magento.

@smaeda-ks
Copy link
Contributor Author

@ninex Thanks, I'll check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants