Skip to content

Commit 62b49b9

Browse files
easonliang28easonliang28
committed
fix(webview): validate the webview-supplied view state id before adopting it
The stable view state id arrives over the webview postMessage channel, so the declared string type is not a guarantee: a crafted message could carry a non-string value (which the sanitizer would crash on), an unbounded string (unbounded growth in the shared viewStates map), or an id another live view already owns (adopting it would load that view's pinned profile, overlay and mode here, and let this webview overwrite that view's persisted entry). setViewStateId now rejects non-strings, bounds the key length, and refuses ids claimed by another live instance; a refused view stays on its temporary id and keeps working against the shared state, so viewStateReadiness is resolved on every refusal path to keep API-driven task creation off its timeout. The spec gains an afterEach that clears the static instance registry: the suite constructs providers directly and never disposes most of them, and a leaked instance would otherwise claim ids ahead of later tests. Proofs: ClineProvider.sticky-mode.spec 33/33 (3 new tests: non-string, oversized, claimed-by-another-view); negative controls: disabling the ownership check reddens exactly the claim test, widening the length bound reddens exactly the oversized test, restores byte-exact (hash match); eslint --max-warnings=0 clean; tsc clean except the two known local telemetry-dist noise lines; prettier ALL-MATCHED on touched files.
1 parent 7d12560 commit 62b49b9

2 files changed

Lines changed: 87 additions & 1 deletion

File tree

‎src/core/webview/ClineProvider.ts‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,7 @@ export class ClineProvider
216216
private static activeInstances: Set<ClineProvider> = new Set()
217217
private static nextViewId = 0
218218
private static readonly MAX_PERSISTED_VIEW_STATES = 50
219+
private static readonly MAX_VIEW_STATE_ID_LENGTH = 64
219220
private static persistedViewStateWriteQueue: Promise<void> = Promise.resolve()
220221
private disposables: vscode.Disposable[] = []
221222
private webviewDisposables: vscode.Disposable[] = []
@@ -789,7 +790,21 @@ export class ClineProvider
789790
* The identifier is sanitized so it remains a safe object key in the shared viewStates map.
790791
*/
791792
public async setViewStateId(viewStateId: string | undefined): Promise<void> {
792-
const normalizedViewStateId = viewStateId?.trim().replace(/[^A-Za-z0-9_-]/g, "_")
793+
// The webview generates this id on purpose - it has to survive a webview reload,
794+
// which recreates the view on both sides - so the host cannot mint it instead.
795+
// What the host owns is which view may hold a given persisted key, so everything
796+
// below validates the supplied value before it becomes one.
797+
if (typeof viewStateId !== "string") {
798+
// A crafted postMessage can carry any JSON value here even though the message
799+
// type declares a string; the persisted key must stay a string.
800+
this.log(`[setViewStateId] Ignoring a non-string viewStateId for view ${this.viewId}.`)
801+
// The view falls back to shared state; do not leave API-driven task creation
802+
// waiting on a registration that will not happen.
803+
this.viewStateReadinessResolve()
804+
return
805+
}
806+
807+
const normalizedViewStateId = viewStateId.trim().replace(/[^A-Za-z0-9_-]/g, "_")
793808

794809
if (
795810
!normalizedViewStateId ||
@@ -806,6 +821,35 @@ export class ClineProvider
806821
return
807822
}
808823

824+
// Bound the key: the webview generates a UUID, so a much longer value is not a
825+
// view id, and an unbounded key is unbounded growth in the shared viewStates map.
826+
if (normalizedViewStateId.length > ClineProvider.MAX_VIEW_STATE_ID_LENGTH) {
827+
this.log(
828+
`[setViewStateId] Ignoring an oversized viewStateId (${normalizedViewStateId.length} chars) for view ${this.viewId}.`,
829+
)
830+
// The view falls back to shared state; do not leave API-driven task creation
831+
// waiting on a registration that will not happen.
832+
this.viewStateReadinessResolve()
833+
return
834+
}
835+
836+
const claimant = ClineProvider.getAllInstances().find(
837+
(instance) => instance !== this && instance.viewStateId === normalizedViewStateId,
838+
)
839+
if (claimant) {
840+
// Ownership check: a key another live view holds must not be adopted. Doing so
841+
// would load that view's pinned profile, nested overlay, and mode into this
842+
// provider, and this webview could then overwrite that view's persisted entry.
843+
// Staying on the temporary id keeps this view working against the shared state.
844+
this.log(
845+
`[setViewStateId] Refusing viewStateId ${normalizedViewStateId} for view ${this.viewId}: already claimed by live view ${claimant.viewId}.`,
846+
)
847+
// The view falls back to shared state; do not leave API-driven task creation
848+
// waiting on a registration that will not happen.
849+
this.viewStateReadinessResolve()
850+
return
851+
}
852+
809853
const previousViewStateId = this.viewStateId
810854

811855
this.viewStateId = normalizedViewStateId

‎src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,8 @@ vi.mock("@roo-code/cloud", () => ({
121121
get instance() {
122122
return {
123123
isAuthenticated: vi.fn().mockReturnValue(false),
124+
on: vi.fn(),
125+
off: vi.fn(),
124126
}
125127
},
126128
},
@@ -221,6 +223,13 @@ describe("ClineProvider - Sticky Mode", () => {
221223
}
222224
}
223225

226+
afterEach(() => {
227+
// Tests construct providers directly and the suite never disposes most of
228+
// them; clear the static registry so a leaked instance cannot claim a stable
229+
// view state id ahead of a later test.
230+
ClineProvider["activeInstances"] = new Set()
231+
})
232+
224233
beforeEach(async () => {
225234
vi.clearAllMocks()
226235

@@ -1736,4 +1745,37 @@ describe("ClineProvider - Sticky Mode", () => {
17361745
expect(states["stable-test-view"]).toBeUndefined()
17371746
})
17381747
})
1748+
1749+
describe("setViewStateId input validation", () => {
1750+
it("ignores a non-string viewStateId from a crafted message", async () => {
1751+
const before = provider["viewStateId"]
1752+
// A crafted postMessage can carry any JSON value despite the declared type.
1753+
await provider.setViewStateId(42 as unknown as string)
1754+
expect(provider["viewStateId"]).toBe(before)
1755+
})
1756+
1757+
it("ignores an oversized viewStateId", async () => {
1758+
const before = provider["viewStateId"]
1759+
await provider.setViewStateId("x".repeat(200))
1760+
expect(provider["viewStateId"]).toBe(before)
1761+
})
1762+
1763+
it("refuses a viewStateId already claimed by another live view", async () => {
1764+
const other = new ClineProvider(
1765+
mockContext,
1766+
mockOutputChannel,
1767+
"sidebar",
1768+
new ContextProxy(mockContext),
1769+
new WebviewFocusTracker(),
1770+
)
1771+
try {
1772+
await other.setViewStateId("claimed-by-other-view")
1773+
await provider.setViewStateId("claimed-by-other-view")
1774+
expect(provider["viewStateId"]).not.toBe("claimed-by-other-view")
1775+
expect(other["viewStateId"]).toBe("claimed-by-other-view")
1776+
} finally {
1777+
await other.dispose()
1778+
}
1779+
})
1780+
})
17391781
})

0 commit comments

Comments
 (0)