Add wdl token: a local credential store#1
Conversation
`wdl token set/list/use/rm` stores control URLs and tokens in ~/.config/wdl/credentials, so commands resolve them without a per-shell ADMIN_TOKEN or a token in every project's .env. There's no "login": `set` reads the token from stdin, validates it against /whoami, confirms its principal matches the namespace, then stores it; `rm` deletes the local copy without revoking it. The store reuses the project-.env dialect (one parser), is written 0600, and is the lowest-precedence credential layer (flag > shell env > project .env > store). It's trusted and exempt from the cross-origin guard, but a project .env endpoint is still dropped when its token isn't from that .env, so an untrusted cwd can't redirect a stored token. A base WDL_NS names a default namespace (like a project .env's base WDL_NS): the first stored namespace becomes the default, `--default` / `use` change it, `list` marks it `*`, selection is --ns > shell WDL_NS > .env WDL_NS > store default. CHANGELOG sits under Unreleased; the version bump and tag are a separate release commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6fdcab0b4
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const token = (await readStdin(context.stdin, { | ||
| prompt: `Token for ${ns} @ ${controlUrl} (input hidden): `, | ||
| stderr: context.stderr, |
There was a problem hiding this comment.
When stdin is an interactive TTY, this calls readTtyLine, which only registers data/end listeners and never disables terminal echo. So wdl token set --ns ... displays the token in the terminal/scrollback even though the prompt and docs say input is hidden; use a hidden-input implementation or avoid advertising hidden input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in ac69f99: readTtyLine now switches the TTY to raw mode (echo off) for hidden input and wdl token set passes hidden: true. It also fails closed — it errors rather than echo if raw mode can't be enabled.
| const nextSection = parseDotEnvSection(line, idx + 1); | ||
| if (nextSection !== null) { | ||
| section = nextSection; | ||
| namespaces[section] ??= {}; |
There was a problem hiding this comment.
For a valid tenant namespace such as constructor, this inherited-property lookup sees Object.prototype.constructor instead of creating an own section entry. The parsed namespace is then omitted by Object.keys() in token list/writeTokenStore, and commands like wdl token use constructor can rewrite the credentials file without the token section; use a null-prototype map or an own-property check before assigning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in ac69f99: sections are created with Object.defineProperty and looked up with Object.hasOwn throughout, so constructor/toString/__proto__ namespaces round-trip without vanishing from Object.keys or touching the prototype.
| .replaceAll("\\", "\\\\") | ||
| .replaceAll('"', '\\"') | ||
| .replaceAll("\n", "\\n") | ||
| .replaceAll("\r", "\\r") | ||
| .replaceAll("\t", "\\t"); |
There was a problem hiding this comment.
When a stored token or label contains a literal backslash followed by n, r, t, or ", this escaping writes doubled backslashes, but parseDotEnvValue later decodes the second backslash sequence first. For example abc\\ndef is read back as abc\\\ndef, corrupting the validated token and causing later commands to authenticate with the wrong value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in ac69f99: parseDotEnvValue now unescapes in a single left-to-right pass (unescapeDoubleQuoted), so a literal backslash sequence round-trips intact.
|
|
||
| // Read the store once: it supplies both the lowest-precedence default | ||
| // namespace and the per-namespace control URL + token gap-fills below. | ||
| const store = readStore(env) || {}; |
There was a problem hiding this comment.
Because the token store is read unconditionally here, a malformed or unreadable ~/.config/wdl/credentials aborts autoloaded commands even when the user supplies higher-precedence credentials such as --ns, --control-url, and --token. In that scenario the documented lowest-precedence store is not needed, so users cannot work around a corrupt store except by fixing or deleting it; defer reading it until a store default or gap-fill is actually required.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in ac69f99: the store is read lazily — only to supply a default namespace or fill an empty, non-flag credential slot — so a corrupt store no longer aborts a command whose credentials come from flags/shell/.env.
Address Codex review on #1: - Hide the token on a TTY. readTtyLine now switches to raw mode (echo off) for hidden input; `set` previously echoed the typed token into the terminal and scrollback despite its "(input hidden)" prompt. Hidden input fails closed — if raw mode cannot be enabled it errors rather than silently echo a secret. `wdl secret put` reads its value through the same hidden path (identical issue). - Preserve namespaces named like Object.prototype keys (constructor, toString, __proto__). readTokenStore built sections with `??=`/assignment on a plain object, so those names matched an inherited member (and `__proto__` hit the prototype setter), and the section vanished from Object.keys; sections are now created with Object.defineProperty and looked up with own-property checks throughout the store and command paths. - Round-trip literal backslash escapes. parseDotEnvValue unescaped with chained replaceAll in an order that turned a stored "\n" (backslash + n) into a newline, corrupting tokens; replace it with a single left-to-right scan. - Read the global store lazily. It is the lowest-precedence, optional layer, so a corrupt ~/.config/wdl/credentials no longer aborts a command whose namespace and credentials already come from flags / shell / .env — the store is consulted only to supply a default namespace or fill an empty, non-flag credential slot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac69f99a50
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const covered = { CONTROL_URL: controlUrlFromFlag, ADMIN_TOKEN: tokenFromFlag }; | ||
| const needsFill = STORE_ENV_KEYS.some((k) => !covered[k] && (env[k] == null || env[k] === "")); | ||
| if (needsFill) fillFromTokenStore(env, ns, getStore().namespaces || {}, onLoad, covered); |
There was a problem hiding this comment.
When a user configures the endpoint with the compatibility env var ADMIN_URL and relies on the token store for the missing token, this still treats the endpoint slot as empty because only CONTROL_URL is checked. fillFromTokenStore then writes the store's CONTROL_URL, and resolveControlUrl() prefers that over ADMIN_URL, so a lower-precedence stored endpoint can override a shell/CI ADMIN_URL and send the command to the wrong control plane. Treat ADMIN_URL as covering the endpoint before filling from the store.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — resolved in 5458b19 by removing the --admin flag and the ADMIN_URL env var entirely, rather than special-casing ADMIN_URL in the store gap-fill. They were legacy compatibility aliases for --control-url/CONTROL_URL with no remaining consumer (the upstream integration harness always sets CONTROL_URL too), so there is no ADMIN_URL left to honor. Noted as BREAKING in the CHANGELOG; use --control-url / CONTROL_URL.
| if (store.defaultNs === ns) { | ||
| const remaining = Object.keys(store.namespaces); | ||
| store.defaultNs = remaining.length === 1 ? remaining[0] : null; |
There was a problem hiding this comment.
After the default has already been cleared by an earlier removal from an ambiguous set, deleting another non-default namespace can leave exactly one stored token, but this branch does not run and defaultNs stays null. For example, store acme, demo, and prod, remove default acme so the default is cleared, then remove demo; prod is now the only entry but commands without --ns still cannot use it. Promote the sole survivor after any removal, not only when the removed namespace was the current default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5458b19: wdl token rm now promotes a sole surviving namespace to default after any removal (not only when the removed one was the current default), so removing the default from an ambiguous set and then removing another entry no longer leaves the last one without a usable default. Regression test added.
BREAKING: drop the `--admin` flag and the `ADMIN_URL` environment variable —
legacy compatibility aliases for the control endpoint. Use `--control-url` and
`CONTROL_URL` instead. `--admin` is now an unknown option and `ADMIN_URL` is no
longer read from the shell or `.env`. Removed from the option/preset defs,
`resolveControlUrl`'s fallback chain, the cross-origin guard's endpoint-key set,
the `.env` key set (so it is also no longer stripped from the wrangler child
env), `config explain`'s source labels, and the GUIDE. This also resolves
Codex's "honor ADMIN_URL before the store" finding by removing the alias
outright — there is no ADMIN_URL left to honor.
Also promote a sole surviving namespace as the default after any `wdl token rm`,
not only when the removed namespace was the current default: removing the
default from {a,b,c} clears it, and a later removal down to one entry now makes
that entry the default again instead of leaving the store with no usable
default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5458b194eb
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const storePath = tokenStorePath(context.env); | ||
| const store = readTokenStore(storePath); | ||
| const previous = Object.hasOwn(store.namespaces, ns) ? store.namespaces[ns] : {}; | ||
| store.namespaces[ns] = { |
There was a problem hiding this comment.
When the namespace is the operator-reserved shape __proto__, wdl token set validates it and reports success, but this assignment hits Object.prototype's __proto__ setter on the fresh {} returned by readTokenStore, so no own section is created and writeTokenStore later omits the token and default. Fresh evidence beyond the earlier prototype-safety thread is that this write path still uses bracket assignment for a new namespace; create the section with Object.defineProperty or a null-prototype map before assigning fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4be3d3e: token set now creates the section with Object.defineProperty instead of store.namespaces[ns] = …, so a __proto__ namespace lands as an own section (mirrors readTokenStore) rather than hitting the prototype setter and vanishing on write. Regression test added.
| // The first stored namespace (no default yet), or an explicit --default, | ||
| // becomes the default used when --ns/WDL_NS is omitted — the store's analogue | ||
| // of a base WDL_NS in a project .env. | ||
| const becameDefault = Boolean(values.default) || !store.defaultNs; |
There was a problem hiding this comment.
If the default was intentionally cleared because multiple namespaces remain (for example, remove the default from a three-token store), any later wdl token set --ns <other> without --default makes that namespace the default solely because store.defaultNs is null. That silently changes where commands without --ns go, even though the ambiguous store still has multiple entries and docs/tests say later sets should not steal the default; only the first stored namespace (or an explicit --default) should do this.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4be3d3e: a set now auto-claims the default only for the first stored namespace (an empty store) or an explicit --default, captured as wasEmpty before the write. A set into a still-ambiguous store whose default was deliberately cleared no longer steals it. Regression test added.
| tokenFromFlag: scanned.tokenFromFlag, | ||
| controlUrlFromFlag: scanned.controlUrlFromFlag, | ||
| loadEnv: loadEnvOverride, | ||
| readStore: (e) => readTokenStore(tokenStorePath(e)), |
There was a problem hiding this comment.
Because the dispatcher now provides readStore to every autoloaded command, wdl init demo --ns acme still enters loadCliControlEnv; commands/init.js has autoloadEnv: true but never needs control credentials, so when ADMIN_TOKEN/CONTROL_URL are unset the gap-fill path reads ~/.config/wdl/credentials. A malformed or unreadable token store then aborts project scaffolding even though the store is irrelevant to init; mark init as not autoloading credentials or only wire the store for commands that actually need control credentials.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4be3d3e: init is now autoloadEnv: false — it only scaffolds files locally and never enters loadCliControlEnv, so it reads neither .env control vars nor the token store, and a corrupt store can no longer abort scaffolding. (Same commit also makes wdl init's --ns optional, since the namespace is a deploy-time concern.)
token set (Codex round 3): - Create the namespace section with Object.defineProperty instead of `store.namespaces[ns] = …`, so a "__proto__" namespace lands as an own section rather than hitting the prototype setter and vanishing from the written store (mirrors readTokenStore's section creation). - Only auto-set the default for the first stored namespace (an empty store) or an explicit --default. Previously any set with `store.defaultNs` null claimed the default — including after it was deliberately cleared by removing it from an ambiguous set — silently changing where no-`--ns` commands go. wdl init: - `--ns` is now optional: the namespace is a deploy-time concern (resolved from `--ns` / `WDL_NS` / a project `.env` / a `wdl token` default). With `--ns` the scaffolded deploy script stays `wdl deploy . --ns <ns>`; without it it is `wdl deploy .` and the next-steps output explains how to supply it. - init no longer autoloads control credentials (autoloadEnv: false); it only scaffolds files locally, so a corrupt token store can no longer abort scaffolding (Codex "skip token-store reads for init"). - The README AI-usage steps now point at `wdl doctor` / `wdl token set` for credential setup — credentials may resolve from shell env, a project `.env`, or the token store — instead of mandating WDL_NS/ADMIN_TOKEN/CONTROL_URL in a shell rc, and no longer assume `--ns` at init time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4be3d3e3b8
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // so the rest of the pipeline (control-URL resolution, the [ns] overlay, | ||
| // resolveNamespace in callers) sees the same namespace an explicit one would. | ||
| if (!ns) { | ||
| const s = getStore(); |
There was a problem hiding this comment.
For namespace-optional diagnostics such as wdl whoami --control-url ... --token ... (and config explain/doctor), no namespace is needed because /whoami can report the token principal, but this default lookup still calls readStore() whenever --ns/WDL_NS is absent. Fresh evidence beyond the earlier lazy-read fix is that a corrupt ~/.config/wdl/credentials still throws here before the explicit flag credentials are used, so users cannot validate or inspect a token without first repairing an unrelated store; only consult the store default when the caller actually needs a namespace or the store can fill a missing credential.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5d17dda: the default-namespace lookup now tolerates a corrupt/unreadable store (it is the lowest-precedence, optional ns source), so a broken ~/.config/wdl/credentials no longer aborts a command that needs no namespace — e.g. wdl whoami --control-url … --token …, which gets the namespace from /whoami. The credential gap-fill read stays strict, so a store that is the actual credential source still surfaces its corruption, and wdl token commands report it directly. Regression test added.
| writeFileSync(storePath, lines.join("\n"), { mode: 0o600 }); | ||
| // writeFileSync's mode only applies on create; force perms on an existing file. | ||
| chmodSync(storePath, 0o600); |
There was a problem hiding this comment.
When the credentials file already exists with permissive permissions (for example, a user hand-created or restored it as 0644), writeFileSync(..., { mode: 0o600 }) does not change that existing mode; the new token bytes are written while the file is still readable, and a crash or chmod failure leaves them exposed. Chmod or replace with a 0600 temp file before writing the secret contents so wdl token set never stores a fresh token under the old permissions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5d17dda: writeTokenStore now chmods an existing file to 0600 before writing the token bytes (writeFileSync's mode only applies when it creates the file), so a pre-existing permissive (e.g. 0644) file never holds the secret while still world-readable; a non-ENOENT chmod failure aborts before the token is written. Regression test added.
- The default-namespace lookup in loadCliControlEnv now tolerates a corrupt or unreadable token store. The default ns is the lowest-precedence, optional namespace source, so a broken ~/.config/wdl/credentials must not abort a command that needs no namespace (e.g. `wdl whoami --control-url … --token …`, which gets the namespace from /whoami). The credential gap-fill read stays strict, so a store that is the actual credential source still surfaces its corruption, and the `wdl token` commands report it directly. - writeTokenStore tightens an existing credentials file to 0600 BEFORE writing the token bytes (writeFileSync's mode only applies when it creates the file), so a pre-existing permissive (e.g. 0644) file never holds the secret while still world-readable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d17ddaf84
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ns) { | ||
| const covered = { CONTROL_URL: controlUrlFromFlag, ADMIN_TOKEN: tokenFromFlag }; | ||
| const needsFill = STORE_ENV_KEYS.some((k) => !covered[k] && (env[k] == null || env[k] === "")); | ||
| if (needsFill) fillFromTokenStore(env, ns, getStore().namespaces || {}, onLoad, covered); |
There was a problem hiding this comment.
When the selected project .env contains a control endpoint plus an empty ADMIN_TOKEN= placeholder, the guard above treats the endpoint as same-source because ADMIN_TOKEN was loaded, but this gap-fill then replaces the empty token from the store while leaving the .env endpoint in place. In that context, an untrusted project can redirect the stored token to its own CONTROL_URL/CONTROL_CONNECT_HOST; only skip the guard for a non-empty dotenv token, or drop endpoints again before filling from the store.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 56da8e5: the cross-origin guard now treats a .env control endpoint as same-source only when the .env supplies a NON-EMPTY ADMIN_TOKEN. An empty ADMIN_TOKEN= placeholder no longer marks the endpoint trusted while the gap-fill afterwards supplies the real token from the store, so an untrusted .env can no longer redirect the stored token. Regression test + e2e added.
| if (becameDefault) store.defaultNs = ns; | ||
| writeTokenStore(storePath, store); | ||
| context.stdout( | ||
| `Stored token for ${escapeTerminalText(ns)} @ ${escapeTerminalText(controlUrl)} (${maskToken(token)}).` |
There was a problem hiding this comment.
When the token's last four characters contain terminal controls (for example a pasted token ending in ESC[2J), maskToken(token) preserves that suffix and this success path writes it directly to stdout, unlike token list which goes through writeResult. In that scenario wdl token set can emit terminal control sequences even though the token is supposed to be masked; escape the masked value before printing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 56da8e5: token set now wraps the masked suffix in escapeTerminalText ((${escapeTerminalText(maskToken(token))})), matching what token list already gets via writeResult. Regression test added.
| // defineProperty, not `store.namespaces[ns] = …`: a namespace named "__proto__" | ||
| // would otherwise hit the prototype setter and never create an own section | ||
| // (mirrors readTokenStore's section creation). | ||
| Object.defineProperty(store.namespaces, ns, { |
There was a problem hiding this comment.
When a self-hosted or misconfigured control plane returns a principal namespace containing section syntax such as ] or a newline, and the user passed that same --ns, this stores the raw string as a section key; writeTokenStore later serializes it as [${ns}], so the injected characters become extra lines or sections and the credentials file is corrupted on the next read. Validate the namespace against the same acceptable namespace grammar before adding it to the store.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 56da8e5: token set validates the namespace with isAdminAcceptableNs (the same grammar store/.env sections use) before storing, so a value containing ] or newlines is rejected and can't inject lines/sections into the credentials file. __proto__/__name__ operator-reserved forms are still accepted. Regression test added.
| if (principalNs !== ns) { | ||
| throw new CliError( | ||
| principalNs | ||
| ? `token principal is namespace "${principalNs}", not "${ns}" — run with --ns ${principalNs}` |
There was a problem hiding this comment.
When /whoami returns a mismatched namespace containing terminal control bytes, this error interpolates the control-plane value directly and handleCliError prints it raw. In that scenario a malicious or misconfigured control URL can emit escape sequences even though other control-plane output is escaped; escape principalNs (and the suggested --ns value) before building the message.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 56da8e5: the principal-mismatch error now escapes principalNs and ns with escapeTerminalText before interpolation, so a malicious/misconfigured control plane can't emit terminal escape sequences through it. Regression test added.
- P1: the cross-origin .env guard now treats a .env control endpoint as same-source only when the .env supplies a NON-EMPTY ADMIN_TOKEN. An empty `ADMIN_TOKEN=` placeholder no longer marks a malicious .env endpoint trusted while the gap-fill afterwards supplies the real token from the store — which would have sent the stored token to a host the .env chose. - token set validates the namespace against the section grammar (isAdminAcceptableNs) before storing, so a value containing `]` / newlines (e.g. echoed back via --ns from a misconfigured control plane) can no longer inject lines/sections and corrupt the credentials file on the next read. - token set escapes terminal control bytes in the masked token suffix it prints and in the principal-mismatch error message, so a pasted token or a malicious control plane cannot emit escape sequences to the terminal (token list already escaped via writeResult; the set path did not). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A self-review found that `token use` / `token rm` interpolate the user-supplied namespace into their "no stored token" errors without escapeTerminalText, while token set's errors and the use/rm success messages already escape it. handleCliError prints the message raw, so a --ns / positional containing terminal control bytes could emit escape sequences. Wrap ns in escapeTerminalText, matching the rest of the command's output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79d4be010d
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| function tokenRemove({ context }) { | ||
| const ns = context.resolveNamespace(); |
There was a problem hiding this comment.
When WDL_NS is set in the shell, this resolves that ambient namespace, so a bare wdl token rm deletes the stored token even though the usage and error text say rm requires --ns. Because rm is destructive and has no confirmation, users who omit the flag expecting a missing-argument error can remove the active namespace's local credential; check the parsed --ns value explicitly for this subcommand instead of falling back to WDL_NS.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93d4506. token rm now takes the namespace only from an explicit --ns (via flagSet), never the ambient WDL_NS — a bare wdl token rm errors with "requires an explicit --ns" instead of acting on WDL_NS. The convenience fallback stays for the non-destructive set/use. Added a test asserting a stray WDL_NS=acme does not let a bare rm delete acme.
| export const CLI_DOTENV_KEYS = new Set([ | ||
| "ADMIN_TOKEN", | ||
| "ADMIN_URL", | ||
| "CONTROL_CONNECT_HOST", | ||
| "CONTROL_URL", | ||
| "WDL_NS", |
There was a problem hiding this comment.
wranglerChildEnv deletes only keys from CLI_DOTENV_KEYS, so removing ADMIN_URL here means users who still have the legacy WDL control endpoint exported will now pass it through to Wrangler during deploy/doctor dry-runs and any build scripts Wrangler runs. Even if the alias is no longer read for CLI configuration, it is still a WDL control-plane variable that used to be scrubbed alongside CONTROL_URL; keep it in a separate child-env denylist or otherwise delete it before spawning Wrangler.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 31dd421. Root cause: CLI_DOTENV_KEYS was doing double duty — the .env load allowlist and the wrangler-child scrub denylist. Removing the ADMIN_URL alias dropped it from both, so it silently stopped being scrubbed.
Decoupled the two: WRANGLER_SCRUB_KEYS = CLI_DOTENV_KEYS + {ADMIN_URL}. The CLI no longer reads the alias, but still strips it before spawning wrangler so a stale shell/CI export can't leak the control endpoint into the bundler or the build scripts wrangler runs. (Note: re-adding it to CLI_DOTENV_KEYS would have re-enabled reading it, which we intentionally removed — hence a separate set.) Extended the wranglerChildEnv test to assert ADMIN_URL is stripped from the child env.
Behavior-preserving cleanup (342 tests / lint / typecheck green):
Dedup:
- `readStdin` was copied identically in `token set` and `secret put` → extracted
to `readSecretStdin` in common.js (next to readTtyLine), with direct tests for
the hidden-TTY / pipe-EOF / trailing-newline contract.
- the dotenv encoder `quoteValue` moved from token-store.js to common.js beside
its decoder (parseDotEnvValue / unescapeDoubleQuoted) so the round-trip pair
lives in one file; the store imports it.
- the six `typeof values[x] === "string" && length > 0` flag checks across
bin/wdl.js and config-state.js → a `flagSet(values, name)` helper.
- `maskToken` moved from config-state.js to common.js, so `wdl token` no longer
depends on config-state for a display util.
Output hardening:
- added `writeStatusLine` (the non-JSON analogue of writeResult): a choke point
that escapes a single human status line once, so callers interpolate raw
values without per-field escaping. `wdl secret`, `wdl token`, `wdl deploy`, and
`wdl r2` route their status output through it; this also neutralizes
previously-raw user-controllable fields (secret's `scopeLabel`/`keyArg`,
deploy's upload + `✓ … live` lines carrying `ns`/`workerName`, r2's `--out`
path). Error messages and multi-line stderr notes still escape inline — they
are not single-line stdout.
- added `writeJsonOr`: the `--json` half of a compound command's output (emit
body as machine JSON, else defer to the human path), replacing the repeated
`if (json) { stdout(JSON.stringify(…)); return; }` branch in `wdl secret`. It
and writeResult emit machine JSON through one `writeJson(stdout, body)` so the
format can't drift.
- prompts and confirmations escape at their write points: readTtyLine escapes
the prompt (covering `token set` / `secret put` and every `[y/N]` confirm) and
confirmAction escapes the `action` in its refusal error. This hardens the raw
user args that the d1 / r2 / delete / workflows confirms previously
interpolated unescaped, and lets `wdl secret` drop all per-field escaping
(stdout via writeStatusLine, prompts via the choke point).
Left `loadCliControlEnv` and the two INI parse loops as-is on purpose: the
former is a security-ordered linear pipeline (guard before fill), the latter
have genuinely different rules — merging either would obscure more than it saves.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
token rm took its namespace through resolveNamespace(), which falls back to the ambient WDL_NS. Since rm deletes and rewrites the credential store with no confirmation, a stray WDL_NS could make a bare `wdl token rm` silently delete a different namespace's token than the operator named. Take the namespace only from an explicit --ns (via flagSet); the convenience fallback stays for the non-destructive set/use subcommands. Pin ws to ^8.21.0 via overrides to clear GHSA-96hv-2xvq-fx4p (high) in the wrangler -> miniflare -> ws@8.20.1 chain that fails CI's `npm audit --audit-level=moderate`. The DoS needs miniflare's dev server (wrangler dev); wdl deploy only runs `wrangler deploy --dry-run` and never starts it. `npm audit fix --force` would downgrade wrangler 4 -> 3.107.3 (major, breaking), and even the latest wrangler still pins the vulnerable ws, so the override -- just past the vulnerable range, the only ws in the tree -- is the clean fix. Audit clean, suite green, a real dry-run still bundles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Removing the ADMIN_URL alias also dropped it from CLI_DOTENV_KEYS, which is the same set wranglerChildEnv uses to strip WDL control-plane config before spawning Wrangler. So a user who still has a stale ADMIN_URL exported would now leak the control endpoint into the bundler and any build scripts Wrangler runs during deploy / doctor dry-runs -- a regression, since the alias used to be scrubbed alongside CONTROL_URL. Decouple the scrub denylist from the .env load allowlist: WRANGLER_SCRUB_KEYS is CLI_DOTENV_KEYS plus the legacy ADMIN_URL. The CLI no longer READS the alias, but dropping it as a config input must not turn it into a leak. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31dd421872
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| function tokenUse({ context, nsArg }) { | ||
| const ns = nsArg || context.resolveNamespace(); |
There was a problem hiding this comment.
When WDL_NS is set in the shell, a bare wdl token use falls through to context.resolveNamespace() and rewrites the token-store default to that ambient namespace instead of showing the documented missing-argument error. Because this command mutates where future commands without --ns go, users who omit the required <namespace> can silently switch the default; use only nsArg here (or document and confirm the --ns/env behavior explicitly).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in da74f66. token use now takes the namespace from its positional or an explicit --ns only, never the ambient WDL_NS — a bare wdl token use errors instead of silently switching the default. The fallback was also pointless: WDL_NS already overrides the store default at resolution time, so inheriting it here would only reswitch the default to a namespace the user didn't name. A test asserts a stray WDL_NS doesn't let a bare use switch the default.
| } | ||
|
|
||
| async function tokenSet({ values, context }) { | ||
| const ns = context.resolveNamespace(); |
There was a problem hiding this comment.
When WDL_NS is exported and the pasted token belongs to that namespace, wdl token set --control-url ... succeeds even though the user omitted the documented --ns: it creates or overwrites that store entry and, on an empty store, makes it the default. That mutates the global credential store in a command the usage/error text says should reject, so this subcommand should check the parsed --ns flag explicitly before falling back to ambient namespace resolution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in da74f66. token set now takes the namespace from an explicit --ns only (via flagSet), never the ambient WDL_NS — wdl token set --control-url ... without --ns errors "token set requires --ns " even when WDL_NS is exported. Same change applied to use (and rm earlier), since all three mutate the global store. Also gave token its own --ns help text, since the shared preset still advertised "(env: WDL_NS)" which no longer applies here. A test asserts a stray WDL_NS=acme can't make a bare set store under acme.
Follow-up to 93d4506: the same ambient-WDL_NS hazard applies to the other two store-mutating subcommands. set/use/rm all name which global store entry to create, switch, or delete, so that target must come from an explicit --ns/positional, not a WDL_NS a user may have exported for an unrelated deploy. - set fell back to resolveNamespace(), so `token set --control-url ...` with WDL_NS exported would store (and on an empty store, default to) that namespace despite usage documenting --ns as required. - use fell back too, so a bare `token use` would silently switch the default to WDL_NS -- pointless as well as surprising, since WDL_NS already overrides the store default at resolution time. Both now take the namespace explicitly (via flagSet; use still accepts its documented positional). `token` also gets its own --ns help text, since the shared preset still advertised "(env: WDL_NS)", which no longer applies. Also de-staled rm's comment. Tests assert a stray WDL_NS cannot make a bare set/use mutate the store. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da74f66e45
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - [deploy.md](./deploy.md) — `ADMIN_TOKEN` / `CONTROL_URL` precedence and the | ||
| `.env` layout the store sits beneath. |
There was a problem hiding this comment.
AGENTS.md's Documentation Sync Policy says the per-feature docs are authoritative and that English/Chinese pairs must be updated together when command/env precedence changes. This new token reference sends readers to deploy.md for ADMIN_TOKEN/CONTROL_URL precedence, but docs/deploy.md/docs/deploy-zh.md still describe the chain as stopping at .env and say --ns is optional only with WDL_NS, so users and generated-project agents following the deploy docs will miss the token-store fallback/default namespace and may keep putting tokens in project .env files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 738ba3b. deploy.md/-zh now extend the precedence chain with the wdl token store as the lowest layer and note --ns is optional with a stored default — and, going further, flip the recommendation: the token store is the recommended setup (0600, validated, out of every repo, default ns ⇒ no --ns), a project .env is the per-repo alternative, and CI injects env vars. GUIDE.md/-zh mark the store as the recommended setup; README.md/-zh add it to the precedence aside (and split the wdl token synopsis per subcommand so it no longer implies --ns is optional for set/rm). token.md/-zh also state that set/use/rm take an explicit --ns only, never ambient WDL_NS.
To close the loop on the "users keep putting tokens in .env" concern: the Missing admin token troubleshooting rows — and the error message itself (lib/common.js, lib/whoami.js) — now lead with wdl token set, so someone hitting a missing token isn't sent back to the .env model. The wdl-deploy skill and templates/AGENTS.md already defer to deploy.md.
…n error docs/token.md pointed readers to deploy.md for ADMIN_TOKEN/CONTROL_URL precedence, but the deploy docs still stopped the chain at .env, said --ns is optional only with WDL_NS, and called a project .env the "recommended path" -- so readers and generated-project agents kept the token in .env files and never met the store or default namespace. AGENTS.md's Documentation Sync Policy makes the bilingual pairs + GUIDE authoritative. - deploy.md/-zh: flip the recommendation -- the `wdl token` store is now the recommended setup (0600, validated, out of every repo, default ns => no --ns); a project .env is the per-repo alternative; CI injects env vars. Extend the precedence chain with the store as the lowest layer; note --ns is also optional with a stored default. - GUIDE.md/-zh: mark the managed store as the recommended setup. - README.md/-zh: include the store in the precedence aside; split the `wdl token` command synopsis per subcommand so it no longer implies --ns is optional for set/rm (it is required for set/rm, positional/--ns for use). - token.md/-zh: state that set/use/rm are the exception to the namespace chain -- explicit --ns only, never ambient WDL_NS. - deploy.md/-zh, GUIDE.md/-zh: the "Missing admin token" troubleshooting row now leads with `wdl token set`. - lib/common.js, lib/whoami.js: the "Missing admin token" error itself now leads with `wdl token set` (recommended), then --token / ADMIN_TOKEN -- the surface a user hits directly. Existing tests match the "Missing admin token" prefix, so they still pass. The wdl-deploy skill and templates/AGENTS.md already defer to deploy.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 738ba3b2aa
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `CONTROL_URL`, `CONTROL_CONNECT_HOST`, and `WDL_NS`. Precedence is | ||
| `CLI flag > shell/CI env > [resolved-ns] section > base .env`, and if none | ||
| supplies a value the command fails — there is no built-in default. Namespace | ||
| resolution is `--ns`, then `WDL_NS` from your shell or base `.env`. Section |
There was a problem hiding this comment.
This full-manual precedence text still says resolution ends at the base .env and that the command fails if none of those layers supplies a value, but the new loadCliControlEnv falls through to the wdl token store and can also source the default namespace from it. Users following GUIDE/GUIDE-zh will be told the recommended token store cannot satisfy deploy on its own; update this chain and the namespace sentence to include the token store (the Chinese paragraph mirrors the same omission).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6f8a430. Extended the GUIDE precedence chain (EN + ZH) with the wdl token store as the lowest layer, and the namespace-resolution sentence with the store's default namespace — matching deploy.md / token.md. The earlier sync marked the store as recommended in GUIDE but left these two lines stopping at base .env.
| if (def && Object.hasOwn(namespaces, def)) { | ||
| ns = def; | ||
| if (env.WDL_NS == null || env.WDL_NS === "") { | ||
| env.WDL_NS = ns; |
There was a problem hiding this comment.
When WDL_NS is present in the shell/CI environment but empty, protectedKeys still prevents the base .env WDL_NS from loading; because the empty value is then treated as absent, this new fallback materializes the token-store default into env.WDL_NS. In that CI/shell-empty case a lower-precedence store default can override the project's .env namespace and make wdl deploy target the wrong namespace; let .env fill empty protected values before selecting the store default, or do not fall through to the store when an empty protected namespace blocked .env.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6f8a430. An empty value is "absent" everywhere else here (firstNonEmptyString / flagSet), so it must not protect its key — protectedEnvKeys now protects only keys with a non-empty string value, letting the project .env fill the slot an empty shell WDL_NS left open and restoring .env > store-default (this also closes the same latent inversion for ADMIN_TOKEN / CONTROL_URL).
Two follow-ups in the same commit: the identical override in resolveCliConfigState is fixed too — otherwise config explain / doctor / whoami would have reported a different namespace/control/token than an operating command actually uses (the worst kind of diagnostic bug). And the predicate now also excludes undefined / non-string values, so injected plain-object envs follow the same "non-empty string = set" convention. Regression tests cover the runtime loader, resolveCliConfigState, and the predicate itself.
Code: when WDL_NS is present-but-empty in the shell/CI environment (e.g. WDL_NS="" in a CI job), it landed in protectedKeys, so the base .env's WDL_NS was skipped; firstNonEmptyString then treated the empty value as absent, and the token-store fallback materialized the store's default namespace into env.WDL_NS. Net: a lower-precedence store default overrode the project's .env namespace, so wdl deploy could target the wrong namespace. An unset value is "absent" everywhere else here, so it must not protect its key. Extract protectedEnvKeys (protect only keys with a non-empty string value, matching firstNonEmptyString / flagSet -- so "" and an injected `undefined` both count as unset) and use it for both the loadCliControlEnv / loadCliDotEnv defaults AND resolveCliConfigState, which built its own protectedKeys with the old behavior -- otherwise config explain / doctor / whoami would resolve a different namespace/control/token than an operating command. Restores .env > store-default (also closes the same latent inversion for ADMIN_TOKEN / CONTROL_URL). Regression tests cover both the runtime loader and resolveCliConfigState. Docs: the earlier sync (738ba3b) marked the store as recommended in GUIDE but left the precedence line stopping at base .env and the namespace sentence ending at shell/.env WDL_NS. Extend both (EN + ZH) with the wdl token store as the lowest layer and its default namespace, matching deploy.md / token.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
wdl token set/list/use/rmadds a local credential store at~/.config/wdl/credentials, so commands resolve a control URL + token without aper-shell
ADMIN_TOKENor a token in every project's.env.setreads the token from stdin, validates it via/whoami, and confirms theprincipal matches the namespace;
rmdeletes the local copy (no revoke).flag > shell env > project .env > store. Trustedand exempt from the cross-origin guard, but a project
.envendpoint is stilldropped when its token isn't from that
.env.WDL_NSsets a default namespace (the analogue of a project.env'sbase): the first stored ns becomes default;
--default/usechange it;listmarks it*. Selection:--ns > shell > .env > store default.318 tests, lint, and typecheck green. No version bump — the release (1.1.0 +
tag) is a separate commit after merge.
🤖 Generated with Claude Code