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

Make tool_tip Trusted Types compatible #1717

Merged

Conversation

KyFaSt
Copy link
Contributor

@KyFaSt KyFaSt commented Dec 19, 2022

Description

This change makes primer view_components compatible with the CSP directive Trusted Types. This CSP directive allows developers to mark a value as a Trusted Type, usually this would be done in conjunction with running some type of sanitizer like DOMPurify to ensure the value doesn't contain any unsafe elements. Fortunately, view_components doesn't have major violations, just this one but unfortunately the change in this PR does not buy any security benefits, it just adheres to the Trusted Types API -- not passing bare strings directly to potentially dangerous injection sinks. Currently this implementation is the best way to make this library compatible with trusted types.

Integration

Does this change require any updates to code in production?

No, this should stay functionally equivalent.

Merge checklist

I omitted these because I don't think this change should affect tests or documentation

- [ ] Added/updated tests
- [ ] Added/updated documentation
- [ ] Added/updated previews

* this change doesn't actually add any security to primer, it effectively
  just adheres to the trusted types API by not passing bare strings directly
  to
@KyFaSt KyFaSt requested review from a team and keithamus December 19, 2022 21:33
@changeset-bot
Copy link

changeset-bot bot commented Dec 19, 2022

🦋 Changeset detected

Latest commit: 879b8f9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@camertron camertron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @KyFaSt, thanks for this! Would you mind adding a changeset? Just run npx changeset and follow the prompts (we only bump the patch version for PVC right now).

@jonrohan
Copy link
Member

@keithamus @khiga8 I'm curious if there was any reason we did innerHTML here before?

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

innerHTML is just a bit neater. This change is 👍 though

@jonrohan jonrohan enabled auto-merge (squash) December 29, 2022 17:08
@jonrohan jonrohan merged commit da56d34 into primer:main Dec 29, 2022
@primer-css primer-css mentioned this pull request Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants