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

Fix production CSP headers #912

Closed
wants to merge 1 commit into from

Conversation

superboum
Copy link
Contributor

@superboum superboum commented May 17, 2022

Context

We are deploying Cryptpad in production behind our reverse proxy that is just... a reverse proxy.
It can not serve files or have complex headers logic, so we use the standalone node.js process to serve our files and compute our headers.
From the CSP headers perspective, the main change with the development environment is that the sandbox URL is different from the domain URL.

Bug

Currently, there are multiple errors when generating the headers, all due to the fact that the sandbox URL is different from the domain one:

  1. The comparison sandbox !== domain is always false as we have prepended a white space in front of the domain variable two lines before but not (yet) in front of the sandbox URL. This is a benign error as we can expect the user has either not set the sandbox URL OR that they defined it to a different value.
  2. /^https:/.test(domain) is always false as we have prepended a space at the beginning of the domain variable. This is our critical bug, as we are falling back in the http case, but there is no http:// entry, so we are simply outputting the domain entry without any modification, which means in the end we are simply duplicating the https entry and we lack the wss entry, which make the checkup script fails.
  3. I don't understand why we have a wildcard on secured websockets but not on the http part. It would make sense to use the same logic on http and https.

My patch

It seems that mutating the domain variable to prepend a space breaks the least astonishment property from the developer perspective. Indeed, the following bugs are due to the fact code has been written with the assumption that this space does not exist. As a consequence, I have removed this space from the domain and sandbox variables and manually added spaces where needed.

I have simplified the websocket rewrite of the URL for the connect-src entry. It works for us in production (https://pad.deuxfleurs.fr), it should be a bit more secured, and easier to maintain.

I have fixed a comment that seemed outdated (there was no wildcard for plain text websockets, and now, there is no websocket wildcard at all).

@ghost ghost added the Configuration Something wrong with the setup label Aug 26, 2022
@yflory yflory added this to the 5.4.1 milestone Aug 17, 2023
@yflory
Copy link
Contributor

yflory commented Aug 18, 2023

This will be fixed in the upcoming version 5.4.1 with this commit along with an inconsistency issue introduced more recently.

@yflory yflory closed this Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Configuration Something wrong with the setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants