Conversation
fix: Enhance Zapier CLI app
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![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: eda198064f
ℹ️ 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".
| * JWTs. Configure the Zapier OAuth client's accessTokenTtl for a | ||
| * long TTL (e.g. 365 days) to avoid frequent reconnections. | ||
| */ | ||
| autoRefresh: false |
There was a problem hiding this comment.
Setting autoRefresh to false here removes Zapier’s ability to renew access tokens, but the new /api/integration/ever-gauzy/oauth/token exchange does not return a refresh_token (see OAuthAppController.token) and access tokens are still bounded by global JWT_TOKEN_EXPIRATION_TIME in AuthService.exchangeOAuthAppAuthorizationCode (default is 1 day). As a result, established Zap connections will expire and require manual reconnection instead of self-recovering, which is a production regression from the previous refresh-capable flow.
Useful? React with 👍 / 👎.
| integrationId: integration.id, | ||
| name: IntegrationEnum.ZAPIER | ||
| }; | ||
| } catch { |
There was a problem hiding this comment.
This empty catch swallows every failure from resolveIntegrationFromBearerToken, not just the intended “integration not created yet” case. That means errors like BadRequestException from tenant/org ambiguity (multiple Zapier integrations) are masked, and the method falls back to verifyJwtToken, so /auth/test can report success while webhook subscription endpoints later fail on the same token resolution path. Restrict the fallback to explicit not-found conditions so auth test and runtime behavior stay consistent.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eda1980. Configure here.
| }; | ||
| } catch { | ||
| // No IntegrationTenant yet — fall through to JWT-only verification | ||
| } |
There was a problem hiding this comment.
Catch-all silently swallows configuration errors from resolution
Medium Severity
The bare catch blocks around resolveIntegrationFromBearerToken in testAuth and getConnectionInfo swallow all exceptions — including BadRequestException ("Multiple Zapier integrations found…") and infrastructure errors — then fall through to JWT-only verification. Because verifyJwtToken can succeed independently, these endpoints return authenticated: true even when the integration is misconfigured (ambiguous multi-org setup) or the database is unreachable. The resolveIntegrationFromBearerToken method itself discriminates errors carefully, but the controller's unfiltered catch undoes that. Only NotFoundException and UnauthorizedException are appropriate to catch here; other errors like BadRequestException indicate real configuration problems that the user needs to see.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit eda1980. Configure here.
Greptile SummaryThis PR enhances the Zapier integration by adding JWT-based bearer token resolution alongside the existing opaque token path, switches the Zapier CLI app to the multi-app OAuth endpoints ( Two P1 issues need attention before merge:
Confidence Score: 3/5Not safe to merge as-is; two P1 bugs in error handling paths affect HTTP status codes and can mask DB failures during auth. Two P1 issues were found: deleteWebhook incorrectly converts auth errors to 500 (incorrect status codes returned to Zapier platform), and bare catch{} blocks in testAuth/getConnectionInfo can silently mask DB failures and return authenticated:true incorrectly. Both are on the changed code paths and require fixes before merge. zapier-webhook.controller.ts (deleteWebhook error handling) and zapier-authorization.controller.ts (bare catch blocks in testAuth and getConnectionInfo) Important Files Changed
Sequence DiagramsequenceDiagram
participant Z as Zapier CLI App
participant EG as multi-app OAuth provider
participant ZA as /zapier/auth/test and /zapier/auth/me
participant WH as /zapier/webhooks
participant SVC as ZapierService
participant DB as Database
Note over Z,EG: OAuth flow - new endpoints
Z->>EG: GET /authorize with state param
EG-->>Z: Redirect with authorization code
Z->>EG: POST /token with code and credentials
EG-->>Z: JWT access_token (long-lived, autoRefresh disabled)
Note over Z,ZA: Auth test - supports opaque and JWT
Z->>ZA: GET /auth/test with Bearer token
ZA->>SVC: resolveIntegrationFromBearerToken(token)
SVC->>DB: findOne by opaque token value
alt opaque token found
DB-->>SVC: IntegrationTenant
SVC-->>ZA: integration
ZA-->>Z: authenticated true with integrationId
else NotFoundException - try JWT path
SVC->>SVC: verifyJwtToken(token)
SVC->>DB: findIntegrationByTenantId
DB-->>SVC: IntegrationTenant
ZA-->>Z: authenticated true
else DB failure - bare catch swallows error
ZA->>SVC: verifyJwtToken(token)
ZA-->>Z: authenticated true - false positive risk
end
Note over Z,WH: Webhook subscribe
Z->>WH: POST /webhooks with Bearer token
WH->>SVC: resolveIntegrationFromBearerToken(token)
SVC-->>WH: integration
WH-->>Z: 201 subscription created
Note over Z,WH: Webhook unsubscribe - bug
Z->>WH: DELETE /webhooks/:id with Bearer token
WH->>SVC: resolveIntegrationFromBearerToken(token)
SVC-->>WH: throws NotFoundException or UnauthorizedException
WH-->>Z: 500 InternalServerError - should be 401 or 403
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Try opaque token first (backward compat) — resolves via IntegrationTenant | ||
| try { | ||
| const integration = await this.zapierService.resolveIntegrationFromBearerToken(token); | ||
| return { | ||
| authenticated: true, | ||
| integrationId: integration.id, | ||
| name: IntegrationEnum.ZAPIER | ||
| }; | ||
| } catch { | ||
| // No IntegrationTenant yet — fall through to JWT-only verification |
There was a problem hiding this comment.
catch {} silently swallows infrastructure errors
Both testAuth (lines 249–258) and getConnectionInfo (lines 296–305) use a bare catch {} that swallows ALL exceptions from resolveIntegrationFromBearerToken, including database failures. resolveIntegrationFromBearerToken already re-throws non-NotFoundException infrastructure errors immediately, but this inner catch {} discards them and falls through to JWT verification. On a DB failure with a valid JWT token, the endpoint returns authenticated: true even though the opaque-token path could not be checked — silent false positive.
The catch should be narrowed to only the "not found" case:
} catch (e) {
if (!(e instanceof NotFoundException)) {
throw e; // Re-throw infrastructure errors
}
// NotFoundException — no opaque-token match, fall through to JWT
}The same fix is needed in getConnectionInfo at the equivalent block.
| } catch (error: any) { | ||
| // Let known HTTP exceptions (auth, not-found, bad-request) surface with their status code. | ||
| // Only re-throw unexpected errors (DB failures, etc.) as-is. | ||
| if (error instanceof HttpException) { | ||
| this.logger.debug('resolveIntegrationFromBearerToken: no integration found — %s', error?.message); | ||
| throw error; | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
The if/else in the JWT catch block logs differently for HttpException but both branches unconditionally throw error. The comment implies they should behave differently for "infrastructure errors", but the code is identical for both paths. This is misleading; the if can be removed entirely.
| } catch (error: any) { | |
| // Let known HTTP exceptions (auth, not-found, bad-request) surface with their status code. | |
| // Only re-throw unexpected errors (DB failures, etc.) as-is. | |
| if (error instanceof HttpException) { | |
| this.logger.debug('resolveIntegrationFromBearerToken: no integration found — %s', error?.message); | |
| throw error; | |
| } | |
| throw error; | |
| } | |
| } catch (error: any) { | |
| this.logger.debug('resolveIntegrationFromBearerToken: no integration found — %s', error?.message); | |
| throw error; | |
| } |
| verifyJwtToken(token: string): { id: string; tenantId: string; organizationId?: string } { | ||
| try { | ||
| const decoded = verify(token, environment.JWT_SECRET!); | ||
| if (typeof decoded !== 'object' || !decoded || !('tenantId' in decoded)) { | ||
| throw new Error('Invalid JWT payload structure'); | ||
| } | ||
| return decoded as { id: string; tenantId: string; organizationId?: string }; | ||
| } catch { | ||
| throw new UnauthorizedException('Invalid or expired access token'); | ||
| } |
There was a problem hiding this comment.
The non-null assertion on environment.JWT_SECRET suppresses the TypeScript error, but if the secret is unset at runtime, jsonwebtoken throws internally and the bare catch {} re-throws it as UnauthorizedException('Invalid or expired access token'), making a server misconfiguration indistinguishable from a bad token in both HTTP responses and logs. Consider guarding for the missing secret before calling verify and surfacing it as InternalServerErrorException instead.
2 participants
fix: Enhance Zapier CLI app
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Note
Medium Risk
Touches authentication/token verification paths and changes how public endpoints resolve integrations, so misconfiguration of
JWT_SECRETor tenant/org resolution could break Zapier connections. Dependency bumps and Zapier app OAuth endpoint changes may also affect existing installs/refresh behavior.Overview
Improves the Zapier integration to work with multi-app OAuth by accepting either legacy opaque tokens or signed JWT access tokens across Zapier’s
auth/test,auth/me, and webhook subscription endpoints.This introduces centralized bearer-token resolution in
ZapierService(JWT verification viaenvironment.JWT_SECRET, tenant/org-based integration lookup with a fail-closed ambiguity check) and adjusts token setting lookups to bypass tenant scoping for@Public()endpoints.Separately updates the Zapier CLI app to use the
/api/integration/ever-gauzy/oauth/*authorization/token endpoints, disables auto-refresh, adds a globalcleanInputDataflag, and bumps Zapier platform dependencies to18.4.0(plusskipLibCheckfor the Zapier app TS build).Reviewed by Cursor Bugbot for commit eda1980. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Add JWT-based auth to the Zapier integration and switch the CLI app to the multi-app OAuth flow, while keeping legacy opaque tokens working. This improves connection reliability and unifies token handling across auth tests and webhooks.
New Features
/api/integration/ever-gauzy/oauth/{authorize,token}and disable refresh (use long-lived JWTs).zapier-platform-coreandzapier-platform-clito18.4.0; addskipLibCheckand set app flagcleanInputData: false.Migration
JWT_SECRETis set for the API so JWTs can be verified.Written for commit eda1980. Summary will update on new commits.