Skip to content

Commit 472d780

Browse files
cdeustclaude
andcommitted
fix(security): escape quotes in HTML attrs + mark node-id hash non-security
CodeQL alerts: - #80–83 (Medium, js/incomplete-sanitization): _esc/esc in trace.js + trace_detail.js escaped <>& but NOT quotes, so a value containing " broke out of data-file="…" / data-path="…" / data-sid="…" attributes (XSS). Both helpers now escape " → &quot; and ' → &#39; — safe in text AND quoted-attribute contexts. Also escape the (constant) data-lazy value. - #79 (High, py/weak-sensitive-data-hashing): _short_hash in workflow_graph_schema.py mints DETERMINISTIC NODE IDs (non-security). Add usedforsecurity=False to document intent + let CodeQL skip it. SHA-1 kept (not upgraded) so existing node IDs stay stable across graphs/snapshots. py compile + ruff clean; trace.js/trace_detail.js parse. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c573a3f commit 472d780

3 files changed

Lines changed: 22 additions & 5 deletions

File tree

mcp_server/core/workflow_graph_schema.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,16 @@ class WorkflowEdge(BaseModel):
116116

117117

118118
def _short_hash(value: str, width: int = 10) -> str:
119-
"""Stable, non-cryptographic short hash for ID stability across runs."""
120-
return hashlib.sha1(value.encode("utf-8")).hexdigest()[:width]
119+
"""Stable, non-cryptographic short hash for ID stability across runs.
120+
121+
``usedforsecurity=False`` (Python 3.9+) documents intent — this hash mints
122+
deterministic node IDs, it never protects sensitive data — and lets CodeQL's
123+
weak-hashing query correctly skip it. SHA-1 is retained (not upgraded to
124+
SHA-256) so existing node IDs stay stable across graphs and snapshots.
125+
"""
126+
return hashlib.sha1(value.encode("utf-8"), usedforsecurity=False).hexdigest()[
127+
:width
128+
]
121129

122130

123131
class NodeIdFactory:

ui/unified/js/trace.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -346,8 +346,13 @@
346346
}
347347

348348
function _esc(s) {
349+
// Escapes the full HTML special set INCLUDING quotes, so the result is
350+
// safe in both element text AND quoted-attribute contexts (e.g.
351+
// data-file="..."). Without the quote escapes a value containing `"`
352+
// breaks out of the attribute → injection (CodeQL js/incomplete-sanitization).
349353
return String(s == null ? '' : s)
350-
.replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;');
354+
.replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;')
355+
.replace(/"/g, '&quot;').replace(/'/g, '&#39;');
351356
}
352357

353358
// ── Trace detail panel: kind-dispatched rich info ────────────────────

ui/unified/js/trace_detail.js

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,12 @@
2323
};
2424

2525
function esc(s) {
26+
// Full HTML escape incl. quotes → safe in both text and quoted-attribute
27+
// contexts (data-path="...", data-sid="..."). Quote escapes prevent
28+
// attribute breakout (CodeQL js/incomplete-sanitization).
2629
return String(s == null ? '' : s)
27-
.replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;');
30+
.replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;')
31+
.replace(/"/g, '&quot;').replace(/'/g, '&#39;');
2832
}
2933
function shortStr(t, n) {
3034
n = n || 60;
@@ -54,7 +58,7 @@
5458
function section(title, id, bodyHtml, opts) {
5559
opts = opts || {};
5660
return '<details class="td-sec"' + (opts.open ? ' open' : '')
57-
+ (opts.lazy ? ' data-lazy="' + opts.lazy + '"' : '')
61+
+ (opts.lazy ? ' data-lazy="' + esc(opts.lazy) + '"' : '')
5862
+ (opts.path ? ' data-path="' + esc(opts.path) + '"' : '')
5963
+ (opts.sid ? ' data-sid="' + esc(opts.sid) + '"' : '') + '>'
6064
+ '<summary>' + esc(title) + '</summary>'

0 commit comments

Comments
 (0)