Repository navigation
item 7: the webhook approval provider - #13
Merged
Merged
Conversation
SPEC-v0.2 §7, acceptance tests T27 and T28. Outbound is core because it needs neither an HTTP server nor an SDK: one signed POST over stdlib urllib.request. The inbound endpoint is the gateway's, because that is the one server this release ships. The signature is over the exact bytes sent. Two encoders that agree on meaning and differ on whitespace would produce a MAC the receiver cannot verify, and a receiver that re-serialized the payload to check would be verifying its own guess rather than what arrived. Every inbound condition is checked before anything is written, and the refusal message has the same shape for all of them: telling a caller which condition it missed is telling it what to try next. A replayed answer is idempotent only where it asks for the answer the record already carries — a grant replayed onto a denied or consumed record is a different question, and is refused. Single use stays where v0.1 §4.2 A2 put it, at consumption. An undelivered notification is not an approval. If every attempt fails the request stays pending, ApprovalRequired is raised with a request_id that `ctrlrun approve` can still answer, and the failure is logged without the secret or the payload — the payload carries the arguments, which are the customer identifiers and amounts. Redirects are never followed: one to another host would deliver the signed payload, and the action inside it, somewhere the operator did not name. An http:// url is refused unless it is loopback and meant. urllib.request is imported inside the methods that use it, not at module scope. It drags in `ssl`, and §1.1's rule — a core install pays only for what it uses — reads the same against the stdlib as against the extras. The examples' no-network harness caught this: it had replaced socket.socket with a function, which `ssl` cannot subclass. The harness now replaces the type with one that refuses to connect, which is what a cut cable actually looks like.
…branch Mutation-testing §7 found more than the usual crop. Two were untested rather than subsumed: no test sent a request id the store had never seen, and none sent a decision that was neither grant nor deny. Both now exist. The path/body request-id comparison had a test that passed for the wrong reason: with only the path wrong, the lookup finds nothing and the refusal is right by accident. Parametrized over both directions now — with only the body wrong, the lookup finds the *real* record and the comparison is the only thing between it and a grant. `if response.status >= 400` was dead: urlopen raises HTTPError for anything at or above 400, and HTTPError is a URLError, which _deliver already treats as a failed delivery. Removed. The redirect test was the interesting one. It used a 307 — the one code urllib refuses on a POST by itself — so the no-redirect handler was never exercised. urllib follows 301, 302 and 303, converting the POST to a GET, and the test's second server only recorded POSTs, so even a followed redirect was invisible to it. Parametrized over the three codes urllib actually follows, and the second server records GETs. The empty-secret check is subsumed by the length check and stays for its message: "empty" points at the configuration, "too short" at the value. The test asserts which one it got.
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.
Build-list item 7. SPEC-v0.2 §7, acceptance tests T27 and T28.
Outbound is core, not an extra: it needs neither an HTTP server nor an SDK, just one signed POST over stdlib
urllib.request. The inbound endpoint is the gateway's, because that is the one server this release ships.The signature is over the exact bytes sent
CTRLRun-Signature: t=<unix seconds>,v1=<hex>, HMAC-SHA256 overf"{t}.{body}". Two encoders that agree on meaning and differ on whitespace would produce a MAC the receiver cannot verify, and a receiver that re-serialized the payload to check would be verifying its own guess rather than what arrived.Inbound, every condition is checked before anything is written, and the refusal message has the same shape for all of them: telling a caller which condition it missed is telling it what to try next. A replayed answer is idempotent only where it asks for the answer the record already carries — a grant replayed onto a denied or consumed record is a different question, and is refused. Single use stays where v0.1 §4.2 A2 put it, at consumption.
An undelivered notification is not an approval. Every attempt failing leaves the request
pending,ApprovalRequiredraised with arequest_idthatctrlrun approvecan still answer, and a warning logged without the secret or the payload — the payload carries the arguments, which are the customer identifiers and amounts.One thing that changed outside §7
urllib.requestis now imported inside the methods that use it. It drags inssl, and §1.1's rule — a core install pays only for what it uses — reads the same against the stdlib as against the extras.The examples' no-network harness caught this, which is the nice part: it had replaced
socket.socketwith a function, andssldoesclass SSLSocket(socket). The harness now replaces the type with one that refuses to connect, which is what a cut cable actually looks like.tests/test_packaging.pypinssslout ofimport ctrlrun.Mutation table — 20 mutations, all red
compare_digest, the replay windowHTTPErroris a failurerespond_toEight started green. Two were harness faults of familiar kinds. The rest were real, and two are worth your time:
if response.status >= 400was dead code.urlopenraisesHTTPErrorfor anything at or above 400, andHTTPErroris aURLError, which_deliveralready treats as a failed delivery. The branch could never run.The redirect test used a 307 — the one code
urllibrefuses on a POST by itself. So the no-redirect handler, which is the whole defence against delivering a signed payload somewhere the operator did not name, was never exercised.urllibfollows 301, 302 and 303, converting the POST to a GET; and the test's second server only recorded POSTs, so even a followed redirect was invisible to the assertion. Both fixed: parametrized over the three codesurllibactually follows, and the second server records GETs.That second one is a shape worth naming alongside the subsumed-guard pattern: a negative test proves nothing unless the thing it forbids would otherwise happen. I asserted "the redirect was not followed" against a case where nothing would have followed it anyway, and against a recorder that could not have seen it if it had.
Also fixed: the path/body id test passed for the wrong reason (with only the path wrong, the lookup finds nothing), and neither an unknown request id nor a nonsense
decisionhad any test.Verification
1207 tests pass (1162 → 1207),
mypy --strict src/,ruff checkandruff format --checkclean.import ctrlruntouches neitherssl,httpxnorctrlrun.gateway.Remaining: item 8 (OTel sink + ACS note, T29 and T30's named test), then item 9 releases.