helmetjs / helmet

Help secure Express apps with various HTTP headers
https://helmetjs.github.io/
MIT License
10.24k stars 369 forks source link

Resolves #404, updates content-security-policy README #458

Closed webketje closed 5 months ago

webketje commented 6 months ago
EvanHahn commented 6 months ago

Thanks! I'll take a look at this when I get a chance (probably not for a few weeks).

webketje commented 6 months ago

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 directiveName: null, I first expected (req, res) => null. But the code currently would then concatenate the string 'null':

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}`;
EvanHahn commented 6 months ago

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.

webketje commented 6 months ago

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

EvanHahn commented 6 months ago

Okay. I'll review it when I get a chance.

EvanHahn commented 5 months ago

Thanks for this, but I think I'm going to close. This inspired me to make some documentation changes around CSP (0710466f4aed2d6792c90dcbd53f115f26d82dd9), which should hopefully make things easier for folks.

webketje commented 5 months ago

@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

EvanHahn commented 5 months ago

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.

webketje commented 5 months ago

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