Remove duplicated Content-Security-Policy meta headers (already exists in HTTP headers).

[dbaker-arch]

Description

We already use the helmet module to define Content-Security-Policy as HTTP server response headers. This PR removes the duplication of this information in HTTP meta headers.

See also #385 where the helmet defaults are adjusted.

Change Visualization

I've tested the element in frontend/index.html

I have not tested the change in frontend/electron.main.js as I don't use that part of the code.

[ravjotbrar]

Thanks for the PR! The reasoning makes sense for Web/Docker mode where helmet provides CSP via HTTP headers, but this would remove CSP enforcement entirely from the desktop (Electron) app.

The session.defaultSession.webRequest.onHeadersReceived block in electron.main.js is also effectively dead code for the packaged app (there are no HTTP responses to attach headers to when loading from file://), so removing that part is fine.

[dbaker-arch]

Ah! I see what you're saying.

Thank you looking into the details. I'm quite unfamiliar with the details of running something as a desktop/electron app.

The onHeadersReceived is dead code and should be removed.

The meta header in index.html is duplicated for "regular" use since helmet does this for you, but is necessary when running inside electron because the helmet functionality is not included in that path.

I'll revert that change with a comment.

--> update. Are you sure onHeadersReceived is dead? Looking into this I found this page -- https://www.electronjs.org/docs/latest/tutorial/security#csp-http-headers -- that suggests using it to set the headers.

We could simplify this by inverting the last change; keeping onHeadersReceived and removing the index.html.

What's kind of clear is that I don't know electron details, so maybe it's best we close this PR entirely and someone familiar with it can start fresh?

[ravjotbrar]

LGTM with the changes, just need the merge conflicts resolved.

[dbaker-arch]

Updated per the discussion. electron.main.js's header manipulation should be enough to protect running as electron app. the index.html meta header is superfluous when running in server mode with the helmet module.

@ravjotbrar Are you able to test to confirm electron functionality before we proceed with merging please?

[ravjotbrar]

I believe there may have been a miscommunication on my part. We'll need to reinstate the meta tag, as it's the preferred method when loading a resource using the file:// protocol (electron app). After that, I'll test manually and we can get this PR across the line :)

[dbaker-arch]

I'm in the awkward position of trying to write a PR that I have no way of testing myself.

From my brief reading the "session.defaultSession.webRequest.onHeadersReceived((details, callback)" should protect electron apps in place of the meta header.

I'm going to suggest the best course of action is for me to close this PR and leave it un-merged. Someone who can test the electron side can see if either one, or both of the protections operate for electron and create the smallest PR that achieves the goal.