Harden composition audio-bed URL validation to match production asset policy #148
Description
Activity
Picked up by /implement - driver Codex, branch
148-bed-url-policy, 2026-05-23T15:34:28Z.gc_codex_review — cycle 1 of 1 (pre-push) on issue #148 (branch
148-bed-url-policy)Core review
Verdict:
shipThis change is shaped correctly: it promotes the existing asset URL policy into a shared contract at the asset-preloader seam, then threads that policy through validation, scene loading, and runtime audio construction while continuing to use the incumbent resolveAssetUrl helper for base URL and scheme handling. The cross-cutting concern is URL resolution policy, and the design boundary is cleanly expressed as injected AssetUrlPolicy rather than duplicated validation logic. It closes the prior composition-audio-bed gap without foreclosing the obvious next variation, such as per-deployment scheme hardening or alternate base URLs.
No blocking findings.
Security review
Verdict:
shipThis change is shaped as a security-boundary tightening: it extracts the URL policy into a shared type, threads that policy through validation, scene loading, and audio construction, and resolves audio bed/source URLs before handing them to the engine. The seam is the asset URL policy, which is the right boundary for scheme and base URL enforcement; the change also preserves the existing explicit allowlist check for scene-declared audio sources. I do not see this foreclosing the next variation, such as per-deck policy or a stricter production default.
No blocking findings.
gc_codex_review pre-push cycle 1 of 1 complete for issue #148 on branch '148-bed-url-policy'. 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 #148)
Reviewer: codex
Cycle: 1Architectural read:
Core reviewer: This change is shaped correctly: it promotes the existing asset URL policy into a shared contract at the asset-preloader seam, then threads that policy through validation, scene loading, and runtime audio construction while continuing to use the incumbent resolveAssetUrl helper for base URL and scheme handling. The cross-cutting concern is URL resolution policy, and the design boundary is cleanly expressed as injected AssetUrlPolicy rather than duplicated validation logic. It closes the prior composition-audio-bed gap without foreclosing the obvious next variation, such as per-deployment scheme hardening or alternate base URLs.
Security reviewer: This change is shaped as a security-boundary tightening: it extracts the URL policy into a shared type, threads that policy through validation, scene loading, and audio construction, and resolves audio bed/source URLs before handing them to the engine. The seam is the asset URL policy, which is the right boundary for scheme and base URL enforcement; the change also preserves the existing explicit allowlist check for scene-declared audio sources. I do not see this foreclosing the next variation, such as per-deck policy or a stricter production default.
Blocking findings: 0 (clean run)
gc_test_quality_review cycle 1 of 1 — issue #148
Reviewer: test-quality (claude-sonnet-4-6 via gc_test_quality_review)
Branch:148-bed-url-policy
Cycle: 1 / 1
Findings: 1Finding 1 — [warning]
tests/runtime/audio.test.ts::createAudioService — lifecycle / cleanup (ADR-004)::mutating methods are inert after dispose; introspectors still workProblem: The test calls
service.mute(true)after disposal to verify that mute is inert, but the only assertion that could detect a bypass isexpect(fake.calls).toEqual([]). The fake engine'scallsarray tracks handle-level methods (play/stop/fade/loop/volume/rate/unload) only —engine.setMasterMute()is NOT recorded there. Removing theif (disposed) return;guard frommute()would cause the post-disposal call to reachengine.setMasterMute(true), but every assertion in the test would still pass:fake.callsstays empty,fake.createdstays at length 1, andservice.isDisposed()stays true.
Why it matters: A stale async cleanup callback that callsctx.audio.mute(false)on a disposed service would silently flip the engine's master-mute state, potentially unmuting audio for the NEXT navigation. The test purports to guard this scenario but cannot.
Fix: Afterservice.mute(true)on the already-disposed service, addexpect(fake.masterMuted()).toBe(false). TheFakeEngineinterface already exposesmasterMuted()— the assertion is a one-liner that gives the test an observable actually influenced by whethersetMasterMutewas called.gc_test_quality_review cycle 1 of 1 complete for issue #148 on branch '148-bed-url-policy'. 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 #148)
Reviewer: test-quality
Cycle: 1Architectural read:
Three-file test suite covering a well-separated audio stack.
audio.test.tsunit-testscreateAudioServiceagainst a handwritten deterministic fake engine that records every handle call with full argument lists — this is the right boundary, and the fake is rich enough to enable precise delegation assertions without a mock framework.scene-loader-audio.test.tsintegration-tests the loader's wiring of the service: signal-binding, output-policy mapping, allowedSources plumbing, unlock-gate sequencing, and bed lifecycle.validation.test.tsunit-tests the static validation pass against raw declarative inputs. None of the files touch real I/O, DOM, audio APIs, or a real HTTP layer. TheafterEachsingleton-reset fornoopAudioEngineaddresses the prior test-quality review cycle's finding. The overall shape is correct and fits the codebase vocabulary.Blocking findings: 1
Finding 1 —
one-off- ID:
F1 - Title: (no title)
- Decision: fix
- Rationale: Addressed by next cycle
- ID:
Proceeding without an over-cap test-quality review cycle per user instruction after fixing the cycle-1 finding and passing local verification (
@keplerops/pulsar@0.1.0 lint /home/atomik/src/pulsar
biome check .Checked 191 files in 202ms. No fixes applied.
@keplerops/pulsar@0.1.0 typecheck /home/atomik/src/pulsar
tsc --noEmit@keplerops/pulsar@0.1.0 test /home/atomik/src/pulsar
vitest runRUN v3.2.4 /home/atomik/src/pulsar
✓ tests/runtime/rng.test.ts (8 tests) 304ms
✓ tests/system/chrome-slots.test.ts (10 tests) 315ms
✓ tests/runtime/timeline.test.ts (98 tests) 338ms
✓ tests/system/helpers.test.ts (20 tests) 467ms
✓ tests/runtime/policy-q004-resource-cleanup.test.ts (145 tests) 462ms
✓ tests/system/chrome-extras.test.ts (5 tests) 321ms
✓ tests/runtime/policy-a006-slide-frameworks.test.ts (14 tests) 946ms
✓ PUL-A006 — live runtime independent of slide frameworks (source scan) > runtime tree (current code revision) > contains no A006 violations across the runtime-core file set 913ms
✓ tests/runtime/scrub-cue-gating.test.ts (1 test) 1075ms
✓ PUL-F017 — monotonic-forward audio cue gating > fires a cue on a forward crossing of its master time and suppresses it on a reverse crossing 1073ms
✓ tests/runtime/policy-a004-export-pipeline.test.ts (12 tests) 1151ms
✓ PUL-A004 — live runtime independent of export pipeline (source scan) > runtime tree (current code revision) > contains no A004 violations across the runtime-core file set 1119ms
✓ tests/system/presenter-bridge.test.ts (10 tests) 98ms
✓ tests/runtime/policy-q008-dom-css-accessibility.test.ts (85 tests) 1115ms
✓ PUL-Q008 — DOM/CSS accessibility (source scan) > runtime tree (current code revision) > contains no Q008 violations acrosssrc/runtime/**/*.ts712ms
✓ tests/runtime/policy-a003-rendering-libraries.test.ts (15 tests) 1207ms
✓ PUL-A003 — optional rendering libraries are scene-local (source scan) > runtime tree (current code revision) > contains no A003 violations across the runtime-core file set 1161ms
✓ tests/runtime/policy-a010-export-metadata-share.test.ts (117 tests) 1629ms
✓ PUL-A010 — live and export share scene metadata (source scan) > runtime tree (current code revision) > rule 2: zero forbidden parallel scene/composition declarations acrosssrc/**/*.ts1389ms
✓ tests/runtime/navigation.test.ts (111 tests) 89ms
✓ tests/runtime/policy-a002-audio-encapsulation.test.ts (47 tests) 148ms
✓ tests/runtime/policy-scene-trust-model-doc.test.ts (2 tests) 104ms
✓ tests/runtime/policy-a009-captions-single-source.test.ts (145 tests) 1963ms
✓ PUL-A009 — captions / prompter single source (source scan) > runtime tree (current code revision) > rule 2: zero forbidden parallel caption-schema declarations acrosssrc/**/*.ts1593ms
✓ tests/runtime/scene.test.ts (125 tests) 35ms
✓ tests/runtime/asset-preloader.test.ts (34 tests) 72ms
✓ tests/runtime/policy-a008-mode-dispatch.test.ts (80 tests) 452ms
✓ tests/runtime/policy-q003-url-state-determinism.test.ts (130 tests) 2212ms
✓ PUL-Q003 — URL state determinism (source scan) > runtime tree (current code revision) > contains no Q003 violations acrosssrc/**/*.ts2075ms
✓ tests/runtime/policy-a001-timeline-encapsulation.test.ts (27 tests) 175ms
✓ tests/runtime/policy-q007-remote-code-execution.test.ts (68 tests) 2511ms
✓ PUL-Q007 — no remote code execution (source scan) > runtime tree (current code revision) > contains no Q007 violations acrosssrc/**/*.ts2426ms
✓ tests/runtime/composition-resolver.test.ts (42 tests) 75ms
✓ tests/runtime/screenshot-determinism-source.test.ts (80 tests) 2363ms
✓ PUL-Q001 — screenshot determinism source scan > runtime tree (current code revision) > contains no non-deterministic primitives acrosssrc/**/*.ts2285ms
✓ tests/runtime/scene-loader-present.test.ts (47 tests) 64ms
✓ tests/runtime/composition.test.ts (116 tests) 67ms
✓ tests/runtime/validation.test.ts (59 tests) 37ms
✓ tests/runtime/scene-navigation.test.ts (63 tests) 33ms
✓ tests/runtime/scene-loader.test.ts (29 tests) 72ms
✓ tests/runtime/policy-a005-declarative-composition.test.ts (38 tests) 77ms
✓ tests/runtime/audio.test.ts (117 tests) 132ms
✓ tests/runtime/policy-q002-browser-support.test.ts (21 tests) 81ms
✓ tests/runtime/scene-loader-audio.test.ts (46 tests) 82ms
✓ tests/runtime/scene-loader-screenshot-prompter.test.ts (33 tests) 65ms
✓ tests/runtime/audio-engine.test.ts (15 tests) 40ms
✓ tests/system/audio-helpers.test.ts (13 tests) 37ms
✓ tests/runtime/scene-loader-paused-scrub.test.ts (34 tests) 42ms
✓ tests/runtime/registry.test.ts (35 tests) 23ms
✓ tests/runtime/scene-loader-beat-mode.test.ts (28 tests) 33ms
✓ tests/system/presenter-driven.test.ts (11 tests) 27ms
✓ tests/scenes/loop-fixture.test.ts (20 tests) 11ms
✓ tests/runtime/scene-loader-standalone-loop.test.ts (26 tests) 43ms
✓ tests/runtime/policy-biome-complexity-gate.test.ts (5 tests) 70ms
✓ tests/runtime/scene-loader-chrome.test.ts (19 tests) 88ms
✓ tests/runtime/scene-loader-screenshot-seed.test.ts (9 tests) 15ms
✓ tests/scenes/placeholder.test.ts (15 tests) 29ms
✓ tests/runtime/pul-q006-error-context.test.ts (16 tests) 32ms
✓ tests/system/transitions.test.ts (13 tests) 18ms
✓ tests/scenes/browser-support-fixture.test.ts (19 tests) 28ms
✓ tests/scenes/scrub-fixture.test.ts (18 tests) 23ms
✓ tests/system/scrub-controls.test.ts (13 tests) 16ms
✓ tests/runtime/presenter.test.ts (26 tests) 24ms
✓ tests/runtime/prompter.test.ts (12 tests) 28ms
✓ tests/runtime/workbench-chrome.test.ts (22 tests) 17ms
✓ tests/scenes/dom-css-accessibility-fixture.test.ts (22 tests) 13ms
✓ tests/runtime/composition-registry.test.ts (24 tests) 21ms
✓ tests/system/standalone-audio-bed.test.ts (3 tests) 14ms
✓ tests/scenes/paused-fixture.test.ts (19 tests) 17ms
✓ tests/system/counter.test.ts (12 tests) 10ms
✓ tests/system/presenter-renderers.test.ts (6 tests) 19ms
✓ tests/system/presenter-keyboard.test.ts (11 tests) 32ms
✓ tests/scenes/screenshot-rng-fixture.test.ts (8 tests) 9ms
✓ tests/runtime/error.test.ts (16 tests) 10ms
✓ tests/runtime/audio-unlock-dom.test.ts (7 tests) 21ms
✓ tests/system/register-tokens.test.ts (2 tests) 4ms
✓ tests/runtime/id-registry.test.ts (5 tests) 8ms
✓ tests/system/templates-options.test.ts (20 tests) 29ms
✓ tests/system/templates.test.ts (58 tests) 28ms
✓ tests/runtime/version.test.ts (2 tests) 6ms
✓ tests/system/prompter-window-url.test.ts (4 tests) 11ms
✓ tests/system/terminal-audio.test.ts (8 tests) 15ms
✓ tests/system/pulsar-intro-deck.test.ts (7 tests) 7ms
✓ tests/runtime/workbench-graph.test.ts (6 tests) 9msTest Files 74 passed (74)
Tests 2609 passed (2609)
Start at 17:51:00
Duration 6.40s (transform 7.25s, setup 931ms, collect 35.30s, tests 21.26s, environment 31ms, prepare 9.58s), plus focused audio/validation/loader tests).Final report — issue #148 complete
PR: #155
PR #155 is open and green for issue 148; issue closed after implementation handoff.
Files changed
Added:
changelog.d/148.fixed.md
Modified:
src/runtime/asset-preloader.tssrc/runtime/audio.tssrc/runtime/scene-loader.tssrc/runtime/validation.tstests/runtime/audio.test.tstests/runtime/scene-loader-audio.test.tstests/runtime/validation.test.ts
Reviews
- codex: Cycle 1 clean; 0 findings.
- test-quality: Cycle 1 reported one test weakness in the disposed-service mute assertion; fixed locally, verified, and user authorized proceeding without another review cycle after the cap.
- sonarcloud: GitHub SonarCloud and SonarCloud Code Analysis checks passed. The Ground Control watcher could not authenticate because SONAR_TOKEN is unset on the MCP host.
Traceability reconciliation
- IMPLEMENTS / TESTS / DOCUMENTS added: 0
- Links updated: 0
- Stale links removed: 0
No in-scope requirement UIDs were attached to issue 148. Existing traceability for the touched audio-bed validation/runtime files and tests remains present; no links changed.
Status
- CI: ✅ green
- SonarCloud: ✅ passed
- PR ready for user review and merge.
Review finding
Composition-level
audioBed.srcis not validated against the production asset URL policy. Hardened deployments can passassets: { baseUrl, allowedSchemes: ['https:'] }to validation and preloading, but composition beds are outsidescene.assetsand currently use the audio service's hardcoded default URL policy.Evidence
docs/asset-url-policy.md:35documents the hardened production profile withbaseUrlandallowedSchemes: ['https:'].src/runtime/validation.ts:379checkCompositionAudioBed()calls onlyassertAudioBedDeclaration(), which validates shape but not URL scheme or base URL resolution.src/runtime/audio.ts:990validates audio URLs withresolveAssetUrl(url, undefined, DEFAULT_ALLOWED_SCHEMES), not the deployment's supplied asset policy.src/runtime/scene-loader.ts:1315passes the bed directly tocreateAudioService()after scene preloading; the preloader never sees composition-level bed URLs.Impact
A production entrypoint that tightens scene assets to
https:can still accept and attempt to play composition beds declared ashttp:,data:, orblob:URLs. That bypasses the documented transport and inline-payload controls for hardened deployments.Recommended fix
Thread the same asset URL policy used by
validateRuntime()and the preloader into composition audio-bed validation and runtime audio-bed source checks.Acceptance checks
validateRuntime({ assets: { baseUrl, allowedSchemes: ['https:'] } })reports findings for composition beds whosesrcresolves tohttp:,data:,blob:,file:, or another disallowed scheme.createAudioService()or the loader receives the effective asset URL policy and rejects bed sources that violate it if validation was skipped.audioBed.srcvalues.