-
Notifications
You must be signed in to change notification settings - Fork 170
chore: Added Trusted Types to iconSVG #2574
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2574 +/- ##
=======================================
Coverage 95.79% 95.79%
=======================================
Files 741 741
Lines 20282 20282
Branches 6896 6887 -9
=======================================
Hits 19429 19429
Misses 797 797
Partials 56 56 ☔ View full report in Codecov by Sentry. |
Did a bunch of test builds on the internal infra, this fails too many consumers because of |
I updated the branch with an additional commit to pin the Previously, I used |
Expanded the type of iconSVG in a drawers plugin to support trusted types.
Pinned the @types/react version to 16.14.40, because the newer versions move the JSX namespace, which is problematic for the consumers.
a1de601
to
ef118f0
Compare
Completed several dry-runs (6988134134, 6988133849, 6988133252) the change seems compatible. But still not buying this change. See below: |
"@types/react-dom": "^16.9.14", | ||
"@types/react-resizable": "^1.7.4", | ||
"@types/react-router": "^5.1.18", | ||
"@types/react-router-dom": "^5.3.2", | ||
"@types/react-transition-group": "^4.4.4", | ||
"@types/trusted-types": "^2.0.7", |
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 dependency is problematic. It is supposed to be a bult-in dom.d.ts
type, but temporarily it exists as a library.
When it becomes built-in, our consumers may have problems upgrading to the latest typescript because of that.
Is type-safety here really that much important, or can we get away with type casting
iconSvg: trustedPolicy.createHTML(svgHtml) as any as string,
After investigating a bit further, you're right. I'm not sure I can make to 100% working correctly with all the consumers. I guess I can go back to it, once the trusted types related types (incl. |
I expanded the type of
components/src/internal/plugins/controllers/drawers.ts
to also support the Trusted Types API. In short, Trusted Types is a browser API, which allows the developers to write the code with a better awareness of the injection sinks (e.g.,dangerouslySetInnerHTML
in React) usage. With the Trusted Types enforcement enabled, each injection sinks requires the usage of a trusted object, which can have a sanitization or other kinds of allowlisting in place.With this new change, the
iconSvg
inDrawerConfig
supportsstring | TrustedHTML
.As a result, the developer experience has improved, since now they can use something as simple as:
Instead of type casting just to make it compliant:
The already existing implementation of
AppLayout
implemented incomponents/src/app-layout/runtime-api.tsx
supports the trusted types, sincedangerouslySetInnerHTML
API already accepts typestring | TrustedHTML
(types update might be required though).Related links, issue #, if available: n/a
How has this been tested?
I ran
npm test
locally.npm run test:unit
:npm run test:integ
:npm run test:a11y
:npm run test:motion
:Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.