Conversation
…cipient as well as the proposal an approval becomes a token, and this module is the whole of that mechanism: no clock, no database, no config lookup. time and the secret arrive as arguments, which is what makes "expired" something the tests assert rather than wait for. - hmac-sha256, not a signature: the same trust boundary mints and verifies (D15), so a symmetric secret is the right tool and the jwt algorithm-confusion family never applies. the moment minting moves to another service that flips - the claims bind amount + recipient_label as well as the proposal hash, jti, approver subject and window. a mac over the hash alone is LIFTABLE: attach it to a bigger amount or another destination and it still checks out - the scheme label is inside the mac, so a token can't be relabelled to a future scheme to have different rules applied - same reason schema_version sits inside the proposal hash (D32). a kid for secret rotation would live there too - verify() order is authenticity -> binding -> window, because every later check reads values from the payload and reading UNAUTHENTICATED values to decide anything is how a verifier gets talked out of its own conclusion. claims are not even parsed until the mac holds - typed failures, never exceptions on untrusted input: garbage is an expected outcome of a verifier, so it returns a Rejected(reason) the caller must branch on. reasons are granular (recipient vs amount vs proposal mismatch) because T16 writes them to the audit trail and a human reads them - hmac.compare_digest, never ==. a weak secret (<32 bytes) RAISES at both ends: that's our own misconfiguration, not untrusted input, and it must be impossible to run with - strict base64: python's b64decode DISCARDS non-alphabet characters by default, so "ab!!cd" and "abcd" decode alike - many token strings sharing one payload, and malformed input reported as a signature failure. found by the malformed- input test - expiry is inclusive (>=): on the money path a boundary rounds toward refusal - EXECUTE_TOKEN_SECRET as SecretStr with a length floor, asserted not to leak through repr/str/model_dump - expiry is NOT single-use. that's T14's compare-and-set on jti; two properties, two mechanisms tamper cases are derived from TokenClaims.model_fields, so a new claim without a tamper test fails the suite instead of shipping an unverified binding. gate green, 209 passed.
…ken, ttl ceiling
the high finding is the kind of thing that only shows up when someone reads the
template as a user: EXECUTE_TOKEN_SECRET=change-me-at-least-32-characters-long is
37 characters, so it passed the length floor and produced a fully WORKING system
- with the hmac key that mints every approval published in a public repo. every
other placeholder here fails on first use because the remote service rejects it;
this one had no remote service to refuse it.
- the template value is now too short to validate AND the word "generate" is on a
reject-list, because a length floor can't catch a placeholder (a memorable
sentence is long enough). the committed value is read by a test, so the two
can't drift
- exactly one spelling per token. validate=True was not enough: b64decode
translates altchars BEFORE its regex, so +/ pass through, and it says nothing
about non-canonical trailing bits - the reviewer produced 4 distinct strings
from one token that all verified. now re-encoded and compared. nothing keys on
the token string today (T14 dedupes on jti), but "the token string is canonical"
is exactly what a later idempotency key or log-based replay detector would
assume
- MAX_TOKEN_TTL (15 min) enforced at mint (our own bug -> raise) and at verify
(authentic-but-absurd window -> WINDOW_TOO_LONG). everything here is justified
by the token being short-lived and nothing enforced it: days=5 where minutes=5
was meant would have verified fine
- the mac frames the scheme label by LENGTH PREFIX, not a "." separator. the old
docstring's argument was false - the canonical payload contains dots (an amount
serializes as "0.25") - and the framing has to carry the guarantee itself once
a kid joins the label
- Binding is validated like the claims it's compared against: a float amount
(0.1 != Decimal("0.1")), an upper-case digest or an unstripped label would each
have produced a plausible *_MISMATCH, recording a CALLER's bug in the audit
trail as "the token was for a different recipient"
- details that reach approvals.reason are sanitized: the unknown-scheme echo was
unbounded attacker text (newlines, ansi escapes, megabytes) into an audit row.
CLAIMS_INVALID now names the offending FIELDS instead of an error count, and
never their values
- claim bounds match their columns (subject 255, recipient_label 64), so a long
value fails at mint rather than at INSERT after the human approved
- amount gt=0 documented as a precondition on T16: an approved hold/alert is
legally zero-amount and has nothing to execute, so the gate must refuse it as a
domain outcome rather than hitting a ValidationError in here
- except ValueError instead of the 3.14-only unparenthesized tuple form
(binascii.Error subclasses ValueError anyway)
gate green, 226 passed.
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.
The approval mechanism itself, and the last M2 task that is pure logic — no database, no clock, no config lookup inside the module. Time and the secret arrive as arguments, which is what makes "expired" something the tests assert rather than wait for.
why HMAC and not a signature
The same trust boundary mints and verifies: the execute domain issues a token to itself, minutes later. A symmetric secret is the correct tool for that shape, and the whole JWT algorithm-confusion family (
alg: none, HS-signed-with-the-public-key) simply doesn't apply. The moment minting moves to a separate service this becomes the wrong choice — that's the upgrade D15 already records.what the token binds, and why
A MAC over the proposal hash alone is liftable: attach it to a bigger amount or a different destination and the digest still checks out. So the claims carry:
proposal_hashamount,recipient_labelsubjectjtiissued_at,expires_atThe scheme label is inside the MAC too, so a token can't be relabelled to a future scheme and have different rules applied to it — the same reason
schema_versionsits inside the proposal hash (D32). Akidfor secret rotation would live alongside it, and would need the same treatment.the ordering in
verifyis the designAuthenticity → binding → window. Every check after the first reads values from the payload, and reading unauthenticated values to decide anything is how a verifier gets talked out of its own conclusion. The claims aren't even parsed until the MAC holds.
hmac.compare_digest, never==— an early-exit comparison leaks the correct MAC byte by byte through timing, and the attacker here gets to retry.Rejected(reason)the caller must branch on rather than an exception the caller might forget to catch. Reasons are granular (recipient_mismatchvsamount_mismatchvsproposal_mismatch) because T16 writes them onto the approval row and a human reads them.now >= expires_at): on the money path a boundary rounds toward refusal, since the cost is a human clicking approve again versus a transfer that shouldn't have happened.one real bug the tests found
Python's
base64.b64decodediscards characters outside the alphabet by default, so"ab!!cd"and"abcd"decode to the same bytes and obvious garbage decodes to something. That meant many distinct token strings shared one payload, and a malformed token was reported as a signature failure — which reads very differently in an audit log. Now decoded strictly (altchars=b"-_",validate=True), so exactly one spelling of a token decodes.expiry is not single-use
Everything here is stateless, so it can only bound how long an approval is good for. "Exactly once" is a different property needing different machinery — T14's atomic compare-and-set on
jti. Two properties, two mechanisms, and neither substitutes for the other (D15).tests
tests/test_gate_token.py— 31 cases. The tamper cases are derived fromTokenClaims.model_fields, so adding a claim without a tamper test fails the suite instead of quietly shipping an unverified binding. Covered: happy path, deterministic minting, per-field payload tampering, authentic-but-wrong-terms (the lifting attack), wrong secret, expired / boundary / not-yet-valid / naive-now, malformed input table, scheme relabelling, authentic-but-nonsensical claims, URL-safety, and the secret not appearing in the token.EXECUTE_TOKEN_SECRETarrives asSecretStrwith a length floor, asserted not to leak throughrepr,str, ormodel_dump.Gate green: ruff, mypy strict, import-linter, 209 passed.
review round (a89182f)
Ten findings, all applied. The gate was green before them — every one is something a test suite and a type checker structurally cannot see.
High — the committed placeholder secret worked.
EXECUTE_TOKEN_SECRET=change-me-at-least-32-characters-longis 37 characters, so it passedField(min_length=32)and produced a fully functional system with the key that mints every approval token published in a public repository. The asymmetry that makes this worth more than a one-line fix: every other placeholder here dies on first use because a remote service rejects it (your-finnhub-keyis refused by Finnhub). A symmetric secret has nothing outside the system to refuse it, so it's the one fake credential that silently succeeds — fail-fast inverted exactly where it matters most. Now the template value is too short to validate and config reject-lists placeholder markers, because a length floor fundamentally can't catch this: a memorable sentence is long enough. A test reads the committed template and asserts it's refused, so the two can't drift.Medium — one authentic token had four valid spellings.
validate=Trueis not canonicality:b64decodetranslatesaltcharsbefore applying its regex (so+//pass through as though urlsafe), and it says nothing about non-canonical trailing bits, so several final characters decode to identical bytes. The reviewer produced four distinct strings from one token that all returnedVerified. Nothing keys on the token string today — T14 dedupes onjti— but "the token string is canonical" is precisely what a later idempotency key, replay cache, or log-based duplicate detector would assume, and it would have been bypassable by changing one character. Now decoded strictly and re-encoded for comparison.Medium — "short-lived" was prose, not code. Three docstrings and the replay-store design justify themselves with it; nothing enforced it.
timedelta(days=5)whereminutes=5was meant would have minted a five-day bearer token that verified perfectly.MAX_TOKEN_TTLis now checked at mint (our own bug → raise) and at verify (WINDOW_TOO_LONG), because an authentic token with an absurd window is what a compromised minter emits — and then it needs to be a property of the gate, not of whoever called mint.The seven lows, each real:
.separator as "cannot occur in either part", but the canonical payload contains dots (an amount serializes as"0.25"). The label is now length-prefixed, so the framing itself carries the guarantee — which is what matters once akidjoins the label.Bindinghad no validation while the claims it's compared against are strict. Afloatamount (0.1 != Decimal("0.1")), an upper-case digest, or an unstripped" cold-wallet "would each produce a plausible*_MISMATCH— recording a caller's bug in the audit trail as "the token was for a different recipient".approvals.reason(aTextcolumn) and log lines: newlines, ANSI escapes, megabytes, from one bogus token. Truncated and stripped to printable ASCII.CLAIMS_INVALIDrecorded an error count ("3"), which tells an operator nothing. It now names the offending fields — and never their values.operators.subject255,tx_attempts.recipient_label64), so an over-long value minted fine and would fail at INSERT after the human approved.amount = Field(gt=0)is a precondition on T16, since an approvedhold/alertproposal is legally zero-amount. Those have nothing to execute, so the gate must refuse them as a domain outcome rather than hitting aValidationErrorin here. Documented at the field.except binascii.Error, ValueError:was redundant (binascii.ErrorsubclassesValueError) and PEP 758 syntax that only parses on 3.14 — a gratuitous floor on the module a third-party tool is most likely to want to read.Gate green: 226 passed.