Skip to content

fix: reject implicit wallet fallback and foreign account polling - #30

Merged
VickyXAI merged 2 commits into
BlockRunAI:mainfrom
KillerQueen-Z:fix/user-switching-audit
Sep 5, 2026
Merged

fix: reject implicit wallet fallback and foreign account polling#30
VickyXAI merged 2 commits into
BlockRunAI:mainfrom
KillerQueen-Z:fix/user-switching-audit

Conversation

@KillerQueen-Z

Copy link
Copy Markdown
Contributor

An empty or malformed BLOCKRUN_API_KEY could silently select an existing wallet. API media jobs also accepted a foreign poll URL and attached the account credential to it. This follow-up to #29 returns a configuration error before wallet setup, preserves explicit wallet selection, and validates account polling origins before dispatch.

README now explains account registration, top-ups, activity, key rotation and returning to wallet mode.

Validation: full local go test -race ./... and go vet ./... pass. Regression tests reproduce invalid-key fallback and a foreign image polling server, and verify that account and wallet clients can share an HTTP client without sharing credentials. No live wallet payments or production ledger reconciliation were performed.

Based on current main; the older #28 remains open separately. Invalid configured keys must now be corrected or unset. An explicit wallet credential still takes precedence.

@VickyXAI VickyXAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both defects are real and both are mine — thank you for catching them. The origin guard especially: the TS SDK already had it (ApiKeyAuth.resolveUrl, "Refusing to send a BlockRun API key to a different origin") and I did not carry it into Go, so an absolute poll_url got the Bearer token attached by applyAuth. Tightening /api stripping to /api/ is right too — my TrimPrefix(u, "/api") would have mangled /apiv2/....

Verified locally on this branch: go build, go vet, and go test -race ./... all pass.

One regression to fix before merging: a blank BLOCKRUN_API_KEY now refuses.

raw, configured := os.LookupEnv(EnvAPIKey)
if !configured { return "", nil }
env := strings.TrimSpace(raw)
if !IsAPIKey(env) { return "", &ValidationError{...} }

BLOCKRUN_API_KEY="" is set but empty, so configured is true, env is "", and a wallet user who never opted into the account rail gets a hard error. Reproduced against this branch:

--- FAIL: TestBlankEnvKeyIsUnsetNotInvalid
    blank BLOCKRUN_API_KEY refused a wallet user: Validation error for
    BLOCKRUN_API_KEY: Invalid configured API key. Correct or unset it, or
    explicitly pass a wallet key.

That shape is common and none of it means "I am on the account rail": a bare docker -e BLOCKRUN_API_KEY, an unpopulated ${{ secrets.X }} in Actions, BLOCKRUN_API_KEY= in a .env. It would break CI for wallet users on upgrade.

The Python fix for this same bug (blockrun-llm#60, merged) draws the line at blank rather than at set-ness, and I think that is the right one:

# Blank is unset, not invalid. `BLOCKRUN_API_KEY=` in a .env file, a bare
# `docker -e BLOCKRUN_API_KEY`, and an unpopulated `${{ secrets.X }}` all
# land here as the empty string, and every one of them means "I am not on
# the account rail" — raising would break wallet users who never opted in.
# A non-blank value that is not a key is a different thing: someone typed a
# credential and got it wrong, and silently spending USDC instead of credit
# is the wrong way to tell them.

So: os.Getenv + strings.TrimSpace, return "", nil when empty, and error only on a non-blank non-key. The malformed case already behaves correctly on this branch — I checked that separately and it refuses, which is the half that matters for money.

Worth a test for each, since they are the two halves of one decision:

func TestBlankEnvKeyIsUnsetNotInvalid(t *testing.T) {
	t.Setenv(EnvAPIKey, "")
	t.Setenv("BLOCKRUN_WALLET_KEY", testPrivateKey)
	c, err := NewLLMClient("")
	if err != nil {
		t.Fatalf("blank BLOCKRUN_API_KEY refused a wallet user: %v", err)
	}
	if c.PaymentMode() != PaymentModeWallet {
		t.Errorf("PaymentMode = %q, want wallet", c.PaymentMode())
	}
}

With that changed I think this should land. Note it needs a release to reach anyone — v0.21.0 is tagged and the module proxy serves tags, so a merge alone leaves go get on the version with both defects.

… rails

Review of this branch. The two defects it fixes are real and the origin
validation is careful — protocol-relative, embedded credentials, case variance
and explicit :443 all classify correctly. Five things on top.

Blank BLOCKRUN_API_KEY was a hard failure. Keying on os.LookupEnv made a
variable that is SET but empty count as configured, so `docker -e
BLOCKRUN_API_KEY`, an unpopulated `${{ secrets.X }}`, and a bare
`BLOCKRUN_API_KEY=` line each broke a wallet user's CI on upgrade. Blank now
means unset; a non-blank non-key still errors, which is the half that stops a
typo from spending USDC instead of credit. This narrows
TestInvalidAPIKeyEnvDoesNotSelectWallet, which asserted the old behaviour for
"" and "   " — that assertion was the regression, so it had to move.

The origin guard now covers the wallet rail too. It protected the API key and
not PAYMENT-SIGNATURE, but image.go and video.go set that header on every poll,
so a gateway answering with poll_url "https://attacker.example/job" collected a
signed payment authorization. It can only ever pay BlockRun's treasury, so the
harm is disclosure plus griefing — burn the nonce and the caller's real
settlement fails as a replay — rather than theft. Guarding one credential and
not the other inside one function was the asymmetry worth closing. Found by the
Codex adversarial pass, verified by reading the two call sites.

A bare "brk_" is a truncated secret, not a key. It passed the prefix check, so
it selected the account rail and 401'd at request time — the opposite of this
branch's own promise that a bad credential fails where you set it.

An absolute same-origin poll_url kept the gateway's /api mount, so
"https://api.blockrun.ai/api/v1/..." would have reached the account rail as
/api/v1 and answered wrong_host: the exact failure resolvePollURL exists to
prevent, for the one URL shape it did not cover. Query and fragment are carried
through, which the video poll_url needs — it signs the billable duration into
the query string.

README: the new section landed after "## License", so it rendered as a
subsection of License at the end of the file. Moved into API Keys & Accounts
with the rest of the credential documentation.

go build, go vet and go test -race all pass; verified live on both rails.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T5RKUETYjxwNRqkURLnatJ
@VickyXAI

VickyXAI commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Reviewed and pushed five fixes to this branch (beb1620). Both defects you found are real and the origin validation is careful — I probed it and protocol-relative //evil/x, embedded user:pw@, case variance and explicit :443 all classify correctly.

One of these changes an assertion you wrote, so flagging it first. TestInvalidAPIKeyEnvDoesNotSelectWallet asserted that "" and " " error. That assertion encodes a regression:

""      -> ERROR: Invalid configured API key...
"   "   -> ERROR: Invalid configured API key...

os.LookupEnv treats set-but-empty as configured, and set-but-empty is exactly what docker -e BLOCKRUN_API_KEY, an unpopulated ${{ secrets.X }}, and a bare BLOCKRUN_API_KEY= line produce. None of them mean "put me on the account rail", so each one broke a wallet user's CI on upgrade. Blank now means unset; a non-blank non-key still errors, which is the half that stops a typo from spending USDC instead of credit. The merged Python fix (blockrun-llm#60) draws the line the same way. I narrowed your test to the non-blank values and added TestBlankEnvKeyIsUnsetNotInvalid alongside it.

The origin guard now covers the wallet rail too. This is the one I'd have missed without an adversarial pass. It protected the API key and not PAYMENT-SIGNATURE — but image.go:447 and video.go:515 set that header on every poll:

pollReq.Header.Set("PAYMENT-SIGNATURE", pollSig)

and the wallet branch returned any absolute URL unchanged. A gateway answering poll_url: "https://attacker.example/job" collects a signed payment authorization. It can only ever pay BlockRun's own treasury, so the harm is disclosure plus griefing (burn the nonce and the caller's real settlement fails as a replay) rather than theft — but guarding one credential and not the other inside one function was the asymmetry worth closing. Pre-existing, not introduced by you; this was just the right PR for it.

Three smaller ones:

  • A bare brk_ passed IsAPIKey, so a truncated secret selected the account rail and 401'd at request time — the opposite of this branch's own promise that a bad credential fails where you set it.
  • An absolute same-origin poll_url kept the /api mount: https://api.blockrun.ai/api/v1/… would reach the account rail as /api/v1 and answer wrong_host. That is the exact failure resolvePollURL exists to prevent, for the one URL shape it did not cover. Query and fragment are carried through now, which the video poll needs — it signs the billable duration into the query string.
  • The README section landed after ## License, so it rendered as a subsection of License at the end of the file. Moved into API Keys & Accounts with the rest of the credential docs.

go build, go vet, go test -race all pass, and I ran both rails live against the gateway after the changes.

Before merging, one thing that is not in the diff: v0.21.0 is already tagged, and the Go module proxy serves tags — so merging alone leaves go get on the version with both original defects. This needs a v0.21.1 (VERSION + CHANGELOG commit on main, then the annotated tag) to reach anyone.

@VickyXAI
VickyXAI merged commit c7a785e into BlockRunAI:main Sep 5, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants