Skip to content

injectCredentials: placeholder re-read lets the wrong credential win the wire and over-reports grants #40

Description

@andybons

Split out of #39, where it surfaced. Both bugs predate that branch; #39 fixed only the invalidation consequence and deliberately left wire behavior alone.

The bug

injectCredentials (proxy/proxy.go) first pass:

for _, c := range creds {
    if req.Header.Get(c.Name) != "" {   // <- re-reads after a prior Set
        req.Header.Set(c.Name, c.Value)
        injected[strings.ToLower(c.Name)] = true
        if c.Grant != "" {
            grants = append(grants, c.Grant)
        }

req.Header.Get(c.Name) is evaluated after earlier iterations may have written that same header. When two credentials for a host share a header name and the client sends any placeholder value for it, every same-named credential passes the check.

Reproduced with [{claude, Authorization}, {github-user, Authorization}] and a client placeholder:

on the wire: "Bearer github"
Grants:      [claude github-user]

Consequences

  1. The wrong credential reaches the destination. The last credential in slice order wins regardless of which grant the client's placeholder was meant to select — contradicting the function's own doc comment ("This lets the client choose which credential to use when multiple grants target the same host"). Slice order comes from config order, so this is silent and position-dependent.
  2. Grants over-reports in the canonical log line. Both grants are recorded as used when only one was sent. Audit logs claim a credential was injected that never reached the wire.

Why it wasn't fixed in #39

Fixing the wire selection needs a semantic decision the code can't currently express: when several credentials share a header name, a client placeholder carries no information about which one it meant. The second pass has a preference rule (claude loses to any other grant), but its motivating comment says the opposite for the placeholder case — "the claude grant ... should only be injected when explicitly requested via placeholder." So on a placeholder, claude arguably should win, which is the inverse of the auto-inject rule.

Suggested direction

  • Snapshot the client's headers before injection so the loop cannot feed itself.
  • Decide a single-winner rule per header name for the placeholder path, and state it in the doc comment.
  • Consider matching placeholders by value (there is already a tokenSubstitution mechanism keyed on placeholder strings) so a client can unambiguously select a grant when several share a header name.
  • Fix Grants to record only the credential whose value survived.

credentialInjectionResult.Injected (added in #39) already records exactly the surviving credential per header name and can be reused.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions