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

fix(nextjs-component, e2e-tests): allow all CloudFront HTTP methods for default caching behavior #609

Merged
merged 1 commit into from Sep 19, 2020

Conversation

dphang
Copy link
Collaborator

@dphang dphang commented Sep 18, 2020

This fixes #454 and similar issues.

Not sure why we don't allow all CloudFront methods on default caching behavior, which will allow more HTTP methods e.g form posts on SSR pages, similar to Vercel.

This component already adds a policy to the S3 bucket to only allow GetObject from the origin access identity (CloudFront), so requesters cannot modify S3 objects with these additional methods (e.g using DELETE to delete a public file). See: https://docs.aws.amazon.com/cloudfront/latest/APIReference/API_AllowedMethods.html

Tests

Updated unit and e2e tests, and ran them (e2e tests not yet automated).

SSR page: https://dcrrk28otqvm3.cloudfront.net/ssr-page (allows GET, HEAD, PUT, PATCH, DELETE, POST, OPTIONS). E.g POST/DELETE will return page itself since there is no different behavior here for POST/DELETE

For resources in S3:
SSG page: https://dcrrk28otqvm3.cloudfront.net/ssg-page (allows GET, HEAD. OPTIONS requires origin request header)
Public files: https://dcrrk28otqvm3.cloudfront.net/app-store-badge.png (allows, GET, HEAD. OPTIONS requires origin request header.

The above two, for POST gets 405 Method Not Allowed, and for PUT, PATCH, DELETE gets 403 Forbidden, which should be due to the S3 bucket policy only allow GetObject from the CloudFront distribution.

Static JS (same as before, it cannot be DELETED, as _next/static cache behavior only allows GET, HEAD): https://dcrrk28otqvm3.cloudfront.net/_next/static/chunks/framework.085e84bea8b122ad7b41.js

For comparison:

Vercel SSR page: https://nextjs-repros.vercel.app/anotherSSR (allows GET, HEAD, PUT, PATCH, DELETE, POST, OPTIONS and even more uncommon methods, since they do not use CloudFront but their own custom CDN).
Vercel SSG page: https://nextjs-repros.vercel.app/anotherSSG (allows GET, OPTIONS, HEAD)

@dphang dphang changed the title fix(nextjs-component): allow all CloudFront HTTP methods for default caching behavior fix(nextjs-component, e2e-tests): allow all CloudFront HTTP methods for default caching behavior Sep 18, 2020
@danielcondemarin
Copy link
Contributor

danielcondemarin commented Sep 18, 2020

Not sure why we don't allow all CloudFront methods on default caching behavior, which will allow more HTTP methods e.g form posts on SSR pages, similar to Vercel.

The reasoning was that the default routes would only serve pages, therefore HEAD, GET is sufficient. For an HTTP POST I believe Next.js recommends using an API route although may be possible to do via getInitialProps judging from this thread. Having said all that I don't see downside in allowing the other HTTP methods 👍

@dphang
Copy link
Collaborator Author

dphang commented Sep 18, 2020

Not sure why we don't allow all CloudFront methods on default caching behavior, which will allow more HTTP methods e.g form posts on SSR pages, similar to Vercel.

The reasoning was that the default routes would only serve pages, therefore HEAD, GET is sufficient. For an HTTP POST I believe Next.js recommends using an API route although may be possible to do via getInitialProps judging from this thread. Having said all that I don't see downside in allowing the other HTTP methods 👍

Thanks for taking a look. Yeah, I can't imagine this is common since there are API routes or you would use an external API (like I am), but maybe someone has a use case for other methods, e.g POST of form to the same page, or they want to explicitly render a different page if non-GET method is called on SSR page.

@dphang dphang merged commit 6066fa1 into master Sep 19, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-allowed-methods branch September 19, 2020 00:50
@dphang dphang restored the fix-allowed-methods branch September 19, 2020 00:50
@dphang dphang deleted the fix-allowed-methods branch September 19, 2020 00:50
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.

Getting 403 errors on some of the POST requests
2 participants