Show operators how to use an API key, and let them pause one (#142) - #143
Merged
Conversation
A key an operator cannot work out how to send is a key that does nothing. The docs page lists every route and never says which header carries the credential, so `src/shared/apiUsage.ts` now holds curl, JavaScript and Python samples built against this install's own base URL - one pure source, so the desktop app, the web panel and the docs cannot disagree. `usageSamples` is deliberately self-contained. The panel serves it via `.toString()`, and a reference to a module-level binding becomes a ReferenceError in the page the moment the bundler renames it - a trap this codebase has fallen into before. The smoke now re-parses the function with no scope around it and calls it, which is the only check that turns that into a failure here; reading the source for the identifier would not, because the bundler renames it. Disabling is new and is not revoking. Revoking is permanent and is what you do to a credential you believe has leaked; disabling is what you do to an integration you are pausing, and it can be undone. Enabling a revoked key is refused (409) rather than quietly resurrecting it. The switch is honoured in `isKeyUsable`, the single answer to "may this key be used" - a switch obeyed in some places and not others is worse than no switch at all. Verified: 12/12 gates. Both new checks proved failable by reintroducing the bug - ignoring `disabled` gives "a disabled key still worked: 200", and pointing a sample back at `API_PREFIX` gives the real "ReferenceError: API_PREFIX is not defined" from the detached call.
…#142) Two gaps in the first commit. The new route had no HTTP coverage at all - the smoke called `setKeyDisabled` in-process and never issued a request, so "owner only" and "the route works" were both claims rather than tests. Adding the request found the second, larger problem. All three key-mutation routes documented their body field as `id`. The server has always read `keyId`. An integrator following the reference exactly got 404 from revoke, 404 from disable, and 200 from a delete that deleted nothing - the precise failure this issue is about, sitting inside the document meant to prevent it. So the test now takes the field name *from the route table* instead of spelling it itself. A test that hardcoded `keyId` would have passed happily beside a document that said `id`; this one cannot. Restoring `id` in the table turns the gate red with "owner disable over HTTP: 404 {"error":"key-not-found"}", which is exactly what the operator saw. Also asserts what was only asserted in a comment: an API key, even one holding every scope, gets 403 from the disable route. A key that can switch keys off is the same escalation that keeps key minting off the API. Verified: 12/12 gates, on a fresh build.
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.
Closes #142.
The samples
src/shared/apiUsage.tsholds curl, JavaScript and Python samples built against this install's own base URL. One pure source, so the desktop app, the web panel and the docs page cannot drift apart. They carryPASTE_YOUR_KEY_HERErather than a plausible-looking fake, because the real secret is unrecoverable the moment the creation dialog closes.usageSamplesis deliberately self-contained — it spells/api/v1andX-API-Keyout instead of naming the shared constants. The panel serves it through.toString(), and a reference to a module-level binding becomes aReferenceErrorin the page as soon as the bundler renames it. This codebase has walked into that trap before.The smoke re-parses the function with no scope around it and calls it. Reading the source for a forbidden identifier would not work — the bundler renames it, so the dead reference is no longer called
API_PREFIXby the time it reaches the page. Only running it detached catches this.Disable is not revoke
Revoking is permanent, and is what you do to a credential you think has leaked. Disabling is what you do to an integration you are pausing. Collapsing them into one button loses that distinction, so they are two buttons. Enabling a revoked key answers 409 rather than quietly resurrecting it.
The switch is honoured inside
isKeyUsable, the single answer to "may this key be used". A switch obeyed in some places and not others is worse than no switch.What the self-review found
The new route had no HTTP coverage — the smoke called
setKeyDisabledin-process, so "the route works" and "owner only" were claims, not tests. Adding the request surfaced the real defect:All three key-mutation routes documented their body field as
id. The server has always readkeyId. Anyone following the reference exactly got 404 from revoke, 404 from disable, and 200 from a delete that deleted nothing — the exact failure this issue is about, living inside the document meant to prevent it.The test now takes the field name from the route table rather than spelling it itself, so a document that lies cannot sit beside a test that passes.
Verification
12/12 gates on a fresh build. Three checks proved failable by reintroducing the bug:
isKeyUsableignoresdisableda disabled key still worked: 200API_PREFIXReferenceError: API_PREFIX is not definedidagainowner disable over HTTP: 404 {"error":"key-not-found"}