diff --git a/server/internal/handlers/realtimews/handler_test.go b/server/internal/handlers/realtimews/handler_test.go index 7939d41f2..6fb258355 100644 --- a/server/internal/handlers/realtimews/handler_test.go +++ b/server/internal/handlers/realtimews/handler_test.go @@ -474,6 +474,49 @@ func TestServe_ForeignOrgClosesForbidden(t *testing.T) { r.Equal(realtimews.CloseForbidden, websocket.CloseStatus(readErr)) } +// TestServe_DBSuperAdminForeignOrgClosesForbidden locks in the alignment +// between the WS handshake's org check and middleware.RequireOrgAccess (REST). +// A *DB* super-admin (user.SuperAdmin) whose access token is scoped to a +// different org with a non-super-admin role is 403'd by the REST middleware +// (only a *claims* super-admin may cross orgs there); the WS handshake used to +// let this exact shape through because it skipped the whole org check for any +// DB super-admin. Both paths now reject it identically. +func TestServe_DBSuperAdminForeignOrgClosesForbidden(t *testing.T) { + t.Parallel() + r := require.New(t) + + fx := newWSFixture(t, wsFixtureOpts{}) + // Org "test" (baked into fx.wsURL) must exist so the handshake reaches the + // org-mismatch check rather than the "organization not found" branch. + fx.seedOrgAndUser(t) + + // A DB super-admin whose *claims* are scoped to a different org ("other") + // with role "admin" (not "superadmin"). Login always upgrades a + // super-admin's claim role to superadmin, so mint the token via + // GenerateTokensForOAuth, which honors the role passed verbatim — this is + // the one credential shape the REST middleware rejects but the WS handshake + // historically allowed. + otherOrg := models.NewOrganization("other", "Other Org") + r.NoError(fx.dbSvc.CreateOrganization(t.Context(), otherOrg)) + + user := models.NewUser("dbadmin@example.com") + pwd := "$plaintext$pw" + user.PasswordHash = &pwd + user.SuperAdmin = true + r.NoError(fx.dbSvc.CreateUser(t.Context(), user)) + + resp, err := fx.authService.GenerateTokensForOAuth(t.Context(), user, otherOrg, "admin") + r.NoError(err) + + conn := fx.dialPreAuth(t, resp.AccessToken) + + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + _, _, readErr := conn.Read(ctx) + r.Error(readErr) + r.Equal(realtimews.CloseForbidden, websocket.CloseStatus(readErr)) +} + func TestServe_DisabledClosesImmediately4404(t *testing.T) { t.Parallel() r := require.New(t) diff --git a/server/internal/handlers/realtimews/handshake.go b/server/internal/handlers/realtimews/handshake.go index 0578dcf79..c513869f4 100644 --- a/server/internal/handlers/realtimews/handshake.go +++ b/server/internal/handlers/realtimews/handshake.go @@ -159,10 +159,20 @@ func (h *Handler) authenticate( return nil, nil, CloseForbidden, "organization not found" } + // Mirror middleware.RequireOrgAccess exactly. Only a *claims* super-admin + // may cross orgs: a DB-only super-admin (user.SuperAdmin) whose token is + // scoped to a different org is 403'd by the REST middleware, so the WS + // handshake must reject it too rather than silently allowing a cross-org + // socket the REST API would deny. (In practice a super-admin always logs in + // with super-admin claims, so this divergence is currently unreachable — + // but the two paths claim to mirror each other and now provably do.) + if !claims.IsSuperAdmin() && claims.OrgSlug != orgSlug { + return nil, nil, CloseForbidden, "access to this organization is denied" + } + + // Regular users must be members of the org; claims and DB super-admins skip + // the membership check (mirrors RequireOrgAccess's second guard). if !claims.IsSuperAdmin() && !user.SuperAdmin { - if claims.OrgSlug != orgSlug { - return nil, nil, CloseForbidden, "access to this organization is denied" - } if _, err := h.dbService.GetMemberByUserAndOrg(ctx, user.UID, org.UID); err != nil { return nil, nil, CloseForbidden, "access to this organization is denied" } diff --git a/server/internal/integrations/slack/client.go b/server/internal/integrations/slack/client.go index bd7d56e73..d9b4c3f72 100644 --- a/server/internal/integrations/slack/client.go +++ b/server/internal/integrations/slack/client.go @@ -10,6 +10,7 @@ import ( "log/slog" "net/http" "net/url" + "strconv" "strings" "time" ) @@ -343,8 +344,16 @@ func FetchOpenIDUserInfo(ctx context.Context, userAccessToken string) (*OpenIDUs // next cursor; an empty next cursor ends the loop. If maxListPages is reached // while Slack still returns a cursor, a warning is logged and the pages // fetched so far are kept rather than looping forever. +// +// The loop also stops if the cursor stops advancing — either Slack hands back +// the same cursor it was given (next == cursor) or a cursor already followed +// on an earlier page (tracked in seen). Both mean no forward progress: +// continuing would only re-fetch pages already seen, which is what causes the +// same channels to be appended maxListPages times (conversations.list is known +// to do this on its final page). See the duplicate-destinations spec. func paginate(ctx context.Context, method string, fetchPage func(cursor string) (string, error)) error { cursor := "" + seen := make(map[string]struct{}) for range maxListPages { next, err := fetchPage(cursor) @@ -356,6 +365,19 @@ func paginate(ctx context.Context, method string, fetchPage func(cursor string) return nil } + if next == cursor { + // Non-advancing cursor: Slack returned the same cursor it was + // handed. Stop rather than re-fetch the same page forever. + return nil + } + + if _, ok := seen[next]; ok { + // Repeating cursor: we already followed this cursor on an earlier + // page, so pagination is cycling. Stop. + return nil + } + + seen[next] = struct{}{} cursor = next } @@ -374,15 +396,17 @@ func (c *Client) ListChannels(ctx context.Context) ([]Channel, error) { const method = "conversations.list" channels := make([]Channel, 0, listPageSize) + seen := make(map[string]struct{}) err := paginate(ctx, method, func(cursor string) (string, error) { - payload := map[string]any{ - "types": "public_channel,private_channel", - "exclude_archived": true, - "limit": listPageSize, - } + // Form-encoded, not JSON: conversations.list ignores JSON-body args. + params := url.Values{} + params.Set("types", "public_channel,private_channel") + params.Set("exclude_archived", "true") + params.Set("limit", strconv.Itoa(listPageSize)) + if cursor != "" { - payload["cursor"] = cursor + params.Set("cursor", cursor) } var result struct { @@ -391,11 +415,21 @@ func (c *Client) ListChannels(ctx context.Context) ([]Channel, error) { ResponseMetadata ResponseMetadata `json:"response_metadata"` //nolint:tagliatelle // Slack API field } - if err := c.callAPI(ctx, method, payload, &result); err != nil { + if err := c.callFormAPI(ctx, method, params, &result); err != nil { return "", err } - channels = append(channels, result.Channels...) + // De-duplicate by Slack ID: a repeating cursor or overlapping pages + // must never surface the same channel twice in the picker. + for i := range result.Channels { + channel := result.Channels[i] + if _, ok := seen[channel.ID]; ok { + continue + } + + seen[channel.ID] = struct{}{} + channels = append(channels, channel) + } return result.ResponseMetadata.NextCursor, nil }) @@ -412,13 +446,15 @@ func (c *Client) ListUsers(ctx context.Context) ([]SlackUser, error) { const method = "users.list" users := make([]SlackUser, 0, listPageSize) + seen := make(map[string]struct{}) err := paginate(ctx, method, func(cursor string) (string, error) { - payload := map[string]any{ - "limit": listPageSize, - } + // Form-encoded, not JSON: users.list ignores JSON-body args. + params := url.Values{} + params.Set("limit", strconv.Itoa(listPageSize)) + if cursor != "" { - payload["cursor"] = cursor + params.Set("cursor", cursor) } var result struct { @@ -427,15 +463,24 @@ func (c *Client) ListUsers(ctx context.Context) ([]SlackUser, error) { ResponseMetadata ResponseMetadata `json:"response_metadata"` //nolint:tagliatelle // Slack API field } - if err := c.callAPI(ctx, method, payload, &result); err != nil { + if err := c.callFormAPI(ctx, method, params, &result); err != nil { return "", err } + // Filter out bots/deleted members and de-duplicate by Slack ID so a + // repeating cursor or overlapping pages cannot surface duplicates. for i := range result.Members { - u := result.Members[i] - if !u.IsBot && !u.Deleted { - users = append(users, u) + member := result.Members[i] + if member.IsBot || member.Deleted { + continue + } + + if _, ok := seen[member.ID]; ok { + continue } + + seen[member.ID] = struct{}{} + users = append(users, member) } return result.ResponseMetadata.NextCursor, nil @@ -447,21 +492,51 @@ func (c *Client) ListUsers(ctx context.Context) ([]SlackUser, error) { return users, nil } -// callAPI makes a Slack API call. +// callAPI makes a Slack API call with a JSON-encoded body. This is the right +// transport for Slack's write methods (chat.*, views.*, reactions.*), which +// accept application/json. It is NOT suitable for the cursor-paginated read +// methods (conversations.list, users.list) — see callFormAPI. func (c *Client) callAPI(ctx context.Context, method string, payload map[string]any, result any) error { - url := fmt.Sprintf("%s/%s", c.baseURL, method) - body, err := json.Marshal(payload) if err != nil { return fmt.Errorf("failed to marshal payload: %w", err) } + url := fmt.Sprintf("%s/%s", c.baseURL, method) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) if err != nil { return fmt.Errorf("failed to create request: %w", err) } req.Header.Set("Content-Type", "application/json; charset=utf-8") + + return c.do(req, result) +} + +// callFormAPI makes a Slack API call with form-encoded (application/x-www-form-urlencoded) +// parameters. Slack's cursor-paginated read methods (conversations.list, +// users.list) silently IGNORE arguments sent in a JSON body and fall back to +// defaults — limit 100 (not the requested 200), the first page only (cursor +// ignored, so later pages are never fetched), and public channels only (the +// `types` filter is dropped, hiding private channels). They must be called +// form-encoded so types/limit/cursor/exclude_archived are honored. +func (c *Client) callFormAPI(ctx context.Context, method string, params url.Values, result any) error { + url := fmt.Sprintf("%s/%s", c.baseURL, method) + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(params.Encode())) + if err != nil { + return fmt.Errorf("failed to create request: %w", err) + } + + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + return c.do(req, result) +} + +// do sets auth on a prepared Slack request, sends it, checks the `ok` flag, and +// unmarshals the body into result when provided. Shared by callAPI/callFormAPI. +func (c *Client) do(req *http.Request, result any) error { req.Header.Set("Authorization", "Bearer "+c.token) resp, err := c.httpClient.Do(req) diff --git a/server/internal/integrations/slack/client_test.go b/server/internal/integrations/slack/client_test.go index 75e7cc699..7c753f83e 100644 --- a/server/internal/integrations/slack/client_test.go +++ b/server/internal/integrations/slack/client_test.go @@ -2,10 +2,13 @@ package slack import ( "encoding/json" + "fmt" "maps" "net/http" "net/http/httptest" "slices" + "strconv" + "strings" "sync" "sync/atomic" "testing" @@ -39,7 +42,7 @@ type fakeSlackList struct { pages map[string]map[string]any mu sync.Mutex - payloads []map[string]any + payloads []map[string]string } // newFakeSlackList starts an httptest fake Slack serving the given pages for @@ -55,14 +58,30 @@ func newFakeSlackList(t *testing.T, path string, pages map[string]map[string]any } func (f *fakeSlackList) ServeHTTP(w http.ResponseWriter, req *http.Request) { - var payload map[string]any - if err := json.NewDecoder(req.Body).Decode(&payload); err != nil { - f.t.Errorf("fake slack: decode payload: %v", err) + // Slack's cursor-paginated read methods (conversations.list, users.list) + // only read form-encoded params — a JSON body is silently ignored. Enforce + // form encoding here so a regression back to a JSON body fails loudly + // instead of silently serving Slack's defaults (limit 100, first page, + // public channels only). + if ct := req.Header.Get("Content-Type"); !strings.HasPrefix(ct, "application/x-www-form-urlencoded") { + f.t.Errorf("fake slack: expected form-encoded request, got Content-Type %q", ct) + writeSlackJSON(f.t, w, map[string]any{"ok": false, "error": "not_form_encoded"}) + + return + } + + if err := req.ParseForm(); err != nil { + f.t.Errorf("fake slack: parse form: %v", err) writeSlackJSON(f.t, w, map[string]any{"ok": false, "error": "invalid_payload"}) return } + payload := make(map[string]string, len(req.PostForm)) + for key := range req.PostForm { + payload[key] = req.PostForm.Get(key) + } + f.mu.Lock() f.payloads = append(f.payloads, payload) f.mu.Unlock() @@ -73,7 +92,7 @@ func (f *fakeSlackList) ServeHTTP(w http.ResponseWriter, req *http.Request) { return } - cursor, _ := payload["cursor"].(string) + cursor := payload["cursor"] page, ok := f.pages[cursor] if !ok { @@ -86,7 +105,7 @@ func (f *fakeSlackList) ServeHTTP(w http.ResponseWriter, req *http.Request) { } // recordedPayloads returns a copy of the request payloads seen so far. -func (f *fakeSlackList) recordedPayloads() []map[string]any { +func (f *fakeSlackList) recordedPayloads() []map[string]string { f.mu.Lock() defer f.mu.Unlock() @@ -94,17 +113,20 @@ func (f *fakeSlackList) recordedPayloads() []map[string]any { } // newCappedFakeSlack starts an httptest fake Slack that answers every request -// with a single item and a non-empty next_cursor, i.e. a server that never -// stops advertising more pages. Returns the request counter and a Client. -func newCappedFakeSlack(t *testing.T, itemsField string, item map[string]any) (*atomic.Int64, *Client) { +// with a single unique item and a distinct, *always-advancing* next_cursor, +// i.e. a server that never stops advertising genuinely new pages. Because each +// cursor and item ID is unique per call, neither the non-advancing-cursor guard +// nor the ID de-dup fires — only the page cap stops the loop. Returns the +// request counter and a Client. +func newCappedFakeSlack(t *testing.T, itemsField string) (*atomic.Int64, *Client) { t.Helper() calls := &atomic.Int64{} server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - calls.Add(1) + n := calls.Add(1) writeSlackJSON(t, w, map[string]any{ - itemsField: []map[string]any{item}, - "response_metadata": map[string]any{"next_cursor": "always-more"}, + itemsField: []map[string]any{{"id": fmt.Sprintf("ID%d", n), "name": "endless"}}, + "response_metadata": map[string]any{"next_cursor": fmt.Sprintf("cursor-%d", n)}, }) })) t.Cleanup(server.Close) @@ -114,10 +136,10 @@ func newCappedFakeSlack(t *testing.T, itemsField string, item map[string]any) (* // requirePageSizeLimit asserts that a recorded request payload carries the // standard page-size limit. -func requirePageSizeLimit(r *require.Assertions, payload map[string]any) { - limit, ok := payload["limit"].(float64) +func requirePageSizeLimit(r *require.Assertions, payload map[string]string) { + limit, ok := payload["limit"] r.True(ok, "limit must be sent on every page") - r.Equal(listPageSize, int(limit)) + r.Equal(strconv.Itoa(listPageSize), limit) } func TestNewClientTargetsRealSlackAPI(t *testing.T) { @@ -178,15 +200,12 @@ func TestListChannelsPaginatesAcrossAllPages(t *testing.T) { cursors := make([]string, 0, len(payloads)) for _, payload := range payloads { - cursor, _ := payload["cursor"].(string) + cursor := payload["cursor"] cursors = append(cursors, cursor) requirePageSizeLimit(r, payload) r.Equal("public_channel,private_channel", payload["types"]) - - excludeArchived, ok := payload["exclude_archived"].(bool) - r.True(ok) - r.True(excludeArchived) + r.Equal("true", payload["exclude_archived"]) } r.Equal([]string{"", "cursor-2", "cursor-3"}, cursors) @@ -201,7 +220,7 @@ func TestListChannelsStopsAtPageCap(t *testing.T) { r := require.New(t) - calls, client := newCappedFakeSlack(t, "channels", map[string]any{"id": "C1", "name": "endless"}) + calls, client := newCappedFakeSlack(t, "channels") channels, err := client.ListChannels(t.Context()) r.NoError(err) @@ -275,7 +294,7 @@ func TestListUsersPaginatesAndFiltersAcrossPages(t *testing.T) { cursors := make([]string, 0, len(payloads)) for _, payload := range payloads { - cursor, _ := payload["cursor"].(string) + cursor := payload["cursor"] cursors = append(cursors, cursor) requirePageSizeLimit(r, payload) @@ -289,7 +308,7 @@ func TestListUsersStopsAtPageCap(t *testing.T) { r := require.New(t) - calls, client := newCappedFakeSlack(t, "members", map[string]any{"id": "U1", "name": "endless"}) + calls, client := newCappedFakeSlack(t, "members") users, err := client.ListUsers(t.Context()) r.NoError(err) @@ -297,3 +316,142 @@ func TestListUsersStopsAtPageCap(t *testing.T) { r.Equal(int64(maxListPages), calls.Load()) r.Len(users, maxListPages) } + +// TestListChannelsStopsOnNonAdvancingCursor is the core regression test: when +// Slack hands back a non-empty next_cursor that yields the same page again +// (the conversations.list quirk that surfaced 25 copies of every channel), the +// loop must stop instead of re-fetching maxListPages times, and each channel +// must appear exactly once. +func TestListChannelsStopsOnNonAdvancingCursor(t *testing.T) { + t.Parallel() + + // channelsPage serves the same two channels on every call, advertising the + // given next_cursor. If the loop followed a repeating cursor it would append + // these channels once per page. + channelsPage := func(nextCursor string) map[string]any { + return map[string]any{ + "channels": []map[string]any{ + {"id": "C1", "name": "alpha"}, + {"id": "C2", "name": "beta"}, + }, + "response_metadata": map[string]any{"next_cursor": nextCursor}, + } + } + + tests := []struct { + name string + pages map[string]map[string]any + wantCalls int + }{ + { + // First page ("") advertises "stuck"; the "stuck" page advertises + // "stuck" again — a cursor that never advances. + name: "cursor repeats immediately", + pages: map[string]map[string]any{ + "": channelsPage("stuck"), + "stuck": channelsPage("stuck"), + }, + wantCalls: 2, + }, + { + // Cursor advances for a couple of pages, then page "b" points back + // to the already-followed cursor "a" — a cycle. + name: "cursor repeats after N pages", + pages: map[string]map[string]any{ + "": channelsPage("a"), + "a": channelsPage("b"), + "b": channelsPage("a"), + }, + wantCalls: 3, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + r := require.New(t) + + fake, client := newFakeSlackList(t, "conversations.list", tc.pages) + + channels, err := client.ListChannels(t.Context()) + r.NoError(err) + + names := make([]string, 0, len(channels)) + for _, ch := range channels { + names = append(names, ch.Name) + } + + // Each channel appears exactly once — not wantCalls copies, and + // certainly not maxListPages copies. + r.Equal([]string{"alpha", "beta"}, names) + + // The loop terminated as soon as the cursor stopped advancing. + r.Len(fake.recordedPayloads(), tc.wantCalls) + r.Less(tc.wantCalls, maxListPages) + }) + } +} + +// TestListUsersStopsOnNonAdvancingCursor is the symmetric regression test for +// users.list — a repeating cursor must not surface duplicate users. +func TestListUsersStopsOnNonAdvancingCursor(t *testing.T) { + t.Parallel() + + membersPage := func(nextCursor string) map[string]any { + return map[string]any{ + "members": []map[string]any{ + {"id": "U1", "name": "alice", "real_name": "Alice Anderson"}, + {"id": "U2", "name": "bob", "real_name": "Bob Brown"}, + }, + "response_metadata": map[string]any{"next_cursor": nextCursor}, + } + } + + tests := []struct { + name string + pages map[string]map[string]any + wantCalls int + }{ + { + name: "cursor repeats immediately", + pages: map[string]map[string]any{ + "": membersPage("stuck"), + "stuck": membersPage("stuck"), + }, + wantCalls: 2, + }, + { + name: "cursor repeats after N pages", + pages: map[string]map[string]any{ + "": membersPage("a"), + "a": membersPage("b"), + "b": membersPage("a"), + }, + wantCalls: 3, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + r := require.New(t) + + fake, client := newFakeSlackList(t, "users.list", tc.pages) + + users, err := client.ListUsers(t.Context()) + r.NoError(err) + + ids := make([]string, 0, len(users)) + for _, u := range users { + ids = append(ids, u.ID) + } + + r.Equal([]string{"U1", "U2"}, ids) + + r.Len(fake.recordedPayloads(), tc.wantCalls) + r.Less(tc.wantCalls, maxListPages) + }) + } +} diff --git a/server/internal/integrations/slack/service_destinations_test.go b/server/internal/integrations/slack/service_destinations_test.go index 0a582b3ba..533b1e09b 100644 --- a/server/internal/integrations/slack/service_destinations_test.go +++ b/server/internal/integrations/slack/service_destinations_test.go @@ -1,7 +1,6 @@ package slack import ( - "encoding/json" "net/http" "net/http/httptest" "testing" @@ -45,10 +44,10 @@ func TestGetDestinationsAggregatesAndSorts(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc("/conversations.list", func(w http.ResponseWriter, req *http.Request) { // Two pages, both unsorted: sorting must happen over the aggregate. - var payload map[string]any - _ = json.NewDecoder(req.Body).Decode(&payload) + // conversations.list is form-encoded, so read the cursor from the form. + _ = req.ParseForm() - if cursor, _ := payload["cursor"].(string); cursor == "" { + if cursor := req.PostForm.Get("cursor"); cursor == "" { writeSlackJSON(t, w, map[string]any{ "channels": []map[string]any{ {"id": "C1", "name": "zulu"}, diff --git a/specs/done/2026/07/2026-07-10-01-slack-channel-picker-duplicate-destinations.md b/specs/done/2026/07/2026-07-10-01-slack-channel-picker-duplicate-destinations.md new file mode 100644 index 000000000..3406de0c1 --- /dev/null +++ b/specs/done/2026/07/2026-07-10-01-slack-channel-picker-duplicate-destinations.md @@ -0,0 +1,100 @@ +# Slack channel picker lists every channel dozens of times (duplicate destinations) + +## Problem + +The Slack destination picker (Channel / Direct message dropdown on the +integration settings page) shows every channel repeated many times — e.g. +`#ai-veille` appears over and over, each row carrying the same +"Bot not in channel — run /invite @solidping first" warning. + +Captured in `integrations.priv.har`, the response of +`GET /api/v1/orgs/:org/channels/:uid/slack/destinations` contains **2500 +channel entries for only 100 distinct channels — each channel repeated exactly +25 times**: + +``` +distinct channels: 100 total entries: 2500 (per channel: 25×) +users: 196 distinct = 196 total (NOT duplicated) +``` + +25 is exactly `maxListPages` ([`client.go:44`](server/internal/integrations/slack/client.go)). +So the single 100-channel page of `conversations.list` is being fetched 25 +times in a row, until the page cap stops the loop. After +`GetDestinations` sorts the list by name +([`service.go:1113`](server/internal/integrations/slack/service.go)), the 25 +copies of each channel cluster together, which is what the picker renders. + +### Root cause + +`paginate` ([`client.go:346`](server/internal/integrations/slack/client.go)) +loops until Slack returns an empty `next_cursor` or the page cap is hit. It has +**no guard against a cursor that does not advance**: + +```go +for range maxListPages { + next, err := fetchPage(cursor) + ... + if next == "" { return nil } + cursor = next // blindly trusts Slack; never checks next == cursor +} +``` + +`conversations.list` here returns a non-empty `next_cursor` that yields the same +page again (a known Slack quirk on `conversations.list`, and/or a stable cursor +on the final page), so the same 100 channels are appended 25 times. +`users.list` doesn't exhibit it, which is why users come back clean — the defect +only bites when Slack hands back a repeating/non-advancing cursor. + +There is also **no de-duplication by ID** in `ListChannels` +([`client.go:373`](server/internal/integrations/slack/client.go)) or in +`GetDestinations`, so nothing downstream catches the repeats. + +## Proposal + +Fix in `paginate` (covers both channels and users) plus a defensive de-dup: + +1. **Stop when the cursor does not advance.** In `paginate`, after getting + `next`, break the loop if `next == cursor` (or if `next` has already been + seen). A non-advancing cursor means no forward progress — continuing only + re-fetches the same page. This is the core fix. + +2. **De-duplicate by ID defensively.** In `ListChannels` (and, symmetrically, + `ListUsers`), drop entries whose Slack ID was already collected, so a buggy + cursor or overlapping pages can never surface duplicates in the picker even + if the guard above is bypassed. + +3. **Tests.** Add a table-driven test for `paginate` / `ListChannels` where the + fake Slack client returns a **constant non-empty `next_cursor`** with the + same page each call, and assert the result contains each channel exactly + once (not `maxListPages` copies). Guard the "cursor repeats immediately" + and "cursor repeats after N pages" cases. + +### Open question + +We only have the aggregated `/destinations` response in the HAR, not the raw +`conversations.list` calls, so whether Slack returns the *identical* cursor +string or a *different-but-equivalent* one each time isn't 100% certain. The +`next == cursor` guard handles the identical-string case; the seen-cursor set +plus ID de-dup (step 2) covers the different-string case. Implement both so the +fix is robust either way. + +## Implementation Plan + +Backend-only Go change in `server/internal/integrations/slack/`. + +1. **`paginate` (client.go)** — add cursor-progress guards inside the loop: + - Keep a `seen` set of cursors already followed. + - After fetching, if `next == ""` stop (unchanged). + - If `next == cursor` (non-advancing / identical cursor), stop — no forward progress. + - If `next` is already in `seen` (repeating cursor from an earlier page), stop — the loop is cycling. + - Otherwise record `next` in `seen`, advance `cursor`, continue. Page-cap warning path unchanged (still guards genuinely infinite distinct pages). + +2. **`ListChannels` (client.go)** — collect channels through a `seen` set keyed by Slack `ID`; skip any channel whose ID was already appended. Defensive de-dup so overlapping/repeating pages can never surface duplicates. + +3. **`ListUsers` (client.go)** — symmetric ID de-dup, folded into the existing bot/deleted filter. + +4. **Tests (client_test.go)** — reuse the existing fake-Slack harness: + - Update `newCappedFakeSlack` to emit *distinct advancing* cursors and unique item IDs per call, so the existing `StopsAtPageCap` tests still exercise the genuine infinite-distinct-pages case (cap stops at `maxListPages`, `len == maxListPages`). + - Add table-driven `TestListChannelsStopsOnNonAdvancingCursor` (and a symmetric users test) covering "cursor repeats immediately" and "cursor repeats after N pages", asserting each channel/user appears exactly once (not `maxListPages` copies) and the fetch loop terminates early. + +5. **QA** — `make build-backend lint-back test`. diff --git a/specs/done/2026/07/2026-07-10-02-oauth-returnto-lost-after-third-party-auth.md b/specs/done/2026/07/2026-07-10-02-oauth-returnto-lost-after-third-party-auth.md new file mode 100644 index 000000000..3e6f68cab --- /dev/null +++ b/specs/done/2026/07/2026-07-10-02-oauth-returnto-lost-after-third-party-auth.md @@ -0,0 +1,114 @@ +# Third-party (OAuth/SSO) login discards `returnTo`, always landing on the org root + +## Problem + +When an unauthenticated user hits a deep page (e.g. `/dash0/orgs/acme/checks/foo`), +the API client redirects them to login with `?returnTo={currentPath}` +([`web/dash0/src/api/client.ts:134`](web/dash0/src/api/client.ts:134)). Password, +passkey and org-picker logins all honor that `returnTo` and send the user back to +where they started. **Third-party auth (GitHub, Google, OIDC, Microsoft, GitLab, +Discord) does not** — after the provider round-trip the user always lands on +`/dash0/orgs/{org}`, losing their original destination. + +The surprising part is that `returnTo` survives almost the entire flow: + +1. The login page passes it through as `redirect_uri` on the OAuth login call + ([`login.tsx:502`](web/dash0/src/routes/orgs/$org/login.tsx:502) — `handleOAuthLogin`). +2. The backend stores it in the OAuth `state` + ([`server/internal/handlers/auth/github.go:45`](server/internal/handlers/auth/github.go:45)), + recovers it in the callback ([`github.go:81`](server/internal/handlers/auth/github.go:81)), + and builds the success redirect on top of it + ([`buildSuccessRedirect`, `github.go:111`](server/internal/handlers/auth/github.go:111)) — + appending `access_token`, `refresh_token`, `expires_in` and `org`. All social + providers share this shape (`google.go`, `oidc.go`, `microsoft.go`, `gitlab.go`, + `discord.go`). +3. The browser actually lands on the correct deep path with tokens appended, e.g. + `/dash0/orgs/acme/checks/foo?access_token=…&org=acme`. + +Then it's thrown away. The pre-React OAuth-handoff IIFE in +[`web/dash0/src/main.tsx:29`](web/dash0/src/main.tsx:29) runs before the router +mounts and rewrites the URL: + +```ts +const dest = handoff.org ? `${basepath}/orgs/${handoff.org}` : window.location.pathname; +window.history.replaceState(null, "", dest); // <-- drops the deep path +``` + +Because `buildSuccessRedirect` always sets the `org` query param +([`github.go:121`](server/internal/handlers/auth/github.go:121)), `handoff.org` is +always truthy, so the ternary unconditionally replaces the (already-correct) +`window.location.pathname` with the org root. [`main.tsx:36`](web/dash0/src/main.tsx:36) +is the single line that loses the original destination. + +The other login paths avoid this because they funnel through +`resolveDestination()` ([`web/dash0/src/lib/login-destination.ts:28`](web/dash0/src/lib/login-destination.ts:28)), +which honors `returnTo` when it's safe/same-origin/org-matching and only falls back +to the org root otherwise. The `main.tsx` handoff never calls it — it makes its own +naive decision because it runs before React. + +## Proposal + +Make the `main.tsx` OAuth handoff preserve the deep path instead of unconditionally +rewriting to the org root, reusing the same guards as every other login path. + +At [`main.tsx:36`](web/dash0/src/main.tsx:36), replace the naive ternary with a call +to `resolveDestination(handoff.org, window.location.pathname, basepath)` (or the +equivalent) so the handoff inherits the existing safe-path / same-origin / org-match +checks from [`login-destination.ts`](web/dash0/src/lib/login-destination.ts). The +current `window.location.pathname` already equals the honored `returnTo` the backend +redirected to, so the fix is essentially: keep it when it passes the guards, fall +back to `/orgs/{org}` when it doesn't. + +Notes / open questions: +- Confirm `resolveDestination` is importable this early (it must not pull in the + router or other not-yet-mounted modules). If it does, factor out the pure + safe-path/org-match helpers so the handoff can use them standalone. +- The comment at [`oauth-handoff.ts:14`](web/dash0/src/lib/oauth-handoff.ts:14) notes + `OrgLayout` has a second, equivalent handoff effect that never fires because this + IIFE strips the params first — worth confirming it stays dormant (or is removed) + after the fix. +- Verify the fix across all providers, not just GitHub, since they share + `buildSuccessRedirect`. `discord.go` defaults `redirect_uri` to `/` rather than the + org dashboard — check that path too. + +### Test coverage +- Add a dash0 E2E (or unit) test that starts on a deep page, goes through the + (mocked) OAuth handoff with `access_token` + `org` + a deep `returnTo` path, and + asserts the final URL is the deep path, not `/orgs/{org}`. +- Add a negative test: an unsafe/cross-org `returnTo` still falls back to the org + root. + +## Implementation Plan + +1. **Extract a pure handoff-destination helper** in + `web/dash0/src/lib/oauth-handoff.ts`: + `resolveHandoffDestination(org, pathname, basepath) => string`. It reuses + `resolveDestination()` from `login-destination.ts` (a pure module with no + router/DOM imports, safe to load before React mounts) and converts its + `LoginDestination` union into a plain URL string: + - `{ href }` → `href` (the honored deep `returnTo`, already in + `window.location.pathname`). + - `{ to, params: { org } }` → `${basepath}/orgs/${org}` (the org-root + fallback for unsafe / cross-org / non-org paths). + - When `org` is absent (nothing to resolve against), keep the current + `pathname` — preserving today's `handoff.org ? … : pathname` fallback. + +2. **Rewrite the `main.tsx` IIFE** (line ~36) to call + `resolveHandoffDestination(handoff.org, window.location.pathname, basepath)` + instead of the naive `handoff.org ? \`${basepath}/orgs/${handoff.org}\` : + pathname` ternary. This makes the pre-React handoff honor the deep path when + it passes the same-origin / safe-path / org-match guards, and fall back to + the org root otherwise. + +3. **Providers**: all social providers share `buildSuccessRedirect`, which + always sets `org`, so they all funnel through `resolveDestination`. Discord's + `redirect_uri` default of `/` still carries `org`, so pathname `/` fails the + guards and correctly falls back to the org root (not `/`). + +4. **OrgLayout dormant effect** (`oauth-handoff.ts:14` note): the IIFE still + strips the token params via `replaceState` before React mounts, so the + second handoff effect stays dormant — unchanged by this fix. + +5. **Tests**: unit-test `resolveHandoffDestination` in + `oauth-handoff.test.ts` — deep path preserved (positive), cross-org / unsafe + / non-org paths fall back to org root (negative), and no-org keeps pathname. diff --git a/specs/done/2026/07/2026-07-10-03-live-updates-silent-failure-e2e.md b/specs/done/2026/07/2026-07-10-03-live-updates-silent-failure-e2e.md new file mode 100644 index 000000000..e2449aa58 --- /dev/null +++ b/specs/done/2026/07/2026-07-10-03-live-updates-silent-failure-e2e.md @@ -0,0 +1,162 @@ +# Live updates silently unavailable — WS connects but registers nothing; add E2E coverage of event delivery + +## Problem + +On https://solidping.k8xp.com/dash0/orgs/stonaltech/checks the dashboard shows the +"Live updates unavailable" badge and no realtime updates arrive, with no +explanation. The WebSocket itself connects fine — the upgrade returns `101 +Switching Protocols` — but the connection never gets past auth: the client sends +its `auth` frame and nothing else ever happens (no `hello`, no `subscribe`, no +events). + +Captured HAR (`bad_ws.priv.har`, 2026-07-09 23:29 UTC) of the failing session: + +- `GET wss://solidping.k8xp.com/api/v1/orgs/stonaltech/events/ws` → `101` OK. +- The client sends exactly one frame: `{"type":"auth","token":"eyJ…"}`. +- **The JWT in that frame decodes to `"orgSlug":"test"`** — not `stonaltech` — + with `role":"user"`, `iat` 2026-07-09T23:28:51Z (issued ~17 s before the dial, + so it is a *fresh* token for the wrong org, not an expired one). +- No server frames and no further client frames appear; the UI lands on the + "Live updates unavailable" badge. + +Server-side this is exactly the deny path in +`server/internal/handlers/realtimews/handshake.go:162-168` +(`authenticate`): for a non-super-admin, `claims.OrgSlug != orgSlug` → +`CloseForbidden` ("access to this organization is denied"), socket closed, no +`hello`. So "connects but doesn't register to anything" is the client dialing, +failing org validation, and (apparently) never recovering — while the rest of +the page keeps rendering from cached REST data, making the failure look +inexplicable. + +## Likely root cause (to confirm) + +The dash0 client keeps **one global token per browser profile**, not per org: + +- `web/dash0/src/api/client.ts:31` / `:72` — a single `TOKEN_KEY` (+ + `solidping_org` at `:119`) in `localStorage`, shared by every tab on the + domain. +- `web/dash0/src/lib/live-socket.ts:200` — the WS run-loop reads that global + `getToken()` on every dial and sends it as the `auth` frame (`:296`). + +So a login or token refresh performed for another org (e.g. a second tab open +on the `test` org of the same deployment, or an org switch) clobbers the token +the `stonaltech` tab relies on. The next WS dial picks up the wrong-org token → +4403 → badge. The freshness of the wrong-org token in the HAR (`iat` 17 s +before the dial) fits a concurrent refresh/login under org `test`. + +Related inconsistency noticed while tracing (worth aligning while in there): +the WS handshake lets a **DB** super-admin (`user.SuperAdmin`) cross orgs +(`handshake.go:162`), while the REST middleware only exempts **claims** +super-admins and 403s a DB super-admin with a mismatched `claims.OrgSlug` +(`server/internal/middleware/auth.go:172-178`). The two paths claim to mirror +each other; today they don't. + +## Proposal + +Primary deliverable requested: **an end-to-end test around live updates**, so +this class of failure can't regress silently again. + +1. **E2E happy path — event delivery, not just subscription.** The existing + `web/dash0/e2e/check-live-subscription-slug.spec.ts` already covers a + subscribe rejection surfacing the badge, but nothing verifies events + actually flow. Add a Playwright spec (`web/dash0/e2e/`, patterned on that + file) that: + - logs in, opens the checks page, and asserts the full handshake on the real + socket: `auth` sent → `hello` received → `subscribe` sent → `subscribed` + ack (assert via Playwright's WebSocket frame inspection, as the existing + spec does); + - then triggers a server-side change that emits a live event (e.g. create / + rename / toggle a check via the REST API) and asserts the UI updates + **without a reload** — this is the "does it register to anything" gap the + incident exposed. + +2. **E2E regression — wrong-org token on the auth frame.** Simulate the + observed failure: seed `localStorage` with a valid token for a *different* + org (obtainable via the login API for a second org, or by overwriting + `TOKEN_KEY` mid-session), navigate to the first org's checks page, and + assert: + - the socket is closed by the server (4403) and no `hello` arrives; + - the "Live updates unavailable" badge appears (today's behavior at + minimum — see fix below for the desired end state). + +3. **Fix direction (scope to confirm before implementing).** The test in (2) + documents current behavior; the actual fix likely involves one or both of: + - client: on a 4403 close, re-resolve credentials for the org in the URL + (or at least surface *why* live updates are unavailable) instead of + parking on the badge forever; + - client: make token storage org-aware so one tab's login/refresh can't + poison another org's tab (this affects REST too, not just WS). + - server: align the WS handshake's and REST middleware's super-admin/org + checks (see inconsistency above). + +## Open questions + +- Why did the browser hold a fresh `test`-org token while browsing + `stonaltech`? Second tab, org switcher, or an auto-refresh racing a + navigation? Worth reproducing before choosing the client fix. +- Should REST requests on the page have failed too (same clobbered token)? + If they visibly kept working from TanStack Query cache, the badge may be the + only signal we get — which strengthens the case for a more explicit + "reconnect / re-login" affordance on 4403. + +## Implementation Plan + +### Findings that shape the plan +- **Event-delivery-without-reload is already partly covered.** `web/dash0/e2e/live-updates.spec.ts` + drives a heartbeat check through the public heartbeat endpoint and asserts the + dashboard/detail page updates live. But nothing asserts the *full frame handshake* + (`auth` sent → `hello` received → `subscribe` sent → `subscribed` ack) explicitly — the + existing helper only waits for a `subscribed` ack. That explicit assertion is the gap in (1). +- **Check CRUD does not emit realtime hints.** Create/rename/toggle a check publishes no + `checks`/`results` hint (verified: `server/internal/handlers/checks/service.go` only calls the + domain-event `eventNotifier.Notify`, not the realtime publisher). The reliable, deterministic + server-side trigger for a live UI change is a **heartbeat push** (`s.rt.Publish(..., KindResults)` + in `server/internal/handlers/heartbeat/service.go`), exactly as the existing spec uses. +- **The test user is a super-admin.** `test@test.com` is seeded with `SuperAdmin=true` and login + mints `Role: "superadmin"` claims, so `claims.IsSuperAdmin()` is true and the WS handshake org + check is bypassed. To reproduce a real server 4403 we need a *non-super-admin* token whose + `orgSlug` claim differs from the URL org. Mint one by seeding a fresh user via the test-only + `POST /api/v1/test/users` endpoint, then `POST /api/v1/orgs` to create an org for them — the + response carries a fresh org-scoped, role=`admin` (non-super-admin) token. +- **The "Live updates unavailable" badge is the sidebar `live-status-dot`.** A 4403 close → + `onDisabled()` → status `"disabled"` (`data-status="disabled"`, tooltip `liveStatus.unavailable`). + A generic bad-token/network close → `"reconnecting"` instead, so asserting `"disabled"` + specifically distinguishes the server-side forbidden path. +- **Server inconsistency confirmed.** `realtimews/handshake.go` `authenticate()` skips the whole + org check for a **DB** super-admin (`user.SuperAdmin`), while REST `middleware/auth.go` + `RequireOrgAccess` only exempts **claims** super-admins for the org-slug-match step and 403s a + DB-only super-admin with a mismatched `claims.OrgSlug`. In practice a DB super-admin always gets + super-admin claims, so the divergence is currently unreachable — but it is a real, cheap, + testable alignment. + +### Steps +1. **Server-side alignment (secondary, low-risk).** Restructure `realtimews/handshake.go` + `authenticate()` to mirror `RequireOrgAccess` exactly: (a) only a *claims* super-admin may cross + orgs — `if !claims.IsSuperAdmin() && claims.OrgSlug != orgSlug → CloseForbidden`; (b) the + membership check stays gated on `!claims.IsSuperAdmin() && !user.SuperAdmin`. Add a Go test in + `server/internal/handlers/realtimews/handler_test.go` locking it in: a DB super-admin + (`user.SuperAdmin=true`) with a token minted via `GenerateTokensForOAuth(user, otherOrg, "admin")` + (claims `orgSlug="other"`, role `admin`) dialing the `test` org WS must now close `CloseForbidden`. + Seed org `test` first so the check reached is the org-mismatch, not "organization not found". +2. **E2E happy path — full handshake + event delivery** (new file + `web/dash0/e2e/live-updates-handshake.spec.ts`). Create a heartbeat check via REST, register + frame listeners on the `/events/ws` socket before navigating to the check detail page, then + assert the full ordered handshake (`auth` sent → `hello` received → `subscribe` sent → + `subscribed` ack) via Playwright `framesent`/`framereceived` inspection. Then send an `up` + heartbeat and assert the UI flips to "Currently up for …" without a reload. +3. **E2E regression — wrong-org token on the auth frame** (same new file). Seed a fresh + non-super-admin user + their own org (token `T_B`, `orgSlug=B`), seed `T_B` into `localStorage` + before load, navigate to the `test` org checks page, and assert: an `auth` frame is sent, no + `hello` is ever received, the socket closes, and the sidebar `live-status-dot` reaches + `data-status="disabled"` (never `"live"`) — documenting today's behavior (badge appears). Skip + cleanly if the test-only user-seed endpoint is unavailable (server not in `SP_RUNMODE=test`), + mirroring `create-org.spec.ts`. +4. **QA.** `make build-backend lint-back test` for the Go change; `make build-dash0` + + `cd web/dash0 && bun run lint` for the E2E files. Author the E2E and run it if a test server is + reachable; otherwise report authored-but-not-run. + +### Explicitly deferred (scope-to-confirm, high risk) +- The org-aware **client token storage** rewrite of `web/dash0/src/api/client.ts` (one global token + per profile → per-org) and client **recovery on 4403** (re-resolve credentials / surface why live + is unavailable) are left for the user. The regression E2E documents the current badge behavior, + which the spec accepts as satisfying "document current behavior". diff --git a/web/dash0/e2e/live-updates-handshake.spec.ts b/web/dash0/e2e/live-updates-handshake.spec.ts new file mode 100644 index 000000000..a0aa37055 --- /dev/null +++ b/web/dash0/e2e/live-updates-handshake.spec.ts @@ -0,0 +1,291 @@ +import { test, expect, type Page } from "./fixtures"; + +// Regression coverage for the "live updates silently unavailable" incident +// (spec 2026-07-10-03): the realtime WebSocket connected (101 Switching +// Protocols) but never got past auth — the client sent one `auth` frame +// carrying a token for the WRONG org, the server closed it 4403, and the UI +// parked on the "Live updates unavailable" badge with no `hello`, no +// `subscribe`, and no events, while the rest of the page kept rendering from +// cached REST data. +// +// Two guarantees are locked in here: +// 1. Happy path — the socket completes the FULL handshake on the real +// wire (`auth` sent -> `hello` received -> `subscribe` sent -> +// `subscribed` ack) AND a server-side change (a heartbeat) reaches the +// UI live, without a reload. Nothing else asserts the whole frame +// sequence explicitly. +// 2. Regression — a valid token scoped to a DIFFERENT org on the auth frame +// is closed by the server (no `hello`), and the sidebar live-status dot +// lands on the terminal "disabled" state (the badge the incident showed), +// never "live". + +const API_BASE = ( + process.env.E2E_BASE_URL ?? "http://localhost:4000/dash0/" +).replace(/\/dash0\/?$/, ""); + +async function getAuthToken(page: Page): Promise { + const resp = await page.request.post(`${API_BASE}/api/v1/auth/login`, { + data: { org: "test", email: "test@test.com", password: "test" }, + }); + const body = await resp.json(); + return body.accessToken; +} + +interface HeartbeatCheck { + uid: string; + name: string; + hbToken: string; +} + +async function createHeartbeatCheck( + page: Page, + token: string, + name: string, +): Promise { + const hbToken = `e2e-hs-${Date.now()}-${Math.floor(Math.random() * 1e6)}`; + const resp = await page.request.post(`${API_BASE}/api/v1/orgs/test/checks`, { + headers: { Authorization: `Bearer ${token}` }, + data: { + name, + type: "heartbeat", + config: { token: hbToken }, + confirmationPeriodSeconds: 0, + recoveryPeriodSeconds: 0, + }, + }); + expect(resp.status()).toBe(201); + const body = await resp.json(); + return { uid: body.uid, name, hbToken }; +} + +async function sendHeartbeat( + page: Page, + check: HeartbeatCheck, + status: "up" | "down", +): Promise { + const resp = await page.request.get( + `${API_BASE}/api/v1/heartbeat/test/${check.uid}?token=${check.hbToken}&status=${status}`, + ); + expect(resp.status()).toBe(200); +} + +async function deleteCheck( + page: Page, + token: string, + uid: string, +): Promise { + await page.request.delete(`${API_BASE}/api/v1/orgs/test/checks/${uid}`, { + headers: { Authorization: `Bearer ${token}` }, + }); +} + +/** + * Frame-level view of the realtime `/events/ws` socket. Records — in wire + * order, via a monotonic sequence counter — the first `auth` frame the client + * sends and the first `hello`/`subscribe`/`subscribed` milestones, so a test + * can assert both that each frame appeared and that they appeared in the + * correct order. Attaches to whichever socket opens first for `/events/ws` + * (there is exactly one per org layout). + */ +function watchHandshake(page: Page) { + let seq = 0; + const hs = { + firstSentType: null as string | null, + authSentSeq: 0, + helloSeq: 0, + subscribeSentSeq: 0, + subscribedSeq: 0, + closed: false, + get authSent() { + return this.authSentSeq > 0; + }, + get helloReceived() { + return this.helloSeq > 0; + }, + get subscribeSent() { + return this.subscribeSentSeq > 0; + }, + get subscribedReceived() { + return this.subscribedSeq > 0; + }, + }; + + const typeOf = (payload: string | Buffer): string => { + const text = typeof payload === "string" ? payload : ""; + const match = text.match(/"type"\s*:\s*"([^"]+)"/); + return match ? match[1] : ""; + }; + + page.on("websocket", (ws) => { + if (!ws.url().includes("/events/ws")) return; + + ws.on("framesent", (frame) => { + const type = typeOf(frame.payload); + if (hs.firstSentType === null) hs.firstSentType = type; + if (type === "auth" && hs.authSentSeq === 0) hs.authSentSeq = ++seq; + if (type === "subscribe" && hs.subscribeSentSeq === 0) { + hs.subscribeSentSeq = ++seq; + } + }); + + ws.on("framereceived", (frame) => { + const type = typeOf(frame.payload); + if (type === "hello" && hs.helloSeq === 0) hs.helloSeq = ++seq; + if (type === "subscribed" && hs.subscribedSeq === 0) { + hs.subscribedSeq = ++seq; + } + }); + + ws.on("close", () => { + hs.closed = true; + }); + }); + + return hs; +} + +test.describe("Live updates handshake", () => { + test("the check detail socket completes auth -> hello -> subscribe -> subscribed and delivers a heartbeat live without a reload", async ({ + authenticatedPage, + }) => { + const page = authenticatedPage; + const token = await getAuthToken(page); + const check = await createHeartbeatCheck( + page, + token, + `E2E Handshake ${Date.now()}`, + ); + + try { + // Watch frames from before the socket exists so nothing is missed. + const hs = watchHandshake(page); + + await page.goto(`orgs/test/checks/${check.uid}`); + await expect(page.getByTestId("check-detail-header")).toBeVisible(); + + // The whole handshake must complete on the real socket. + await expect.poll(() => hs.authSent, { timeout: 15000 }).toBe(true); + await expect.poll(() => hs.helloReceived, { timeout: 15000 }).toBe(true); + await expect.poll(() => hs.subscribeSent, { timeout: 15000 }).toBe(true); + await expect.poll(() => hs.subscribedReceived, { timeout: 15000 }).toBe(true); + + // ...and in the right order: the very first frame the client sends is + // `auth`, the server acks it with `hello` before any `subscribe` goes + // out, and the `subscribed` ack follows the `subscribe`. + expect(hs.firstSentType, "the first client frame must be auth").toBe( + "auth", + ); + expect(hs.authSentSeq).toBeLessThan(hs.helloSeq); + expect(hs.helloSeq).toBeLessThan(hs.subscribeSentSeq); + expect(hs.subscribeSentSeq).toBeLessThan(hs.subscribedSeq); + + // A server-side change (the first heartbeat) reaches the UI live — the + // summary card flips to "Currently up for …" within a couple of + // seconds, far under the fallback poll, so only the live hint path can + // have produced it. No reload. + await sendHeartbeat(page, check, "up"); + await expect(page.getByText("Currently up for")).toBeVisible({ + timeout: 4000, + }); + } finally { + await deleteCheck(page, token, check.uid); + } + }); + + test("a valid token for a DIFFERENT org is closed by the server with no hello and lands on the disabled live badge", async ({ + page, + }) => { + // Reproduce the incident's exact credential shape: a *fresh, valid* + // non-super-admin token scoped to another org. The seeded test user is a + // super-admin (claims role "superadmin"), which bypasses the WS org check, + // so it cannot reproduce the 4403 — mint a regular user + their own org + // instead. `POST /api/v1/test/users` is SP_RUNMODE=test only; skip cleanly + // when it is unavailable, mirroring create-org.spec.ts. + const stamp = Date.now(); + const email = `wrong-org-${stamp}@unknown.example`; + const password = "Strong-Pass-123!"; + + const createUserResp = await page.request.post( + `${API_BASE}/api/v1/test/users`, + { data: { email, password, name: "Wrong Org User" } }, + ); + if (createUserResp.status() !== 201) { + test.skip( + true, + `test user-seed endpoint unavailable (server not in SP_RUNMODE=test?): ${createUserResp.status()}`, + ); + } + + // Log in (zero org), then create the user's own org — the create-org + // response mints a fresh org-scoped token with role "admin" (NOT + // super-admin), whose orgSlug claim is this new org. + const loginResp = await page.request.post( + `${API_BASE}/api/v1/auth/login`, + { data: { email, password } }, + ); + expect(loginResp.status()).toBe(200); + const session = (await loginResp.json()) as { accessToken: string }; + + const orgSlug = `wrongorg-${stamp.toString(36)}`; + const createOrgResp = await page.request.post(`${API_BASE}/api/v1/orgs`, { + headers: { Authorization: `Bearer ${session.accessToken}` }, + data: { name: `Wrong Org Co ${stamp}`, slug: orgSlug }, + }); + expect(createOrgResp.status()).toBe(201); + const orgSession = (await createOrgResp.json()) as { + accessToken: string; + refreshToken?: string; + expiresIn?: number; + }; + expect(orgSession.accessToken).toBeTruthy(); + + // Seed the wrong-org token before the app loads, then browse the `test` + // org — the socket will dial /orgs/test/events/ws and send this org-B + // token on the auth frame. + await page.addInitScript( + ({ accessToken, refreshToken, expiresIn }) => { + localStorage.setItem("solidping_session_token", accessToken as string); + if (refreshToken) { + localStorage.setItem("solidping_refresh_token", refreshToken as string); + } + if (expiresIn) { + localStorage.setItem( + "solidping_expires_at", + String(Date.now() + Number(expiresIn) * 1000), + ); + localStorage.setItem("solidping_expires_in", String(expiresIn)); + } + }, + { + accessToken: orgSession.accessToken, + refreshToken: orgSession.refreshToken ?? "", + expiresIn: orgSession.expiresIn ?? 0, + }, + ); + + const hs = watchHandshake(page); + await page.goto("orgs/test/checks"); + + // The sidebar live-status dot must reach the terminal "disabled" state: + // that is the client's reaction to a 4403 (or 4404) permanent close only — + // a generic/network close would be "reconnecting", and a successful auth + // would be "live". Reaching "disabled" here is the "Live updates + // unavailable" badge the incident reported. + const dot = page.getByTestId("live-status-dot"); + await expect(dot).toHaveAttribute("data-status", "disabled", { + timeout: 20000, + }); + + // The socket dialed and tried to authenticate, the server rejected it, and + // no `hello` ever arrived — the deny path, not a subscribe-time rejection. + expect(hs.authSent, "the client must have sent an auth frame").toBe(true); + expect( + hs.helloReceived, + "the server must never send hello on a wrong-org token", + ).toBe(false); + expect( + hs.subscribedReceived, + "no scope can be subscribed when auth is rejected", + ).toBe(false); + }); +}); diff --git a/web/dash0/src/lib/oauth-handoff.test.ts b/web/dash0/src/lib/oauth-handoff.test.ts index a2f0fded7..40def0e58 100644 --- a/web/dash0/src/lib/oauth-handoff.test.ts +++ b/web/dash0/src/lib/oauth-handoff.test.ts @@ -8,7 +8,11 @@ vi.mock("@/api/client", () => ({ setSessionMock(accessToken, refreshToken, expiresIn), })); -import { applyOAuthHandoff, parseOAuthHandoff } from "./oauth-handoff"; +import { + applyOAuthHandoff, + parseOAuthHandoff, + resolveHandoffDestination, +} from "./oauth-handoff"; describe("parseOAuthHandoff", () => { it("returns null when there is no access_token param (the common case)", () => { @@ -85,3 +89,62 @@ describe("applyOAuthHandoff", () => { expect(store["solidping_org"]).toBeUndefined(); }); }); + +describe("resolveHandoffDestination", () => { + const BASE = "/dash0"; + + it("preserves a deep returnTo path whose org matches the handoff org", () => { + // The core regression: after the provider round-trip the browser lands on + // the deep path (already in window.location.pathname); it must be kept, + // not replaced by the org root. + expect( + resolveHandoffDestination("acme", "/dash0/orgs/acme/checks/foo", BASE), + ).toBe("/dash0/orgs/acme/checks/foo"); + }); + + it("keeps the org root when the deep path already is the org root", () => { + expect(resolveHandoffDestination("acme", "/dash0/orgs/acme", BASE)).toBe( + "/dash0/orgs/acme", + ); + }); + + it("falls back to the org root for a cross-org returnTo path", () => { + expect( + resolveHandoffDestination("acme", "/dash0/orgs/other/checks", BASE), + ).toBe("/dash0/orgs/acme"); + }); + + it("falls back to the org root for a non-org path (Discord's redirect_uri=/)", () => { + // Discord defaults redirect_uri to "/" but buildSuccessRedirect still sets + // org, so we land the user on the org root — never on "/". + expect(resolveHandoffDestination("acme", "/", BASE)).toBe( + "/dash0/orgs/acme", + ); + expect(resolveHandoffDestination("acme", "/dash0", BASE)).toBe( + "/dash0/orgs/acme", + ); + }); + + it("falls back to the org root for an unsafe protocol-relative path", () => { + expect( + resolveHandoffDestination("acme", "//evil.com/dash0/orgs/acme", BASE), + ).toBe("/dash0/orgs/acme"); + }); + + it("keeps the current pathname when no org was handed off", () => { + // Nothing to resolve against — mirrors the previous fallback so behaviour + // is unchanged for the (unexpected) org-less redirect. + expect( + resolveHandoffDestination(undefined, "/dash0/orgs/acme/checks", BASE), + ).toBe("/dash0/orgs/acme/checks"); + }); + + it("works with an empty base path", () => { + expect( + resolveHandoffDestination("acme", "/orgs/acme/incidents", ""), + ).toBe("/orgs/acme/incidents"); + expect(resolveHandoffDestination("acme", "/orgs/other", "")).toBe( + "/orgs/acme", + ); + }); +}); diff --git a/web/dash0/src/lib/oauth-handoff.ts b/web/dash0/src/lib/oauth-handoff.ts index d908402aa..cc639390c 100644 --- a/web/dash0/src/lib/oauth-handoff.ts +++ b/web/dash0/src/lib/oauth-handoff.ts @@ -18,6 +18,7 @@ // left in place as harmless defense-in-depth. import { setSession } from "@/api/client"; +import { resolveDestination } from "./login-destination"; export interface OAuthHandoff { accessToken: string; @@ -58,3 +59,33 @@ export function applyOAuthHandoff(handoff: OAuthHandoff): void { localStorage.setItem("solidping_org", handoff.org); } } + +/** + * Decides where the browser should land after an OAuth handoff, once the + * token-bearing query string is dropped. Reuses {@link resolveDestination}'s + * safe-path / same-origin / org-match guards — the same funnel every other + * login path uses — so a deep `returnTo` the backend already honored (and + * which is therefore reflected in `window.location.pathname`) is preserved + * instead of unconditionally bouncing to the org root. + * + * - When the handoff carries an `org`, run the guards: a matching in-app deep + * path is kept, anything unsafe / cross-org / non-org falls back to that + * org's root (`{basepath}/orgs/{org}`). This covers providers like Discord + * whose `redirect_uri` defaults to `/` — the `/` pathname fails the guards + * and lands on the org root rather than `/`. + * - When no `org` was handed off there is nothing to resolve against, so keep + * the current pathname (mirrors the previous `handoff.org ? … : pathname` + * fallback). + * + * `login-destination.ts` is a pure module (no router/DOM imports), so this is + * safe to call from main.tsx's pre-React IIFE, before the router mounts. + */ +export function resolveHandoffDestination( + org: string | undefined, + pathname: string, + basepath: string, +): string { + if (!org) return pathname; + const dest = resolveDestination(org, pathname, basepath); + return "href" in dest ? dest.href : `${basepath}/orgs/${dest.params.org}`; +} diff --git a/web/dash0/src/main.tsx b/web/dash0/src/main.tsx index 2c16e8a6f..579e6ce07 100644 --- a/web/dash0/src/main.tsx +++ b/web/dash0/src/main.tsx @@ -8,7 +8,11 @@ import { AuthProvider, useAuth } from "@/contexts/AuthContext"; import { ErrorBoundary, RouteErrorFallback } from "@/components/shared/error-boundary"; import { ApiError, NetworkError } from "@/api/client"; import { installErrorCollector } from "@/components/feedback/errorCollector"; -import { applyOAuthHandoff, parseOAuthHandoff } from "@/lib/oauth-handoff"; +import { + applyOAuthHandoff, + parseOAuthHandoff, + resolveHandoffDestination, +} from "@/lib/oauth-handoff"; import "@fontsource-variable/inter/index.css"; import "./i18n"; import "./index.css"; @@ -32,8 +36,15 @@ const basepath = import.meta.env.VITE_BASE_URL || ""; applyOAuthHandoff(handoff); - // Drop the token from the URL and land on the resolved org dashboard. - const dest = handoff.org ? `${basepath}/orgs/${handoff.org}` : window.location.pathname; + // Drop the token from the URL. Preserve the deep `returnTo` path the backend + // already redirected us to (now in window.location.pathname) when it passes + // the shared safe-path / org-match guards, falling back to the org root + // otherwise — instead of unconditionally discarding it for the org root. + const dest = resolveHandoffDestination( + handoff.org, + window.location.pathname, + basepath, + ); window.history.replaceState(null, "", dest); })();