feat(api): gate the wallet-backend-fronted routes behind --wallet-backend-routes-enabled - #146
Conversation
POST /api/v1/accounts/balances returns 500 on every valid request in prd,
stg, and local: it fronts wallet-backend, and a client is only built when
both a URL and a signing key are configured (services/wallet_backend.go:70,
:81). Only dev sets WALLET_BACKEND_*, so everywhere else
configureNetworkClient returns nil and the handler fails immediately with
"wallet backend client not configured for network". The endpoint is
publicly reachable in prd while broken, and no client calls it yet.
Add --balances-enabled (env BALANCES_ENABLED, default true) controlling
whether the route is registered at all. When false it is never added to the
mux, so the path 404s exactly as an unknown path would: no handler, no auth
middleware, no upstream call. prd sets it false; dev, stg, and local keep
their current behaviour. Re-enabling is an env-var change and a restart, not
a release.
Implementation notes:
- The route table gains an `enabled` field rather than conditionally
appending. The table's stated purpose is to be the one source of truth
that both initHandlers and the strict-mode guard test enumerate, so a
route "cannot silently skip the auth guard"; making its shape depend on
config would break that and hide the route from the guard. Cost is a
mechanical `true` on the other 10 entries, since Go positional composite
literals require every field.
- testCfg must set BalancesEnabled: true. It builds a mostly zero-value
AppConfig, so the flag would default to false in tests and
AllUserFacingRoutesGatedInStrict would see a 404 where it asserts 401 —
a failure that reads like an auth regression. That guard's assertion
message now names the unregistered-route case explicitly.
- Two new tests pin both states: 404 when disabled, and 401-in-strict when
enabled (proving the route is registered *and* still auth-gated, so the
flag cannot become an accidental auth bypass).
Verified by running the binary both ways: with BALANCES_ENABLED=false the
route 404s, /token-prices is unaffected, and startup logs "route disabled by
config; not registering"; with the flag unset it 500s (registered, upstream
unconfigured) and 400s on a bad address.
This does not fix the endpoint. Only configuring WALLET_BACKEND_* for stg
and prd does that. GET /accounts/{address}/transactions is broken
identically and is deliberately left untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a runtime configuration gate for the account-balances route, allowing production to return 404 while its wallet-backend dependency remains unavailable.
Changes:
- Adds the
--balances-enabled/BALANCES_ENABLEDsetting. - Conditionally registers the balances route.
- Tests enabled and disabled routing behavior and documents deployment considerations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
internal/config/config.go |
Defines the balances toggle. |
cmd/serve/serve.go |
Registers the CLI/environment-backed flag. |
internal/api/serve.go |
Skips disabled routes during mux registration. |
internal/api/serve_test.go |
Tests disabled and authenticated enabled states. |
docs/design/2026-07-29-balances-endpoint-toggle.md |
Documents design and deployment behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The api-package tests set AppConfig.BalancesEnabled directly, so they exercise route registration but not the config path prd actually uses — prd disables the route via the BALANCES_ENABLED env var, not a CLI flag. A renamed flag or a broken env binding would leave those tests green while the endpoint stayed registered in prd: a fail-open on the control this PR exists to provide. Add three command-level tests covering the real chain (env var -> viper AutomaticEnv + '-'->'_' replacer -> bindFlags -> the bound config field): - the flag defaults to true; - BALANCES_ENABLED=false reaches AppConfig.BalancesEnabled; - BALANCES_ENABLED=true does too, so the binding works in both directions rather than only latching off (re-enabling prd flips this same variable). Confirmed the guards are load-bearing by mutation testing rather than assuming a passing test means a real one. Each mutation is caught by the intended test: - flag default flipped to false -> DefaultsTrue fails - flag renamed to a different word -> FalseFromEnv fails - v.AutomaticEnv() removed -> FalseFromEnv fails That exercise also corrected the comment I first wrote: renaming the flag balances-enabled -> balances_enabled does NOT break the env binding, because the replacer maps both spellings onto BALANCES_ENABLED. The test pins the env-var spelling, not the flag's punctuation. Addresses Copilot review feedback on #146. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
should we also gate |
…t-backend-routes
GET /api/v1/accounts/{address}/transactions is broken for exactly the same reason
as balances and was left publicly 500ing. Confirmed both in code and against the
live services:
- Identical nil-client guard at services/wallet_backend.go:320-323, reached via
the same configureNetworkClient, which returns nil when no WALLET_BACKEND_*
URL + signing key pair is configured.
- 500 on all four combinations measured 2026-07-29 — prd and stg, PUBLIC and
TESTNET — each instant (~0.08-0.4s, no round trip), same as balances.
Gate it with the same flag and rename to reflect the real scope:
BalancesEnabled -> WalletBackendRoutesEnabled
--balances-enabled -> --wallet-backend-routes-enabled
BALANCES_ENABLED -> WALLET_BACKEND_ROUTES_ENABLED
One flag rather than two: the routes share one dependency and one failure mode, so
there is no state where enabling exactly one is correct — a second flag's only
novel setting would be a misconfiguration. The name states the dependency rather
than an endpoint list, so a future third wallet-backend route needs no new flag,
and it groups with the WALLET_BACKEND_* vars already in the manifests.
These two are the COMPLETE set of wallet-backend-dependent routes, not just the
two we happened to notice: walletBackendService reaches exactly two handlers
(serve.go:219,222), and the interface's third method GetHealth is wired to no HTTP
handler at all.
Tests are now table-driven over both routes, so a third one extends coverage by a
line instead of being silently missed, plus WalletBackendRoutesGatedTogether
asserts routes() disables EXACTLY these two patterns. That last guard exists
because gating only one route — leaving the other publicly 500ing in prd, the very
bug this flag closes — would otherwise pass every other test.
Mutation-tested: reverting account-history to always-enabled fails both
WalletBackendRoutesDisabledNotRegistered/account-history and GatedTogether.
Verified end-to-end on the built binary. With WALLET_BACKEND_ROUTES_ENABLED=false
both paths 404 and the startup log names both skipped patterns; with the flag unset
both 500 (registered, upstream unconfigured). token-prices and ping are unaffected
in both states.
Design doc renamed and rewritten for the wider scope; account-history is no longer
listed as a non-goal.
Addresses @aristidesstaffieri's review comment on #146.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Yes — confirmed, and done in 0fedf7f. Thanks, this was the right call. TL;DR: You're right that it has the same dependency, and it's broken the same way right now — I measured it returning errors in both production and staging, on both networks. Both endpoints are now turned off by one shared switch, which I've renamed to describe the dependency rather than one endpoint. I also checked whether anything else depends on wallet-backend: these two are the complete set. Implementation details (for agents)Confirmation, in code. freighter-backend-v2/internal/services/wallet_backend.go Lines 318 to 324 in 0fedf7f Confirmation, live. Measured 2026-07-29, anonymous (both deployments run
All four instant and network-independent, which is what separates the nil-client path from a wallet-backend outage. Completeness check — these two are the whole set. Rather than gate the two we'd noticed, I checked what else touches the service: Rename: One flag, not two. They share one dependency and one failure mode, so there's no state where enabling exactly one is correct — a second flag's only novel setting would be a misconfiguration. Naming it after the dependency also means a future third wallet-backend route needs no new flag, and it groups with the freighter-backend-v2/internal/api/serve.go Lines 249 to 262 in 0fedf7f Tests are now table-driven over both routes, so a third extends coverage by one line instead of being missed. I also added Verified on the built binary with the new variable, both routes:
Off logs Companion PRs updated for the rename: stellar/kube#4783 (prd manifest) and stellar/wallet-eng-runbooks#21 (on-call note now covers both routes). The kube one still needs to land before the next prd image bump — the variable is inert on the running image, so that order closes the window rather than opening one. |
TL;DR
Two endpoints return a 500 to every caller in production, staging, and local dev — the account-balances one and the account-history one. Both depend on wallet-backend, which is only configured in dev; everywhere else the service has no wallet-backend client at all, so requests fail before going anywhere. Both have been publicly reachable and broken in production this whole time; no Freighter client calls either, so nothing depends on them.
This adds one config switch controlling whether those endpoints are served at all. Turned off, their paths behave as though they were never built: callers get a 404 and nothing runs behind them. Production turns them off; dev, staging, and local are unchanged. Turning them back on later is one environment-variable change and a pod restart — no rebuild and no release.
To be clear about what this is not: this does not fix either endpoint. It stops production from publicly serving errors. Making them actually work requires configuring wallet-backend for those environments, which is separate work.
Updated after review: originally gated only account-balances. @aristidesstaffieri pointed out account-history has the same dependency — confirmed, so both are now gated by one flag and it is renamed accordingly.
Implementation details (for agents)
Root cause of the 500. Both service methods fail at their first step, on the same guard, because a wallet-backend client is only constructed when both a URL and a signing key are non-empty:
freighter-backend-v2/internal/services/wallet_backend.go
Lines 178 to 181 in 0fedf7f
freighter-backend-v2/internal/services/wallet_backend.go
Lines 318 to 324 in 0fedf7f
freighter-backend-v2/internal/services/wallet_backend.go
Lines 69 to 88 in 0fedf7f
Those flags default to
"", and only the dev deployment setsWALLET_BACKEND_*(verified againststellar/kubemasterate0ce2144d). Measured 2026-07-29 — 500 on all four combinations, prd and stg ×PUBLICandTESTNET, each instant (~0.08–0.4s, no round trip). Instant and network-independent is what distinguishes the nil-client path from an upstream outage.Why exactly these two routes — this is the complete set, not just the two we noticed.
walletBackendServiceis passed to exactly two handlers,NewAccountBalancesHandlerandNewAccountHistoryHandler. The interface's third method,GetHealth, is wired to no HTTP handler at all (the onlyGetHealthcall underinternal/api/is onrpcService, inrpc_health.go). So gating both closes the entire wallet-backend-dependent surface.What changed:
internal/config/config.go— newWalletBackendRoutesEnabledfield onAppConfig.cmd/serve/serve.go—--wallet-backend-routes-enabled, defaulttrue. Viper'sAutomaticEnvwith a-→_replacer binds it toWALLET_BACKEND_ROUTES_ENABLEDwith no extra wiring, the same mechanism as the existing--db-enabled/DB_ENABLEDpair.internal/api/serve.go— the route table gains anenabledfield;initHandlersskips disabled routes, so their paths fall through to the mux's 404 with no handler, no auth middleware, and no upstream call.freighter-backend-v2/internal/api/serve.go
Lines 249 to 262 in 0fedf7f
Why one flag rather than two. The routes share one dependency and one failure mode, so there is no state where enabling exactly one is correct — a second flag's only novel setting would be a misconfiguration. The name states the dependency rather than an endpoint list, so a future third wallet-backend route needs no new flag, and it groups with the
WALLET_BACKEND_*vars already in the manifests. If a route is later added that can work without wallet-backend, it should get its own gate rather than widening this one.Why a table field rather than conditionally appending. The table's stated purpose is to be the one source of truth that both
initHandlersand the strict-mode guard test enumerate, so a route "cannot silently skip the auth guard." Making the table's shape depend on config would break that property and hide disabled routes from the guard test. The cost is a mechanicaltrueon the other 9 entries, since Go positional composite literals require every field.A trap this had to avoid.
testCfgbuilds a mostly zero-valueAppConfig, so the flag would have defaulted tofalsein tests — andAllUserFacingRoutesGatedInStrict, which probes every gated route for a 401, would have seen a 404 and failed in a way that reads like an auth regression.testCfgnow sets ittrueexplicitly, and that guard's assertion message names the unregistered-route case so a future failure is self-explanatory.Tests. Table-driven over both routes — 404 when disabled, 401-in-strict when enabled (the latter proves each route is registered and still auth-gated, so the flag can't quietly become an auth bypass). Adding a third wallet-backend route extends that coverage by one line rather than being silently missed. Plus
WalletBackendRoutesGatedTogether, which assertsroutes()disables exactly these two patterns — it exists because gating only one route, leaving the other publicly 500ing in prd, would otherwise pass every other test. Command-level tests cover the config path prd actually uses (flag default, andWALLET_BACKEND_ROUTES_ENABLED=false/=truereaching the config field through viper), since the api-package tests set the field directly and would stay green through a renamed flag or broken env binding — a fail-open.Mutation-tested rather than assumed. A passing test isn't evidence it guards anything, so each guard was checked against the break it's meant to catch:
falseDefaultsTrueFalseFromEnvv.AutomaticEnv()removedFalseFromEnvDisabledNotRegistered/account-historyandGatedTogetherVerification. Full suite green (19 packages);
go vet,gofmt, andshadowclean. Beyond the tests, I ran the built binary both ways, since the env var is what kube actually sets:WALLET_BACKEND_ROUTES_ENABLED=falseThe off run logs
route disabled by config; not registeringonce per route, naming both patterns; the on run logs neither. The 500s in the second row reproduce the prd/stg failure exactly, which independently confirms the diagnosis above.golangci-lintandgofumptaren't installed in my environment andstaticcheckis built against an older Go than this module requires, somake checkcan't complete locally — those three need CI to cover.Observability note. While disabled, the metrics middleware labels unmatched requests
handler="unknown", so this traffic is counted there rather than under its own handler labels. Anything keyed on those labels goes quiet — intended, but worth knowing before someone hunts for the missing series.Deploy ordering. The companion
stellar/kubechange must land before the prd image bump.WALLET_BACKEND_ROUTES_ENABLEDis inert on images that predate the flag, so that order gives zero exposure window; the reverse ships the new image with both routes still enabled in prd until the manifest catches up.Follow-ups / out of scope:
WALLET_BACKEND_*is configured. The signing keys are secrets and must be seeded in each namespace's own Vault folder, since cross-namespace reads are blocked by therestrict-external-secrets-to-namespacesKyverno policy.WALLET_BACKEND_TESTNET_SIGNING_KEY, so?network=TESTNET500s there today. Pre-existing and unrelated to this change./api/v1/account-balances, which does not exist — verified against prd, that path 404s while the real one 500s. Tracked separately; notefreighter-backendv1 genuinely serves/api/v1/account-balances/<pubkey>, so a blanket rename would break the v1 runbooks.docs/design/2026-07-29-wallet-backend-routes-toggle.md, which also records the rejected alternatives (deriving the gate from whether wallet-backend is configured, two independent flags, and blocking at the Traefik edge).