Skip to content

fix(cli): confine auth email content_path to project root (CLI-2320) - #6489

Merged
Coly010 merged 2 commits into
developfrom
columferry/cli-2320-config-push-contain-auth-email-content_path-resolution-to
Sep 7, 2026
Merged

fix(cli): confine auth email content_path to project root (CLI-2320)#6489
Coly010 merged 2 commits into
developfrom
columferry/cli-2320-config-push-contain-auth-email-content_path-resolution-to

Conversation

@Coly010

@Coly010 Coly010 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Summary

config push resolves auth.email.template.*.content_path and
auth.email.notification.*.content_path from disk before uploading their
bytes as part of the auth config write. Resolution previously joined a
relative path onto the project root (or passed an absolute path through
unchanged) with no containment check, so a crafted content_path could
resolve outside the project. content_path is now confined to the project
root: any resolved path — a relative .. escape or an absolute path
elsewhere on disk — aborts before the file is read, using the existing
Invalid config for auth.email.<kind>.<name>.content_path: <reason> error
shape.

Linear: CLI-2320 (surfaced by CLI-2313's security review; intentionally kept
out of that PR).

What changed

  • push.auth-email-content.ts — new resolveContainedContentPath normalizes
    the candidate path with path.resolve and rejects anything outside the
    discovered project root before readTemplateContent runs;
    legacyLoadAuthEmailContent's template and notification loops both route
    through it. The existing notification legacy-fallback behavior
    (supabase/-relative lookup) is preserved as long as the resolved result
    stays inside the root.
  • push.auth-email-content.unit.test.ts — new coverage: absolute path outside
    root (template + notification), relative .. escape (template +
    notification), and a boundary case for a path resolving to exactly the
    project root.
  • SIDE_EFFECTS.md — documents the containment rule in Files Read, Notes, and
    the exit-code table.

…oot (CLI-2320)

content_path resolution for auth.email.template.*/auth.email.notification.*
previously allowed relative .. traversal and absolute paths outside the
project root, so a config push could read and upload an arbitrary local
file. Resolution now rejects anything outside the discovered project root
before the file is read.
@Coly010 Coly010 self-assigned this Sep 7, 2026
@Coly010
Coly010 marked this pull request as ready for review September 7, 2026 09:53
@Coly010
Coly010 requested a review from a team as a code owner September 7, 2026 09:53
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Supabase CLI preview

npx --yes https://pkg.pr.new/supabase/cli/supabase@4ecb369efc53e08a29515727cde2a1eb55778fc9

Preview package for commit 4ecb369.

Comment thread apps/cli/src/commands/config/push/push.auth-email-content.ts Outdated

@github-actions github-actions 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.

🤖 AI Review

Six deduplicated findings were adjudicated from 6 Claude and 2 Codex findings. Four are confirmed, including the jointly identified symlink escape and false rejection of valid in-root paths. Two documentation findings are refuted because metadata-only stat calls do not read file contents and the documented notification fallback selects an in-root final path.

Findings

Severity Location Category Sources Claim
🔴 CRITICAL apps/cli/src/commands/config/push/push.auth-email-content.ts:69 security claude+codex The containment check can be bypassed with an in-project symlink, allowing an outside file to be read and uploaded.
🟡 MINOR apps/cli/src/commands/config/push/push.auth-email-content.ts:38 correctness claude+codex The containment predicate rejects valid in-root paths whose first component merely begins with two dots.
🟡 MINOR apps/cli/src/commands/config/push/push.auth-email-content.ts:63 consistency claude Containment is implemented only in config push, while validation and supabase start still accept and consume out-of-root content_path values.
⚪ NIT apps/cli/src/commands/config/push/push.auth-email-content.ts:72 error-handling claude The containment error omits the rejected resolved path, making the failure harder to diagnose.
Refuted findings (kept for transparency, not posted as review comments)
  • apps/cli/src/commands/config/push/push.auth-email-content.ts:63 (documentation): The statement that an out-of-root notification path is never read is inaccurate because the legacy resolver stats it before containment is checked.
    Refuted: statSync reads filesystem metadata, not the file's contents. The documentation says containment occurs before the file is read, and readFileSync is indeed reachable only after the containment check succeeds.
  • apps/cli/src/commands/config/push/SIDE_EFFECTS.md:354 (documentation): The documentation incorrectly promises that every notification path initially resolving outside the root aborts, because the legacy fallback can redirect it to an in-root file.
    Refuted: The documentation already states that notifications use the legacy fallback. In the cited ../secrets.html case, the fallback's final selected path is inside the project root, so it satisfies the documented final-path containment rule rather than contradicting it.

Stats

Claude findings: 6 · Codex findings: 2 · Confirmed: 4 · Refuted: 2 · Uncertain: 0


Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run

This review runs once per PR. A maintainer can request another with a /ai-review comment.

Comment thread apps/cli/src/commands/config/push/push.auth-email-content.ts Outdated
Comment thread apps/cli/src/commands/config/push/push.auth-email-content.ts
Comment thread apps/cli/src/commands/config/push/push.auth-email-content.ts
Comment thread apps/cli/src/commands/config/push/push.auth-email-content.ts
The containment check normalized paths with path.resolve, which is purely
lexical and never follows symlinks, so an in-root symlink pointing outside
the project root could still bypass it and get its target's bytes read and
uploaded. Both the project root and the candidate path are now resolved
with realpathSync before the containment comparison (falling back to
lexical resolution only when the target doesn't exist yet, since there's
no symlink to dereference in that case).

Also fixes a boundary bug in the containment predicate: it rejected any
relative result starting with "..", which wrongly rejected a legitimate
in-root path like "..templates/invite.html"; it now only rejects a genuine
".." traversal, matching the existing pattern in
shared/functions/deploy.ts's isContainedPath. The abort message now
includes the rejected resolved path for diagnosability.
@Coly010
Coly010 added this pull request to the merge queue Sep 7, 2026
Merged via the queue into develop with commit ad95a9a Sep 7, 2026
66 checks passed
@Coly010
Coly010 deleted the columferry/cli-2320-config-push-contain-auth-email-content_path-resolution-to branch September 7, 2026 11:04
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