Skip to content

fix(catalog): confine hosted preview asset downloads - #3843

Merged
jrusso1020 merged 2 commits into
mainfrom
fix/security-catalog-download-boundary
Sep 10, 2026
Merged

fix(catalog): confine hosted preview asset downloads#3843
jrusso1020 merged 2 commits into
mainfrom
fix/security-catalog-download-boundary

Conversation

@jrusso1020

Copy link
Copy Markdown
Collaborator

Catalog previews run contributor manifests on pull requests. Previously a hosted asset URL could redirect from HTTPS to a private runner endpoint, and its entire response was buffered without a limit. Confine initial URLs and every redirect to https://static.heygen.ai, reject URL credentials, follow at most five redirects, and cap both declared and actual streamed bytes at 256 MiB with a 120-second request deadline. Failed downloads leave an existing destination untouched.

The extracted helper keeps manifest destination containment and uses the same real project root for validation and writing. Byte contents and manifest paths remain unchanged; empty successful responses retain existing behavior. The CDN restriction is deliberate: all 396 current hosted-file references use 23 distinct URLs on that origin. Live HEAD checks for all 23 succeeded, with the largest asset 154,010 bytes, well below the cap. Future registry assets on another origin require an explicit policy change.

Addresses the concrete fetch-boundary defect behind #903 and the connected persistence flow #901. Neither alert is dismissed by this PR; final scanner state and any by-design classification require independent verification. DNS for the fixed CDN remains trusted.

Validation: full workspace build, 213 script tests, 86 catalog tests, and 12 focused download regressions pass. The original fetch function fails the new initial-origin regression. Tests cover private/downgrade/lookalike redirects, exact bytes, traversal rejection, redirect limits, declared and actual size caps, stream/HTTP failures, and preservation of existing output. Lint/format, strict repository typecheck and signed hooks pass. Fallow passes its changed-code gate with four pre-existing findings excluded; no suppression added.

Comment thread scripts/catalog-hosted-files.ts Dismissed

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at head 7069e303 in an isolated worktree. Additive to the CodeQL bot's note (the new alert is the same sink, relocated — see Notes).

Strengths

  • scripts/catalog-hosted-files.ts:51-53assertCdnUrl runs on current before every fetch, including hop 0, and redirect: "manual" means no hop is ever followed by the runtime. Mutating the loop to assert only on hop 0 fails exactly the four redirect tests.
  • catalog-hosted-files.ts:11 — comparing url.origin (not host or a prefix) gets scheme, host and port in one check. Probed at this head: https://user:pw@static.heygen.ai/a, https://static.heygen.ai:8443/a and a punycode lookalike are rejected before any request; https://STATIC.heygen.ai:443/a normalises and is allowed, which is correct.
  • catalog-hosted-files.ts:16-39 — declared and streamed bytes are both capped; Number(null) === 0 means an absent Content-Length falls through to the stream count rather than bypassing it. Dropping the stream cap makes the understated-length test never terminate; dropping the header check fails the declared-oversize test.
  • catalog-hosted-files.ts:84-90 + registry-target-paths.mjs:64-70root is the realpath and isContainedIn refuses any existing symlink component, so a committed symlink at the destination or in a parent directory pointing outside the copy is skipped without a request. Probed both at this head.

Important (non-blocking)

  • catalog-hosted-files.test.ts — two mutants survive the 12 tests: removing the username/password refusal, and replacing the origin compare with protocol + hostname (which would admit :8443). The behaviour is right at this head (probed above), but neither the credential nor the port arm is pinned. Two one-line initial-URL cases close it.
  • catalog-hosted-files.test.ts:72-99 — the streaming-cap test is backed by an infinite pull(). If the cap regresses, the test hangs rather than fails (my mutant had to be killed at 120 s). Enqueue a bounded number of chunks (three of 129 MiB is enough to cross the cap) so a regression reads as a red test.

Nits

  • The 256 MiB cap is generous against a 154 KB maximum in the current 396 references. A cap near the largest plausible asset (tens of MiB) is a tighter resource bound for a pull_request runner and costs nothing today; a comment naming the rationale either way would help the next reader.
  • This is now a third fetch policy alongside the engine's BlockList and the media skill's redirect policy. Deliberately different (allowlist vs. public-only), and the skill ships outside the workspace, so I am not asking for a refactor — only noting the layering so a future host change is made in all three.
  • fetchHostedAsset error messages no longer include the offending URL (the old inline code did), which makes a red preview job harder to read; consider appending url to the CDN and HTTP-error messages.

Verified at this head

  • node --import tsx --test scripts/catalog-hosted-files.test.ts: 12/12.
  • Mutants: hop-0-only assertion (killed, 4 tests), drop stream cap (killed by non-termination), drop Content-Length check (killed), hop limit 5 → 50 (killed by the loop test), drop credential check (survives), origin → protocol+hostname (survives).
  • Every hosted url in the repo's registry-item.json files (396) is on https://static.heygen.ai, matching the PR's count.
  • Required checks all green at 7069e303, including Tests on windows-latest; mergeable.

Notes

  • CodeQL re-raised the network-to-file rule on the relocated writeFileSync (catalog-hosted-files.ts:90). That is the same sink with the source now confined to one HTTPS origin, size-capped and path-contained; by design rather than a false positive, and the classification is the maintainers' call. No alert is touched by this review.
  • The 120 s deadline is one AbortSignal.timeout shared across all hops and the body read, so it bounds the whole download rather than only the headers (read, not exercised — the mocked Response cannot observe it).

Verdict: APPROVE
Reasoning: The boundary is exact-origin per hop, size-capped on both declared and actual bytes, and path-contained on the realpath, and every one of those arms is pinned by a test that a mutant flips; the two surviving mutants cover behaviour that is correct at this head but untested.

— Miga (pr-review)

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at head f0ba3e93b6a8978d5422fa13d2d92d81886ea69f. Interdiff from 7069e303 (which I approved): scripts/catalog-hosted-files.test.ts +20/−11 and one comment line in scripts/catalog-hosted-files.ts. No runtime change.

What the new head fixes from my previous review

  • catalog-hosted-files.test.ts:61-75 — the initial-URL rejection is now a table over 127.0.0.1, user:pw@static.heygen.ai and static.heygen.ai:8443, each asserting zero fetch calls. Verified: the two mutants that survived last time are now both killed — dropping the url.username || url.password check fails test 7; replacing the exact-origin comparison with protocol+hostname fails test 8.
  • catalog-hosted-files.test.ts:80-91 — the streaming-cap fixture closes after three 129 MiB chunks. Verified: with the streamed-bytes cap raised to Number.MAX_SAFE_INTEGER, the test now fails in under two seconds with "Missing expected rejection" instead of hanging the runner.
  • catalog-hosted-files.ts:8 — the 256 MiB rationale (match the media-freeze ceiling) is now in the source.
  • 14/14 tests pass locally at this head.

Why COMMENT rather than APPROVE right now
Required check Render on windows-latest is red at f0ba3e93. I read the job log: the canary render fails because the downloaded chrome-headless-shell.exe --version probe was killed with SIGKILL, ETIMEDOUT on the runner. This PR touches only scripts/ and does not reach that path; the Windows lane is green on main at a7e2e385, d393541c and 962c9540. So the failure is environmental, but I will not approve over a red required check. Test is still pending on this head as well. Once the Windows job is rerun green and Test passes at this same commit, I will post the approval pointing back here; nothing else is outstanding.

Verdict: COMMENT
Reasoning: Code and tests are ready (both previously surviving mutants now killed, cap test terminates), but a required check is red on this head for an unrelated runner failure; approval follows a green rerun at f0ba3e93.

— Miga (pr-review)

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at head f0ba3e93b6a8978d5422fa13d2d92d81886ea69f. Nothing in the code changed since my comment review at this same head (#3843 (review)), which holds the verification: 14/14 locally, the credential and non-default-port mutants now killed, and the streaming-cap test failing promptly rather than hanging when the cap is removed.

The one thing that was outstanding is now resolved: every required check, including Render on windows-latest after its rerun and Test, is green at f0ba3e93.

Verdict: APPROVE
Reasoning: Same verified head as the comment review; the only blocker was a red required check caused by a runner-side Chrome timeout, and it is green on rerun. Merging is Miguel's call.

— Miga (pr-review)

@jrusso1020
jrusso1020 merged commit cd1a996 into main Sep 10, 2026
61 of 62 checks passed
@jrusso1020
jrusso1020 deleted the fix/security-catalog-download-boundary branch September 10, 2026 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants