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

Set content security policy http header for all responses #621

Merged
merged 3 commits into from
May 6, 2022

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Apr 11, 2022

Tested with this browser plugin that it works without any errors.

This site analyzes the policy. Main problems are frame-ancestors (do we really want to block iframes?) and upgrade-insecure-requests (not sure if this would break http images).

@Nutomic Nutomic requested a review from dessalines as a code owner April 11, 2022 11:45
@dessalines
Copy link
Member

Broke it again. Check the browser console.

@Nutomic
Copy link
Member Author

Nutomic commented Apr 12, 2022

I checked it. What error do you see?

@dessalines
Copy link
Member

Content Security Policy: The page’s settings blocked the loading of a resource at eval (“script-src”). client.js:675

@Nutomic
Copy link
Member Author

Nutomic commented Apr 13, 2022

I cant reproduce that, but added unsafe-eval. Would be good if we could get rid of all the unsafe btw.

@dessalines
Copy link
Member

Now I'm getting:

Content Security Policy: The page’s settings blocked the loading of a resource at ws://localhost:8536/api/v3/ws (“connect-src”).

To reproduce this, you have to be running yarn start in the lemmy-ui folder, and going to http://localhost:1234

@Nutomic
Copy link
Member Author

Nutomic commented Apr 19, 2022

Found a way to fix it.

@Nutomic Nutomic force-pushed the csp-http-header2 branch from 88352b1 to ef669cf Compare May 3, 2022 21:06
@Nutomic
Copy link
Member Author

Nutomic commented May 4, 2022

This should be good to merge.

@dessalines
Copy link
Member

Okay, but we gotta remember to check the federation test instances before we do our next deploy to make sure it didn't break websocket.

@dessalines dessalines merged commit b77689e into main May 6, 2022
@dessalines
Copy link
Member

Nevermind, I'm reverting this again as it broke development over a local network.

dessalines added a commit that referenced this pull request May 13, 2022
@dessalines
Copy link
Member

Actually there's another way to do it, I'll include that with another PR.

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.

2 participants