8000 Use more specific CSP directives by tdeo · Pull Request #1195 · Shopify/maintenance_tasks · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use more specific CSP directives #1195

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Mar 31, 2025
Merged

Use more specific CSP directives #1195

merged 5 commits into from
Mar 31, 2025

Conversation

tdeo
Copy link
Contributor
@tdeo tdeo commented Mar 25, 2025

We wanted to try out this gem in our repo but we're using the script-src-elem directive in our CSP for <script> inline tags, which fallbacks to script-src if unset.

But since script-src-elem is not defined in maintenance-tasks, we're seeing the following error at this moment:

image

Opening this as draft for now since there are no tests yet

@etiennebarrie
Copy link
Member

Ah good catch thanks! It seems we have the same issue with style-src-elem. Basically we missed the concept of fallbacks.

What you should do is set the strictest policy possible in the dummy app, then use the smallest possible fix in the engine. In that case, if the dummy app disallows everything with script-src-elem instead of script-src, the engine can just allow our script element with script-src-elem.

@tdeo
Copy link
Contributor Author
tdeo commented Mar 26, 2025

What you should do is set the strictest policy possible in the dummy app, then use the smallest possible fix in the engine. In that case, if the dummy app disallows everything with script-src-elem instead of script-src, the engine can just allow our script element with script-src-elem.

Hi @etiennebarrie, thanks for the pointers! To be as conservative as possible, I did set default-src(:none) on the dummy app so that all missing directives fallback to that one and be as exhaustive as possible.

I had to keep both frame-src and style-src-elem set for the dummy app iframe to load and for tests to remain green. I'm not sure I fully undestand why MaintenanceTasks::ApplicationController does override policy.frame_ancestors(:self), inherently allowing iframing into the same app but nowhere else

@tdeo
Copy link
Contributor Author
tdeo commented Mar 26, 2025

I have signed the CLA!

@tdeo tdeo marked this pull request as ready for review March 26, 2025 10:59
@etiennebarrie
Copy link
Member

all missing directives fallback to that one and be as exhaustive as possible

My understanding is that it's the opposite we want: if the application is using specific directives, but the engine defines fallback directives, we have the bug you're reporting where the more specific directives apply and script/styles are not allowed.

So we want the dummy app to be super specific and to not use fallbacks, that way it forces the engine to use the most-specific directives to make sure it overrides what needs to be overridden.

I think the engine just needs to be changed to

policy.frame_ancestors(:self), inherently allowing iframing into the same app

It's a feature request that we had to be able to render the engine into the application as an iframe: #168 #170

@tdeo tdeo changed the title Add script-src-elem directive in the CSP Use more specific CSP directives Mar 27, 2025
@tdeo
Copy link
Contributor Author
tdeo commented Mar 27, 2025

Indeed, I did update the dummy app to list all possible CSP directives (all those available in ActionDispatch except the ones unsupported in Selenium), to ensure that indeed the directives are set as specifically and not thanks to fallbacks

etiennebarrie
etiennebarrie reviewed Mar 28, 2025
View reviewed changes
etiennebarrie
etiennebarrie approved these changes Mar 31, 2025
View reviewed changes
Copy link
Member
@etiennebarrie etiennebarrie left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution!

@etiennebarrie etiennebarrie merged commit 383782c into Shopify:main Mar 31, 2025
20 checks passed
@tdeo tdeo deleted the patch-1 branch March 31, 2025 15:49
@etiennebarrie etiennebarrie mentioned this pull request May 6, 2025
Merged
@etiennebarrie
Copy link
Member

Just realized this broke Safari. Somehow it uses style-src even when style-src-elem is set.

@tdeo
Copy link
Contributor Author
tdeo commented May 15, 2025

Hello @etiennebarrie which safari are you using ? I do get working styles with the latest version 2.12.0 locally

@etiennebarrie
Copy link
Member

Version 18.5 (20621.2.5.11.8), the one that ships with macOS 15.5 released a few days ago.

@tdeo
Copy link
Contributor Author
tdeo commented May 16, 2025

I just tested on that same safari and it seems to be working for me on 2.12.0:

image

@etiennebarrie
Copy link
Member

Super weird, I'm definitely reproducing:

Image image

@etiennebarrie etiennebarrie mentioned this pull request Jun 24, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etiennebarrie etiennebarrie etiennebarrie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tdeo @etiennebarrie
0