Skip to content

OpenAPI declares a response body for statuses answered with no body #7122

Description

@gustavosbarreto

Summary

The OpenAPI spec declares a JSON body for statuses the server answers without one, so the
response validator refuses those responses. Under strict the validator replaces them with
a 500.

Why the responses have no body

c.NoContent(status) (echo/v5@v5.3.1/context.go:726) only calls WriteHeader. No body is
written, so Go never sniffs a type and no Content-Type header is sent. That is correct:
RFC 9110 asks for Content-Type on a message that has content. On the wire:

HTTP/1.1 401 Unauthorized
Content-Length: 0
(no Content-Type)

kin-openapi resolves the empty Content-Type to */*, finds no such media type in the
declared content, and returns a ResponseError
(openapi3filter/validate_response.go). A status that is not declared at all passes
silently, which is why this went unnoticed for so long.

Scale

Two independent counts, both measured against the bundled spec.

By middleware. Every authorization middleware answers bodyless:
Authorize, BlockAPIKey, RequiresPermission and RequiresTenant
(server/api/routes/middleware/authorize.go:18,27,38,52,72,82), plus the 401 in
authn.go. So any route behind them can produce a bodyless 401 or 403. The spec declares
401 with a body on 150 operations and 403 on 92. Probing the validator with a
bodyless 403 fails 92 of the 93 operations that declare one.

By handler. Mapping every handler that calls NoContent to its route gives 66
(route, status) pairs that exist in the spec. Of those, 35 would fail validation today:

status pairs examples
400 11 GET /api/devices, GET /api/sessions, GET /api/namespaces
401 13 POST /api/ssh-identities, GET /api/ssh-approvals/{code}
200 7 POST /api/devices/{uid}/tunnels, DELETE /api/namespaces/{tenant}/members
403 3 GET /api/namespaces/{tenant}, POST /api/devices/pairing/prepare
404 1 GET /api/namespaces/{tenant}

NoContent call sites, excluding tests and mocks: 200 (35 core, 27 cloud), 400 (22, 12),
401 (15, 2), 403 (10, 2), 402 (0, 7), 404 (1, 0), 500 (0, 1), 429/204/201 (few, and those
statuses are already declared without content).

39 route registrations could not be matched to a spec path by the mapping script, so the
handler-level count is a lower bound.

Where it bites

  • Production: nowhere. SHELLHUB_OPENAPI_VALIDATION unset means off outside
    development (pkg/envs/envs.go:115).
  • Development: noise. The mode is report, so every refusal logs
    WARN OpenAPI response validation failed.
  • E2E: a 500. docker-compose.test.yml:14 sets strict, and the middleware answers
    the response does not match the OpenAPI schema with a 500 instead of the real status.

Why the e2e suite never caught it

It asserts no 403 and no 401 anywhere (grep StatusForbidden tests/*.go returns nothing)
and authenticates as the namespace owner, so a refusal is never produced. It also never
calls the two routes whose 200 body diverges from its schema. Strict mode only rejects what
a test provokes.

Proposed fix

Correct the spec, not the validator and not the responses.

The server is right, so its responses stay. Teaching the validator to ignore an empty body
would clear every one of these at once, but it would also stop it catching a response that
genuinely lost its body, which is the reason the check exists.

  1. Drop content from openapi/spec/components/responses/{400,401,402,403,404}.yaml,
    leaving description. OpenAPI cannot say "the body is optional", and an absent content
    validates no body at all, which accepts both shapes. 409 and 406 keep theirs: no
    handler answers them bodyless.
  2. Leave 500.yaml alone. The only bodyless 500 in the workspace is a dead branch in the
    cloud license middleware, tracked in shellhub-io/team#245 and fixed there. 140 path files
    declare a 500 that the error handler does render as JSON, so the component is correct.
  3. Point the 200 of each of the 7 routes above at components/responses/200.yaml, which is
    description only. DELETE /api/namespaces/{tenant}/members is the one that loses real
    documentation: LeaveNamespace (server/api/routes/nsadm.go:224-243) returns userAuth
    or nothing, depending on whether a fresh token was minted.

Guarding against regression

Add a spec-driven sweep to tests/, reading the same bundle the server validates against
(openapi/static/openapi.json, already mounted by docker-compose.test.yml:18). For every
operation, call it with no credential and as an observer, with dummy path parameters, and
assert the response is never the strict-mode marker. A 404 is a fine outcome; the shape is
still validated. dc.R(ctx), dc.JWT() and dc.NewMember() already exist in
tests/environment/docker_compose.go.

This covers error paths, including those of routes added later. It does not cover success
bodies of mutating routes, which need real payloads.

Out of scope

Eight warnings in a development log show response body doesn't match schema on 200 for
GET /api/namespaces/{tenant} and GET /api/service-accounts. That is a different defect,
a real body against its schema, and the sweep above will not reach it. Worth its own issue.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions