Skip to content

ci: check api routes against openapi spec and harden workflows - #5675

Open
martinconic wants to merge 1 commit into
masterfrom
ci/openapi-route-check
Open

martinconic wants to merge 1 commit into
masterfrom
ci/openapi-route-check

Conversation

@martinconic

Copy link
Copy Markdown
Contributor

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have added or updated fuzz targets for any code handling untrusted input.
  • I have filled out the description and linked the related issues.

Description

Follow-up to #5673. The checks added there only look at what the spec already documents, so a new route added to the router without a spec entry would go unnoticed. This PR closes that gap, and also cleans up two existing security warnings in our workflows.

Route check (pkg/api/router_spec_test.go)

A unit test walks the real API router and compares it with openapi/Swarm.yaml in both directions:

  • every route served by the API is documented in the spec,
  • every path in the spec is served by a route,
  • each route serves exactly the HTTP methods the spec documents.

Most routes keep their methods inside jsonhttp.MethodHandler, which mux can't see. The test sends each route a request with an unknown method and reads the Allow header of the 405 response, so the router needs no changes. The only additions outside the test are a Router() accessor and a RootPath constant in export_test.go.

Some routes are intentionally not in the spec and are skipped: /, /robots.txt, /metrics and /debug/* (pprof, expvar). The /v1 prefixed copies and the *.swarm.localhost subdomain routes are skipped too, since they serve the same handlers.

It runs with the normal unit tests, takes milliseconds, and fails with messages like:

route /peers is not documented in ../../openapi/Swarm.yaml
path /peerz is documented in ../../openapi/Swarm.yaml, but no route serves it
route /peers serves methods [GET], but the spec documents [DELETE GET]

Spec fix found by the check

HEAD /bzz/{address}/{path} is served but was not documented. The spec only had HEAD /bzz/{address}, which is really a redirect to /bzz/{address}/. I added the HEAD operation to /bzz/{address}/{path}, using the same headers and responses as the existing one.

Workflow hardening

  • beekeeper.yml: the two debug steps used ${{ github.head_ref }} directly inside the shell script. Branch names come from the PR author, so this is a script injection risk (flagged by actionlint). They now go through environment variables, together with the PR author.
  • pr-title.yml: commitlint is now installed with --ignore-scripts and run from node_modules/.bin instead of npx (flagged by SonarCloud). I checked that it still accepts valid titles and rejects invalid ones.

actionlint now reports no issues for any workflow in the repo.

Open API Spec Version Changes (if applicable)

No version bump here. Documenting an existing endpoint is a patch-level change, and #5674 already bumps the version to 8.2.2. Merging both in the same release covers it.

Motivation and Context (Optional)

Part of #4974: keeping the hand-written spec complete and accurate, so bee-js and other clients can be generated from it.

Related Issue (Optional)

#4974, follow-up to #5673

AI Disclosure

  • This PR contains code that has been generated by an LLM.
  • I have reviewed the AI generated code thoroughly.
  • I possess the technical expertise to responsibly review the code generated in this PR.

Add a test that walks the API router and compares every route and its
HTTP methods with openapi/Swarm.yaml, in both directions. Methods are
read from the Allow header of the method not allowed response, so the
check needs no changes to the router. Metrics, profiling and the index
pages are intentionally left out of the spec.

The check found one gap: HEAD on /bzz/{address}/{path} was served but
not documented. Document it.

Pass the branch name and PR author to the beekeeper debug steps through
environment variables instead of inline expressions, and install
commitlint in the PR title workflow without running install scripts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant