fix(scripts): make the robinhood credential guard catch the error it claimed to - #416
Merged
Merged
Conversation
…claimed to `_SEED_B64_LEN`'s comment said checking the private key's length "turns the most likely operator error -- pasting the public key -- into a precise message instead of a 401." It cannot, and never could: a raw Ed25519 seed and a raw Ed25519 public key are both 32 bytes, so both are 44 base64 characters, drawn from the same alphabet. Nothing about length, alphabet or decoded size separates them. On 2026-08-19 an operator pasted the public key into ROBINHOOD_API_KEY. Every guard passed. Every request signed correctly and came back 401 -- indistinguishable from a revoked key, a stale clock or a signing bug -- and telling those apart cost an afternoon. The only thing that actually distinguishes the two values is that one is derived from the other, so the guard now derives it. Two checks, ordered most-specific first: * ROBINHOOD_API_KEY equals the public key of ROBINHOOD_PRIVATE_KEY. Named outright, with the fix, because this one needs no round trip to diagnose: the value sitting in the wrong variable is exactly what Robinhood's credential page wants pasted into it. * ROBINHOOD_API_KEY is any 32-byte base64 value. An Ed25519 key is never an API key identifier whatever the identifier format turns out to be, so this gates even when there is nothing to match it against. `validate=True` on the decode is load-bearing, not defensive: without it `b64decode` DISCARDS out-of-alphabet characters rather than refusing, so a genuine `rh-api-<uuid>` -- which contains `-` -- could decode to 32 bytes by accident and be rejected as a pasted key. Pinned by test. The observed `rh-api-<uuid>` shape is quoted in the messages as guidance and deliberately NOT enforced: it is an observation about one credential, not a documented contract, and a hard gate would reject a valid key the day Robinhood changes the format. The length check stays -- it still catches a PEM, a hex string or a truncated paste -- with its overreaching claim removed and replaced by a note saying which case it does not cover. `_public_key_b64` returns None rather than raising on a malformed seed or a missing pynacl: a diagnostic that crashes on the malformed input it exists to describe is worse than one that stays quiet, and the length check above already covers the malformed-seed path. Verified against the real .env that produced the incident: the guard now names the variable, the mistake and the fix. Also corrects the test suite's `_VALID_SEED_B64`, which was `"A" * 44` -- not a valid base64 32-byte seed at all (44 unpadded characters decode to 33 bytes), which a constant of that name should not be. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eaitbrahim
added a commit
that referenced
this pull request
Aug 20, 2026
Minor, not patch. Three things since v0.9.3 change what an implementor or a deployment can rely on: * A NEW DISTRIBUTION. `keel-broker-alpaca` (#382, #384) plus the paper-equities profile that selects it (#386), so a deployment can now be US equities via the broker port rather than crypto only. * THE PORT CONTRACT MOVED TWICE. `market_clock`/`market_schedule` made venues session-aware (#385), and `place_order` gained `idempotency_key` (#419). Both carry defaults so no CALLER breaks, but a third-party adapter that does not accept them is no longer a `Broker` -- the conformance suite now says so. That is exactly the kind of change a patch bump must not hide. * THE OPERATOR CONSOLE. The TUI became keel's console across #399-#408, and `keel update` (#415/#417) makes a deployment self-updating. Every pinned sibling moves with it. The four production distributions are required `==` at this exact version (`RELEASING.md`, "Release assets"), so a bump that missed one would install a mixed set -- the `keel-trader 0.5.7` against `keel-core 0.5.5` failure `keel versions` exists to catch, and which `~/keel` actually ran across two releases. Also in this window, on the Robinhood adapter: the best_bid_ask fixture corrected against the live venue (#414), a credential guard that catches the error it only claimed to (#416), pre-flight sizing reported on the preview (#418), transport backoff (#420), and the fenced one-order probe (#421). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 guard didn't do what its comment said
scripts/robinhood_smoke.py:It cannot. A raw Ed25519 public key is also 32 bytes, so it is also 44 base64 characters, from the same alphabet. Length, alphabet and decoded size are all identical. The check never covered the case it named.
On 2026-08-19 an operator pasted the public key into
ROBINHOOD_API_KEY. Every guard passed, every request signed correctly, and the venue returned 401 — indistinguishable from a revoked key, a stale clock, or a signing bug. Telling those apart cost an afternoon.What actually separates the two values
One is derived from the other. So the guard derives it:
That is the verbatim output of the new guard run against the real
.envthat produced the incident.Two checks, most-specific first:
ROBINHOOD_API_KEYequals the public key ofROBINHOOD_PRIVATE_KEY— named outright, with the fix, because this one needs no round trip to diagnose: the value in the wrong variable is exactly what the credential page wants.ROBINHOOD_API_KEYis any 32-byte base64 value — an Ed25519 key is never an API key identifier whatever the identifier format turns out to be, so this gates even with nothing to match against.Two details that are load-bearing
validate=Trueon the decode. Without itb64decodediscards out-of-alphabet characters rather than refusing, so a genuinerh-api-<uuid>— which contains-— could decode to 32 bytes by accident and be rejected as a pasted key. Pinned bytest_a_real_api_key_is_not_mistaken_for_base64.The
rh-api-<uuid>shape is quoted, not enforced. It is an observation about one credential, not a documented contract; a hard gate would reject a valid key the day Robinhood changes the format. The two checks that do gate are the ones that cannot be wrong._public_key_b64returnsNonerather than raising on a malformed seed or a missing pynacl — a diagnostic that crashes on the malformed input it exists to describe is worse than one that stays quiet, and the length check above already covers that path.The length check stays (it still catches a PEM, a hex string, a truncated paste) with its overreaching claim removed and a note saying which case it does not cover.
Also
_VALID_SEED_B64in the test suite was"A" * 44— not a valid base64 32-byte seed at all. 44 unpadded base64 characters decode to 33 bytes, so a constant of that name was never one. Nowbase64.b64encode(bytes(range(32))), which the new derivation check requires anyway.Full suite: 3656 passed, 3 skipped.
ruff checkandmypyclean.🤖 Generated with Claude Code