fix(security): close bridge auth bypass, verify real SEP-10 signers, reserve RBAC keys - #223
Merged
Merged
Conversation
…reserve RBAC keys
Three critical findings from an org-wide source review.
1. The shipped bridge ran the WebSocket feed with all authentication disabled.
main.ts constructed `new ZkStateSyncer(propagator, { port })` — no `auth`,
no `serverSigningKey`. Both are optional, so `verifyClient` was spread in
conditionally and omitted entirely (any connection accepted on :8080), and
the broadcast guard `if (serverSigningKey && !authenticatedAs) continue`
short-circuited, sending every ZK state commitment to every socket. The whole
RelayerAuth/SEP-10 layer was unreachable in the only production entrypoint.
main.ts now wires auth from the environment, and ZkStateSyncer fails closed:
it throws unless `auth`, `serverSigningKey`, or an explicit
`allowUnauthenticated: true` is supplied, warning loudly in the last case.
2. SEP-10 verification ignored the client account's real signer configuration.
verifyChallengeTxThreshold was called with a fabricated set —
`[{ key: clientAccountID, weight: 1 }]`, threshold 1 — where the account ID
came from the submitted XDR itself. The account was never fetched, so a
master key revoked to weight 0 still authenticated, and any single signer
satisfied a multi-sig account regardless of its med_threshold. These are the
treasury and governance accounts, i.e. the ones expected to be multi-sig.
verifyResponse is now async and takes an AccountLoader, using the account's
real signers and med_threshold. It fails closed if the account can't be
loaded rather than falling back to the master-key assumption.
3. RESERVED_KEYS omitted the RBAC slots owned by core::access, so update_param
could overwrite C_ADMIN (an Address) with a u64. Every later require_role
then fails host conversion, and access::initialize refuses to re-run because
C_INIT is still set — no recovery path. Added C_INIT, C_ADMIN, C_OPERS,
C_AUDIT and the proxy GAP slot.
Tests: engine-bridge 93 passed; engine-core 95 passed. Adds regression tests for
each — the startup guard, revoked-weight and below-threshold multi-sig
rejection, loader failure, and reserved RBAC keys.
N-i-xx
approved these changes
Aug 24, 2026
N-i-xx
left a comment
There was a problem hiding this comment.
Reviewed: bridge auth wiring, SEP-10 signer verification, and RBAC key reservation. All checks green.
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.
Summary
Three critical findings from an org-wide source review. All verified by test.
1. The shipped bridge ran the WebSocket feed with authentication disabled
engine-bridge/src/main.tsconstructednew ZkStateSyncer(propagator, { port })— noauth, noserverSigningKey. Both are optional, so:verifyClientis spread in conditionally (...(options.auth && { verifyClient })) and was omitted entirely — the server accepted any connection on port 8080 andRelayerAuthnever ran.if (this.options.serverSigningKey && !state.authenticatedAs) continueshort-circuited on the missing key, so every connected socket received all ZK state commitments.The whole RelayerAuth/SEP-10 layer was unreachable in the only production entrypoint. Worth noting this also means #202 (timing-safe comparison inside
RelayerAuth) describes code that never executed.Fix:
main.tswires auth from the environment, andZkStateSyncernow fails closed — it throws unlessauth,serverSigningKey, or an explicitallowUnauthenticated: trueis supplied, warning loudly in that last case.2. SEP-10 verification ignored the account's real signer configuration
wallet-connector.tscalledverifyChallengeTxThresholdwith a fabricated signer set —[{ key: clientAccountID, weight: 1 }]andthreshold: 1— whereclientAccountIDwas read out of the submitted XDR itself. The account was never fetched.So a master key revoked to weight 0 (the standard move after a key compromise) still authenticated, and any single signer satisfied a multi-sig account regardless of its real
med_threshold. These are the treasury and governance accounts — precisely the ones expected to be multi-sig.Fix:
verifyResponseis now async and takes anAccountLoader, using the account's actual signers andmed_threshold. It fails closed when the account can't be loaded rather than falling back to the master-key assumption.horizonAccountLoader()provides the real implementation; the interface keeps it mockable.3.
update_paramcould permanently brick RBACRESERVED_KEYSomitted the storage slots owned bycore::access. WritingC_ADMINreplaces anAddresswith au64; every laterrequire_rolethen fails host conversion, andaccess::initializerefuses to re-run becauseC_INITis still set — no recovery path. AddedC_INIT,C_ADMIN,C_OPERS,C_AUDIT, and the proxyGAPslot.Tests
engine-bridge: 93 passed. engine-core: 95 passed. New regression tests cover the startup guard, revoked-weight rejection, below-threshold multi-sig rejection, loader failure, and each reserved RBAC key.
Two existing tests asserted the old insecure defaults and were updated to opt in explicitly — worth a look during review, since that's a behaviour change contributors may hit.
New environment variables
RELAYER_API_KEYS,RELAYER_JWT_SECRET,SERVER_SIGNING_KEY,HORIZON_URL,NETWORK_PASSPHRASE,AUTH_DOMAIN, andALLOW_UNAUTHENTICATED_SYNCER. A deployment supplying none of these will now fail to start rather than running open — that's intentional, but it does mean the bridge needs configuring before the next deploy.