P2 Zapier: public app contract#682
Conversation
Add the Zapier manifest, auth test, trigger polling, action wrappers, webhook subscription registration, OAuth bearer validation, and setup docs for #589. Constraint: Full reliable webhook delivery worker is not present in this branch, so this slice stores signed Zapier webhook subscriptions and documents the delivery handoff. Tested: make check; npx vitest run tests/zapier.test.ts tests/zapier-routes.test.ts; npx vitest run tests/account-security-route.test.ts tests/workspace-invite-route.test.ts tests/team-cycles-cycleId-route.test.ts tests/project-templates-route.test.ts; npx vitest run tests/auth.test.ts; npx vitest run tests/security-settings-view.test.tsx; npx vitest run tests/agent-logic.test.tsx Not-tested: make test times out under full-suite concurrency in unrelated suites that pass individually; make test-e2e is blocked by local Postgres password authentication failure for user postgres. Confidence: medium Co-authored-by: OmX <omx@oh-my-codex.dev>
|
Controller intake: blocked before QA/merge. Current blocker:
Required fix:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e978b4fc01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (url.protocol !== "https:") { | ||
| throw new ZapierActionError("targetUrl must use HTTPS.", { | ||
| field: "targetUrl", |
There was a problem hiding this comment.
This new subscription path only verifies https: and skips the existing validateWebhookUrl guard used by the normal webhook API, so a direct Zapier API call can persist targets such as https://127.0.0.1/..., private/link-local hosts, or URLs with fragments. When webhook delivery runs server-side, that becomes an SSRF/internal-callback path; please reuse the shared webhook URL validator before inserting the subscription.
Useful? React with 👍 / 👎.
| if (!zapierSessionHasScope(context.session, "webhooks:write")) { | ||
| throw new ZapierActionError("Zapier requires the webhooks:write scope.", { | ||
| status: 403, | ||
| code: "insufficient_scope", | ||
| }); |
There was a problem hiding this comment.
Checking only webhooks:write lets any workspace member who can create an API key, or authorize an OAuth app with that scope, create workspace-wide webhook subscriptions; the existing /api/workspaces/current/api webhook create/delete path gates this on canManageWorkspaceApi because these hooks can exfiltrate workspace issue activity. Please carry the member role through OAuth sessions and require owner/admin before subscribe/unsubscribe.
Useful? React with 👍 / 👎.
| redirect_uri: bundle.inputData?.redirect_uri, | ||
| }, | ||
| }), | ||
| autoRefresh: false, |
There was a problem hiding this comment.
The OAuth token endpoint returns one-hour access tokens with a refresh token, but this Zapier app sets autoRefresh: false and defines no refreshAccessToken, so OAuth-connected Zaps will keep sending the expired lin_oauth_at_... token after about an hour and every trigger/action will start receiving 401s. Zapier's OAuth docs describe using refreshAccessToken/autoRefresh for expiring tokens: https://docs.zapier.com/integrations/build-cli/overview.
Useful? React with 👍 / 👎.
| trigger === "new_issue" ? issue.createdAt : issue.updatedAt; | ||
| const rows = await db | ||
| .select({ | ||
| id: issue.id, |
There was a problem hiding this comment.
For the updated_issue polling path this returns the stable issue id as Zapier's default primary key, so once a Zap's initial poll has seen an issue, later updates to that same issue are deduped away and never trigger. Zapier's deduplication docs recommend making updated-item ids include the update timestamp, or defining primary output fields: https://docs.zapier.com/integrations/build/deduplication.
Useful? React with 👍 / 👎.
|
Closing in favor of regeneration on the current architecture. This PR was authored against the pre-monorepo-split layout ( Review also flagged blocking security/correctness issues (varies per PR: SSRF, OAuth CSRF nonce never verified, full-table workspace scans, issue-number races, Re-generate from the spec in #589 on current |
1 participant
Summary
apps/zapierpublic Zapier Platform app contract for OAuth, polling/rest-hook triggers, actions, and structured error handling/api/zapierroutes through proxy so bearer/API key auth reaches route handlersValidation
make checkmake test(passed on rerun; first run exposed a transienttests/initiatives-view.test.tsxisolation/order failure that passed alone)TZ=Asia/Seoul npx vitest run tests/zapier-app.test.ts tests/zapier.test.ts tests/zapier-routes.test.ts tests/middleware.test.tsnode .scratch/zapier-manifest-smoke.mjsagainst local dev server on port 3015Notes
make test-e2eis environment-blocked because the Playwright Chromium binary is missing from the shared cache.Closes #589