fix(commerce-server): return 400/404 from order-status instead of 500

.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.

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.

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<typeof vi.fn<typeof fetch>>;
 
 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<void> => {
+    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<void> => {
+  ])('reports an order bound to another store or channel as not found', async (result): Promise<void> => {
     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<void> => {
+    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<void> => {
+    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('<pre>Cannot GET /v1/x</pre>', { 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<void> => {
+    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<void> => {
+      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<void> => {
-      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<void>((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<void> => {
+      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<void> => {
+    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<void> => {
+    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('<pre>Cannot GET /v1/x</pre>', { 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<void> => {
+      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<void> => {
+    vi.resetModules();
+    vi.doMock('@/lib/commerce/get-order-status', () => ({
+      getOrderStatus: async (): Promise<never> => {
+        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 });
+  });
 });

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,