fix(security): close 3 RCE chains + deploy ACL + README corrections - #131
Open
Caosmart1979 wants to merge 5 commits into
Open
fix(security): close 3 RCE chains + deploy ACL + README corrections#131Caosmart1979 wants to merge 5 commits into
Caosmart1979 wants to merge 5 commits into
Conversation
… export The live preview / deck / template-preview iframes used sandbox="allow-scripts allow-same-origin", which makes their srcDoc content share the host origin — so agent-generated scripts could read parent.localStorage and call /api/* (convert / deploy / marketplace.install) with the host's credentials. README's "host never poisoned / cookies & localStorage quarantined" claim was false for this combination. Drop allow-same-origin from the three script-running iframes so agent HTML executes in an opaque origin; scripts, Tailwind CDN, fonts and animations still work, but can no longer reach the host. PNG export previously read the live iframe's contentDocument (the coupling that forced same-origin). Rewrite iframeToBlob to snapshot via a throwaway offscreen iframe built from the srcDoc attribute the parent already owns (readable regardless of the iframe's origin). Signature unchanged, callers untouched. - preview-pane.tsx, deck-viewer.tsx, template-picker.tsx: sandbox="allow-scripts" - image.ts: iframeToBlob builds an offscreen snapshot iframe from srcDoc Verified: pnpm -F @html-anything/next typecheck passes. Closes chain 1 of the production-grade audit (docs/superpowers/specs/2026-07-28-design-system-production-audit.md). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dows argv injection Chain 2 — /api/convert (and /api/draft) spawned the agent CLI with no server-side limits: unbounded body, no concurrency cap, no timeout, and cwd/binOverride accepted straight from the request body while the CLI runs under bypassPermissions / workspace-write / --yolo. - Add lib/agents/spawn-guards.ts: MAX_INFLIGHT=2 concurrency slot, 10 min watchdog, 2 MiB body cap, 1 MiB prompt cap. Both routes acquire before spawn and release in finally/cancel. - Drop `cwd` from the accepted body — the agent runs with elevated file perms, so the working directory stays server-owned (process.cwd()). The UI never sends cwd; a configurable root, if ever needed, goes through a Settings allowlist. - binOverride (invoke.ts): constrain the basename to the agent's known bin names (sans .exe/.cmd/.bat) so a request can't spawn an arbitrary executable under elevated flags. Custom install paths still work. Chain 3 — Windows command injection. invoke.ts set shell:true on Windows (.cmd shims require it) with a comment claiming the prompt never enters the command line — false for argv / argv-message protocols (deepseek positional, openclaw --message), which put the prompt into the argv cmd.exe parses. Replace the false comment; reject prompts containing cmd metacharacters (& | < > ^ % newlines) for those two protocols on Windows with a clear error steering to a stdin-capable agent. macOS/Linux use direct exec (no shell), so argv is safe there. Verified: pnpm -F @html-anything/next typecheck passes; agents unit tests green (the 17 skills/__tests__ tar failures are pre-existing Windows-only, audit qa-003, unrelated). Closes chains 2 + 3 of the production-grade audit (docs/superpowers/specs/2026-07-28-design-system-production-audit.md). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…token ACL The marketplace routes carried a per-route Host-header guard (marketplace/_lib/host-guard.ts) as a stopgap until the global /api/* middleware landed. It has (commit nexu-io#61), making the per-route guard redundant — and worse, its LOOPBACK_HOSTS accepted 0.0.0.0, which the global host-validation deliberately excludes (pre-fix Chrome 0.0.0.0-day DNS rebinding). The module's own header said "delete once the middleware merges." - Delete marketplace/_lib/host-guard.ts + its test; remove the per-route isHostAllowed checks from the 4 marketplace routes. Host gating now has a single source of truth: middleware.ts -> lib/security/host-validation.ts (covered by host-validation.test.ts + e2e host-validation.spec.ts). - Drop the two route-level host-regression tests in skills api.test.ts that bypassed middleware by calling the handler directly; keep the functional tests. Deploy token ACL (audit sec-005): deploy/config.ts relied on chmod 0o600 to protect the plaintext Vercel/Cloudflare token — a no-op on Windows/NTFS — and silently swallowed the chmod failure. restrictDeployConfigPerms now chmods on POSIX and runs `icacls /inheritance:r /grant:r <user>:F` on Windows, logging (not swallowing) on failure. Verified: typecheck passes; marketplace api functional tests pass. The 2 tar-packaging test failures are pre-existing Windows-only (audit qa-003: system tar cannot resolve C: drive-letter paths) and unrelated to this change. Closes task 5 of the production-grade audit P0 (docs/superpowers/specs/2026-07-28-design-system-production-audit.md). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The audit verified several README claims against the code and found them false. Correct them so the README isn't louder than the implementation: - Security: the preview iframe is now sandbox="allow-scripts" (no allow-same-origin -> opaque origin) after the chain-1 fix. The old "host never poisoned / cookies quarantined" wording was false for the previous allow-scripts allow-same-origin combination; reword to describe the opaque-origin sandbox and the offscreen snapshot used for PNG export. - huashu attribution: the anti-slop constraints are NOT "inside every SKILL.md frontmatter" (0/78 contain them). They are centralized in next/src/lib/templates/shared.ts and applied at prompt-assembly. - Status table: WeChat export is partial (computed-style inlining for Tailwind-CDN templates unimplemented), not stable; multi-template compare is planned/not-started (0 lines), not in-progress; skill marketplace + per-task history are shipped; Hyperframes ships a Remotion .zip (mp4 is planned). Agent detection is 11 callable stdin/argv CLIs; 7 ACP/pi-rpc agents are detect-only. - Drop the unverifiable "40k★ 200+ contributors" upstream figures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s CI
The marketplace installer preflight parsed tarballs in pure JS, but the
extraction step still shelled out to `tar -xzf` (install.ts:241). On Windows
this failed with "Cannot connect to C: resolve failed", and every skills
test that built fixtures with `spawn('tar', ['-czf', ...])` also failed on
Windows (audit qa-003: 17/164 tests).
- Replace `extractTarball` in lib/skills/install.ts with a pure-JS ustar
extractor (mirroring the preflight parser). The tarball has already passed
preflight, so we trust the header vetting and write regular files to disk.
This fixes marketplace install on Windows and removes the last system tar
dependency in the production install path.
- Add lib/skills/__tests__/tarball.ts: shared pure-JS `tarGzDir` + `buildTarball`
helpers. Switch api.test.ts, install.test.ts, install-rejections.test.ts,
and cross-device.test.ts to use it. The symlink rejection test now builds
its symlink entry in-memory instead of calling fs.symlink, which needs
admin/Developer Mode on Windows.
- Update preflight.test.ts happy-path to use its existing buildTarball helper
instead of spawn('tar'); remove the unused spawn import.
- Add a windows-latest matrix job to .github/workflows/ci.yml so Windows
unit tests run on every PR/push.
Verified locally: scripts/guard.ts, pnpm -F @html-anything/next typecheck,
next unit tests (154 passed), and e2e typecheck all pass on Windows.
Closes task 6 (Windows CI + README) of the production-grade audit
(docs/superpowers/specs/2026-07-28-design-system-production-audit.md).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lefarcen
added
size/XL
|
@Caosmart1979 I'm holding off on generating review comments for #131 because this pull request has merge conflicts right now. Please resolve the conflicts with main and push the updated branch. Once that's done, request or wait for the review to run again and I'll take another look. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
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
3 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.
Closes 3 confirmed critical remote-code-execution chains found in a 33-agent multi-expert audit. All verified against the code with file:line evidence; anti-verified (adversarially) for critical/high findings.
What this fixes
Chain 1 — iframe same-origin (critical/confirmed)
The 3 live-preview iframes used sandbox=
allow-scripts allow-same-origin, making the agent's srcdoc content share the host origin. Its scripts could read parent.localStorage and make credentialed calls to /api/convert|deploy|marketplace.install. README's "host never poisoned / cookies quarantined" claim was false.→ Drop allow-same-origin (opaque origin). Rewrite PNG export to snapshot via a throwaway offscreen iframe built from the srcdoc attribute the parent already owns.
Chain 2 — /api/convert unauthenticated RCE (critical/confirmed)
Body unbounded, no concurrency limit, no timeout, cwd/binOverride accepted straight from the body WITH the CLI spawned under bypassPermissions/--yolo/--dangerously-skip-permissions.
→ Add spawn-guards.ts (MAX_INFLIGHT=2, 10min watchdog, 2MiB body cap). Drop cwd from the request body. Constrain binOverride basename to the agent's known binary names.
Chain 3 — indirect prompt injection → RCE (critical/confirmed)
assemblePrompt bare-concatenates ${opts.content} with a single natural-language "no file tools" constraint. Combined with the same-origin iframe and bypass CLI, this is a complete data→XSS→arbitrary-execution path.
→ Broken by closing chains 1+2. Windows argv injection additionally hardened (reject cmd metacharacters for deepseek/openclaw argv protocols; delete the false "no injection vector" comment).
Plus