fix: include accurate Allow headers on 405 responses

[cubap]

Summary

Resolves #287.

GitHub Copilot implemented this change to ensure every route-generated 405 response includes an accurate RFC 9110 Allow header.

• Added shared 405 response handling with endpoint-specific method lists.
• Added Allow: PATCH,POST to invalid PATCH override responses.
• Included HEAD where routes explicitly support it.
• Removed the generic success-response Allow values that advertised unsupported methods.
• Added regression assertions covering route-declared methods.

Verification

• node --env-file-if-exists=.env --import ./test/bootstrap.js --test routes/__tests__/route_wrappers.test.js: 27 passed.
• npm test: 22 test files passed, 0 failed.
• Manual in-process HTTP checks against an ephemeral Express server: 10 representative unsupported-method requests returned 405 with the expected Allow values.
• git diff --check: passed.

The standalone npm start probe was attempted, but requests did not complete reliably in the debug-instrumented terminal environment; the in-process server checks completed successfully.

[thehabes]

Static Review Comments

Branch: 287-headers-gh-copilot
Review Date: 2026-08-25
Reviewer: Pair Static Review - Claude & @thehabes

Claude and Bryan make mistakes. Verify all issues and suggestions. Avoid unnecessary scope creep.

Category	Issues Found
🔴 Critical	0
🟠 Major	2
🟡 Minor	5
🔵 Suggestions	5

Verification performed

Every finding below was checked against the running app (pm2, http://localhost:3001) rather than read off the diff alone.

• All 24 route-level 405s return the correct Allow — /v1/id/:_id, /v1/id/:_id/expanded, /v1/api/{query,search,search/phrase,create,bulkCreate,bulkUpdate,delete/:_id,overwrite,update,patch,set,unset,release/:_id}, /v1/since/:_id, /v1/history/:_id, /gog/{fragmentsInManuscript,glossesInManuscript,id/:_id}. No misses.
• HEAD genuinely works everywhere Allow advertises it. Confirmed 200 on a real record (5dc08060d5de6ba6e202993d) for /v1/id, /v1/id/../expanded, /v1/since, /v1/history, plus the explicit .head() on /v1/api/query. The GET,HEAD values are honest, not aspirational.
• Legacy express-urlrewrite paths inherit the header for free — /v1/api/getByProperties → POST,HEAD, /v1/api/batch_create → POST, /v1/api/create.action → POST.
• Unknown methods are handled — TRACE /v1/api/create → 405 Allow: POST; PROPFIND /v1/id/:_id → 405 Allow: GET,HEAD.
• npm test — 211 tests, 38 suites, 0 failures.
• git diff main...HEAD --check — clean.

The core of the issue is solved. Findings are about the edges.

Critical Issues 🔴

None.

Major Issues 🟠

🟠 Issue 1: OPTIONS is supported everywhere but appears in no Allow value

File: all 20 route modules (e.g. routes/create.js:12, routes/id.js:11, routes/id.js:17)
Category: Logic error / RFC 9110 compliance

Problem:

The cors middleware in app.js:32-57 short-circuits every OPTIONS request with a 204 before routing ever reaches the .all() fallback. Verified:

OPTIONS /v1/id/abc123    -> 204
OPTIONS /v1/api/create   -> 204
OPTIONS /v1/api/query    -> 204
OPTIONS /gog/id/abc123   -> 204

So OPTIONS is a method supported by every one of these target resources — but it is absent from all 20 hardcoded Allow values.

RFC 9110 §10.2.1 defines Allow as "the set of methods supported by the target resource." Issue #287 words the goal the same way: "Line up the Allow header value with the actual list of available methods. Do not emit values that are incorrect." The old blanket value was wrong by over-advertising; the new values are wrong by under-advertising, on the one method every route actually implements.

A conformant client that reads Allow: POST from a 405 will conclude preflight is unavailable and may skip it.

Current Code:

// routes/create.js
router.route('/')
    .post(auth.checkJwt, rest.verifyJsonContentType, controller.create)
    .all((req, res, next) => {
        rest.sendMethodNotAllowed(res, 'Improper request method for creating, please use POST.', 'POST')
    })

Suggested Fix:

Append OPTIONS to every value, ideally inside the helper so it cannot be forgotten:

// rest.js
const sendMethodNotAllowed = (res, message, allowedMethods) => {
    const methods = allowedMethods.split(",").map(m => m.trim()).filter(Boolean)
    if (!methods.includes("OPTIONS")) methods.push("OPTIONS")
    res.statusMessage = message
    res.set("Allow", methods.join(","))
    return res.status(405).end()
}

That yields GET,HEAD,OPTIONS, POST,OPTIONS, PATCH,POST,OPTIONS, etc. with no edit to the 20 call sites.

If the team decides OPTIONS should stay out because it is answered by CORS rather than by RERUM, that is a defensible call — but it should be a comment in rest.js, not an omission, since the next reader will re-file it.

How to Verify:

curl -s -o /dev/null -D - -X OPTIONS http://localhost:3001/v1/api/create | head -1   # 204 today
curl -s -o /dev/null -D - -X GET     http://localhost:3001/v1/api/create | grep -i allow
# expect: Allow: POST,OPTIONS

---

🟠 Issue 2: sendMethodNotAllowed silently emits Allow: undefined

File: rest.js:47-51
Category: Runtime error / unvalidated input

Problem:

allowedMethods is a required argument in spirit but optional in practice. Express res.set() stringifies whatever it is given, so a caller who forgets the third argument ships a syntactically invalid header rather than failing. Confirmed against Express:

status: 405 | Allow header value: "undefined"

All 20 current call sites pass the argument, so this is latent — but the helper is now a public export on rest, the PR's whole contract is "the Allow value is never wrong," and this failure mode is silent.

Current Code:

const sendMethodNotAllowed = (res, message, allowedMethods) => {
    res.statusMessage = message
    res.set("Allow", allowedMethods)
    return res.status(405).end()
}

Suggested Fix:

const sendMethodNotAllowed = (res, message, allowedMethods) => {
    if (!allowedMethods) throw new Error("sendMethodNotAllowed() requires an allowedMethods list for the Allow header.")
    res.statusMessage = message
    res.set("Allow", allowedMethods)
    return res.status(405).end()
}

A throw is appropriate here — it is a programming error, not a request error, and it surfaces at the first test run instead of in production headers. See Suggestion 1 for the version that removes the argument entirely.

How to Verify:

Temporarily drop the third argument from one route, hit it with a wrong method, and confirm the response fails loudly instead of returning Allow: undefined.

---

Minor Issues 🟡

🟡 Issue 3: Mangled indentation in routes/patchUpdate.js

File: routes/patchUpdate.js:15
Category: Code hygiene

Problem:

This file indents with tabs. The replacement line uses three tabs followed by three spaces where its siblings use two tabs. cat -A confirms:

^I.all((req, res, next) => {$
^I^I^I   rest.sendMethodNotAllowed(res, '...', 'PATCH,POST')$
^I})$

git diff --check does not catch this (it is not trailing whitespace), and it is the only one of the 20 edited routes with the defect — a copy-paste artifact.

Suggested Fix:

	.all((req, res, next) => {
		rest.sendMethodNotAllowed(res, 'Improper request method for updating, please use PATCH to alter existing keys on this object.', 'PATCH,POST')
	})

How to Verify:

sed -n '13,17p' routes/patchUpdate.js | cat -A

---

🟡 Issue 4: sendMethodNotAllowed is the only undocumented export in rest.js

File: rest.js:47
Category: Unnecessary/missing comments

Problem:

checkPatchOverrideSupport, createPatchOverrideMiddleware, hasMultipleContentTypes, verifyJsonContentType, verifyEitherContentType, and messenger all carry JSDoc blocks. The new shared helper carries none, and the project's CLAUDE.md calls for JSDoc as code is touched. It is also the one place worth recording why the Allow value is required.

Suggested Fix:

/**
 * Send a RESTful 405 Method Not Allowed with the Allow header RFC 9110 §15.5.6 requires.
 * The Allow value must list every method the target resource supports, so callers pass the
 * route's own method list rather than a blanket value.
 *
 * @param {Object} res - Express response object
 * @param {string} message - Reason phrase explaining the correct method for this endpoint
 * @param {string} allowedMethods - Comma-delimited supported methods, e.g. "PATCH,POST"
 * @returns {Object} The ended Express response
 */
const sendMethodNotAllowed = (res, message, allowedMethods) => {

---

🟡 Issue 5: Two comments now contradict the code

File: app.js:92, rest.js:211
Category: Outdated comments

Problem:

Both comments claim 405s are handled in api-routes.js. They are not, and were not — they are handled in the individual route modules, now via rest.sendMethodNotAllowed. app.js:92 additionally says "they res.send()", but these responses res.end() with an empty body (Content-Length: 0, verified). This PR is the moment 405 handling became centralized, so it is the natural moment to correct the pointers.

Current Code:

// app.js:92
 * Important to note api-routes.js handles all the 405s without failing to here - they res.send()

// rest.js:210-212
        case 405:
            // These are all handled in api-routes.js already.
            break

Suggested Fix:

// app.js
 * Important to note the route modules handle all the 405s via rest.sendMethodNotAllowed() without failing to here

        case 405:
            // These are all handled by rest.sendMethodNotAllowed() in the route modules already.
            break

---

🟡 Issue 6: The 405 assertion is order-sensitive

File: routes/__tests__/route_wrappers.test.js:109-115
Category: Test quality

Problem:

getAllowedMethods() joins Object.keys(route.methods) in registration order and the assertion compares exact strings. Reordering .get() and .post() on /id/:_id/expanded — a semantically irrelevant change — would flip the expectation to POST,GET,HEAD and fail against the still-correct hardcoded GET,POST,HEAD. The test would be reporting a defect that does not exist.

Suggested Fix:

Compare as sets so the test pins the contract (which methods) and not an incidental detail (what order):

function assertUnsupportedMethodOnPath(router, path) {
  const route = getRoute(router, path)
  const fallbackLayer = route.stack.at(-1)
  assert.ok(fallbackLayer, `Expected fallback .all() layer for '${path}'`)

  const { res, nextCalls } = invokeLayer(fallbackLayer)

  assert.strictEqual(res.statusCode, 405)
  assert.deepStrictEqual(
    (res.headers.Allow ?? '').split(',').map(m => m.trim()).filter(Boolean).sort(),
    getAllowedMethods(route).split(',').sort()
  )
  assert.strictEqual(res.ended, true)
  assert.deepStrictEqual(nextCalls, [])
}

---

🟡 Issue 7: The OpenAPI contract documents 405s without the Allow header

File: openapi/contracts/core-provider.openapi.yaml:94, 120, 154, 481, 540, 599
Category: Documentation drift

Problem:

Six operations document a 405 response and none declare an Allow header, so the published contract still does not promise what this PR now guarantees. The file already has a components/headers section (line 646) with CanonicalLocation, CurrentOverwrittenVersion, etc., so the pattern exists. __tests__/core_provider_contract.test.js only syncs paths and methods — it never inspects response headers, so nothing catches this.

Worth noting separately: 405 is documented on only 6 of the 24 operations that can produce one. /id/{id}, /since/{id}, /history/{id}, /api/query, /api/create, /api/delete/{id} and the rest all return 405 at runtime with no contract entry.

Suggested Fix:

  headers:
    AllowedMethods:
      description: >-
        The methods this resource supports, per RFC 9110 §10.2.1. Required on every 405.
      schema:
        type: string
        example: GET,HEAD,OPTIONS

        '405':
          description: Method not allowed — this endpoint only permits GET, HEAD, and POST.
          headers:
            Allow:
              $ref: '#/components/headers/AllowedMethods'

Filling in the 18 missing 405 blocks is a larger job and reasonable to defer to its own issue — flagging so it is a decision rather than an oversight.

---

Suggestions 🔵

🔵 Suggestion 1: Derive Allow from the route instead of hardcoding it 20 times

File: rest.js:47, all 20 route modules

The test already proves the correct value is derivable from route.methods. Express exposes that same object to the handler as req.route, so the implementation can derive it too — which deletes the entire class of drift the hardcoded strings invite (a future .head() added to a route silently leaves its Allow stale, and nothing but a hand-edit fixes it).

Verified working — output is byte-identical to the hardcoded values:

/**
 * Build the .all() fallback that answers unsupported methods with a 405 and an accurate
 * Allow header derived from the methods actually registered on this route.
 *
 * @param {string} message - Reason phrase explaining the correct method for this endpoint
 * @returns {Function} Express handler
 */
const methodNotAllowed = (message) => (req, res) => {
    const methods = Object.keys(req.route?.methods ?? {})
        .filter(method => method !== "_all")
        .map(method => method.toUpperCase())
    // Express dispatches HEAD to the GET handler when no explicit .head() is registered
    if (methods.includes("GET") && !methods.includes("HEAD")) methods.push("HEAD")
    if (!methods.includes("OPTIONS")) methods.push("OPTIONS")
    res.statusMessage = message
    res.set("Allow", methods.join(","))
    return res.status(405).end()
}

Each route then loses its hardcoded list, and Issues 1 and 2 dissolve:

router.route('/')
    .post(auth.checkJwt, rest.verifyJsonContentType, controller.create)
    .all(rest.methodNotAllowed('Improper request method for creating, please use POST.'))

Proof run against Express 5:

DELETE  /a  -> 405  Allow: GET,POST,HEAD     (route: .get .post .all)
GET     /b  -> 405  Allow: POST,HEAD         (route: .post .head .all)
PUT     /c  -> 405  Allow: PATCH,POST        (route: .patch .post .all)

The tests then assert real behavior rather than mirroring the implementation's own derivation. This is a bigger diff than the PR currently carries, so it is a judgment call — the hardcoded version is correct today, and correct-and-shipped beats elegant-and-pending.

---

🔵 Suggestion 2: Six routes still answer 404 where 405 is correct

Outside this PR's scope, but it is the other half of "all 405 responses are covered." These have no .all() fallback, so a wrong method falls through to the catch-all 404 in app.js:99. Verified:

Request	Today	Should be
POST /v1/api	404	405 Allow: GET,HEAD,OPTIONS
POST /	404	405 Allow: GET,HEAD,OPTIONS
POST /client/register	404	405 Allow: GET,HEAD,OPTIONS
GET /client/request-new-access-token	404	405 Allow: POST,OPTIONS
GET /client/request-new-refresh-token	404	405 Allow: POST,OPTIONS
POST /client/verify	404	405 Allow: GET,HEAD,OPTIONS

Returning 404 for a valid resource reached with the wrong method is misleading — it tells the client the resource does not exist. Recommend a follow-up issue rather than growing this PR.

---

🔵 Suggestion 3: OPTIONS responses carry no Allow at all

Related to Issue 1 and to the utils.js removal. Before this PR, configureWebAnnoHeadersFor() put an Allow on 200 responses; it was wrong, and removing it was the right call. The side effect is that Allow now appears only on 405s.

RFC 9110 §9.3.7 says a successful OPTIONS response SHOULD advertise supported methods, and OPTIONS is the request a client makes precisely to ask that question. Today it answers 204 with nothing:

OPTIONS /v1/id/abc123 -> 204   Allow: <none>

Clients can read Access-Control-Allow-Methods from the CORS layer, but that is the blanket GET,OPTIONS,HEAD,PUT,PATCH,DELETE,POST from app.js:34 — the same over-advertisement issue #287 objected to, just relocated. Worth its own issue.

---

🔵 Suggestion 4: The 405 explanation is invisible to most clients

Every 405 sends Content-Length: 0 and puts its message in the HTTP reason phrase:

HTTP/1.1 405 Improper request method for creating, please use POST.
Allow: POST
Content-Length: 0

The reason phrase is not surfaced by fetch(), is dropped entirely by HTTP/2, and is invisible in most tooling. Since rest.messenger already returns text/plain bodies for other error classes, 405 could send its message as a body for consistency. Pre-existing behavior and a client-visible change, so it belongs in its own issue — noting it because this PR consolidated all 20 of these into one helper, which makes it a one-line change if the team wants it.

---

🔵 Suggestion 5: req and next are unused in all 20 fallback handlers

.all((req, res, next) => {
    rest.sendMethodNotAllowed(res, '...', 'POST')
})

Neither is read. Harmless and consistent with the pre-existing style, so leaving it is fine — but adopting Suggestion 1 removes the whole signature anyway, and if that is declined, .all((req, res) => ...) is more honest about what the handler touches.

---

What this PR gets right

• It fixes the real bug. The blanket Allow: GET,OPTIONS,HEAD,PUT,PATCH,DELETE,POST in utils.js was actively lying on every 200 — issue 405 Responses Need Allow Header #287 named it, and removing it was the correct call rather than trying to make it per-route.
• Express's implicit HEAD→GET dispatch is handled correctly. Adding HEAD alongside GET is easy to get wrong in either direction, and it is right on all five GET routes — confirmed against a live record, not assumed.
• /v1/api/query is exactly right. It has an explicit .head() and no .get(), and its value is POST,HEAD rather than a reflexive POST,GET,HEAD. That is the case a careless pass would have broken.
• The legacy compatibility layer works for free. express-urlrewrite paths land on the rewritten route's fallback and inherit the correct value with no special casing.
• One helper, not 20 copy-pasted blocks. The consolidation is what makes Issues 1 and 2 one-line fixes instead of 20-file sweeps.
• The tests derive expectations from Express route metadata rather than restating the same literals the implementation uses, so they can actually catch drift.

---

If there are significant code changes in response to this review please test those changes. Run the application manually and test. Run internal programmatic tests when applicable.