Skip to content

@remotion/cloudrun: Validate full site names - #9476

Merged
JonnyBurger merged 2 commits into
remotion-dev:mainfrom
santhiprakash:fix/cloudrun-validate-site-name
Jul 23, 2026
Merged

@remotion/cloudrun: Validate full site names#9476
JonnyBurger merged 2 commits into
remotion-dev:mainfrom
santhiprakash:fix/cloudrun-validate-site-name

Conversation

@santhiprakash

@santhiprakash santhiprakash commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Problem

packages/cloudrun/src/shared/validate-site-name.ts used the same unanchored regex as @remotion/lambda previously did: .match(/([0-9a-zA-Z-!_.*'()]+)/g). Any name containing one allowed substring passed, so path separators, spaces, @, #, Unicode, and other characters could reach deploy-time in Cloud Run.

Triage / Root cause

The regex is unanchored, dot segments (. / ..) are not explicitly rejected, and the error message references the wrong (unanchored) regex.

Fix

  • Replace the unanchored match with /^[0-9a-zA-Z-!_.*'()]+$/ and .test().
  • Explicitly reject . and .. dot segments.
  • Update the error message to reference the anchored regex.
  • Add unit tests in packages/cloudrun/src/test/validate-site-name.test.ts.

Verification

  • bun test packages/cloudrun/src/test/validate-site-name.test.ts passed.
  • bunx tsgo -d passed (type-check / build for the package).
  • bun run stylecheck passed (only pre-existing warnings outside cloudrun).

Notes / Risks

Follow-up to #9470 (same bug in the lambda package). The two validators now behave identically.

@vercel

vercel Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
bugs Ready Ready Preview, Comment Jul 23, 2026 8:24am
remotion Ready Ready Preview, Comment Jul 23, 2026 8:24am

Request Review

@pullfrog pullfrog Bot 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 changes — the PR fixes the site-name validator in @remotion/cloudrun to reject strings containing invalid characters anywhere (not just as a substring match), tightens the regex to anchor full-string validation, rejects . and .. dot segments, and ensures the compositor is always shut down in extractAudio even when executeCommand fails.

  • packages/cloudrun/src/shared/validate-site-name.ts — replaces unanchored /g regex match with RegExp.prototype.test and anchored regex; fixes projectName typo in the typeof error; adds explicit . / .. rejection.
  • packages/cloudrun/src/test/validate-site-name.test.ts — adds unit tests for valid names, invalid characters, dot segments, empty strings, whitespace, slashes, undefined, and non-string inputs.
  • packages/renderer/src/extract-audio.ts — wraps executeCommand in try/catch so shutDownOrKill always runs and the original execution error is preserved.

Important

Two concerns need to be addressed before merging: an inconsistency with the Lambda validator's typeof error message, and a behavior change in extractAudio that now swallows shutdown errors when an earlier execution error occurred. Both are small but have real cross-package and operational implications.

⚠️ Lambda validator drift

packages/lambda/src/shared/validate-site-name.ts still uses the old unanchored regex and the old projectName error string. Since the PR description says this is "the same fix already applied to @remotion/lambda in #9470", the Lambda file appears to have been missed or reverted; leaving this commit whose title is "validate full site names" without an equivalent @remotion/lambda update will create inconsistent behavior and error wording across the two packages.

Technical details
# Lambda validator drift

## Affected sites
- `packages/lambda/src/shared/validate-site-name.ts:14` — still uses unanchored `/([0-9a-zA-Z-!_.*'()]+)/g`.
- `packages/lambda/src/shared/validate-site-name.ts:8` — still says `'projectName'` while the cloudrun version now says `'siteName'`.

## Required outcome
- Apply the same anchored-regex validation and `.` / `..` rejection to `packages/lambda/src/shared/validate-site-name.ts`.
- Align the non-string error message wording between cloudrun and lambda, or decide intentionally that they should differ.

## Suggested approach
Either include `packages/lambda/src/shared/validate-site-name.ts` in this PR (matching the PR title and description) or open a follow-up that applies the same logic immediately after merge, so the two packages do not diverge.

## Open questions for the human
- Does #9470 already contain the Lambda changes and this branch is simply behind it, or was the Lambda file never updated?

⚠️ extractAudio silently drops shutdown errors after an execution failure

packages/renderer/src/extract-audio.ts:33-39 calls shutDownOrKill and, if executeError is set, discards any shutdown error. When extraction fails, a later compositor shutdown failure (e.g., process already crashed) will be swallowed, making post-mortem debugging harder because the original error is preserved but the shutdown error is lost entirely.

Technical details
# extractAudio silently drops shutdown errors after an execution failure

## Affected sites
- `packages/renderer/src/extract-audio.ts:33-39` — shutdown error is dropped when `executeError` is truthy.

## Required outcome
- Avoid losing contextual information when both execution and shutdown fail.

## Suggested approach
Consider attaching the shutdown error as a property on the original error (e.g., `executeError.shutdownError = shutdownError`), or wrapping both in an aggregate error. The simplest minimal change is to stash `shutdownError` on `executeError` before re-throwing it, so logs and Sentry reports can still see both failure modes.

## Open questions for the human
- Is suppressing shutdown errors intentional because they are expected noise after an execution failure, or should the renderer preserve both? Other long-running-compositor helpers such as `getSilentParts` and `getVideoMetadata` currently do not suppress shutdown errors at all.

Note: 1 inline comment(s) dropped because they did not anchor to lines inside the PR diff:

  • packages/cloudrun/src/shared/validate-site-name.ts:14-28 (RIGHT) — line 28 (RIGHT) is not inside a diff hunk

Pullfrog  | Fix it ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) (Claude Opus not used — the program covers this model; add its provider key to run your pick) | 𝕏

The cloudrun site-name validator used .match(/.../g), so any name
containing an allowed substring passed validation. Path separators,
spaces, @, #, and Unicode could therefore reach deploy-time.

- Anchor the allowlist regex with ^...+$ so the entire string is checked.
- Explicitly reject `.` and `..` dot segments.
- Fix the error message to reference the anchored regex.
- Add unit tests for valid, invalid, dot-segment, and non-string inputs.
@JonnyBurger JonnyBurger changed the title @remotion/cloudrun: Validate full site names @remotion/cloudrun: Validate full site names Jul 23, 2026
@JonnyBurger JonnyBurger reopened this Jul 23, 2026
@JonnyBurger
JonnyBurger merged commit 153fb1e into remotion-dev:main Jul 23, 2026
5 checks passed
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.

2 participants