fix(skill-creator): make eval-viewer HTML escaping attribute-safe#1395
Open
griffithsbs wants to merge 1 commit into
Open
fix(skill-creator): make eval-viewer HTML escaping attribute-safe#1395griffithsbs wants to merge 1 commit into
griffithsbs wants to merge 1 commit into
Conversation
renderGrades and renderBenchmark build HTML by string concatenation and assign it via innerHTML, relying on escapeHtml at each interpolation. Two gaps let attacker-influenced data (grader "evidence" and assertion text can quote a skill-under-test's output; eval and config names are user-authored) reach the DOM as markup: - escapeHtml used the textContent->innerHTML idiom, which escapes < > & but not quotes, yet it is used inside a title="..." attribute in the per-assertion table. Evidence containing a double quote breaks out of the attribute and can attach an event handler. Escape & < > " ' explicitly so the helper is safe in attributes and text. - Several externally-sourced strings were interpolated raw (configLabel, metadata.timestamp/evals_run/runs_per_configuration, delta.*). Route them through escapeHtml too, so every externally sourced string rendered via innerHTML is escaped and only locally-computed numbers are interpolated directly. The escaped forms render identically for normal data, so there is no visible change.
|
+1 to this. I independently hit the same |
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
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.
Fixes #1394.
Problem
viewer.htmlassembles HTML by string concatenation and assigns it withinnerHTMLinrenderGradesandrenderBenchmark, relying onescapeHtml()at each interpolation. Two gaps let attacker-influenced data (grader "evidence"
and assertion text can quote a skill-under-test's output; eval and config names
are user-authored) reach the DOM as markup:
escapeHtml()was not attribute-safe. It used thetextContent→innerHTMLround-trip, which escapes<,>,&but not quotes — yet it isused inside a
title="..."attribute in the per-assertion table. Evidencecontaining a double quote breaks out of the attribute and can attach an event
handler (e.g.
x" onmouseover="alert(document.domain)).in the per-eval breakdown rows (while the same value is escaped in the header),
and
metadata.timestamp/metadata.evals_run/metadata.runs_per_configuration/ the
delta.*cells.Fix
escapeHtml()to escape&,<,>,",'explicitly, so it issafe in both element-text and attribute contexts.
every externally-sourced string rendered via
innerHTMLis escaped and onlylocally-computed numbers are interpolated directly.
The escaped forms render identically for normal data, so there is no visible
change.
Scope / related
This is the display path. The embed path —
generate_review.pyinjectingEMBEDDED_DATAinto the inline<script>without escaping</, which lets a</script>in skill output break out of the script element (#1075) — is fixedseparately in #1393. The two together harden the viewer end to end; they touch
different files and do not conflict. The xlsx preview relies on SheetJS's own
cell-text escaping and is left as-is.