fix(studio) #5580: carry schema names in data-* attributes instead of inline onclick handlers

[robfrank]

Closes #5580

Follow-up to #5575.

Summary

Around twenty controls in studio-database.js concatenated a schema object name into an inline onclick. The name landed in three nested contexts at once - HTML attribute, JS string literal, handler argument - while only the first was escaped. The browser HTML-decodes the attribute before parsing it as JavaScript, so a name containing a double quote terminated the string early and the button became inert. Verified in Chrome with the name a"b'c`d\e<f>&g: the pre-fix spelling parses as dropType("a"b'cd\e&g")and raisesSyntaxError: missing ) after argument list. Several call sites were worse than the documented case and interpolated the name with **no escaping at all** (showTypeDetailon super/sub type links,browseType, dropProperty, dropIndex), making a crafted type name a stored-XSS vector rather than just a broken button. All such names are reachable, since CREATE DOCUMENT TYPE` accepts back-ticked names containing quotes, back-ticks and backslashes.

schemaActionAttrs(action, name, parent) is now the only place a schema name is spelled into an attribute: one HTML-escape, emitted as data-action / data-name / data-parent, read back through dataset as a plain string. Two delegated registries on document dispatch the 19 actions. Delegation beats per-container wiring here because shared renderers (renderProperties, renderIndexes, the badge builders) are injected into more than one container, and a document-level handler survives every .html() replacement. The registry doubles as the allowlist, so an unknown data-action falls through untouched and studio-security.js keeps its own [data-action='…'] handlers. Handlers read this.dataset.* rather than jQuery's .data(), which coerces values that look numeric or boolean - a type legitimately named 123 would otherwise reach quoteSqlName() as a number.

The #4087 repartition button was folded into the same mechanism and its bespoke .js-repartition-btn wiring removed, so the file now has exactly one pattern. Section-header buttons keep their inline onclick, because none carries a user-controlled name. createTimeSeriesType(), createMaterializedView() and createGraphAnalyticalView() pass no argument at all; createType(sec.key) does pass one, but sec.key is a fixed section constant from the hardcoded sections array (vertex / edge / document), never schema-derived. (Corrected in review cycle 3 - the original wording here said they all "pass no argument", which was inaccurate.)

Test plan

• cd studio && npm test - 41/41 pass, including 17 new tests in studio/test/schema-action-attributes.test.js
• Every renderer round-trips eight hostile names (a"b, a'b, a`b, a\b, a<script>…, a&b, x"); alert(1); //, it's a "type") through the attribute and back, byte-exact
• Source-level guards: every emitted data-action has a registry entry, and no registry entry is dead weight (a typo either side renders a button that silently does nothing)
• Proof the tests can fail: reverting only renderTypeLink to the old inline spelling drops the suite to 16/17; restoring it returns 17/17
• Browser verification (Chrome, jQuery 4, real dispatcher and renderers): all 22 rendered controls fired and delivered a"b'c`d\e<f>&g byte-exact, zero corrupted arguments; the same page confirms the pre-fix spelling raises SyntaxError and never fires
• Reviewer sanity check in a running Studio: create a type named a"b, open the Database tab, and use the sidebar badge, quick-action buttons, Drop Property / Drop Index, and the MV / GAV panes

Note on an existing test

studio/test/render-indexes.test.js gained two eval lines in its extraction harness, because renderIndexes now calls schemaActionAttrs (which calls escapeHtml) and the harness evaluates functions standalone. No assertion was changed.

🤖 Generated with Claude Code

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Coverage ∅ diff coverage · -6.82% coverage variation

Metric	Results
Coverage variation	✅ -6.82% coverage variation
Diff coverage	✅ ∅ diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (8a4cf19)	149642	113740	76.01%
Head commit (ae87b13)	181652 (+32010)	125680 (+11940)	69.19% (-6.82%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5634)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Review: #5580 - schema names in data-* attributes instead of inline onclick

Reviewed the diff, escapeHtml/quoteSqlName in studio-utils.js, the delegated handlers in studio-security.js, and the surviving inline handlers in studio-database.js. This is a well-scoped, well-tested fix. Nice work.

What's solid

• Root-cause fix is correct. Collapsing three nested contexts (HTML attr / JS string / handler arg) down to a single HTML-escaped data-* value read back through dataset is the right remedy. The single-quoted attributes are safe because escapeHtml escapes both " (") and ' ('), so a name like a'b can't terminate the attribute - this is the one thing that would have silently broken the whole approach, and it holds.
• Backslash round-trips correctly. escapeHtml doesn't touch \, but a backslash is inert inside an HTML attribute, so a\b reaches dataset.name byte-exact and is only escaped later by quoteSqlName for the SQL layer. Good separation of concerns.
• dataset over jQuery .data() is the right call and worth the explicit test - a type named 123 or true staying a string genuinely matters downstream.
• Argument mapping verified at every converted call site, including the two-name ones: drop-property emits data-name=property.name / data-parent=row.name and the handler calls dropProperty(d.parent, d.name) = dropProperty(type, property), matching the original; drop-index preserves (index.name, row.name) order. Easy to get backwards, done right.
• No collision with studio-security.js. Its handlers bind on specific [data-action='edit-user'] selectors and its action names (edit-user, delete-user, delete-token, edit-group, delete-group) don't overlap the schema registry, so the broad [data-action] handler falls through (and importantly does not preventDefault) for them. Unknown actions are inert, not hijacked.
• Test coverage is excellent - hostile-name round-trips across every renderer, plus the two source-level guards (every emitted action has a registry entry / no dead registry entry) that catch typos a runtime test never would. The documented "revert one renderer -> 16/17" falsification check is exactly the right rigor.

Minor notes (non-blocking)

1. Dead id on the repartition button (studio-database.js showTypeDetail): with .js-repartition-btn gone, btnId is still computed and emitted as id='...' but nothing references it anymore. Harmless, but could be dropped along with the orphaned wiring for a cleaner diff.
2. renderMaterializedViewsSidebarSection remains dead + duplicated. You already flag this in the PR doc and chose to convert-not-delete to stay focused - reasonable, just a marker for the follow-up.
3. studio-security.js still reads $(this).data("name") for user/group names, which has the same numeric-coercion footgun you fixed here. Correctly noted as out of scope; worth a small follow-up issue so it isn't forgotten.
4. Out-of-scope inline handlers confirmed benign: the remaining arg-carrying onclicks (createType(sec.key), updateDatabaseSetting, restoreBackupAction/deleteBackupAction, reference-example/timeseries helpers) all carry fixed-vocabulary or server-generated values, not user-controlled schema names, consistent with the PR's stated scope.

One thing I couldn't do

The sandbox blocked me from executing node --test, so I verified the tests by reading them rather than running them. The suite genuinely exercises the renderers (extract-and-eval, real escapeHtml), and the falsification evidence in the PR description is convincing, so I have no concern - just flagging that the 41/41 is your reported result, not one I re-ran.

Overall: correct, safe, and thoroughly tested. LGTM.

[claude]

Code Review

Reviewed the full diff (both commits). This is a high-quality, well-scoped security fix. The core insight - that an inline onclick nests three contexts (HTML attribute -> JS string literal -> handler argument) while only the first is escaped, so the browser HTML-decodes " back to " before parsing the JS and breaks the string - is correct and clearly documented. Moving to a single data-* escaping point read back through dataset is the right remedy, and consolidating ~19 call sites onto one pattern is a genuine maintainability win.

What I verified

• Escaping is sound. Attributes are emitted with single quotes and escapeHtml escapes both " -> " and ' -> ' (plus & < >), so there is no attribute breakout for any of the hostile names. Values round-trip through dataset as plain strings.
• Argument order preserved. drop-property -> dropProperty(d.parent, d.name) = dropProperty(typeName, propertyName) and drop-index -> dropIndex(d.name, d.parent) = dropIndex(indexName, typeName) both match the pre-fix call sites exactly.
• No allowlist collision with studio-security.js. Its delegated actions (edit-user, delete-user, delete-token, edit-group, delete-group) don't intersect the schema registry, so the generic [data-action] handler falls through untouched for those, as claimed.
• .dataset over jQuery .data() is the correct call - a type named 123/true stays a string. Good catch, and good that the same coercion risk in studio-security.js is flagged as an explicit follow-up rather than silently touched.
• The previously unescaped XSS sinks (showTypeDetail on super/sub-type links, browseType, dropProperty, dropIndex, GAV/MV handlers) now escape both the visible text and the attribute. This is the more serious half of the fix and it's handled.
• The now-orphaned btnId/.js-repartition-btn wiring was correctly removed in the follow-up commit.

Minor observations (non-blocking)

1. Icon clicks inside buttons rely on jQuery delegation binding this to the [data-action] element rather than event.target - which is exactly how it behaves, so <i> clicks dispatch correctly. Worth a one-line comment only if you want to guard against a future refactor to e.target.dataset.
2. stopPropagation dropped vs. the old return false. The click handler calls preventDefault() but not stopPropagation(). For <a href='#'> this is sufficient (and arguably cleaner), and there's no nested data-action element in this UI, so no double-dispatch. Fine as-is; just noting the behavioral delta.
3. Future footgun. Because the generic handler dispatches any data-action present in schemaClickActions, a new data-action value added elsewhere that happens to reuse one of these 18 names would get double-dispatched. The registry-as-allowlist comment covers the read side well; a brief note that action names are a shared global namespace would help the next contributor.
4. The change handler correctly omits preventDefault, and a click on the GAV <select> finds no entry in schemaClickActions and returns without swallowing the native dropdown - good.

Test coverage

Strong. 17 new tests round-tripping eight hostile names byte-exact, plus the source-level guards (every emitted data-action has a registry entry and no entry is dead weight) - the latter catches the class of typo bug no runtime test would. The "revert renderTypeLink -> 16/17" falsifiability check is exactly the right thing to include. The two added eval lines in render-indexes.test.js are a mechanical consequence of renderIndexes now calling schemaActionAttrs/escapeHtml and change no assertions.

The acknowledged out-of-scope items (backup-name inline handlers, studio-security.js .data() coercion, the dead renderMaterializedViewsSidebarSection) are reasonable to defer and are clearly listed.

LGTM. Nice, disciplined fix with a proof-of-failure test and a good root-cause writeup.

[claude]

Review: LGTM

Reviewed the full diff, escapeHtml, and cross-checked studio-security.js. This is a clean, correct fix with unusually strong tests. Summary below.

What's right

• Root cause is accurate. The nested-context (HTML attr -> JS string literal -> argument) analysis is correct: escapeHtml only covers context 1, and the browser HTML-decodes the attribute before parsing the JS, so a " in a name broke out of the string. Several sites (showTypeDetail super/sub links, browseType, dropProperty, dropIndex, the GAV/MV badges) interpolated with no escaping at all, so calling this a real stored-XSS fix (not just a broken-button fix) is justified.
• Single escaping + single dispatch point is the right shape. schemaActionAttrs() as the only place a name reaches an attribute, read back via dataset, collapses three contexts to one.
• dataset over jQuery .data() is a sharp call - .data() coerces "123"/"true", and a schema type genuinely named 123 would then reach quoteSqlName() as a number. Good catch, and tested.
• Argument order preserved at the two two-name sites: drop-property -> dropProperty(d.parent, d.name) = (type, property), and drop-index -> dropIndex(d.name, d.parent) = (index, type). Both match the pre-fix calls.
• No namespace collision with studio-security.js. It binds specific [data-action='edit-user']-style selectors with names (edit-user, delete-user, delete-token, edit-group, delete-group) that are disjoint from the schema registry, and the generic schema handler falls through (handler == null) for them without calling preventDefault. The added namespace comment is a good defensive note.
• Delegation on document is the correct choice given the shared renderers get injected into multiple containers and survive .html() replacement. Icon-in-button clicks resolve correctly because jQuery sets this to the delegate (the [data-action] element), not the click target. The change handler correctly omits preventDefault so the native <select> still opens.
• Tests are excellent - hostile-name round-trips, breakout assertions, and especially the source-level guards (every emitted data-action has a registry entry and vice-versa) plus the recorded falsification check. These catch the silent-typo failure mode that no runtime test would.

Minor, non-blocking

1. action param isn't escaped in schemaActionAttrs (" data-action='" + action + "'"). Safe today because every caller passes a string literal, and the every emitted data-action is covered test would flag a malformed one at CI - but a one-line comment noting "action is always a trusted literal" would document the assumption for the next contributor.
2. Description accuracy: the summary says section-header buttons "pass no argument," but createType("+sec.key+") at line 3497 does pass one. It's benign (sec.key is a fixed section constant, not schema-derived), so no code change needed - just noting the blanket statement has one exception.
3. renderMaterializedViewsSidebarSection remains dead and now byte-duplicates renderMaterializedViewsSidebarBadges. Converting-not-deleting keeps the diff focused (as stated), but deleting it in a follow-up would be cleaner.
4. The out-of-scope follow-ups you flagged (backup/settings inline handlers at lines 4000/4179-4180, and studio-security.js still using $(this).data("name") with the same coercion footgun) are correctly scoped out - worth a tracking issue for the studio-security.js one since it's the same class of bug.

Nice work - the doc, the falsification proof, and the guard tests set a high bar.

[claude]

Review: PR #5634 - carry schema names in data-* attributes instead of inline onclick

Reviewed the full diff, the surrounding call sites in studio-database.js, escapeHtml in studio-utils.js, and the [data-action] handlers in studio-security.js. This is a genuine, well-scoped security fix, not just a refactor. LGTM with a few non-blocking notes.

What is good

• Real vulnerability, correctly diagnosed. Several call sites (showTypeDetail super/sub-type links, browseType, dropProperty, dropIndex) interpolated the schema name with no escaping at all, and back-ticked CREATE DOCUMENT TYPE names genuinely reach them - so this closes a stored-XSS vector, not only a broken-button bug. The three-nested-context analysis (HTML attr / JS string / handler arg) is accurate.
• Single escaping + single dispatch point. schemaActionAttrs() is now the only place a name is spelled into an attribute; escapeHtml escapes the single quote to ' (verified at studio-utils.js:165), so the single-quoted data-* delimiters cannot be broken out of. Values read back through dataset as plain strings - one context instead of three.
• dataset over jQuery .data() is the right call and the reasoning is sound: .data() would coerce a type legitimately named 123 / true, which then reaches quoteSqlName() as the wrong type. Nice catch to guard this with a source-level test.
• Argument order preserved at the two-name sites: drop-property maps to dropProperty(parent, name) = (type, property), and drop-index maps to dropIndex(name, parent) = (index, type). Both match the pre-fix inline spelling.
• Clean removal of the bespoke feat: partition-aware planner pruning in SQL/Cypher+ partitioning integrity guardrails #4087 .js-repartition-btn wiring - grepped src/test and confirmed zero dangling references to js-repartition-btn / btnRepartition / data-type-name / dataset.typeName.
• Excellent tests. Eight hostile names round-tripped byte-exact, the revert-one-renderer falsification proof (16/17), and the two source-level guards (every emitted data-action has a registry entry, and no registry entry is dead weight) are exactly the kind of check a browserless suite cannot otherwise catch. The registry-as-allowlist doubling is elegant.

Non-blocking notes

1. Residual unescaped inline handler at studio-database.js:4004 (updateDatabaseSetting). Correctly out of the issue scope (setting keys, not schema names), but note that row[0] and row[1] are emitted with zero escaping - row[1] is the setting value, which is more free-form than the key. If any value ever contains a double quote, the link breaks the same way Studio: replace inline onclick handlers carrying schema names with data-* attributes #5580 described. Worth carrying into the follow-up you already flagged for restoreBackupAction / deleteBackupAction.

2. Dispatch wiring itself is only tested indirectly. The renderers are covered exhaustively, and the source-level test asserts the handlers read dataset, but the actual click delegation (preventDefault + dataset read then function call) has no automated test - it rests on the manual Chrome pass. Reasonable given there is no jsdom in the suite; flagging only so it is a conscious tradeoff. The change-handler-omits-preventDefault behavior (so the native <select> still opens) is subtle and easy to regress later.

3. studio-security.js still uses $(this).data("name") (lines 956-968) - the same coercion footgun this PR fixes for schema names, now living next to the corrected code. You have already noted it as a follow-up; agree it is out of scope here.

4. Minor cosmetic: in showMaterializedViewDetail, source-type links moved from an anchor with font-weight:600 to a span with font-weight:600 wrapping an anchor with class link. The bold now sits on the wrapping span and the anchor picks up .link styling it did not have before. Harmless, just noting the visual delta is intentional.

Nothing here blocks merge. The core change is correct, tested, and improves security.

Automated review by Claude. I could not run node --test in this sandbox (approval-gated); the pass/fail claims above are from reading the code and tests, and I trust the PR reported 41/41.

[claude]

Review: fix(studio) #5580 - data-* attributes instead of inline onclick

Overall: LGTM. This is a clean, well-reasoned fix that collapses three nested escaping contexts (HTML attribute + JS string literal + handler argument) down to one, and it closes a real stored-XSS vector on the call sites that previously interpolated schema names with no escaping at all (showTypeDetail super/sub links, browseType, dropProperty, dropIndex). The single-escaping-point + registry-as-allowlist design is the right shape. I verified the key correctness properties independently:

• Argument order preserved at the two-name sites. dropProperty(type, property) (studio-database.js:894) is reached as dropProperty(d.parent, d.name) with name=property.name, parent=row.name -> correct. dropIndex(indexName, type) (:931) is reached as dropIndex(d.name, d.parent) with name=index.name, parent=row.name -> correct.
• No conflict with studio-security.js. Its handlers bind to specific [data-action='edit-user']-style selectors; the new broad [data-action] handler returns early (no preventDefault) for any action not in schemaClickActions/schemaChangeActions, so the security actions (edit-user, delete-user, delete-token, ...) still reach their own handlers untouched.
• dataset over jQuery .data() is the correct call for schema names that look numeric/boolean - the regression test guarding this is a nice touch.
• Nested <i> icon clicks dispatch correctly because jQuery delegation sets this to the [data-action] ancestor, not the clicked target.
• escapeHtml not escaping backslash is fine here: backslash has no special meaning in an HTML attribute value and round-trips through dataset byte-exact.

Non-blocking observations

1. updateDatabaseSetting (studio-database.js:4004) is the same bug class, unfixed. onclick='updateDatabaseSetting("row[0]", "row[1]")' interpolates both the key and the value with zero escaping - strictly worse than several sites this PR fixes, since the value is free-form. The PR description honestly flags it as out of scope, which I respect for keeping the diff focused, but this is the strongest follow-up candidate and worth a tracking issue so it does not get lost. Same for the studio-security.js $(this).data("name") coercion footgun.

2. renderMaterializedViewsSidebarSection (:4408) is dead code - defined, never called, and now a near-exact duplicate of renderMaterializedViewsSidebarBadges. Confirmed via grep. Converting it for consistency means it now has to be maintained in lockstep with its twin. Deleting it would be cleaner than converting it, but I understand the "keep the change focused" rationale.

3. Page-wide data-action namespace. Because dispatch is bound to document, data-action values are now effectively a global namespace shared with studio-security.js. The added comment documents this well for the next contributor; no change needed.

Tests

The 17 new tests in schema-action-attributes.test.js are thorough - hostile-name round-tripping, no-breakout assertions, and (the best part) the source-level guards that every emitted data-action has a registry entry and no registry entry is dead weight. The revert-one-renderer falsification proof is exactly the right way to show the suite bites. One honest gap the description already acknowledges: the delegation/dispatch itself has no automated coverage (no jsdom in the suite), so it rests on the manual Chrome pass. Not a blocker given the constraint, but if jsdom is ever added, a dispatch test would be the natural next step.

Nice work - the writeup and the falsification discipline are exemplary.

Reviewed with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.24%. Comparing base (8a4cf19) to head (ae87b13).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5634      +/-   ##
============================================
+ Coverage     67.20%   67.24%   +0.04%     
- Complexity        0     1117    +1117     
============================================
  Files          1771     1771              
  Lines        149642   149660      +18     
  Branches      31738    31738              
============================================
+ Hits         100560   100638      +78     
+ Misses        35942    35894      -48     
+ Partials      13140    13128      -12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.