Repository navigation
fix(ui): consolidate HTML escaping into one helper that escapes single quotes - #122
Merged
Merged
Conversation
…e quotes
The UI carried eleven copies of `escapeHtml`. Ten of them replaced `&`,
`<`, `>` and `"` but not `'`, and every one of those ten is interpolated
into a single-quoted JavaScript string literal inside an HTML attribute,
for example:
onclick="window.app.deleteAlias('tool', '${escapeHtml(alias)}')"
A value containing `'` closes that JavaScript string literal early, so the
remainder of the value is parsed as code. HTML escaping was applied and was
nonetheless insufficient, because it does not neutralise the quoting
character of the context the value actually lands in.
An alias, profile name, server id, task id, or capability key containing
`'); window.app.someMethod('` therefore executed arbitrary script in the
control-plane origin, where it could call any `window.app` method including
the approval and policy functions.
`components/audit.ts` already had a correct copy that escaped the single
quote. Move that implementation to `ui/src/escape.ts` and import it from
all eleven modules, deleting the ten unsound copies.
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
1 participant
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.
The UI carried eleven copies of escapeHtml. Ten replaced & < > " but not ', and all ten are interpolated into single-quoted JavaScript string literals inside HTML attributes, e.g. onclick="window.app.deleteAlias('tool', '${escapeHtml(alias)}')". A value containing ' closes that JavaScript literal early so the remainder parses as code. An alias, profile name, server id, task id or capability key containing '); window.app.someMethod(' executed script in the control-plane origin, where it could reach any window.app method including the approval and policy functions. components/audit.ts already had a correct copy escaping the single quote; that implementation moves to ui/src/escape.ts and is imported from all eleven modules. Verified both directions with a harness reproducing the aliases.ts:29 render. No committed test: the repo has no UI test runner.