fix: AMPH-v2 full codebase audit remediation (C1-C7, H1-H7)#33
fix: AMPH-v2 full codebase audit remediation (C1-C7, H1-H7)#33projectamazonph wants to merge 19 commits into
Conversation
Apply all fixes from the July 17 full-codebase audit across critical (C1-C7), high-severity (H1-H7), and medium-severity findings: - C1: Add source.chargeable + payment.paid webhook handlers for Source flow - C2: Return 5xx on handler errors instead of always-200 - C3: Add shared validateRedirectUrl() for auth XSS/open-redirect prevention - C4: Guest accounts require cryptographically random claim token - C5: Remove fallback admin credentials from seed script - C6: Prevent XP farming with atomic complete transitions - C7: Atomic refund dedup + cumulative partial refund tracking - H1: Include checkout_id in PayMongo redirect URL - H2: Validate currency, reconcile payment amounts - H3: Load admin role from DB instead of JWT - H4: Remove ProcessedWebhook from SOFT_DELETE_MODELS - H5: Atomic session claiming for simulator submissions - H6: Guard discount increment with maxUses check - H7: Avoid redirect() inside createSafeAction - Medium: Fix no-JS signup, normalize emails to lowercase - New migration: claim token fields on User model
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe update adds guest account-claim tokens, redirect validation, PayMongo source and payment webhook flows, transactional refund handling, idempotent progress and tool submissions, database schema changes, and safer operational configuration. ChangesAudit remediation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PayMongo
participant WebhookRoute
participant EnrollmentHandlers
participant Database
PayMongo->>WebhookRoute: Send payment event
WebhookRoute->>EnrollmentHandlers: Dispatch event
EnrollmentHandlers->>Database: Check idempotency and update payment and enrollment
Database-->>EnrollmentHandlers: Commit transaction
EnrollmentHandlers-->>WebhookRoute: Return result
WebhookRoute-->>PayMongo: Acknowledge or return HTTP 500 for retry
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/actions/checkout.ts (1)
158-213: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd regression coverage for the new payment-flow contract.
src/app/actions/checkout.ts#L158-L213: test that the source redirect URL and metadata contain the created checkout session ID.src/app/(public)/checkout/complete/page.tsx#L52-L57: testcheckout_idresolution and invalidreturnUrlfallback behavior.src/app/api/paymongo/webhook/route.ts#L120-L136: test signedsource.chargeableandpayment.paiddispatch, plus handler failures returning 500.As per coding guidelines, “New features must include tests; admin and business-layer features require tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/actions/checkout.ts` around lines 158 - 213, Add regression tests covering src/app/actions/checkout.ts lines 158-213 to verify the PayMongo source redirect URL and metadata include the created checkout session ID; src/app/(public)/checkout/complete/page.tsx lines 52-57 to verify checkout_id resolution and invalid returnUrl fallback behavior; and src/app/api/paymongo/webhook/route.ts lines 120-136 to verify signed source.chargeable and payment.paid dispatches, including handler failures returning HTTP 500.Source: Coding guidelines
🟠 Major comments (12)
src/app/actions/refunds.ts-82-85 (1)
82-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow partially refunded payments through the eligibility check.
Line 267 changes the payment to
PARTIALLY_REFUNDED, butisWithinRefundWindowaccepts onlyCOMPLETED. This prevents a later request for the remaining refundable balance, making the cumulative partial-refund flow unreachable.Proposed fix
- if (status !== PaymentStatus.COMPLETED) return false; + if ( + status !== PaymentStatus.COMPLETED && + status !== PaymentStatus.PARTIALLY_REFUNDED + ) { + return false; + }Also applies to: 101-113, 260-267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/actions/refunds.ts` around lines 82 - 85, Update the refund eligibility checks around isWithinRefundWindow and the later status update so PARTIALLY_REFUNDED payments remain eligible for subsequent refunds, while preserving the existing refund-window validation and COMPLETED behavior.prisma/seed.ts-37-46 (2)
37-46: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the remaining fallback credential literals.
The guard prevents fallback credentials from being used, but
main()still retains a fallback email andChangeMe123!at Lines [393-394]. This contradicts “Never publish or retain a fallback password” and the repository rule against storing secrets in source.Proposed fix
- console.log(`\nSign in with: ${process.env.ADMIN_EMAIL ?? '[email protected]'}`); - console.log(`Default password: ${process.env.ADMIN_PASSWORD ?? 'ChangeMe123!'}`); - console.log('(Change the password after first sign-in.)'); + console.log('\nAdmin credentials loaded from the environment.');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prisma/seed.ts` around lines 37 - 46, Remove the fallback email and password literals still assigned in main(), including ChangeMe123!, and make the seed flow use the validated ADMIN_EMAIL and ADMIN_PASSWORD values instead. Preserve the existing missing-environment-variable guard so execution fails before credentials are consumed.Source: Coding guidelines
37-46: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd seed-guard regression tests for
prisma/seed.ts:36-46so missing and whitespace-onlyADMIN_EMAIL/ADMIN_PASSWORDvalues stay rejected, and valid values stay accepted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prisma/seed.ts` around lines 37 - 46, Extend the seed-guard test coverage around the environment-variable validation in prisma/seed.ts to verify that missing and whitespace-only ADMIN_EMAIL or ADMIN_PASSWORD values are rejected, while non-empty valid values are accepted. Reuse the existing seed execution or validation test setup and cover each variable without changing the guard’s behavior.Source: Coding guidelines
src/app/actions/progress.ts-53-62 (1)
53-62: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake lesson-progress transitions atomic.
All three paths read completion state separately from the write, allowing concurrent requests to downgrade progress or duplicate XP.
src/app/actions/progress.ts#L53-L62: conditionally start only a missing or non-completed progress record.src/app/actions/progress.ts#L108-L117: award XP only when the transaction wins the completion transition.src/app/actions/progress.ts#L233-L245: use the same atomic transition for passing quizzes.Add concurrent regression coverage. As per coding guidelines, “When fixing a defect, reproduce it with the smallest test, fix the root cause, and add a regression test.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/actions/progress.ts` around lines 53 - 62, Make lesson-progress updates atomic across src/app/actions/progress.ts lines 53-62, 108-117, and 233-245: replace separate completion reads and writes with a conditional transactional transition that creates or starts only missing/non-completed records, awards XP only when the transaction wins completion, and applies the same transition for passing quizzes. Add the smallest concurrent regression tests covering duplicate completion and downgrade prevention.Source: Coding guidelines
src/app/actions/tools.ts-108-125 (1)
108-125: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not permanently claim the session before fallible grading.
Any exception after this update, such as missing scenario data, invalid submitted state, or a persistence failure, leaves the session
SUBMITTEDand blocks retry. Grade and validate first, then transactionally claim the session, persist the result, and update XP.Add a regression test that forces a post-claim failure. As per coding guidelines, “When fixing a defect, reproduce it with the smallest test, fix the root cause, and add a regression test.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/actions/tools.ts` around lines 108 - 125, Move the status update out of the initial claim flow around claimResult and perform grading and validation before claiming. Then transactionally change the session from IN_PROGRESS to SUBMITTED while persisting the result and updating XP, ensuring any failure leaves the session retryable; add a regression test that forces a failure after the former claim point and verifies the status is not permanently changed.Source: Coding guidelines
src/lib/enrollment.ts-799-809 (1)
799-809: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlways link the new payment to the enrollment.
For repeat purchases, the enrollment already has an older linked payment, so
alreadyLinkedis true and the newly created payment remains unlinked. UpdatelocalPayment.enrollmentIdunconditionally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/enrollment.ts` around lines 799 - 809, Update the payment-linking logic in the enrollment flow to unconditionally set localPayment.enrollmentId to enrollment.id via the existing tx.payment.update call. Remove the alreadyLinked lookup and conditional guard so newly created payments are linked even when the enrollment has an older payment.src/lib/enrollment.ts-751-766 (1)
751-766: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the discount usage limit part of the update predicate.
The read-then-increment sequence is not atomic. Concurrent payments can both observe capacity and increment
currentUsesbeyondmaxUses. Use a guardedupdateManyor row lock and require one updated row before applying the discount.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/enrollment.ts` around lines 751 - 766, Update the discount usage flow around discountCode lookup and the tx.discountCode.update call to make capacity enforcement atomic: replace the read-then-increment with a guarded updateMany whose predicate requires the matching discountCodeId and either unlimited usage or currentUses below maxUses. Require exactly one updated row before applying the discount, and preserve the existing behavior for missing or exhausted discount codes.prisma/migrations/20260717000001_claim_token/migration.sql-5-5 (1)
5-5: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove or declare the claim-token index.
prisma/schema.prismadoes not declare this index, and the supplied claim flow never searches by token hash. This creates schema drift while normal index creation can block writes. Remove it, or declare it in the schema and plan a concurrent rollout if a hash lookup is required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prisma/migrations/20260717000001_claim_token/migration.sql` at line 5, Remove the undeclared "User_claimTokenHash_idx" creation from the claim-token migration, since the claim flow does not query by claimTokenHash. Keep the Prisma schema and migration aligned; do not add an index unless the claim flow is updated to require hash lookups and a concurrent rollout is planned.Source: Linters/SAST tools
src/lib/enrollment.ts-553-570 (1)
553-570: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject currency and amount mismatches before fulfillment.
The source handler logs an amount mismatch and continues, while the payment handler never checks
currencyand also continues on mismatched amounts. This can create a completed payment and enrollment for a value different from the checkout contract.Also applies to: 617-619, 676-684
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/enrollment.ts` around lines 553 - 570, Update the source and payment handlers to reject currency or amount mismatches before creating a payment or fulfilling enrollment. In the source handler, replace the amount-mismatch warning/continuation with the existing rejection behavior, and add equivalent PHP currency and amount validation to the payment handler using checkout.finalAmountPhp and amountCentavos. Preserve the current error logging context and ensure no downstream payment or enrollment work runs after validation failure.src/lib/auth.ts-235-247 (1)
235-247: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when the authoritative user lookup fails.
dbUser?.role ?? user.rolerestores the stale JWT role if the user disappears betweenrequireAuth()and this query. A deleted user with an ADMIN token can therefore pass this check. Reject missing records instead of falling back.Proposed fix
- const effectiveRole = dbUser?.role ?? user.role; - if (effectiveRole !== 'ADMIN') { + if (!dbUser || dbUser.role !== 'ADMIN') { log.warn( - { component: 'auth', userId: user.id, role: effectiveRole }, + { component: 'auth', userId: user.id, role: dbUser?.role ?? 'missing' }, 'non-admin redirect /', ); redirect('/'); }Add a regression test for a stale ADMIN token whose database user no longer exists. As per coding guidelines, when fixing a defect, reproduce it with the smallest test, fix the root cause, and add a regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/auth.ts` around lines 235 - 247, Update the authoritative lookup and effectiveRole logic in the admin authorization flow so a missing dbUser fails closed rather than falling back to user.role; only a database user with role ADMIN may proceed, while absent users follow the existing non-admin redirect path. Add a focused regression test covering an ADMIN JWT whose database record no longer exists.Source: Coding guidelines
src/app/actions/auth.ts-182-188 (1)
182-188: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the actual confirmation field in the no-JavaScript path.
Assigning
confirmPassword: passwordmakes the schema comparison pass regardless of what the user entered. ReadconfirmPasswordfromformDatainstead.Proposed fix
- confirmPassword: password, + confirmPassword: formData.get('confirmPassword'),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/actions/auth.ts` around lines 182 - 188, Update the no-JavaScript signup payload in signUpAction to read confirmPassword from formData instead of assigning the password value, while preserving the existing password field and other payload mappings so schema confirmation validation compares the user’s actual inputs.src/lib/enrollment.ts-543-547 (1)
543-547: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude the full checkout shape in these payment paths. In
src/lib/enrollment.ts,handleSourceChargeableonly fetchesid,finalAmountPhp, andprocessPaymentPaidInTransactionalso readspricingTierId,discountCodeId, andpricingTier. Inpayment.paid, replace the top-levelselectplusincludewith one nestedselectforpricingTier, and drop theanycasts so Prisma can enforce the contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/enrollment.ts` around lines 543 - 547, Update handleSourceChargeable’s checkoutSession query to select pricingTierId, discountCodeId, and the nested pricingTier alongside the existing fields required by processPaymentPaidInTransaction. In payment.paid, replace the top-level select/include combination with a single select containing the pricingTier relation, and remove the related any casts so Prisma enforces the checkout shape.Source: Coding guidelines
🟡 Minor comments (6)
src/app/actions/refunds.ts-127-127 (1)
127-127: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmail the requested refund amount, not the original payment amount.
For a payment with prior processed refunds, Line 144 reports the full purchase amount even though the new request covers only the remaining balance.
Proposed fix
- return { requestId: request.id, payment }; + return { requestId: request.id, payment, refundAmountPhp }; ... - amountPhp: result.payment.amountPhp, + amountPhp: result.refundAmountPhp,Also applies to: 138-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/actions/refunds.ts` at line 127, The refund response and email flow currently use the original payment amount instead of the newly requested refund amount. Update the logic around the return at the end of the refund action and the email handling around lines 138-146 to use the requested refund amount, preserving the existing behavior for request ID and payment data.prisma/seed.ts-39-46 (1)
39-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject whitespace-only credentials.
The current check accepts values such as
' ', which can create an invalid email or whitespace-only password. Trim the email and reject blank credentials before the upsert, while preserving the password’s actual characters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prisma/seed.ts` around lines 39 - 46, Update the credential validation in the seed flow to trim the ADMIN_EMAIL value and reject it when blank, while also rejecting a whitespace-only ADMIN_PASSWORD. Preserve the password’s original characters after validation, and ensure the validated email and password are used by the subsequent upsert.src/app/actions/progress.ts-242-245 (1)
242-245: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep
QuizAttempt.xpEarnedconsistent with the actual award.When
alreadyCompletedis true, Line 225 still recordsxpEarned: 50even though this branch skips the XP increment. Determine completion status before creating the attempt and store zero for repeat passes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/actions/progress.ts` around lines 242 - 245, Determine the lesson’s completion status before constructing the QuizAttempt in the progress handling flow, and set xpEarned to zero when alreadyCompleted is true. Preserve the existing 50 XP value only for first-time passed attempts, keeping it consistent with the award logic in the passed-and-not-alreadyCompleted branch.src/app/actions/checkout.ts-165-175 (1)
165-175: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace typographic em dashes in changed TypeScript.
src/app/actions/checkout.ts#L165-L175: replace the em dash in the session-creation comment.src/app/api/paymongo/webhook/route.ts#L129-L136: replace em dashes in the retry comment and response text.src/lib/db.ts#L46-L46: replace the em dash in theProcessedWebhookcomment.As per coding guidelines, “Do not use emojis in code or commit messages, and do not use em-dashes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/actions/checkout.ts` around lines 165 - 175, Replace all em dashes in the changed text: update the session-creation comment near createCheckoutSessionAtomic in src/app/actions/checkout.ts lines 165-175, the retry comment and response text in src/app/api/paymongo/webhook/route.ts lines 129-136, and the ProcessedWebhook comment in src/lib/db.ts line 46. Preserve the existing meaning using standard punctuation.Source: Coding guidelines
src/app/actions/checkout.ts-49-49 (1)
49-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim email before
.email().z.string().email()runs before.transform(), so pasted addresses with surrounding spaces are rejected. Usez.preprocess(...)or trim first, and add a regression case for" User@Example.com ".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/actions/checkout.ts` at line 49, Update the email schema to trim the input before validation, using z.preprocess or an equivalent pre-validation step, while retaining lowercase normalization. Add a regression test covering " User@Example.com " and verify it is accepted with the expected normalized email.Source: Coding guidelines
src/lib/enrollment.ts-127-128 (1)
127-128: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the actual wallet payment method.
Every source payment is stored as
GCASH, although this flow explicitly handles Maya and GrabPay sources too. Carry the source type into the transaction and map it to the correctPaymentMethod.Also applies to: 703-735
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/enrollment.ts` around lines 127 - 128, Update the enrollment transaction flow around the source type field and the handling covered by lines 703-735 so the actual wallet source type is propagated instead of always being stored as GCASH. Map Maya, GrabPay, and GCASH source types to their corresponding PaymentMethod values while preserving the existing behavior for other transaction data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/actions/auth.ts`:
- Around line 51-83: The claim-token update in the registration flow must
atomically consume the token to prevent concurrent reuse. Update the database
operation in the surrounding signup action to conditionally match the existing
user’s placeholder password marker, claimTokenHash, and claimTokenExpiresAt,
then require exactly one row to be updated before creating or returning a
session; handle a zero-row result as an invalid or already-consumed token. Add a
minimal concurrent regression test exercising two requests with the same claim
token and verifying only one succeeds.
- Around line 31-36: Update signUpWithClaimSchema to use Zod’s safeExtend
instead of extend when adding claimToken to the refined signUpSchema, preventing
module-initialization failures. Add a test that imports or constructs this
schema and verifies construction succeeds.
In `@src/app/actions/refunds.ts`:
- Around line 268-273: Update the refund flow around refundPayment and the
payment update to persist an idempotency key before or with the external refund,
and distinguish PayMongo call failures from post-refund database reconciliation
failures. If refundPayment succeeds but the database update fails, do not mark
the request as an API failure; retain the successful refund state and surface
reconciliation failure for safe retry without issuing another refund.
- Around line 262-263: Update the payment query used by the refund flow to
select refundAmountPhp before it is accessed in the existingRefunded
calculation, ensuring the generated payment type exposes this field under strict
TypeScript without using any or weakening types.
- Around line 60-63: Update the transaction callback around db.$transaction in
the refund action to enforce at most one PENDING refund per paymentId under
concurrent requests, using a database unique constraint or row-locking with
retry handling. Ensure duplicate attempts return the existing conflict behavior,
and add a regression test that submits concurrent refund requests for the same
paymentId and verifies only one succeeds.
In `@src/lib/enrollment.ts`:
- Around line 722-723: Update the enrollment flow around findOrCreateUserByEmail
to retain rawClaimToken alongside userId, propagate it through the post-commit
result, and use it to send the guest claim email exactly once. Remove any
attempt to recover the token from its hash and avoid logging the purchaser’s
email address after discarding the raw token.
- Around line 470-487: The refund flow around approveRefundAction and the
webhook payment update must write each refund only once. Use the PayMongo refund
ID to make one path the sole ledger writer and have the other path skip
already-recorded refunds, preserving cumulative totals and status transitions
without double-counting.
- Line 537: Remove the backslash escapes before the template-literal delimiters
in the amountCentavos interpolated strings at both occurrences, leaving valid
TypeScript template literals.
- Around line 548-550: Update the early-return paths in the source chargeable,
checkout/course lookup flows around the checkout and related record checks so
missing required records throw a retryable error instead of returning null or
otherwise acknowledging the webhook. Ensure ProcessedWebhook completion is
recorded only after all required records are found and fulfillment succeeds,
including the additional locations noted in the comment.
- Around line 531-603: Refactor the flow around markWebhookProcessed,
createPaymentFromSource, processPaymentPaidInTransaction, and
sendPostPurchaseEmails so the webhook claim commits before any PayMongo call.
Perform the external payment creation after that transaction, then execute
payment and enrollment database writes in a new transaction and send
post-purchase emails only after it commits; preserve idempotency so retries
cannot duplicate the external payment.
---
Outside diff comments:
In `@src/app/actions/checkout.ts`:
- Around line 158-213: Add regression tests covering src/app/actions/checkout.ts
lines 158-213 to verify the PayMongo source redirect URL and metadata include
the created checkout session ID; src/app/(public)/checkout/complete/page.tsx
lines 52-57 to verify checkout_id resolution and invalid returnUrl fallback
behavior; and src/app/api/paymongo/webhook/route.ts lines 120-136 to verify
signed source.chargeable and payment.paid dispatches, including handler failures
returning HTTP 500.
---
Major comments:
In `@prisma/migrations/20260717000001_claim_token/migration.sql`:
- Line 5: Remove the undeclared "User_claimTokenHash_idx" creation from the
claim-token migration, since the claim flow does not query by claimTokenHash.
Keep the Prisma schema and migration aligned; do not add an index unless the
claim flow is updated to require hash lookups and a concurrent rollout is
planned.
In `@prisma/seed.ts`:
- Around line 37-46: Remove the fallback email and password literals still
assigned in main(), including ChangeMe123!, and make the seed flow use the
validated ADMIN_EMAIL and ADMIN_PASSWORD values instead. Preserve the existing
missing-environment-variable guard so execution fails before credentials are
consumed.
- Around line 37-46: Extend the seed-guard test coverage around the
environment-variable validation in prisma/seed.ts to verify that missing and
whitespace-only ADMIN_EMAIL or ADMIN_PASSWORD values are rejected, while
non-empty valid values are accepted. Reuse the existing seed execution or
validation test setup and cover each variable without changing the guard’s
behavior.
In `@src/app/actions/auth.ts`:
- Around line 182-188: Update the no-JavaScript signup payload in signUpAction
to read confirmPassword from formData instead of assigning the password value,
while preserving the existing password field and other payload mappings so
schema confirmation validation compares the user’s actual inputs.
In `@src/app/actions/progress.ts`:
- Around line 53-62: Make lesson-progress updates atomic across
src/app/actions/progress.ts lines 53-62, 108-117, and 233-245: replace separate
completion reads and writes with a conditional transactional transition that
creates or starts only missing/non-completed records, awards XP only when the
transaction wins completion, and applies the same transition for passing
quizzes. Add the smallest concurrent regression tests covering duplicate
completion and downgrade prevention.
In `@src/app/actions/refunds.ts`:
- Around line 82-85: Update the refund eligibility checks around
isWithinRefundWindow and the later status update so PARTIALLY_REFUNDED payments
remain eligible for subsequent refunds, while preserving the existing
refund-window validation and COMPLETED behavior.
In `@src/app/actions/tools.ts`:
- Around line 108-125: Move the status update out of the initial claim flow
around claimResult and perform grading and validation before claiming. Then
transactionally change the session from IN_PROGRESS to SUBMITTED while
persisting the result and updating XP, ensuring any failure leaves the session
retryable; add a regression test that forces a failure after the former claim
point and verifies the status is not permanently changed.
In `@src/lib/auth.ts`:
- Around line 235-247: Update the authoritative lookup and effectiveRole logic
in the admin authorization flow so a missing dbUser fails closed rather than
falling back to user.role; only a database user with role ADMIN may proceed,
while absent users follow the existing non-admin redirect path. Add a focused
regression test covering an ADMIN JWT whose database record no longer exists.
In `@src/lib/enrollment.ts`:
- Around line 799-809: Update the payment-linking logic in the enrollment flow
to unconditionally set localPayment.enrollmentId to enrollment.id via the
existing tx.payment.update call. Remove the alreadyLinked lookup and conditional
guard so newly created payments are linked even when the enrollment has an older
payment.
- Around line 751-766: Update the discount usage flow around discountCode lookup
and the tx.discountCode.update call to make capacity enforcement atomic: replace
the read-then-increment with a guarded updateMany whose predicate requires the
matching discountCodeId and either unlimited usage or currentUses below maxUses.
Require exactly one updated row before applying the discount, and preserve the
existing behavior for missing or exhausted discount codes.
- Around line 553-570: Update the source and payment handlers to reject currency
or amount mismatches before creating a payment or fulfilling enrollment. In the
source handler, replace the amount-mismatch warning/continuation with the
existing rejection behavior, and add equivalent PHP currency and amount
validation to the payment handler using checkout.finalAmountPhp and
amountCentavos. Preserve the current error logging context and ensure no
downstream payment or enrollment work runs after validation failure.
- Around line 543-547: Update handleSourceChargeable’s checkoutSession query to
select pricingTierId, discountCodeId, and the nested pricingTier alongside the
existing fields required by processPaymentPaidInTransaction. In payment.paid,
replace the top-level select/include combination with a single select containing
the pricingTier relation, and remove the related any casts so Prisma enforces
the checkout shape.
---
Minor comments:
In `@prisma/seed.ts`:
- Around line 39-46: Update the credential validation in the seed flow to trim
the ADMIN_EMAIL value and reject it when blank, while also rejecting a
whitespace-only ADMIN_PASSWORD. Preserve the password’s original characters
after validation, and ensure the validated email and password are used by the
subsequent upsert.
In `@src/app/actions/checkout.ts`:
- Around line 165-175: Replace all em dashes in the changed text: update the
session-creation comment near createCheckoutSessionAtomic in
src/app/actions/checkout.ts lines 165-175, the retry comment and response text
in src/app/api/paymongo/webhook/route.ts lines 129-136, and the ProcessedWebhook
comment in src/lib/db.ts line 46. Preserve the existing meaning using standard
punctuation.
- Line 49: Update the email schema to trim the input before validation, using
z.preprocess or an equivalent pre-validation step, while retaining lowercase
normalization. Add a regression test covering " User@Example.com " and verify it
is accepted with the expected normalized email.
In `@src/app/actions/progress.ts`:
- Around line 242-245: Determine the lesson’s completion status before
constructing the QuizAttempt in the progress handling flow, and set xpEarned to
zero when alreadyCompleted is true. Preserve the existing 50 XP value only for
first-time passed attempts, keeping it consistent with the award logic in the
passed-and-not-alreadyCompleted branch.
In `@src/app/actions/refunds.ts`:
- Line 127: The refund response and email flow currently use the original
payment amount instead of the newly requested refund amount. Update the logic
around the return at the end of the refund action and the email handling around
lines 138-146 to use the requested refund amount, preserving the existing
behavior for request ID and payment data.
In `@src/lib/enrollment.ts`:
- Around line 127-128: Update the enrollment transaction flow around the source
type field and the handling covered by lines 703-735 so the actual wallet source
type is propagated instead of always being stored as GCASH. Map Maya, GrabPay,
and GCASH source types to their corresponding PaymentMethod values while
preserving the existing behavior for other transaction data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d3967282-c972-4569-8673-fd7a306cefcd
📒 Files selected for processing (20)
CHANGELOG.mdprisma/migrations/20260717000001_claim_token/migration.sqlprisma/migrations/20260717000001_claim_token/migration_lock.tomlprisma/schema.prismaprisma/seed.tssrc/app/(public)/auth/signin/SignInForm.tsxsrc/app/(public)/auth/signin/page.tsxsrc/app/(public)/auth/signup/SignUpForm.tsxsrc/app/(public)/auth/signup/page.tsxsrc/app/(public)/checkout/complete/page.tsxsrc/app/actions/auth.tssrc/app/actions/checkout.tssrc/app/actions/progress.tssrc/app/actions/refunds.tssrc/app/actions/tools.tssrc/app/api/paymongo/webhook/route.tssrc/lib/auth.tssrc/lib/db.tssrc/lib/enrollment.tssrc/lib/validation.ts
| // Extended schema for guest-account claiming with a claim token | ||
| const signUpWithClaimSchema = signUpSchema.extend({ | ||
| claimToken: z.string().optional(), | ||
| }); | ||
|
|
||
| export const signUpAction = createSafeAction(signUpWithClaimSchema, async (data) => { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Use safeExtend for the refined Zod schema.
signUpSchema contains .refine(...). In Zod 4, extending a schema with refinements through .extend() can throw during module initialization. Use .safeExtend(...) and cover schema construction in a test.
Proposed fix
-const signUpWithClaimSchema = signUpSchema.extend({
+const signUpWithClaimSchema = signUpSchema.safeExtend({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Extended schema for guest-account claiming with a claim token | |
| const signUpWithClaimSchema = signUpSchema.extend({ | |
| claimToken: z.string().optional(), | |
| }); | |
| export const signUpAction = createSafeAction(signUpWithClaimSchema, async (data) => { | |
| // Extended schema for guest-account claiming with a claim token | |
| const signUpWithClaimSchema = signUpSchema.safeExtend({ | |
| claimToken: z.string().optional(), | |
| }); | |
| export const signUpAction = createSafeAction(signUpWithClaimSchema, async (data) => { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/actions/auth.ts` around lines 31 - 36, Update signUpWithClaimSchema
to use Zod’s safeExtend instead of extend when adding claimToken to the refined
signUpSchema, preventing module-initialization failures. Add a test that imports
or constructs this schema and verifies construction succeeds.
| const isClaimable = | ||
| existing.passwordHash === 'placeholder_claim' && | ||
| existing.claimTokenHash && | ||
| existing.claimTokenExpiresAt && | ||
| existing.claimTokenExpiresAt > new Date(); | ||
| if (!isClaimable) { | ||
| throw new Error('An account with that email already exists.'); | ||
| } | ||
|
|
||
| // Validate the claim token (C4 - must prove access to the email inbox) | ||
| if (!data.claimToken || !existing.claimTokenHash || !existing.claimTokenExpiresAt) { | ||
| throw new Error( | ||
| 'This email is registered to a guest checkout account. ' + | ||
| 'Check your email for the claim link with your account token.', | ||
| ); | ||
| } | ||
| if (!verifyClaimToken(data.claimToken, existing.claimTokenHash, existing.claimTokenExpiresAt)) { | ||
| throw new Error( | ||
| 'Invalid or expired claim token. Request a new claim link.', | ||
| ); | ||
| } | ||
|
|
||
| // Invalidate all claim tokens after successful use | ||
| const upgraded = await db.user.update({ | ||
| where: { id: existing.id }, | ||
| data: { | ||
| name: data.name ?? existing.name, | ||
| passwordHash: await hashPassword(data.password), | ||
| emailVerified: new Date(), | ||
| claimTokenHash: null, | ||
| claimTokenExpiresAt: null, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Atomically consume the one-time claim token.
Two concurrent requests can both verify the same hash before either update clears it. Both then set a password and receive valid sessions, with the last password winning. Guard the update by the placeholder marker, token hash, and expiry, and proceed only when exactly one row is updated.
Add a concurrent regression test. As per coding guidelines, when fixing a defect, reproduce it with the smallest test, fix the root cause, and add a regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/actions/auth.ts` around lines 51 - 83, The claim-token update in the
registration flow must atomically consume the token to prevent concurrent reuse.
Update the database operation in the surrounding signup action to conditionally
match the existing user’s placeholder password marker, claimTokenHash, and
claimTokenExpiresAt, then require exactly one row to be updated before creating
or returning a session; handle a zero-row result as an invalid or
already-consumed token. Add a minimal concurrent regression test exercising two
requests with the same claim token and verifying only one succeeds.
Source: Coding guidelines
| // C7: Atomic check-and-insert inside a single transaction to prevent | ||
| // duplicate refund requests. The transaction ensures that if two | ||
| // concurrent requests both pass the checks, only one succeeds. | ||
| const result = await db.$transaction(async (tx) => { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
schema="$(fd -a '^schema\.prisma$' | head -n1)"
package="$(fd -a '^package\.json$' | head -n1)"
rg -n -C4 'model RefundRequest|paymentId|status|@@unique|@@index' "$schema"
rg -n -C3 '"`@prisma/client`"|"prisma"' "$package"
rg -n -C3 '\$transaction|TransactionIsolationLevel|Serializable' --type=tsRepository: projectamazonph/amph-v2
Length of output: 16137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/app/actions/refunds.ts"
wc -l "$file"
ast-grep outline "$file" --view expanded
printf '\n--- relevant slice ---\n'
sed -n '1,180p' "$file"
printf '\n--- tests / related refs ---\n'
rg -n -C3 'createRefundRequestAction|RefundRequest|refundAmountPhp|PARTIALLY_REFUNDED|Serializable|TransactionIsolationLevel|$transaction' src test testsRepository: projectamazonph/amph-v2
Length of output: 26624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/app/actions/refunds.ts"
wc -l "$file"
ast-grep outline "$file" --view expanded
printf '\n--- relevant slice ---\n'
sed -n '1,180p' "$file"
printf '\n--- tests / related refs ---\n'
rg -n -C3 'createRefundRequestAction|RefundRequest|refundAmountPhp|PARTIALLY_REFUNDED|Serializable|TransactionIsolationLevel|\$transaction' src test testsRepository: projectamazonph/amph-v2
Length of output: 36041
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,170p' src/app/actions/refunds.tsRepository: projectamazonph/amph-v2
Length of output: 5906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '176,310p' src/app/actions/refunds.tsRepository: projectamazonph/amph-v2
Length of output: 4609
Enforce a single refund request per payment
db.$transaction alone does not stop two concurrent requests from both passing the blocking check and inserting separate PENDING rows. Add a unique constraint or lock/retry path for paymentId, and add a concurrent regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/actions/refunds.ts` around lines 60 - 63, Update the transaction
callback around db.$transaction in the refund action to enforce at most one
PENDING refund per paymentId under concurrent requests, using a database unique
constraint or row-locking with retry handling. Ensure duplicate attempts return
the existing conflict behavior, and add a regression test that submits
concurrent refund requests for the same paymentId and verifies only one
succeeds.
Source: Coding guidelines
| await db.payment.update({ | ||
| where: { id: request.payment.id }, | ||
| data: { | ||
| status: newPaymentStatus, | ||
| refundedAt: new Date(), | ||
| refundAmountPhp: request.amountPhp, | ||
| refundAmountPhp: newTotalRefunded, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Do not mark successful PayMongo refunds as API failures.
If this database update fails after refundPayment succeeds, the catch block marks the request FAILED and reports that the API call failed. The customer may already have been refunded, and a later request can issue another refund. Persist an idempotency key and distinguish external-call failures from post-refund reconciliation failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/actions/refunds.ts` around lines 268 - 273, Update the refund flow
around refundPayment and the payment update to persist an idempotency key before
or with the external refund, and distinguish PayMongo call failures from
post-refund database reconciliation failures. If refundPayment succeeds but the
database update fails, do not mark the request as an API failure; retain the
successful refund state and surface reconciliation failure for safe retry
without issuing another refund.
| // C7: Track cumulative refund amounts. Sum the current refundAmountPhp | ||
| // with the new refund amount to handle partial refunds correctly. | ||
| const currentRefunded = payment.refundAmountPhp ?? 0; | ||
| const newTotalRefunded = currentRefunded + amount; | ||
|
|
||
| // Determine new status based on cumulative amounts | ||
| const fullyRefunded = newTotalRefunded >= payment.amountPhp; | ||
| const newStatus = fullyRefunded | ||
| ? PaymentStatus.REFUNDED | ||
| : PaymentStatus.PARTIALLY_REFUNDED; | ||
|
|
||
| await tx.payment.update({ | ||
| where: { id: payment.id }, | ||
| data: { | ||
| status: PaymentStatus.REFUNDED, | ||
| status: newStatus, | ||
| refundedAt: new Date(), | ||
| refundAmountPhp: amount, | ||
| refundAmountPhp: newTotalRefunded, | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Do not count the same refund in both the action and webhook.
approveRefundAction already adds the approved amount to refundAmountPhp. This webhook adds it again, so one partial refund can become fully refunded and revoke enrollment access. Make either the webhook or the approval path the sole ledger writer, keyed by the PayMongo refund ID.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/enrollment.ts` around lines 470 - 487, The refund flow around
approveRefundAction and the webhook payment update must write each refund only
once. Use the PayMongo refund ID to make one path the sole ledger writer and
have the other path skip already-recorded refunds, preserving cumulative totals
and status transitions without double-counting.
| return db.$transaction(async (tx) => { | ||
| const firstTime = await markWebhookProcessed( | ||
| eventId, | ||
| 'source.chargeable', | ||
| 'source', | ||
| sourceId, | ||
| \`amount=\${amountCentavos}\`, | ||
| 200, | ||
| tx, | ||
| ); | ||
| if (!firstTime) return null; | ||
|
|
||
| // Find the CheckoutSession by source ID | ||
| const checkout = await tx.checkoutSession.findFirst({ | ||
| where: { paymongoSourceId: sourceId, deletedAt: null }, | ||
| select: { id: true, finalAmountPhp: true, email: true }, | ||
| }); | ||
| if (!checkout) { | ||
| logger.warn({ sourceId }, 'source.chargeable: no checkout session found for source'); | ||
| return null; | ||
| } | ||
|
|
||
| // H2: Verify amount consistency with webhook amount and validate currency. | ||
| // The finalAmountPhp is in centavos — verify consistency with webhook amount. | ||
| const currency = event.data.attributes.data.attributes.currency; | ||
| if (currency && currency !== 'PHP') { | ||
| logger.error( | ||
| { sourceId, currency, expectedCurrency: 'PHP' }, | ||
| 'source.chargeable: unexpected currency — rejecting', | ||
| ); | ||
| throw new Error(`Unexpected currency: ${currency}. Expected PHP.`); | ||
| } | ||
|
|
||
| const expectedAmount = checkout.finalAmountPhp; | ||
| if (expectedAmount !== amountCentavos) { | ||
| logger.warn( | ||
| { sourceId, expectedAmount, actualAmount: amountCentavos }, | ||
| 'source.chargeable: amount mismatch — creating payment with webhook amount', | ||
| ); | ||
| } | ||
|
|
||
| try { | ||
| const payment = await createPaymentFromSource({ | ||
| amountCentavos, | ||
| sourceId, | ||
| description: \`Checkout \${checkout.id} — Amazon PH Academy\`, | ||
| metadata: { checkoutId: checkout.id, ...metadata }, | ||
| }); | ||
|
|
||
| logger.info( | ||
| { sourceId, paymentId: payment.id, status: payment.status }, | ||
| 'source.chargeable: payment created from source', | ||
| ); | ||
|
|
||
| // If already paid, process immediately | ||
| if (payment.status === 'paid') { | ||
| const result = await processPaymentPaidInTransaction( | ||
| tx as any, | ||
| payment, | ||
| checkout, | ||
| ); | ||
| if (result) { | ||
| // Send enrollment confirmation + claim token email | ||
| await sendPostPurchaseEmails(result, checkout); | ||
| } | ||
| } | ||
|
|
||
| return payment; | ||
| } catch (err) { | ||
| logger.error({ err, sourceId }, 'source.chargeable: failed to create payment from source'); | ||
| throw err; // Will be caught by webhook handler | ||
| } | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Move PayMongo calls and post-purchase work outside the database transaction.
If PayMongo creates the payment and the transaction later fails, the processed marker rolls back and a retry can issue the external write again. Invoice and email work also runs before commit. Commit the event claim first, make the external call idempotent, perform local writes in a new transaction, then run post-commit side effects.
Also applies to: 690-695
🧰 Tools
🪛 Biome (2.5.3)
[error] 537-537: unexpected token \
(parse)
[error] 561-561: expected , but instead found Unexpected
(parse)
[error] 561-561: expected , but instead found currency
(parse)
[error] 561-561: expected , but instead found :
(parse)
[error] 564-564: expected , but instead found const
(parse)
[error] 576-576: unexpected token \
(parse)
🪛 GitHub Actions: CI / 2_Quality Gates.txt
[error] 537-537: TypeScript compilation failed (tsc --noEmit): TS1127 Invalid character.
🪛 GitHub Actions: CI / Quality Gates
[error] 537-537: TypeScript (tsc) failed: TS1127 Invalid character.
🪛 GitHub Check: Quality Gates
[failure] 561-561:
Identifier expected.
[failure] 561-561:
Unexpected keyword or identifier.
[failure] 561-561:
Declaration or statement expected.
[failure] 561-561:
Unexpected keyword or identifier.
[failure] 561-561:
';' expected.
[failure] 537-537:
Invalid character.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/enrollment.ts` around lines 531 - 603, Refactor the flow around
markWebhookProcessed, createPaymentFromSource, processPaymentPaidInTransaction,
and sendPostPurchaseEmails so the webhook claim commits before any PayMongo
call. Perform the external payment creation after that transaction, then execute
payment and enrollment database writes in a new transaction and send
post-purchase emails only after it commits; preserve idempotency so retries
cannot duplicate the external payment.
| if (!checkout) { | ||
| logger.warn({ sourceId }, 'source.chargeable: no checkout session found for source'); | ||
| return null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Do not mark incomplete webhook processing as successful.
These early returns commit the ProcessedWebhook record. A temporarily missing checkout or course is therefore acknowledged permanently and cannot succeed on retry. Throw a retryable error, or record completion only after all required records are found and fulfillment succeeds.
Also applies to: 649-651, 741-749
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/enrollment.ts` around lines 548 - 550, Update the early-return paths
in the source chargeable, checkout/course lookup flows around the checkout and
related record checks so missing required records throw a retryable error
instead of returning null or otherwise acknowledging the webhook. Ensure
ProcessedWebhook completion is recorded only after all required records are
found and fulfillment succeeds, including the additional locations noted in the
comment.
| // Create or find user | ||
| const { id: userId } = await findOrCreateUserByEmail(checkout.email, null, tx); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Deliver the generated claim token instead of discarding it.
findOrCreateUserByEmail returns rawClaimToken, but this caller keeps only userId. The later code cannot recover the raw value from its hash and merely logs that an email was sent. Guest purchasers therefore receive no usable claim link. Propagate the raw token in the post-commit result, send the claim email once, then discard it without logging the email address.
Also applies to: 824-860
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/enrollment.ts` around lines 722 - 723, Update the enrollment flow
around findOrCreateUserByEmail to retain rawClaimToken alongside userId,
propagate it through the post-commit result, and use it to send the guest claim
email exactly once. Remove any attempt to recover the token from its hash and
avoid logging the purchaser’s email address after discarding the raw token.
Source: Linters/SAST tools
- C2: webhook 5xx only for actual handler errors - refunds: email uses requested refund amount, not full payment - progress: quiz xpEarned=0 when already completed - checkout: email schema trims before validation - enrollment: propagate actual wallet source type instead of hardcoded GCASH - seed: validate ADMIN_EMAIL/ADMIN_PASSWORD for blank/whitespace
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/actions/progress.ts (1)
214-244: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCritical:
alreadyCompletedused before declaration (temporal dead zone).Line 215 references
alreadyCompletedbut it is not declared until line 244. Whenpassedistrue, this throws aReferenceErrorat runtime, crashing every passing quiz submission.Move the
lessonProgressquery andalreadyCompleteddeclaration above their first usage:🐛 Proposed fix: move declaration before usage
// Get the next attempt number const priorAttempts = await db.quizAttempt.count({ where: { userId: user.id, quizId: lesson.quiz.id }, }); + // Check current progress before creating the attempt to determine + // whether XP should be awarded (first-time completion only). + const lessonProgress = passed + ? await db.lessonProgress.findUnique({ + where: { + userId_lessonId: { userId: user.id, lessonId: lesson.id }, + }, + select: { status: true }, + }) + : null; + + const alreadyCompleted = lessonProgress?.status === ProgressStatus.COMPLETED; + // Persist attempt. Only award XP on first-time completion. const attemptXp = passed && !alreadyCompleted ? 50 : 0; await db.quizAttempt.create({ data: { userId: user.id, quizId: lesson.quiz.id, attemptNumber: priorAttempts + 1, status: passed ? AttemptStatus.GRADED : AttemptStatus.SUBMITTED, answers: JSON.stringify(data.answers), score, correctCount, totalQuestions, xpEarned: attemptXp, timeSpentSeconds: data.timeSpentSeconds ?? 0, completedAt: new Date(), }, }); - // C6: Award XP only on the atomic transition from non-complete to complete. - // Check current progress before creating the attempt. Set xpEarned to 0 - // when the lesson is already completed. - const lessonProgress = passed - ? await db.lessonProgress.findUnique({ - where: { - userId_lessonId: { userId: user.id, lessonId: lesson.id }, - }, - select: { status: true }, - }) - : null; - - const alreadyCompleted = lessonProgress?.status === ProgressStatus.COMPLETED; - // If passed and not already completed, mark lesson complete and award XP if (passed && !alreadyCompleted) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/actions/progress.ts` around lines 214 - 244, Move the lessonProgress lookup and alreadyCompleted declaration above the attemptXp calculation in the quiz submission flow. Ensure attemptXp uses the initialized alreadyCompleted value, while preserving the existing conditional query for passed submissions and the attempt creation behavior.Source: Linters/SAST tools
src/app/actions/checkout.ts (1)
210-213: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftHandle the source/session link failure window.
createSource(...)succeeds before thecheckoutSession.update(...), and the webhook flow still keys offpaymongoSourceId, so a failed update leaves the source unlinked and the payment unprocessable. Add compensation or an idempotent retry path, and cover the update-failure case with a regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/actions/checkout.ts` around lines 210 - 213, Update the checkout flow around createSource and the checkoutSession.update call to handle update failures: add compensation for the created source or an idempotent retry path that reliably links source.id to paymongoSourceId. Preserve webhook processing keyed by paymongoSourceId, and add a regression test covering a failed session update and the resulting recovery behavior.src/lib/validation.ts (1)
34-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim and lowercase before email validation.
z.string().email()checks the raw input first, so values likeUser@Example.comfail before the later transform runs.
src/lib/validation.ts#L34src/lib/validation.ts#L48src/app/actions/checkout.ts#L49Move the trim/lowercase step ahead of
.email(), or split the schema with.pipe()so normalization happens first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/validation.ts` at line 34, Normalize email values by trimming and lowercasing before email validation in the email schema at src/lib/validation.ts:34 and src/lib/validation.ts:48, and ensure the corresponding checkout handling at src/app/actions/checkout.ts:49 uses the normalized value before validation. Preserve the existing validation behavior for already-normalized emails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/actions/checkout.ts`:
- Line 49: Update the email schema in the checkout action to trim and validate
non-empty input before applying email validation, then lowercase the normalized
value. Add a regression test covering padded, mixed-case email input and verify
it is accepted and normalized.
In `@src/lib/__tests__/enrollment.test.ts`:
- Around line 521-542: Update makeSourceEvent to return the explicit
SourceChargeableEvent type instead of any, and type its constructed fixture
accordingly so TypeScript validates the webhook contract while preserving the
existing overrides behavior.
In `@src/lib/redirect-url.ts`:
- Around line 28-29: Replace the broad encoded-byte rejection in the redirect
validation around the path check with decoding followed by narrow validation
that rejects decoded schemes, control characters, backslashes, and network paths
while allowing safe encoded internal paths. Update src/lib/redirect-url.ts lines
28-29 accordingly, and add a positive regression test for a safe encoded path in
src/lib/__tests__/validation.test.ts lines 113-115.
---
Outside diff comments:
In `@src/app/actions/checkout.ts`:
- Around line 210-213: Update the checkout flow around createSource and the
checkoutSession.update call to handle update failures: add compensation for the
created source or an idempotent retry path that reliably links source.id to
paymongoSourceId. Preserve webhook processing keyed by paymongoSourceId, and add
a regression test covering a failed session update and the resulting recovery
behavior.
In `@src/app/actions/progress.ts`:
- Around line 214-244: Move the lessonProgress lookup and alreadyCompleted
declaration above the attemptXp calculation in the quiz submission flow. Ensure
attemptXp uses the initialized alreadyCompleted value, while preserving the
existing conditional query for passed submissions and the attempt creation
behavior.
In `@src/lib/validation.ts`:
- Line 34: Normalize email values by trimming and lowercasing before email
validation in the email schema at src/lib/validation.ts:34 and
src/lib/validation.ts:48, and ensure the corresponding checkout handling at
src/app/actions/checkout.ts:49 uses the normalized value before validation.
Preserve the existing validation behavior for already-normalized emails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68e0c255-e4b8-453c-aa17-34630689e41a
📒 Files selected for processing (17)
prisma/schema.prismaprisma/seed.tssrc/app/(public)/auth/signin/SignInForm.tsxsrc/app/(public)/auth/signin/page.tsxsrc/app/(public)/auth/signup/SignUpForm.tsxsrc/app/(public)/auth/signup/page.tsxsrc/app/(public)/checkout/complete/page.tsxsrc/app/actions/checkout.tssrc/app/actions/progress.tssrc/app/actions/refunds.tssrc/app/api/paymongo/webhook/route.tssrc/lib/__tests__/auth.test.tssrc/lib/__tests__/enrollment.test.tssrc/lib/__tests__/validation.test.tssrc/lib/enrollment.tssrc/lib/redirect-url.tssrc/lib/validation.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/app/(public)/auth/signin/page.tsx
- src/app/(public)/checkout/complete/page.tsx
- src/app/(public)/auth/signin/SignInForm.tsx
- src/app/(public)/auth/signup/page.tsx
- src/app/api/paymongo/webhook/route.ts
- prisma/schema.prisma
- src/app/(public)/auth/signup/SignUpForm.tsx
- prisma/seed.ts
- src/app/actions/refunds.ts
- src/lib/enrollment.ts
| const checkoutSchema = z.object({ | ||
| pricingTierId: z.string().min(1), | ||
| email: z.string().email(), | ||
| email: z.string().email().trim().min(1).transform((v) => v.toLowerCase()), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== checkout.ts context =="
sed -n '1,120p' src/app/actions/checkout.ts | cat -n
echo
echo "== zod version markers =="
if [ -f package.json ]; then
jq -r '.dependencies.zod // .devDependencies.zod // empty' package.json
fi
if [ -f package-lock.json ]; then
jq -r '.. | objects | select(has("zod")) | .zod.version? // empty' package-lock.json | head
fi
if [ -f pnpm-lock.yaml ]; then
rg -n "zod@" pnpm-lock.yaml | head -20
fi
if [ -f yarn.lock ]; then
rg -n "^zod@" yarn.lock | head -20
fi
echo
echo "== local zod install check =="
node - <<'JS'
try {
const zod = require('zod');
console.log('zod require ok');
console.log('zod version', require('zod/package.json').version);
} catch (e) {
console.log('zod require failed:', e && e.message ? e.message : e);
}
JSRepository: projectamazonph/amph-v2
Length of output: 5439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const { z } = require('zod');
const schemas = {
email_then_trim: z.string().email().trim().min(1),
trim_then_email: z.string().trim().min(1).email(),
pipe_email: z.string().trim().min(1).pipe(z.email()),
email_only: z.string().email(),
};
const inputs = [
'User@Example.com',
' User@Example.com ',
'\tUser@Example.com\n',
' user@example.com ',
'',
' ',
'not-an-email',
];
for (const [name, schema] of Object.entries(schemas)) {
console.log(`\n== ${name} ==`);
for (const input of inputs) {
try {
const out = schema.parse(input);
console.log(JSON.stringify(input), '=>', JSON.stringify(out));
} catch (e) {
const issues = e.issues?.map((i) => i.message) ?? [e.message];
console.log(JSON.stringify(input), '=> ERROR:', issues.join(' | '));
}
}
}
JSRepository: projectamazonph/amph-v2
Length of output: 1735
Trim before email validation email().trim() rejects padded addresses like User@Example.com. Move trimming before the email check, for example with z.string().trim().min(1).pipe(z.email()).transform((v) => v.toLowerCase()), and add a regression test for padded mixed-case input.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/actions/checkout.ts` at line 49, Update the email schema in the
checkout action to trim and validate non-empty input before applying email
validation, then lowercase the normalized value. Add a regression test covering
padded, mixed-case email input and verify it is accepted and normalized.
Source: Coding guidelines
| function makeSourceEvent(overrides: Record<string, unknown> = {}): any { | ||
| return { | ||
| data: { | ||
| id: 'evt-src-1', | ||
| attributes: { | ||
| type: 'source.chargeable', | ||
| data: { | ||
| id: 'src_test_001', | ||
| attributes: { | ||
| id: 'src_test_001', | ||
| amount: 299900, | ||
| currency: 'PHP', | ||
| status: 'chargeable', | ||
| type: 'gcash', | ||
| metadata: {}, | ||
| ...overrides, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Type the webhook fixture as SourceChargeableEvent.
Returning any prevents these tests from detecting drift in PayMongo's event contract.
Proposed fix
- function makeSourceEvent(overrides: Record<string, unknown> = {}): any {
+ function makeSourceEvent(
+ overrides: Record<string, unknown> = {},
+ ): SourceChargeableEvent {As per coding guidelines, “Use strict TypeScript. Do not use any; define explicit types or use unknown with narrowing.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function makeSourceEvent(overrides: Record<string, unknown> = {}): any { | |
| return { | |
| data: { | |
| id: 'evt-src-1', | |
| attributes: { | |
| type: 'source.chargeable', | |
| data: { | |
| id: 'src_test_001', | |
| attributes: { | |
| id: 'src_test_001', | |
| amount: 299900, | |
| currency: 'PHP', | |
| status: 'chargeable', | |
| type: 'gcash', | |
| metadata: {}, | |
| ...overrides, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }; | |
| } | |
| function makeSourceEvent( | |
| overrides: Record<string, unknown> = {}, | |
| ): SourceChargeableEvent { | |
| return { | |
| data: { | |
| id: 'evt-src-1', | |
| attributes: { | |
| type: 'source.chargeable', | |
| data: { | |
| id: 'src_test_001', | |
| attributes: { | |
| id: 'src_test_001', | |
| amount: 299900, | |
| currency: 'PHP', | |
| status: 'chargeable', | |
| type: 'gcash', | |
| metadata: {}, | |
| ...overrides, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/__tests__/enrollment.test.ts` around lines 521 - 542, Update
makeSourceEvent to return the explicit SourceChargeableEvent type instead of
any, and type its constructed fixture accordingly so TypeScript validates the
webhook contract while preserving the existing overrides behavior.
Source: Coding guidelines
| // Reject encoded schemes (%6A%61%76%61%73%63%72%69%70%74 = "javascript") | ||
| if (/^\/[^/]*%[0-9a-fA-F]{2}/.test(trimmed)) return fallback; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Allow safe percent-encoded internal paths.
The regex rejects any encoded byte in the first segment, so valid redirects such as /search%20results fall back to /. Decode and revalidate the path instead of rejecting all encoding.
src/lib/redirect-url.ts#L28-L29: narrowly reject decoded schemes, control characters, backslashes, and network paths.src/lib/__tests__/validation.test.ts#L113-L115: add a positive regression case for a safe encoded path.
📍 Affects 2 files
src/lib/redirect-url.ts#L28-L29(this comment)src/lib/__tests__/validation.test.ts#L113-L115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/redirect-url.ts` around lines 28 - 29, Replace the broad encoded-byte
rejection in the redirect validation around the path check with decoding
followed by narrow validation that rejects decoded schemes, control characters,
backslashes, and network paths while allowing safe encoded internal paths.
Update src/lib/redirect-url.ts lines 28-29 accordingly, and add a positive
regression test for a safe encoded path in src/lib/__tests__/validation.test.ts
lines 113-115.
|
Superseded by #46, which reconciles this PR with #34 (they conflict on 11 of 12 shared files) and fixes 10 bugs found reviewing the combined result — including this PR's dead-code guest-checkout claim-token flow (token was minted but never emailed). Recommend closing in favor of #46; don't merge both. Generated by Claude Code |
…st-review hardening (#46) Reconciles the two competing audit-remediation PRs (#33, #34) into one shippable change, fixes the broken Source-based checkout flow that main never handled, and adds post-review hardening: webhook idempotency + amount/currency validation, certificate P2002 race handling, refund audit logging, requireAdmin fail-closed, email canonicalization + collision safety, and the CI seed fix. Supersedes #33 and #34.
2 participants
Summary
This PR applies all fixes identified in the July 17 full-codebase audit of AMPH-v2. All 7 critical findings (C1-C7) and 7 high-severity findings (H1-H7) are addressed.
Critical fixes
handleSourceChargeableandhandlePaymentPaidwebhook handlers so the Source-based PayMongo flow actually creates Payment + Enrollment recordsvalidateRedirectUrl()— rejects javascript:, data:, //, backslash, control characters. Applied to sign-in, sign-up, and checkout-complete pagesHigh-severity fixes
requireAdmin()now loads role from DB instead of stale JWTdeletedAtcolumn)updateManybefore gradingmarkLessonCompleteActionreturns redirect URL instead of callingredirect()inside the safe action wrapperMedium fixes
confirmPasswordto validation schema.transform()Migration
Added
20260717000001_claim_token— addsclaimTokenHashandclaimTokenExpiresAtcolumns to User model.Summary by CodeRabbit
checkout_idand improved safe return URL handling.