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
Merged

Conversation

@t3chguy
Copy link
Collaborator

t3chguy commented Feb 5, 2020

Fixes #3632

@t3chguy t3chguy requested a review from vector-im/riot-web Feb 5, 2020
@jryans jryans requested review from jryans and removed request for vector-im/riot-web Feb 6, 2020
Copy link
Member

jryans left a comment

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 Feb 6, 2020
…or backwards compat and split onto multiple lines for readability

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Co-Authored-By: J. Ryan Stinnett <jryans@gmail.com>
@t3chguy t3chguy requested a review from jryans Feb 6, 2020
@stoically

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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.

@jryans
jryans approved these changes Feb 6, 2020
Copy link
Member

jryans left a comment

Looks great, thanks! 😁

@t3chguy t3chguy merged commit eb62972 into develop Feb 6, 2020
7 checks passed
7 checks passed
buildkite/riot-web Build #1952 passed (19 minutes, 7 seconds)
Details
buildkite/riot-web/build Passed (5 minutes, 57 seconds)
Details
buildkite/riot-web/eslint-js-lint Passed (3 minutes, 56 seconds)
Details
buildkite/riot-web/eslint-types-lint Passed (1 minute, 1 second)
Details
buildkite/riot-web/i18n Passed (3 minutes, 48 seconds)
Details
buildkite/riot-web/jest-tests Passed (3 minutes, 59 seconds)
Details
buildkite/riot-web/pipeline Passed (3 seconds)
Details
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:;

This comment has been minimized.

Copy link
@rugk

rugk Feb 9, 2020

Contributor

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?

This comment has been minimized.

Copy link
@t3chguy

t3chguy Feb 9, 2020

Author Collaborator

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

This comment has been minimized.

Copy link
@rugk

rugk Feb 10, 2020

Contributor

done: #12304

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.