Skip to content

Commit c2be1d0

Browse files
bbrowningbbrowning
authored
Merge pull request #13 from bbrowning/502-on-credential-inject-failure
Fix silent credential injection failures by returning 502 on error
2 parents 3d5124d + c64af94 commit c2be1d0

7 files changed

Lines changed: 153 additions & 35 deletions

File tree

‎internal/credentials/config_test.go‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ func TestBuildFromConfig_Bearer(t *testing.T) {
188188
URL: &url.URL{Host: "api.openai.com"},
189189
Header: make(http.Header),
190190
}
191-
if !store.InjectCredentials(req) {
191+
if matched, injected := store.InjectCredentials(req); !matched || !injected {
192192
t.Error("should match api.openai.com")
193193
}
194194
if got := req.Header.Get("Authorization"); got != "Bearer sk-test-123" {
@@ -216,7 +216,7 @@ func TestBuildFromConfig_APIKey(t *testing.T) {
216216
URL: &url.URL{Host: "api.anthropic.com"},
217217
Header: make(http.Header),
218218
}
219-
if !store.InjectCredentials(req) {
219+
if matched, injected := store.InjectCredentials(req); !matched || !injected {
220220
t.Error("should match api.anthropic.com")
221221
}
222222
if got := req.Header.Get("x-api-key"); got != "sk-ant-test" {
@@ -244,7 +244,7 @@ func TestBuildFromConfig_GitHubBearer(t *testing.T) {
244244
URL: &url.URL{Host: "github.com"},
245245
Header: make(http.Header),
246246
}
247-
if !store.InjectCredentials(req) {
247+
if matched, injected := store.InjectCredentials(req); !matched || !injected {
248248
t.Error("should match github.com")
249249
}
250250
if got := req.Header.Get("Authorization"); got != "Bearer ghp_test" {
@@ -256,7 +256,7 @@ func TestBuildFromConfig_GitHubBearer(t *testing.T) {
256256
URL: &url.URL{Host: "raw.githubusercontent.com"},
257257
Header: make(http.Header),
258258
}
259-
if !store.InjectCredentials(req2) {
259+
if matched, injected := store.InjectCredentials(req2); !matched || !injected {
260260
t.Error("should match raw.githubusercontent.com")
261261
}
262262
if got := req2.Header.Get("Authorization"); got != "Bearer ghp_test" {
@@ -289,7 +289,7 @@ func TestBuildFromConfig_MissingEnvVarSkipped(t *testing.T) {
289289
URL: &url.URL{Host: "api.example.com"},
290290
Header: make(http.Header),
291291
}
292-
if store.InjectCredentials(req) {
292+
if matched, _ := store.InjectCredentials(req); matched {
293293
t.Error("should not match when env var is unset")
294294
}
295295
}
@@ -324,7 +324,7 @@ func TestBuildFromConfig_MultipleEntries(t *testing.T) {
324324
URL: &url.URL{Host: "api.a.com"},
325325
Header: make(http.Header),
326326
}
327-
if !store.InjectCredentials(req1) {
327+
if matched, injected := store.InjectCredentials(req1); !matched || !injected {
328328
t.Error("should match api.a.com")
329329
}
330330
if got := req1.Header.Get("Authorization"); got != "Bearer key-a" {
@@ -336,7 +336,7 @@ func TestBuildFromConfig_MultipleEntries(t *testing.T) {
336336
URL: &url.URL{Host: "api.b.com"},
337337
Header: make(http.Header),
338338
}
339-
if !store.InjectCredentials(req2) {
339+
if matched, injected := store.InjectCredentials(req2); !matched || !injected {
340340
t.Error("should match api.b.com")
341341
}
342342
if got := req2.Header.Get("Authorization"); got != "Bearer key-b" {
@@ -372,7 +372,7 @@ func TestBuildFromConfig_GCloudFromJSON(t *testing.T) {
372372
URL: &url.URL{Host: "storage.googleapis.com"},
373373
Header: make(http.Header),
374374
}
375-
if !store.InjectCredentials(req) {
375+
if matched, _ := store.InjectCredentials(req); !matched {
376376
t.Error("should match storage.googleapis.com with gcloud injector from JSON")
377377
}
378378
}
@@ -403,7 +403,7 @@ func TestBuildFromConfig_GCloudJSONPreferredOverFile(t *testing.T) {
403403
URL: &url.URL{Host: "storage.googleapis.com"},
404404
Header: make(http.Header),
405405
}
406-
if !store.InjectCredentials(req) {
406+
if matched, _ := store.InjectCredentials(req); !matched {
407407
t.Error("should match storage.googleapis.com with gcloud injector from JSON")
408408
}
409409
}
@@ -473,7 +473,7 @@ func TestBuildFromConfig_ExactAndSuffixDomains(t *testing.T) {
473473
URL: &url.URL{Host: "exact.com"},
474474
Header: make(http.Header),
475475
}
476-
if !store.InjectCredentials(req1) {
476+
if matched, injected := store.InjectCredentials(req1); !matched || !injected {
477477
t.Error("should match exact.com")
478478
}
479479

@@ -482,7 +482,7 @@ func TestBuildFromConfig_ExactAndSuffixDomains(t *testing.T) {
482482
URL: &url.URL{Host: "sub.suffix.com"},
483483
Header: make(http.Header),
484484
}
485-
if !store.InjectCredentials(req2) {
485+
if matched, injected := store.InjectCredentials(req2); !matched || !injected {
486486
t.Error("should match sub.suffix.com")
487487
}
488488

@@ -491,7 +491,7 @@ func TestBuildFromConfig_ExactAndSuffixDomains(t *testing.T) {
491491
URL: &url.URL{Host: "other.com"},
492492
Header: make(http.Header),
493493
}
494-
if store.InjectCredentials(req3) {
494+
if matched, _ := store.InjectCredentials(req3); matched {
495495
t.Error("should not match other.com")
496496
}
497497
}

‎internal/credentials/gcloud.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,24 +72,25 @@ func (g *GCloudInjector) init() error {
7272

7373
// Inject sets the Authorization: Bearer header with a fresh OAuth2 token.
7474
// Always overrides — the agent may have a token from a dummy ADC file.
75-
func (g *GCloudInjector) Inject(req *http.Request) {
75+
func (g *GCloudInjector) Inject(req *http.Request) bool {
7676
if err := g.init(); err != nil {
7777
log.Printf("ERROR gcloud credential init failed: %v", err)
78-
return
78+
return false
7979
}
8080

8181
token, err := g.credentials.TokenSource.Token()
8282
if err != nil {
8383
log.Printf("ERROR gcloud token refresh failed: %v", err)
84-
return
84+
return false
8585
}
8686

8787
if !token.Valid() {
8888
log.Printf("WARN gcloud token is invalid after refresh")
89-
return
89+
return false
9090
}
9191

9292
req.Header.Set("Authorization", "Bearer "+token.AccessToken)
93+
return true
9394
}
9495

9596
// Available returns true if ADC credentials can be loaded (from JSON or file).

‎internal/credentials/static.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,9 @@ type HeaderInjector struct {
1414
Value string
1515
}
1616

17-
func (h *HeaderInjector) Inject(req *http.Request) {
17+
func (h *HeaderInjector) Inject(req *http.Request) bool {
1818
req.Header.Set(h.Header, h.Value)
19+
return true
1920
}
2021

2122
// BearerInjector injects an Authorization: Bearer header with a static token.
@@ -24,8 +25,9 @@ type BearerInjector struct {
2425
Token string
2526
}
2627

27-
func (b *BearerInjector) Inject(req *http.Request) {
28+
func (b *BearerInjector) Inject(req *http.Request) bool {
2829
req.Header.Set("Authorization", "Bearer "+b.Token)
30+
return true
2931
}
3032

3133
// APIKeyInjector injects a key into a custom header (e.g., x-api-key).
@@ -35,6 +37,7 @@ type APIKeyInjector struct {
3537
Key string
3638
}
3739

38-
func (a *APIKeyInjector) Inject(req *http.Request) {
40+
func (a *APIKeyInjector) Inject(req *http.Request) bool {
3941
req.Header.Set(a.HeaderName, a.Key)
42+
return true
4043
}

‎internal/credentials/store.go‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ import (
1010
// Injector can inject credentials into an HTTP request.
1111
type Injector interface {
1212
// Inject adds credential headers to the request.
13-
// It should only inject if the relevant header is not already present.
14-
Inject(req *http.Request)
13+
// Returns true if the credential was successfully set, false on error.
14+
Inject(req *http.Request) bool
1515
}
1616

1717
// Route maps a domain pattern to a credential injector.
@@ -48,8 +48,10 @@ func (s *Store) AddRoute(route Route) {
4848
}
4949

5050
// InjectCredentials finds the first matching route for the request's
51-
// host and injects credentials. Returns true if credentials were injected.
52-
func (s *Store) InjectCredentials(req *http.Request) bool {
51+
// host and injects credentials. Returns (matched, injected) where matched
52+
// indicates a route was found and injected indicates the credential was
53+
// successfully set.
54+
func (s *Store) InjectCredentials(req *http.Request) (bool, bool) {
5355
s.mu.RLock()
5456
defer s.mu.RUnlock()
5557

@@ -72,11 +74,15 @@ func (s *Store) InjectCredentials(req *http.Request) bool {
7274
}
7375

7476
if matched {
75-
route.Injector.Inject(req)
76-
log.Printf("CREDENTIAL_INJECT host=%s pattern=%s method=%s path=%s", host, matchedPattern, req.Method, req.URL.Path)
77-
return true
77+
ok := route.Injector.Inject(req)
78+
if ok {
79+
log.Printf("CREDENTIAL_INJECT host=%s pattern=%s method=%s path=%s", host, matchedPattern, req.Method, req.URL.Path)
80+
} else {
81+
log.Printf("CREDENTIAL_INJECT_FAILED host=%s pattern=%s method=%s path=%s", host, matchedPattern, req.Method, req.URL.Path)
82+
}
83+
return true, ok
7884
}
7985
}
8086

81-
return false
87+
return false, false
8288
}

‎internal/credentials/store_test.go‎

Lines changed: 51 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ func TestStore_InjectCredentials_ExactDomain(t *testing.T) {
1818
Header: make(http.Header),
1919
}
2020

21-
if !store.InjectCredentials(req) {
21+
matched, injected := store.InjectCredentials(req)
22+
if !matched || !injected {
2223
t.Error("should match github.com")
2324
}
2425
if got := req.Header.Get("Authorization"); got != "Bearer ghp_test123" {
@@ -38,7 +39,8 @@ func TestStore_InjectCredentials_DomainSuffix(t *testing.T) {
3839
Header: make(http.Header),
3940
}
4041

41-
if !store.InjectCredentials(req) {
42+
matched, injected := store.InjectCredentials(req)
43+
if !matched || !injected {
4244
t.Error("should match api.openai.com via suffix .openai.com")
4345
}
4446
if got := req.Header.Get("Authorization"); got != "Bearer sk-test" {
@@ -58,7 +60,8 @@ func TestStore_InjectCredentials_NoMatch(t *testing.T) {
5860
Header: make(http.Header),
5961
}
6062

61-
if store.InjectCredentials(req) {
63+
matched, _ := store.InjectCredentials(req)
64+
if matched {
6265
t.Error("should not match evil.com")
6366
}
6467
if got := req.Header.Get("Authorization"); got != "" {
@@ -80,7 +83,10 @@ func TestStore_InjectCredentials_AlwaysOverrides(t *testing.T) {
8083
// Agent sets a dummy/placeholder token
8184
req.Header.Set("Authorization", "Bearer paude-proxy-managed")
8285

83-
store.InjectCredentials(req)
86+
matched, injected := store.InjectCredentials(req)
87+
if !matched || !injected {
88+
t.Error("should match and inject for api.openai.com")
89+
}
8490
if got := req.Header.Get("Authorization"); got != "Bearer proxy-token" {
8591
t.Errorf("proxy should override agent's dummy token: got %q, want %q", got, "Bearer proxy-token")
8692
}
@@ -90,15 +96,19 @@ func TestAPIKeyInjector(t *testing.T) {
9096
inj := &APIKeyInjector{HeaderName: "x-api-key", Key: "sk-ant-test"}
9197

9298
req := &http.Request{Header: make(http.Header)}
93-
inj.Inject(req)
99+
if !inj.Inject(req) {
100+
t.Error("Inject should return true")
101+
}
94102
if got := req.Header.Get("x-api-key"); got != "sk-ant-test" {
95103
t.Errorf("x-api-key = %q, want %q", got, "sk-ant-test")
96104
}
97105

98106
// Should override existing (agent may have a dummy placeholder)
99107
req2 := &http.Request{Header: make(http.Header)}
100108
req2.Header.Set("x-api-key", "paude-proxy-managed")
101-
inj.Inject(req2)
109+
if !inj.Inject(req2) {
110+
t.Error("Inject should return true")
111+
}
102112
if got := req2.Header.Get("x-api-key"); got != "sk-ant-test" {
103113
t.Errorf("should override dummy key: got %q, want %q", got, "sk-ant-test")
104114
}
@@ -120,8 +130,42 @@ func TestStore_FirstMatchWins(t *testing.T) {
120130
Header: make(http.Header),
121131
}
122132

123-
store.InjectCredentials(req)
133+
matched, injected := store.InjectCredentials(req)
134+
if !matched || !injected {
135+
t.Error("should match and inject for api.openai.com")
136+
}
124137
if got := req.Header.Get("Authorization"); got != "Bearer exact-token" {
125138
t.Errorf("first match should win: got %q", got)
126139
}
127140
}
141+
142+
// failingInjector is a mock that always fails injection.
143+
type failingInjector struct{}
144+
145+
func (f *failingInjector) Inject(req *http.Request) bool {
146+
return false
147+
}
148+
149+
func TestStore_InjectCredentials_InjectorFails(t *testing.T) {
150+
store := NewStore()
151+
store.AddRoute(Route{
152+
ExactDomain: "example.com",
153+
Injector: &failingInjector{},
154+
})
155+
156+
req := &http.Request{
157+
URL: &url.URL{Host: "example.com"},
158+
Header: make(http.Header),
159+
}
160+
161+
matched, injected := store.InjectCredentials(req)
162+
if !matched {
163+
t.Error("should match example.com")
164+
}
165+
if injected {
166+
t.Error("should not report injection as successful")
167+
}
168+
if got := req.Header.Get("Authorization"); got != "" {
169+
t.Errorf("Authorization should be empty, got %q", got)
170+
}
171+
}

‎internal/proxy/integration_test.go‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -691,3 +691,59 @@ func TestIntegration_UntrustedUpstreamCert(t *testing.T) {
691691
// Connection error is also acceptable — proxy rejected the upstream
692692
t.Logf("got error (expected — proxy rejected untrusted upstream cert): %v", err)
693693
}
694+
695+
// failingInjector always fails to inject credentials.
696+
type failingInjector struct{}
697+
698+
func (f *failingInjector) Inject(req *http.Request) bool {
699+
return false
700+
}
701+
702+
func TestIntegration_CredentialInjectionFailure_Returns502(t *testing.T) {
703+
skipIntegration(t)
704+
705+
ca, err := GenerateCA()
706+
if err != nil {
707+
t.Fatalf("generate CA: %v", err)
708+
}
709+
710+
upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
711+
w.WriteHeader(http.StatusOK)
712+
}))
713+
defer upstream.Close()
714+
715+
upstreamURL, _ := url.Parse(upstream.URL)
716+
upstreamHostname := upstreamURL.Hostname()
717+
718+
df := filter.NewDomainFilter(upstreamHostname)
719+
720+
store := credentials.NewStore()
721+
store.AddRoute(credentials.Route{
722+
ExactDomain: upstreamHostname,
723+
Injector: &failingInjector{},
724+
})
725+
726+
upstreamCAs := upstreamCertPool(t, upstream)
727+
upstreamCert := upstream.TLS.Certificates[0]
728+
upstreamCA, _ := x509.ParseCertificate(upstreamCert.Certificate[0])
729+
730+
proxyAddr, cleanup := startTestProxy(t, ca, df, store, nil, upstreamCAs)
731+
defer cleanup()
732+
733+
client := httpClientViaProxy(t, proxyAddr, ca.Certificate, upstreamCA)
734+
735+
resp, err := client.Get(upstream.URL + "/test")
736+
if err != nil {
737+
t.Fatalf("request failed: %v", err)
738+
}
739+
defer resp.Body.Close()
740+
741+
if resp.StatusCode != http.StatusBadGateway {
742+
t.Errorf("expected 502 Bad Gateway, got %d", resp.StatusCode)
743+
}
744+
745+
body, _ := io.ReadAll(resp.Body)
746+
if got := string(body); got != "Proxy credential injection failed" {
747+
t.Errorf("expected injection failure message, got %q", got)
748+
}
749+
}

‎internal/proxy/proxy.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,15 @@ func New(cfg Config) *http.Server {
311311

312312
// Inject credentials for API requests
313313
if cfg.CredStore != nil {
314-
cfg.CredStore.InjectCredentials(req)
314+
matched, injected := cfg.CredStore.InjectCredentials(req)
315+
if matched && !injected {
316+
log.Printf("CREDENTIAL_INJECT_FAILED_502 method=%s host=%s path=%s", req.Method, req.URL.Host, req.URL.Path)
317+
return req, goproxy.NewResponse(req,
318+
goproxy.ContentTypeText,
319+
http.StatusBadGateway,
320+
"Proxy credential injection failed",
321+
)
322+
}
315323
}
316324

317325
// Suppress proxy identity headers

0 commit comments

Comments
 (0)