From 5092af8a0fc907328ffd47866d621cd2ba3a3d96 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 30 Mar 2026 21:14:09 +0200 Subject: [PATCH 1/3] fix(policies): handle 401/403 from policy provider as unauthorized error Previously, HTTP 401/403 responses from policy providers were returned as generic errors, falling through to LogAndMaskErr which masked them from the client. Now they surface as proper unauthorized errors with descriptive messages. Signed-off-by: Miguel Martinez Trivino --- app/controlplane/pkg/biz/workflowcontract.go | 11 +- .../pkg/policies/policyprovider.go | 25 ++- .../pkg/policies/policyprovider_http_test.go | 185 ++++++++++++++++++ 3 files changed, 212 insertions(+), 9 deletions(-) create mode 100644 app/controlplane/pkg/policies/policyprovider_http_test.go diff --git a/app/controlplane/pkg/biz/workflowcontract.go b/app/controlplane/pkg/biz/workflowcontract.go index e538cf37c..0615678a9 100644 --- a/app/controlplane/pkg/biz/workflowcontract.go +++ b/app/controlplane/pkg/biz/workflowcontract.go @@ -1,5 +1,5 @@ // -// Copyright 2024-2025 The Chainloop Authors. +// Copyright 2024-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -500,6 +500,9 @@ func (uc *WorkflowContractUseCase) ValidatePolicyAttachment(providerName string, } if err = provider.ValidateAttachment(att, token); err != nil { + if errors.Is(err, policies.ErrUnauthorized) { + return NewErrUnauthorized(fmt.Errorf("invalid attachment: %w", err)) + } return fmt.Errorf("invalid attachment: %w", err) } @@ -695,6 +698,9 @@ func (uc *WorkflowContractUseCase) GetPolicy(providerName, policyName, policyOrg if errors.Is(err, policies.ErrNotFound) { return nil, NewErrNotFound(fmt.Sprintf("policy %q", policyName)) } + if errors.Is(err, policies.ErrUnauthorized) { + return nil, NewErrUnauthorized(fmt.Errorf("failed to resolve policy %q: %w", policyName, err)) + } return nil, fmt.Errorf("failed to resolve policy: %w. Available providers: %s", err, uc.policyRegistry.GetProviderNames()) } @@ -716,6 +722,9 @@ func (uc *WorkflowContractUseCase) GetPolicyGroup(providerName, groupName, group if errors.Is(err, policies.ErrNotFound) { return nil, NewErrNotFound(fmt.Sprintf("policy group %q", groupName)) } + if errors.Is(err, policies.ErrUnauthorized) { + return nil, NewErrUnauthorized(fmt.Errorf("failed to resolve policy group %q: %w", groupName, err)) + } return nil, fmt.Errorf("failed to resolve policy: %w. Available providers: %s", err, uc.policyRegistry.GetProviderNames()) } diff --git a/app/controlplane/pkg/policies/policyprovider.go b/app/controlplane/pkg/policies/policyprovider.go index 88a8fc57e..7b0d11d4f 100644 --- a/app/controlplane/pkg/policies/policyprovider.go +++ b/app/controlplane/pkg/policies/policyprovider.go @@ -1,5 +1,5 @@ // -// Copyright 2024-2025 The Chainloop Authors. +// Copyright 2024-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -78,7 +78,10 @@ type ProviderAuthOpts struct { OrgName string } -var ErrNotFound = fmt.Errorf("policy not found") +var ( + ErrNotFound = fmt.Errorf("policy not found") + ErrUnauthorized = fmt.Errorf("unauthorized request to policy provider") +) // Resolve calls the remote provider for retrieving a policy func (p *PolicyProvider) Resolve(policyName, policyOrgName string, authOpts ProviderAuthOpts) (*schemaapi.Policy, *PolicyReference, error) { @@ -147,12 +150,15 @@ func (p *PolicyProvider) ValidateAttachment(att *schemaapi.PolicyAttachment, tok } if resp.StatusCode != http.StatusOK { - if resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusMethodNotAllowed { + switch resp.StatusCode { + case http.StatusNotFound, http.StatusMethodNotAllowed: // Ignore endpoint not found as it might not be implemented by the provider return nil + case http.StatusUnauthorized, http.StatusForbidden: + return ErrUnauthorized + default: + return fmt.Errorf("expected status code 200 but got %d", resp.StatusCode) } - - return fmt.Errorf("expected status code 200 but got %d", resp.StatusCode) } resBytes, err := io.ReadAll(resp.Body) @@ -233,11 +239,14 @@ func (p *PolicyProvider) queryProvider(url *url.URL, digest, orgName string, aut } if resp.StatusCode != http.StatusOK { - if resp.StatusCode == http.StatusNotFound { + switch resp.StatusCode { + case http.StatusNotFound: return "", "", ErrNotFound + case http.StatusUnauthorized, http.StatusForbidden: + return "", "", ErrUnauthorized + default: + return "", "", fmt.Errorf("expected status code 200 but got %d", resp.StatusCode) } - - return "", "", fmt.Errorf("expected status code 200 but got %d", resp.StatusCode) } resBytes, err := io.ReadAll(resp.Body) diff --git a/app/controlplane/pkg/policies/policyprovider_http_test.go b/app/controlplane/pkg/policies/policyprovider_http_test.go new file mode 100644 index 000000000..f226eed89 --- /dev/null +++ b/app/controlplane/pkg/policies/policyprovider_http_test.go @@ -0,0 +1,185 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package policies + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResolveHTTPStatusHandling(t *testing.T) { + testCases := []struct { + name string + statusCode int + wantErr error + }{ + { + name: "401 returns ErrUnauthorized", + statusCode: http.StatusUnauthorized, + wantErr: ErrUnauthorized, + }, + { + name: "403 returns ErrUnauthorized", + statusCode: http.StatusForbidden, + wantErr: ErrUnauthorized, + }, + { + name: "404 returns ErrNotFound", + statusCode: http.StatusNotFound, + wantErr: ErrNotFound, + }, + { + name: "500 returns generic error", + statusCode: http.StatusInternalServerError, + wantErr: nil, // generic error, not a sentinel + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tc.statusCode) + })) + defer server.Close() + + provider := &PolicyProvider{ + name: "test", + url: server.URL, + } + + _, _, err := provider.Resolve("test-policy", "", ProviderAuthOpts{Token: "test-token"}) + require.Error(t, err) + + if tc.wantErr != nil { + assert.ErrorIs(t, err, tc.wantErr) + } else { + assert.NotErrorIs(t, err, ErrUnauthorized) + assert.NotErrorIs(t, err, ErrNotFound) + assert.Contains(t, err.Error(), fmt.Sprintf("got %d", tc.statusCode)) + } + }) + } +} + +func TestResolveGroupHTTPStatusHandling(t *testing.T) { + testCases := []struct { + name string + statusCode int + wantErr error + }{ + { + name: "401 returns ErrUnauthorized", + statusCode: http.StatusUnauthorized, + wantErr: ErrUnauthorized, + }, + { + name: "403 returns ErrUnauthorized", + statusCode: http.StatusForbidden, + wantErr: ErrUnauthorized, + }, + { + name: "404 returns ErrNotFound", + statusCode: http.StatusNotFound, + wantErr: ErrNotFound, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tc.statusCode) + })) + defer server.Close() + + provider := &PolicyProvider{ + name: "test", + url: server.URL, + } + + _, _, err := provider.ResolveGroup("test-group", "", ProviderAuthOpts{Token: "test-token"}) + require.Error(t, err) + assert.ErrorIs(t, err, tc.wantErr) + }) + } +} + +func TestValidateAttachmentHTTPStatusHandling(t *testing.T) { + testCases := []struct { + name string + statusCode int + wantErr error + errNil bool + }{ + { + name: "401 returns ErrUnauthorized", + statusCode: http.StatusUnauthorized, + wantErr: ErrUnauthorized, + }, + { + name: "403 returns ErrUnauthorized", + statusCode: http.StatusForbidden, + wantErr: ErrUnauthorized, + }, + { + name: "404 is ignored", + statusCode: http.StatusNotFound, + errNil: true, + }, + { + name: "405 is ignored", + statusCode: http.StatusMethodNotAllowed, + errNil: true, + }, + { + name: "500 returns generic error", + statusCode: http.StatusInternalServerError, + wantErr: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tc.statusCode) + })) + defer server.Close() + + provider := &PolicyProvider{ + name: "test", + url: server.URL, + } + + err := provider.ValidateAttachment(nil, "test-token") + if tc.errNil { + assert.NoError(t, err) + return + } + + require.Error(t, err) + if tc.wantErr != nil { + assert.ErrorIs(t, err, tc.wantErr) + } else { + assert.NotErrorIs(t, err, ErrUnauthorized) + assert.Contains(t, err.Error(), fmt.Sprintf("got %d", tc.statusCode)) + } + }) + } +} From 79e5e789cc98464308e6afb2fcee8f7f1a7e7b46 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 30 Mar 2026 21:17:26 +0200 Subject: [PATCH 2/3] fix(policies): preserve upstream error message on 401/403 responses Read the response body from the policy provider and include it in the error so the caller sees the actual reason for the rejection instead of a bare sentinel. Signed-off-by: Miguel Martinez Trivino --- .../pkg/policies/policyprovider.go | 15 ++++- .../pkg/policies/policyprovider_http_test.go | 55 +++++++++++++++++-- 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/app/controlplane/pkg/policies/policyprovider.go b/app/controlplane/pkg/policies/policyprovider.go index 7b0d11d4f..97fa5e84a 100644 --- a/app/controlplane/pkg/policies/policyprovider.go +++ b/app/controlplane/pkg/policies/policyprovider.go @@ -155,7 +155,7 @@ func (p *PolicyProvider) ValidateAttachment(att *schemaapi.PolicyAttachment, tok // Ignore endpoint not found as it might not be implemented by the provider return nil case http.StatusUnauthorized, http.StatusForbidden: - return ErrUnauthorized + return fmt.Errorf("%w: %s", ErrUnauthorized, readBodyMsg(resp)) default: return fmt.Errorf("expected status code 200 but got %d", resp.StatusCode) } @@ -243,7 +243,7 @@ func (p *PolicyProvider) queryProvider(url *url.URL, digest, orgName string, aut case http.StatusNotFound: return "", "", ErrNotFound case http.StatusUnauthorized, http.StatusForbidden: - return "", "", ErrUnauthorized + return "", "", fmt.Errorf("%w: %s", ErrUnauthorized, readBodyMsg(resp)) default: return "", "", fmt.Errorf("expected status code 200 but got %d", resp.StatusCode) } @@ -287,6 +287,17 @@ func (p *PolicyProvider) queryProvider(url *url.URL, digest, orgName string, aut return response.Digest, orgName, nil } +// readBodyMsg reads the response body and returns it as a trimmed string. +// If the body cannot be read, it returns the HTTP status instead. +func readBodyMsg(resp *http.Response) string { + defer resp.Body.Close() + body, err := io.ReadAll(io.LimitReader(resp.Body, 512)) + if err != nil || len(body) == 0 { + return resp.Status + } + return string(body) +} + func unmarshalFromRaw(raw *RawMessage, out proto.Message) error { var format unmarshal.RawFormat switch raw.Format { diff --git a/app/controlplane/pkg/policies/policyprovider_http_test.go b/app/controlplane/pkg/policies/policyprovider_http_test.go index f226eed89..c4660c05a 100644 --- a/app/controlplane/pkg/policies/policyprovider_http_test.go +++ b/app/controlplane/pkg/policies/policyprovider_http_test.go @@ -29,17 +29,30 @@ func TestResolveHTTPStatusHandling(t *testing.T) { testCases := []struct { name string statusCode int + body string wantErr error + wantMsg string // substring expected in error message }{ { - name: "401 returns ErrUnauthorized", + name: "401 returns ErrUnauthorized with upstream message", statusCode: http.StatusUnauthorized, + body: "token expired", wantErr: ErrUnauthorized, + wantMsg: "token expired", }, { - name: "403 returns ErrUnauthorized", + name: "403 returns ErrUnauthorized with upstream message", statusCode: http.StatusForbidden, + body: "insufficient permissions", wantErr: ErrUnauthorized, + wantMsg: "insufficient permissions", + }, + { + name: "401 with empty body falls back to status text", + statusCode: http.StatusUnauthorized, + body: "", + wantErr: ErrUnauthorized, + wantMsg: "401 Unauthorized", }, { name: "404 returns ErrNotFound", @@ -57,6 +70,9 @@ func TestResolveHTTPStatusHandling(t *testing.T) { t.Run(tc.name, func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(tc.statusCode) + if tc.body != "" { + _, _ = w.Write([]byte(tc.body)) + } })) defer server.Close() @@ -75,6 +91,9 @@ func TestResolveHTTPStatusHandling(t *testing.T) { assert.NotErrorIs(t, err, ErrNotFound) assert.Contains(t, err.Error(), fmt.Sprintf("got %d", tc.statusCode)) } + if tc.wantMsg != "" { + assert.Contains(t, err.Error(), tc.wantMsg) + } }) } } @@ -83,17 +102,23 @@ func TestResolveGroupHTTPStatusHandling(t *testing.T) { testCases := []struct { name string statusCode int + body string wantErr error + wantMsg string }{ { - name: "401 returns ErrUnauthorized", + name: "401 returns ErrUnauthorized with upstream message", statusCode: http.StatusUnauthorized, + body: "invalid token", wantErr: ErrUnauthorized, + wantMsg: "invalid token", }, { - name: "403 returns ErrUnauthorized", + name: "403 returns ErrUnauthorized with upstream message", statusCode: http.StatusForbidden, + body: "access denied", wantErr: ErrUnauthorized, + wantMsg: "access denied", }, { name: "404 returns ErrNotFound", @@ -106,6 +131,9 @@ func TestResolveGroupHTTPStatusHandling(t *testing.T) { t.Run(tc.name, func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(tc.statusCode) + if tc.body != "" { + _, _ = w.Write([]byte(tc.body)) + } })) defer server.Close() @@ -117,6 +145,9 @@ func TestResolveGroupHTTPStatusHandling(t *testing.T) { _, _, err := provider.ResolveGroup("test-group", "", ProviderAuthOpts{Token: "test-token"}) require.Error(t, err) assert.ErrorIs(t, err, tc.wantErr) + if tc.wantMsg != "" { + assert.Contains(t, err.Error(), tc.wantMsg) + } }) } } @@ -125,18 +156,24 @@ func TestValidateAttachmentHTTPStatusHandling(t *testing.T) { testCases := []struct { name string statusCode int + body string wantErr error + wantMsg string errNil bool }{ { - name: "401 returns ErrUnauthorized", + name: "401 returns ErrUnauthorized with upstream message", statusCode: http.StatusUnauthorized, + body: "token revoked", wantErr: ErrUnauthorized, + wantMsg: "token revoked", }, { - name: "403 returns ErrUnauthorized", + name: "403 returns ErrUnauthorized with upstream message", statusCode: http.StatusForbidden, + body: "org mismatch", wantErr: ErrUnauthorized, + wantMsg: "org mismatch", }, { name: "404 is ignored", @@ -159,6 +196,9 @@ func TestValidateAttachmentHTTPStatusHandling(t *testing.T) { t.Run(tc.name, func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(tc.statusCode) + if tc.body != "" { + _, _ = w.Write([]byte(tc.body)) + } })) defer server.Close() @@ -180,6 +220,9 @@ func TestValidateAttachmentHTTPStatusHandling(t *testing.T) { assert.NotErrorIs(t, err, ErrUnauthorized) assert.Contains(t, err.Error(), fmt.Sprintf("got %d", tc.statusCode)) } + if tc.wantMsg != "" { + assert.Contains(t, err.Error(), tc.wantMsg) + } }) } } From 483e995132f5d998d35ffb5776b52f7684e040b6 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 30 Mar 2026 21:19:52 +0200 Subject: [PATCH 3/3] fix(policies): extract reason from JSON error responses Upstream policy providers return structured JSON with a reason field. Parse the response body and extract the reason for a clearer error message instead of dumping the raw JSON blob. Signed-off-by: Miguel Martinez Trivino --- app/controlplane/pkg/policies/policyprovider.go | 15 ++++++++++++--- .../pkg/policies/policyprovider_http_test.go | 7 +++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/app/controlplane/pkg/policies/policyprovider.go b/app/controlplane/pkg/policies/policyprovider.go index 97fa5e84a..b76d0cbb8 100644 --- a/app/controlplane/pkg/policies/policyprovider.go +++ b/app/controlplane/pkg/policies/policyprovider.go @@ -287,14 +287,23 @@ func (p *PolicyProvider) queryProvider(url *url.URL, digest, orgName string, aut return response.Digest, orgName, nil } -// readBodyMsg reads the response body and returns it as a trimmed string. -// If the body cannot be read, it returns the HTTP status instead. +// readBodyMsg reads the response body and extracts a human-readable error message. +// It tries to parse the body as JSON and extract the "reason" field (common in +// Connect/gRPC error responses). Falls back to the raw body, or the HTTP status text. func readBodyMsg(resp *http.Response) string { defer resp.Body.Close() - body, err := io.ReadAll(io.LimitReader(resp.Body, 512)) + body, err := io.ReadAll(io.LimitReader(resp.Body, 1024)) if err != nil || len(body) == 0 { return resp.Status } + + var structured struct { + Reason string `json:"reason"` + } + if json.Unmarshal(body, &structured) == nil && structured.Reason != "" { + return structured.Reason + } + return string(body) } diff --git a/app/controlplane/pkg/policies/policyprovider_http_test.go b/app/controlplane/pkg/policies/policyprovider_http_test.go index c4660c05a..7e7327f3f 100644 --- a/app/controlplane/pkg/policies/policyprovider_http_test.go +++ b/app/controlplane/pkg/policies/policyprovider_http_test.go @@ -47,6 +47,13 @@ func TestResolveHTTPStatusHandling(t *testing.T) { wantErr: ErrUnauthorized, wantMsg: "insufficient permissions", }, + { + name: "401 with JSON reason extracts reason field", + statusCode: http.StatusUnauthorized, + body: `{"level":"info","code":"unauthenticated","reason":"repository has no linked projects"}`, + wantErr: ErrUnauthorized, + wantMsg: "repository has no linked projects", + }, { name: "401 with empty body falls back to status text", statusCode: http.StatusUnauthorized,