diff --git a/.changeset/true-plums-switch.md b/.changeset/true-plums-switch.md new file mode 100644 index 000000000..ce26765b0 --- /dev/null +++ b/.changeset/true-plums-switch.md @@ -0,0 +1,6 @@ +--- +"@knocklabs/react-core": patch +"@knocklabs/react": patch +--- + +[Slack] Add support for nonce verification in slack auth diff --git a/packages/react-core/src/index.ts b/packages/react-core/src/index.ts index 69b69b96a..6994637ca 100644 --- a/packages/react-core/src/index.ts +++ b/packages/react-core/src/index.ts @@ -59,6 +59,7 @@ export { type KnockSlackProviderProps, type KnockSlackProviderState, type SlackChannelQueryOptions, + getSlackNonceStorageKey, useConnectedSlackChannels, useKnockSlackClient, useSlackAuth, diff --git a/packages/react-core/src/modules/core/hooks/useAuthPostMessageListener.ts b/packages/react-core/src/modules/core/hooks/useAuthPostMessageListener.ts index 40451af5d..cc3cbfaa5 100644 --- a/packages/react-core/src/modules/core/hooks/useAuthPostMessageListener.ts +++ b/packages/react-core/src/modules/core/hooks/useAuthPostMessageListener.ts @@ -7,13 +7,44 @@ export interface UseAuthPostMessageListenerOptions { popupWindowRef: React.MutableRefObject; setConnectionStatus: (status: ConnectionStatus) => void; onAuthenticationComplete?: (authenticationResp: string) => void; + /** + * The sessionStorage key where the CSRF nonce was stored when the auth URL + * was built. When provided, the listener will verify the nonce returned in + * the postMessage payload matches the stored value. + */ + nonceStorageKey?: string; +} + +/** + * Extracts the message type from a postMessage event data payload. + * Supports both the legacy string format ("authComplete") and the new + * structured format ({ type: "authComplete", nonce: "..." }). + */ +function getMessageType(data: unknown): string { + if (typeof data === "object" && data !== null && "type" in data) { + return (data as { type: string }).type; + } + return data as string; +} + +/** + * Extracts the nonce from a structured postMessage event data payload. + * Returns undefined for legacy string-format messages. + */ +function getMessageNonce(data: unknown): string | undefined { + if (typeof data === "object" && data !== null && "nonce" in data) { + const nonce = (data as { nonce: unknown }).nonce; + return typeof nonce === "string" ? nonce : undefined; + } + return undefined; } /** * Hook that listens for postMessage events from OAuth popup windows. * * Handles "authComplete" and "authFailed" messages sent from the OAuth flow popup, - * validates the message origin, updates connection status, and closes the popup. + * validates the message origin, optionally verifies the CSRF nonce, updates + * connection status, and closes the popup. * * @param options - Configuration options for the postMessage listener * @@ -24,6 +55,7 @@ export interface UseAuthPostMessageListenerOptions { * popupWindowRef, * setConnectionStatus, * onAuthenticationComplete, + * nonceStorageKey: "knock:slack-auth-nonce:channel_123:user_1", * }); * ``` */ @@ -35,26 +67,54 @@ export function useAuthPostMessageListener( popupWindowRef, setConnectionStatus, onAuthenticationComplete, + nonceStorageKey, } = options; useEffect(() => { + const closePopup = () => { + if (popupWindowRef.current && !popupWindowRef.current.closed) { + popupWindowRef.current.close(); + } + popupWindowRef.current = null; + }; + const receiveMessage = (event: MessageEvent) => { // Validate message origin for security if (event.origin !== knockHost) { return; } - if (event.data === "authComplete") { + const messageType = getMessageType(event.data); + + if (messageType === "authComplete") { + // Verify CSRF nonce when a nonceStorageKey is configured. + if (nonceStorageKey) { + const returnedNonce = getMessageNonce(event.data); + const storedNonce = sessionStorage.getItem(nonceStorageKey); + sessionStorage.removeItem(nonceStorageKey); + + // If nonce already consumed by a prior handler invocation, then bail + // out from checking again. + if ( + !returnedNonce || + (storedNonce && storedNonce !== returnedNonce) + ) { + setConnectionStatus("error"); + onAuthenticationComplete?.("authFailed"); + closePopup(); + return; + } + } + setConnectionStatus("connected"); - onAuthenticationComplete?.(event.data); - // Clear popup ref so polling stops and doesn't trigger callback again - if (popupWindowRef.current && !popupWindowRef.current.closed) { - popupWindowRef.current.close(); + onAuthenticationComplete?.(messageType); + closePopup(); + } else if (messageType === "authFailed") { + if (nonceStorageKey) { + sessionStorage.removeItem(nonceStorageKey); } - popupWindowRef.current = null; - } else if (event.data === "authFailed") { setConnectionStatus("error"); - onAuthenticationComplete?.(event.data); + onAuthenticationComplete?.(messageType); popupWindowRef.current = null; } }; @@ -66,5 +126,6 @@ export function useAuthPostMessageListener( onAuthenticationComplete, setConnectionStatus, popupWindowRef, + nonceStorageKey, ]); } diff --git a/packages/react-core/src/modules/slack/hooks/index.ts b/packages/react-core/src/modules/slack/hooks/index.ts index efb29ce33..75d192629 100644 --- a/packages/react-core/src/modules/slack/hooks/index.ts +++ b/packages/react-core/src/modules/slack/hooks/index.ts @@ -1,4 +1,7 @@ export { default as useSlackConnectionStatus } from "./useSlackConnectionStatus"; export { default as useSlackChannels } from "./useSlackChannels"; export { default as useConnectedSlackChannels } from "./useConnectedSlackChannels"; -export { default as useSlackAuth } from "./useSlackAuth"; +export { + default as useSlackAuth, + getSlackNonceStorageKey, +} from "./useSlackAuth"; diff --git a/packages/react-core/src/modules/slack/hooks/useSlackAuth.ts b/packages/react-core/src/modules/slack/hooks/useSlackAuth.ts index 91f5c9eb1..393f77a2c 100644 --- a/packages/react-core/src/modules/slack/hooks/useSlackAuth.ts +++ b/packages/react-core/src/modules/slack/hooks/useSlackAuth.ts @@ -12,6 +12,13 @@ const DEFAULT_SLACK_SCOPES = [ "groups:read", ]; +export function getSlackNonceStorageKey( + channelId: string, + userId: string, +): string { + return `knock:slack-auth-nonce:${channelId}:${userId}`; +} + type UseSlackAuthOutput = { buildSlackAuthUrl: () => string; disconnectFromSlack: () => void; @@ -83,8 +90,15 @@ function useSlackAuth( ]); const buildSlackAuthUrl = useCallback(() => { + const nonce = crypto.randomUUID(); + sessionStorage.setItem( + getSlackNonceStorageKey(knockSlackChannelId, knock.userId!), + nonce, + ); + const rawParams = { state: JSON.stringify({ + nonce, redirect_url: redirectUrl, access_token_object: { object_id: tenantId, @@ -104,6 +118,7 @@ function useSlackAuth( tenantId, knockSlackChannelId, knock.apiKey, + knock.userId, knock.userToken, knock.branch, slackClientId, diff --git a/packages/react-core/src/modules/slack/index.ts b/packages/react-core/src/modules/slack/index.ts index 1e3d15438..47b6ac7a2 100644 --- a/packages/react-core/src/modules/slack/index.ts +++ b/packages/react-core/src/modules/slack/index.ts @@ -9,6 +9,7 @@ export { useSlackChannels, useConnectedSlackChannels, useSlackAuth, + getSlackNonceStorageKey, } from "./hooks"; export { type ContainerObject, diff --git a/packages/react-core/test/core/hooks/useAuthPostMessageListener.test.ts b/packages/react-core/test/core/hooks/useAuthPostMessageListener.test.ts index c84e24b70..dfaed2a79 100644 --- a/packages/react-core/test/core/hooks/useAuthPostMessageListener.test.ts +++ b/packages/react-core/test/core/hooks/useAuthPostMessageListener.test.ts @@ -1,8 +1,25 @@ -import { renderHook } from "@testing-library/react"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { cleanup, renderHook } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { useAuthPostMessageListener } from "../../../src/modules/core/hooks/useAuthPostMessageListener"; +const mockSessionStorage = (() => { + let store: Record = {}; + return { + getItem: vi.fn((key: string) => store[key] ?? null), + setItem: vi.fn((key: string, value: string) => { + store[key] = value; + }), + removeItem: vi.fn((key: string) => { + delete store[key]; + }), + clear: vi.fn(() => { + store = {}; + }), + }; +})(); +vi.stubGlobal("sessionStorage", mockSessionStorage); + describe("useAuthPostMessageListener", () => { const knockHost = "https://api.knock.app"; let popupWindowRef: { current: Window | null }; @@ -10,8 +27,13 @@ describe("useAuthPostMessageListener", () => { let onAuthenticationComplete: ReturnType | undefined; let mockPopup: { close: ReturnType; closed: boolean }; + afterEach(() => { + cleanup(); + }); + beforeEach(() => { vi.clearAllMocks(); + mockSessionStorage.clear(); // Create a mock popup window mockPopup = { @@ -24,7 +46,7 @@ describe("useAuthPostMessageListener", () => { onAuthenticationComplete = vi.fn(); }); - it("should handle authComplete message and update status", () => { + it("should handle authComplete string message and update status", () => { renderHook(() => useAuthPostMessageListener({ knockHost, @@ -34,7 +56,6 @@ describe("useAuthPostMessageListener", () => { }), ); - // Simulate postMessage event with authComplete const event = new MessageEvent("message", { data: "authComplete", origin: knockHost, @@ -47,7 +68,29 @@ describe("useAuthPostMessageListener", () => { expect(popupWindowRef.current).toBeNull(); }); - it("should handle authFailed message and update status", () => { + it("should handle authComplete object message and update status", () => { + renderHook(() => + useAuthPostMessageListener({ + knockHost, + popupWindowRef, + setConnectionStatus, + onAuthenticationComplete, + }), + ); + + const event = new MessageEvent("message", { + data: { type: "authComplete", nonce: "abc" }, + origin: knockHost, + }); + window.dispatchEvent(event); + + expect(setConnectionStatus).toHaveBeenCalledWith("connected"); + expect(onAuthenticationComplete).toHaveBeenCalledWith("authComplete"); + expect(mockPopup.close).toHaveBeenCalled(); + expect(popupWindowRef.current).toBeNull(); + }); + + it("should handle authFailed string message and update status", () => { renderHook(() => useAuthPostMessageListener({ knockHost, @@ -57,7 +100,6 @@ describe("useAuthPostMessageListener", () => { }), ); - // Simulate postMessage event with authFailed const event = new MessageEvent("message", { data: "authFailed", origin: knockHost, @@ -65,6 +107,28 @@ describe("useAuthPostMessageListener", () => { window.dispatchEvent(event); expect(setConnectionStatus).toHaveBeenCalledWith("error"); + expect(mockPopup.close).not.toHaveBeenCalled(); + expect(popupWindowRef.current).toBeNull(); + }); + + it("should handle authFailed object message and update status", () => { + renderHook(() => + useAuthPostMessageListener({ + knockHost, + popupWindowRef, + setConnectionStatus, + onAuthenticationComplete, + }), + ); + + const event = new MessageEvent("message", { + data: { type: "authFailed" }, + origin: knockHost, + }); + window.dispatchEvent(event); + + expect(setConnectionStatus).toHaveBeenCalledWith("error"); + expect(mockPopup.close).not.toHaveBeenCalled(); expect(popupWindowRef.current).toBeNull(); }); @@ -78,7 +142,6 @@ describe("useAuthPostMessageListener", () => { }), ); - // Simulate postMessage event from different origin const event = new MessageEvent("message", { data: "authComplete", origin: "https://evil.com", @@ -111,4 +174,163 @@ describe("useAuthPostMessageListener", () => { removeEventListenerSpy.mockRestore(); }); + + describe("CSRF nonce verification", () => { + const nonceStorageKey = "knock:slack-auth-nonce:channel_123:user_123"; + + it("should accept authComplete when nonce matches", () => { + mockSessionStorage.setItem(nonceStorageKey, "valid-nonce"); + + renderHook(() => + useAuthPostMessageListener({ + knockHost, + popupWindowRef, + setConnectionStatus, + onAuthenticationComplete, + nonceStorageKey, + }), + ); + + const event = new MessageEvent("message", { + data: { type: "authComplete", nonce: "valid-nonce" }, + origin: knockHost, + }); + window.dispatchEvent(event); + + expect(setConnectionStatus).toHaveBeenCalledWith("connected"); + expect(onAuthenticationComplete).toHaveBeenCalledWith("authComplete"); + expect(mockSessionStorage.removeItem).toHaveBeenCalledWith( + nonceStorageKey, + ); + }); + + it("should reject authComplete when nonce does not match", () => { + mockSessionStorage.setItem(nonceStorageKey, "expected-nonce"); + + renderHook(() => + useAuthPostMessageListener({ + knockHost, + popupWindowRef, + setConnectionStatus, + onAuthenticationComplete, + nonceStorageKey, + }), + ); + + const event = new MessageEvent("message", { + data: { type: "authComplete", nonce: "wrong-nonce" }, + origin: knockHost, + }); + window.dispatchEvent(event); + + expect(setConnectionStatus).toHaveBeenCalledWith("error"); + expect(onAuthenticationComplete).toHaveBeenCalledWith("authFailed"); + expect(mockPopup.close).toHaveBeenCalled(); + expect(popupWindowRef.current).toBeNull(); + }); + + it("should reject legacy string format when nonceStorageKey is set", () => { + mockSessionStorage.setItem(nonceStorageKey, "stored-nonce"); + + renderHook(() => + useAuthPostMessageListener({ + knockHost, + popupWindowRef, + setConnectionStatus, + onAuthenticationComplete, + nonceStorageKey, + }), + ); + + // Legacy string has no nonce — fails verification when nonceStorageKey is configured + const event = new MessageEvent("message", { + data: "authComplete", + origin: knockHost, + }); + window.dispatchEvent(event); + + expect(setConnectionStatus).toHaveBeenCalledWith("error"); + expect(onAuthenticationComplete).toHaveBeenCalledWith("authFailed"); + expect(mockPopup.close).toHaveBeenCalled(); + expect(popupWindowRef.current).toBeNull(); + }); + + it("should reject structured authComplete without nonce field", () => { + mockSessionStorage.setItem(nonceStorageKey, "stored-nonce"); + + renderHook(() => + useAuthPostMessageListener({ + knockHost, + popupWindowRef, + setConnectionStatus, + onAuthenticationComplete, + nonceStorageKey, + }), + ); + + // Structured object but missing nonce — should not bypass verification + const event = new MessageEvent("message", { + data: { type: "authComplete" }, + origin: knockHost, + }); + window.dispatchEvent(event); + + expect(setConnectionStatus).toHaveBeenCalledWith("error"); + expect(onAuthenticationComplete).toHaveBeenCalledWith("authFailed"); + expect(mockPopup.close).toHaveBeenCalled(); + expect(popupWindowRef.current).toBeNull(); + }); + + it("should treat missing stored nonce as already consumed and accept", () => { + // Do NOT store a nonce — simulates nonce already consumed by a prior + // handler invocation; the code intentionally passes through. + renderHook(() => + useAuthPostMessageListener({ + knockHost, + popupWindowRef, + setConnectionStatus, + onAuthenticationComplete, + nonceStorageKey, + }), + ); + + const event = new MessageEvent("message", { + data: { type: "authComplete", nonce: "some-nonce" }, + origin: knockHost, + }); + window.dispatchEvent(event); + + expect(setConnectionStatus).toHaveBeenCalledWith("connected"); + expect(onAuthenticationComplete).toHaveBeenCalledWith("authComplete"); + expect(mockPopup.close).toHaveBeenCalled(); + expect(popupWindowRef.current).toBeNull(); + }); + + it("should clean up stored nonce on authFailed", () => { + mockSessionStorage.setItem(nonceStorageKey, "stored-nonce"); + + renderHook(() => + useAuthPostMessageListener({ + knockHost, + popupWindowRef, + setConnectionStatus, + onAuthenticationComplete, + nonceStorageKey, + }), + ); + + const event = new MessageEvent("message", { + data: { type: "authFailed", nonce: "stored-nonce" }, + origin: knockHost, + }); + window.dispatchEvent(event); + + expect(setConnectionStatus).toHaveBeenCalledWith("error"); + expect(mockSessionStorage.removeItem).toHaveBeenCalledWith( + nonceStorageKey, + ); + expect(mockPopup.close).not.toHaveBeenCalled(); + expect(popupWindowRef.current).toBeNull(); + }); + }); }); diff --git a/packages/react-core/test/slack/useSlackAuth.test.tsx b/packages/react-core/test/slack/useSlackAuth.test.tsx index 2b608ff4c..34e5b6173 100644 --- a/packages/react-core/test/slack/useSlackAuth.test.tsx +++ b/packages/react-core/test/slack/useSlackAuth.test.tsx @@ -2,9 +2,30 @@ import { act, renderHook } from "@testing-library/react"; import { beforeEach, describe, expect, test, vi } from "vitest"; import useSlackAuth from "../../src/modules/slack/hooks/useSlackAuth"; +import { getSlackNonceStorageKey } from "../../src/modules/slack/hooks/useSlackAuth"; const TEST_BRANCH_SLUG = "lorem-ipsum-branch"; +const mockRandomUUID = vi.fn(() => "test-nonce-uuid"); +vi.stubGlobal("crypto", { randomUUID: mockRandomUUID }); + +const mockSessionStorage = (() => { + const store: Record = {}; + return { + getItem: vi.fn((key: string) => store[key] ?? null), + setItem: vi.fn((key: string, value: string) => { + store[key] = value; + }), + removeItem: vi.fn((key: string) => { + delete store[key]; + }), + clear: vi.fn(() => { + for (const key in store) delete store[key]; + }), + }; +})(); +vi.stubGlobal("sessionStorage", mockSessionStorage); + const mockSetConnectionStatus = vi.fn(); const mockSetActionLabel = vi.fn(); @@ -26,6 +47,7 @@ vi.mock("../../src/modules/core", () => ({ slack: mockSlackClient, apiKey: "test_api_key", userToken: "test_user_token", + userId: "test_user_id", branch: TEST_BRANCH_SLUG, }), })); @@ -48,6 +70,7 @@ describe("useSlackAuth", () => { "chat:write,chat:write.public,channels:read,groups:read", ); expect(state).toEqual({ + nonce: "test-nonce-uuid", redirect_url: "http://localhost:3000", access_token_object: { object_id: "test_tenant_id", @@ -60,6 +83,42 @@ describe("useSlackAuth", () => { }); }); + test("buildSlackAuthUrl stores nonce in sessionStorage", () => { + const { result } = renderHook(() => + useSlackAuth("test_client_id", "http://localhost:3000"), + ); + + result.current.buildSlackAuthUrl(); + + expect(mockSessionStorage.setItem).toHaveBeenCalledWith( + getSlackNonceStorageKey("test_channel_id", "test_user_id"), + "test-nonce-uuid", + ); + }); + + test("buildSlackAuthUrl generates a new nonce each time", () => { + mockRandomUUID + .mockReturnValueOnce("nonce-1") + .mockReturnValueOnce("nonce-2"); + + const { result } = renderHook(() => + useSlackAuth("test_client_id", "http://localhost:3000"), + ); + + const url1 = new URL(result.current.buildSlackAuthUrl()); + const state1 = JSON.parse( + new URLSearchParams(url1.search).get("state") || "{}", + ); + + const url2 = new URL(result.current.buildSlackAuthUrl()); + const state2 = JSON.parse( + new URLSearchParams(url2.search).get("state") || "{}", + ); + + expect(state1.nonce).toBe("nonce-1"); + expect(state2.nonce).toBe("nonce-2"); + }); + test("buildSlackAuthUrl uses custom scopes when provided", () => { const { result } = renderHook(() => useSlackAuth("test_client_id", "http://localhost:3000", { diff --git a/packages/react/src/modules/slack/components/SlackAuthButton/SlackAuthButton.tsx b/packages/react/src/modules/slack/components/SlackAuthButton/SlackAuthButton.tsx index 0da638b33..f4cb9ba1f 100644 --- a/packages/react/src/modules/slack/components/SlackAuthButton/SlackAuthButton.tsx +++ b/packages/react/src/modules/slack/components/SlackAuthButton/SlackAuthButton.tsx @@ -1,4 +1,5 @@ import { + getSlackNonceStorageKey, useAuthPolling, useAuthPostMessageListener, useKnockClient, @@ -64,6 +65,10 @@ export const SlackAuthButton: FunctionComponent = ({ popupWindowRef, setConnectionStatus, onAuthenticationComplete, + nonceStorageKey: getSlackNonceStorageKey( + knockSlackChannelId, + knock.userId!, + ), }); useAuthPolling({