diff --git a/internal/container/agent_profile_test.go b/internal/container/agent_profile_test.go index 42548ec..ee7da70 100644 --- a/internal/container/agent_profile_test.go +++ b/internal/container/agent_profile_test.go @@ -242,6 +242,31 @@ func TestBuildEnvInjectionScript_QuotesValuesForSourcing(t *testing.T) { } } +func TestBuildEnvInjectionScript_ChownsEnvFileToDirOwner(t *testing.T) { + // Sluice's docker exec runs as the image's USER (root for the + // upstream openclaw and hermes images), so the awk rename and + // heredoc append leave the file root-owned. The agent runtime + // runs as a non-root user, so without a chown back to the dir + // owner the agent cannot run `hermes claw migrate` or in-agent + // secret edits. Verify the script emits a chown step. + for _, hasValues := range []bool{true, false} { + envMap := map[string]string{} + if hasValues { + envMap["KEY"] = "value" + } + script, err := BuildEnvInjectionScript(envMap, false, true) + if err != nil { + t.Fatalf("hasValues=%v: build script: %v", hasValues, err) + } + if !strings.Contains(script, `stat -c '%u:%g'`) { + t.Errorf("hasValues=%v: script must stat the parent dir to derive owner: %s", hasValues, script) + } + if !strings.Contains(script, `chown "$DIR_OWNER" "$ENV_FILE"`) { + t.Errorf("hasValues=%v: script must chown the env file to dir owner: %s", hasValues, script) + } + } +} + func TestBuildEnvInjectionScript_RejectsNewlineInValue(t *testing.T) { // A newline in the value would split the env-file entry across two // lines. The second line would either be silently lost or interpreted diff --git a/internal/container/types.go b/internal/container/types.go index 207d1fe..7c1766c 100644 --- a/internal/container/types.go +++ b/internal/container/types.go @@ -295,37 +295,76 @@ func BuildEnvInjectionScriptForProfile(profile *AgentProfile, envMap map[string] break } } - if !hasContent { - return script.String(), nil - } - // Append the new block via a quoted-tag heredoc. The single quotes - // around the tag (`<<'TAG'`) tell the shell to perform NO expansion - // on the body — every byte we write between the heredoc start and - // the end tag lands in the file verbatim. That lets us emit - // `KEY='value'` lines whose contents are safe under both shell - // `source` and dotenv parsing. - script.WriteString(fmt.Sprintf(" && cat >> \"$ENV_FILE\" <<'%s'\n", envHeredocTag)) - script.WriteString(EnvBlockBegin) - script.WriteString("\n") - for _, k := range keys { - v := envMap[k] - if v == "" { - continue + // Match the env file's ownership to its parent directory. The + // docker exec runs the script as the image's USER (typically + // root for agent containers), so both the awk rename above and + // the heredoc `cat >>` leave the file root-owned. The agent + // runtime runs as a non-root user (UID 10000 for the upstream + // hermes image, configurable via HERMES_UID); a root-owned env + // file blocks agent-side write paths like `hermes claw migrate` + // or in-agent secret edits with EACCES. Inheriting the parent + // directory's owner is portable across agents because each + // agent's entrypoint chowns its data dir to its runtime user + // before sluice ever exec's in. The chown is best-effort: a + // read-only env file still satisfies sluice's own usage, so a + // stat or chown failure does not abort the script. + // + // Stat flag portability: GNU stat (Linux containers like + // alpine, debian) uses `-c`, BSD stat (macOS guests booted via + // the tart backend) uses `-f`. The two `stat` calls are run as + // fallbacks under a single subshell so whichever userland is + // in the agent's container resolves the directory owner. + const chownStep = ` && { DIR_OWNER=$(stat -c '%u:%g' "$(dirname "$ENV_FILE")" 2>/dev/null` + + ` || stat -f '%u:%g' "$(dirname "$ENV_FILE")" 2>/dev/null);` + + ` [ -n "$DIR_OWNER" ] && chown "$DIR_OWNER" "$ENV_FILE" 2>/dev/null;` + + ` true; }` + + if hasContent { + // Append the new block via a quoted-tag heredoc. The single + // quotes around the tag (`<<'TAG'`) tell the shell to perform + // NO expansion on the body. Every byte between the heredoc + // start and the end tag lands in the file verbatim, so we + // can emit KEY='value' lines whose contents are safe under + // both shell source and dotenv parsing. + // + // The chownStep is appended to the SAME line as the heredoc + // `<<` operator (before the heredoc body) because shell will + // not let a `&&` at the start of a new line attach to a + // command whose heredoc body is in between. With this + // placement, the parser sees one logical line: + // cat >> file <> \"$ENV_FILE\" <<'%s'%s\n", envHeredocTag, chownStep)) + script.WriteString(EnvBlockBegin) + script.WriteString("\n") + for _, k := range keys { + v := envMap[k] + if v == "" { + continue + } + // Escape embedded single quotes via the four-character + // idiom (apostrophe, backslash, apostrophe, apostrophe). + // Wrapped in single quotes, the result is one + // well-formed dotenv/shell string with no expansion. + escaped := strings.ReplaceAll(v, "'", `'\''`) + script.WriteString(k) + script.WriteString("='") + script.WriteString(escaped) + script.WriteString("'\n") } - // Escape embedded single quotes: 'value' -> 'val'\''ue'. - // The result, wrapped in single quotes, is one well-formed - // dotenv/shell string with no expansion. - escaped := strings.ReplaceAll(v, "'", `'\''`) - script.WriteString(k) - script.WriteString("='") - script.WriteString(escaped) - script.WriteString("'\n") + script.WriteString(EnvBlockEnd) + script.WriteString("\n") + script.WriteString(envHeredocTag) + script.WriteString("\n") + } else { + // No managed-block emission. The awk pre-pass still + // rewrote the file (removing any prior block), so we + // still want to fix ownership of the result. + script.WriteString(chownStep) } - script.WriteString(EnvBlockEnd) - script.WriteString("\n") - script.WriteString(envHeredocTag) - script.WriteString("\n") return script.String(), nil } diff --git a/internal/proxy/addon.go b/internal/proxy/addon.go index 9e6982d..235b521 100644 --- a/internal/proxy/addon.go +++ b/internal/proxy/addon.go @@ -7,6 +7,7 @@ import ( "log" "net" "net/http" + "runtime/debug" "sort" "strconv" "strings" @@ -127,6 +128,14 @@ type SluiceAddon struct { // time.Sleep-based synchronization. Nil in production. persistDone chan struct{} + // responsePanicHook is a test injection point for the Response + // handler's deferred recover. When non-nil it is invoked between + // the OAuth swap and the DLP scan, so a test can force the + // downstream-of-OAuth panic shape we observed in production + // without having to engineer a malformed Flow that triggers a + // real nil deref. Always nil in production. + responsePanicHook func() + // auditLog, when non-nil, receives per-request deny/inject events. auditLog *audit.FileLogger @@ -547,11 +556,53 @@ func (a *SluiceAddon) injectHeaders(f *mitmproxy.Flow, host string, port int) { } defer secret.Release() - f.Request.Header.Set(binding.Header, binding.FormatValue(secret.String())) + f.Request.Header.Set(binding.Header, binding.FormatValue(extractInjectableSecret(a.oauthIndex.Load(), binding.Credential, secret.String()))) log.Printf("[ADDON-INJECT] injected header %q for %s:%d (credential %q)", binding.Header, host, port, binding.Credential) } +// extractInjectableSecret returns the value to substitute into a binding's +// `{value}` template. +// +// Static credentials are plain strings stored as-is in the vault; the +// value to inject is the string itself. OAuth credentials are +// JSON-marshalled OAuthCredential structs (access_token + refresh_token +// + token_url + expires_at); the value to inject is just the +// access_token, so a binding like `Authorization: Bearer {value}` +// produces `Bearer ` rather than `Bearer {"access_token":...}`. +// +// We dispatch on the credential's metadata type (looked up via the +// supplied OAuthIndex, populated from credential_meta on startup and +// SIGHUP) rather than inferring from the secret's JSON shape. Shape +// inference would mis-handle a static credential whose value happens +// to be OAuth-shaped JSON. The credential_meta table is the single +// source of truth for cred_type elsewhere in sluice; the injection +// path follows the same rule. +// +// If the metadata says oauth but parsing fails (corrupted vault +// entry, schema drift, etc.) we fall back to the raw secret. That +// preserves the previous behavior on broken state instead of +// returning an empty string and silently producing `Bearer ` headers. +// +// A nil index (no oauth credentials registered yet, or the QUIC +// path running before UpdateOAuthIndex fires) means every +// credential is treated as static. +func extractInjectableSecret(idx *OAuthIndex, credName, secret string) string { + if idx == nil || !idx.Has(credName) { + return secret + } + cred, err := vault.ParseOAuth([]byte(secret)) + if err != nil || cred == nil || cred.AccessToken == "" { + // Generic [INJECT] prefix because both the HTTP/1+2 and the + // HTTP/3 (QUIC) header-injection paths share this helper. + // An [ADDON-INJECT] tag would mislead a reader who saw the + // line in a deployment that uses HTTP/3 exclusively. + log.Printf("[INJECT] credential %q registered as oauth but vault payload not parseable; injecting raw secret", credName) + return secret + } + return cred.AccessToken +} + // Request performs Pass 2 (scoped phantom replacement) and Pass 3 (strip // unbound phantoms) on the fully-buffered request body, headers, URL query, // and URL path. Called by go-mitmproxy after the request body has been read @@ -647,12 +698,52 @@ func (a *SluiceAddon) StreamRequestModifier(f *mitmproxy.Flow, in io.Reader) io. // redact patterns (see SetRedactRules) so credential strings in upstream // responses are scrubbed before being relayed to the agent. func (a *SluiceAddon) Response(f *mitmproxy.Flow) { + // Top-level recover so a panic inside any sub-step (OAuth swap, + // DLP scan, future hooks) cannot escape into go-mitmproxy's + // generic recover, which abandons the response body and leaves + // the agent reading an empty stream. We log the full stack so + // the underlying bug can be diagnosed later, but the response + // continues with whatever state f.Response was in at the time + // of the panic. Real tokens cannot leak: processOAuthResponseIfMatching + // has its own snapshot/rollback, and any panic in DLP runs AFTER + // OAuth swap (so tokens are already phantoms by then). + defer func() { + if r := recover(); r != nil { + host := "unknown" + method := "" + if f != nil && f.Request != nil { + if f.Request.URL != nil { + host = f.Request.URL.Host + } + method = f.Request.Method + } + log.Printf("[ADDON] PANIC in Response handler for %s %s: %v\n%s", method, host, r, debug.Stack()) + } + }() + + // Nil-flow guard. The deferred recover above dereferences f to + // build the log line; without this early return, a nil flow + // (which go-mitmproxy never produces in practice but tests can) + // would hit the recover path on every call. Mirror what + // StreamResponseModifier does so both entry points handle nil + // flows uniformly. + if f == nil { + return + } if f.Response == nil || f.Request == nil { return } a.processOAuthResponseIfMatching(f) + // Test-only panic injection. Always nil in production. Lets a + // regression test exercise the deferred recover above without + // having to construct a Flow that triggers a real downstream + // nil deref. + if a.responsePanicHook != nil { + a.responsePanicHook() + } + // Outbound DLP: scan response body and headers for credential // patterns that should not reach the agent. Runs after OAuth // processing so real tokens are already swapped to phantoms. @@ -711,10 +802,56 @@ func (a *SluiceAddon) processOAuthResponseIfMatching(f *mitmproxy.Flow) { // emit a single WARNING per client connection so operators notice the // gap without log spam from multi-chunk streams. The dedup state lives // on dlpStreamWarned, scoped to the client connection id. -func (a *SluiceAddon) StreamResponseModifier(f *mitmproxy.Flow, in io.Reader) io.Reader { - if f.Request == nil { - return in - } +func (a *SluiceAddon) StreamResponseModifier(f *mitmproxy.Flow, in io.Reader) (out io.Reader) { + // Default to passing the input through unchanged; named return + // lets the deferred recover ensure we always hand SOMETHING + // usable back to go-mitmproxy on a panic, instead of letting + // the panic escape into mitmproxy's outer recover (which + // abandons the response body entirely). + out = in + + // Defensive nil-input guard up front, BEFORE the flow checks + // below. If both `f` and `in` are nil (rare but possible in tests + // or on an unusual go-mitmproxy code path), the f-nil early + // return below would otherwise hand back a nil io.Reader, which + // the proxy's downstream copy would nil-deref on. http.NoBody + // keeps the response well-framed (zero bytes) and the panic is + // avoided regardless of what `f` looks like. + if in == nil { + out = http.NoBody + return out + } + + if f == nil || f.Request == nil { + return out + } + + // Known-safe fallback for the panic recover. Set ONLY after the + // OAuth phantom swap has produced a clean buffer that contains + // no real tokens. Critically: we never assign the raw upstream + // bytes here, because a matched OAuth token-endpoint response + // contains real access and refresh tokens, and a panic between + // io.ReadAll and a successful swapOAuthTokens would otherwise + // leak those tokens straight to the agent. If the panic fires + // before safeFallback is set, the recover hands back http.NoBody + // instead. The agent sees an empty 2xx token body and surfaces + // the failure as a parse error, which is the strictly safer + // outcome compared to leaking a real bearer. + var safeFallback []byte + defer func() { + if r := recover(); r != nil { + host := "unknown" + if f.Request != nil && f.Request.URL != nil { + host = f.Request.URL.Host + } + log.Printf("[ADDON] PANIC in StreamResponseModifier for %s: %v\n%s", host, r, debug.Stack()) + if safeFallback != nil { + out = bytes.NewReader(safeFallback) + } else { + out = http.NoBody + } + } + }() // Warn when DLP rules are configured but the response is streamed. // Dedupe by client connection id so we emit at most one warning per @@ -782,10 +919,20 @@ func (a *SluiceAddon) StreamResponseModifier(f *mitmproxy.Flow, in io.Reader) io modified, err := a.swapOAuthTokens(body, contentType, credName) if err != nil { + // The body did not parse as an OAuth token response. This is + // usually an HTML error page from a misconfigured token + // endpoint, not a credentials envelope, so passing it through + // is the historical behavior. We deliberately do NOT set + // safeFallback here: if a later panic somehow fires while + // returning, the recover defaults to http.NoBody rather than + // leaking whatever this body contains. log.Printf("[ADDON-OAUTH] stream token parse error for credential %q: %v", credName, err) return bytes.NewReader(body) } + // Swap completed. The modified buffer is phantom-only and safe to + // hand back on a late panic. + safeFallback = modified return bytes.NewReader(modified) } diff --git a/internal/proxy/addon_test.go b/internal/proxy/addon_test.go index 9faec39..d7c7394 100644 --- a/internal/proxy/addon_test.go +++ b/internal/proxy/addon_test.go @@ -1191,6 +1191,142 @@ func TestAddonResponse_OAuthMalformedBodyDoesNotPanic(t *testing.T) { addon.Response(f) } +// TestAddonResponse_RecoversFromDownstreamPanic verifies that the +// top-level recover in Response catches a panic that fires AFTER the +// OAuth swap completes (the production shape that bit us during the +// OpenAI Codex device-code flow on the deployed proxy). The agent +// must still receive the phantom-swapped body via f.Response.Body +// instead of an empty stream caused by go-mitmproxy's outer recover. +func TestAddonResponse_RecoversFromDownstreamPanic(t *testing.T) { + oauthCred := &vault.OAuthCredential{ + AccessToken: "old-access", + RefreshToken: "old-refresh", + TokenURL: testOAuthTokenURL, + } + addon, _ := setupOAuthAddon(t, "panic_recover_oauth", oauthCred) + client := setupAddonConn(addon, "auth.example.com:443") + + // Force a panic between the OAuth swap and the DLP scan, to + // reproduce the shape we observed in production where something + // downstream of the swap nil-derefs. + addon.responsePanicHook = func() { + var p *int + _ = *p // intentional nil deref + } + + respBody := mustJSON(t, map[string]interface{}{ + "access_token": "real-access-recover", + "refresh_token": "real-refresh-recover", + "expires_in": 3600, + "token_type": "Bearer", + }) + + f := newTestResponseFlow(client, testOAuthTokenURL, 200, respBody, "application/json") + + defer func() { + if r := recover(); r != nil { + t.Fatalf("Response handler must not propagate panic; got: %v", r) + } + }() + addon.Response(f) + + // Body must hold phantoms (the swap completed before the panic + // fired). Real tokens must not be present. + body := string(f.Response.Body) + if strings.Contains(body, "real-access-recover") || strings.Contains(body, "real-refresh-recover") { + t.Errorf("real tokens leaked through panic recovery path: %q", body) + } + if !strings.Contains(body, oauthPhantomAccess("panic_recover_oauth")) { + t.Errorf("expected phantom access token to remain in body after recover, got: %q", body) + } + + waitAddonPersist(t, addon) +} + +func TestAddonStreamResponseModifier_HandlesNilInput(t *testing.T) { + // go-mitmproxy normally passes a non-nil reader, but a nil here + // must not nil-deref. The handler should fall through cleanly. + oauthCred := &vault.OAuthCredential{ + AccessToken: "old", + TokenURL: testOAuthTokenURL, + } + addon, _ := setupOAuthAddon(t, "nil_input_oauth", oauthCred) + client := setupAddonConn(addon, "auth.example.com:443") + + f := newTestResponseFlow(client, testOAuthTokenURL, 200, nil, "application/json") + + defer func() { + if r := recover(); r != nil { + t.Fatalf("StreamResponseModifier must not panic on nil input; got: %v", r) + } + }() + out := addon.StreamResponseModifier(f, nil) + // http.NoBody is what we hand back for nil input — keeps the + // streamed response well-framed (zero bytes) instead of an + // undefined body that some HTTP clients trip on. + if out != http.NoBody { + t.Errorf("expected http.NoBody for nil input, got: %T", out) + } +} + +func TestExtractInjectableSecret(t *testing.T) { + // Build a metadata-driven OAuth index that lists "oauth_cred" + // as oauth and treats any other name as static. Mirrors what + // Server.UpdateOAuthIndex does on startup / SIGHUP. + idx := NewOAuthIndex([]store.CredentialMeta{ + {Name: "oauth_cred", CredType: "oauth", TokenURL: "https://auth.example.com/oauth/token"}, + }) + + // Static credentials are stored as plain strings. With the cred + // name absent from the OAuth index, the value passes through. + if got := extractInjectableSecret(idx, "github_pat", "ghp_static-pat-abc123"); got != "ghp_static-pat-abc123" { + t.Errorf("static cred should pass through unchanged, got %q", got) + } + + // A static credential whose value happens to be OAuth-shaped + // JSON must NOT be misclassified. Without metadata-driven + // dispatch, shape inference would strip the access_token field + // and silently change behavior for legitimate static creds. + staticOAuthShape := `{"access_token":"intentional","token_url":"https://x"}` + if got := extractInjectableSecret(idx, "github_pat", staticOAuthShape); got != staticOAuthShape { + t.Errorf("static cred with oauth-shaped value must pass through, got %q", got) + } + + // OAuth credentials registered in the index parse as JSON and + // yield just the access_token. The binding template wants + // `Bearer `, not `Bearer {"access_token":"",...}`. + oauthBlob := `{"access_token":"jwt-access-xyz","refresh_token":"jwt-refresh-abc","token_url":"https://auth.example.com/oauth/token"}` + if got := extractInjectableSecret(idx, "oauth_cred", oauthBlob); got != "jwt-access-xyz" { + t.Errorf("oauth cred should yield access_token, got %q", got) + } + + // Leading whitespace in a stored JSON envelope must not bypass + // extraction. json.Unmarshal accepts leading whitespace; the + // extractor delegates to ParseOAuth which uses the same parser. + leadingWS := " \n\t" + oauthBlob + if got := extractInjectableSecret(idx, "oauth_cred", leadingWS); got != "jwt-access-xyz" { + t.Errorf("oauth cred with leading whitespace should yield access_token, got %q", got) + } + + // OAuth credential with corrupted vault payload: parsing fails, + // fall back to the raw secret. Better to forward a malformed + // header than silently substitute an empty string and produce + // `Bearer ` (which would also 401 but with no diagnostic clue). + for _, broken := range []string{"", `{"foo":"bar"}`, `{not-json`} { + got := extractInjectableSecret(idx, "oauth_cred", broken) + if got != broken { + t.Errorf("broken oauth payload %q should fall back, got %q", broken, got) + } + } + + // Nil index means no oauth credentials are registered yet. + // Every credential is treated as static. Same as the early- + // startup path before the first UpdateOAuthIndex fires. + if got := extractInjectableSecret(nil, "anything", oauthBlob); got != oauthBlob { + t.Errorf("nil index should pass through unchanged, got %q", got) + } +} + func TestAddonResponse_Non2xxPassesThrough(t *testing.T) { oauthCred := &vault.OAuthCredential{ AccessToken: "old-access", diff --git a/internal/proxy/oauth_index.go b/internal/proxy/oauth_index.go index 607d3ac..d63c9c6 100644 --- a/internal/proxy/oauth_index.go +++ b/internal/proxy/oauth_index.go @@ -101,3 +101,24 @@ func (idx *OAuthIndex) Len() int { } return len(idx.entries) } + +// Has returns true if the named credential is registered as OAuth in +// this index (i.e. its credential_meta entry had cred_type="oauth" and +// a usable token_url). The injection path uses this to decide whether +// the secret returned by the vault is a JSON-marshalled OAuthCredential +// envelope that needs access_token extraction, vs a static credential +// whose value should be passed through to the binding template +// verbatim. We treat credential metadata as authoritative rather than +// inferring from the secret's shape, so a static credential whose +// value happens to be JSON cannot be misclassified. +func (idx *OAuthIndex) Has(credName string) bool { + if idx == nil { + return false + } + for _, e := range idx.entries { + if e.credential == credName { + return true + } + } + return false +} diff --git a/internal/proxy/quic.go b/internal/proxy/quic.go index 8b0884a..4c0bc50 100644 --- a/internal/proxy/quic.go +++ b/internal/proxy/quic.go @@ -75,6 +75,14 @@ type QUICProxy struct { audit *audit.FileLogger rules atomic.Pointer[quicInspectRules] + // oauthIndex points at the same OAuthIndex the SluiceAddon uses + // so QUIC/HTTP3 header injection follows the same OAuth-vs-static + // dispatch rules as the HTTP/1+2 path. Optional: a nil index means + // every credential is treated as static (the right answer when no + // oauth credentials are registered). Updated atomically via + // SetOAuthIndex from Server.UpdateOAuthIndex. + oauthIndex atomic.Pointer[OAuthIndex] + // upstreamTLSConfig overrides the TLS configuration for outbound HTTP/3 // connections to real upstreams. Nil uses system roots. Tests set this // to trust the test CA. @@ -109,6 +117,14 @@ type QUICProxy struct { closed bool } +// SetOAuthIndex atomically replaces the QUIC proxy's OAuth index. The +// addon and the QUIC proxy each hold their own pointer so concurrent +// header-injection paths can read without locking; both are kept in +// sync from Server.UpdateOAuthIndex. +func (q *QUICProxy) SetOAuthIndex(idx *OAuthIndex) { + q.oauthIndex.Store(idx) +} + // NewQUICProxy creates a QUIC proxy that terminates agent QUIC connections. // The caCert is used to sign per-host TLS certificates derived from the SNI. // Block rules cause requests with matching body content to be rejected. @@ -412,7 +428,7 @@ func (q *QUICProxy) buildHandler(upstreamHost string, destPort int, checker *Req log.Printf("[QUIC-MITM] credential %q lookup failed: %v", binding.Credential, err) } else { if binding.Header != "" { - r.Header.Set(binding.Header, binding.FormatValue(secret.String())) + r.Header.Set(binding.Header, binding.FormatValue(extractInjectableSecret(q.oauthIndex.Load(), binding.Credential, secret.String()))) } secret.Release() } diff --git a/internal/proxy/server.go b/internal/proxy/server.go index 17bc1f4..2ded07a 100644 --- a/internal/proxy/server.go +++ b/internal/proxy/server.go @@ -85,6 +85,19 @@ type Server struct { closed atomic.Bool serving atomic.Bool activeConns sync.WaitGroup + + // oauthMetasCache holds the latest credential_meta slice the + // server saw via UpdateOAuthIndex. Cached so a later + // quicProxy initialization (or re-init) can re-apply it. + // Without this, an UpdateOAuthIndex call that arrived before + // the QUIC proxy existed would leave QUICProxy.oauthIndex nil, + // re-introducing the OAuth-envelope header injection bug for + // HTTP/3 traffic until the next SIGHUP. Guarded by + // oauthMetasMu because UpdateOAuthIndex can fire concurrently + // from the SIGHUP reload path and Telegram credential + // mutations. + oauthMetasMu sync.Mutex + oauthMetasCache []store.CredentialMeta } type contextKey string @@ -606,13 +619,17 @@ func (s *Server) setupInjection(cfg Config, _ net.Listener) error { } s.addon = NewSluiceAddon(addonOpts...) - // Populate the OAuth token URL index from credential metadata so - // the response handler can detect OAuth token endpoints from startup. + // Load credential metadata once and stash it; we cannot mirror it + // into the QUIC proxy yet because that proxy is initialized later + // in this function. The deferred apply at the bottom of the setup + // hits both the addon and the QUIC proxy in one Server-level call. + var oauthMetas []store.CredentialMeta if cfg.Store != nil { - if metas, metaErr := cfg.Store.ListCredentialMeta(); metaErr == nil { - s.addon.UpdateOAuthIndex(metas) - } else { + metas, metaErr := cfg.Store.ListCredentialMeta() + if metaErr != nil { log.Printf("[MITM-OAUTH] failed to load credential meta for index: %v", metaErr) + } else { + oauthMetas = metas } } @@ -705,6 +722,12 @@ func (s *Server) setupInjection(cfg Config, _ net.Listener) error { log.Printf("QUIC proxy disabled: %v", qpErr) } else { s.quicProxy = qp + // Re-apply any UpdateOAuthIndex that may have arrived before + // quicProxy was assigned. The current setupInjection path + // calls UpdateOAuthIndex below this block so the cache is + // usually empty here, but being defensive means a future + // reorder cannot silently drop the QUIC OAuth dispatch. + s.applyCachedOAuthIndexToQUIC() go func() { if listenErr := qp.ListenAndServe("127.0.0.1:0"); listenErr != nil { log.Printf("[QUIC] listener stopped: %v", listenErr) @@ -712,6 +735,13 @@ func (s *Server) setupInjection(cfg Config, _ net.Listener) error { }() } + // Apply the OAuth metadata to both addon and QUIC proxy now that + // both are initialized. UpdateOAuthIndex is idempotent and handles + // nil quicProxy gracefully (when QUIC failed to start). + if oauthMetas != nil { + s.UpdateOAuthIndex(oauthMetas) + } + log.Printf("credential injection enabled (%s)", cfg.Provider.Name()) return nil } @@ -2379,9 +2409,44 @@ func (s *Server) StoreResolver(r *vault.BindingResolver) { // after Telegram credential mutations so the response handler detects // new or removed OAuth token endpoints. func (s *Server) UpdateOAuthIndex(metas []store.CredentialMeta) { + // Cache the metas so a later quicProxy initialization can pick + // up the latest index. setupInjection runs UpdateOAuthIndex + // before quicProxy is created in some startup paths (and tests + // can construct a Server without a QUIC proxy entirely), so + // without this cache the QUIC OAuth-vs-static dispatch would + // silently fall back to nil and the OAuth-envelope header + // injection bug would re-emerge for HTTP/3 traffic until the + // next SIGHUP. + s.oauthMetasMu.Lock() + s.oauthMetasCache = metas + s.oauthMetasMu.Unlock() if s.addon != nil { s.addon.UpdateOAuthIndex(metas) } + // Mirror the index into the QUIC proxy so HTTP/3 header injection + // follows the same OAuth-vs-static dispatch as the HTTP/1+2 path. + // Without this, a credential with metadata cred_type="oauth" would + // be injected as the full JSON envelope on QUIC requests. + if s.quicProxy != nil { + s.quicProxy.SetOAuthIndex(NewOAuthIndex(metas)) + } +} + +// applyCachedOAuthIndexToQUIC pushes the most recently cached OAuth +// metadata into the QUIC proxy. Call this after assigning +// s.quicProxy so a UpdateOAuthIndex that arrived earlier (before +// quicProxy existed) is not lost. +func (s *Server) applyCachedOAuthIndexToQUIC() { + if s.quicProxy == nil { + return + } + s.oauthMetasMu.Lock() + metas := s.oauthMetasCache + s.oauthMetasMu.Unlock() + if len(metas) == 0 { + return + } + s.quicProxy.SetOAuthIndex(NewOAuthIndex(metas)) } // SetOnOAuthRefresh configures a callback on the addon that is invoked