diff --git a/server/internal/handler/slack.go b/server/internal/handler/slack.go index b0bea24a9..d426f4c42 100644 --- a/server/internal/handler/slack.go +++ b/server/internal/handler/slack.go @@ -143,7 +143,7 @@ func (h *Handler) RegisterSlackBYO(w http.ResponseWriter, r *http.Request) { }) if err != nil { switch { - case errors.Is(err, slack.ErrInvalidBotToken), errors.Is(err, slack.ErrInvalidAppToken): + case errors.Is(err, slack.ErrInvalidBotToken), errors.Is(err, slack.ErrInvalidAppToken), errors.Is(err, slack.ErrTokenAppMismatch): writeError(w, http.StatusBadRequest, err.Error()) case errors.Is(err, slack.ErrTeamOwnedByAnotherWorkspace): writeError(w, http.StatusConflict, "this Slack app is already connected to a different Multica workspace") diff --git a/server/internal/integrations/slack/byo_install.go b/server/internal/integrations/slack/byo_install.go index 794e46428..e94bd2486 100644 --- a/server/internal/integrations/slack/byo_install.go +++ b/server/internal/integrations/slack/byo_install.go @@ -22,6 +22,11 @@ import ( var ( ErrInvalidBotToken = errors.New("slack: bot token must start with xoxb-") ErrInvalidAppToken = errors.New("slack: app-level token must start with xapp- and embed an app id") + // ErrTokenAppMismatch is returned when the pasted bot token and app-level + // token belong to DIFFERENT Slack apps. Persisting that pair would "connect" + // but be broken: inbound arrives on the app token's socket (routed by its + // app id) while mention detection + outbound use the bot token's identity. + ErrTokenAppMismatch = errors.New("slack: the bot token and app-level token are from different Slack apps") ) // RegisterBYOParams are the inputs for a bring-your-own-app install: the agent @@ -69,8 +74,28 @@ func (s *InstallService) RegisterBYO(ctx context.Context, p RegisterBYOParams) ( if err != nil { return db.ChannelInstallation{}, fmt.Errorf("slack auth.test: %w", err) } - if auth.TeamID == "" || auth.UserID == "" { - return db.ChannelInstallation{}, errors.New("slack auth.test: response missing team_id / user_id") + if auth.TeamID == "" || auth.UserID == "" || auth.BotID == "" { + return db.ChannelInstallation{}, errors.New("slack auth.test: response missing team_id / user_id / bot_id") + } + + // Prove the two tokens belong to the SAME Slack app: resolve the bot's + // OWNING app id (bots.info on the bot id auth.test returned) and require it to + // equal the app id embedded in the app-level token. Without this, pasting app + // A's bot token with app B's app token would "connect" but be broken — + // inbound arrives on app B's socket (routed by api_app_id=B) while mention + // detection + outbound use app A's bot identity / token (Niko review). + botAppID, err := s.botAppID(ctx, botToken, auth.BotID) + if err != nil { + return db.ChannelInstallation{}, fmt.Errorf("slack bots.info: %w", err) + } + if botAppID != appID { + return db.ChannelInstallation{}, ErrTokenAppMismatch + } + + // Validate the app-level token is live (Socket Mode can actually open) so we + // never persist a token that will silently never receive events. + if err := s.validateAppToken(ctx, appToken); err != nil { + return db.ChannelInstallation{}, fmt.Errorf("slack apps.connections.open: %w", err) } sealedBot, err := s.box.Seal([]byte(botToken)) @@ -106,11 +131,11 @@ func (s *InstallService) RegisterBYO(ctx context.Context, p RegisterBYOParams) ( }) } -// authTest calls Slack auth.test with the given bot token, honoring the apiURL -// override so tests can point it at an httptest server (mirrors exchangeCode). -// The Slack SDK appends the method name to the endpoint, so the base must end -// in a slash. -func (s *InstallService) authTest(ctx context.Context, botToken string) (*slack.AuthTestResponse, error) { +// slackOpts builds the slack.Client options shared by the install-time Web API +// calls, honoring the apiURL override so tests can point them at an httptest +// server. The Slack SDK appends the method name to the endpoint, so the base +// must end in a slash. A fresh slice is returned each call (safe to append to). +func (s *InstallService) slackOpts() []slack.Option { httpClient := s.httpClient if httpClient == nil { httpClient = http.DefaultClient @@ -123,7 +148,33 @@ func (s *InstallService) authTest(ctx context.Context, botToken string) (*slack. } opts = append(opts, slack.OptionAPIURL(base)) } - return slack.New(botToken, opts...).AuthTestContext(ctx) + return opts +} + +// authTest calls Slack auth.test with the bot token: validates it and returns +// the team id, the bot's own user id, and the bot id (for the bots.info lookup). +func (s *InstallService) authTest(ctx context.Context, botToken string) (*slack.AuthTestResponse, error) { + return slack.New(botToken, s.slackOpts()...).AuthTestContext(ctx) +} + +// botAppID resolves the Slack app that OWNS the bot, via bots.info on the bot id +// from auth.test. It is the only token→app_id path for a bot token, so it is how +// we prove the pasted bot + app tokens belong to the same app. +func (s *InstallService) botAppID(ctx context.Context, botToken, botID string) (string, error) { + bot, err := slack.New(botToken, s.slackOpts()...).GetBotInfoContext(ctx, slack.GetBotInfoParameters{Bot: botID}) + if err != nil { + return "", err + } + return bot.AppID, nil +} + +// validateAppToken confirms the app-level token can open a Socket Mode +// connection (apps.connections.open) — a live check that the xapp is valid for +// THIS app, so we never store a token that will silently receive nothing. +func (s *InstallService) validateAppToken(ctx context.Context, appToken string) error { + api := slack.New("", append(s.slackOpts(), slack.OptionAppLevelToken(appToken))...) + _, _, err := api.StartSocketModeContext(ctx) + return err } // parseSlackAppID extracts the real Slack app id from an app-level token. The diff --git a/server/internal/integrations/slack/byo_install_test.go b/server/internal/integrations/slack/byo_install_test.go index 9bdf6e63b..096abde3a 100644 --- a/server/internal/integrations/slack/byo_install_test.go +++ b/server/internal/integrations/slack/byo_install_test.go @@ -3,6 +3,7 @@ package slack import ( "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" "strings" @@ -13,19 +14,51 @@ import ( db "github.com/multica-ai/multica/server/pkg/db/generated" ) -// authTestServer stubs Slack auth.test. ok=false drives the bad-token path. -func authTestServer(t *testing.T, ok bool) *httptest.Server { +// slackMock parameterizes the install-time Slack API stub. botAppID defaults to +// the app id embedded in byoParams' xapp token (so the same-app check passes). +type slackMock struct { + authOK bool // auth.test result + botAppID string // bots.info -> bot.app_id + appTokenOK bool // apps.connections.open result +} + +// slackMockServer stubs the three Web API calls RegisterBYO makes: auth.test +// (bot token), bots.info (bot id -> owning app id), apps.connections.open (app +// token live check). +func slackMockServer(t *testing.T, m slackMock) *httptest.Server { t.Helper() - return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if m.botAppID == "" { + m.botAppID = "A0BCXGVCS7R" + } + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - if !ok { - _, _ = w.Write([]byte(`{"ok":false,"error":"invalid_auth"}`)) - return + switch r.URL.Path { + case "/auth.test": + if !m.authOK { + _, _ = w.Write([]byte(`{"ok":false,"error":"invalid_auth"}`)) + return + } + _, _ = w.Write([]byte(`{"ok":true,"team_id":"T999","user_id":"UBOTBYO","bot_id":"B0BOT","team":"Acme Inc","url":"https://acme.slack.com/"}`)) + case "/bots.info": + _, _ = w.Write([]byte(fmt.Sprintf(`{"ok":true,"bot":{"id":"B0BOT","app_id":%q,"user_id":"UBOTBYO"}}`, m.botAppID))) + case "/apps.connections.open": + if !m.appTokenOK { + _, _ = w.Write([]byte(`{"ok":false,"error":"invalid_auth"}`)) + return + } + _, _ = w.Write([]byte(`{"ok":true,"url":"wss://example.test/link"}`)) + default: + _, _ = w.Write([]byte(`{"ok":false,"error":"unknown_method"}`)) } - _, _ = w.Write([]byte(`{"ok":true,"team_id":"T999","user_id":"UBOTBYO","team":"Acme Inc","url":"https://acme.slack.com/"}`)) })) } +// authTestServer is the happy-path stub (valid bot token, matching app id, live +// app token) unless ok=false, which makes auth.test reject the bot token. +func authTestServer(t *testing.T, ok bool) *httptest.Server { + return slackMockServer(t, slackMock{authOK: ok, appTokenOK: true}) +} + func byoParams(ws, agent string) RegisterBYOParams { return RegisterBYOParams{ WorkspaceID: pgtypeUUID(ws), @@ -219,3 +252,44 @@ func TestRegisterBYO_AgentMove_RetiresStaleSessionBindings(t *testing.T) { t.Fatal("an agent change must retire the installation's chat-session bindings") } } + +func TestRegisterBYO_TokenAppMismatch(t *testing.T) { + // The bot token belongs to a DIFFERENT app (bots.info -> A0OTHER) than the + // app id embedded in the xapp token (A0BCXGVCS7R) — must be rejected so we + // never persist a broken installation (Niko review). + srv := slackMockServer(t, slackMock{authOK: true, botAppID: "A0OTHERAPP", appTokenOK: true}) + defer srv.Close() + q := &fakeInstallQueries{} + svc := newTestInstallService(t, q) + svc.apiURL = srv.URL + "/" + + if _, err := svc.RegisterBYO(context.Background(), byoParams( + "11111111-1111-1111-1111-111111111111", + "22222222-2222-2222-2222-222222222222", + )); err != ErrTokenAppMismatch { + t.Fatalf("mismatched tokens = %v, want ErrTokenAppMismatch", err) + } + if q.upsertCalled { + t.Error("mismatched bot/app tokens must be rejected before the upsert") + } +} + +func TestRegisterBYO_AppTokenNotLive(t *testing.T) { + // auth.test + same-app check pass, but apps.connections.open rejects the app + // token — we must not persist a token that will never receive events. + srv := slackMockServer(t, slackMock{authOK: true, appTokenOK: false}) + defer srv.Close() + q := &fakeInstallQueries{} + svc := newTestInstallService(t, q) + svc.apiURL = srv.URL + "/" + + if _, err := svc.RegisterBYO(context.Background(), byoParams( + "11111111-1111-1111-1111-111111111111", + "22222222-2222-2222-2222-222222222222", + )); err == nil { + t.Fatal("expected an error when the app-level token is not live") + } + if q.upsertCalled { + t.Error("an invalid app token must not persist an installation") + } +}