(P1: 3) [Security/Infra] Enforce origin posture, harden DB integrity constraints, and finalize shop observability safety (no console, correlation IDs)#179
Conversation
…ut same-origin; internal/webhooks non-browser) + docs
…cross all shop routes; purge console.*; enforce explicit error codes
liudmylasovetovs
requested review from
AM1007 and
ViktorSvertoka
as code owners
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds origin-posture enforcement (browser same-origin and non-browser guards), CSRF gating, request-scoped tracing, standardized no-store JSON responses, expanded structured logging, tests and test-helper updates, and a DB migration to add ON DELETE CASCADE for stripe_events.order_id. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant NextAPI as Next.js API
participant Guard as Origin Guard
participant CSRF as CSRF Checker
participant Handler as Route Handler
participant DB as Database
participant Resp as Response
Browser->>NextAPI: POST /admin/... (Origin header)
NextAPI->>Guard: guardBrowserSameOrigin(request)
Guard->>Guard: validate Origin vs allowlist
alt Origin blocked
Guard->>Resp: 403 ORIGIN_NOT_ALLOWED (no-store)
Resp->>Browser: 403 no-store
else Origin allowed
NextAPI->>CSRF: requireAdminCsrf(request)
alt CSRF invalid
CSRF->>Resp: 403 CSRF_REJECTED (no-store)
Resp->>Browser: 403 no-store
else CSRF valid
NextAPI->>Handler: invoke handler (with requestId, start time)
Handler->>DB: perform DB operations
DB->>Handler: result / error
Handler->>Resp: 200/4xx/5xx via noStoreJson
Resp->>Browser: response (no-store)
end
end
sequenceDiagram
participant ExternalJob
participant NextAPI as Next.js API
participant Guard as Non-Browser Guard
participant Handler as Job Handler
participant DB as Database
participant Resp as Response
ExternalJob->>NextAPI: POST /internal/restock-stale (no browser headers)
NextAPI->>Guard: guardNonBrowserOnly(request)
Guard->>Guard: check Origin / Sec-Fetch-Site absence
alt Browser context detected
Guard->>Resp: 403 BROWSER_CONTEXT_NOT_ALLOWED (no-store)
Resp->>ExternalJob: 403 no-store
else Verified non-browser
NextAPI->>Handler: validate payload & generate requestId
alt Invalid payload
Handler->>Resp: 400 INVALID_PAYLOAD (no-store)
Resp->>ExternalJob: 400 no-store
else Valid
Handler->>DB: run restock job
DB->>Handler: results
Handler->>Resp: 200 result (no-store)
Resp->>ExternalJob: 200 no-store
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@frontend/.env.example`:
- Around line 2-6: The dotenv example file has APP_ORIGIN and
APP_ADDITIONAL_ORIGINS out of order causing dotenv-linter warnings; reorder the
Core/Environment block so keys follow the linter's expected sequence (place
APP_URL and NEXT_PUBLIC_SITE_URL before APP_ENV if required by your linter
conventions or move APP_ORIGIN and APP_ADDITIONAL_ORIGINS to the proper
position) — update the APP_ENV, APP_URL, NEXT_PUBLIC_SITE_URL, APP_ORIGIN,
APP_ADDITIONAL_ORIGINS keys in .env.example to match the dotenv-linter expected
ordering.
In `@frontend/app/api/shop/catalog/route.ts`:
- Around line 30-34: The normalizeLocale function treats uppercase/mixed inputs
as non-UK and falls back to 'en'; to fix, normalize the input to lowercase
before checks: in normalizeLocale, compute the raw string by trimming and then
calling toLowerCase() on the input (e.g., const raw = typeof input === 'string'
? input.trim().toLowerCase() : ''); then keep the existing checks (raw === 'uk'
|| raw.startsWith('uk-')) to return 'uk' and return 'en' otherwise.
In `@frontend/app/api/shop/internal/orders/restock-stale/route.ts`:
- Around line 282-289: The current log uses a hardcoded 'UNAUTHORIZED' even
though requireInternalJanitorAuth(request) may return different statuses
(401/403/503); change the block that handles authRes to derive the actual HTTP
status from authRes (e.g., authRes.status || authRes.statusCode) and set that
value as the code field passed to logWarn (use a fallback like String(status) or
'UNAUTHORIZED' if missing), keeping the same variables (authRes, baseMeta,
logWarn) and returning authRes as before.
In `@frontend/lib/security/rate-limit.ts`:
- Around line 110-125: The .env.example is missing the TRUST_CF_CONNECTING_IP
flag which is referenced by getClientIpFromHeaders; add a line
TRUST_CF_CONNECTING_IP=0 to .env.example next to the existing
TRUST_FORWARDED_HEADERS entry and include a short security note explaining that
enabling it trusts Cloudflare's cf-connecting-ip header (only set for Cloudflare
fronted deployments) and that it should remain 0 in untrusted environments to
avoid IP spoofing.
🧹 Nitpick comments (15)
frontend/lib/security/origin.ts (1)
5-14: Consider setting no-store in origin error responses.
Centralizing Cache-Control here avoids relying on each caller.♻️ Suggested tweak
function buildErrorResponse(code: string, message: string) { - return NextResponse.json( + const res = NextResponse.json( { error: { code, message, }, }, { status: 403 } ); + res.headers.set('Cache-Control', 'no-store'); + return res; }frontend/app/api/shop/admin/orders/[id]/refund/route.ts (1)
131-169: Avoid double‑logging known errors as error‑level.
OrderNotFoundErrorandInvalidPayloadErrorare already handled; logging them as errors first can inflate error metrics. Consider logging errors only for unexpected cases.♻️ Suggested adjustment
- logError('admin_orders_refund_failed', error, { - ...baseMeta, - orderId: orderIdForLog, - code: 'ADMIN_REFUND_FAILED', - durationMs: Date.now() - startedAtMs, - }); - if (error instanceof OrderNotFoundError) { logWarn('admin_orders_refund_not_found', { ...baseMeta, code: error.code, orderId: orderIdForLog, durationMs: Date.now() - startedAtMs, }); return noStoreJson( { error: error.message, code: error.code }, { status: 404 } ); } if (error instanceof InvalidPayloadError) { logWarn('admin_orders_refund_invalid_payload', { ...baseMeta, code: error.code, orderId: orderIdForLog, durationMs: Date.now() - startedAtMs, }); return noStoreJson( { error: error.message, code: error.code }, { status: 400 } ); } + logError('admin_orders_refund_failed', error, { + ...baseMeta, + orderId: orderIdForLog, + code: 'ADMIN_REFUND_FAILED', + durationMs: Date.now() - startedAtMs, + }); return noStoreJson( { error: 'Unable to refund order', code: 'INTERNAL_ERROR' }, { status: 500 } );frontend/app/api/shop/admin/orders/reconcile-stale/route.ts (1)
9-9: Use info-level logging for successful reconcile
A normal success path should not emit warnings; considerlogInfoto keep warn signal meaningful.♻️ Suggested change
-import { logError, logWarn } from '@/lib/logging'; +import { logError, logInfo, logWarn } from '@/lib/logging'; ... - logWarn('admin_reconcile_stale_succeeded', { + logInfo('admin_reconcile_stale_succeeded', { ...baseMeta, code: 'OK', processed, olderThanMinutes: DEFAULT_STALE_MINUTES, durationMs: Date.now() - startedAtMs, });Also applies to: 98-104
frontend/app/api/shop/webhooks/stripe/route.ts (3)
575-591: Consider extracting charge ID resolution into a helper function.The deeply nested ternary for
bestEffortRefundChargeIdis functionally correct but hard to read. Since this pattern (extracting ID from string or object) appears multiple times in this file, a small helper would improve maintainability.♻️ Suggested refactor
function extractStripeId(value: unknown): string | null { if (typeof value === 'string' && value.trim().length > 0) return value.trim(); if (value && typeof value === 'object' && 'id' in value && typeof (value as any).id === 'string') { return (value as any).id; } return null; } // Usage: const bestEffortRefundChargeId = refundObject ? extractStripeId((refundObject as any).charge) : null;
438-443: RedundantCache-Controlheader setting.The
rateLimitResponse()helper already setsCache-Control: no-store(seefrontend/lib/security/rate-limit.tslines 263-264). The additionalres.headers.set('Cache-Control', 'no-store')on line 442 is redundant.♻️ Suggested removal
const res = rateLimitResponse({ retryAfterSeconds: decision.retryAfterSeconds, details: { scope: 'stripe_webhook', reason: 'missing_signature' }, }); - res.headers.set('Cache-Control', 'no-store'); return res;
413-418: Minor formatting inconsistency.Line 417 has unusual indentation (
return res;) compared to the rest of the file. This appears to be a typo.♻️ Suggested fix
const res = noStoreJson( { error: 'invalid_payload', code: 'INVALID_PAYLOAD' }, { status: 400 } ); - return res; + return res;frontend/drizzle/0005_modern_bromley.sql (1)
9-16: Consider wrapping in explicit transaction for safety.The
DROP CONSTRAINTandADD CONSTRAINTare separate statements. If the migration runner doesn't wrap this in a transaction and theADD CONSTRAINTfails (e.g., orphanedorder_idreferences), the constraint will be permanently lost. Drizzle typically runs migrations in transactions, but an explicit block adds defense-in-depth.♻️ Explicit transaction wrapper
DO $$ BEGIN IF EXISTS ( SELECT 1 FROM pg_constraint c WHERE c.conname = 'stripe_events_order_id_orders_id_fk' AND c.conrelid = 'public.stripe_events'::regclass AND pg_get_constraintdef(c.oid) NOT ILIKE '%ON DELETE CASCADE%' ) THEN + -- Wrap in savepoint for safety within DO block ALTER TABLE public.stripe_events DROP CONSTRAINT stripe_events_order_id_orders_id_fk; ALTER TABLE public.stripe_events ADD CONSTRAINT stripe_events_order_id_orders_id_fk FOREIGN KEY (order_id) REFERENCES public.orders(id) ON DELETE CASCADE; END IF; END $$;Note: DO blocks are already atomic in PostgreSQL, so this is actually safe. The entire block runs as a single transaction.
frontend/lib/tests/origin-posture.test.ts (1)
30-37: Consider adding test forAPP_ADDITIONAL_ORIGINS.The tests stub
APP_ADDITIONAL_ORIGINSas empty. Consider adding a test case that verifies requests from an additional origin are also allowed, to ensure the multi-origin allowlist logic works correctly.📝 Suggested additional test
it('guardBrowserSameOrigin allows POST with additional allowed Origin', () => { vi.stubEnv('APP_ADDITIONAL_ORIGINS', 'https://staging.example.com,https://preview.example.com'); const req = makeReq({ method: 'POST', headers: { origin: 'https://staging.example.com' }, }); const res = guardBrowserSameOrigin(req); expect(res).toBeNull(); });frontend/docs/security/origin-posture.md (1)
44-46: Consider adding links to implementation files.Adding references to the actual guard implementations (
frontend/lib/security/origin.ts) and example route handlers would help developers quickly navigate to the code.📝 Suggested addition
## Implementation references - Guard utilities: `frontend/lib/security/origin.ts` - Tests: `frontend/lib/tests/origin-posture.test.ts` - Example browser-exposed route: `frontend/app/api/shop/checkout/route.ts` - Example non-browser route: `frontend/app/api/shop/webhooks/stripe/route.ts`frontend/app/api/shop/admin/products/[id]/status/route.ts (1)
125-149: Avoid error-level logging for known 404s.
PRODUCT_NOT_FOUNDis an expected outcome; logging it as an error adds noise before the warn + 404 response. Consider handling it before the generic error log.🔧 Proposed reorder
- logError('admin_product_status_failed', error, { - ...baseMeta, - code: 'ADMIN_PRODUCT_STATUS_FAILED', - productId: productIdForLog, - durationMs: Date.now() - startedAtMs, - }); - if (error instanceof Error && error.message === 'PRODUCT_NOT_FOUND') { logWarn('admin_product_status_not_found', { ...baseMeta, code: 'PRODUCT_NOT_FOUND', productId: productIdForLog, durationMs: Date.now() - startedAtMs, }); return noStoreJson( { error: 'Product not found', code: 'PRODUCT_NOT_FOUND' }, { status: 404 } ); } + + logError('admin_product_status_failed', error, { + ...baseMeta, + code: 'ADMIN_PRODUCT_STATUS_FAILED', + productId: productIdForLog, + durationMs: Date.now() - startedAtMs, + });frontend/app/api/shop/checkout/route.ts (1)
63-129: Add no-store headers to checkout responses (including origin-blocked).Checkout responses can include sensitive payment/order metadata; explicitly setting
Cache-Control: no-storekeeps them out of intermediary caches and aligns with other shop endpoints.🔧 Proposed update
function errorResponse( code: string, message: string, status: number, details?: unknown ) { - return NextResponse.json( + const res = NextResponse.json( { code, message, ...(details === undefined ? {} : { details }), }, { status } ); + res.headers.set('Cache-Control', 'no-store'); + return res; } @@ function buildCheckoutResponse({ order, itemCount, clientSecret, status, }: { order: CheckoutOrderShape; itemCount: number; clientSecret: string | null; status: number; }) { - return NextResponse.json( + const res = NextResponse.json( { success: true, order: { id: order.id, currency: order.currency, totalAmount: order.totalAmount, itemCount, paymentStatus: order.paymentStatus, paymentProvider: order.paymentProvider, paymentIntentId: order.paymentIntentId, clientSecret, }, orderId: order.id, paymentStatus: order.paymentStatus, paymentProvider: order.paymentProvider, paymentIntentId: order.paymentIntentId, clientSecret, }, { status } ); + res.headers.set('Cache-Control', 'no-store'); + return res; } @@ if (blocked) { logWarn('checkout_origin_blocked', { ...baseMeta, code: 'ORIGIN_BLOCKED', }); - - return blocked; + blocked.headers.set('Cache-Control', 'no-store'); + return blocked; }Also applies to: 167-175
frontend/app/api/shop/admin/orders/[id]/route.ts (1)
37-48: Origin guard is a no-op for GET requests.According to
guardBrowserSameOriginimplementation, it returnsnullfor GET and HEAD methods (safe requests don't require origin validation). This block will never execute for this GET handler, making it dead code.Consider removing this guard for GET, or clarify if there's an intent to change
guardBrowserSameOriginbehavior in the future.irs.frontend/app/api/shop/admin/products/[id]/route.ts (3)
115-126: Origin guard is a no-op for GET requests.Same as the orders route -
guardBrowserSameOriginreturnsnullfor GET/HEAD methods, making this block unreachable.
326-420: Duplicate sale rule validation is redundant.Sale rules are validated twice:
- Lines 326-370:
getSaleViolationFromFormData(formData)validates raw form data- Lines 395-420:
findSaleRuleViolation(parsed.data)validates afterparseAdminProductFormIf
parseAdminProductFormdoesn't transform the sale/price data, the second check will always pass if the first did. Consider removing the first validation to simplify the code, or document why both are needed (e.g., if prices can be modified during parsing).
374-376: Type assertion could be cleaner.The complex cast
((parsed.error as any)?.issues?.length as number | undefined) ?? 0suggests the error type fromparseAdminProductFormlacks proper typing. Consider adding a typed error structure or using Zod's typed error format.
| APP_ENV= | ||
| APP_URL= | ||
| NEXT_PUBLIC_SITE_URL= | ||
| APP_ORIGIN=https://example.test | ||
| APP_ADDITIONAL_ORIGINS=https://admin.example.test |
There was a problem hiding this comment.
Align key order with dotenv-linter to avoid CI warnings.
The linter flags APP_ORIGIN and APP_ADDITIONAL_ORIGINS ordering. Consider reordering the Core/Environment block.
🧹 Proposed reorder
-APP_ENV=
-APP_URL=
-NEXT_PUBLIC_SITE_URL=
-APP_ORIGIN=https://example.test
-APP_ADDITIONAL_ORIGINS=https://admin.example.test
+APP_ADDITIONAL_ORIGINS=https://admin.example.test
+APP_ENV=
+APP_ORIGIN=https://example.test
+APP_URL=
+NEXT_PUBLIC_SITE_URL=📝 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.
| APP_ENV= | |
| APP_URL= | |
| NEXT_PUBLIC_SITE_URL= | |
| APP_ORIGIN=https://example.test | |
| APP_ADDITIONAL_ORIGINS=https://admin.example.test | |
| APP_ADDITIONAL_ORIGINS=https://admin.example.test | |
| APP_ENV= | |
| APP_ORIGIN=https://example.test | |
| APP_URL= | |
| NEXT_PUBLIC_SITE_URL= |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 5-5: [UnorderedKey] The APP_ORIGIN key should go before the APP_URL key
(UnorderedKey)
[warning] 6-6: [UnorderedKey] The APP_ADDITIONAL_ORIGINS key should go before the APP_ENV key
(UnorderedKey)
🤖 Prompt for AI Agents
In `@frontend/.env.example` around lines 2 - 6, The dotenv example file has
APP_ORIGIN and APP_ADDITIONAL_ORIGINS out of order causing dotenv-linter
warnings; reorder the Core/Environment block so keys follow the linter's
expected sequence (place APP_URL and NEXT_PUBLIC_SITE_URL before APP_ENV if
required by your linter conventions or move APP_ORIGIN and
APP_ADDITIONAL_ORIGINS to the proper position) — update the APP_ENV, APP_URL,
NEXT_PUBLIC_SITE_URL, APP_ORIGIN, APP_ADDITIONAL_ORIGINS keys in .env.example to
match the dotenv-linter expected ordering.
…mobile cards UI; tighten env/docs, locale normalization, cache-control, and logging semantics
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/api/shop/admin/orders/`[id]/route.ts:
- Around line 49-59: The CSRF check is being applied unconditionally in the
admin order GET handler via requireAdminCsrf (and its csrfRes handling/logWarn
with code 'CSRF_REJECTED'), which is unnecessary for safe/read-only GETs; update
the logic in route.ts so that you only call requireAdminCsrf (and execute the
csrfRes.headers.set('Cache-Control', 'no-store') / return) for non-GET methods
(e.g., when request.method !== 'GET') or remove the CSRF check path entirely for
GET requests while keeping it for POST/PUT/PATCH/DELETE, leaving logWarn and the
existing metadata in place for rejected state-changing requests.
🧹 Nitpick comments (11)
frontend/app/api/shop/admin/products/[id]/status/route.ts (2)
19-23: Consider extractingnoStoreJsonto a shared utility.Per the AI summary, this pattern is used across multiple shop routes. Extracting it to a shared module (e.g.,
@/lib/responses) would reduce duplication and ensure consistent behavior.
125-137: String-based error check is fragile.Using
error.message === 'PRODUCT_NOT_FOUND'for error identification is brittle—if the message changes intoggleProductStatus, this check silently fails and falls through to the 500 handler.Consider creating a custom error class (similar to
AdminUnauthorizedError) for type-safe error handling:♻️ Suggested refactor
// In a shared errors module (e.g., `@/lib/errors/products.ts`) export class ProductNotFoundError extends Error { code = 'PRODUCT_NOT_FOUND' as const; constructor(productId: string) { super(`Product not found: ${productId}`); this.name = 'ProductNotFoundError'; } }Then update the check:
- if (error instanceof Error && error.message === 'PRODUCT_NOT_FOUND') { + if (error instanceof ProductNotFoundError) {frontend/lib/services/products/mutations/delete.ts (1)
2-46: Prefer returningimagePublicIdfrom the delete to avoid stale cleanup and an extra round‑trip.The current pre-read can become stale if the product is updated between the select and delete. Returning
imagePublicIdfrom the delete makes the cleanup consistent with the deleted row and removes the extra query.♻️ Proposed refactor
-import { eq, sql } from 'drizzle-orm'; +import { sql } from 'drizzle-orm'; @@ - const [existing] = await db - .select({ id: products.id, imagePublicId: products.imagePublicId }) - .from(products) - .where(eq(products.id, id)) - .limit(1); - - if (!existing) { - throw new Error('PRODUCT_NOT_FOUND'); - } - // Atomic delete: prices first, then product, all-or-nothing. const result = await db.execute(sql` WITH del_prices AS ( DELETE FROM ${productPrices} WHERE ${productPrices.productId} = ${id} ), del_product AS ( DELETE FROM ${products} WHERE ${products.id} = ${id} - RETURNING ${products.id} AS id + RETURNING ${products.id} AS id, ${products.imagePublicId} AS imagePublicId ) - SELECT id FROM del_product; + SELECT id, imagePublicId FROM del_product; `); - const rows = - (result as unknown as { rows?: Array<{ id: string }> }).rows ?? []; - if (rows.length === 0) { + const rows = + (result as unknown as { + rows?: Array<{ id: string; imagePublicId: string | null }>; + }).rows ?? []; + const [deleted] = rows; + if (!deleted) { // concurrent delete edge-case throw new Error('PRODUCT_NOT_FOUND'); } - if (existing.imagePublicId) { + if (deleted.imagePublicId) { try { - await destroyProductImage(existing.imagePublicId); + await destroyProductImage(deleted.imagePublicId); } catch (error) { logError('Failed to cleanup product image after delete', error); } }frontend/app/[locale]/shop/admin/orders/page.tsx (1)
61-292: Reduce duplication between mobile and desktop render paths.Both views recompute the same currency/total formatting; a small view‑model extraction avoids drift and simplifies future changes.
♻️ Example refactor
const hasNext = all.length > PAGE_SIZE; const items = all.slice(0, PAGE_SIZE); + const viewModels = items.map(order => { + const currency = orderCurrency(order, locale); + const totalMinor = pickMinor(order?.totalAmountMinor, order?.totalAmount); + const totalFormatted = + totalMinor === null ? '-' : formatMoney(totalMinor, currency, locale); + return { order, totalFormatted }; + }); @@ - {items.map(order => { - const currency = orderCurrency(order, locale); - const totalMinor = pickMinor( - order?.totalAmountMinor, - order?.totalAmount - ); - const totalFormatted = - totalMinor === null - ? '-' - : formatMoney(totalMinor, currency, locale); + {viewModels.map(({ order, totalFormatted }) => { return ( @@ - return ( + return ( <tr key={order.id} className="hover:bg-muted/50"> @@ - {totalFormatted} + {totalFormatted} </td>frontend/lib/security/origin.ts (1)
22-24: Consider validating URL format innormalizeOrigin.The function only trims and removes trailing slashes, but doesn't validate that the input is a valid origin URL. Malformed origins could pass through.
💡 Optional: Add basic URL validation
export function normalizeOrigin(input: string): string { const trimmed = input.trim().replace(/\/+$/, ''); // Optional: validate it looks like a valid origin try { const url = new URL(trimmed); return url.origin; } catch { return trimmed; // fallback to original behavior } }Using
URL.originensures consistent normalization (e.g., handling default ports).frontend/app/api/shop/webhooks/stripe/route.ts (1)
63-69: OPTIONS handler should also reject browser context.The OPTIONS handler doesn't use
guardNonBrowserOnly, which means browsers could send preflight requests. While this returns 405 anyway, for consistency with the documented posture (non-browser endpoints reject browser context), consider adding the guard.💡 Optional: Add origin guard to OPTIONS
-export function OPTIONS() { - const res = noStoreJson( +export function OPTIONS(request: NextRequest) { + const blocked = guardNonBrowserOnly(request); + if (blocked) { + blocked.headers.set('Cache-Control', 'no-store'); + return blocked; + } + + return noStoreJson( { error: 'METHOD_NOT_ALLOWED', code: 'METHOD_NOT_ALLOWED' }, { status: 405, headers: { Allow: 'POST' } } ); - return res; }frontend/app/[locale]/shop/admin/products/page.tsx (1)
54-68: Consider using schema references instead of raw table names in SQL.The
isInUsequery uses hardcoded table names (order_items,inventory_moves) rather than the exported schema objects. UsingorderItemsandinventoryMovesfrom@/db/schemawould provide a single source of truth and prevent drift if table names change.💡 Suggested approach
Update the query to use schema references:
+import { orderItems, inventoryMoves } from '@/db/schema'; + isInUse: sql<boolean>` ( exists ( select 1 - from order_items oi + from ${orderItems} oi where oi.product_id = ${products.id} ) OR exists ( select 1 - from inventory_moves im + from ${inventoryMoves} im where im.product_id = ${products.id} ) ) `,frontend/app/api/shop/admin/orders/reconcile-stale/route.ts (1)
52-65: Clarify the purpose of the dual origin checks or consolidate if redundant.The
isSameOrigincheck on line 57 performs similar origin validation toguardBrowserSameOrigincalled on line 33. IfguardBrowserSameOriginalready restricts requests to allowed origins, the subsequentisSameOrigincheck may be redundant in most configurations. Consider documenting why both are necessary (e.g., different validation semantics for CSRF defense), or if safe to do so, consolidate into a single check to reduce complexity. Note that other admin routes userequireAdminCsrffor full CSRF token verification instead of this manual pattern.frontend/app/api/shop/checkout/route.ts (1)
227-231: Consider documenting the truncation rationale foridempotencyKeyShort.The key is truncated to 32 characters for logging. While this is reasonable to avoid log bloat, adding a brief comment explaining this would clarify intent for future maintainers.
frontend/app/api/shop/admin/products/[id]/route.ts (2)
30-36: Consider extractingnoStoreJsonto a shared utility.This helper is duplicated across multiple route files (checkout, admin routes). Centralizing it in a shared location (e.g.,
@/lib/api/responses) would reduce duplication and ensure consistent behavior.
454-464: Type assertion onparsed.datacould be avoided.The cast
parsed.data as UpdateProductInputsuggests a potential type mismatch betweenparseAdminProductFormoutput andupdateProductinput. If types align, the assertion shouldn't be needed.
…s (order_items/inventory_moves)
2 participants
Description
This PR hardens the Shop platform’s safety rails across three areas: origin/caller posture, database integrity, and observability. It documents and enforces the expected request origins for browser-exposed endpoints, aligns Drizzle schema/snapshots to match real Postgres constraints to prevent drift, and completes structured logging coverage across the Shop API surface (including correlation IDs and explicit error codes) while removing
console.*usage.Related Issue
Issue: #<issue_number>
Changes
stripe_events.order_id → orders.idto be consistently ON DELETE CASCADE with an idempotent guard.requestId+ base metadata + explicitcodefields and added correlation identifiers where applicable (e.g.,orderId,attemptId,stripeEventId,providerRef), and purged remainingconsole.*.Database Changes (if applicable)
How Has This Been Tested?
Screenshots (if applicable)
N/A (backend/security/infra changes)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.