fix(framework,medusa): reject sorting admin orders by computed fields with a 400#15809
fix(framework,medusa): reject sorting admin orders by computed fields with a 400#15809hhuang2088 wants to merge 4 commits into
Conversation
Sorting GET /admin/orders by a computed field (total, payment_status, fulfillment_status) reached MikroORM and threw a 500, since those are not persisted columns on the order table. Add an optional `allowedOrderBy` to QueryConfig, enforced in prepareListQuery's order guard (falling back to `allowed` so existing routes are unaffected), and supply the orders list config with the sortable column allow list (display_id, created_at, updated_at) that mirrors the admin dashboard's sort options. Invalid sort fields now return a clean 400 instead of a 500. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 88e3865 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 |
|
Thanks for the contribution! Initial automated review looks good. Well-scoped fix for issue #15353: sorting the admin orders list by computed fields (total, payment/fulfillment status) returned a 500; this adds an optional allowedOrderBy to QueryConfig and an admin orders sort allowlist so invalid sort keys return a clean 400. PR template is complete, a changeset is included, and integration tests cover both 400 and 200 cases. The fallback (allowedOrderBy || allowed) preserves existing routes. Non-blocking consideration: the allowlist (display_id, created_at, updated_at) also narrows sorting by other valid persisted columns like status/version, which previou Triggered by: new PR opened |
Cover the new allowedOrderBy behavior in prepareListQuery: rejecting a disallowed sort field, allowing a permitted one, preferring allowedOrderBy over allowed (sort decoupled from field selection), and sorting by a field present only in allowedOrderBy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| "@medusajs/types": patch | ||
| --- | ||
|
|
||
| fix(framework,medusa): reject sorting the admin orders list by non-sortable fields with a 400 |
There was a problem hiding this comment.
Should be fix(framework,medusa,types): reject sorting the admin orders list by non-sortable fields with a 400
| * they aren't columns on the order table, so ordering by them throws at the | ||
| * database layer. | ||
| */ | ||
| export const allowedAdminOrderSortFields = [ |
There was a problem hiding this comment.
This will make it a breaking change for anyone already sorting on the core order list endpoint by something other than these three fields. To avoid this, I think it might be better to expose the inverse forbiddenOrderBy and then set these three, making the corresponding changes inside prepareListQuery
…ny list Per review feedback on medusajs#15809: an allow list of three fields would 400 any other real column that sorted fine before. Replace allowedOrderBy with forbiddenOrderBy and forbid only the computed fields from medusajs#15353 (total, payment_status, fulfillment_status), which previously threw 500 at the database layer — so nothing that worked before changes behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the contribution! A few items need to be addressed before this can move forward: Well-scoped, well-tested change that turns the 500 into a clean 400 for the three dashboard sort fields, and the allowed-fallback keeps other routes unchanged. One gap: the deny-list only covers total, payment_status, fulfillment_status, but the admin orders list exposes many other computed totals (subtotal, tax_total, discount_total, item_total, shipping_total and their original_/credit_line_ variants), so GET /admin/orders?order=subtotal still 500s — the same bug class. Consider an allow-list of persisted columns instead. Also, the PR description still refers to allowedOrderBy, but the code
Triggered by: new commit pushed |
The initial deny list only covered the three fields from medusajs#15353, but every computed total (subtotal, tax_total, discount_total, item_*, shipping_*, original_*, credit_line_*) hits the same bug class — none are columns on the order table, so sorting by them threw 500. Mirror the order module's shouldIncludeTotals list plus the computed statuses and summary. All of these previously errored, so forbidding them is not a behavior change for any working request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hhuang2088
changed the title
|
Thanks for the contribution! Initial automated review looks good. Re-review: the prior gap (deny-list missing computed totals beyond total/payment_status/fulfillment_status) is now resolved — forbiddenAdminOrderSortFields covers all computed totals, and code/description consistently use forbiddenOrderBy (deny-list). Logic in prepareListQuery correctly strips the '-' prefix before checking and rejects with a clean 400. Denied fields are all computed (not persisted columns), so no previously-working sort regresses. Template complete, changeset present, unit + integration tests cover 400 and 200 cases. No new blockers found. Triggered by: new commit pushed |
|
Superseded by #16070 — same fix reworked per the review feedback here (sort allow list → |
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.
Closes #15353
🤖 Generated with Claude Code