Repository navigation
Use URL parsing and opener isolation for prompter popout windows #150
Description
Activity
🛠️ Picked up by /implement — driver Codex, branch
150-prompter-popout-url, 2026-05-24T03:19:49Z.gc workflow phase recorded:
preflight(issue #150). Posted by the MCP server to enforce ordering between workflow steps (issue #794 MVP-2). Do not edit or delete — used by downstream tools to gate phase prerequisites.gc workflow phase recorded:
plan(issue #150). Posted by the MCP server to enforce ordering between workflow steps (issue #794 MVP-2). Do not edit or delete — used by downstream tools to gate phase prerequisites.Implementation Plan
Issue #150 is a requirement-free bug fix. No Ground Control requirement clauses are in scope, so the acceptance contract is the issue body.
- Modify
src/system/presenter/prompter-window.tsonly at the prompter popout helper boundary. Replace manual query-string rewriting withnew URL(baseUrl, window.location.href)andurl.searchParams.set('mode', 'prompter'), then openurl.href. - Enforce the same-origin boundary before opening. If the parsed target origin differs from
window.location.origin, returnnulland do not callwindow.open; external prompter targets remain a non-goal. - Harden popup isolation by always adding
noopener,noreferrerto the feature string, while preserving caller-supplied window features, and defensively clearingopened.openerwhen a window object is returned. - Extend
tests/system/prompter-window-url.test.tsfirst to cover existingmode, query values containingmode=, hash preservation, relative URLs, absolute same-origin URLs, cross-origin rejection, feature isolation flags, and defensive opener clearing. - Add
changelog.d/150.fixed.md. Do not editCHANGELOG.mddirectly.
Security: the change touches browser URL parsing, same-origin gating, and the popup opener boundary only; it introduces no storage, config, secrets, network policy, or error envelope surface. Maintainability: keep the behavior in the existing helper and tests, with browser
URL/URLSearchParamsas the parser. Extensibility: future external targets would need an explicit allowed-origin policy instead of ad hoc string exceptions. Whole-repo view: no ADR, schema, navigation grammar, runtime prompter semantics, or presenter routing changes are needed.- Modify
gc_codex_review — cycle 1 of 1 (pre-push) on issue #150 (branch
150-prompter-popout-url)Core review
Verdict:
shipThis change is shaped correctly: the URL construction moves from ad hoc query-string rewriting to the platform URL API, and the popup isolation concern is kept local to the prompter-window boundary where
window.openis actually invoked. The same-origin gate is the right seam for this helper because the prompter is a same-app presenter mode, and the tests now pin the behavioral edge cases that motivated the refactor: embeddedmode=text, hashes, relative resolution, absolute same-origin URLs, cross-origin rejection, and opener isolation. It does not introduce a premature abstraction or foreclose the obvious next variation, such as adding more prompter-specific query params.No blocking findings.
Security review
Verdict:
shipThis change is shaped correctly as boundary hardening at the prompter-window seam: URL parsing, same-origin enforcement, query mutation, and popup opener isolation all live at the single point where presenter code crosses into
window.open. The cross-cutting concern is browser trust boundary handling, and the diff tightens the previous string-based URL rewrite without broadening the caller surface or foreclosing the obvious next variation of adding more prompter query parameters. I do not see a concrete exploitable security regression introduced by this diff.No blocking findings.
gc_codex_review pre-push cycle 1 of 1 complete for issue #150 on branch '150-prompter-popout-url'. Posted by the MCP server to enforce the pre-push hard-cap-1 contract (issues #796, #804, #906). Do not edit or delete — used by the next
gc_codex_review(uncommitted) invocation to count cycles.Review decision record — codex cycle 1 (issue #150)
Reviewer: codex
Cycle: 1Architectural read:
Core reviewer: This change is shaped correctly: the URL construction moves from ad hoc query-string rewriting to the platform URL API, and the popup isolation concern is kept local to the prompter-window boundary where
window.openis actually invoked. The same-origin gate is the right seam for this helper because the prompter is a same-app presenter mode, and the tests now pin the behavioral edge cases that motivated the refactor: embeddedmode=text, hashes, relative resolution, absolute same-origin URLs, cross-origin rejection, and opener isolation. It does not introduce a premature abstraction or foreclose the obvious next variation, such as adding more prompter-specific query params.Security reviewer: This change is shaped correctly as boundary hardening at the prompter-window seam: URL parsing, same-origin enforcement, query mutation, and popup opener isolation all live at the single point where presenter code crosses into
window.open. The cross-cutting concern is browser trust boundary handling, and the diff tightens the previous string-based URL rewrite without broadening the caller surface or foreclosing the obvious next variation of adding more prompter query parameters. I do not see a concrete exploitable security regression introduced by this diff.Blocking findings: 0 (clean run)
gc_test_quality_review cycle 1 of 1 — issue #150
Reviewer: test-quality (claude-sonnet-4-6 via gc_test_quality_review)
Branch:150-prompter-popout-url
Cycle: 1 / 1
Findings: 0 (clean run)gc_test_quality_review cycle 1 of 1 complete for issue #150 on branch '150-prompter-popout-url'. Posted by the MCP server to enforce the gc_test_quality_review hard-cap-1 contract (issue #884 follow-up, default lowered in #906). Do not edit or delete — used by the next
gc_test_quality_reviewinvocation to count cycles.Review decision record — test-quality cycle 1 (issue #150)
Reviewer: test-quality
Cycle: 1Architectural read:
The tests exercise
openPrompterWindowend-to-end through a minimal fakewindowglobal injected viaObject.defineProperty— the right approach for browser-API-dependent code under Vitest/Node. Every significant code path inprompter-window.tsis covered: URL normalization withURLSearchParams.set(including the edge case wheremode=appears inside a query value), relative-URL resolution againstwindow.location.href, the cross-origin security guard, hash preservation, feature-string deduplication, opener nullification, and the_blanktarget. Assertions are on concrete outputs — the URL string passed towindow.open, the return value ofopenPrompterWindow, and the mutated state ofopenedWindow.opener— not on mock call counts. Removing or no-op'ing any of the three internal helpers (buildPrompterUrl,withPopupIsolationFeatures,isolateOpenedWindow) would cause at least one test to fail. This is shaped correctly; I would ship it.Blocking findings: 0 (clean run)
Final report — issue #150 complete
PR: #157
Plan: #150 (comment)Prompter popout URLs now use the platform URL parser, reject cross-origin targets, and isolate the opened browsing context.
Files changed
Added:
changelog.d/150.fixed.md
Modified:
src/system/presenter/prompter-window.tstests/system/prompter-window-url.test.ts
Reviews
- codex: Cycle 1 clean, 0 findings remaining
- test-quality: Cycle 1 clean, 0 findings remaining
- sonarcloud: GitHub SonarCloud checks passed on PR fix: harden prompter popout URLs #157
Traceability reconciliation
- IMPLEMENTS / TESTS / DOCUMENTS added: 0
- Links updated: 0
- Stale links removed: 0
Requirement-free issue; PR body records IMPLEMENTS and TESTS issue traceability.
Status
- CI: ✅ green
- SonarCloud: ✅ passed
- PR ready for user review and merge.
Review finding
The prompter popout helper builds URLs with string replacement and opens a
_blankwindow without explicit opener isolation.Evidence
src/system/presenter/prompter-window.ts:15usesbaseUrl.includes('mode=')andbaseUrl.replace(/mode=[^&]*/, 'mode=prompter')instead of URL parsing.src/system/presenter/prompter-window.ts:17appends&mode=prompterto any string containing?, which places the mode parameter after#fragmentfor hash URLs.src/system/presenter/prompter-window.ts:32callswindow.open(url, '_blank', features)with default features that omitnoopener,noreferrer.Impact
URLs with query values that contain
mode=can be corrupted, and hash URLs may not actually enter prompter mode. If an external URL ever flows through this exported helper, the opened page can retainwindow.openeraccess unless the browser applies stronger defaults.Recommended fix
Use the platform URL API and explicit opener isolation.
Acceptance checks
new URL(baseUrl, window.location.href)andsearchParams.set('mode', 'prompter').noopener,noreferrerto the feature string and defensively nullopened.openerwhere possible.mode, a query value containingmode=, hash URLs, relative URLs, and absolute same-origin URLs.