From 9c0f95bab10a2cc081f063d4d06f9cf6049d5eb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Sibiril?= <81782+aureliensibiril@users.noreply.github.com> Date: Tue, 21 Jul 2026 18:36:06 +0200 Subject: [PATCH] Restore trailing slash on Sentry API requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit f5703d390 replaced the fmt.Sprintf URL construction with url.JoinPath, which calls path.Join and therefore strips a trailing slash unless the final element carries one. Sentry's API only routes slashed paths and answers 404 without redirecting, so every ListAccounts call failed on its first request and no access-review campaign targeting Sentry could fetch a single account. The failure was invisible for two reasons. queryMembers maps 404 to errSentryOrgNotAccessible, so a routing bug surfaced to users as "reconnect the connector with the correct organization" -- advice that could never help, because the slug was never wrong. And commit 74ce2bc5d edited the recorded request URL in testdata/sentry.yaml to match the new construction instead of re-recording the cassette, which kept CI green; that cassette still carries Sentry's own Link header with the trailing slash, contradicting its own request line. Pass the slash on the final JoinPath element in both the members endpoint and the organization name resolver, revert the cassette to the URL Sentry actually served, and add a regression test that drives the driver against a server which 404s unslashed paths, so the URL shape is pinned independently of the cassette matcher. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com> --- pkg/accessreview/drivers/name_resolver.go | 3 +- .../drivers/name_resolver_test.go | 2 +- pkg/accessreview/drivers/sentry.go | 5 ++- pkg/accessreview/drivers/sentry_test.go | 41 ++++++++++++++++++- pkg/accessreview/drivers/testdata/sentry.yaml | 2 +- 5 files changed, 47 insertions(+), 6 deletions(-) diff --git a/pkg/accessreview/drivers/name_resolver.go b/pkg/accessreview/drivers/name_resolver.go index 4a02c3420..37730bd2f 100644 --- a/pkg/accessreview/drivers/name_resolver.go +++ b/pkg/accessreview/drivers/name_resolver.go @@ -708,7 +708,8 @@ func (r *sentryNameResolver) ResolveInstanceName(ctx context.Context) (string, e return "", nil } - endpoint, err := url.JoinPath("https://sentry.io", "api", "0", "organizations", url.PathEscape(r.orgSlug)) + // Trailing slash required; see SentryDriver.ListAccounts. + endpoint, err := url.JoinPath("https://sentry.io", "api", "0", "organizations", url.PathEscape(r.orgSlug)+"/") if err != nil { return "", fmt.Errorf("cannot build sentry organization URL: %w", err) } diff --git a/pkg/accessreview/drivers/name_resolver_test.go b/pkg/accessreview/drivers/name_resolver_test.go index 0e2aa7d43..71c83b18a 100644 --- a/pkg/accessreview/drivers/name_resolver_test.go +++ b/pkg/accessreview/drivers/name_resolver_test.go @@ -170,7 +170,7 @@ func TestSentryNameResolver(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, http.MethodGet, r.Method) - assert.Equal(t, "/api/0/organizations/acme", r.URL.Path) + assert.Equal(t, "/api/0/organizations/acme/", r.URL.Path) w.Header().Set("Content-Type", "application/json") w.WriteHeader(tc.status) _, _ = w.Write([]byte(tc.body)) diff --git a/pkg/accessreview/drivers/sentry.go b/pkg/accessreview/drivers/sentry.go index f45391112..d6a95ac1d 100644 --- a/pkg/accessreview/drivers/sentry.go +++ b/pkg/accessreview/drivers/sentry.go @@ -101,7 +101,10 @@ func (d *SentryDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error var records []AccountRecord - nextURL, err := url.JoinPath("https://sentry.io", "api", "0", "organizations", url.PathEscape(orgSlug), "members") + // The trailing slash is required: Sentry's API only routes slashed + // paths and answers 404 (without redirecting) otherwise. url.JoinPath + // keeps it only when the last element carries it. + nextURL, err := url.JoinPath("https://sentry.io", "api", "0", "organizations", url.PathEscape(orgSlug), "members/") if err != nil { return nil, fmt.Errorf("cannot build sentry members URL: %w", err) } diff --git a/pkg/accessreview/drivers/sentry_test.go b/pkg/accessreview/drivers/sentry_test.go index 23ee89029..181005a8d 100644 --- a/pkg/accessreview/drivers/sentry_test.go +++ b/pkg/accessreview/drivers/sentry_test.go @@ -25,6 +25,7 @@ import ( "net/http" "net/http/httptest" "os" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -54,12 +55,48 @@ func TestSentryDriver(t *testing.T) { assert.NotEmpty(t, r.Roles) } +// TestSentryDriverRequestsTrailingSlashPaths pins the exact request paths +// against Sentry's real routing: its API is Django-based and only matches +// paths ending in a slash, answering 404 (no redirect) otherwise. A 404 is +// indistinguishable from a revoked membership here, so dropping the slash +// silently turns every campaign fetch into a bogus "reconnect" error. +func TestSentryDriverRequestsTrailingSlashPaths(t *testing.T) { + t.Parallel() + + var gotPaths []string + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPaths = append(gotPaths, r.URL.Path) + + // Mimic Sentry: unslashed paths do not route. + if !strings.HasSuffix(r.URL.Path, "/") { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"detail":"The requested resource does not exist"}`)) + + return + } + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[{"id":"42","email":"alice@example.com","name":"Alice","orgRole":"member"}]`)) + })) + defer srv.Close() + + client := &http.Client{Transport: &hostRewriter{target: srv.URL}} + + records, err := NewSentryDriver(client, "acme-corp").ListAccounts(context.Background()) + require.NoError(t, err) + require.Len(t, records, 1) + assert.Equal(t, "alice@example.com", records[0].Email) + assert.Equal(t, []string{"/api/0/organizations/acme-corp/members/"}, gotPaths) +} + func TestSentryDriverListAccountsStaleSlug(t *testing.T) { t.Parallel() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, http.MethodGet, r.Method) - assert.Equal(t, "/api/0/organizations/acme-old/members", r.URL.Path) + assert.Equal(t, "/api/0/organizations/acme-old/members/", r.URL.Path) w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"detail":"The requested resource does not exist"}`)) })) @@ -88,7 +125,7 @@ func TestSentryDriverListAccountsAutoDiscoversSlug(t *testing.T) { assert.Equal(t, "true", r.URL.Query().Get("member")) w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(`[{"slug":"` + discoveredSlug + `","name":"Discovered Org"}]`)) - case "/api/0/organizations/" + discoveredSlug + "/members": + case "/api/0/organizations/" + discoveredSlug + "/members/": w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(`[{"id":"42","email":"alice@example.com","name":"Alice","orgRole":"member"}]`)) default: diff --git a/pkg/accessreview/drivers/testdata/sentry.yaml b/pkg/accessreview/drivers/testdata/sentry.yaml index 599b457b3..3e0e6e9eb 100644 --- a/pkg/accessreview/drivers/testdata/sentry.yaml +++ b/pkg/accessreview/drivers/testdata/sentry.yaml @@ -8,7 +8,7 @@ interactions: proto_minor: 1 content_length: 0 host: sentry.io - url: https://sentry.io/api/0/organizations/acme-corp/members + url: https://sentry.io/api/0/organizations/acme-corp/members/ method: GET response: proto: HTTP/2.0