-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat(CSP): add 'trusted-types' CSP directive support #58
base: master
Are you sure you want to change the base?
Conversation
add possibility to specify 'trusted-types' and 'require-trusted-types-for' CSP directives re juunas11#57
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR; I do have a couple suggestions for improvements.
@@ -81,6 +81,11 @@ public class CspBuilder | |||
/// Setups up rules for controlling when subresource integrity must be used | |||
/// </summary> | |||
public CspRequireSriBuilder RequireSri { get; } = new CspRequireSriBuilder(); | |||
|
|||
/// <summary> | |||
/// Setups up rules for controlling when to restrict the creation of Trusted Types policies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "Setups up" -> "Sets up". I do notice this typo is in existing code as well :\
It could be good to link to the MDN documentation here.
return this; | ||
} | ||
|
||
public CspTrustedTypesBuilder RequireTrustedTypesForScript() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function needs a comment explaining what effect it has.
/// Allow CSS from the given | ||
/// <paramref name="policyName"/>. | ||
/// </summary> | ||
/// <param name="policyName">A valid policy name consists only of alphanumeric characters, or one of "-#=_/@.%". </param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a valid set of characters, should we validate it?
|
||
/// <summary> | ||
/// Instructs user agents to control the data passed to DOM XSS sink functions, like Element.innerHTML setter. | ||
/// Those functions only accept non-spoofable, typed values created by Trusted Type policies, and reject strings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment could use an MDN link as well.
|
||
|
||
[Fact] | ||
public void RequireTrustedTypes_ReturnsCorrectHeader() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more unit tests would be nice. At least one with a specific policy name and one that sets up the require-trusted-types-for directive.
Any chance of reviving this? |
add possibility to specify 'trusted-types' and 'require-trusted-types-for' CSP directives
re #57