diff --git a/CHANGELOG.md b/CHANGELOG.md index 050a3a38f07..2927975a1a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Plugins/Google Chat: log webhook auth rejection reasons only after all candidates fail, and warn when add-on `appPrincipal` values do not match configuration. Fixes #71078. (#71145) Thanks @luyao618. - Models/configure: preserve the existing default model when provider auth is re-run from configure while keeping explicit default-setting commands authoritative. Fixes #70696. (#70793) Thanks @Sathvik-1007. - Codex harness/models: keep legacy `codex/*` harness shorthand out of model picker and `/models` choice surfaces while migrating primary legacy refs to canonical `openai/*` plus explicit Codex harness config. (#71193) Thanks @vincentkoc. - Plugins/runtime deps: respect explicit plugin and channel disablement when repairing bundled runtime dependencies, so doctor and health checks no longer install deps for disabled configured channels. diff --git a/extensions/googlechat/src/monitor-webhook.test.ts b/extensions/googlechat/src/monitor-webhook.test.ts index 5ee092bc21a..77ddf41e5fe 100644 --- a/extensions/googlechat/src/monitor-webhook.test.ts +++ b/extensions/googlechat/src/monitor-webhook.test.ts @@ -23,6 +23,7 @@ vi.mock("./auth.js", () => ({ type ProcessEventFn = (event: GoogleChatEvent, target: WebhookTarget) => Promise; let createGoogleChatWebhookRequestHandler: typeof import("./monitor-webhook.js").createGoogleChatWebhookRequestHandler; +let warnAppPrincipalMisconfiguration: typeof import("./monitor-webhook.js").warnAppPrincipalMisconfiguration; function createRequest(authorization?: string): IncomingMessage { return { @@ -93,7 +94,8 @@ async function runWebhookHandler(options?: { describe("googlechat monitor webhook", () => { beforeAll(async () => { - ({ createGoogleChatWebhookRequestHandler } = await import("./monitor-webhook.js")); + ({ createGoogleChatWebhookRequestHandler, warnAppPrincipalMisconfiguration } = + await import("./monitor-webhook.js")); }); beforeEach(() => { @@ -156,14 +158,217 @@ describe("googlechat monitor webhook", () => { expect(res.headers["Content-Type"]).toBe("application/json"); }); + it("logs WARN with reason when verification fails (missing token)", async () => { + const logFn = vi.fn(); + installSimplePipeline([ + { + account: { + accountId: "acct-1", + config: { appPrincipal: "chat-app" }, + }, + runtime: { log: logFn, error: vi.fn() }, + audienceType: "app-url", + audience: "https://example.com/googlechat", + }, + ]); + readJsonWebhookBodyOrReject.mockResolvedValue({ + ok: true, + value: { + commonEventObject: { hostApp: "CHAT" }, + authorizationEventObject: { systemIdToken: "bad-token" }, + chat: { + messagePayload: { + space: { name: "spaces/AAA" }, + message: { name: "spaces/AAA/messages/1", text: "hi" }, + }, + }, + }, + }); + resolveWebhookTargetWithAuthOrReject.mockImplementation(async ({ isMatch, targets, res }) => { + for (const target of targets) { + if (await isMatch(target)) { + return target; + } + } + res.statusCode = 401; + res.end("unauthorized"); + return null; + }); + verifyGoogleChatRequest.mockResolvedValue({ ok: false, reason: "missing token" }); + const { processEvent, res } = await runWebhookHandler(); + + expect(logFn).toHaveBeenCalledWith(expect.stringContaining("acct-1")); + expect(logFn).toHaveBeenCalledWith(expect.stringContaining("missing token")); + expect(processEvent).not.toHaveBeenCalled(); + expect(res.statusCode).toBe(401); + }); + + it("logs WARN with reason when verification fails (unexpected principal)", async () => { + const logFn = vi.fn(); + installSimplePipeline([ + { + account: { + accountId: "acct-2", + config: { appPrincipal: "chat-app" }, + }, + runtime: { log: logFn, error: vi.fn() }, + audienceType: "app-url", + audience: "https://example.com/googlechat", + }, + ]); + readJsonWebhookBodyOrReject.mockResolvedValue({ + ok: true, + value: { + commonEventObject: { hostApp: "CHAT" }, + authorizationEventObject: { systemIdToken: "bad-token" }, + chat: { + messagePayload: { + space: { name: "spaces/AAA" }, + message: { name: "spaces/AAA/messages/1", text: "hi" }, + }, + }, + }, + }); + resolveWebhookTargetWithAuthOrReject.mockImplementation(async ({ isMatch, targets, res }) => { + for (const target of targets) { + if (await isMatch(target)) { + return target; + } + } + res.statusCode = 401; + res.end("unauthorized"); + return null; + }); + verifyGoogleChatRequest.mockResolvedValue({ + ok: false, + reason: "unexpected add-on principal: 999999999999999999999", + }); + const { processEvent, res } = await runWebhookHandler(); + + expect(logFn).toHaveBeenCalledWith(expect.stringContaining("acct-2")); + expect(logFn).toHaveBeenCalledWith( + expect.stringContaining("unexpected add-on principal: 999999999999999999999"), + ); + expect(processEvent).not.toHaveBeenCalled(); + expect(res.statusCode).toBe(401); + }); + + it("does not log WARN when verification succeeds", async () => { + const logFn = vi.fn(); + installSimplePipeline([ + { + account: { + accountId: "acct-ok", + config: { appPrincipal: "chat-app" }, + }, + runtime: { log: logFn, error: vi.fn() }, + statusSink: vi.fn(), + audienceType: "app-url", + audience: "https://example.com/googlechat", + }, + ]); + readJsonWebhookBodyOrReject.mockResolvedValue({ + ok: true, + value: { + commonEventObject: { hostApp: "CHAT" }, + authorizationEventObject: { systemIdToken: "good-token" }, + chat: { + eventTime: "2026-03-22T00:00:00.000Z", + user: { name: "users/123" }, + messagePayload: { + space: { name: "spaces/AAA" }, + message: { name: "spaces/AAA/messages/1", text: "hi" }, + }, + }, + }, + }); + resolveWebhookTargetWithAuthOrReject.mockImplementation(async ({ isMatch, targets }) => { + for (const target of targets) { + if (await isMatch(target)) { + return target; + } + } + return null; + }); + verifyGoogleChatRequest.mockResolvedValue({ ok: true }); + const { res } = await runWebhookHandler(); + + expect(logFn).not.toHaveBeenCalled(); + expect(res.statusCode).toBe(200); + }); + + it("does not log failed candidate targets when another target verifies", async () => { + const logA = vi.fn(); + const logB = vi.fn(); + installSimplePipeline([ + { + account: { + accountId: "acct-a", + config: { appPrincipal: "chat-app-a" }, + }, + runtime: { log: logA, error: vi.fn() }, + audienceType: "app-url", + audience: "https://example.com/googlechat", + }, + { + account: { + accountId: "acct-b", + config: { appPrincipal: "chat-app-b" }, + }, + runtime: { log: logB, error: vi.fn() }, + statusSink: vi.fn(), + audienceType: "app-url", + audience: "https://example.com/googlechat", + }, + ]); + readJsonWebhookBodyOrReject.mockResolvedValue({ + ok: true, + value: { + commonEventObject: { hostApp: "CHAT" }, + authorizationEventObject: { systemIdToken: "shared-path-token" }, + chat: { + eventTime: "2026-03-22T00:00:00.000Z", + user: { name: "users/123" }, + messagePayload: { + space: { name: "spaces/BBB" }, + message: { name: "spaces/BBB/messages/1", text: "hi" }, + }, + }, + }, + }); + resolveWebhookTargetWithAuthOrReject.mockImplementation(async ({ isMatch, targets }) => { + for (const target of targets) { + if (await isMatch(target)) { + return target; + } + } + return null; + }); + verifyGoogleChatRequest + .mockResolvedValueOnce({ ok: false, reason: "unexpected add-on principal: 111" }) + .mockResolvedValueOnce({ ok: true }); + const { processEvent, res } = await runWebhookHandler(); + + expect(logA).not.toHaveBeenCalled(); + expect(logB).not.toHaveBeenCalled(); + expect(processEvent).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + account: expect.objectContaining({ accountId: "acct-b" }), + }), + ); + expect(res.statusCode).toBe(200); + }); + it("rejects missing add-on bearer tokens before dispatch", async () => { + const logFn = vi.fn(); installSimplePipeline([ { account: { accountId: "default", config: { appPrincipal: "chat-app" }, }, - runtime: { error: vi.fn() }, + runtime: { log: logFn, error: vi.fn() }, }, ]); readJsonWebhookBodyOrReject.mockResolvedValue({ @@ -181,7 +386,61 @@ describe("googlechat monitor webhook", () => { const { processEvent, res } = await runWebhookHandler(); expect(processEvent).not.toHaveBeenCalled(); + expect(logFn).toHaveBeenCalledWith(expect.stringContaining("default")); + expect(logFn).toHaveBeenCalledWith(expect.stringContaining("missing token")); expect(res.statusCode).toBe(401); expect(res.body).toBe("unauthorized"); }); }); + +describe("warnAppPrincipalMisconfiguration", () => { + it("warns when appPrincipal is missing for app-url audience", () => { + const log = vi.fn(); + warnAppPrincipalMisconfiguration({ + accountId: "acct-missing", + audienceType: "app-url", + appPrincipal: undefined, + log, + }); + expect(log).toHaveBeenCalledOnce(); + expect(log).toHaveBeenCalledWith(expect.stringContaining("acct-missing")); + expect(log).toHaveBeenCalledWith(expect.stringContaining("appPrincipal is missing")); + expect(log).toHaveBeenCalledWith(expect.stringContaining("numeric OAuth 2.0 client ID")); + }); + + it("warns when appPrincipal contains @ for app-url audience", () => { + const log = vi.fn(); + warnAppPrincipalMisconfiguration({ + accountId: "acct-email", + audienceType: "app-url", + appPrincipal: "bot@example.iam.gserviceaccount.com", + log, + }); + expect(log).toHaveBeenCalledOnce(); + expect(log).toHaveBeenCalledWith(expect.stringContaining("acct-email")); + expect(log).toHaveBeenCalledWith(expect.stringContaining("looks like an email")); + expect(log).toHaveBeenCalledWith(expect.stringContaining("numeric OAuth 2.0 client ID")); + }); + + it("does not warn for valid numeric appPrincipal with app-url audience", () => { + const log = vi.fn(); + warnAppPrincipalMisconfiguration({ + accountId: "acct-ok", + audienceType: "app-url", + appPrincipal: "123456789012345678901", + log, + }); + expect(log).not.toHaveBeenCalled(); + }); + + it("does not warn for project-number audience even with missing appPrincipal", () => { + const log = vi.fn(); + warnAppPrincipalMisconfiguration({ + accountId: "acct-pn", + audienceType: "project-number", + appPrincipal: undefined, + log, + }); + expect(log).not.toHaveBeenCalled(); + }); +}); diff --git a/extensions/googlechat/src/monitor-webhook.ts b/extensions/googlechat/src/monitor-webhook.ts index cf382d29672..9de1487432f 100644 --- a/extensions/googlechat/src/monitor-webhook.ts +++ b/extensions/googlechat/src/monitor-webhook.ts @@ -101,17 +101,84 @@ function parseGoogleChatInboundPayload( return { ok: true, event, addOnBearerToken }; } -async function isAuthorizedGoogleChatTarget( +type GoogleChatWebhookAuthRejection = { + target: WebhookTarget; + reason: string; +}; + +async function verifyGoogleChatTargetAuth( target: WebhookTarget, bearer: string, -): Promise { +): Promise<{ ok: true } | { ok: false; reason: string }> { const verification = await verifyGoogleChatRequest({ bearer, audienceType: target.audienceType, audience: target.audience, expectedAddOnPrincipal: target.account.config.appPrincipal, }); - return verification.ok; + return verification.ok ? { ok: true } : { ok: false, reason: verification.reason ?? "unknown" }; +} + +function logGoogleChatWebhookAuthRejections(rejections: GoogleChatWebhookAuthRejection[]): void { + for (const rejection of rejections) { + rejection.target.runtime.log?.( + `[${rejection.target.account.accountId}] Google Chat webhook auth rejected: ${rejection.reason}`, + ); + } +} + +function logGoogleChatWebhookAuthRejectedForTargets( + targets: readonly WebhookTarget[], + reason: string, +): void { + logGoogleChatWebhookAuthRejections(targets.map((target) => ({ target, reason }))); +} + +async function resolveGoogleChatWebhookTargetWithAuthOrReject(params: { + targets: readonly WebhookTarget[]; + res: ServerResponse; + bearer: string; +}): Promise { + const rejections: GoogleChatWebhookAuthRejection[] = []; + let verifiedTargetCount = 0; + const selectedTarget = await resolveWebhookTargetWithAuthOrReject({ + targets: params.targets, + res: params.res, + isMatch: async (target) => { + const verification = await verifyGoogleChatTargetAuth(target, params.bearer); + if (verification.ok) { + verifiedTargetCount += 1; + return true; + } + rejections.push({ target, reason: verification.reason }); + return false; + }, + }); + if (!selectedTarget && verifiedTargetCount === 0) { + logGoogleChatWebhookAuthRejections(rejections); + } + return selectedTarget; +} + +export function warnAppPrincipalMisconfiguration(params: { + accountId: string; + audienceType?: string; + appPrincipal?: string | null; + log?: (message: string) => void; +}): void { + if (params.audienceType !== "app-url") { + return; + } + const principal = params.appPrincipal?.trim(); + if (!principal) { + params.log?.( + `[${params.accountId}] appPrincipal is missing for audienceType "app-url"; add-on token verification will fail. Set appPrincipal to the numeric OAuth 2.0 client ID (uniqueId, 21 digits), not an email.`, + ); + } else if (principal.includes("@")) { + params.log?.( + `[${params.accountId}] appPrincipal "${principal}" looks like an email address. Set appPrincipal to the numeric OAuth 2.0 client ID (uniqueId, 21 digits), not an email.`, + ); + } } export function createGoogleChatWebhookRequestHandler(params: { @@ -156,10 +223,10 @@ export function createGoogleChatWebhookRequestHandler(params: { }; if (headerBearer) { - selectedTarget = await resolveWebhookTargetWithAuthOrReject({ + selectedTarget = await resolveGoogleChatWebhookTargetWithAuthOrReject({ targets, res, - isMatch: (target) => isAuthorizedGoogleChatTarget(target, headerBearer), + bearer: headerBearer, }); if (!selectedTarget) { return true; @@ -178,15 +245,16 @@ export function createGoogleChatWebhookRequestHandler(params: { parsedEvent = parsed.event; if (!parsed.addOnBearerToken) { + logGoogleChatWebhookAuthRejectedForTargets(targets, "missing token"); res.statusCode = 401; res.end("unauthorized"); return true; } - selectedTarget = await resolveWebhookTargetWithAuthOrReject({ + selectedTarget = await resolveGoogleChatWebhookTargetWithAuthOrReject({ targets, res, - isMatch: (target) => isAuthorizedGoogleChatTarget(target, parsed.addOnBearerToken), + bearer: parsed.addOnBearerToken, }); if (!selectedTarget) { return true; diff --git a/extensions/googlechat/src/monitor.ts b/extensions/googlechat/src/monitor.ts index 0fc76f4e187..762780182d5 100644 --- a/extensions/googlechat/src/monitor.ts +++ b/extensions/googlechat/src/monitor.ts @@ -30,6 +30,7 @@ import type { GoogleChatRuntimeEnv, WebhookTarget, } from "./monitor-types.js"; +import { warnAppPrincipalMisconfiguration } from "./monitor-webhook.js"; import { getGoogleChatRuntime } from "./runtime.js"; import type { GoogleChatAttachment, GoogleChatEvent } from "./types.js"; export type { GoogleChatMonitorOptions, GoogleChatRuntimeEnv } from "./monitor-types.js"; @@ -477,6 +478,13 @@ export function monitorGoogleChatProvider(options: GoogleChatMonitorOptions): () const audience = options.account.config.audience?.trim(); const mediaMaxMb = options.account.config.mediaMaxMb ?? 20; + warnAppPrincipalMisconfiguration({ + accountId: options.account.accountId, + audienceType, + appPrincipal: options.account.config.appPrincipal, + log: options.runtime.log, + }); + const unregisterTarget = registerGoogleChatWebhookTarget({ account: options.account, config: options.config, diff --git a/extensions/googlechat/src/monitor.webhook-routing.test.ts b/extensions/googlechat/src/monitor.webhook-routing.test.ts index 44c839ca9de..aefe5750532 100644 --- a/extensions/googlechat/src/monitor.webhook-routing.test.ts +++ b/extensions/googlechat/src/monitor.webhook-routing.test.ts @@ -88,13 +88,15 @@ const baseAccount = (accountId: string) => function registerTwoTargets() { const sinkA = vi.fn(); const sinkB = vi.fn(); + const logA = vi.fn(); + const logB = vi.fn(); const core = {} as PluginRuntime; const config = {} as OpenClawConfig; const unregisterA = registerGoogleChatWebhookTarget({ account: baseAccount("A"), config, - runtime: {}, + runtime: { log: logA }, core, path: "/googlechat", statusSink: sinkA, @@ -103,7 +105,7 @@ function registerTwoTargets() { const unregisterB = registerGoogleChatWebhookTarget({ account: baseAccount("B"), config, - runtime: {}, + runtime: { log: logB }, core, path: "/googlechat", statusSink: sinkB, @@ -111,6 +113,8 @@ function registerTwoTargets() { }); return { + logA, + logB, sinkA, sinkB, unregister: () => { @@ -177,7 +181,7 @@ describe("Google Chat webhook routing", () => { it("routes to the single verified target when earlier targets fail verification", async () => { mockSecondVerifierSuccess(); - const { sinkA, sinkB, unregister } = registerTwoTargets(); + const { logA, logB, sinkA, sinkB, unregister } = registerTwoTargets(); try { await expectVerifiedRoute({ @@ -190,6 +194,8 @@ describe("Google Chat webhook routing", () => { sinkB, expectedSink: "B", }); + expect(logA).not.toHaveBeenCalled(); + expect(logB).not.toHaveBeenCalled(); } finally { unregister(); }