-
Notifications
You must be signed in to change notification settings - Fork 723
Pass values to CSP frame_ancestors as individual arguments #1929
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
Conversation
Rails core has patched a CVE preventing passing a string with whitespace as an argument. rails/rails@3da2479 This patch passes the arguments individually instead which achieves the same result whilst meeting the new requirements.
|
I have signed the CLA! |
|
Thanks @matoakley - effecting me too. |
sle-c
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing this. I encountered this issue yesterday as I was testing as well.
|
although let's fix the test cases, the frame_ancestors config uses a proc to avoid the test case failing I think. Maybe try returning an array instead of a string? policy.frame_ancestors(-> do
domain_host = current_shopify_domain || "*.#{::ShopifyApp.configuration.myshopify_domain}"
[
"#{ShopifyAPI::Context.host_scheme}://#{domain_host}",
"https://admin.#{::ShopifyApp.configuration.unified_admin_domain}",
]
end) |
@sle-c has pointed out that the tests rely on the proc and suggests reimplemeting the proc and returning an array. This patch implements the recommendation and achieves the same result.
Thanks for the feedback @sle-c. I've taken your suggestion on-board and submitted a commit with green tests 🟢 |
|
thanks again for fixing it @matoakley . Merging now! |
|
Thank you for the fast update! |
What this PR does
Rails core has patched a CVE preventing passing a string with whitespace as an argument.
rails/rails@3da2479
This patch passes the arguments individually instead which achieves the same result whilst meeting the new requirements.
Reviewer's guide to testing
There is no change in overall functionality.
Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.mdif the changes would impact usersREADME.md, if appropriate./docs, if necessary