fix(framework,medusa): reject sorting admin orders by computed fields with a 400#16070
fix(framework,medusa): reject sorting admin orders by computed fields with a 400#16070hhuang2088 wants to merge 3 commits into
Conversation
… with a 400 Sorting GET /admin/orders by a computed field — any total (subtotal, tax_total, item_*, shipping_*, original_*, credit_line_*, ...), payment_status, or fulfillment_status — threw a 500 because these are not persisted columns on the order table (MikroORM: "Trying to order by not existing property"). Fixes medusajs#15353. - Add optional forbiddenOrderBy to QueryConfig (@medusajs/types): a sort-only deny list that does not affect field selection. - Enforce it in prepareListQuery (@medusajs/framework) alongside the untouched allowed check, so no other route changes behavior. - Define forbiddenAdminOrderSortFields on the orders list config: computed totals (mirroring shouldIncludeTotals in the order module service), payment_status, fulfillment_status, and summary. A deny list rather than an allow list keeps this non-breaking: every forbidden field previously 500'd, while real columns (email, status, currency_code, ...) keep sorting as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 4b386ac The changes in this PR will be included in the next version bump. This PR includes changesets to release 79 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@NicolasGorga following up on your review of #15809, which this PR supersedes: reworked as you suggested. One extension beyond the three fields you mentioned: an automated review pointed out that the other computed totals ( |
|
Thanks for the contribution! Initial automated review looks good. Cleanly guards the admin orders list against sorting by computed fields (totals, payment_status, fulfillment_status, summary) that previously threw a 500 at the DB layer, returning a 400 instead. Adds an optional forbiddenOrderBy to QueryConfig enforced in prepareListQuery alongside the untouched allowed check. Deny-list approach is non-breaking: every listed field already errored, so no working sort changes. Order-field parsing correctly strips leading '-'; nested sort keys won't false-match. PR template is complete, changeset included, and both unit and integration tests cover forbidden and Triggered by: new PR opened |
shahednasser
left a comment
shahednasser
left a comment
There was a problem hiding this comment.
Thank you for your contribution @hhuang2088 ! Works well but just one comment
There was a problem hiding this comment.
Can you apply this to the store orders API route as well?
Apply the admin orders sort deny list to GET /store/orders as well. The store list route runs through the same getOrdersListWorkflow and prepareListQuery path, so sorting by a computed field (any total, payment_status, fulfillment_status, summary) 500'd the same way — these are not persisted columns on the order table. - Define forbiddenStoreOrderSortFields on the store orders list config, mirroring forbiddenAdminOrderSortFields (kept in sync per the file's existing "copied over from admin" convention). Includes discount_subtotal, a computed field the store surface exposes. - Add integration tests asserting a 400 for computed sort fields and a 200 for real columns (created_at, display_id, status). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| /** | ||
| * Fields rejected in the `order` query param (sorting). Unlike `allowed`, | ||
| * this does NOT affect field selection — a field can stay selectable while | ||
| * being forbidden as a sort key. Sorting by a field in this list throws | ||
| * INVALID_DATA (400). | ||
| */ | ||
| forbiddenOrderBy?: string[] | ||
| * Fields and relations that must never be resolved, regardless of what the | ||
| * caller requests. Any requested field whose path contains one of these | ||
| * segments (e.g. `orders` matches `orders.customer.email`) is stripped before |
There was a problem hiding this comment.
there's incorrect TSDoc block opening here which seems like it's due to merging develop and resolving conflicts incorrectly. Can you resolve it?
2 participants
What
Sorting the admin orders list (
GET /admin/orders) by a computed field —any total (
total,subtotal,tax_total,item_*,shipping_*,original_*,credit_line_*, …),payment_status, orfulfillment_status—currently throws a 500, because these are not persisted columns on the order
table (MikroORM: "Trying to order by not existing property Order.total").
This PR rejects sorting by these fields with a clean 400.
Why
The orders list query config never guarded the
orderquery param, so itflowed unchecked to the data layer. This mirrors the filtering fix in #15262
(
nonFilterableFields), but for sorting — the counterpart was missing.Per review feedback, the guard is a deny list rather than an allow list:
an allow list would 400 real columns (
email,status,currency_code, …)that sorted fine before — a breaking change. Every field in the deny list
previously threw a 500, so forbidding them changes no working request.
How
forbiddenOrderBytoQueryConfig(@medusajs/types).Unlike
allowed, it governs only sorting, not field selection — so acomputed field like
totalstays selectable while being rejected as a sortkey.
prepareListQuery(@medusajs/framework), alongside theuntouched pre-existing
allowedcheck — no other route changes behavior.forbiddenAdminOrderSortFieldson the orders list config: thecomputed totals (mirroring
shouldIncludeTotalsin the order moduleservice),
payment_status,fulfillment_status, andsummary.Testing
packages/core/framework/src/http/utils/__tests__/get-query-config.spec.ts):added cases for
prepareListQuery— rejecting a sort field inforbiddenOrderBy(ascending and descending), allowing one outside it,rejecting a forbidden field even when it is in
allowed(sort decoupledfrom field selection), and still enforcing
allowedwhenforbiddenOrderByis set.
integration-tests/http/__tests__/order/admin/order.spec.ts):added cases under
GET /admin/orders— 400 when sorting bytotal,-payment_status,fulfillment_status,subtotal,-shipping_total,credit_line_tax_total, ororiginal_item_subtotal; 200 when sorting byreal columns
-created_at,display_id,email, andstatus(includingones outside any allow list, proving no regression for existing sorts).
The pre-existing
fields=id,total&order=-created_attest still passes,proving field selection is unaffected.
Supersedes #15809 — same change reworked per review feedback there (allow
list → deny list), rebased onto the latest develop and squashed into a single
commit, on a branch named for the final approach.
Closes #15353
🤖 Generated with Claude Code