Repository navigation
fix(api): answer 410 on every retired Companion route - #665
Merged
Merged
Conversation
The handler behind the eleven retired Companion routes took `**_: str`, which FastAPI reads as a required query parameter named `_`. A call without `?_=` got a 422 before the handler ran, so only the `/companion/...` prefix gave the 410 Gone that docs/SECURITY.md promises. The handler now takes no parameters. A test calls every method each retired route is registered for, with and without a query string and body, and checks the 410, its body and that no cookie is set. Fixes: #664
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Description
Eleven retired Companion routes answered 422 instead of the 410 Gone that
docs/SECURITY.mdpromises. Their shared handler took**_: str, which FastAPI reads as a required query parameter named_, so only a call with?_=reached it. The handler now takes no parameters, since it reads nothing.A new test calls every method each retired route is registered for, the
/companion/{legacy_path:path}prefix included, once bare and once with a query string and a JSON body. It checks the 410, the documented body and that no cookie is set. A second test checks that its table matches the routes onlegacy_companion_router, so a route added or dropped there cannot go untested. A third checks that the retired paths stay out of the OpenAPI schema and that no_parameter appears in it. On main, 14 of the 20 route and method cases fail with the 422; the six prefix cases pass.Decision record
request_id,recording_id) still match without being declared, and the handler never read them.include_in_schema=False, so the_parameter never reached the published OpenAPI schema. I kept them hidden, since a retired endpoint should not be advertised, and the new test pins that.system.pykeeps its own copy of the 410 body for/system/companion-releases. Importing it fromapi.pywould be circular, and both copies are checked against the same expected body in the tests.Fixes: #664
Type of change
Checks run
scripts/check.py lint format blindexcept heldpins typecheck docs alembicand the full pytest run (3633 passed, with Postgres) innojoin-dev-tests:local;check_whitespace.py,check_file_size.py,validate_docs.pyandgit diff --checkon the hostcd frontend && npm run lint && npm run check:contrast && npm run test && npm run buildcd site && npm ci && npm run buildpython3 scripts/validate_docs.pyMigration impact
python3 scripts/check_migrations.pyrun against a throwaway Postgres.Documentation impact
docs/SECURITY.mdalready says these endpoints return 410; they now do..github/CONTRIBUTING.md, "Documentation Ownership").Security impact
docs/SECURITY.mdboundaries preserved and updated where behaviour changed.The retired routes issued no credentials before or after; only the status code changes from 422 to 410.
Manual verification
Against the app in the test container:
GET /api/v1/companion/and/api/v1/companion/a/b/c?x=1answer 410;GETon the POST-only/login/companion-pairing/disconnectanswers 405, as before; a CORS preflight from a disallowed origin is refused by the CORS middleware, as before.Screenshots (if relevant)
None.