add auth profiles to wrangler #14200
Conversation
🦋 Changeset detectedLatest commit: 79fb374 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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
commit: |
emily-shen
force-pushed
the
emily/profiles-2
branch
from
cd0ac71 to
24ac3d7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| export function listProfilePaths(): string[] { | ||
| const configDir = path.join(getGlobalWranglerConfigPath(), "config"); | ||
| if (!existsSync(configDir)) { | ||
| return []; | ||
| } | ||
|
|
||
| const files = readdirSync(configDir); | ||
| return files | ||
| .filter((f) => f.endsWith(".toml")) | ||
| .map((f) => f.replace(/\.toml$/, "")); | ||
| } |
There was a problem hiding this comment.
🟡 listProfilePaths includes staging-environment default config as a user-visible profile
The listProfilePaths function lists all .toml files in the config directory and strips the extension to produce profile names. In staging environments, getAuthConfigFilePath() (without a profile) creates staging.toml for the default profile. This means wrangler auth list will show "staging" as a listed profile even though it's just the default profile's staging-environment variant, not a user-created profile. This is confusing because validateProfileName rejects "staging" as a reserved name, so the user would see a profile they cannot manage with wrangler auth delete or other profile commands.
Was this helpful? React with 👍 or 👎 to provide feedback.
| MULTIWORKER: false, | ||
| RESOURCES_PROVISION: false, | ||
| AUTOCREATE_RESOURCES: false, | ||
| profile: "default", |
There was a problem hiding this comment.
🚩 Programmatic API (unstable_dev) hardcodes profile to 'default'
In api/dev.ts:233, the programmatic API entry point sets profile: "default" unconditionally. This means the unstable_dev() / programmatic API cannot use named auth profiles. This is likely intentional — the programmatic API doesn't expose a --profile flag and should use the default credential chain. But if a user sets up directory bindings and then uses the programmatic API from that directory, the profile binding will be ignored. Worth documenting if not already covered.
Was this helpful? React with 👍 or 👎 to provide feedback.
| MULTIWORKER: Array.isArray(args.config), | ||
| RESOURCES_PROVISION: false, | ||
| AUTOCREATE_RESOURCES: false, | ||
| profile: "default", |
There was a problem hiding this comment.
🚩 pages/dev.ts also hardcodes profile to 'default'
Similar to the programmatic API, pages/dev.ts:960 sets profile: "default" when calling run(). This means wrangler pages dev does not participate in the profile resolution system. Since pages dev is always a local-only dev command that doesn't make authenticated API calls in the same way as other commands, this is likely fine. But it means profile bindings will be ignored during pages development.
Was this helpful? React with 👍 or 👎 to provide feedback.
emily-shen
force-pushed
the
emily/profiles-2
branch
from
3364c17 to
7d67d39
Compare
| } | ||
| } | ||
|
|
||
| await logout(args.name); |
There was a problem hiding this comment.
🔴 wrangler auth delete silently fails to remove profile file when CLOUDFLARE_API_TOKEN is set
The authDeleteCommand handler calls await logout(args.name) at packages/wrangler/src/user/profiles.ts:126, which delegates to oauthFlow.logout(profile). However, in packages/workers-auth/src/flow.ts:290-297, the logout function short-circuits when ctx.hasEnvCredentials() returns true — it prints "You are logged in with an API Token" and returns without revoking the token or deleting the file. The authDeleteCommand then proceeds to log Profile "<name>" deleted. even though the profile's TOML file was never removed from disk. Users who have CLOUDFLARE_API_TOKEN set (common in CI or multi-account setups — exactly the audience for profiles) will believe the profile was deleted when it wasn't.
Prompt for agents
In authDeleteCommand handler (packages/wrangler/src/user/profiles.ts), the call to `await logout(args.name)` delegates to `oauthFlow.logout()` which short-circuits (no-op) when CLOUDFLARE_API_TOKEN is set in the environment. This means the profile file on disk is never deleted, yet the handler logs 'Profile deleted'.
The fix should bypass the OAuth flow's logout entirely when deleting a named profile. Instead of calling `logout(args.name)`, the handler should:
1. Directly call `deleteProfileFile(args.name)` (already exported from @cloudflare/workers-auth) to remove the TOML file.
2. Optionally attempt to revoke the stored refresh token (best-effort), but not let the env-credentials guard prevent file deletion.
Alternatively, the `oauthFlow.logout` could be modified to accept an option like `{ force: true }` that skips the env-credentials check, but the simpler approach is to use `deleteProfileFile` directly in the delete command handler since the profile management commands should always operate on profile files regardless of env credentials.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (args.profile) { | ||
| validateProfileName(args.profile); | ||
| return args.profile; |
There was a problem hiding this comment.
🟡 --profile default throws misleading 'reserved profile name' error on any command
resolveProfile is called for every command at packages/wrangler/src/core/register-yargs-command.ts:134. When a user passes --profile default (e.g., wrangler deploy --profile default), resolveProfile at packages/workers-auth/src/profiles.ts:212-213 calls validateProfileName("default") which throws a UserError: "default" is a reserved profile name. Use wrangler login.... This error message is designed for wrangler auth create default but is confusing when a user simply wants to explicitly use the default profile for a regular command. The --profile flag is globally available, so users may reasonably try --profile default to override a directory binding.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function getProfileForDirectoryFromBindings( | ||
| startDir: string, | ||
| bindings: Record<string, string> | ||
| ): { profile: string; dir: string } | undefined { | ||
| const normalizedDir = path.resolve(startDir); | ||
|
|
||
| const sortedEntries = Object.entries(bindings).sort( | ||
| ([a], [b]) => b.length - a.length | ||
| ); | ||
|
|
||
| for (const [boundDir, profile] of sortedEntries) { | ||
| if (normalizedDir === boundDir) { | ||
| return { profile, dir: boundDir }; | ||
| } | ||
| if ( | ||
| normalizedDir.startsWith(boundDir) && | ||
| normalizedDir[boundDir.length] === path.sep | ||
| ) { | ||
| return { profile, dir: boundDir }; | ||
| } | ||
| } | ||
|
|
||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🚩 Directory binding prefix matching uses string comparison — correct on POSIX, needs Windows case-insensitivity consideration
The getProfileForDirectoryFromBindings function at profiles.ts:148-171 uses path.resolve() and exact string comparison (normalizedDir === boundDir) plus startsWith for prefix matching. On Windows, file paths are case-insensitive, so C:\Projects and c:\projects should match. path.resolve() normalizes separators but does NOT normalize case on Windows. If a user activates a profile for C:\Projects but their cwd resolves to c:\projects, the match would fail. This is a pre-existing pattern (many tools have this issue on Windows), and since path.resolve() typically preserves the case from the OS, it may rarely trigger in practice.
Was this helpful? React with 👍 or 👎 to provide feedback.
emily-shen
force-pushed
the
emily/profiles-2
branch
from
7d67d39 to
3170b68
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
There was a problem hiding this comment.
🔴 getAPIToken() and requireApiToken() always read from default storage, ignoring the active profile
The getAPITokenInternal and requireApiTokenInternal functions in flow.ts always use the static storage (the default/no-profile backend) rather than profile-aware getStorage(profile). This means that while loginOrRefreshIfRequired correctly refreshes the token for the active profile (e.g. writing to work.toml), subsequent credential resolution via requireApiToken() reads from the default storage (default.toml). Every API call in wrangler goes through requireApiToken() in packages/wrangler/src/cfetch/internal.ts:69 and :157, so commands like wrangler deploy will authenticate with the default profile's credentials even when a non-default profile is active.
Affected code in flow.ts
At packages/workers-auth/src/flow.ts:452-462, getAPITokenInternal passes storage (the default) rather than getStorage(profile). Same issue at lines 464-474 for requireApiTokenInternal. Meanwhile, all the login/refresh/logout functions correctly use getStorage(props.profile) or getStorage(profile).
(Refers to lines 452-462)
Prompt for agents
The getAPITokenInternal and requireApiTokenInternal functions in flow.ts (lines 452-474) always use the default `storage` rather than the profile-aware `getStorage(profile)`. These are the functions that actually resolve API credentials for all wrangler API calls via packages/wrangler/src/cfetch/internal.ts.
The fix requires threading the active profile into these functions. Since these are synchronous and called without a profile argument currently, the simplest approach would be to either:
1. Accept an optional profile parameter on getAPIToken/requireApiToken (matching the pattern of other profile-aware functions in the flow), OR
2. Have the wrangler-side wrapper (packages/wrangler/src/user/user.ts getAPIToken/requireApiToken) read getProfile() from the experimental-flags store and construct a profile-specific storage to pass.
The current OAuthFlowAPI interface for getAPIToken() and requireApiToken() doesn't accept a profile parameter, so the interface would need updating too. The wrangler consumer in packages/wrangler/src/user/user.ts at lines 245-253 would also need to pass the profile.
Was this helpful? React with 👍 or 👎 to provide feedback.
| normalizeCwd( | ||
| normalizeSlashes( | ||
| normalizeTempDirs(stripTimings(replaceThinSpaces(input))) |
There was a problem hiding this comment.
🔴 normalizeCwd fails on Windows after normalizeSlashes strips drive letters and converts backslashes
The reordering of normalizeCwd and normalizeSlashes in the normalization pipeline breaks the cwd replacement on Windows. Previously, normalizeCwd ran before normalizeSlashes, so it could match process.cwd() (e.g. C:\Users\runner\tmp) against the raw string before backslash conversion. Now normalizeSlashes runs first, converting C:\Users\runner\tmp to /Users/runner/tmp and stripping the drive letter. Then normalizeCwd tries to replaceAll(process.cwd(), "<cwd>") — but process.cwd() on Windows still returns C:\Users\runner\tmp, which no longer exists in the already-normalized string. This means test snapshots on Windows would contain leaked absolute paths instead of <cwd>.
| normalizeCwd( | |
| normalizeSlashes( | |
| normalizeTempDirs(stripTimings(replaceThinSpaces(input))) | |
| normalizeCwd( | |
| normalizeSlashes( | |
| normalizeTempDirs(stripTimings(replaceThinSpaces(input))) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const resolved = profile ?? "default"; | ||
| let fileName: string; | ||
| if (resolved === "default") { | ||
| const environment = getCloudflareApiEnvironmentFromEnv(); | ||
| fileName = | ||
| environment === "production" ? "default.toml" : `${environment}.toml`; | ||
| } else { | ||
| fileName = `${resolved}.toml`; | ||
| } |
There was a problem hiding this comment.
🚩 Non-default profile names bypass the staging/production environment file-name logic
In both packages/workers-auth/src/profiles.ts:29-36 and packages/wrangler/src/user/auth-config-file.ts:91-99, the getAuthConfigFilePath function only consults getCloudflareApiEnvironmentFromEnv() for the "default" profile. Named profiles like "work" always map to work.toml regardless of whether the user is targeting staging or production. This means a user with a "work" profile would use the same token file for both staging and production environments. This appears intentional — named profiles represent a single identity — but could surprise users who switch environments.
Was this helpful? React with 👍 or 👎 to provide feedback.
emily-shen
force-pushed
the
emily/profiles-2
branch
from
3170b68 to
79fb374
Compare
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)