Skip to content

fix(cli): keep token-file credentials away from repository-chosen API URLs - #5515

Merged
kwakayama merged 1 commit into
mainfrom
fix/token-file-repo-steered-url
Nov 11, 2026
Merged

kwakayama merged 1 commit into
mainfrom
fix/token-file-repo-steered-url

Conversation

@kwakayama

Copy link
Copy Markdown
Contributor

A VERYFRONT_API_TOKEN_FILE credential now gets the same repository-steered-URL rule as shell and .env tokens.

  • The token file wins when the credential is read, but its provenance came from VERYFRONT_API_TOKEN. A literal token in the steering .env made the token-file secret look repository-supplied.
  • A token file is now always operator-owned, including when the .env names its path.
  • Pre-existing on main; raised by Codex on feat(cli): accept VERYFRONT_API_KEY as the one credential name #5514.

Fixes veryfront/veryfront-issue-inbox#4310

… URLs

A token file wins over VERYFRONT_API_TOKEN when the credential is read, but its
provenance came from VERYFRONT_API_TOKEN. A project .env with a literal token
beside its own API URL made a token-file secret look repository-supplied.

Fixes veryfront/veryfront-issue-inbox#4310
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-11T15:48:27.226086Z e06cf2a Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

Copy link
Copy Markdown

📦 Client bundle boundary

Entrypoint Modules Source size Server leaks
src/index.client.ts 66 366 KiB ✅ 0
src/react/components/Head.tsx 5 74 KiB ✅ 0
src/react/router/index.tsx 5 74 KiB ✅ 0
src/react/context/index.tsx 5 74 KiB ✅ 0
src/react/fonts/index.ts 6 81 KiB ✅ 0
src/react/components/ui/index.ts 111 718 KiB ✅ 0
src/react/components/ui/adapter/contract.ts 1 18 KiB ✅ 0
src/react/components/ui/icons/index.ts 3 17 KiB ✅ 0
src/chat/index.ts 305 2047 KiB ✅ 0
src/chat/ag-ui.ts 61 459 KiB ✅ 0
src/chat/protocol.ts 1 11 KiB ✅ 0
src/chat/types.ts 46 246 KiB ✅ 0
src/chat/message-prep.ts 68 517 KiB ✅ 0
src/markdown/index.ts 6 30 KiB ✅ 0
src/mdx/index.ts 2 2 KiB ✅ 0
src/agent/identity-contracts.ts 1 3 KiB ✅ 0
src/schemas/index.ts 43 220 KiB ✅ 0
src/platform/env.ts 5 53 KiB ✅ 0
src/workflow/claude-code/react/index.ts 9 91 KiB ✅ 0
src/events/index.ts 31 567 KiB ✅ 0
src/events/ag-ui/public.ts 30 561 KiB ✅ 0
src/run-events/index.ts 54 321 KiB ✅ 0

A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in scripts/lint/client-bundle-baseline.json to burn down.

@github-actions

Copy link
Copy Markdown

@codex review

Copy link
Copy Markdown
Contributor Author

Review score: 86/100

This is a focused, correct security fix. Before this change, the token's provenance came from VERYFRONT_API_TOKEN even when VERYFRONT_API_TOKEN_FILE actually supplied the secret. A literal token in the steering .env could make an operator's token-file secret count as repository-supplied and send it to a repository-chosen host.

Strengths

  • The fix is one helper, getApiTokenSource(), and it matches the read order in environment-config.ts:173. When VERYFRONT_API_TOKEN_FILE is set, readVeryfrontApiTokenFile either returns a value or throws, so the ?? fallback to the literal token never runs. Treating any configured token file as the effective source is therefore exact, not an approximation.
  • All three provenance reads in cli/shared/config.ts change together: isRepositorySuppliedCredential, resolveApiCredentialCandidates, and resolveApiCredentialForFallback. That covers both the candidate ordering path and the trust gate.
  • The rule fails closed. A token file is always operator-owned, including when the .env names its path. The comment explains why: the repository chose the path, never the contents.
  • The regression test is parameterized over both layouts (path exported in the shell, path named in the .env). It asserts three things: the secret is the effective env.apiToken, it is absent from the auth candidates, and resolveConfig rejects with the untrusted-URL error. Cleanup restores env, loader, and config snapshot state.

Concerns and suggestions (non-blocking)

  • Sibling reads of the same provenance. These still call getEnvSource("VERYFRONT_API_TOKEN") directly:

    • cli/shared/runtime-auth.ts:163 (staleConfigToken)
    • cli/auth/login.ts:71 (describeApiTokenSource)
    • src/server/bootstrap.ts:325 (debug log)

    None of them is a credential-forwarding gate. login.ts also compares values before naming the file. Still, with a token file active, these paths report or reason about a token that is not the one actually sent. Consider exporting getApiTokenSource (or something like it) and using it in these places, or add a note that leaves them out on purpose. Per AGENTS.md, a sweep across all callers of the same root cause is expected.

  • Behavior change for self-hosted layouts. A .env that sets VERYFRONT_API_URL, VERYFRONT_API_TOKEN, and VERYFRONT_API_TOKEN_FILE together now refuses the steered URL unless the URL is confirmed in the shell. The token-file candidate is also promoted to an authoritative shell ("env") candidate, ahead of config-file and the stored login. Both changes are the correct security posture. Neither is called out in the PR description or the refusal text. The remedy in describeUntrustedApiUrl ("Add a literal VERYFRONT_API_TOKEN to the same .env file") does not help when a token file is also set, so the message can mislead a developer in this case.

  • Test evidence. The PR does not show the new test failing on main without the fix. The AGENTS.md "Fixing review findings" section asks for that evidence. I could not run deno task test:file cli/shared/config.test.ts in this review environment because Deno is not installed, so CI is the gate here.

Approve after you decide whether the remedy text needs a token-file branch. The sibling sweep can be a follow-up.


Generated by Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: e06cf2ac15

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@codecov

codecov Bot commented Nov 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@kwakayama
kwakayama added this pull request to the merge queue Nov 11, 2026
@sonarqubecloud

Copy link
Copy Markdown

Merged via the queue into main with commit b9c8011 Nov 11, 2026
1261 of 1275 checks passed
@kwakayama
kwakayama deleted the fix/token-file-repo-steered-url branch November 11, 2026 17:34
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.

1 participant