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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

webketje
Copy link

  • 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
@EvanHahn
Copy link
Member

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

@webketje
Copy link
Author

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
Copy link
Member

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
Copy link
Author

webketje commented May 8, 2024

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
Copy link
Member

EvanHahn commented May 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants