Created Missing Content Security Policy #1484
Conversation
…d to any emotion based styles. Fixed majority of the CSP style tag problems but a few still remain. (Still in Progress)
|
@CamClendenon & @pogoco26 It has been a while since the last work on this pull request. OED appreciates your work so far and OED would like to move this work forward. If you are working on this or need help then please leave a comment. If nothing is posted by 1 Aug. then OED will evaluate the work to see if it can be completed or if this PR needs to be closed. |
|
@CamClendenon & @pogoco26 This is to let you know a new team is working in this area. If you don't want them to take over this work then please let me know in the near future. Thanks. |
|
I have reviewed and tested the code in this pull request. The code is functional and addresses the CSP issue, and the documentation is sufficient. |
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
I've looked this over at the high level. I've made some comments to consider. In the big picture, I want better commenting and an understanding of what this is doing and if any issues exist. Also, how was/can it be tested to see it is working correctly. I'm not an expert here so I need help.
I also merged in the latest development and did minor changes.
I'm happy to help anyone with this.
|
|
||
| import createCache from '@emotion/cache'; | ||
|
|
||
| const nonce = (document.querySelector('script[nonce]') as HTMLScriptElement | null)?.nonce; |
There was a problem hiding this comment.
How does nonce relate to __webpack_nonce__ in index.tsx?
I think some commenting to describe what the code in this file is doing is needed.
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| const __webpack_nonce__ = (document.querySelector('script[nonce]') as HTMLScriptElement | null)?.nonce; |
There was a problem hiding this comment.
I think some commenting to describe what the new code is doing is needed.
| (window as any).__webpack_nonce__ = __webpack_nonce__; | ||
| (window as any).__plotly_nonce__ = __webpack_nonce__; | ||
|
|
||
| console.log('Set __webpack_nonce__ to:', __webpack_nonce__); |
There was a problem hiding this comment.
There are a number of console.log statements. They should go before this is finalized.
There was a problem hiding this comment.
@huss Could you clarify what this comment means? What is the issue with the placement of the console.log statements right now?
There was a problem hiding this comment.
@huss Could you clarify what this comment means? What is the issue with the placement of the console.log statements right now?
@BrianRaymond800 console.log statements are fine for debugging but should not be present in final code. OED does not want output like this in merged code. If information needs to be given then the log system should be used. I think these are for debugging so should go when no longer needed.
There was a problem hiding this comment.
@huss That makes sense. Thank you.
| throw err; | ||
| } | ||
| }; | ||
| // document.head.appendChild = function (node: any) { |
There was a problem hiding this comment.
It should be determined if this commented out code should be used or removed.
| nonce="{{nonce}}"> | ||
|
|
||
| <!-- | ||
| The meta tag with "Content-Security-Policy" below is the Content Security Policy, by having default-src as self all CSP rules that are unspecified will only |
There was a problem hiding this comment.
Some of these lines exceed 150 characters.
| a site to the exception list. Another example for adding a site (https://newException.com) to a tag with multiple sites as an exceptions would be : | ||
| img-src 'self' http://example.com https://site_example.net; becomes img-src 'self' http://example.com https://site_example.net https://newException.com; | ||
| --> | ||
| <!-- <meta http-equiv="Content-Security-Policy" content=" |
There was a problem hiding this comment.
I'm wondering how these are working given the lines are commented out. How does this relate to the code in app.js? I think some may be examples but it needs to be clarified.
| { test: /\.css$/, use: [ | ||
| {loader: 'style-loader'}, | ||
| {loader: 'style-loader', | ||
| options: { |
| const subdir = config.subdir || '/'; | ||
| let htmlPlusData = html.toString().replace('SUBDIR', subdir); | ||
|
|
||
| const nonce = crypto.randomBytes(16).toString('base64url') |
There was a problem hiding this comment.
A comment on what this is doing would help.
3 participants
Description
This update enhances security by adding a missing Content Security Policy (CSP) to the src/client/index.html file, within the header section. We have also included documentation to explain how the CSP tags function. It also explains how clients can add their sites to comply with the policy, ensuring they are not inadvertently blocked by the CSP.
The contributors to this work are @pogoco26 and @CamClendenon. We researched the issue, drafted the implementation, and wrote the documentation. This pull request is part of our Capstone project with CSUMB in collaboration with OED, to address security vulnerabilities, such as the above mentioned.
Type of change
Checklist
Limitations
The primary limitation is uncertainty regarding how this change may affect client integrations. While no issues are expected, any problems can likely be resolved by adding the client's site to the CSP's list of allowed sources.