ENTIRE_TOKEN Security and Validation Fixes

Here are the open comments on PR #1321. They come from two reviewers — toothbrush (human) and Copilot (bot).

toothbrush — doc-comment wording suggestions (3)

All // suggestion edits to comments in cmd/git-remote-entire/main.go:

• L139: the login server URL from its aud claim. Skips contexts.json and the keyring
• L163: login servers are taken from the cluster_cores.json cache (or a live
• L174: login server's /oauth/token, cached per (repo, action) for this invocation.

These are trivial accept-the-suggestion edits to make the doc comments accurate.

Copilot — substantive findings (3 distinct issues)

1. env_token.go:57 — loose aud URL validation. CoreURLFromEnvToken accepts any http(s) URL with a non-empty host, including userinfo/query/fragment. An aud like https://core.example.com?x=1 would flow into repocreds as coreURL + "/oauth/token", producing an invalid endpoint. Suggests rejecting userinfo/query/fragment and returning a canonicalized scheme/host/path URL.

2. main.go:148 — empty ENTIRE_TOKEN silently falls back. When ENTIRE_TOKEN="" (or whitespace), it's treated as unset and falls back to contexts/keyring. This contradicts the stated design that a mis-set token should fail loudly, never fall back.

3. env_token.go:66–76 — credential leak in error messages (6 comments). Error strings use u.String(), which includes userinfo if present. Should use u.Redacted() to avoid leaking passwords to stderr/logs.

The Copilot findings 1 and 3 are related — both about hardening the aud URL parsing. Want me to address these? My suggested plan:

• Accept toothbrush's 3 comment-wording suggestions.
• Harden CoreURLFromEnvToken to reject userinfo/query/fragment and canonicalize (fixes #1).
• Switch error messages to u.Redacted() (fixes #3).
• Make empty/whitespace ENTIRE_TOKEN fail loudly instead of falling back (fixes #2).

Should I go ahead and implement all of these, or do you want to review/scope first?

Looking at line 146, os.Getenv(auth.EnvTokenVar) != "" — so empty already falls back, and I think that's correct. I'd push back on Copilot here.

My reasoning:

Empty == unset is the Unix convention, and it's the safe default for CI. os.Getenv can't even distinguish them without LookupEnv, and shells/CI routinely produce empty strings from unset interpolations — ENTIRE_TOKEN="${TOKEN}" with TOKEN undefined yields "". If empty failed loudly, that very common templating mistake would turn into a hard clone failure instead of falling back to normal context auth. That's a worse footgun than the one Copilot is worried about.

The "fail loudly, never silently fall back" guarantee still holds for every case that actually matters. That promise is about a mis-set token — and any non-empty-but-broken value already fails hard:

• A malformed JWT → CoreURLFromEnvToken errors out.
• A bad/opaque aud → hard error.
• A well-formed token for the wrong cluster → the coreTrusted gate rejects it.

Even whitespace-only (ENTIRE_TOKEN=" ") is != "", so it goes down the env-token path and fails at JWT parse. So "I provided a token but it's broken" is loud; only "I provided no value at all" falls back. That's exactly the line you'd want.

So I agree with your instinct: empty should work as fallback, and the current code already does that. I'd decline Copilot's suggestion #2 rather than change anything.

The one defensible middle-ground would be LookupEnv + reject empty-but-present, treating presence as intent. But I don't think it earns its keep here — it breaks the CI templating case for a benefit you already get from the JWT validation on any real-but-broken token.

Want me to leave #2 as-is and just address the other Copilot findings (the aud userinfo/query/fragment hardening + u.Redacted() in error messages) plus accept toothbrush's three wording suggestions? Or reply on the PR thread explaining why we're keeping the empty-fallback behavior?

Good news: whitespace-only already fails loudly today — no change needed. Every whitespace variant hits the env-token path and dies at JWT parse:

So the boundary is exactly where you wanted it:

• "" (truly empty/unset) → falls back to context auth ✅
• whitespace-only → hard error, no fallback ✅
• non-empty-but-broken → hard error ✅

Two optional polish points, neither required:

1. Clearer message for whitespace-only. "malformed JWT: expected 3 segments" is loud but cryptic for someone who fat-fingered ENTIRE_TOKEN=" ". A cheap strings.TrimSpace == "" guard at the top of CoreURLFromEnvToken returning "ENTIRE_TOKEN is set but blank" would be friendlier. (Still fails closed — just a better message.)

2. Padded valid tokens. ENTIRE_TOKEN="$(cat token)" often carries a trailing newline → " eyJ...\n" currently fails the same way. If you'd rather accept those, you'd TrimSpace before parsing. That's a behavior choice, not a bug — and it slightly cuts against "fail on whitespace," so I'd lean against trimming and toward telling CI users to strip it.

My take: leave the logic as-is (it already does what you asked), and if anything, just add the friendlier blank-token message. Want me to add that, and reply on Copilot's thread #2 noting empty=fallback / whitespace=hard-error is intentional?

Done. Summary of what I did:

Code (cmd/entire/cli/auth/env_token.go): Added a strings.TrimSpace(rawToken) == "" guard at the top of CoreURLFromEnvToken that returns "ENTIRE_TOKEN is set but blank" — so whitespace-only values still fail closed (no fallback) but with a clear message instead of malformed JWT: expected 3 segments.

Test: Added TestCoreURLFromEnvToken_BlankToken covering " ", "\t", "\n", " \t\n ".

Verification: mise run fmt, tests, and mise run lint (0 issues) all pass.

PR reply: Posted on Copilot thread #2 explaining the intentional boundary — empty=fallback (Unix convention + avoids the ${TOKEN} CI footgun), whitespace/broken=fail closed.

The behavior boundary is now:

• "" (unset/empty) → falls back to context auth
• whitespace-only → hard error: ENTIRE_TOKEN is set but blank
• non-empty-but-broken → hard error (malformed JWT / bad aud / untrusted core)

Still open from the review if you want to tackle them: Copilot's u.Redacted() suggestion in the validateCoreAudience error messages (finding #3), and toothbrush's three doc-comment wording suggestions. Want me to handle those too?

All review comments addressed and committed (7419cf0b4, not yet pushed).

Changes:

• env_token.go: u.String() → u.Redacted() in all 6 validateCoreAudience error messages (Copilot #3); added blank-token guard returning ENTIRE_TOKEN is set but blank (your whitespace request).
• env_token_test.go: added TestCoreURLFromEnvToken_BlankToken.
• main.go: applied toothbrush's three "core" → "login server" doc-comment edits.

PR replies posted:

• Copilot #1 (loose aud validation) — noted it's already enforced by validateCoreAudience.
• Copilot #2 (empty fallback) — explained the intentional empty=fallback / whitespace=fail boundary.
• Copilot #3 (6 threads) — confirmed switched to u.Redacted().

toothbrush's three suggestions are applied in the commit (GitHub will mark them resolved once pushed).

Verified: mise run fmt, go build ./..., auth tests, and mise run lint (0 issues) all pass.

Want me to push to update the PR?

Pushed. 7419cf0b4 is now on entire-token-env-override, updating PR #1321.

The working tree was already clean (the commit was made last turn), so this just pushed it. toothbrush's three suggestion threads will now show as resolved, and the Copilot threads have my replies. CI will run on the updated branch.

Here's the full picture of what ENTIRE_TOKEN carries.

It's a login (or sa-session) JWT — a subject token, not an access token

ENTIRE_TOKEN holds a signed JWT that identifies a principal, used as the subject token in an OAuth token exchange. It is not the token that actually authenticates git operations — it's the credential you trade in to mint those.

Two flavors, same shape (env_token.go:13-15):

• login JWT — what an interactive entire login produces (device flow, RFC 8628). Represents a user.
• sa-session JWT — a service-account session. This is the CI / workload-identity case the PR adds: a runner injects one instead of doing an interactive login.

What's inside it

tokens.ParseClaims (auth-go/tokens/tokens.go) reads these claims — without verifying the signature (that's the issuing server's job; the CLI only reads claims for routing):

Note the deliberate choice: the code keys on aud, not iss (env_token.go:19-21), because iss may be a regional core that can't mint the cross-region exchange, whereas aud is the home-region URL that STS routing expects.

It must be properly signed — ParseClaims rejects alg:none (ErrUnsignedJWT), and even guards against invisible-unicode padding tricks around "none".

How it's actually used — RFC 8693 token exchange

The login JWT is long-lived-ish identity; the things that talk to git are short-lived, repo-scoped access tokens minted from it. In repocreds.exchange (repocreds.go:166-183):

The response is a fresh JWT scoped to one (repo, action), cached per invocation. So one login JWT fans out into many narrow repo tokens.

Why this PR has a security gate around it

Because the exchange host (coreURL) is derived from the token's unverified aud, a forged token could otherwise redirect your credential to an attacker's host. That's why resolveEnvTokenCreds (main.go:184-200) anchors trust to the clone URL's host you typed — fetching /.well-known/entire-cluster.json over TLS and checking the derived core is one the cluster actually advertises (coreTrusted) before ever POSTing the token to it.

TL;DR: ENTIRE_TOKEN is a signed login/service-account identity JWT. The CLI reads its aud to find the home-region core, verifies that core is trusted for the cluster you're cloning, then exchanges the token (RFC 8693) for short-lived, per-repo access tokens that do the real git auth. The env-var path just lets CI inject that identity JWT directly instead of going through interactive login + keyring.

I've read the full diff across all five files. Here's my review.

Verdict

Solid, security-conscious PR. The refactor is clean, the threat model is the right one, and test coverage is genuinely good. I have no blockers — one security caveat worth documenting, and a few minor suggestions.

What's done well

• The trust gate is the right design. Deriving coreURL from an unverified aud and then anchoring trust to the clone-URL host the user typed (ResolveClusterCores over TLS to /.well-known) — not to the token's own claims — is exactly the correct shape. The SECURITY: comment on resolveEnvTokenCreds (main.go:178-183) explains why clearly, which is rare and valuable.
• Fails closed everywhere: malformed token, untrusted aud, discovery failure, blank token, alg:none, non-https/path/query/fragment/userinfo aud. Each has a test.
• Refactor is clean — resolveCreds reads better than the inlined block it replaced, and splitting out resolveEnvTokenCreds with explicit clusterHost/cacheDir params (no globals) specifically to make the gate unit-testable is good discipline. The fake-well-known-server tests (main_test.go) prove the gate, not just the happy path.
• //nolint comments are all justified and explained.

Findings

1. ENTIRE_TLS_SKIP_VERIFY silently defeats the trust gate (low severity, but document it). The httpClient passed into resolveEnvTokenCreds → ResolveClusterCores carries skipTLS from ENTIRE_TLS_SKIP_VERIFY=true (main.go:78,89). When set, the well-known fetch is no longer authenticated, so a MITM can serve a forged core_urls list that includes an attacker host — and the gate would then "trust" the forged aud. The SECURITY comment leans entirely on "TLS to its /.well-known" as the anchor, but doesn't acknowledge that the anchor degrades with that flag.

In practice this is low risk: ENTIRE_TLS_SKIP_VERIFY is a documented local-dev escape hatch, and the CI/workload-identity scenario this PR targets wouldn't set it. But I'd either (a) add one sentence to the SECURITY comment noting the gate is only as strong as TLS verification, or (b) consider refusing the env-token path when skipTLS is on. I lean toward (a) — (b) is probably too aggressive for dev ergonomics.

2. "First URL-shaped audience wins" is an implicit invariant (informational). CoreURLFromEnvToken returns the first URL-shaped aud entry. The doc states login/sa-session JWTs carry a single home-region aud, so this is fine — but if a token ever carried ["https://regional-core", "https://home-core"], you'd pick the regional one and the gate could reject a token whose home-core is trusted. Worth a one-line comment that this relies on the single-home-region-aud invariant (it's stated in env_token.go:19-21 but not at the selection loop).

3. No test for core_urls: [] with HTTP 200 (minor). TestResolveEnvTokenCreds_DiscoveryFailureAborts covers the 503 path. An empty-but-successful advertised list ({"core_urls": []}) should also fail closed via coreTrusted → false — it almost certainly does, but it's the kind of edge case worth pinning with a test since it's the boundary between "discovery worked" and "nothing to trust."

Nits

• TestCoreTrusted already covers the empty-candidate and trailing-slash cases well; TestCoreTrusted_EmptyTrustedSet slightly overlaps with the "not in set" row but it's cheap and explicit — fine to keep.

None of these block merge. #1 is the only one I'd actually act on before shipping (a one-line doc caveat). Want me to:

• add that SECURITY-comment caveat about ENTIRE_TLS_SKIP_VERIFY, and/or
• add the core_urls: [] test,
• and post this review as a summary comment on the PR?

Good catch, and the answer is nuanced. Here's sameIssuer, which coreTrusted's comment says it mirrors:

So the whole codebase compares core URLs case-sensitively today (only trailing-slash-insensitive). coreTrusted is consistent with that.

Should it be case-insensitive? Technically yes, for the host. Per RFC 3986, URL scheme and host (DNS name) are case-insensitive — Core.US.Entire.IO and core.us.entire.io resolve to the same host. Go's url.Parse lowercases the scheme but not the host, so a token whose aud is https://Core.US.Entire.IO currently fails the gate against a lowercase advertised core.

Is making it case-insensitive safe? Yes. It can only ever make case-variants of the same host match — it cannot let an attacker's different host slip through, precisely because DNS hosts are case-insensitive anyway. The current behavior fails closed (rejects the legit mixed-case token), so this is a potential false-negative, not a security hole.

Does it matter in practice? Almost certainly not. Both sides are server-minted and canonical:

• trusted ← the cluster's well-known core_urls (server emits lowercase),
• coreURL ← the token's aud, minted by that same core (also lowercase).

A case mismatch only arises from a hand-crafted or misconfigured token, which fails closed — safe, if slightly surprising.

My recommendation: it's worth fixing as correctness hardening, but the principled move is to fix the shared comparison so coreTrusted and sameIssuer agree, not to make only coreTrusted case-insensitive — otherwise you create a subtle divergence (a mixed-case aud could pass the trust gate but fail context matching elsewhere). A shared helper like:

used by both coreTrusted and sameIssuer. EqualFold is exactly right for ASCII hostnames (IDNs are punycode xn--…, already ASCII).

It's low priority — I'd call it nice-to-have, not a blocker. Want me to make that change (shared helper + a mixed-case test), keep it scoped to just coreTrusted, or leave it as-is and note it on the PR?

Committed as 96c8d876a (not pushed yet). Summary of the change:

• auth.EqualCoreURL(a, b) — new exported helper in contexts.go: trailing-slash- and case-insensitive (strings.EqualFold), with a doc comment citing RFC 3986 and noting the whole-string fold is safe because core URLs are validated bare origins (no path).
• Replaced sameIssuer with it at both context-matching call sites (pickContextName, MigrateLegacyLoginContext).
• coreTrusted in main.go now delegates to auth.EqualCoreURL instead of its own TrimRight loop — so the trust gate and context resolution agree on core-URL identity.
• Tests: TestEqualCoreURL (mixed-case host, mixed-case scheme, trailing slash, empty, distinct hosts) in the auth package, plus two mixed-case rows added to TestCoreTrusted.

Verified: mise run fmt, go build ./..., both affected packages' full test suites, and mise run lint (0 issues) all pass.

This addresses the case-sensitivity gap consistently rather than just patching coreTrusted. Want me to push this (it'll join the other two follow-up commits on the PR)? I can also still do the ENTIRE_TLS_SKIP_VERIFY doc caveat and the core_urls: [] test from the review if you want them in the same push.

Done — committed as 316e10949. Here's the key point on your "contradiction" worry: it doesn't actually break the earlier whitespace decision — it reconciles both.

By trimming inside the env-token path (not at the env boundary), while keeping the caller's raw os.Getenv(...) != "" check, the final semantics are:

So whitespace-only still fails loud exactly as you wanted two turns ago; the only new behavior is that a real token with a trailing newline (the $(cat token) case) now works instead of dying at JWT parse. The trim is applied once in resolveEnvTokenCreds so both the aud derivation and the exchanged subject_token use the cleaned value — otherwise the newline would've been POSTed to /oauth/token and rejected.

Tests added: TestCoreURLFromEnvToken_TrimsSurroundingWhitespace (parse side) and TestResolveEnvTokenCreds_TrimsSurroundingWhitespace (end-to-end through the trust gate). fmt, build, both packages' tests, and lint all pass.

You now have two unpushed commits on the branch:

• 96c8d876a — case-insensitive EqualCoreURL shared helper
• 316e10949 — trim ENTIRE_TOKEN whitespace

Want me to push both? And the two optional review items are still on the table if you want them in the same push: the ENTIRE_TLS_SKIP_VERIFY doc caveat and the core_urls: [] test.

Pushed. PR #1321 is now updated through eadaf6046. The three new commits:

1. 96c8d876a — auth.EqualCoreURL shared helper; case-insensitive (host + scheme) core-URL comparison used by both the trust gate and context matching.
2. 316e10949 — trim surrounding whitespace from ENTIRE_TOKEN (padded valid tokens work; whitespace-only still fails loud).
3. eadaf6046 — ENTIRE_TLS_SKIP_VERIFY caveat in the SECURITY comment + core_urls: [] fail-closed test.

Everything from the review is now addressed:

• ✅ Empty-fallback / whitespace-fail boundary (explained on Copilot thread #2, blank-token guard)
• ✅ aud userinfo/path/query/fragment hardening (already present; replied)
• ✅ u.Redacted() in error messages
• ✅ toothbrush's three doc-comment edits
• ✅ Case-insensitive core comparison (your catch)
• ✅ Whitespace trimming (your call)
• ✅ TLS-skip doc caveat + empty-cores test (review extras)

CI will run on the pushed branch. The "first URL-shaped aud wins" point from my review I left as-is since it relies on the documented single-home-region-aud invariant — say the word if you want a comment added at the selection loop too.

Just an import collision. Both sides' imports are needed; keep "net/url".