Skip to content

Conversation

@matoakley
Copy link
Contributor

@matoakley matoakley commented Dec 11, 2024

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:

  • Update CHANGELOG.md if the changes would impact users
  • Update README.md, if appropriate.
  • Update any relevant pages in /docs, if necessary
  • For security fixes, the Disclosure Policy must be followed.

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.
@matoakley matoakley requested a review from a team as a code owner December 11, 2024 01:55
@matoakley
Copy link
Contributor Author

I have signed the CLA!

@chrisedington
Copy link

Thanks @matoakley - effecting me too.

Copy link
Contributor

@sle-c sle-c left a 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.

@sle-c
Copy link
Contributor

sle-c commented Dec 11, 2024

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.
@matoakley
Copy link
Contributor Author

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)

Thanks for the feedback @sle-c. I've taken your suggestion on-board and submitted a commit with green tests 🟢

@sle-c
Copy link
Contributor

sle-c commented Dec 11, 2024

thanks again for fixing it @matoakley . Merging now!

@sle-c sle-c merged commit e8798eb into Shopify:main Dec 11, 2024
6 checks passed
@resistorsoftware
Copy link

Thank you for the fast update!

alexvko-goody added a commit to ongoody/shopify_app_ that referenced this pull request Mar 25, 2025
alexvko-goody added a commit to ongoody/shopify_app_ that referenced this pull request Mar 25, 2025
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.

4 participants