fix(security): harden Puter origin, comment splice, WAR scope, nav guards#260
Open
heznpc wants to merge 3 commits into
Open
fix(security): harden Puter origin, comment splice, WAR scope, nav guards#260heznpc wants to merge 3 commits into
heznpc wants to merge 3 commits into
Conversation
…ards Fixes from an adversarially-verified security + defect review. - page-bridge: pin globalThis.PUTER_API_ORIGIN / PUTER_ORIGIN to the official Puter origins (non-writable, non-configurable) BEFORE the SDK bundle executes, so a page-world script on a Skilljar page can no longer redirect authenticated Puter requests (Bearer token + prompts) or the sign-in popup to a hostile origin. Also capture + scrub globalThis.puter synchronously in onload to shrink the transient full-SDK exposure window. - code-comments: splice comment translations in descending source-index order. Matches were applied line-first then block-first (not source order), so a block comment appearing before a line comment shifted later offsets and corrupted the rendered code block. - content: drop the fonts.googleapis.com fetch and rely on the OS-native font stack already in content.css, removing an undisclosed third-party contact that leaked IP + reading language and contradicted the privacy policy. - manifest: narrow the claude.com web_accessible_resources match to /resources/tutorials/* (the content-script scope) so bundled data is no longer probeable for extension fingerprinting from other claude.com pages. - bookmarks/resume: gate location.href navigation on ^https? like the dashboard open handler already does. - ci(dependabot-auto-merge): create-or-update the manual-review label before add-label, so major dependency bumps stop failing the auto-merge job (UNSTABLE). Adds regression tests: tests/page-bridge-origin-pin.test.js (origin pinning) and tests/code-comments-order.test.js (block-before-line splice ordering). Full suite: 28 suites / 597 tests green; eslint clean.
Follow-up for defects a max-effort review surfaced in the prior commit.
- page-bridge: _pinPuterOrigins now fails closed. A pre-existing
non-configurable origin global is trusted ONLY when it is already a
non-writable data property equal to the official origin; a writable data
property (page can reassign after us) or an accessor/getter (can return a
hostile value on a later read) is rejected instead of silently accepted.
- page-bridge: force the captured SDK instance's API origin via
setAPIOrigin('https://api.puter.com'), closing the `?puter.api_origin=`
query-param path (reached when `?puter.app_instance_id=` sets env "app")
that the page-global pin cannot cover.
- code-comments: drop overlapping comment matches before splicing. A `//`
inside a `/* ... */` block (e.g. a URL) produced overlapping line+block
matches whose in-place splices corrupted the code the learner copies; keep a
non-overlapping subset (earliest start wins). Corrects the earlier
"order-independent" comment, which only held for disjoint matches.
- ci(dependabot-auto-merge): add `issues: write` (gh label create writes repo
labels, an Issues-scoped resource, and 403s without it) and create the
`dependencies` label too so the add-label step can't fail on a missing label.
Tests: adds tests/page-bridge-origin-lock.test.js (fail-closed), a setAPIOrigin
assertion in the origin-pin test, and an overlapping-URL case in the
code-comments test. 29 suites / 599 tests green; eslint clean.
…us tests A second-round review of the follow-up commit found the setAPIOrigin fix ran too late to matter, and two regression tests did not actually guard their regressions. - page-bridge: refuse to load Puter when the host page URL carries Puter "app" params (?puter.app_instance_id / ?puter.api_origin / ?puter.domain). The SDK reads these at CONSTRUCTION — app_instance_id flips env to "app", unlocking the api_origin override, and the constructor then auto-loads the stored token from localStorage and fires /rao + /whoami with it — all synchronously during bundle evaluation, BEFORE the post-load setAPIOrigin runs. So setAPIOrigin could not intercept the leak. Failing closed here prevents the SDK from ever constructing against a page-supplied origin. setAPIOrigin stays as defense-in-depth; its comment no longer overclaims. - test(code-comments): the URL-in-block-comment case was vacuous — the spurious inner line match was dropped by the `source === 'original'` skip, not the new overlap filter, so the test passed on pre-fix code too. Map the inner text so it would translate; the test now fails on pre-fix code (which corrupts `run();`). - test(page-bridge): add a host-URL-param refusal test that drives the new guard, and soften the setAPIOrigin assertion's comment (it is defense-in-depth, not the thing that closes the query-param path). - eslint: declare URLSearchParams (a browser global, used by the new guard). 29 suites / 600 tests green; eslint clean.
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
1 participant
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.
Security & defect review — fixes
Result of a multi-agent review (8 dimension finders → 3-lens adversarial verification, 2-of-3 to confirm). 10 raw findings → 6 confirmed; all fixed here. Two HIGH candidates were correctly rejected as false positives and are noted at the bottom.
Confirmed findings fixed
src/lib/page-bridge.js(Puter SDK)globalThis.PUTER_API_ORIGIN/PUTER_ORIGIN) that aren't env-gated. Since the SDK is injected into the untrusted Skilljar page's main world, a page script could redirect authenticated Puter requests (Bearer token + prompts) and the sign-in popup to an attacker origin.src/lib/page-bridge.jsglobalThis.puterfor ~100 ms after load before the polling scrub runs.onload. Complete remediation (isolate the SDK in an extension-owned realm) is flagged as a follow-up.src/content/code-comments.jsmatch.index.src/content/content.jsfonts.googleapis.comfetch leaked IP + reading language to Google on non-Latin language selection, contradicting the privacy policy.content.css.manifest.jsonweb_accessible_resourcesexposedsrc/data/*to all ofclaude.com/*while the content script runs only on/resources/tutorials/*→ fingerprintable from other pages.https://claude.com/resources/tutorials/*.src/content/bookmarks.js,resume.jslocation.hrefwith no scheme check (dashboard validates).^https?:gate.Also
dependabot-auto-merge.ymlfailed every major bump because themanual-reviewlabel doesn't exist. Now creates-or-updates it before--add-label.Tests
tests/page-bridge-origin-pin.test.js,tests/code-comments-order.test.js.npm test: 28 suites / 597 tests pass.npm run lint: clean.Correctly rejected (false positives, verified)
chat-history.js"wipes entire store on quota hit" — the prune loop is correctly bounded and already hardened.dependabot-auto-merge"auto-publishes a tampered extension" — CD triple-gates on deploy-relevant file changes, an exact dashboard-version match, and a publication-paused flag; a dependency bump matches none.