fix: tenant auth for Nango actions + invitation error handling#2785
fix: tenant auth for Nango actions + invitation error handling#2785omar-inkeep wants to merge 2 commits intomainfrom
Conversation
…rors - Nango server actions (list/update/delete) now verify tenant membership via backend API before executing privileged operations with shared secret - getPendingInvitations returns discriminated result type instead of silently swallowing errors as empty arrays - /no-organization page shows error state with retry on fetch failure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
TL;DR — Adds tenant-scoped authorization checks to Nango integration actions (delete, list, update credentials) that previously relied only on the shared Nango secret key, and replaces silent error swallowing in the invitation-fetching flow with a discriminated result type and a user-facing error state. Key changes
Summary ｜ 6 files ｜ 2 commits ｜ base: Tenant authorization for Nango integration actions
Invitation error handling with discriminated result type
The
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good direction — the verifyTenantAccess calls and the discriminated result type for invitations are both solid improvements. One security concern with assertKeyBelongsToTenant needs attention before merge.
| function assertKeyBelongsToTenant(uniqueKey: string, tenantId: string): void { | ||
| if (!uniqueKey.includes(`-${tenantId}`)) { | ||
| throw new NangoError(`Integration key '${uniqueKey}' does not belong to tenant '${tenantId}'`); | ||
| } |
There was a problem hiding this comment.
Security: includes() is too loose for ownership assertion. A tenant ID that is a substring of another tenant's ID will pass this check. For example, if tenant abc calls this with a key belonging to tenant abcdef, the check "github-abcdef-x8f2kq".includes("-abc") returns true.
The listing function at line 232 already uses a stricter startsWith(prefix) check with the provider name. This function should be at least as strict — ideally using an exact segment match. Since the key format is {provider}-{tenantId} or {provider}-{tenantId}-{suffix}, the safest approach that doesn't require the provider name is:
function assertKeyBelongsToTenant(uniqueKey: string, tenantId: string): void {
const tenantSegment = `-${tenantId}-`;
const tenantSuffix = `-${tenantId}`;
// Key ends with tenantId (no suffix) OR contains tenantId followed by a delimiter
if (!uniqueKey.endsWith(tenantSuffix) && !uniqueKey.includes(tenantSegment)) {
throw new NangoError(`Integration key '${uniqueKey}' does not belong to tenant '${tenantId}'`);
}
}This handles both key formats ({provider}-{tenantId} and {provider}-{tenantId}-{suffix}) without the substring collision.
| * Throws if the user is not authenticated or not a member of the tenant. | ||
| */ | ||
| async function verifyTenantAccess(tenantId: string): Promise<void> { | ||
| await makeManagementApiRequest(`tenants/${tenantId}/projects?limit=1`); |
There was a problem hiding this comment.
This is a creative approach to verify tenant membership — piggyback on an authenticated management API call. However, it's worth noting this is a read operation (GET projects?limit=1) used as an authorization gate. If this endpoint ever changes behavior (e.g., returns 200 for unauthenticated requests, or is cached at a CDN layer), the gate silently opens.
Consider adding a brief inline note that this is intentionally used as an auth gate so future readers don't accidentally break the invariant, or consider using a more explicit endpoint if one exists (e.g., a membership check).
agents-manage-ui/src/app/page.tsx
Outdated
| } | ||
|
|
||
| // No invitations either - show no-org page | ||
| // No invitations (or failed to fetch) - show no-org page |
There was a problem hiding this comment.
Nit: the (or failed to fetch) parenthetical in this comment is accurate but could mislead — a failure to fetch invitations silently redirects to /no-organization, where the user will see the error state and can retry. This is fine behavior, but the comment should clarify the intent rather than just the condition: e.g., // No invitations or fetch failed — the no-org page handles both cases.
| } else { | ||
| setInviteError(result.error); | ||
| } | ||
| setLoadingInvites(false); |
There was a problem hiding this comment.
Minor: When retry succeeds after a prior error, inviteError is cleared and pendingInvites is set — but if the result has 0 invitations, the user transitions from the error state to the "no organization" fallback at the bottom. This is correct behavior, just noting the flow is sound.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
This PR adds important security controls for Nango tenant isolation and improves error handling for invitation fetching. The changes are well-structured but have a few issues that should be addressed before merging.
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
nango.ts:35Weak substring matching in tenant ownership assertion allows potential bypass - 🟠 Major:
no-organization/page.tsx:80Retry button missing disabled state causes concurrent requests
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
invitations.ts:29Missing server-side error logging for observability - 🟡 Minor:
no-organization/page.tsx:46Stale pendingInvites state not cleared on error
💭 Consider (2) 💭
💭 1) no-organization/page.tsx:69,98 Decorative icons missing aria-hidden
Issue: The AlertTriangle and Mail icons are decorative (content conveyed via adjacent text) but lack aria-hidden="true".
Why: Screen readers will announce the icon names before the headings, adding redundant information.
Fix: Add aria-hidden="true" to both icons.
💭 2) page.tsx:82-93 Silent redirect on invitation fetch failure
Issue: When getPendingInvitations fails in page.tsx, users are redirected to /no-organization without context that the check failed (comment on line 90 confirms: "No invitations (or failed to fetch)").
Why: The no-organization page now properly handles errors with retry UI, so this is acceptable UX. However, users experience a double-request pattern (fail on page.tsx → redirect → retry on no-organization).
Fix: Consider passing a query param like ?fetchFailed=true to provide immediate context, but this is a minor enhancement.
🧹 While You're Here (2) 🧹
These pre-existing issues were identified during review and are worth addressing alongside this PR:
🧹 1) nango.ts:173 fetchNangoIntegration lacks tenant authorization
Issue: This function fetches integration details (including masked credentials) using the shared Nango secret key without tenant verification. A caller who knows another tenant's integration key could retrieve credential metadata.
Why: This PR establishes a pattern of tenant auth for Nango operations. Extending it to fetchNangoIntegration would complete the security boundary.
Fix: Add tenantId parameter and call verifyTenantAccess(tenantId) + assertKeyBelongsToTenant(uniqueKey, tenantId).
🧹 2) nango.ts:415 createProviderConnectSession lacks tenant authorization
Issue: This function creates integrations and connect sessions without verifying tenant membership. The organizationId parameter is caller-supplied and not validated against authenticated context.
Why: Similar to above - completing the tenant auth pattern across all Nango operations would strengthen the security model.
Fix: Add required tenantId parameter and validate against authenticated session.
🚫 REQUEST CHANGES
Summary: The core security improvement (tenant auth for Nango delete/update/list) is good, but the substring-based ownership check at line 35 should use the same startsWith pattern as listNangoProviderIntegrations for consistency and defense in depth. The retry button also needs a disabled state to prevent concurrent requests.
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
4 | 0 | 0 | 2 | 1 | 0 | 0 |
pr-review-errors |
3 | 0 | 1 | 0 | 2 | 0 | 0 |
pr-review-standards |
2 | 0 | 0 | 0 | 1 | 0 | 1 |
pr-review-frontend |
3 | 0 | 1 | 0 | 0 | 0 | 2 |
| Total | 12 | 0 | 2 | 2 | 4 | 0 | 3 |
Note: Standards reviewer finding on substring match deduplicated with security-iam reviewer.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
nango.ts:509 |
getNango*Metadata functions lack tenant auth | Pre-existing issue not modified by this PR, and lower risk than other functions since connectionId/providerConfigKey are typically loaded from authorized credential records |
no-organization/page.tsx:69 |
AlertTriangle icon missing aria-hidden | Moved to Consider section |
no-organization/page.tsx:98 |
Mail icon missing aria-hidden | Duplicate of above, same issue pattern |
| * Integration keys follow the convention: {provider}-{tenantId}-{suffix} | ||
| */ | ||
| function assertKeyBelongsToTenant(uniqueKey: string, tenantId: string): void { | ||
| if (!uniqueKey.includes(`-${tenantId}`)) { |
There was a problem hiding this comment.
🟠 MAJOR: Weak substring matching allows potential tenant boundary bypass
Issue: Using .includes(-${tenantId}) for tenant ownership validation is vulnerable to substring collisions. A key like provider-abc123-suffix would incorrectly pass validation for tenant abc since it contains -abc.
Why: While tenant IDs are likely UUIDs making exploitation unlikely in practice, this check is inconsistent with the stricter startsWith pattern used in listNangoProviderIntegrations at line 234. For defense in depth, the assertion should use the same validation approach.
Fix: Align with the existing pattern by checking the prefix:
| if (!uniqueKey.includes(`-${tenantId}`)) { | |
| function assertKeyBelongsToTenant(uniqueKey: string, tenantId: string): void { | |
| const expectedPrefix = `-${tenantId}`; | |
| const providerEndIndex = uniqueKey.indexOf('-'); | |
| const keyAfterProvider = providerEndIndex >= 0 ? uniqueKey.slice(providerEndIndex) : ''; | |
| if (!keyAfterProvider.startsWith(expectedPrefix)) { | |
| throw new NangoError(`Integration key '${uniqueKey}' does not belong to tenant '${tenantId}'`); | |
| } | |
| } |
Refs:
| </CardDescription> | ||
| </CardHeader> | ||
| <CardContent className="space-y-3"> | ||
| <Button onClick={checkInvitations} className="w-full"> |
There was a problem hiding this comment.
🟠 MAJOR: Retry button can trigger multiple concurrent requests
Issue: The "Try Again" button lacks a disabled state during the async operation. While checkInvitations sets loadingInvites(true), this state only controls the initial loading spinner (line 55), not this button.
Why: Rapid clicks will fire multiple concurrent API calls, potentially causing race conditions and unnecessary load.
Fix:
| <Button onClick={checkInvitations} className="w-full"> | |
| <Button onClick={checkInvitations} disabled={loadingInvites} className="w-full"> | |
| {loadingInvites ? 'Checking...' : 'Try Again'} | |
| </Button> |
| } catch { | ||
| return []; | ||
| return { success: true, invitations }; | ||
| } catch (error) { |
There was a problem hiding this comment.
🟡 Minor: Missing server-side error logging
Issue: Unlike verifyInvitation at line 67 which logs errors, this function returns errors without logging them.
Why: Without server-side logging, operators have no visibility into invitation fetch failure rates or causes, making it harder to diagnose patterns of failures.
Fix:
| } catch (error) { | |
| } catch (error) { | |
| console.error('[getPendingInvitations] Error:', error); | |
| const message = error instanceof ApiError ? error.message : 'Failed to load invitations'; | |
| return { success: false, error: message }; |
| })) | ||
| ); | ||
| } else { | ||
| setInviteError(result.error); |
There was a problem hiding this comment.
🟡 Minor: Stale state not cleared on error
Issue: When the invitation check fails, inviteError is set but pendingInvites is not cleared. If a user previously had a successful fetch, pendingInvites would contain stale data.
Why: While the conditional ordering at line 63 prevents display issues, maintaining inconsistent internal state can lead to subtle bugs during future refactors.
Fix:
| setInviteError(result.error); | |
| } else { | |
| setInviteError(result.error); | |
| setPendingInvites([]); // Clear stale state on error | |
| } |
Ito Test Report ❌19 test cases ran. 1 failed, 18 passed. Across 6 report shards covering 19 test cases, 18 passed and 1 failed, indicating strong overall stability for no-organization routing, invitation-state handling, provider credential CRUD, authorization redirects, cross-tenant tampering protections, and session invalidation behavior. The key issue is a Medium-severity defect in /no-organization where rapid repeated Try Again clicks can trigger overlapping invitation fetches that resolve out of order, allowing a stale failed response to overwrite a newer successful one and leave users incorrectly stuck in an error state (likely introduced by the PR due to missing request cancellation/freshness guards). ❌ Failed (1)
🟠 Rapid retry-clicking does not produce inconsistent invitation state
Relevant code:
const checkInvitations = useCallback(async () => {
if (!user?.email) {
setLoadingInvites(false);
return;
}
setLoadingInvites(true);
setInviteError(null);
const result = await getPendingInvitations(user.email);
if (result.success) {
setPendingInvites(
result.invitations.map((inv) => ({
id: inv.id,
organizationName: inv.organizationName,
role: inv.role,
}))
);
} else {
setInviteError(result.error);
}
setLoadingInvites(false);
}, [user?.email]);
if (inviteError) {
return (
<div className="flex min-h-screen items-center justify-center px-4 py-12">
<Card className="w-full max-w-md shadow-none border-none bg-transparent space-y-3">
<CardHeader>
<div className="flex items-center gap-2">
<AlertTriangle className="h-6 w-6 text-destructive" />
<CardTitle className="text-2xl font-medium tracking-tight text-foreground">
Unable to load invitations
</CardTitle>
</div>✅ Passed (18)
Commit: Tell us how we did: Give Ito Feedback |
- Fix substring collision in assertKeyBelongsToTenant (exact segment match) - Add request-order guard to prevent stale retry responses overwriting state - Disable retry button during loading to prevent concurrent requests - Add inline comment on verifyTenantAccess auth gate intent - Clear stale pendingInvites on error - Clarify no-org redirect comment Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: ca6f39c The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta review scoped to changes since commit 805a694c14f6 (1 new commit: ca6f39cb0).
✅ Prior Feedback Addressed
The new commit comprehensively addresses all issues raised in the previous review:
| Prior Issue | Fix Applied |
|---|---|
🟠 Weak substring matching — .includes() vulnerable to tenant ID substring collisions |
✅ Fixed with endsWith/includes(segment) pattern that correctly handles both {provider}-{tenantId} and {provider}-{tenantId}-{suffix} key formats |
| 🟠 Retry button missing disabled state — could trigger concurrent requests | ✅ Added disabled={loadingInvites} + loading text feedback |
| 🟡 Stale pendingInvites not cleared on error — inconsistent internal state | ✅ Added setPendingInvites([]) in error branch |
| 🔴 Ito test failure — rapid retry clicks causing race condition | ✅ Added requestIdRef pattern to discard stale responses |
💭 Consider (1) 💭
💭 1) invitations.ts:29 Missing error logging remains unaddressed
Issue: The previous review flagged that getPendingInvitations catches errors without logging them, limiting observability for ops debugging.
Why: This is a minor enhancement and doesn't block the security fix. The discriminated result type is the more important improvement.
Fix: Consider adding server-side logging in a follow-up:
} catch (error) {
console.error('[getPendingInvitations] Error:', error);
const message = error instanceof ApiError ? error.message : 'Failed to load invitations';
return { success: false, error: message };
}🧹 While You're Here (2) 🧹
These pre-existing issues were noted in the previous review and remain unaddressed. They are optional enhancements that would complete the tenant auth pattern:
🧹 1) nango.ts:173 fetchNangoIntegration lacks tenant authorization — Could add tenant auth to prevent cross-tenant metadata reads.
🧹 2) nango.ts:415 createProviderConnectSession lacks tenant authorization — Could validate organizationId against authenticated session.
✅ APPROVE
Summary: Excellent follow-up commit! All Major and Minor issues from the previous review have been resolved. The substring matching fix is correctly implemented, the race condition is properly guarded with requestIdRef, and the UI now provides appropriate loading feedback. The Nango tenant isolation security improvement and invitation error handling are now both solid. 🎉
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta) |
4 | 0 | 1 | 2 | 0 | 0 | 0 |
| Total | 4 | 0 | 1 | 2 | 0 | 0 | 0 |
Note: Delta review performed directly by orchestrator — prior Major/Minor issues verified as resolved, no new domain-specific reviewers dispatched.
1 participant
Summary
listNangoProviderIntegrations,deleteNangoIntegration, andupdateNangoIntegrationCredentialsnow verify tenant membership via backend API before executing with the shared Nango secret. Adds key-ownership assertion to prevent cross-tenant manipulation.getPendingInvitationsreturns a discriminated result type (success/error) instead of silently returning[]on failure./no-organizationpage shows an error state with retry button instead of falsely reporting no invitations.Test plan
/no-organizationshows error + retry when invitation API is unreachablepnpm buildpassespnpm checkpasses🤖 Generated with Claude Code