add auth profiles to wrangler #14200
Conversation
🦋 Changeset detectedLatest commit: 24ac3d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
workers-devprod
requested review from
a team and
penalosa
and removed request for
a team
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
I'm Bonk, and I've done a quick review of your PR. This PR adds named auth profiles to wrangler (per-directory bindings, a global Two issues, ranked by severity: 1. (medium)
let stored = readStoredAuthState({ warningLogger: ctx.logger }); // no profile
...
stored = readStoredAuthState({ warningLogger: ctx.logger }); // re-read, also no profileEvery other read path you touched ( Its only consumer is Suggested fix: give 2. (low) In |
|
Posted my review on PR #14200. Summary of what I flagged:
Note: the inline batch review failed with a 422 because the relevant |
|
✅ All changesets look good |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
@cloudflare/wrangler-bundler
commit: |
emily-shen
force-pushed
the
emily/profiles-2
branch
from
cd0ac71 to
24ac3d7
Compare
There was a problem hiding this comment.
🟡 Deprecated API token warning shows wrong file path for non-default profiles
In state.ts line 106, getAuthConfigFilePath() is called without the profile argument, even though profile is available in scope (destructured at line 77 from the options object). When a non-default profile's config file contains a deprecated api_token field, the warning message will display the default profile's file path instead of the actual profile's path, confusing the user about which file to fix.
(Refers to line 106)
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (args.profile) { | ||
| return args.profile; |
There was a problem hiding this comment.
🔴 Path traversal via unvalidated --profile global flag
The --profile global CLI flag passes user input directly through resolveProfile() (packages/workers-auth/src/profiles.ts:218-219) without any validation. This raw string flows into the command context at packages/wrangler/src/core/register-yargs-command.ts:134-146, where it's stored via run({ ...experimentalFlags, profile }). Subsequently, when getProfile() returns this string, it reaches getAuthConfigFilePath() (packages/workers-auth/src/auth-config-file.ts:48) which interpolates it directly into a file path: fileName = \${resolved}.toml`. A user running wrangler deploy --profile "../../tmp/evil"would cause wrangler to read auth tokens from (and potentially write tokens to)~/.wrangler/config/../../tmp/evil.toml, escaping the intended config directory. During token refresh, writeAuthConfigFilealso callsmkdirSyncwithrecursive: true` along the traversed path, which could create directories in unintended locations.
Affected code path
resolveProfile({ profile: args.profile })returns raw string- Stored in AsyncLocalStorage context
getProfile()returns itgetAuthConfigFilePath(profile)→path.join(base, "config",${profile}.toml)- Reads/writes to arbitrary path
Prompt for agents
The resolveProfile function at packages/workers-auth/src/profiles.ts:214-236 returns the raw args.profile string without validation. This flows into getAuthConfigFilePath() which interpolates it into a file path, enabling path traversal.
Fix approach: Add validation in resolveProfile when the profile comes from args.profile. Call validateProfileName(args.profile) before returning it. This ensures the --profile flag (which is the untrusted input boundary) is validated the same way as the auth create/delete/activate commands.
Alternatively (defense-in-depth), add validation directly in getAuthConfigFilePath() at packages/workers-auth/src/auth-config-file.ts:40-54 to reject any profile value that doesn't match the safe pattern /^[a-zA-Z0-9_-]+$/ before using it in the file path. This protects against all callers, including potential future code paths that might pass unvalidated strings.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
See changeset and discussion #14161 for design.
(i did also delete a bunch of unnecessary snapshots, since adding a new global flag caused snapshot mismatches in way too many places...)
A picture of a cute animal (not mandatory, but encouraged)