Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use "unsafe-wasm-eval" instead of "unsafe-eval" in script-src CSP #20606

Merged
merged 3 commits into from
Nov 15, 2022
Merged

Use "unsafe-wasm-eval" instead of "unsafe-eval" in script-src CSP #20606

merged 3 commits into from
Nov 15, 2022

Conversation

prplecake
Copy link
Contributor

@prplecake prplecake commented Nov 14, 2022

In the CSP we are replacing the recently added unsafe-eval with the more strict unsafe-wasm-eval, as unsafe-eval is and was not really needed. This was initially intended in #18817 but we couldn't get it working until now.

(Thanks @felixdoerre)

@felixdoerre
Copy link

Thanks. I think you changed the wrong unsafe-eval. You want to change the one in production (line 39) and not the one for dev (line 34)

@ineffyble ineffyble added the security Security issues and fixes, vulnerabilities label Nov 14, 2022
@Gargron
Copy link
Member

Gargron commented Nov 14, 2022

Please explain what you are doing and why

@felixdoerre
Copy link

In the CSP we are replacing the recently added unsafe-eval with the more strict unsafe-wasm-eval, as unsafe-eval is and was not really needed. This was initially intended in #18817 but we couldn't get it working until now.

@prplecake
Copy link
Contributor Author

Thanks. I think you changed the wrong unsafe-eval. You want to change the one in production (line 39) and not the one for dev (line 34)

D'oh! Fixed in last push.

@Gargron
Copy link
Member

Gargron commented Nov 15, 2022

Wait, is it actually supposed to be double-quoted like that?

@Gargron Gargron merged commit b46b7c3 into mastodon:main Nov 15, 2022
@@ -36,7 +36,7 @@ def host_to_url(str)
p.worker_src :self, :blob, assets_host
else
p.connect_src :self, :data, :blob, assets_host, media_host, Rails.configuration.x.streaming_api_base_url
p.script_src :self, assets_host, :unsafe_eval
p.script_src :self, assets_host, "'unsafe-wasm-eval'"
Copy link
Member

Choose a reason for hiding this comment

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

Wait, according to the issue discussion, it's wasm-unsafe-eval, not unsafe-wasm-eval. Which is it?

@prplecake prplecake deleted the fix-content-security-policy branch November 15, 2022 02:24
@prplecake
Copy link
Contributor Author

Wait, is it actually supposed to be double-quoted like that?

Yes those double quotes are intentional, though I couldn't tell you why. See the diffs in the issue, single quoting didn't work for me but double quoting did.

And I'm pretty sure I have the correct directive, also see the issue. "unsafe-wasm-eval" doesn't produce CSP errors from the browser.

@Gargron
Copy link
Member

Gargron commented Nov 15, 2022

There are no search results for "unsafe-wasm-eval" but there are Google/Firefox pages referencing "wasm-unsafe-eval", also the person who told you about the quotes used "wasm-unsafe-eval" in their code snippet.

@prplecake prplecake restored the fix-content-security-policy branch November 15, 2022 03:08
@prplecake
Copy link
Contributor Author

You're right, it should be wasm-unsafe-eval.

kadoshita pushed a commit to kadoshita/mastodon that referenced this pull request Nov 19, 2022
…stodon#20606)

* Add "unsafe-eval" to script-src CSP

* Use 'unsafe-wasm-eval' instead of 'unsafe-eval'
mimikun added a commit to mimikun/mastodon that referenced this pull request Nov 23, 2022
mimikun added a commit to mimikun/mastodon that referenced this pull request Nov 24, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
security Security issues and fixes, vulnerabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants