feat(account): super-admin read-only "view as" (customer impersonation)#262
Open
kilbot wants to merge 14 commits into
Open
feat(account): super-admin read-only "view as" (customer impersonation)#262kilbot wants to merge 14 commits into
kilbot wants to merge 14 commits into
Conversation
getSessionCustomer() is the real logged-in owner identity, reachable independently of getCustomer's cached seam. Behavior unchanged in this task; a later task adds an impersonation branch to getCustomer.
…callback, stale-target auto-exit
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
kilbot has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds admin impersonation support, routes reads through the impersonated customer where applicable, blocks mutating routes during impersonation, and updates middleware, account UI, and exit/start flows to manage impersonation state. ChangesAdmin Impersonation Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant AdminPage
participant startImpersonationAction
participant isAdmin
participant findAdminCustomerByEmail
participant startImpersonation
AdminPage->>startImpersonationAction: submit target email
startImpersonationAction->>isAdmin: verify real session
startImpersonationAction->>findAdminCustomerByEmail: resolve target customer
alt not found
startImpersonationAction-->>AdminPage: {error: not_found}
else found
startImpersonationAction->>startImpersonation: set impersonation cookie
startImpersonationAction-->>AdminPage: redirect /account
end
sequenceDiagram
participant Middleware
participant getImpersonation
participant getCustomer
participant AdminAPI
participant SessionAPI
Middleware->>Middleware: strip spoofed account-request header
Middleware->>Middleware: stamp header only for account-scoped paths
getImpersonation->>getImpersonation: read header, cookie, and session
getImpersonation-->>getCustomer: impersonation state
alt impersonating
getCustomer->>AdminAPI: getAdminCustomerById(targetId)
else not impersonating
getCustomer->>SessionAPI: getSessionCustomer()
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🚀 Preview: https://wcpos-rn4yksq3c-wcpos.vercel.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
src/middleware.test.ts (1)
137-150: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd regression coverage for early-return non-account routes.
This test only exercises the generic
/api/*branch. Please also cover/?wc-api=am-software-apiandupdates.wcpos.com/api/*, since those branches can bypass the sanitized header forwarding path.🤖 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/middleware.test.ts` around lines 137 - 150, The current middleware test only covers the generic /api/* path, so add regression coverage for the other early-return non-account routes that may bypass the sanitized header forwarding path. Extend middleware.test.ts with cases for the root ?wc-api=am-software-api route and updates.wcpos.com/api/*, using middleware() and the ACCOUNT_REQUEST_HEADER assertions to verify the client-supplied header is still stripped from the forwarded NextResponse.next({ request }) headers.src/lib/impersonation.test.ts (1)
65-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the cookie-delete-throws branch (lines 52-56 in impersonation.ts).
This test verifies
cookieStore.deleteis called when session isn't admin, but doesn't exercise thecatchpath wheredelete()throws (the real-world RSC scenario per Next.js docs, since.set/.deleteare only supported in Server Functions/Route Handlers). Add a case wherecookieStore.deletethrows to confirmgetImpersonationstill resolves tonullwithout propagating the error.Suggested additional test
+ it('returns null even if clearing the cookie throws (read-only RSC context)', async () => { + cookieStore.value = 'cus_target' + headerStore.value = '1' + cookieStore.delete.mockImplementation(() => { + throw new Error('Cookies can only be modified in a Server Action or Route Handler') + }) + getSessionCustomer.mockResolvedValue({ email: 'attacker@evil.com' }) + await expect(getImpersonation()).resolves.toBeNull() + })🤖 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/impersonation.test.ts` around lines 65 - 71, Add test coverage for the error-handling path in getImpersonation where cookieStore.delete throws in the non-admin branch. Extend the existing impersonation tests to mock cookieStore.delete throwing, then verify getImpersonation still returns null and does not propagate the exception, using the getImpersonation function and cookieStore delete mock to locate the behavior.src/lib/impersonation.ts (1)
70-75: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider defense-in-depth: verify admin status inside
startImpersonationtoo.The function trusts the doc comment's precondition ("already verified the caller is an admin") without re-checking. If a future caller invokes this without that check, there's no internal guard against a non-admin session setting the cookie.
♻️ Optional defensive check
+import { getSessionCustomer } from '`@/lib/medusa-auth`' +import { isAdmin } from '`@/lib/admin`' + export async function startImpersonation(targetId: string): Promise<void> { + const session = await getSessionCustomer() + if (!isAdmin(session?.email)) throw new Error('Not authorized to impersonate') const cookieStore = await cookies() cookieStore.set(IMPERSONATION_COOKIE, targetId, IMPERSONATION_COOKIE_OPTIONS) }🤖 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/impersonation.ts` around lines 70 - 75, `startImpersonation` currently relies only on the caller’s precondition and can set `IMPERSONATION_COOKIE` without verifying the session is an admin. Add an internal admin check at the start of `startImpersonation` in `src/lib/impersonation.ts` before calling `cookies()` and setting the cookie, using the existing auth/admin validation path already used by the admin action so the function is safe even if called directly.src/lib/customer-orders.ts (1)
188-203: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid paging the full impersonation order list here.
fetchImpersonatedOrders()goes throughlistAdminCustomerOrders, which can walk up to 5,000 orders to find one match. The admin/admin/ordersendpoint acceptsid, so this path can use a single filtered lookup instead.🤖 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/customer-orders.ts` around lines 188 - 203, The getOrderById path is unnecessarily loading the full impersonation order list through fetchImpersonatedOrders/listAdminCustomerOrders. Update getOrderById to use a single filtered admin orders lookup when impersonated access is active, passing the orderId through the existing fetch/admin order retrieval flow instead of scanning all impersonated orders. Keep the fallback behavior with getAuthToken and fetchOrders unchanged, and use the existing getOrderById, fetchImpersonatedOrders, and fetchOrders symbols to locate the logic.src/app/api/account/licenses/[licenseId]/discord/members/[memberId]/route.test.ts (1)
11-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard mock added, but the 403 branch itself is untested here.
This mock only keeps existing tests passing; no test asserts
DELETEreturns 403read_only_inspectionwhenassertViewOnlyrejects withViewOnlyError(unlikesrc/app/api/account/profile/route.test.ts, which added an explicit case for this). Consider adding an analogous test, e.g. mockingassertViewOnlyto reject once and asserting the response status/body.🤖 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/api/account/licenses/`[licenseId]/discord/members/[memberId]/route.test.ts around lines 11 - 15, The current `route.test.ts` only mocks `assertViewOnly` in `@/lib/impersonation`, but it does not cover the `DELETE` 403 `read_only_inspection` branch. Add an explicit test for the `DELETE` handler in this route that makes `assertViewOnly` reject with `ViewOnlyError`, then assert the response status and body match the expected 403 `read_only_inspection` behavior, similar to the pattern used in `profile/route.test.ts`.src/app/[locale]/account/admin/actions.test.ts (1)
1-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage of forbidden/not_found/success paths.
Consider adding a case for the
rate_limitedbranch (Line 40 inactions.ts) since it's currently untested.🤖 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/`[locale]/account/admin/actions.test.ts around lines 1 - 57, Add test coverage for the rate-limited path in startImpersonationAction: mock the rate limiter so consume() returns a failed result, then assert the action returns the rate_limited error and does not call findAdminCustomerByEmail or startImpersonation. Keep the new test alongside the existing startImpersonationAction cases in actions.test.ts and use the existing mocked createRateLimiter/clientIp setup to target this branch.src/app/[locale]/account/admin/page.tsx (1)
13-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winResult of
startImpersonationActionis silently discarded.On
forbidden/not_found/rate_limited, the form just re-renders with no feedback — the admin can't tell why "View as" did nothing. The code comment already flags this as a known follow-up (useActionState).Want me to wire this page up with
useActionStateto surface the error to the admin now?🤖 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/`[locale]/account/admin/page.tsx around lines 13 - 19, The submit flow in the admin account page is discarding the result from startImpersonationAction, so forbidden/not_found/rate_limited cases re-render without any admin-visible feedback. Update the submit/use server flow in page.tsx to use useActionState (or equivalent action state handling) so the returned error is stored and rendered inline, and keep the existing redirect-on-success behavior from startImpersonationAction.
🤖 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/`[locale]/account/admin/actions.ts:
- Around line 32-40: The rate limit key in the admin action is being derived
from untrusted forwarded headers via clientIp() on a synthetic Request, which
allows spoofing. Update the action to use a trusted proxy-derived client address
or validate the forwarded headers before passing them to the limiter, and prefer
passing the existing Headers object into clientIp() instead of constructing a
new Request. Keep the fix localized around getSessionCustomer(), isAdmin(),
clientIp(), and limiter.consume().
- Around line 42-51: The impersonation redirect currently uses a bare account
path, which drops the active locale and can send users to the default-language
page. Update the redirect logic in the impersonation start flow (the action that
calls startImpersonation and redirect) and the impersonation exit route to
preserve or pass through locale, either by threading locale into the target path
or by using the locale-aware redirect helper already used elsewhere. Keep the
redirect destination explicitly locale-prefixed so users return to the same
locale they came from.
In
`@src/app/api/account/licenses/`[licenseId]/discord/members/[memberId]/route.ts:
- Line 13: The view-only authorization guard is duplicated across this route and
the other listed handlers, so extract the repeated try/catch logic into a shared
helper in the impersonation utilities and reuse it everywhere. Add a helper
alongside assertViewOnly and ViewOnlyError in src/lib/impersonation.ts that
performs the guard and throws the same 403 response shape, then update the route
handler here (and the matching profile, machines, cart, and payment session
routes) to call that helper instead of inlining the block.
In `@src/app/api/store/cart/complete/route.ts`:
- Line 6: The store cart completion route is not protected by the
impersonation/view-only guard because `assertViewOnly()` depends on
`x-wcpos-account-request`, which is only set by `src/middleware.ts` for account
APIs. Update the middleware scope to stamp that trusted header for the store
cart/checkout API paths as well, or add a direct impersonation check inside
`route.ts` before completing the cart; use `assertViewOnly` and the
`/api/store/cart/complete` handler as the key touchpoints.
In `@src/lib/customer-orders.ts`:
- Around line 95-106: fetchImpersonatedOrders currently lets
listAdminCustomerOrders throw, which can bubble up and crash
getOrdersPage/getAllOrders/getOrderById under impersonation. Mirror the error
handling used in fetchOrders: wrap the admin API call in a try/catch, log or
otherwise handle the failure, and return a safe fallback such as null or an
empty order result so impersonated reads degrade gracefully instead of throwing.
In `@src/lib/discord/medusa-admin.ts`:
- Around line 99-134: Align the error handling in findAdminCustomerByEmail with
getAdminCustomerById so a medusaAdminFetch failure does not escape as an
unhandled exception from startImpersonationAction callers. Add a try/catch
around the admin/customers lookup in findAdminCustomerByEmail, return null on
failure to match the lookup contract, and use the same customer-fetch symbols
(medusaAdminFetch, findAdminCustomerByEmail, getAdminCustomerById) to keep both
lookup paths consistent. Also update getAdminCustomerById’s catch to log the
failure before returning null so real admin API outages are distinguishable from
a missing customer.
- Around line 99-115: The customer lookup in findAdminCustomerByEmail currently
returns the first Medusa result, which can pick a guest record when the same
email exists for both guest and registered customers. Update this function to
prefer a customer with has_account set to true before falling back to any
remaining match, using the existing medusaAdminFetch and page.customers data to
disambiguate safely. Ensure the returned MedusaCustomer from
findAdminCustomerByEmail reflects the registered profile when duplicates exist.
In `@src/middleware.ts`:
- Around line 163-178: The account-request handling in `middleware` is too broad
because `pathnameWithoutLocale.startsWith('/account')` also matches routes like
`/accounting`. Update the path check to only cover the `/account` segment
boundary by using the same `pathnameWithoutLocale` guard in `src/middleware.ts`,
so the `NextRequest` header injection and `intlMiddleware` call only run for
`/account` and its subpaths.
- Around line 120-128: The API-route branch in middleware is dropping
impersonation context for cart endpoints that now use assertViewOnly(). Update
the NextResponse.next handling in middleware.ts so /api/store/cart/* also gets
the same request header stamping as /api/account/*, using the existing
ACCOUNT_REQUEST_HEADER with sanitizedHeaders. Keep the current account behavior
intact, but ensure the cart/checkout guard paths can still see
getImpersonation() and enforce the intended 403.
---
Nitpick comments:
In `@src/app/`[locale]/account/admin/actions.test.ts:
- Around line 1-57: Add test coverage for the rate-limited path in
startImpersonationAction: mock the rate limiter so consume() returns a failed
result, then assert the action returns the rate_limited error and does not call
findAdminCustomerByEmail or startImpersonation. Keep the new test alongside the
existing startImpersonationAction cases in actions.test.ts and use the existing
mocked createRateLimiter/clientIp setup to target this branch.
In `@src/app/`[locale]/account/admin/page.tsx:
- Around line 13-19: The submit flow in the admin account page is discarding the
result from startImpersonationAction, so forbidden/not_found/rate_limited cases
re-render without any admin-visible feedback. Update the submit/use server flow
in page.tsx to use useActionState (or equivalent action state handling) so the
returned error is stored and rendered inline, and keep the existing
redirect-on-success behavior from startImpersonationAction.
In
`@src/app/api/account/licenses/`[licenseId]/discord/members/[memberId]/route.test.ts:
- Around line 11-15: The current `route.test.ts` only mocks `assertViewOnly` in
`@/lib/impersonation`, but it does not cover the `DELETE` 403
`read_only_inspection` branch. Add an explicit test for the `DELETE` handler in
this route that makes `assertViewOnly` reject with `ViewOnlyError`, then assert
the response status and body match the expected 403 `read_only_inspection`
behavior, similar to the pattern used in `profile/route.test.ts`.
In `@src/lib/customer-orders.ts`:
- Around line 188-203: The getOrderById path is unnecessarily loading the full
impersonation order list through
fetchImpersonatedOrders/listAdminCustomerOrders. Update getOrderById to use a
single filtered admin orders lookup when impersonated access is active, passing
the orderId through the existing fetch/admin order retrieval flow instead of
scanning all impersonated orders. Keep the fallback behavior with getAuthToken
and fetchOrders unchanged, and use the existing getOrderById,
fetchImpersonatedOrders, and fetchOrders symbols to locate the logic.
In `@src/lib/impersonation.test.ts`:
- Around line 65-71: Add test coverage for the error-handling path in
getImpersonation where cookieStore.delete throws in the non-admin branch. Extend
the existing impersonation tests to mock cookieStore.delete throwing, then
verify getImpersonation still returns null and does not propagate the exception,
using the getImpersonation function and cookieStore delete mock to locate the
behavior.
In `@src/lib/impersonation.ts`:
- Around line 70-75: `startImpersonation` currently relies only on the caller’s
precondition and can set `IMPERSONATION_COOKIE` without verifying the session is
an admin. Add an internal admin check at the start of `startImpersonation` in
`src/lib/impersonation.ts` before calling `cookies()` and setting the cookie,
using the existing auth/admin validation path already used by the admin action
so the function is safe even if called directly.
In `@src/middleware.test.ts`:
- Around line 137-150: The current middleware test only covers the generic
/api/* path, so add regression coverage for the other early-return non-account
routes that may bypass the sanitized header forwarding path. Extend
middleware.test.ts with cases for the root ?wc-api=am-software-api route and
updates.wcpos.com/api/*, using middleware() and the ACCOUNT_REQUEST_HEADER
assertions to verify the client-supplied header is still stripped from the
forwarded NextResponse.next({ request }) headers.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b5a4a903-1e06-4059-a6d4-5c4a450f705c
📒 Files selected for processing (35)
src/app/[locale]/account/admin/actions.test.tssrc/app/[locale]/account/admin/actions.tssrc/app/[locale]/account/admin/page.tsxsrc/app/[locale]/account/layout.tsxsrc/app/api/account/impersonate/exit/route.tssrc/app/api/account/licenses/[licenseId]/discord/members/[memberId]/route.test.tssrc/app/api/account/licenses/[licenseId]/discord/members/[memberId]/route.tssrc/app/api/account/licenses/[licenseId]/machines/[machineId]/route.test.tssrc/app/api/account/licenses/[licenseId]/machines/[machineId]/route.tssrc/app/api/account/profile/route.test.tssrc/app/api/account/profile/route.tssrc/app/api/auth/[provider]/callback/route.test.tssrc/app/api/auth/[provider]/callback/route.tssrc/app/api/store/cart/complete/route.test.tssrc/app/api/store/cart/complete/route.tssrc/app/api/store/cart/line-items/route.test.tssrc/app/api/store/cart/line-items/route.tssrc/app/api/store/cart/payment-sessions/route.test.tssrc/app/api/store/cart/payment-sessions/route.tssrc/app/api/store/cart/route.test.tssrc/app/api/store/cart/route.tssrc/components/account/impersonation-banner.tsxsrc/lib/admin.test.tssrc/lib/admin.tssrc/lib/customer-orders.test.tssrc/lib/customer-orders.tssrc/lib/discord/medusa-admin.test.tssrc/lib/discord/medusa-admin.tssrc/lib/impersonation.integration.test.tssrc/lib/impersonation.test.tssrc/lib/impersonation.tssrc/lib/medusa-auth.test.tssrc/lib/medusa-auth.tssrc/middleware.test.tssrc/middleware.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 213d71ee93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
kilbot has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Contributor
|
Review-fix triage for commit 7e7472c.
Skipped threads:
Validation:
Fresh post-resolution thread inventory: 0 unresolved review threads. |
|
🚀 Preview: https://wcpos-hftap7rwu-wcpos.vercel.app |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/app/api/account/impersonate/exit/route.ts (2)
7-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing next-intl's
getPathnameinstead of hand-rolled prefixing.The
locale !== defaultLocale ? '/${locale}/account' : '/account'logic duplicates next-intl's ownlocalePrefix: 'as-needed'behavior outside the shared routing config. If that config ever changes (e.g. toalwaysor domain-based routing), this manual branch will silently drift.getPathname({ locale, href: '/account' })from@/i18n/navigationencodes the same intent without duplicating the prefixing rule.🤖 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/api/account/impersonate/exit/route.ts` around lines 7 - 25, The accountUrl helper is duplicating locale prefixing logic instead of using the shared next-intl routing API. Update accountUrl in the impersonate exit route to build the /account target through getPathname from `@/i18n/navigation`, using the resolved locale and keeping the existing referer/explicitLocale selection logic, so prefix behavior stays aligned with the central routing config.
27-32: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSame PII-in-logs concern as
actions.ts.
authLogger.info\Impersonation STOP: admin=${session?.email ?? 'unknown'}`` logs the admin's email; flagged by static analysis (CWE-532). Lower risk than logging the target customer's email (this is the admin's own identity for audit purposes), but consider standardizing on a non-PII identifier if the same convention is applied elsewhere.🤖 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/api/account/impersonate/exit/route.ts` around lines 27 - 32, The exit impersonation flow in exit() is still logging PII by using session?.email in authLogger.info, which triggers the same CWE-532 concern as actions.ts. Update the log statement in exit to avoid raw email addresses and standardize on a non-PII identifier already available from getSessionCustomer() or another stable audit-safe value, keeping the Impersonation STOP event usable without exposing email data.Source: Linters/SAST tools
src/lib/discord/medusa-admin.ts (1)
96-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent error handling vs. sibling lookups.
findAdminCustomerByEmailandgetAdminCustomerByIdboth catchmedusaAdminFetchfailures and log viainfraLogger.error, returningnull.getAdminCustomerOrderByIdlets failures propagate uncaught. The only current caller (fetchImpersonatedOrderByIdinsrc/lib/customer-orders.ts) happens to wrap it in try/catch, but this breaks the just-established convention in this file and is fragile against future callers.♻️ Suggested fix for consistency
export async function getAdminCustomerOrderById( customerId: string, orderId: string ): Promise<MedusaOrder | null> { const query = new URLSearchParams({ limit: '1', customer_id: customerId, id: orderId, }) - - const page = await medusaAdminFetch<AdminOrdersResponse>( - `/admin/orders?${query.toString()}` - ) - const [order] = page.orders ?? [] - return order?.id === orderId ? order : null + try { + const page = await medusaAdminFetch<AdminOrdersResponse>( + `/admin/orders?${query.toString()}` + ) + const [order] = page.orders ?? [] + return order?.id === orderId ? order : null + } catch (error) { + infraLogger.error`Failed to fetch admin order ${orderId} for customer ${customerId}: ${error}` + return null + } }🤖 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/discord/medusa-admin.ts` around lines 96 - 111, getAdminCustomerOrderById is the only lookup here that does not mirror the try/catch + infraLogger.error + null fallback used by findAdminCustomerByEmail and getAdminCustomerById. Wrap the medusaAdminFetch call in getAdminCustomerOrderById with the same error handling pattern, log the failure through infraLogger.error with enough context to identify the order/customer lookup, and return null on failure so the behavior stays consistent for future callers.src/app/[locale]/account/admin/actions.ts (1)
33-58: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid logging customer email addresses in plaintext.
Static analysis flags lines 39, 51, and 55 for logging PII (CWE-532).
target=${email}/target=${target.email}log the impersonated customer's email address on every lookup attempt. Consider loggingtarget.id(or a redacted/hashed form) instead of the raw email to preserve audit value while reducing PII exposure in log aggregators.🔒 Suggested reduction of logged PII
- authLogger.info`Impersonation lookup miss: admin=${adminEmail} target=${email}` + authLogger.info`Impersonation lookup miss: admin=${adminEmail}` return { error: 'not_found' } } - authLogger.info`Impersonation START: admin=${adminEmail} target=${target.email} (${target.id})` + authLogger.info`Impersonation START: admin=${adminEmail} target_id=${target.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/app/`[locale]/account/admin/actions.ts around lines 33 - 58, The impersonation audit logs in startImpersonationAction are leaking customer email PII by writing the target email in plaintext. Update the authLogger.info calls in the lookup-miss and START paths to log a non-PII identifier such as target.id, or a redacted/hashed value, while keeping the adminEmail context for auditability. Keep the existing flow in startImpersonationAction and only change the logged target fields so the messages remain useful without exposing raw customer emails.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@src/app/`[locale]/account/admin/actions.ts:
- Around line 33-58: The impersonation audit logs in startImpersonationAction
are leaking customer email PII by writing the target email in plaintext. Update
the authLogger.info calls in the lookup-miss and START paths to log a non-PII
identifier such as target.id, or a redacted/hashed value, while keeping the
adminEmail context for auditability. Keep the existing flow in
startImpersonationAction and only change the logged target fields so the
messages remain useful without exposing raw customer emails.
In `@src/app/api/account/impersonate/exit/route.ts`:
- Around line 7-25: The accountUrl helper is duplicating locale prefixing logic
instead of using the shared next-intl routing API. Update accountUrl in the
impersonate exit route to build the /account target through getPathname from
`@/i18n/navigation`, using the resolved locale and keeping the existing
referer/explicitLocale selection logic, so prefix behavior stays aligned with
the central routing config.
- Around line 27-32: The exit impersonation flow in exit() is still logging PII
by using session?.email in authLogger.info, which triggers the same CWE-532
concern as actions.ts. Update the log statement in exit to avoid raw email
addresses and standardize on a non-PII identifier already available from
getSessionCustomer() or another stable audit-safe value, keeping the
Impersonation STOP event usable without exposing email data.
In `@src/lib/discord/medusa-admin.ts`:
- Around line 96-111: getAdminCustomerOrderById is the only lookup here that
does not mirror the try/catch + infraLogger.error + null fallback used by
findAdminCustomerByEmail and getAdminCustomerById. Wrap the medusaAdminFetch
call in getAdminCustomerOrderById with the same error handling pattern, log the
failure through infraLogger.error with enough context to identify the
order/customer lookup, and return null on failure so the behavior stays
consistent for future callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dd78a26a-0a86-41ab-9a88-6f9c83443ed1
📒 Files selected for processing (14)
src/app/[locale]/account/admin/actions.test.tssrc/app/[locale]/account/admin/actions.tssrc/app/[locale]/account/admin/page.tsxsrc/app/[locale]/account/layout.tsxsrc/app/api/account/impersonate/exit/route.tssrc/app/api/account/licenses/[licenseId]/discord/members/[memberId]/route.test.tssrc/lib/customer-orders.test.tssrc/lib/customer-orders.tssrc/lib/discord/medusa-admin.test.tssrc/lib/discord/medusa-admin.tssrc/lib/impersonation.test.tssrc/lib/impersonation.tssrc/middleware.test.tssrc/middleware.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/customer-orders.test.ts
- src/app/[locale]/account/admin/page.tsx
- src/lib/impersonation.ts
- src/app/[locale]/account/layout.tsx
- src/lib/customer-orders.ts
- src/middleware.ts
There was a problem hiding this comment.
kilbot has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
🚀 Preview: https://wcpos-ky7fto8hf-wcpos.vercel.app |
Contributor
|
Review-fix triage for commit a4a2f7d.
Remaining unresolved review threads after post-push GraphQL inventory: 0. Validation:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Super-admin "view as" — read-only account impersonation
Lets an owner-allowlisted account browse the entire
/accountarea as another customer, read-only, to verify exactly what downloads/entitlements that customer sees. You stay logged in as yourself; a target identity is layered on top for display + reads only.Spec:
docs/superpowers/specs/2026-07-05-super-admin-view-as-design.md· Plan:docs/superpowers/plans/2026-07-05-super-admin-view-as.md(both local scratch, gitignored).How it works
getSessionCustomer()) — used for the admin gate + audit. A plain httpOnly cookie (wcpos-impersonate) names a target. When you're inside/accountand your real session is an admin,getCustomer()and the order fetchers transparently resolve the target instead (via Medusa's built-in/adminreads with the existingMEDUSA_ADMIN_API_TOKEN). Every downstream entitlement/license/download path is already customer-agnostic, so the whole account renders as them. No backend change./account/admin→ enter an email → rate-limited, audited lookup → redirect to/account, now viewing as them. An amber banner shows who you're inspecting with a one-click Exit.assertViewOnly()blocks every mutating account/cart route (profile edit, machine/Discord removal, cart/checkout) with a 403. Reads — including the actual download path — stay open so you can confirm a file really downloads.Security model
Authority to impersonate is decided only from your real session's email against a config-in-code allowlist (
src/lib/admin.ts) — never from the cookie. A planted/forged cookie does nothing without an admin session. Scoping to/accountis enforced by a middleware-set request header that is stripped on all other paths so it can't be client-spoofed.Independently adversarially reviewed. Core authority model + order isolation + write-blocking all verified solid. Three review findings fixed in
213d71e: (1) strip spoofable scope header in middleware, (2) OAuth callback usesgetSessionCustomer(), (3) stale-target auto-exits to owner.Validation
vitest run) — includes the security invariant truth-table, the two-part override, write-block on every guarded route, and an end-to-end resolve+orders integration test.next buildclean, no type errors.src/lib/admin.tshasADMIN_EMAILS = ['paul@kilbot.com']. This must exactly match the email on your real Medusa customer account or the feature won't activate for you. (Your session email showed aspaul@kilbot.com.au— please set whichever is correct.)/adminreads. The feature reusesMEDUSA_ADMIN_API_TOKEN(today only used by Discord sync) to read/admin/customers+/admin/orders. Confirm it works:curl -s -H "Authorization: Bearer $MEDUSA_ADMIN_API_TOKEN" "$MEDUSA_BACKEND_URL/admin/customers?limit=1&fields=id,email"→ should return acustomersarray, not 401/403. If it 401s, we need a narrow custom admin route on wcpos-medusa (follow-up)./account/admin→ enter a known customer's email → confirm the Downloads tab shows their entitlements, a profile edit is blocked (403), and Exit returns you to your own account.Follow-ups (out of scope)
useActionStateerror messaging on the admin page (currently the lookup miss re-renders without an inline message).Summary by CodeRabbit
read_only_inspection).x-wcpos-account-requestbehavior for safer routing.