diff --git a/.changeset/clear-order-status-errors.md b/.changeset/clear-order-status-errors.md new file mode 100644 index 00000000..bcd7f745 --- /dev/null +++ b/.changeset/clear-order-status-errors.md @@ -0,0 +1,5 @@ +--- +'@godaddy/gd-commerce-server': minor +--- + +Return 400 from the order-status route for invalid order IDs, including IDs the Orders API rejects as malformed, and 404 when the Orders API reports the order missing or it belongs to another store or channel, instead of 500. A 404 without the Orders API's `NOT_FOUND` code, such as from a misconfigured base URL, stays a logged 500. The 500 response no longer includes the internal error message; it is logged server-side instead. Export `InvalidOrderIdError`, `OrderNotFoundError`, and `ORDER_STATUS_UNKNOWN` for in-process `getOrderStatus()` callers; check `error.name` rather than `instanceof` so the check holds when a host has more than one copy of this package. diff --git a/packages/commerce-server/README.md b/packages/commerce-server/README.md index eb867c1f..91ed9574 100644 --- a/packages/commerce-server/README.md +++ b/packages/commerce-server/README.md @@ -56,4 +56,4 @@ Configure `checkoutReturnUrls` on the router using trusted deployment settings. Without this policy, HTTP checkout returns 503 before creating a session. Invalid request destinations return 400. This applies to both router presets that expose checkout. Trusted in-process callers of `createCheckoutSession()` own their return URLs and must construct or validate them server-side. -A return from hosted checkout is not proof of payment. The order-status route uses the authorized Orders REST API, which supports completed orders, and returns its payment status (for example `PAID` or `PENDING`; `unknown` if absent). The server OAuth client must be granted `commerce.order:read`. The helper verifies the returned order ID, store, and channel and returns a limited summary without customer contact data. Hosts must authenticate callers and authorize access to each requested order before exposing this route. +A return from hosted checkout is not proof of payment. The order-status route uses the authorized Orders REST API, which supports completed orders, and returns its payment status (for example `PAID` or `PENDING`; the exported `ORDER_STATUS_UNKNOWN`, `'unknown'`, if absent). Show it only as display enrichment and never present an unconfirmed payment as paid. The server OAuth client must be granted `commerce.order:read`. The helper verifies the returned order ID, store, and channel and returns a limited summary without customer contact data. The route returns 400 for a missing, blank, padded, `.`, or `..` order ID (padded IDs are rejected, not trimmed) or one the Orders API rejects as malformed (422 `VALIDATION_FAILED`), and 404 when the Orders API reports the order missing (404 `NOT_FOUND`) or the order belongs to another store or channel. A 404 without that code, such as from a misconfigured API base URL, is treated as an upstream failure. Credential and other upstream failures, including a response for a different order ID, return 500 with a generic body; the detail is logged server-side with `console.error`. In-process callers can identify the exported `InvalidOrderIdError` and `OrderNotFoundError` by `error.name`; prefer that to `instanceof`, which fails when a host has more than one copy of this package. Hosts must authenticate callers and authorize access to each requested order before exposing this route. diff --git a/packages/commerce-server/src/get-order-status.test.ts b/packages/commerce-server/src/get-order-status.test.ts index ca89fc3f..0cb85279 100644 --- a/packages/commerce-server/src/get-order-status.test.ts +++ b/packages/commerce-server/src/get-order-status.test.ts @@ -2,7 +2,7 @@ import { once } from 'node:events'; import express from 'express'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { createRuntimeCommerceConfiguration } from './lib/commerce/config'; -import { getOrderStatus } from './lib/commerce/get-order-status'; +import { getOrderStatus, InvalidOrderIdError, OrderNotFoundError } from './lib/commerce/get-order-status'; import { createGoDaddyPaymentsRouter } from './router'; const clientFetch = globalThis.fetch; @@ -34,6 +34,17 @@ const summary = { updatedAt: order.updatedAt, lineItems: [{ id: 'line-1', name: 'Mug', quantity: 1 }], }; +// Bodies the Orders API's REST error handler sends for a missing order and an undecodable ID. +const ordersApiNotFound = (): Response => + Response.json( + { code: 'NOT_FOUND', message: 'Order not found. Possible reasons: invalid orderId, storeID mismatch.' }, + { status: 404 }, + ); +const ordersApiInvalidId = (): Response => + Response.json( + { code: 'VALIDATION_FAILED', message: 'Invalid global ID: completed-order' }, + { status: 422 }, + ); let upstream: ReturnType>; beforeEach((): void => { @@ -52,6 +63,7 @@ beforeEach((): void => { }); afterEach((): void => { vi.unstubAllGlobals(); + vi.restoreAllMocks(); }); describe('authorized order lookup', () => { @@ -128,26 +140,76 @@ describe('authorized order lookup', () => { it.each([ undefined, + { ...order, context: undefined }, + { ...order, context: {} }, + { ...order, context: { channelId: order.context.channelId } }, + { ...order, context: { storeId: order.context.storeId } }, + { ...order, context: { ...order.context, storeId: '' } }, + { ...order, id: undefined }, + { ...order, id: 42 }, { ...order, id: 'another-order' }, + ])('rejects an incomplete or mismatched order response', async (result): Promise => { + upstream + .mockResolvedValueOnce(Response.json({ access_token: 'order-token' })) + .mockResolvedValueOnce(Response.json({ order: result })); + const lookup = getOrderStatus(order.id, configuration); + await expect(lookup).rejects.toThrow('Order lookup did not return the requested order'); + await expect(lookup).rejects.not.toBeInstanceOf(OrderNotFoundError); + }); + + it.each([ { ...order, context: { ...order.context, storeId: 'another-store' } }, { ...order, context: { ...order.context, channelId: 'another-channel' } }, - { ...order, context: undefined }, - ])('rejects missing or mismatched order bindings', async (result): Promise => { + ])('reports an order bound to another store or channel as not found', async (result): Promise => { upstream .mockResolvedValueOnce(Response.json({ access_token: 'order-token' })) .mockResolvedValueOnce(Response.json({ order: result })); - await expect(getOrderStatus(order.id, configuration)).rejects.toThrow('Order lookup'); + await expect(getOrderStatus(order.id, configuration)).rejects.toBeInstanceOf(OrderNotFoundError); + }); + + it('reports an Orders API NOT_FOUND as not found without exposing its body', async (): Promise => { + upstream + .mockResolvedValueOnce(Response.json({ access_token: 'order-token' })) + .mockResolvedValueOnce(ordersApiNotFound()); + const lookup = getOrderStatus(order.id, configuration); + await expect(lookup).rejects.toBeInstanceOf(OrderNotFoundError); + await expect(lookup).rejects.toThrow(/^Order not found$/); }); - it.each([401, 403, 404, 500])( + it('reports an Orders API VALIDATION_FAILED for the order ID as an invalid order ID', async (): Promise => { + upstream + .mockResolvedValueOnce(Response.json({ access_token: 'order-token' })) + .mockResolvedValueOnce(ordersApiInvalidId()); + await expect(getOrderStatus(order.id, configuration)).rejects.toBeInstanceOf(InvalidOrderIdError); + }); + + it.each([ + ['an HTML 404 from an unrouted path', new Response('
Cannot GET /v1/x
', { status: 404 })], + ['a JSON 404 without a code', Response.json({ message: 'Not Found' }, { status: 404 })], + ['a 404 with another code', Response.json({ code: 'ROUTE_NOT_FOUND' }, { status: 404 })], + ['a 422 with another code', Response.json({ code: 'CONFLICT' }, { status: 422 })], + ['a non-JSON 422', new Response('Unprocessable', { status: 422 })], + ])('treats %s as an upstream failure', async (_case, failure): Promise => { + upstream + .mockResolvedValueOnce(Response.json({ access_token: 'order-token' })) + .mockResolvedValueOnce(failure); + const lookup = getOrderStatus(order.id, configuration); + await expect(lookup).rejects.toThrow(`Failed to load order: upstream returned ${failure.status}`); + await expect(lookup).rejects.not.toBeInstanceOf(OrderNotFoundError); + await expect(lookup).rejects.not.toBeInstanceOf(InvalidOrderIdError); + }); + + it.each([401, 403, 500])( 'preserves an upstream order lookup failure (%i) without exposing its body', async (status): Promise => { + const cancel = vi.spyOn(ReadableStream.prototype, 'cancel'); upstream .mockResolvedValueOnce(Response.json({ access_token: 'order-token' })) .mockResolvedValueOnce(new Response('Private upstream details', { status })); await expect(getOrderStatus(order.id, configuration)).rejects.toThrow( `Failed to load order: upstream returned ${status}`, ); + expect(cancel).toHaveBeenCalledTimes(1); }, ); @@ -157,11 +219,145 @@ describe('authorized order lookup', () => { expect(upstream).toHaveBeenCalledTimes(1); }); - it.each(['', ' ', '.', '..'])( + it.each(['', ' ', '.', '..', ' completed-order', 'completed-order\n'])( 'rejects invalid order ID %j before requesting credentials', async (id): Promise => { - await expect(getOrderStatus(id, configuration)).rejects.toThrow('a valid orderId is required'); + const lookup = getOrderStatus(id, configuration); + await expect(lookup).rejects.toBeInstanceOf(InvalidOrderIdError); + await expect(lookup).rejects.toThrow('a valid orderId is required'); + expect(upstream).not.toHaveBeenCalled(); + }, + ); +}); + +describe('order-status route', () => { + async function requestOrderStatus(query: string): Promise<{ status: number; body: unknown }> { + const app = express(); + app.use('/api/commerce', createGoDaddyPaymentsRouter(configuration)); + const server = app.listen(0, '127.0.0.1'); + await once(server, 'listening'); + try { + const address = server.address(); + if (!address || typeof address === 'string') throw new Error('Expected a listening TCP server'); + const response = await clientFetch( + `http://127.0.0.1:${address.port}/api/commerce/order-status${query}`, + ); + return { status: response.status, body: await response.json() }; + } finally { + await new Promise((resolve, reject) => + server.close((error) => (error ? reject(error) : resolve())), + ); + } + } + + it.each([ + '', + '?orderId=', + '?orderId=%20', + '?orderId=.', + '?orderId=..', + '?orderId=%20cart-1', + '?orderId=a&orderId=b', + ])( + 'returns 400 for invalid order ID query %j without an upstream request', + async (query): Promise => { + await expect(requestOrderStatus(query)).resolves.toEqual({ + status: 400, + body: { success: false, error: 'missing or invalid orderId query parameter' }, + }); expect(upstream).not.toHaveBeenCalled(); }, ); + + it('returns 400 when the Orders API rejects the order ID format', async (): Promise => { + upstream + .mockResolvedValueOnce(Response.json({ access_token: 'order-token' })) + .mockResolvedValueOnce(ordersApiInvalidId()); + await expect(requestOrderStatus(`?orderId=${order.id}`)).resolves.toEqual({ + status: 400, + body: { success: false, error: 'missing or invalid orderId query parameter' }, + }); + }); + + it.each([ + ['an Orders API NOT_FOUND', ordersApiNotFound()], + [ + 'another store', + Response.json({ order: { ...order, context: { ...order.context, storeId: 'another-store' } } }), + ], + ])('returns 404 for %s', async (_case, orderResponse): Promise => { + upstream + .mockResolvedValueOnce(Response.json({ access_token: 'order-token' })) + .mockResolvedValueOnce(orderResponse); + await expect(requestOrderStatus(`?orderId=${order.id}`)).resolves.toEqual({ + status: 404, + body: { success: false, error: 'Order not found' }, + }); + }); + + it.each([ + ['a denied token', [new Response('Invalid scope', { status: 403 })]], + [ + 'an upstream 500', + [ + Response.json({ access_token: 'order-token' }), + new Response('Private upstream details', { status: 500 }), + ], + ], + ['an incomplete order', [Response.json({ access_token: 'order-token' }), Response.json({})]], + [ + 'an order missing its store binding', + [ + Response.json({ access_token: 'order-token' }), + Response.json({ order: { ...order, context: { channelId: order.context.channelId } } }), + ], + ], + [ + 'a 404 from an unrouted path', + [ + Response.json({ access_token: 'order-token' }), + new Response('
Cannot GET /v1/x
', { status: 404 }), + ], + ], + [ + 'a different order ID', + [ + Response.json({ access_token: 'order-token' }), + Response.json({ order: { ...order, id: 'another-order' } }), + ], + ], + ])( + 'returns a generic 500 for %s and logs the detail server-side', + async (_case, responses): Promise => { + const log = vi.spyOn(console, 'error').mockImplementation((): void => {}); + for (const response of responses) upstream.mockResolvedValueOnce(response); + const result = await requestOrderStatus(`?orderId=${order.id}`); + expect(result).toEqual({ status: 500, body: { success: false, error: 'Failed to get order status' } }); + expect(log).toHaveBeenCalledWith('order-status: failed to get order status', expect.any(Error)); + }, + ); +}); + +describe('order-status route with another copy of the package', () => { + afterEach((): void => { + vi.doUnmock('@/lib/commerce/get-order-status'); + vi.resetModules(); + }); + + it.each([ + ['InvalidOrderIdError', 400, 'missing or invalid orderId query parameter'], + ['OrderNotFoundError', 404, 'Order not found'], + ])('matches a %s thrown by a different class by name', async (name, status, error): Promise => { + vi.resetModules(); + vi.doMock('@/lib/commerce/get-order-status', () => ({ + getOrderStatus: async (): Promise => { + throw Object.assign(new Error('from another copy'), { name }); + }, + })); + const { default: handler } = await import('./server/api/commerce/order-status/GET'); + const res = { locals: {}, status: vi.fn().mockReturnThis(), json: vi.fn().mockReturnThis() }; + await handler({ query: { orderId: order.id } } as never, res as never); + expect(res.status).toHaveBeenCalledWith(status); + expect(res.json).toHaveBeenCalledWith({ success: false, error }); + }); }); diff --git a/packages/commerce-server/src/index.ts b/packages/commerce-server/src/index.ts index fd69b46b..385a8993 100644 --- a/packages/commerce-server/src/index.ts +++ b/packages/commerce-server/src/index.ts @@ -19,6 +19,9 @@ export { export { type CommerceOrderStatus, getOrderStatus, + InvalidOrderIdError, + ORDER_STATUS_UNKNOWN, + OrderNotFoundError, } from './lib/commerce/get-order-status'; export { type CommerceRouterFeatures, diff --git a/packages/commerce-server/src/lib/commerce/get-order-status.ts b/packages/commerce-server/src/lib/commerce/get-order-status.ts index a01c66a3..48d5e4f5 100644 --- a/packages/commerce-server/src/lib/commerce/get-order-status.ts +++ b/packages/commerce-server/src/lib/commerce/get-order-status.ts @@ -6,10 +6,13 @@ import { authorizationHeaders, getOAuthAccessToken } from './checkout-subgraph'; import { type CommerceConfiguration, createRuntimeCommerceConfiguration } from './config'; import type { Money } from './gql'; +/** `CommerceOrderStatus.status` when the Orders API reports no payment status. */ +export const ORDER_STATUS_UNKNOWN = 'unknown'; + export interface CommerceOrderStatus { /** GoDaddy order id. */ id: string; - /** Payment status reported by Commerce, or 'unknown' when it is unavailable. */ + /** Payment status reported by Commerce (e.g. `PAID`, `PENDING`), or `ORDER_STATUS_UNKNOWN`. */ status: string; /** Total amount in the currency's smallest unit (cents for USD). */ amount: number; @@ -23,6 +26,20 @@ export interface CommerceOrderStatus { lineItems?: unknown[]; } +export class InvalidOrderIdError extends Error { + constructor() { + super('getOrderStatus: a valid orderId is required'); + this.name = 'InvalidOrderIdError'; + } +} + +export class OrderNotFoundError extends Error { + constructor() { + super('Order not found'); + this.name = 'OrderNotFoundError'; + } +} + interface OrderResponse { order?: { id: string; @@ -35,12 +52,34 @@ interface OrderResponse { }; } +function isBindingId(value: unknown): value is string { + return typeof value === 'string' && value !== ''; +} + +async function readErrorCode(response: Response): Promise { + try { + const body: unknown = await response.json(); + return body && typeof body === 'object' && 'code' in body && typeof body.code === 'string' + ? body.code + : null; + } catch { + return null; + } +} + export async function getOrderStatus( orderId: string, configuration: CommerceConfiguration = createRuntimeCommerceConfiguration(), ): Promise { - if (typeof orderId !== 'string' || !orderId.trim() || orderId === '.' || orderId === '..') { - throw new Error('getOrderStatus: a valid orderId is required'); + // `.` and `..` survive encodeURIComponent and would resolve the URL to the store resource. + if ( + typeof orderId !== 'string' || + !orderId || + orderId !== orderId.trim() || + orderId === '.' || + orderId === '..' + ) { + throw new InvalidOrderIdError(); } const { storeId, channelId, clientId, clientSecret, apiBaseUrl, currencyCode } = configuration.read(); @@ -61,19 +100,40 @@ export async function getOrderStatus( cache: 'no-store', }, ); - if (!response.ok) throw new Error(`Failed to load order: upstream returned ${response.status}`); + if (!response.ok) { + // The Orders API reports a missing order as 404 NOT_FOUND and an ID it can't decode as 422 + // VALIDATION_FAILED. A 404 without that code comes from an unrouted path or base URL. + const code = + response.status === 404 || response.status === 422 ? await readErrorCode(response) : undefined; + // Unread bodies can hold pooled connections while callers poll. + if (code === undefined) await response.body?.cancel(); + if (response.status === 404 && code === 'NOT_FOUND') throw new OrderNotFoundError(); + if (response.status === 422 && code === 'VALIDATION_FAILED') throw new InvalidOrderIdError(); + throw new Error(`Failed to load order: upstream returned ${response.status}`); + } const data = (await response.json()) as OrderResponse; const order = data?.order; - if (!order?.id || order.id !== orderId) throw new Error('Order lookup did not return the requested order'); - if (order.context?.storeId !== storeId || order.context?.channelId !== channelId) { - throw new Error('Order lookup returned a different store or channel'); + const context = order?.context; + // A binding missing either field is a malformed upstream response, not proof of another store or channel. + // So is a different order id: the lookup was by id, so it means upstream returned the wrong record. + if ( + !isBindingId(order?.id) || + order.id !== orderId || + !isBindingId(context?.storeId) || + !isBindingId(context?.channelId) + ) { + throw new Error('Order lookup did not return the requested order'); + } + // Report an order bound to another store or channel as missing so the route doesn't reveal it exists. + if (context.storeId !== storeId || context.channelId !== channelId) { + throw new OrderNotFoundError(); } const total = order.totals?.total; return { id: order.id, - status: order.statuses?.paymentStatus ?? 'unknown', + status: order.statuses?.paymentStatus ?? ORDER_STATUS_UNKNOWN, amount: total?.value ?? 0, currency: total?.currencyCode ?? currencyCode, createdAt: order.createdAt, diff --git a/packages/commerce-server/src/server/api/commerce/order-status/GET.ts b/packages/commerce-server/src/server/api/commerce/order-status/GET.ts index 797f5404..cf8e288e 100644 --- a/packages/commerce-server/src/server/api/commerce/order-status/GET.ts +++ b/packages/commerce-server/src/server/api/commerce/order-status/GET.ts @@ -11,8 +11,12 @@ * Query: * orderId - GoDaddy order id (required) * - * Response: { success: true, order: CommerceOrderStatus } - * order.status is the payment status returned by the authorized Orders API. + * Responses: + * 200 { success: true, order: CommerceOrderStatus } + * order.status is the payment status returned by the authorized Orders API. + * 400 missing, blank, padded, `.` or `..` orderId, or one the Orders API rejects as malformed + * 404 no order with that id in the configured store and channel + * 500 credential, upstream, or configuration failure * The host must authorize the caller's access to the requested order. */ import type { Request, Response } from 'express'; @@ -20,21 +24,35 @@ import type { Request, Response } from 'express'; import { commerceConfigurationForResponse } from '@/lib/commerce/config'; import { getOrderStatus } from '@/lib/commerce/get-order-status'; +const invalidOrderIdBody = { success: false, error: 'missing or invalid orderId query parameter' }; + +// Matched by name, not instanceof, so errors still match when a host has several copies of this package. +function hasErrorName(error: unknown, name: string): boolean { + return error instanceof Error && error.name === name; +} + export default async function handler(req: Request, res: Response): Promise { try { const { orderId } = req.query; - if (!orderId || typeof orderId !== 'string') { - res.status(400).json({ success: false, error: 'missing or invalid orderId query parameter' }); + // getOrderStatus validates string IDs; only repeated or nested query values need rejecting here. + if (typeof orderId !== 'string') { + res.status(400).json(invalidOrderIdBody); return; } const order = await getOrderStatus(orderId, commerceConfigurationForResponse(res)); res.status(200).json({ success: true, order }); } catch (error) { - res.status(500).json({ - success: false, - error: 'Failed to get order status', - message: error instanceof Error ? error.message : String(error), - }); + if (hasErrorName(error, 'InvalidOrderIdError')) { + res.status(400).json(invalidOrderIdBody); + return; + } + if (hasErrorName(error, 'OrderNotFoundError')) { + res.status(404).json({ success: false, error: 'Order not found' }); + return; + } + // The detail can name upstream statuses, credentials, or configuration, so it stays server-side. + console.error('order-status: failed to get order status', error); + res.status(500).json({ success: false, error: 'Failed to get order status' }); } }