-
Notifications
You must be signed in to change notification settings - Fork 369
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
Resolves #404, updates content-security-policy README #458
Resolves #404, updates content-security-policy README #458
Conversation
webketje
commented
Apr 24, 2024
- replaces HTML5Rocks URL with web.dev (redirect), add links to relevant MDN docs
- adds doc sections/ anchors for defaults, computed directives, disabling directives, and report only header
- clarifies that defaultSrc will default to 'self' (and is thus not required to the user) when useDefaults: true
- solves CSP: document functions as directive values #404, documents function signature and adds conditional CDN script-src loading example
- adds a common recipe to generate subresource-integrity hashes
- documents caveat of non-hostname values mentioned in 'self' and 'none' values lack quotes #454
- replaces HTML5Rocks URL with web.dev (redirect), add links to relevant MDN docs - adds doc sections/ anchors for defaults, computed directives, disabling directives, and report only header - clarifies that defaultSrc will default to 'self' (and is thus not required to the user) when useDefaults: true - solves helmetjs#404, documents function signature and adds conditional CDN script-src loading example - adds a common recipe to generate subresource-integrity hashes - documents caveat of non-hostname values mentioned in helmetjs#454
Thanks! I'll take a look at this when I get a chance (probably not for a few weeks). |
Note I am not sure about one thing: the subresource-integrity hash example supposes that returning an empty string for that directive value will result in it being omitted from the directive, I haven't validated this assertion. To be honest, given that an entire directive can be disabled by doing directiveValue += " " + (element instanceof Function ? element(req, res) : element); This is for a separate issue, but I think it would make more sense not to add a directive subvalue when its value is falsey: let subValue = element;
if (element instanceof Function) subValue = element(req, res);
if (!!subValue) directiveValue += ` ${subValue}`; |
Would you mind splitting this into smaller pull requests for easier review? No worries if not, but I'll be able to review faster if you do so. |
Sorry, that would be hard as the changes all affect the same doc and form a consistent whole. I think the easiest way to review it is to just read through https://github.com/webketje/helmet/blob/feat/%23404-csp-directives-documentation/middlewares/content-security-policy/README.md and have the PR bullet summary to the side. FWIW the hash generation recipe is validated and in use in a production environment, only the computed directive using a function hasn't been validated when an empty string is returned |
Okay. I'll review it when I get a chance. |
Thanks for this, but I think I'm going to close. This inspired me to make some documentation changes around CSP (0710466), which should hopefully make things easier for folks. |
@EvanHahn I'm a bit surprised by this decision. I resolved 2 issues and added useful, working examples so if you could help me understand why the PR was discarded altogether I would be very grateful and better able to contribute. Did I write functionally incorrect docs/ was the fix bad? Did you not like the way it read/ was worded? Were the new examples poor? Was it git history purism? Is it because it was (partially) unsolicited? Any of these is fine but give me something please |
Sorry to close this abruptly, especially after you put in some hard work writing it.
It was difficult for me to review as it had many separate pieces. I also felt that it contained some unnecessary information.
Again, apologies for the abrupt closing. I have a lot on my to-do list and I don't think I gave this the attention it deserved.
|
Thank you! I had a look at the new doc and I get it: it was refocused squarely on the middleware, and abandoned the parts "educating" users about CSP. I think that's a fair call for a README, though personally I prefer more info & examples I still think the subresource-integrity hash example deserves a spot, perhaps rather as a wiki example. I may find a way to re-include these which were left unadressed to the new README in a succinct way in targeted PRs
|