Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/order-sort-deny-list.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@medusajs/framework": patch
"@medusajs/medusa": patch
"@medusajs/types": patch
---

fix(framework,medusa): reject sorting the admin and store orders lists by computed fields (totals, payment_status, fulfillment_status) with a 400 instead of a 500
40 changes: 40 additions & 0 deletions integration-tests/http/__tests__/order/admin/order.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,46 @@ medusaIntegrationTestRunner({
])
})

describe("non-sortable fields", () => {
it.each([
"total",
"-payment_status",
"fulfillment_status",
"subtotal",
"-shipping_total",
"credit_line_tax_total",
"original_item_subtotal",
])(
"should return 400 when sorting by computed field '%s'",
async (sort) => {
const response = await api
.get(`/admin/orders?order=${sort}`, adminHeaders)
.catch((e) => e)

const field = sort.replace(/^-/, "")
expect(response.response.status).toEqual(400)
expect(response.response.data.message).toEqual(
`Order field ${field} is not valid`
)
}
)

it.each(["-created_at", "display_id", "email", "status"])(
"should sort by real column '%s' without error",
async (sort) => {
const response = await api.get(
`/admin/orders?order=${sort}`,
adminHeaders
)

expect(response.status).toEqual(200)
expect(response.data.orders).toEqual([
expect.objectContaining({ id: order.id }),
])
}
)
})

it("should search orders by shipping address", async () => {
let response = await api.get(
`/admin/orders?fields=+shipping_address.address_1,+shipping_address.address_2`,
Expand Down
40 changes: 40 additions & 0 deletions integration-tests/http/__tests__/order/store/order.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,46 @@ medusaIntegrationTestRunner({
expect(response.status).toEqual(401)
expect(response.data.message).toEqual("Unauthorized")
})

describe("non-sortable fields", () => {
it.each([
"total",
"-payment_status",
"fulfillment_status",
"subtotal",
"-shipping_total",
"credit_line_tax_total",
"original_item_subtotal",
])(
"should return 400 when sorting by computed field '%s'",
async (sort) => {
const response = await api
.get(`/store/orders?order=${sort}`, storeHeadersWithCustomer)
.catch((e) => e)

const field = sort.replace(/^-/, "")
expect(response.response.status).toEqual(400)
expect(response.response.data.message).toEqual(
`Order field ${field} is not valid`
)
}
)

it.each(["-created_at", "display_id", "status"])(
"should sort by real column '%s' without error",
async (sort) => {
const response = await api.get(
`/store/orders?order=${sort}`,
storeHeadersWithCustomer
)

expect(response.status).toEqual(200)
expect(response.data.orders).toEqual([
expect.objectContaining({ id: order.id }),
])
}
)
})
})

describe("GET /store/orders/:id", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,98 @@ describe("prepareListQuery", () => {
})
})

it("should throw error when order field is in forbiddenOrderBy", async () => {
const validated: RequestQueryFields = {
order: "total",
limit: 10,
offset: 0,
}

const queryConfig: QueryConfig<any> = {
isList: true,
forbiddenOrderBy: ["total", "payment_status"],
}

await expect(prepareListQuery(validated, queryConfig)).rejects.toThrow(
"Order field total is not valid"
)
})

it("should throw error when descending order field is in forbiddenOrderBy", async () => {
const validated: RequestQueryFields = {
order: "-payment_status",
limit: 10,
offset: 0,
}

const queryConfig: QueryConfig<any> = {
isList: true,
forbiddenOrderBy: ["total", "payment_status"],
}

await expect(prepareListQuery(validated, queryConfig)).rejects.toThrow(
"Order field payment_status is not valid"
)
})

it("should allow order field when it is not in forbiddenOrderBy", async () => {
const validated: RequestQueryFields = {
order: "-created_at",
limit: 10,
offset: 0,
}

const queryConfig: QueryConfig<any> = {
isList: true,
forbiddenOrderBy: ["total", "payment_status"],
}

const result = await prepareListQuery(validated, queryConfig)

expect(result.listConfig.order).toEqual({ created_at: "DESC" })
expect(result.remoteQueryConfig.pagination.order).toEqual({
created_at: "DESC",
})
})

it("should reject a forbiddenOrderBy field even when it is in allowed", async () => {
// `total` is selectable (in `allowed`) but forbidden as a sort key —
// sorting is decoupled from field selection.
const validated: RequestQueryFields = {
order: "total",
limit: 10,
offset: 0,
}

const queryConfig: QueryConfig<any> = {
isList: true,
allowed: ["id", "total", "created_at"],
forbiddenOrderBy: ["total"],
}

await expect(prepareListQuery(validated, queryConfig)).rejects.toThrow(
"Order field total is not valid"
)
})

it("should still enforce allowed when forbiddenOrderBy is set", async () => {
const validated: RequestQueryFields = {
order: "email",
limit: 10,
offset: 0,
}

const queryConfig: QueryConfig<any> = {
isList: true,
allowed: ["id", "created_at"],
forbiddenOrderBy: ["total"],
}

await expect(prepareListQuery(validated, queryConfig)).rejects.toThrow(
"Order field email is not valid"
)
})

it("should allow nested order field when parent relation is in allowed fields", async () => {
const validated: RequestQueryFields = {
order: "product.title",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export async function prepareListQuery<T extends RequestQueryFields, TEntity>(
) {
const {
allowed = [],
forbiddenOrderBy = [],
restricted = [],
disallowed = [],
defaults = [],
Expand Down Expand Up @@ -132,7 +133,10 @@ export async function prepareListQuery<T extends RequestQueryFields, TEntity>(
orderBy = { [order]: "ASC" }
}

if (allowed.length && !allowed.includes(orderField)) {
if (
forbiddenOrderBy.includes(orderField) ||
(allowed.length && !allowed.includes(orderField))
) {
throw new MedusaError(
MedusaError.Types.INVALID_DATA,
`Order field ${orderField} is not valid`
Expand Down
6 changes: 6 additions & 0 deletions packages/core/types/src/common/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,12 @@ export type QueryConfig<TEntity> = {
*/
allowed?: string[]
/**
* 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
Comment on lines 462 to 471

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's incorrect TSDoc block opening here which seems like it's due to merging develop and resolving conflicts incorrectly. Can you resolve it?

Expand Down
38 changes: 38 additions & 0 deletions packages/medusa/src/api/admin/orders/query-config.ts

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you apply this to the store orders API route as well?

Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,46 @@ export const retrieveTransformQueryConfig = {
entity: Entities.order,
}

/**
* Fields the orders list may NOT be sorted by. These are computed at query
* time — not columns on the order table — so ordering by them throws at the
* database layer. They remain selectable; they just can't be sort keys.
* The totals mirror `shouldIncludeTotals` in the order module service.
*/
export const forbiddenAdminOrderSortFields = [
"payment_status",
"fulfillment_status",
"summary",
"total",
"subtotal",
"tax_total",
"discount_total",
"discount_tax_total",
"original_total",
"original_subtotal",
"original_tax_total",
"item_total",
"item_subtotal",
"item_tax_total",
"original_item_total",
"original_item_subtotal",
"original_item_tax_total",
"shipping_total",
"shipping_subtotal",
"shipping_tax_total",
"original_shipping_total",
"original_shipping_subtotal",
"original_shipping_tax_total",
"credit_line_total",
"credit_line_subtotal",
"credit_line_tax_total",
"pending_difference",
"refundable_amount",
]

export const listTransformQueryConfig = {
defaults: defaultAdminOrderFields,
forbiddenOrderBy: forbiddenAdminOrderSortFields,
defaultLimit: 20,
isList: true,
entity: Entities.order,
Expand Down
40 changes: 40 additions & 0 deletions packages/medusa/src/api/store/orders/query-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,47 @@ export const retrieveTransformQueryConfig = {
isList: false,
}

/**
* Fields the orders list may NOT be sorted by. These are computed at query
* time — not columns on the order table — so ordering by them throws at the
* database layer. They remain selectable; they just can't be sort keys.
* The totals mirror `shouldIncludeTotals` in the order module service.
* Kept in sync with `forbiddenAdminOrderSortFields` on the admin route.
*/
export const forbiddenStoreOrderSortFields = [
"payment_status",
"fulfillment_status",
"summary",
"total",
"subtotal",
"tax_total",
"discount_total",
"discount_subtotal",
"discount_tax_total",
"original_total",
"original_subtotal",
"original_tax_total",
"item_total",
"item_subtotal",
"item_tax_total",
"original_item_total",
"original_item_subtotal",
"original_item_tax_total",
"shipping_total",
"shipping_subtotal",
"shipping_tax_total",
"original_shipping_total",
"original_shipping_subtotal",
"original_shipping_tax_total",
"credit_line_total",
"credit_line_subtotal",
"credit_line_tax_total",
"pending_difference",
"refundable_amount",
]

export const listTransformQueryConfig = {
defaults: defaultStoreOrderFields,
forbiddenOrderBy: forbiddenStoreOrderSortFields,
isList: true,
}
Loading