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

Embed CSP meta tag and stop using script-src unsafe-inline #12258

Merged
merged 5 commits into from
Feb 6, 2020

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Feb 5, 2020

Fixes #3632

@t3chguy t3chguy requested a review from a team February 5, 2020 20:01
@jryans jryans requested review from jryans and removed request for a team February 6, 2020 09:37
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Looks great overall!

Please file a new issue for the unsafe-eval if we don't have one already (I'd like to use something separate from #3632 at least).

Thanks also for the font-src fix, and splitting child-src to frame-src and worker-src.

Safari doesn't yet support worker-src... Maybe it's best to keep child-src for older browsers, but also have frame-src and worker-src for newer ones?

src/vector/index.html Outdated Show resolved Hide resolved
src/vector/index.html Outdated Show resolved Hide resolved
t3chguy and others added 2 commits February 6, 2020 11:52
…or backwards compat and split onto multiple lines for readability

Signed-off-by: Michael Telatynski <[email protected]>
Co-Authored-By: J. Ryan Stinnett <[email protected]>
@t3chguy t3chguy requested a review from jryans February 6, 2020 11:54
@stoically
Copy link

stoically commented Feb 6, 2020

Slightly off-topic, just in case someone wonders (like I did) what happens if CSP is defined as HTTP header and meta tag (which probably happens for self-hosted riot instances with CSP via HTTP header with this change):

the browser uses the most-restrictive CSP directives, wherever they’re specified

https://stackoverflow.com/a/51153816

@jryans
Copy link
Collaborator

jryans commented Feb 6, 2020

the browser uses the most-restrictive CSP directives, wherever they’re specified

Right, we'll remove the HTTP specified ones once this PR has been deployed to release.

Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! 😁

@t3chguy t3chguy merged commit eb62972 into develop Feb 6, 2020
default-src 'none';
style-src 'self' 'unsafe-inline';
script-src 'self' 'unsafe-eval' https://www.recaptcha.net https://www.gstatic.com;
img-src * blob: data:;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why img-src *?

At the time I've suggested one it did seem to work without that (in my tests). Maybe I did not use/test some third-party image embedding loading?

That said, loading images directly from third-party servers may/should possibly be avoided due to privacy concerns, should not it?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was copied from the CSP header served at riot.im/app
I did not evaluate img-src, feel free to open an issue for it

Copy link
Contributor

Choose a reason for hiding this comment

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

done: #12304

@t3chguy t3chguy deleted the t3chguy/csp branch May 12, 2022 09:07
t3chguy added a commit that referenced this pull request Oct 17, 2024
* Remove allchange dependency

Signed-off-by: Michael Telatynski <[email protected]>

* Remove stale release scripts

Signed-off-by: Michael Telatynski <[email protected]>

* Update pull request template to remove allchange behaviours

Signed-off-by: Michael Telatynski <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>
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.

Enable strict CSP headers to sandbox against any possible XSS
4 participants