JS DOM-safe rendering: stop string-concat HTML into the toolbar iframe#1074
Merged
Conversation
The toolbar iframe is loaded same-origin with the host app, so any HTML
injection inside it is effectively XSS against the app being developed.
Several panels were composing HTML by string concatenation from data that
crosses the toolbar's trust boundary (cross-origin response headers,
attacker-influenced URLs, raw composer error bodies). Replace each sink
with DOM construction via `document.createElement` / jQuery `.text()` /
`.append()`.
* HistoryPanel: build each row by parsing the template once, substituting
only the (sanitized) UUID `{id}` into the href/data-request attributes,
and populating method/status/type/url/time via `.text()`. Replaces the
prior pattern that interpolated all six placeholders raw into HTML and
parsed the result with `.after(string)`. Also rewrites the
`[data-request=...]` selector as a `.filter()` predicate so a runtime
request id can't slip into selector syntax.
* CachePanel / RequestPanel `addMessage`: switch
`$(`<p>${text}</p>`)` to `$('<p>').text(text)`.
* PackagesPanel: assemble the success / error output as jQuery-built
elements with `.text()`, and `showMessage()` clears the terminal with
`.empty().append(...)` rather than `.html(string)`. Fixes a pre-existing
bug along the way: the error callback was passing the jQuery `textStatus`
argument to `buildErrorMessage` (which expected an XHR object), so every
non-2xx response was previously a silent JSON.parse throw. The error
builder now takes the jqXHR object directly and falls back to
`responseText` / `statusText` / a literal "Request failed".
* postMessage handlers in `inject-iframe.js` and `Toolbar.onMessage` now
reject any message whose origin is not same-origin and whose source is
not the expected counterpart window (the iframe contentWindow on the
parent side, `window.parent` on the iframe side). Today only resize and
XHR-counter messages flow, so the practical impact of the missing check
is low, but the one-line hardening keeps the protocol safe as it grows.
ESLint output for the touched files is clean; the remaining lint
warnings/errors (Turbo undefined, unnamed function expressions in Toolbar
and inject-iframe, Keyboard.js return) are pre-existing on 5.x.
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.
Summary
Third followup PR from the 5.x audit -- the JS hygiene pass.
The toolbar iframe is loaded same-origin with the host app, so any HTML injection inside it is effectively XSS against the app being developed (cookies, CSRF tokens, in-flight session). Several panels were composing HTML by string concatenation from data that crosses the toolbar's trust boundary (cross-origin response
Content-Typeheaders, attacker-influenced URLs, raw composer error bodies). Each sink is rewritten to construct DOM viadocument.createElement/ jQuery.text()/.append()instead.What changed
HistoryPanel.js -- previously the per-XHR row was built by string-replacing six placeholders (
{id},{time},{method},{status},{type},{url}) into raw HTML and inserting the result with.after(htmlString).params.typeis the responseContent-Typeof any cross-origin URL the dev's app fetches and is therefore attacker-controlled;params.urlandparams.methodare similarly untrusted. New flow: substitute only the (sanitized) request UUID{id}into the href anddata-requestattributes, then populate the five visible spans via jQuery.text(). The[data-request=...]highlight selector at the bottom ofinit()is rewritten as a.filter(fn)predicate so a runtime id can't slip into selector syntax.CachePanel.js / RequestPanel.js --
$(${text}
)->$('<p>').text(text). Same UX, no HTML parsing.PackagesPanel.js -- the success / error output is assembled as jQuery-built elements with
.text(), andshowMessage()writes them via.empty().append(...)rather than.html(string). Fixes a pre-existing bug along the way: theerrorcallback was passing jQuery'stextStatusstring tobuildErrorMessage(which expected the jqXHR object), so every non-2xx response was previously a silentJSON.parsethrow with no visible message. The new error builder takes the jqXHR object directly and falls back toresponseText/statusText/ a literal"Request failed"-- credit to codex for catching the regression.postMessage handlers in
inject-iframe.js(parent side) andToolbar.onMessage(iframe side) now reject any message whose origin is not same-origin and whose source is not the expected counterpart window (iframe.contentWindowon the parent,window.parenton the iframe). Today only resize and XHR-counter messages flow, so the practical impact of the missing check is low, but the one-line hardening keeps the protocol safe as it grows.Verification
ESLint output for the touched files is clean; the remaining warnings/errors (
Turboundefined, unnamed function expressions in Toolbar and inject-iframe, Keyboard.js return) are pre-existing on 5.x and untouched by this PR. PHP test suite, PHPStan, and PHPCS all green.