fix: prevent stored XSS via SQL autocomplete (GHSA-vjfq-fvfc-3vjw)#41760
Merged
Conversation
The SQL hint renderer wrote the raw completion text (a database table or column identifier) directly to innerHTML, allowing an authenticated workspace Developer to execute arbitrary JavaScript in another member's browser by creating a table with an HTML payload as its name. Switches the assignment to textContent, matching CodeMirror 5's default safe hint rendering. Adds Jest coverage that fails without the fix: table-name, SVG-payload, and composite table.column variants. No data-pipeline change: identifiers remain raw throughout Redux, the selector, and plugin structure caches, because users legitimately need the exact name to write queries. Fixes APP-15167 Ref: GHSA-vjfq-fvfc-3vjw
…-fvfc-3vjw defense-in-depth) The keyword branch of the tern completion renderer assigned `data.displayText` directly to `element.innerHTML`. Today the label is bounded to JavaScript keyword literals via the surrounding switch, so it is not independently exploitable — but it matches the same sink pattern that GHSA-vjfq-fvfc-3vjw exploited in the SQL hint renderer, so we eliminate the pattern from the autocomplete subsystem. Introduces `renderKeywordHint`, a single shared helper that writes the label via `textContent` and mirrors it onto the `keyword` attribute. Replaces the inline tern renderer with a call to the helper; adds coverage for the helper (text-only rendering, HTML/SVG payloads, empty label). Ref: APP-15167, GHSA-vjfq-fvfc-3vjw
…q-fvfc-3vjw defense-in-depth)
`getCompletionsForKeyword` built CodeMirror completion entries whose
render closures wrote their label to `element.innerHTML`. The assigned
strings are either hardcoded literals ("forin", "forof", "try-catch") or
`completion.text`, and the caller's outer switch restricts
`completion.text` to a fixed set of JS keywords, so there is no current
exploit path. The sink pattern, however, matches the one
GHSA-vjfq-fvfc-3vjw exploited in the SQL hint renderer, so we eliminate
it from the autocomplete subsystem.
Refactors the 13 duplicated inline renderers to call the shared
`renderKeywordHint(element, label, description)` helper. Extends the
helper signature so the CSS `content: attr(keyword)` suffix continues to
show the descriptive label ("For Loop", "While Statement", …) that
users see next to each hint — this is a pure pass-through refactor with
no visual change.
Adds table-driven coverage over every keyword variant plus regression
tests that pin the keyword/label pair for each of the 9 callers.
Ref: APP-15167, GHSA-vjfq-fvfc-3vjw
CodeMirror's typed render signature is `(element: HTMLLIElement, data: Hints, cur: Hint) => void`, but the production closures only consume the element. The initial regression tests cast through `HTMLElement` directly, which tripped a non-overlap error in `yarn run check-types`. Route every production-render invocation through a single-arg `(el: HTMLElement) => void` cast so the tests compile cleanly without changing what they assert. No production change.
Contributor
WalkthroughSecurity patch introducing centralized safe hint rendering across CodeMirror autocomplete system. Changes replace unsafe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/client/src/utils/autocomplete/keywordCompletion.test.ts`:
- Around line 94-119: The test never actually passes the payload to the renderer
because forgedOuterCompletion.text is overwritten to "for" before calling
getCompletionsForKeyword; to make the test honest, keep the payload as the
completion.text (set forgedOuterCompletion.text = payload) so the inner
renderers close over the real payload, and drive the switch that selects the
keyword branch by providing an explicit override for the keyword (e.g. add a
temporary property like forgedOuterCompletion.keywordName = "for" or call
getCompletionsForKeyword with an explicit keyword argument if available) so
getCompletionsForKeyword still picks the "for" branch while the renderer uses
the payload; alternatively, if you cannot force the branch selection externally,
remove this redundant test since the it.each already covers the keyword
branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28daed9e-ba61-462e-9357-8fe03717f648
📒 Files selected for processing (7)
app/client/src/components/editorComponents/CodeEditor/hintHelpers.test.tsapp/client/src/components/editorComponents/CodeEditor/hintHelpers.tsapp/client/src/utils/autocomplete/CodemirrorTernService.tsapp/client/src/utils/autocomplete/keywordCompletion.test.tsapp/client/src/utils/autocomplete/keywordCompletion.tsapp/client/src/utils/autocomplete/keywordHintRenderer.test.tsapp/client/src/utils/autocomplete/keywordHintRenderer.ts
Comment on lines
+94
to
+119
| it("does not parse an HTML payload when a spoofed completion is rendered", () => { | ||
| // Build a completion whose outer `text` is the payload, then pass it | ||
| // through `getCompletionsForKeyword` with `keywordName` forced to a | ||
| // valid JS keyword so the switch matches. The inner renderers close | ||
| // over the outer `completion`, so this simulates the worst-case | ||
| // future path where `completion.text` reaches the sink. | ||
| const payload = '<img src=x onerror="window.__xssFired=true">'; | ||
| const forgedOuterCompletion = { | ||
| ...stubCompletion(payload), | ||
| text: "for", | ||
| } as unknown as Completion<TernCompletionResult>; | ||
|
|
||
| // Patch `text` on the stub so `keywordName = completion.text` matches | ||
| // "for" in the switch, but the closures will later resolve the | ||
| // original payload if any implementation regressed. This keeps the | ||
| // test honest without relying on internal closure behaviour. | ||
| forgedOuterCompletion.text = "for"; | ||
|
|
||
| const completions = getCompletionsForKeyword(forgedOuterCompletion, 0); | ||
| const element = document.createElement("li"); | ||
|
|
||
| (completions[0]?.render as unknown as (el: HTMLElement) => void)?.(element); | ||
|
|
||
| expect(element.querySelector("img")).toBeNull(); | ||
| expect(element.children.length).toBe(0); | ||
| }); |
Contributor
There was a problem hiding this comment.
The "spoofed completion" test doesn't actually exercise the payload path it claims to.
Line 103 sets text: "for" via spread, line 110 reassigns it again to "for", so by the time getCompletionsForKeyword runs, completion.text is just "for" and the renderers close over that. The payload string is never passed to a renderer, making the querySelector("img") assertion trivially true regardless of whether the sink regressed.
If the intent is to simulate a regression where completion.text reaches the sink, keep the payload as text and drive the switch via a separate parameter — otherwise consider dropping the test since the it.each above already covers all keyword branches.
♻️ One way to make the test honest
- const payload = '<img src=x onerror="window.__xssFired=true">';
- const forgedOuterCompletion = {
- ...stubCompletion(payload),
- text: "for",
- } as unknown as Completion<TernCompletionResult>;
-
- // Patch `text` on the stub so `keywordName = completion.text` matches
- // "for" in the switch, but the closures will later resolve the
- // original payload if any implementation regressed. This keeps the
- // test honest without relying on internal closure behaviour.
- forgedOuterCompletion.text = "for";
-
- const completions = getCompletionsForKeyword(forgedOuterCompletion, 0);
+ const payload = '<img src=x onerror="window.__xssFired=true">';
+ // Keep `text` as the payload so any renderer that reads
+ // `completion.text` and regresses to innerHTML would execute it.
+ // `keywordName` matching is bypassed by monkey-patching via a proxy
+ // of `completion.text` read once at switch time.
+ const outer = stubCompletion(payload);
+ let reads = 0;
+ Object.defineProperty(outer, "text", {
+ get() {
+ return reads++ === 0 ? "for" : payload;
+ },
+ });
+
+ const completions = getCompletionsForKeyword(outer, 0);
const element = document.createElement("li");
(completions[0]?.render as unknown as (el: HTMLElement) => void)?.(element);
expect(element.querySelector("img")).toBeNull();
expect(element.children.length).toBe(0);
+ // `@ts-expect-error` test probe
+ expect(window.__xssFired).toBeUndefined();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("does not parse an HTML payload when a spoofed completion is rendered", () => { | |
| // Build a completion whose outer `text` is the payload, then pass it | |
| // through `getCompletionsForKeyword` with `keywordName` forced to a | |
| // valid JS keyword so the switch matches. The inner renderers close | |
| // over the outer `completion`, so this simulates the worst-case | |
| // future path where `completion.text` reaches the sink. | |
| const payload = '<img src=x onerror="window.__xssFired=true">'; | |
| const forgedOuterCompletion = { | |
| ...stubCompletion(payload), | |
| text: "for", | |
| } as unknown as Completion<TernCompletionResult>; | |
| // Patch `text` on the stub so `keywordName = completion.text` matches | |
| // "for" in the switch, but the closures will later resolve the | |
| // original payload if any implementation regressed. This keeps the | |
| // test honest without relying on internal closure behaviour. | |
| forgedOuterCompletion.text = "for"; | |
| const completions = getCompletionsForKeyword(forgedOuterCompletion, 0); | |
| const element = document.createElement("li"); | |
| (completions[0]?.render as unknown as (el: HTMLElement) => void)?.(element); | |
| expect(element.querySelector("img")).toBeNull(); | |
| expect(element.children.length).toBe(0); | |
| }); | |
| it("does not parse an HTML payload when a spoofed completion is rendered", () => { | |
| const payload = '<img src=x onerror="window.__xssFired=true">'; | |
| // Keep `text` as the payload so any renderer that reads | |
| // `completion.text` and regresses to innerHTML would execute it. | |
| // `keywordName` matching is bypassed by monkey-patching via a proxy | |
| // of `completion.text` read once at switch time. | |
| const outer = stubCompletion(payload); | |
| let reads = 0; | |
| Object.defineProperty(outer, "text", { | |
| get() { | |
| return reads++ === 0 ? "for" : payload; | |
| }, | |
| }); | |
| const completions = getCompletionsForKeyword(outer, 0); | |
| const element = document.createElement("li"); | |
| (completions[0]?.render as unknown as (el: HTMLElement) => void)?.(element); | |
| expect(element.querySelector("img")).toBeNull(); | |
| expect(element.children.length).toBe(0); | |
| // `@ts-expect-error` test probe | |
| expect(window.__xssFired).toBeUndefined(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/client/src/utils/autocomplete/keywordCompletion.test.ts` around lines 94
- 119, The test never actually passes the payload to the renderer because
forgedOuterCompletion.text is overwritten to "for" before calling
getCompletionsForKeyword; to make the test honest, keep the payload as the
completion.text (set forgedOuterCompletion.text = payload) so the inner
renderers close over the real payload, and drive the switch that selects the
keyword branch by providing an explicit override for the keyword (e.g. add a
temporary property like forgedOuterCompletion.keywordName = "for" or call
getCompletionsForKeyword with an explicit keyword argument if available) so
getCompletionsForKeyword still picks the "for" branch while the renderer uses
the payload; alternatively, if you cannot force the branch selection externally,
remove this redundant test since the it.each already covers the keyword
branches.
ashit-rath
reviewed
Apr 24, 2026
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| self: any, | ||
| _self: any, |
Contributor
There was a problem hiding this comment.
Why did we add _? Are we not referencing this anywhere?
Collaborator
Author
There was a problem hiding this comment.
Yes, this has been found unused. I will double-check this. Is there any other concern with this PR? @ashit-rath
ashit-rath
approved these changes
Apr 24, 2026
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.
Description
Fixes a stored cross-site scripting vulnerability in the SQL autocomplete dropdown (GHSA-vjfq-fvfc-3vjw, CVSS 8.7 / High). An authenticated workspace Developer who can run DDL against a shared datasource could create a table or column whose name contains an HTML payload; when any other workspace member opened the SQL query editor and triggered autocomplete, the raw identifier was written to
innerHTMLand the payload executed in the victim's session.TL;DR — the SQL hint renderer now writes completion text via
textContentinstead ofinnerHTML. MatchinginnerHTMLsinks in the tern and JS-keyword autocomplete renderers are consolidated into a sharedrenderKeywordHinthelper so the pattern is eliminated from the autocomplete subsystem. Pure client-side change, no backend/API/migration impact, no visual regression.What changed
app/client/src/components/editorComponents/CodeEditor/hintHelpers.ts— SQL hint renderer switched frominnerHTML = texttotextContent = text(primary fix for the actively-exploitable sink).app/client/src/utils/autocomplete/keywordHintRenderer.ts(new) — singlerenderKeywordHint(element, label, description?)helper that setstextContentfor the visible label and mirrors the optional description onto thekeywordattribute (used by the existingcontent: attr(keyword)CSS rule to render the "For Loop" / "While Statement" suffix).app/client/src/utils/autocomplete/CodemirrorTernService.ts— tern keyword completion render routed through the helper (wasinnerHTML = data.displayText).app/client/src/utils/autocomplete/keywordCompletion.ts— 13 duplicate inline renderers collapsed into calls to the helper. Keyword-to-description mapping (for→ "For Loop",try→ "Try-catch Statement", …) is preserved 1:1; zero visual change.Why render-layer, not data-layer
Database identifiers must stay raw through the backend, plugin structure cache, Redux state, and the
getAllDatasourceTableKeysselector, because users legitimately need the exact name to write queries, refresh schemas, and bind widgets. The bug is missing output encoding, not over-permissive input. DDL filtering is deliberately not added — Appsmith's query editor is intentionally a full database console and restricting DDL would break schema-design apps, migration tools, and admin dashboards.Test plan — TDD driven
All three fixes were developed test-first. Jest coverage was written to fail on the vulnerable code path and then verified to pass after the fix.
hintHelpers.test.tstable.columncomposite key with<img src=x onerror=…>payloads render as text, no<img>/<svg>child created,window.__xssFiredstaysundefined. These tests fail on the unpatched code (verified in the TDD red step).keywordHintRenderer.test.tstextContentnotinnerHTML; HTML and SVG payloads do not parse; empty label does not create empty children; optional description populates thekeywordattribute correctly.keywordCompletion.test.tschildren.length === 0); descriptive labels ("For Loop", "While Statement", …) are preserved byte-for-byte after the refactor; spoofed outer completion with HTML payload never parses markup.Verification commands:
Also manually reproduced the advisory PoC (
CREATE TABLE "<svg onload=alert(1)>" (id serial primary key);in Postgres) in a local instance — alert fires onrelease, does not fire after this patch.Fixes https://linear.app/appsmith/issue/APP-15167/security-high-stored-xss-via-database-tablecolumn-names-in-sql
Ref: GHSA-vjfq-fvfc-3vjw
Warning
This is a security-advisory fix. Linear ticket APP-15167 tracks the vulnerability; the GitHub advisory link is attached to the Linear issue. Coordinate advisory publication + CVE assignment with the security team before merge.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/24854712195
Commit: 7a42f72
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 24 Apr 2026 06:35:05 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
(Coordinate via the security advisory disclosure.)
Summary by CodeRabbit
Bug Fixes
Tests