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: use custom directive as-is when overriding csp and the default is 'none' #2076

Merged
merged 3 commits into from
May 2, 2024

Conversation

nkgentile
Copy link
Contributor

@nkgentile nkgentile commented May 2, 2024

WHY are these changes introduced?

Fixes #2064

When extending the default content security policy, if the underlying default directive was set to none (for example, frameAncestors) then createContentSecurityPolicy would merge it with the custom directive and return a header with frame-ancestors 'self' 'none';.

In Chrome, the browser will log an error in the console:

The Content-Security-Policy directive 'frame-ancestors' contains the keyword 'none' alongside with other source expressions. The keyword 'none' must be the only source expression in the directive value, otherwise it is ignored.

WHAT is this pull request doing?

This small PR adjusts the createContentSecurityPolicy helper so that when the default directive is none it won't merge the default into the custom directive.

Checklist

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've added a changeset if this PR contains user-facing or noteworthy changes
  • I've added tests to cover my changes
  • I've added or updated the documentation
@nkgentile
Copy link
Contributor Author

I have signed the CLA!

Copy link
Contributor

@michenly michenly left a comment

Choose a reason for hiding this comment

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

I suggested an improvement, but the current solution is sound as well.

Don't forget to add changeset!

packages/hydrogen/src/csp/csp.ts Outdated Show resolved Hide resolved
@nkgentile nkgentile changed the title fix: use frame-ancestors directive as-is when overriding csp May 2, 2024
@michenly michenly merged commit b70f9c2 into Shopify:main May 2, 2024
7 checks passed
@michenly
Copy link
Contributor

michenly commented May 2, 2024

❤️ thank you for the contribution!

@nkgentile nkgentile deleted the fix/csp-frame-ancestors branch May 2, 2024 18:33
@nkgentile
Copy link
Contributor Author

Thank you for the swift review!

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