Skip to content

Use env for debug/profile only - #2273

Closed
i5hi wants to merge 2 commits into
developfrom
env-debug-profile
Closed

Use env for debug/profile only#2273
i5hi wants to merge 2 commits into
developfrom
env-debug-profile

Conversation

@i5hi

@i5hi i5hi commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Redone pr without formatting

@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown

Claude finished @i5hi's task in 4m 16s —— View job


PR Review: Use env for debug/profile only

  • Read PR diff and changed files
  • Review new StagingEnv / StagingExchangeGuard logic
  • Review consumer changes (locator, dio factory, usecases, routers)
  • Check for correctness bugs, security issues, regressions
  • Post review summary

Reviewed against e8a417366 (PR merge-base). Overall this is a solid, well-motivated change — moving staging endpoints/credentials out of source and gating the testnet exchange behind kReleaseMode + an "is configured" check is the right design. Findings below.


🔒 Security

✅ Good — fixes a credential-leak-to-production bug. Previously onHttpAuthRequest in exchange_auth_screen.dart / exchange_kyc_screen.dart unconditionally proceeded with the staging basic-auth credentials for any HTTP auth challenge, including on mainnet (accounts.bullbitcoin.com). The new isTestnet ? ... : null means prod challenges are no longer auto-answered with staging creds. Nice catch.

⚠️ Rotate the old credentials. bbadmin / We are staging 05! are removed from source (good) but remain in git history and were publicly committed. Removing them from constants.dart does not invalidate them — they should be rotated on the staging server.


🐞 Correctness / Regressions

1. fund_exchange_router.dart — new ShellRoute changes navigation structure. (lib/features/fund_exchange/fund_exchange_router.dart:78)
These were previously top-level GoRoutes; wrapping them in a brand-new ShellRoute introduces a nested Navigator. This can change pop() / back-button / pushNamed behavior for the funding flow (other routers like buy/sell already used ShellRoute, so they're unaffected). Please verify the funding flow still pops correctly end-to-end. The inner routes are also not re-indented under the new ShellRoute (cosmetic only — CI runs flutter analyze, not a format check, so it won't fail, but worth a dart format).

2. Inconsistent "testnet not configured" handling across usecases.

  • convert_* and get_available_currencies silently fall back to the mainnet repo (StagingEnv.useTestnetExchange → false).
  • get_exchange_user_summary throws instead.
  • Other exchange usecases (buy/sell order creation, etc.) still branch on raw isTestnet and were not gated, so in a non-release testnet build without staging env they'd hit a Dio whose baseUrl is '' (StagingEnv.apiUrl returns empty). In practice the StagingExchangeGuard blocks those screens, so it's defense-in-depth rather than a live bug — but the gating pattern is uneven and easy to get wrong later. Consider centralizing the "use testnet exchange?" decision.

3. Silent mainnet rates in testnet. Following from #2, a debug/testnet build without staging env will display mainnet exchange rates while the user believes they're on testnet. This appears intentional (testnet has no real rates), but it's undocumented and could confuse. A log line or comment would help.


Minor / Nits

  • StagingExchangeGuardPage and tryProceed read environment once via context.read<SettingsCubit>(); they won't react to a runtime environment switch without a rebuild/navigation. Likely fine (env changes usually re-route), just noting.
  • StagingEnv getters (apiUrl, authUrl, …) return the raw env value regardless of isConfigured/kReleaseMode. Only isConfigured/useTestnetExchange enforce the release guard — so any future caller using a getter directly without checking isConfigured would bypass the gate. The current callers are all gated correctly; worth keeping in mind.
  • .env.example ships the staging URLs (non-secret) with empty user/pass — good. CI's cp .env.example .env keeps isConfigured false in tests (empty creds), so tests exercise the mainnet-fallback path.

No blocking issues from a correctness standpoint given the guards. Main action items: rotate the leaked staging creds and verify the new fund_exchange ShellRoute navigation.

@basantagoswami

Copy link
Copy Markdown
Contributor

This can be closed. in favor of #2310

@i5hi i5hi closed this Jun 22, 2026
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