hackweek: Trusted Types - #122362
Draft
oioki wants to merge 14 commits into
Draft
Conversation
The CSP effective-directive help strings embedded literal <code> tags and escaped angle brackets, then rendered them with dangerouslySetInnerHTML. Translators had to carry markup, and a missing slash in the worker-src entry (<code>Worker<code>) produced nested unbalanced tags. Use tctCode() with [code:...] groups so the markup is real JSX and the translated strings hold only text. No visual change. Co-Authored-By: Claude <noreply@anthropic.com>
OrganizationLoadingIndicator read the React root's innerHTML and re-injected it, to reuse the loader the Django view rendered and avoid a layout shift on boot. Two problems followed from copying rather than moving it. The non-null assertion on `document.getElementById(ROOT_ELEMENT)` throws whenever that element is absent, which is the case in jsdom. That is why the component had no test coverage at all. Reading the root on every render also means a later render copies whatever is there at that moment. An org switch resets the store back to loading, and by then the root holds the rendered app, so the loading state rendered a frozen copy of the previous page instead of the loader. Capture the loader node at bootstrap and re-parent it with appendChild, which moves the node rather than cloning it. It is the same node, so there is nothing for React to reconcile, and re-mounting picks it back up. When no loader is present the component now renders an empty div instead of throwing. Co-Authored-By: Claude <noreply@anthropic.com>
The categorical series tooltip took ECharts' `param.marker`, an HTML string holding a styled empty span, ran it through DOMPurify and injected it with dangerouslySetInnerHTML. The tooltip is already rendered as React, so the marker can just be a styled component driven by `param.color`. Same markup as ECharts generates: inline-block, 10px circle, 4px right margin, series colour. Co-Authored-By: Claude <noreply@anthropic.com>
printCodes streamed the codes into the hidden print iframe with document.write, which needed a matching document.close() and produced a body with no structure. Append text nodes and <br> elements instead. Same rendered output, and the document.close() call is no longer necessary. Co-Authored-By: Claude <noreply@anthropic.com>
Writing to a script element's textContent is a Trusted Types injection sink, so it is rejected on pages that set `require-trusted-types-for 'script'`. Appending a text node is not, and is otherwise equivalent. Co-Authored-By: Claude <noreply@anthropic.com>
removeChildLevel replaced pruned subtrees by assigning comment markup to innerHTML, which parses a string to build a single comment node. createComment does it directly. Assigning to innerHTML is also a Trusted Types injection sink, so this path is rejected on pages that set `require-trusted-types-for 'script'`. Co-Authored-By: Claude <noreply@anthropic.com>
Nothing in the markdown pipeline emits `id` — marked has not added header ids by default since v12, and no renderer here sets one. It only survived on raw inline HTML in user-authored markdown. An attacker-chosen `id` is the DOM clobbering primitive: it shadows `window.x` and `document.x` for scripts that read globals without guarding. No gadget was found in the app, so this is hardening rather than a fix, but the attribute buys nothing. `align` is kept — markdown table syntax does generate it. Co-Authored-By: Claude <noreply@anthropic.com>
html_content is a whole HTML document. Splicing it into a div discarded everything outside <body>, including the viewport meta the email's own media query depends on, while the one <style> the inliner leaves behind applied to the debug page itself and pulled in a webfont. The reverse leaked too: the email uses generic class names like .btn and .container, which Sentry's own stylesheets also define, so app CSS filled in wherever the email had not set an inline style. The preview was showing something no mail client would render. Render it in an iframe instead, sanitized with WHOLE_DOCUMENT so the shell survives. The frame is sandboxed without allow-scripts, so the document is inert; allow-same-origin is only there to read scrollHeight for sizing. Also corrects html_content's type. It was declared TrustedHTML, but the endpoint serializes a plain string. Co-Authored-By: Claude <noreply@anthropic.com>
The partner support note is server-supplied HTML rendered straight through dangerouslySetInnerHTML with no sanitizer. Today every value is a hardcoded literal in the partnership config, so nothing untrusted reaches it, but the config documents the field as "plain HTML" and nothing enforces that. Run it through DOMPurify. `target` is added to the allowlist because the note links out to the partner's own support site and the default allowlist drops it; without that the link would silently stop opening in a new tab. Also corrects the comment above it, which claimed markdown cannot add attributes to links. That is only true of the HTML-string pipeline in utils/marked; the React <Markdown> renderer routes external links through ExternalLink, which sets target and rel. Adds a spec, which this component did not have. Co-Authored-By: Claude <noreply@anthropic.com>
Trusted Types is delivered on its own Content-Security-Policy-Report-Only header rather than through the CSP_* settings. Browsers evaluate each delivered policy independently, so this collects violations without touching the CSP we enforce — the alternative, flipping the real CSP to report-only for the rollout window, would drop XSS protection in production. Default off, and no policy is emitted unless TRUSTED_TYPES_ENABLED is set, so this change is inert until someone opts in. Requires CSP_REPORT_ONLY = False. This middleware runs before CSPMiddleware on the response path and django-csp bails when its header name is taken, so claiming that name while the CSP is itself report-only would drop the whole CSP. The middleware stands down instead, and logs once so that enabling the setting and seeing nothing is diagnosable. Co-Authored-By: Claude <noreply@anthropic.com>
Every trusted value in the app now comes from exactly one named policy, and each policy sanitizes or validates rather than passing its input through: - `dompurify` — created by DOMPurify when sanitizing with RETURN_TRUSTED_TYPE. Covers markdown and the bulk of untrusted HTML. Warmed at boot so a missing allowlist entry fails on startup rather than mid-render. - `sentry-script-url` — validates same-origin and throws otherwise. Covers serviceWorker.register(). - `sentry-bundler` — Rspack's own policy for chunk loading, with onPolicyCreationFailure: 'continue' so an unallowlisted deploy degrades instead of white-screening. Requires the report-only header from the previous commit to have any effect. Co-Authored-By: Claude <noreply@anthropic.com>
zrender writes to innerHTML in seven places, all of them clearing a container. Assigning to innerHTML is a Trusted Types injection sink, so the browser rejects it even when the value is an empty string — which makes zrender, and therefore ECharts, unusable under enforcement. Mirrors the upstream PR (ecomfe/zrender#1167). Drop this patch once that lands and ECharts picks up a release containing it. Co-Authored-By: Claude <noreply@anthropic.com>
Kept separate from the header plumbing so the two can ship independently: the plumbing is inert and safe to land any time, while enabling collection is only useful once the policies exist and only wanted once the violation stream is worth reading. Also needs CSP_REPORT_ONLY = False locally. Co-Authored-By: Claude <noreply@anthropic.com>
Catches new sinks in review rather than in a production violation report, and covers code nobody has written yet — which neither grep nor the report stream can do. It allowlists sanitizer calls and resolves `const x = sanitizeHtml(...)` through scope, so the twelve correct DOMPurify sites need no annotation. Without that it would be a churn generator rather than a guardrail. Seven sites are annotated: three that are provably safe but not statically provable, and four real TODOs (the Prism token replacement, the ECharts tooltip path, the cross-origin demo analytics script, and replay HTML parsing). Note the rule cannot police node_modules, which is where most sinks turned out to live. It complements the browser-level control rather than replacing it. Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
Bot
added
Scope: Frontend
Contributor
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
Contributor
Story previewsPreview the stories changed in this PR on the Vercel deployment:
Preview deployment: https://sentry-r0czi2owb.sentry.dev |
Contributor
📊 Type Coverage Diff
🔍 5 new type safety issues introduced
Non-null assertions (
Type assertions (
This is informational only and does not block the PR. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| _warned_about_report_only_csp = True | ||
| logger.warning( | ||
| "trusted_types.disabled_by_report_only_csp", | ||
| extra={"header": TRUSTED_TYPES_HEADER}, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hackweek spike: everything below, on one branch, to see it working end to end. Not for merge — the pieces ship as the PRs listed here.
Notion: Hackweek: Trusted Types
Already split out
idfrom the sanitizer allowlisttextContentfixNext
CodeBlock, ECharts tooltip, rrweb-player,showReportDialog, replayDOMParser