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: Remove CloudFront functions #406

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jackylamhk
Copy link

@jackylamhk jackylamhk commented Sep 7, 2024

Fixes #129

  • Use 403/404 error responses to replace the request function
  • Use response headers policy to replace the response function

With these changes, the legacy CloudFront functions used to accomplish SPA redirects and injecting security headers to responses are no longer needed. This also fixes an issue when CloudFront functions limits are reached (mentioned in #350).

@jackylamhk
Copy link
Author

Not sure how tests are run, I keep getting this error:

 FAIL  test/unit/permissions.test.ts
  ● Test suite failed to run

    TypeError: Converting circular structure to JSON
        --> starting at object with constructor 'Serverless'
        |     property 'yamlParser' -> object with constructor 'YamlParser'
        --- property 'serverless' closes the circle
        at stringify (<anonymous>)

      at messageParent (node_modules/jest-worker/build/workers/messageParent.js:34:19)

@mnapoli
Copy link
Member

mnapoli commented Sep 8, 2024

Thanks for the PR!

Yeah unfortunately tests are broken on master because of changes in serverless framework, haven't found time (or even have an idea) to fix it :/

Use 403/404 error responses to replace the request function

Could you explain this? I'm not sure what the impact is? Will the application behave any differently?

Use response headers policy to replace the response function

Are the same exact security headers added? I.e. are there any practical changes for applications already deployed with Lift?

@jackylamhk
Copy link
Author

jackylamhk commented Sep 8, 2024

@mnapoli No worries.

Use 403/404 error responses to replace the request function

Could you explain this? I'm not sure what the impact is? Will the application behave any differently?

SPAs need their origin paths rewritten to /index.html to load the entry point, Lift currently uses a CloudFront function to redirect any paths not ending in a whitelisted extension.

While this works, the function is not necessary - by setting the custom error responses for 404 (not found) and 403 (unauthorized, S3 returns one if the file doesn't exist and s3:ListBucket isn't allowed) we can redirect these to the entry point too.

Use response headers policy to replace the response function

Are the same exact security headers added? I.e. are there any practical changes for applications already deployed with Lift?

Here are the new headers (also linked in docs):

Header Value
Referrer-Policy strict-origin-when-cross-origin
Strict-Transport-Security max-age=31536000
X-Content-Type-Options nosniff
X-Frame-Options SAMEORIGIN
X-XSS-Protection 1; mode=block

And here are the old headers:

Header Value
x-frame-options SAMEORIGIN
x-content-type-options nosniff
x-xss-protection 1; mode=block
strict-transport-security max-age=63072000

So it is slightly different: mainly adding the Referrer-Policy.

I didn't retain the option to remove x-frame-options (security.allowIframe) as folks should use the new responseHeadersPolicy key to create a custom policy with securityHeadersBehavior.contentSecurityPolicy instead - for security, instead of allowing all iframes. I could add it back for backward compatibility, though it might be easier to use custom policies if we keep it.

FYI - this build was tested with a SPA and Serverless deployment.

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.

Move from Cloudfront functions to Response headers for security
2 participants