diff --git a/backend/src/server/errorCodes.test.ts b/backend/src/server/errorCodes.test.ts new file mode 100644 index 0000000..37f4e32 --- /dev/null +++ b/backend/src/server/errorCodes.test.ts @@ -0,0 +1,108 @@ +// The error codes that a client really gets from the server: the real Apollo +// Server with the real schema, depth limit and formatError, run without HTTP. +// These tests need no database: an invalid query is rejected before any +// resolver runs, and the one resolver that runs gets a database that fails. +// formatError.test.ts checks the function on its own; this file checks that it +// works together with Apollo, which wraps its own errors in a way a hand-made +// test error does not copy. + +import type { ApolloServer } from '@apollo/server' +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest' +import type { PrismaClient } from '../generated/prisma/client.js' +import { createApolloServer } from './app.js' +import type { Config } from './config.js' +import { type Context, createContext } from './context.js' +import { MAX_QUERY_DEPTH } from './depthLimit.js' +import { INTERNAL_ERROR_MESSAGE } from './formatError.js' + +const config: Config = { + port: 3001, + imageBaseUrl: 'http://images.test/images', + corsOrigins: [], +} + +/** A database that fails on every call, like one that is down. */ +const brokenDatabase = new Proxy( + {}, + { + get: () => ({ + findMany: () => Promise.reject(new Error('password=secret at db:5432')), + }), + }, +) as unknown as PrismaClient + +describe('the error codes the server gives', () => { + let server: ApolloServer + + beforeAll(async () => { + server = createApolloServer() + await server.start() + }) + + afterAll(async () => { + await server.stop() + }) + + async function errorsOf(request: { + query: string + variables?: Record + operationName?: string + }) { + const response = await server.executeOperation(request, { + contextValue: createContext(brokenDatabase, config), + }) + if (response.body.kind !== 'single') throw new Error('Expected one result') + return response.body.singleResult.errors ?? [] + } + + it('gives an unknown field GRAPHQL_VALIDATION_FAILED', async () => { + const [error] = await errorsOf({ query: '{ nope }' }) + expect(error?.extensions?.['code']).toBe('GRAPHQL_VALIDATION_FAILED') + expect(error?.message).toMatch(/Cannot query field "nope"/) + }) + + it('gives a syntax error GRAPHQL_PARSE_FAILED', async () => { + const [error] = await errorsOf({ query: '{ apparatus ' }) + expect(error?.extensions?.['code']).toBe('GRAPHQL_PARSE_FAILED') + expect(error?.message).toMatch(/Syntax Error/) + }) + + it('gives a query that is too deep GRAPHQL_VALIDATION_FAILED', async () => { + // apparatus → groups → apparatus → … : each pair adds two levels. + let query = 'id' + for (let i = 0; i < MAX_QUERY_DEPTH; i++) { + query = i % 2 === 0 ? `apparatus { ${query} }` : `groups { ${query} }` + } + const [error] = await errorsOf({ query: `{ apparatus { ${query} } }` }) + expect(error?.extensions?.['code']).toBe('GRAPHQL_VALIDATION_FAILED') + expect(error?.message).toMatch(/levels deep/) + }) + + it('gives a missing or invalid variable BAD_USER_INPUT', async () => { + const query = 'query ($id: ID!) { element(id: $id) { id } }' + const missing = await errorsOf({ query }) + const invalid = await errorsOf({ query, variables: { id: { a: 1 } } }) + for (const [error] of [missing, invalid]) { + expect(error?.extensions?.['code']).toBe('BAD_USER_INPUT') + expect(error?.message).toMatch(/\$id/) + } + }) + + it('gives an unknown operationName OPERATION_RESOLUTION_FAILURE', async () => { + const [error] = await errorsOf({ + query: 'query A { apparatus { id } }', + operationName: 'B', + }) + expect(error?.extensions?.['code']).toBe('OPERATION_RESOLUTION_FAILURE') + }) + + it('still hides an unexpected error in a resolver', async () => { + const log = vi.spyOn(console, 'error').mockImplementation(() => {}) + const [error] = await errorsOf({ query: '{ apparatus { id } }' }) + log.mockRestore() + + expect(error?.extensions?.['code']).toBe('INTERNAL_SERVER_ERROR') + expect(error?.message).toBe(INTERNAL_ERROR_MESSAGE) + expect(JSON.stringify(error)).not.toContain('secret') + }) +}) diff --git a/backend/src/server/formatError.test.ts b/backend/src/server/formatError.test.ts index 42d8ca7..8cbb827 100644 --- a/backend/src/server/formatError.test.ts +++ b/backend/src/server/formatError.test.ts @@ -1,11 +1,20 @@ +import { ApolloServerErrorCode } from '@apollo/server/errors' import { GraphQLError } from 'graphql' import { afterEach, describe, expect, it, vi } from 'vitest' import { BadUserInputError, + InvalidItemsError, + LimitReachedError, + NameTakenError, + NotFoundError, NotImplementedError, UnauthenticatedError, } from '../errors.js' -import { formatError } from './formatError.js' +import { + APOLLO_CLIENT_ERROR_CODES, + INTERNAL_ERROR_MESSAGE, + formatError, +} from './formatError.js' /** What Apollo hands to formatError for an error thrown in a resolver. */ const thrownInResolver = (error: Error) => @@ -15,6 +24,17 @@ const thrownInResolver = (error: Error) => extensions: { code: 'INTERNAL_SERVER_ERROR' }, }) +/** + * What Apollo hands to formatError for an error of its own (a syntax error, an + * invalid query, a bad variable): it wraps the GraphQL error in another one, so + * `originalError` is set. This is the shape that was once hidden by mistake. + */ +const raisedByApollo = (message: string, code: string) => + new GraphQLError(message, { + originalError: new GraphQLError(message), + extensions: { code }, + }) + describe('formatError', () => { afterEach(() => { vi.restoreAllMocks() @@ -24,6 +44,13 @@ describe('formatError', () => { [new BadUserInputError('first must be 1–100'), 'BAD_USER_INPUT'], [new UnauthenticatedError(), 'UNAUTHENTICATED'], [new NotImplementedError('login'), 'NOT_IMPLEMENTED'], + [ + new InvalidItemsError('A routine on vault has only one vault.'), + 'INVALID_ITEMS', + ], + [new NotFoundError('The gymnast'), 'NOT_FOUND'], + [new NameTakenError(), 'NAME_TAKEN'], + [new LimitReachedError('A coach can have 200 gymnasts.'), 'LIMIT_REACHED'], ])('gives %s its own code and keeps the message', (error, code) => { const error2 = thrownInResolver(error) const result = formatError(error2.toJSON(), error2) @@ -39,7 +66,7 @@ describe('formatError', () => { ) const result = formatError(error.toJSON(), error) - expect(result.message).toBe('Internal server error') + expect(result.message).toBe(INTERNAL_ERROR_MESSAGE) expect(result.extensions).toEqual({ code: 'INTERNAL_SERVER_ERROR' }) expect(JSON.stringify(result)).not.toContain('secret') expect(log).toHaveBeenCalledOnce() @@ -56,15 +83,31 @@ describe('formatError', () => { }) for (const error of [withOtherCode, withoutCode]) { const result = formatError(error.toJSON(), error) - expect(result.message).toBe('Internal server error') + expect(result.message).toBe(INTERNAL_ERROR_MESSAGE) expect(result.extensions).toEqual({ code: 'INTERNAL_SERVER_ERROR' }) } }) - it('leaves the errors from GraphQL itself as they are', () => { - const error = new GraphQLError('The query is 9 levels deep.', { - extensions: { code: 'GRAPHQL_VALIDATION_FAILED' }, - }) - expect(formatError(error.toJSON(), error)).toEqual(error.toJSON()) + it.each([...APOLLO_CLIENT_ERROR_CODES])( + 'lets an error with the Apollo code %s through, with its message', + (code) => { + const log = vi.spyOn(console, 'error').mockImplementation(() => {}) + const error = raisedByApollo('Something the client can fix.', code) + const result = formatError(error.toJSON(), error) + + expect(result).toEqual(error.toJSON()) + expect(result.extensions?.['code']).toBe(code) + expect(log).not.toHaveBeenCalled() + }, + ) + + it("has decided on every code in Apollo's enum", () => { + // A new code in a later Apollo version fails here, so that someone decides + // whether the client or the server is to blame for it. + for (const code of Object.values(ApolloServerErrorCode)) { + expect(APOLLO_CLIENT_ERROR_CODES.has(code)).toBe( + code !== ApolloServerErrorCode.INTERNAL_SERVER_ERROR, + ) + } }) }) diff --git a/backend/src/server/formatError.ts b/backend/src/server/formatError.ts index 0be3fde..bc32bc7 100644 --- a/backend/src/server/formatError.ts +++ b/backend/src/server/formatError.ts @@ -1,14 +1,47 @@ // How errors look to the client (see "Error handling" in docs/API.md). // - The errors we throw on purpose (BadUserInputError and the others in // src/errors.ts) get their code in `extensions.code`. -// - Errors from GraphQL itself (an invalid query, a query that is too deep) -// keep their own code, for example GRAPHQL_VALIDATION_FAILED. +// - Errors that Apollo Server or GraphQL gives a client error code (an +// invalid query, a syntax error, a bad variable, a query that is too deep) +// keep their own code and message, for example GRAPHQL_VALIDATION_FAILED. // - Anything else is a bug or a failure on our side: the client gets a // general message and no details, and the real error goes to the log. +import { ApolloServerErrorCode } from '@apollo/server/errors' import type { GraphQLFormattedError } from 'graphql' import { isClientError } from '../errors.js' +/** + * The codes that Apollo Server gives to errors that are the client's doing: + * something wrong with the request, not with the server. Every one is listed by + * name, so it is clear what is let through. INTERNAL_SERVER_ERROR is left out on + * purpose: it is the code of an unexpected error, which is hidden. + * + * formatError.test.ts checks that each code in Apollo's enum is either here or + * INTERNAL_SERVER_ERROR, so a new code in a later Apollo version fails the test + * and has to be decided on. + */ +export const APOLLO_CLIENT_ERROR_CODES: ReadonlySet = new Set([ + // The operation has a syntax error. + ApolloServerErrorCode.GRAPHQL_PARSE_FAILED, + // The operation does not fit the schema, or breaks a validation rule such as + // the depth limit (src/server/depthLimit.ts). + ApolloServerErrorCode.GRAPHQL_VALIDATION_FAILED, + // A variable or an argument has an invalid value. + ApolloServerErrorCode.BAD_USER_INPUT, + // The request was wrong before it could be read as a GraphQL operation. + ApolloServerErrorCode.BAD_REQUEST, + // The operation is valid, but the server cannot tell which one to run (for + // example an unknown operationName). + ApolloServerErrorCode.OPERATION_RESOLUTION_FAILURE, + // Automatic persisted queries: the hash is unknown, or the feature is off. + ApolloServerErrorCode.PERSISTED_QUERY_NOT_FOUND, + ApolloServerErrorCode.PERSISTED_QUERY_NOT_SUPPORTED, +]) + +/** What the client reads when an unexpected error is hidden. */ +export const INTERNAL_ERROR_MESSAGE = 'Internal server error' + /** * The error that a resolver threw. GraphQL wraps it in its own error and keeps * it as `originalError`. This reads that field directly and does not use @@ -20,29 +53,42 @@ function unwrapResolverError(error: unknown): unknown { return original ?? error } +/** Apollo Server or GraphQL itself says the client sent something wrong. */ +function hasApolloClientCode(formatted: GraphQLFormattedError): boolean { + const code = formatted.extensions?.['code'] + return typeof code === 'string' && APOLLO_CLIENT_ERROR_CODES.has(code) +} + +/** The answer for an unexpected error: no details, and the code that says so. */ +function hiddenError(formatted: GraphQLFormattedError): GraphQLFormattedError { + return { + message: INTERNAL_ERROR_MESSAGE, + extensions: { code: ApolloServerErrorCode.INTERNAL_SERVER_ERROR }, + ...(formatted.path ? { path: formatted.path } : {}), + } +} + export function formatError( formatted: GraphQLFormattedError, error: unknown, ): GraphQLFormattedError { const original = unwrapResolverError(error) + + // 1. An error we threw on purpose. if (isClientError(original)) { return { ...formatted, extensions: { code: original.code } } } - // An error that wraps something we did not throw on purpose is always masked, - // whatever code it carries. A bare error without a code is masked too. - const wrapsUnknownError = original !== error - const code = formatted.extensions?.['code'] - if ( - wrapsUnknownError || - code === undefined || - code === 'INTERNAL_SERVER_ERROR' - ) { - console.error('Internal server error:', original) - return { - message: 'Internal server error', - extensions: { code: 'INTERNAL_SERVER_ERROR' }, - ...(formatted.path ? { path: formatted.path } : {}), - } + + // 2. An error with one of Apollo's client error codes. Apollo wraps these in + // another error, so `original` can differ from `error` here: only the code + // says whether the client or the server is to blame. A resolver that + // throws an unexpected error never gets one of these codes (it gets + // INTERNAL_SERVER_ERROR), so such an error is still hidden below. + if (hasApolloClientCode(formatted)) { + return formatted } - return formatted + + // 3. Anything else, with whatever code or none: hide it and log the real one. + console.error('Internal server error:', original) + return hiddenError(formatted) } diff --git a/docs/AI_USAGE.md b/docs/AI_USAGE.md index bc5eb73..c29bf28 100644 --- a/docs/AI_USAGE.md +++ b/docs/AI_USAGE.md @@ -371,3 +371,21 @@ - **Review:** The team reviewed the theme changes and confirmed the colors should come from the existing CSS variables rather than new palette values. +## Error codes of invalid queries (`backend/src/server/formatError.ts`) + +- **Tool:** Claude Code +- **What the team did:** Wrote issue #93 (invalid queries came back as + `INTERNAL_SERVER_ERROR`) and decided the rule: a mistake in the request keeps + its real code and message, and an unexpected error is still hidden. Chose a + list of allowed codes over hiding only `INTERNAL_SERVER_ERROR`, because an + unknown error must not leak by default, and asked that every Apollo code is + listed by name. +- **What Claude did:** Ran `formatError` against a real Apollo Server and found + that a missing variable and an unknown `operationName` were hidden too, not + only syntax and validation errors. Compared the Apollo documentation and + other guides, wrote the named list `APOLLO_CLIENT_ERROR_CODES`, the tests + (a real server without a database, one test per own error class, and a test + that fails when a new Apollo code has not been decided on) and the text in + `docs/API.md`. The old unit test used an error without `originalError`, + which is not how Apollo sends them, so it passed while the server test failed; + the new tests use the real shape, and turning the list off makes them fail. diff --git a/docs/API.md b/docs/API.md index 180a2c9..1da696c 100644 --- a/docs/API.md +++ b/docs/API.md @@ -244,9 +244,14 @@ Errors are thrown from the resolvers as GraphQL errors with a machine-readable c | `INVALID_ITEMS` | The items do not fit the routine: a vault outside vault, an element on vault, more than one vault, or an item from another apparatus (constraints 7 and 8). Also applies to a removed item unless it is already in the routine or gymnast's list. | | `BAD_USER_INPUT` | Empty or overlong name, malformed id, `first` out of range, search text over 80 characters or 8 words, unknown apparatus/group, a group from another apparatus, invalid cursor or sort, difficulty sort without an apparatus, difficulty filter with vault, or difficulty value outside 0.1–1.0 steps. | | `LIMIT_REACHED` | The coach has reached a limit: 200 gymnasts, 50 routines per gymnast, 50 items in a routine, or 100 items in a gymnast's list. See [USER_LIST_PAGINATION.md](USER_LIST_PAGINATION.md). | -| `GRAPHQL_VALIDATION_FAILED` | Added by Apollo Server when the query is invalid or exceeds the depth limit. | +| `GRAPHQL_PARSE_FAILED` | Added by Apollo Server when the query has a syntax error. | +| `GRAPHQL_VALIDATION_FAILED` | Added by Apollo Server when the query is invalid (for example an unknown field) or exceeds the depth limit. | | `INTERNAL_SERVER_ERROR` | Added by Apollo Server for an unexpected error. The client shows a general message without details. | +`BAD_USER_INPUT` is also given by Apollo Server when a variable is missing or has an invalid value, and `OPERATION_RESOLUTION_FAILURE` when the `operationName` does not match an operation in the request. + +The server lets through the errors that are the client's doing: our own errors above, and the errors that Apollo Server gives one of its client error codes (`GRAPHQL_PARSE_FAILED`, `GRAPHQL_VALIDATION_FAILED`, `BAD_USER_INPUT`, `BAD_REQUEST`, `OPERATION_RESOLUTION_FAILURE` and the two persisted query codes). They keep their code and message. Every other error is hidden: the client gets `INTERNAL_SERVER_ERROR` and the message "Internal server error", and the real error goes to the log. The list of codes is `APOLLO_CLIENT_ERROR_CODES` in `backend/src/server/formatError.ts`, and a test fails if a new Apollo version adds a code that nobody has decided on. + An error response looks like this: ```json @@ -267,6 +272,12 @@ An error response looks like this: - **`NOT_FOUND` also for other coaches' data.** A separate "forbidden" answer would tell a user which ids exist. The same answer in both cases follows constraint 9 (a coach can only read and change their own gymnasts and routines) without leaking anything. - **Plain GraphQL errors and not typed result unions.** A union such as `Routine | NameTaken | InvalidItems` makes the compiler force the client to handle each error, but it adds a type and a fragment per error to every mutation. For a prototype with a few failure cases this costs more than it gives. If the number of error cases grows, the unions can be introduced for single mutations without changing the rest of the schema. - **Readable messages for errors the user can fix.** `NAME_TAKEN` and `INVALID_ITEMS` have messages that the UI can show as they are. +- **Hide by default, and let through only what is named.** The message of an unexpected error can hold a database address, a file path or a password, so the client must never get it. We let through our own errors and the errors with one of Apollo's client error codes, and hide everything else. The alternative was to hide only errors with the code `INTERNAL_SERVER_ERROR` and let every other code through. That is shorter, but an unexpected error that carries some other code would then show its message to the client. Apollo's documentation shows `formatError` for masking, and the GraphQL Foundation's error masking guide recommends a list of codes that are safe to show. The price is that a client error whose code is missing from the list is hidden too. That is exactly how issue #93 happened (an earlier rule hid more than it should), so the list is covered by tests, see below. +- **Decide by the error code, not by the shape of the error.** Apollo wraps its own errors (a syntax error, an invalid query, a bad variable) in another error, just like it wraps an error thrown in a resolver, so both have an `originalError` and the shape cannot tell them apart. Telling them apart by whether the error has a `path` (only errors from resolvers have one) would work, but it depends on how graphql-js builds its errors. The code is part of Apollo's documented interface, and Apollo's own examples check `extensions.code`. +- **Every Apollo code is listed by name, and a test checks the list.** `APOLLO_CLIENT_ERROR_CODES` in `backend/src/server/formatError.ts` names each code from `ApolloServerErrorCode`, with a comment on each. `INTERNAL_SERVER_ERROR` is left out on purpose. A test goes through all the codes in Apollo's enum and fails if one is neither on the list nor `INTERNAL_SERVER_ERROR`. When Apollo adds a code in a later version, someone has to decide if the client or the server is to blame, and the choice is not made by accident. +- **Our own errors are classes with a name.** Everything that can go wrong because of the request is thrown as one of the classes in `backend/src/errors.ts` (`BadUserInputError`, `NotFoundError`, `InvalidItemsError` and so on). `isClientError` recognises them, so each keeps its code and message. A plain `Error` is never used for a mistake the user can fix, because it would be hidden as an internal error. The plain `Error`s that remain are in start-up and the seed scripts, which no request reaches. +- **The tests run the real Apollo Server.** `backend/src/server/errorCodes.test.ts` sends invalid queries to the real server, with the real schema, depth limit and `formatError`, and needs no database. The first version of the unit test built its error by hand, without the `originalError` that Apollo adds, so it passed while the server test failed. A hand-made error only tests what its author already knew. +- **No stack trace and no error id in the response.** The server runs with `includeStacktraceInErrorResponses: false`. The real error is written to the log, and the client gets a general message. An id that links the response to the log line (the GraphQL Foundation guide suggests one) can be added if the logs become hard to read. The price of this choice is that the types do not force the client to handle the errors. We handle that with tests of the error cases (see the testing section in the README when it exists).