fix: verify state parameter in loopback callback - #328
Conversation
❌ Deploy Preview for stellar-signet failed.
|
|
@Hussman256 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
@Anambraboi-1 is attempting to deploy a commit to the blockchainmaxis-8449's projects Team on Vercel. A member of the Team first needs to authorize it. |
blockchain-maxis
left a comment
There was a problem hiding this comment.
The security fix here is correct and worth keeping: 32 bytes from randomBytes is ample entropy, the check runs before token/handle are read, and a mismatch returns 400 without resolving the promise — so a forged callback can't complete the exchange or unblock the wait. That's #256 answered.
Two things stop me merging it as-is.
It is stacked on #320, which I've asked for changes on. #320 puts the CLI in apps/cli as a TypeScript package; #251 specifies a Go module at cli/, and the rest of the series is already building there. This PR inherits that, so it moves with #320 rather than separately.
The Netlify checks are all red on this branch — the deploy preview isn't building. Same symptom as #320, so most likely the same cause in the workspace change.
Two notes for when this lands in cli/, both worth carrying over:
callbackState !== expectedState is a variable-time comparison. For a 32-byte random state this is not practically attackable, but the callback is the one place an attacker controls the input, and Go's crypto/subtle.ConstantTimeCompare costs nothing to reach for.
More substantially: the state check protects the callback, but the loopback server still accepts a connection from any local process. #348 covers the Private Network Access preflight, and it's worth checking Origin on the callback request too, so a page in the developer's browser can't drive the flow even with a guessed state.
The createLoopbackServer(expectedState) shape — server creation and the state binding in one call, so you cannot construct one without a state — is a good design and I'd keep it in the port. #370 is the open PR for the loopback server in cli/; this logic belongs there.
Fixes #256. Adds a cryptographic state parameter to the loopback callback flow to prevent CSRF. Depends on #258.