Conversation
|
|
||
| ### Prerequisites | ||
|
|
||
| - [Node.js](https://nodejs.org/) (v20 or higher recommended) |
There was a problem hiding this comment.
src/db/index.ts imports { DatabaseSync } from 'node:sqlite', which was introduced in Node.js v22.5.0 and does not exist in Node 20 (running on Node 20 fails immediately with ERR_UNKNOWN_BUILTIN_MODULE: node:sqlite).
Please update this prerequisite to Node.js v22.5.0 or higher (and consider adding "engines": { "node": ">=22.5.0" } to package.json).
Also, package.json runs node --env-file .env and .gitignore has !.env.example, but there is no .env.example in the repo and README.md doesn't document the required environment variables (GOOGLE_OAUTH_CLIENT_ID, GOOGLE_OAUTH_CLIENT_SECRET, GOOGLE_ADDON_SERVICE_ACCOUNT_EMAIL, GOOGLE_ADDON_CLIENT_ID, COOKIE_SECRET, PUBLIC_BASE_URL). Could we add a .env.example file and an environment setup section here?
| "start": "node --env-file .env dist/server.js", | ||
| "dev": "tsx watch --env-file .env src/server.ts", |
There was a problem hiding this comment.
Node's --env-file .env flag exits with a fatal error (ENOENT: .env not found) if .env does not exist yet. Please include a .env.example template in studio/incoming-webhook-starter/ (or use --env-file-if-exists=.env in Node 22.9+) and regenerate package-lock.json (package-lock.json currently still lists better-sqlite3 and @types/better-sqlite3 in the root package entry even though they were removed from package.json).
| // Fallback to sub claim on caller token | ||
| if (!systemPayload.sub) { | ||
| throw new StudioAuthError('Missing sub claim in token payload.'); | ||
| } | ||
|
|
||
| return systemPayload.sub; | ||
| } |
There was a problem hiding this comment.
Security / Auth bug: In production, systemPayload is the verified Bearer token from the Google Add-on Service Account (GOOGLE_ADDON_SERVICE_ACCOUNT_EMAIL), so systemPayload.sub is the service account's subject ID, not the end user's ID.
If authorizationEventObject.userIdToken is missing in production, falling back to systemPayload.sub causes all requests without userIdToken to share the service account's sub as their userId. In production (isProd), we should require userIdFromAddon and throw StudioAuthError if it is missing (only falling back to systemPayload.sub in local/test environments).
| /** | ||
| * Backward-compatible helper for verifying user token explicitly. | ||
| */ | ||
| export async function verifyAddonUserIdToken( | ||
| event: RootEventObject | ||
| ): Promise<string> { | ||
| const rawToken = | ||
| event.authorizationEventObject?.userIdToken || | ||
| event.authorizationEventObject?.systemIdToken; | ||
|
|
||
| if (!rawToken) { | ||
| throw new StudioAuthError('Missing user ID token in authorizationEventObject.'); | ||
| } | ||
|
|
||
| if (customTokenVerifier) { | ||
| const verified = await customTokenVerifier(rawToken); | ||
| return verified.userId; | ||
| } | ||
|
|
There was a problem hiding this comment.
Two issues in verifyAddonUserIdToken:
- When
userIdTokenis absent, line 167 falls back tosystemIdTokenand verifies it againstexpectedAudience = process.env.GOOGLE_ADDON_CLIENT_ID(line 178). In production,systemIdTokenhas the HTTP endpoint URL as itsaudclaim (notGOOGLE_ADDON_CLIENT_ID), so that fallback will fail audience verification. /studio/on-manage(src/studio/routes.ts:326-327) calls bothverifySystemIdToken(req, event)(which already callsauthenticateStudioRequest) andverifyAddonUserIdToken(event). Having/studio/on-managecallconst userId = await authenticateStudioRequest(req, event)directly (like the other 4 routes) avoids verifying the tokens twice and eliminates the need forverifySystemIdToken/verifyAddonUserIdTokenand theas TokenPayloadcast on line 156.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| code_challenge_method: 'S256' as any, |
There was a problem hiding this comment.
google-auth-library exports the CodeChallengeMethod enum, so you can import CodeChallengeMethod from 'google-auth-library' and pass code_challenge_method: CodeChallengeMethod.S256 directly without as any or the eslint-disable comment.
| modifyOperations.push({ | ||
| replaceSection: buildEndpointSection({ | ||
| baseUrl, | ||
| instanceId, | ||
| requireApiKey, | ||
| }), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug on partial card update: When a workflow is already active, /studio/on-config (line 127) passes triggerId: activeSubscription?.triggerId so the card displays https://.../webhook/{triggerId}.
However, both /studio/on-toggle-api-key (here) and /studio/on-add-secret (lines 244–248) call buildEndpointSection without triggerId. If a user opens an active workflow and clicks "Add Secret" or toggles the switch, replaceSection overwrites the active /webhook/{triggerId} URL with /webhook/{instanceId}!
Please look up activeSubscription = getActiveSubscriptionByInstanceId(instanceId, userId) in both handlers and pass triggerId: activeSubscription?.triggerId to buildEndpointSection.
| const notifyUri = | ||
| triggerCreation.notifyUri ?? | ||
| `https://workspacestudio.googleapis.com/v1/triggers/${triggerId}:fire`; |
There was a problem hiding this comment.
Defense-in-depth (SSRF / token exfiltration): notifyUri is persisted from the request payload and later called in POST /webhook/:identifier with Authorization: Bearer ${accessToken} (containing the user's workspace.studio.trigger token). Please validate that notifyUri is a valid https:// URL with hostname workspacestudio.googleapis.com (or ending in .googleapis.com) before storing or calling it.
| let matched = false; | ||
| for (const sec of storedSecrets) { | ||
| if (constantTimeCompare(providedHash, sec.secretHash)) { | ||
| matched = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: You can simplify this loop using Array.prototype.some():
const matched = storedSecrets.some((sec) =>
constantTimeCompare(providedHash, sec.secretHash)
);| let upstreamJson: unknown = null; | ||
| if (upstreamText) { | ||
| try { | ||
| upstreamJson = JSON.parse(upstreamText); | ||
| } catch { | ||
| // Leave as string if not JSON | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: The catch block on line 212 says // Leave as string if not JSON, but leaves upstreamJson as null, causing line 246 (studioResponse: upstreamJson ?? {}) to silently drop non-JSON upstream response bodies on 200 OK. Assign upstreamJson = upstreamText; in the catch block so non-JSON text is preserved.
| await request(app) | ||
| .post('/webhook/trig-abc-123') | ||
| .set('X-Webhook-Secret', 'whsec_testsecret999') | ||
| .send('{"event":"third"}'); | ||
| expect(refreshCallCount).toBe(1); // Still 1! |
There was a problem hiding this comment.
Two test hygiene items:
- Here on lines 305–309, the third
POST /webhook/trig-abc-123call does not assert the HTTP response status (expect(res.status).toBe(200)). BecauserefreshCallCountis already1before line 305 executes,expect(refreshCallCount).toBe(1)passes vacuously even if the third request fails with a 4xx/5xx error. - In
beforeEach(lines 16–31), resetsetTokenExchangerForTesting,setTokenRefresherForTesting, andsetStudioFetchForTestingso test doubles mutated inside individual tests cannot leak across test cases.
2 participants
No description provided.