Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Claude finished @chitalian's task —— View job PR Review Complete ✅Score: 6/10 - Several security and code quality issues that should be addressed before merge Critical Issues Found🔒 Security - High Priority
🐛 Code Quality Issues
|
Greptile OverviewGreptile SummaryThis PR adds four new admin endpoints to support pricing migration operations: Key ChangesNew Admin Endpoints (adminController.ts)
Enhanced Endpoints
Supporting Changes
Critical Issues Found🔴 Security Vulnerabilities (Logic Errors)
🟡 Logic Issues
Impact AssessmentThe security issues in The missing error handling in The performance issue with Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Admin as Admin User
participant Frontend as Admin UI
participant Controller as AdminController
participant DB as Database
participant Stripe as Stripe API
Note over Admin,Stripe: Audit Subscriptions Flow
Admin->>Frontend: Click "Audit Subscriptions"
Frontend->>Controller: POST /pricing-migration/audit
Controller->>Stripe: List all active subscriptions
Stripe-->>Controller: Return subscriptions
Controller->>DB: SELECT all organizations
DB-->>Controller: Return orgs with stripe IDs
Controller->>DB: SELECT auth users & org memberships
DB-->>Controller: Return user/org data
Controller->>Controller: Compare Stripe vs DB state
Controller-->>Frontend: Return mismatches
Frontend-->>Admin: Display audit results
Note over Admin,Stripe: Fix Org Metadata Flow (VULNERABLE)
Admin->>Frontend: Select mismatch, click "Fix Metadata"
Frontend->>Controller: POST /pricing-migration/fix-metadata/{orgId}
Note over Controller: ⚠️ No validation of IDs!
Controller->>DB: UPDATE stripe_customer_id, stripe_subscription_id
DB-->>Controller: Success
Controller-->>Frontend: Return success
Frontend-->>Admin: Show success message
Note over Admin,Stripe: Fix Org Tier Flow (VULNERABLE)
Admin->>Frontend: Click "Fix Tier"
Frontend->>Controller: POST /pricing-migration/fix-tier/{orgId}
Note over Controller: ⚠️ No validation of subscription ownership!
Controller->>Stripe: Retrieve subscription details
Stripe-->>Controller: Return subscription with products
Controller->>Controller: Determine tier from product names
Controller->>DB: UPDATE tier, subscription_status
DB-->>Controller: Success
Controller-->>Frontend: Return new tier
Frontend-->>Admin: Show updated tier
Note over Admin,Stripe: Cancel Subscription Flow
Admin->>Frontend: Click "Cancel Subscription"
Frontend->>Controller: POST /pricing-migration/cancel-subscription/{orgId}
Controller->>DB: SELECT stripe_subscription_id
DB-->>Controller: Return subscription ID
Controller->>Stripe: Cancel subscription
Stripe-->>Controller: Subscription cancelled
Note over Controller: ⚠️ No error check on DB update!
Controller->>DB: UPDATE subscription_status = 'canceled'
Controller-->>Frontend: Return success
Frontend-->>Admin: Show cancellation confirmed
|
| @Post("/pricing-migration/fix-tier/{orgId}") | ||
| public async fixOrgTier( | ||
| @Request() request: JawnAuthenticatedRequest, | ||
| @Path() orgId: string, | ||
| @Body() body: { subscriptionId: string } | ||
| ): Promise<Result<{ message: string; newTier: string }, string>> { | ||
| await authCheckThrow(request.authParams.userId); | ||
|
|
||
| try { | ||
| const stripe = new Stripe(process.env.STRIPE_SECRET_KEY!); | ||
|
|
||
| // Get the subscription to determine what tier it should be | ||
| const subscription = await stripe.subscriptions.retrieve(body.subscriptionId, { | ||
| expand: ["items.data.price.product"], | ||
| }); | ||
|
|
||
| // Determine tier from product names | ||
| let newTier = "pro-20251210"; // default | ||
| for (const item of subscription.items.data) { | ||
| const product = item.price.product; | ||
| if (typeof product === "object" && product !== null && "name" in product) { | ||
| const productName = (product as { name: string }).name.toLowerCase(); | ||
| if (productName.includes("team")) { | ||
| newTier = "team-20251210"; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Update the org | ||
| await dbExecute( | ||
| ` | ||
| UPDATE organization | ||
| SET tier = $1, subscription_status = 'active' | ||
| WHERE id = $2 | ||
| `, | ||
| [newTier, orgId] | ||
| ); |
There was a problem hiding this comment.
The fixOrgTier endpoint accepts a subscriptionId in the request body but does not verify that this subscription actually belongs to the organization specified by orgId. This allows an admin to arbitrarily assign any Stripe subscription to any organization, which could lead to:
- Billing issues: Organization A could be assigned Organization B's subscription
- Data integrity problems: Mismatched subscription data in the database
- Security concerns: Unauthorized access to subscription tiers
Recommendation: Before updating the organization, verify that the subscription's customer ID matches the organization's stripe_customer_id, or that the subscription metadata contains the correct orgId.
| @Post("/pricing-migration/fix-tier/{orgId}") | |
| public async fixOrgTier( | |
| @Request() request: JawnAuthenticatedRequest, | |
| @Path() orgId: string, | |
| @Body() body: { subscriptionId: string } | |
| ): Promise<Result<{ message: string; newTier: string }, string>> { | |
| await authCheckThrow(request.authParams.userId); | |
| try { | |
| const stripe = new Stripe(process.env.STRIPE_SECRET_KEY!); | |
| // Get the subscription to determine what tier it should be | |
| const subscription = await stripe.subscriptions.retrieve(body.subscriptionId, { | |
| expand: ["items.data.price.product"], | |
| }); | |
| // Determine tier from product names | |
| let newTier = "pro-20251210"; // default | |
| for (const item of subscription.items.data) { | |
| const product = item.price.product; | |
| if (typeof product === "object" && product !== null && "name" in product) { | |
| const productName = (product as { name: string }).name.toLowerCase(); | |
| if (productName.includes("team")) { | |
| newTier = "team-20251210"; | |
| break; | |
| } | |
| } | |
| } | |
| // Update the org | |
| await dbExecute( | |
| ` | |
| UPDATE organization | |
| SET tier = $1, subscription_status = 'active' | |
| WHERE id = $2 | |
| `, | |
| [newTier, orgId] | |
| ); | |
| try { | |
| const stripe = new Stripe(process.env.STRIPE_SECRET_KEY!); | |
| // Get the subscription to determine what tier it should be | |
| const subscription = await stripe.subscriptions.retrieve(body.subscriptionId, { | |
| expand: ["items.data.price.product"], | |
| }); | |
| // Verify the organization exists and get its stripe_customer_id | |
| const orgResult = await dbExecute<{ | |
| stripe_customer_id: string | null; | |
| }>( | |
| `SELECT stripe_customer_id FROM organization WHERE id = $1`, | |
| [orgId] | |
| ); | |
| if (!orgResult.data?.[0]) { | |
| return err("Organization not found"); | |
| } | |
| const { stripe_customer_id } = orgResult.data[0]; | |
| // Verify the subscription belongs to this organization's customer | |
| if (stripe_customer_id !== subscription.customer) { | |
| return err(`Subscription ${body.subscriptionId} does not belong to organization ${orgId}`); | |
| } | |
| // Determine tier from product names | |
| let newTier = "pro-20251210"; // default | |
| for (const item of subscription.items.data) { | |
| const product = item.price.product; | |
| if (typeof product === "object" && product !== null && "name" in product) { | |
| const productName = (product as { name: string }).name.toLowerCase(); | |
| if (productName.includes("team")) { | |
| newTier = "team-20251210"; | |
| break; | |
| } | |
| } | |
| } | |
| // Update the org | |
| await dbExecute( | |
| ` | |
| UPDATE organization | |
| SET tier = $1, subscription_status = 'active' | |
| WHERE id = $2 | |
| `, | |
| [newTier, orgId] | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: valhalla/jawn/src/controllers/private/adminController.ts
Line: 3891:3928
Comment:
The `fixOrgTier` endpoint accepts a `subscriptionId` in the request body but does not verify that this subscription actually belongs to the organization specified by `orgId`. This allows an admin to arbitrarily assign any Stripe subscription to any organization, which could lead to:
1. **Billing issues**: Organization A could be assigned Organization B's subscription
2. **Data integrity problems**: Mismatched subscription data in the database
3. **Security concerns**: Unauthorized access to subscription tiers
**Recommendation**: Before updating the organization, verify that the subscription's customer ID matches the organization's `stripe_customer_id`, or that the subscription metadata contains the correct `orgId`.
```suggestion
try {
const stripe = new Stripe(process.env.STRIPE_SECRET_KEY!);
// Get the subscription to determine what tier it should be
const subscription = await stripe.subscriptions.retrieve(body.subscriptionId, {
expand: ["items.data.price.product"],
});
// Verify the organization exists and get its stripe_customer_id
const orgResult = await dbExecute<{
stripe_customer_id: string | null;
}>(
`SELECT stripe_customer_id FROM organization WHERE id = $1`,
[orgId]
);
if (!orgResult.data?.[0]) {
return err("Organization not found");
}
const { stripe_customer_id } = orgResult.data[0];
// Verify the subscription belongs to this organization's customer
if (stripe_customer_id !== subscription.customer) {
return err(`Subscription ${body.subscriptionId} does not belong to organization ${orgId}`);
}
// Determine tier from product names
let newTier = "pro-20251210"; // default
for (const item of subscription.items.data) {
const product = item.price.product;
if (typeof product === "object" && product !== null && "name" in product) {
const productName = (product as { name: string }).name.toLowerCase();
if (productName.includes("team")) {
newTier = "team-20251210";
break;
}
}
}
// Update the org
await dbExecute(
`
UPDATE organization
SET tier = $1, subscription_status = 'active'
WHERE id = $2
`,
[newTier, orgId]
);
```
How can I resolve this? If you propose a fix, please make it concise.| @Post("/pricing-migration/fix-metadata/{orgId}") | ||
| public async fixOrgMetadata( | ||
| @Request() request: JawnAuthenticatedRequest, | ||
| @Path() orgId: string, | ||
| @Body() body: { customerId: string; subscriptionId: string } | ||
| ): Promise<Result<{ message: string }, string>> { | ||
| await authCheckThrow(request.authParams.userId); | ||
|
|
||
| try { | ||
| // Update the org with the stripe IDs | ||
| await dbExecute( | ||
| ` | ||
| UPDATE organization | ||
| SET stripe_customer_id = $1, stripe_subscription_id = $2 | ||
| WHERE id = $3 | ||
| `, | ||
| [body.customerId, body.subscriptionId, orgId] | ||
| ); |
There was a problem hiding this comment.
The fixOrgMetadata endpoint accepts customerId and subscriptionId from the request body but does not validate that these Stripe IDs are related to each other or that they should belong to the specified organization. This creates a critical security vulnerability where:
- An admin could assign arbitrary Stripe customer/subscription IDs to any organization
- The subscription could belong to a different customer than specified
- These IDs could be from completely unrelated Stripe accounts
Recommendation: Add validation to verify:
- The subscription exists in Stripe
- The subscription belongs to the specified customer
- Optionally, verify the subscription metadata or customer email matches the organization
| @Post("/pricing-migration/fix-metadata/{orgId}") | |
| public async fixOrgMetadata( | |
| @Request() request: JawnAuthenticatedRequest, | |
| @Path() orgId: string, | |
| @Body() body: { customerId: string; subscriptionId: string } | |
| ): Promise<Result<{ message: string }, string>> { | |
| await authCheckThrow(request.authParams.userId); | |
| try { | |
| // Update the org with the stripe IDs | |
| await dbExecute( | |
| ` | |
| UPDATE organization | |
| SET stripe_customer_id = $1, stripe_subscription_id = $2 | |
| WHERE id = $3 | |
| `, | |
| [body.customerId, body.subscriptionId, orgId] | |
| ); | |
| try { | |
| const stripe = new Stripe(process.env.STRIPE_SECRET_KEY!); | |
| // Verify the subscription exists and belongs to the customer | |
| const subscription = await stripe.subscriptions.retrieve(body.subscriptionId); | |
| if (subscription.customer !== body.customerId) { | |
| return err(`Subscription ${body.subscriptionId} does not belong to customer ${body.customerId}`); | |
| } | |
| // Verify the organization exists | |
| const orgCheck = await dbExecute<{ id: string }>( | |
| `SELECT id FROM organization WHERE id = $1`, | |
| [orgId] | |
| ); | |
| if (!orgCheck.data?.[0]) { | |
| return err("Organization not found"); | |
| } | |
| // Update the org with the stripe IDs | |
| await dbExecute( | |
| ` | |
| UPDATE organization | |
| SET stripe_customer_id = $1, stripe_subscription_id = $2 | |
| WHERE id = $3 | |
| `, | |
| [body.customerId, body.subscriptionId, orgId] | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: valhalla/jawn/src/controllers/private/adminController.ts
Line: 3943:3960
Comment:
The `fixOrgMetadata` endpoint accepts `customerId` and `subscriptionId` from the request body but does not validate that these Stripe IDs are related to each other or that they should belong to the specified organization. This creates a critical security vulnerability where:
1. An admin could assign arbitrary Stripe customer/subscription IDs to any organization
2. The subscription could belong to a different customer than specified
3. These IDs could be from completely unrelated Stripe accounts
**Recommendation**: Add validation to verify:
- The subscription exists in Stripe
- The subscription belongs to the specified customer
- Optionally, verify the subscription metadata or customer email matches the organization
```suggestion
try {
const stripe = new Stripe(process.env.STRIPE_SECRET_KEY!);
// Verify the subscription exists and belongs to the customer
const subscription = await stripe.subscriptions.retrieve(body.subscriptionId);
if (subscription.customer !== body.customerId) {
return err(`Subscription ${body.subscriptionId} does not belong to customer ${body.customerId}`);
}
// Verify the organization exists
const orgCheck = await dbExecute<{ id: string }>(
`SELECT id FROM organization WHERE id = $1`,
[orgId]
);
if (!orgCheck.data?.[0]) {
return err("Organization not found");
}
// Update the org with the stripe IDs
await dbExecute(
`
UPDATE organization
SET stripe_customer_id = $1, stripe_subscription_id = $2
WHERE id = $3
`,
[body.customerId, body.subscriptionId, orgId]
);
```
How can I resolve this? If you propose a fix, please make it concise.| // Update org in database | ||
| await dbExecute( | ||
| ` | ||
| UPDATE organization | ||
| SET subscription_status = 'canceled' | ||
| WHERE id = $1 | ||
| `, | ||
| [orgId] | ||
| ); | ||
|
|
||
| return ok({ | ||
| message: `Cancelled subscription for org ${orgId} (tier: ${tier})`, | ||
| subscriptionId: stripe_subscription_id, | ||
| }); |
There was a problem hiding this comment.
The cancelSubscription endpoint does not check if the database update operation succeeded. If the dbExecute call fails (e.g., due to a database connection issue), the function will still return success, leading to inconsistent state where the Stripe subscription is cancelled but the database still shows it as active.
Recommendation: Check the result of the database operation and return an error if it fails.
| // Update org in database | |
| await dbExecute( | |
| ` | |
| UPDATE organization | |
| SET subscription_status = 'canceled' | |
| WHERE id = $1 | |
| `, | |
| [orgId] | |
| ); | |
| return ok({ | |
| message: `Cancelled subscription for org ${orgId} (tier: ${tier})`, | |
| subscriptionId: stripe_subscription_id, | |
| }); | |
| // Update org in database | |
| const updateResult = await dbExecute( | |
| ` | |
| UPDATE organization | |
| SET subscription_status = 'canceled' | |
| WHERE id = $1 | |
| `, | |
| [orgId] | |
| ); | |
| if (updateResult.error) { | |
| return err(`Failed to update organization status: ${updateResult.error}`); | |
| } | |
| return ok({ | |
| message: `Cancelled subscription for org ${orgId} (tier: ${tier})`, | |
| subscriptionId: stripe_subscription_id, | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: valhalla/jawn/src/controllers/private/adminController.ts
Line: 3591:3604
Comment:
The `cancelSubscription` endpoint does not check if the database update operation succeeded. If the `dbExecute` call fails (e.g., due to a database connection issue), the function will still return success, leading to inconsistent state where the Stripe subscription is cancelled but the database still shows it as active.
**Recommendation**: Check the result of the database operation and return an error if it fails.
```suggestion
// Update org in database
const updateResult = await dbExecute(
`
UPDATE organization
SET subscription_status = 'canceled'
WHERE id = $1
`,
[orgId]
);
if (updateResult.error) {
return err(`Failed to update organization status: ${updateResult.error}`);
}
return ok({
message: `Cancelled subscription for org ${orgId} (tier: ${tier})`,
subscriptionId: stripe_subscription_id,
});
```
How can I resolve this? If you propose a fix, please make it concise.| // Fetch Stripe status for each org to get trial info and subscription items | ||
| const stripe = new Stripe(process.env.STRIPE_SECRET_KEY!); | ||
| const orgsWithStripeStatus = await Promise.all( | ||
| (result.data ?? []).map(async (org) => { | ||
| let stripe_status: string | null = null; | ||
| let trial_end: number | null = null; | ||
| let subscription_items: Array<{ | ||
| product_name: string | null; | ||
| price_id: string; | ||
| unit_amount: number | null; | ||
| recurring_interval: string | null; | ||
| }> = []; | ||
| let next_invoice_date: number | null = null; | ||
| let next_invoice_amount: number | null = null; | ||
|
|
||
| if (org.stripe_subscription_id) { | ||
| try { | ||
| const subscription = await stripe.subscriptions.retrieve( | ||
| org.stripe_subscription_id, | ||
| { expand: ["items.data.price.product"] } | ||
| ); | ||
| stripe_status = subscription.status; | ||
| trial_end = subscription.trial_end; | ||
|
|
||
| // Extract subscription items with product info | ||
| subscription_items = subscription.items.data.map((item) => { | ||
| const product = item.price.product; | ||
| const productName = | ||
| typeof product === "object" && product !== null && "name" in product | ||
| ? (product as { name: string }).name | ||
| : null; | ||
|
|
||
| return { | ||
| product_name: productName, | ||
| price_id: item.price.id, | ||
| unit_amount: item.price.unit_amount, | ||
| recurring_interval: item.price.recurring?.interval ?? null, | ||
| }; | ||
| }); | ||
|
|
||
| // Get upcoming invoice | ||
| if (org.stripe_customer_id) { | ||
| try { | ||
| const upcomingInvoice = await stripe.invoices.retrieveUpcoming({ | ||
| customer: org.stripe_customer_id, | ||
| }); | ||
| next_invoice_date = upcomingInvoice.next_payment_attempt; | ||
| next_invoice_amount = upcomingInvoice.amount_due; | ||
| } catch (e) { | ||
| // No upcoming invoice (e.g., cancelled subscription) | ||
| } | ||
| } | ||
| } catch (e) { | ||
| stripe_status = "not_found"; | ||
| } | ||
| } | ||
| return { | ||
| ...org, | ||
| stripe_status, | ||
| trial_end, | ||
| subscription_items, | ||
| next_invoice_date, | ||
| next_invoice_amount, | ||
| }; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
The getOrgsWithNewTiers endpoint fetches ALL organizations with new pricing tiers and then makes parallel Stripe API calls for each one using Promise.all. Unlike getOrgsWithLegacyTiers which uses pagination (limit/offset), this endpoint has no pagination.
Potential issues:
- Performance: If there are hundreds or thousands of organizations with new tiers, this will make that many Stripe API calls in parallel
- Rate limiting: Stripe API has rate limits that could be hit with too many parallel requests
- Memory: Loading all organizations and their Stripe data into memory at once
- Timeout: The request could timeout with large datasets
Recommendation: Either add pagination to this endpoint (matching the pattern in getOrgsWithLegacyTiers), or batch the Stripe API calls to avoid overwhelming the Stripe API. For consistency, pagination would be best since the frontend already handles paginated data from the legacy tiers endpoint.
Prompt To Fix With AI
This is a comment left during a code review.
Path: valhalla/jawn/src/controllers/private/adminController.ts
Line: 3243:3308
Comment:
The `getOrgsWithNewTiers` endpoint fetches ALL organizations with new pricing tiers and then makes parallel Stripe API calls for each one using `Promise.all`. Unlike `getOrgsWithLegacyTiers` which uses pagination (limit/offset), this endpoint has no pagination.
**Potential issues**:
1. **Performance**: If there are hundreds or thousands of organizations with new tiers, this will make that many Stripe API calls in parallel
2. **Rate limiting**: Stripe API has rate limits that could be hit with too many parallel requests
3. **Memory**: Loading all organizations and their Stripe data into memory at once
4. **Timeout**: The request could timeout with large datasets
**Recommendation**: Either add pagination to this endpoint (matching the pattern in `getOrgsWithLegacyTiers`), or batch the Stripe API calls to avoid overwhelming the Stripe API. For consistency, pagination would be best since the frontend already handles paginated data from the legacy tiers endpoint.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
This means:
This asymmetry could be confusing. Consider either:
Prompt To Fix With AIThis is a comment left during a code review.
Path: valhalla/jawn/src/controllers/private/adminController.ts
Line: 2961:2973
Comment:
There's an inconsistency in how `targetTier` parameter is handled. When `targetTier === "team"`, the code calls `migrateToNewTeamPricing(true)` to skip tier validation, allowing cross-tier upgrades (e.g., pro → team). However, when `targetTier === "pro"`, it calls `migrateToNewProPricing()` which doesn't accept a skip parameter and will always validate the tier.
This means:
- ✅ You CAN force migrate from any tier to team using `targetTier: "team"`
- ❌ You CANNOT force migrate from team to pro using `targetTier: "pro"` because it will fail validation
This asymmetry could be confusing. Consider either:
1. Making `migrateToNewProPricing` also accept a `skipTierValidation` parameter, or
2. Documenting this limitation clearly, or
3. Rejecting cross-tier downgrades explicitly
```suggestion
// Run the migration (or reapply for orgs already on new tiers)
// If targetTier is specified, use that; otherwise infer from current tier
// skipTierValidation=true when targetTier is specified to allow cross-tier upgrades (e.g., pro -> team)
let migrationResult;
if (body.targetTier === "team") {
migrationResult = await stripeManager.migrateToNewTeamPricing(true);
} else if (body.targetTier === "pro") {
// Note: migrateToNewProPricing doesn't support skipTierValidation
// So downgrades from team to pro will fail if the org is on a team tier
migrationResult = await stripeManager.migrateToNewProPricing();
} else if (tier === "team-20250130" || tier === "team-20251210") {
migrationResult = await stripeManager.migrateToNewTeamPricing();
} else if (
["pro-20240913", "pro-20250202", "growth", "pro-20251210"].includes(tier)
) {
migrationResult = await stripeManager.migrateToNewProPricing();
} else {
return err(`Unknown tier for migration: ${tier}`);
}
```
How can I resolve this? If you propose a fix, please make it concise. |
Ticket
Link to the ticket(s) this pull request addresses.
Component/Service
What part of Helicone does this affect?
Type of Change
Deployment Notes
Screenshots / Demos
Extra Notes
Any additional context, considerations, or notes for reviewers.
Context
Why are you making this change?
Screenshots / Demos