You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Analyze new staging_env.dart / staging_exchange_guard.dart
Check for correctness bugs, security issues, regressions
Post review findings
This is a solid hardening change. Removing the hardcoded staging URLs and basic-auth credentials from constants.dart and gating staging behind kReleaseMode is the right direction, and I verified there are no leftover references to the removed constants (bbApiTestUrl, bbAuthTestUrl, bbKycTestUrl, basicAuthUsername, basicAuthPassword) anywhere in lib/ or test/, so it compiles. The UI guard wrapping preserves the original navigation branches in each screen.
A few things worth addressing before merge:
🔴 Rotate the leaked staging credentials
lib/core/utils/constants.dart previously contained basicAuthUsername = 'bbadmin' / basicAuthPassword = 'We are staging 05!'. Removing them from HEAD does not remove them from git history — they remain permanently visible in prior commits of a public repo. Moving them to .env only helps going forward. These credentials should be rotated on the staging proxy, otherwise the change provides no real secret-hygiene benefit.
🟠 kReleaseMode blocks use but not embedding in the binary
lib/core/utils/staging_env.dart:6-10 — the values come from String.fromEnvironment(...) as static const. If a release build is ever built with --dart-define-from-file=.env, the staging URLs/creds are still compiled into the release binary, even though isConfigured returns false at runtime and never uses them. So the PR's guarantee ("env is not used for release builds") holds at runtime, but the only thing preventing the secrets from being embedded is build discipline (not passing the dart-define to store builds). The comment in .env.example documents this, which is good — just flagging that the kReleaseMode check is a runtime guard, not a build-time one.
🟠 Empty-string baseUrl fallback is fragile
Several call sites use StagingEnv.apiUrl ?? '' (exchange_locator.dart:91,125,143, bullbitcoin_api_dio_factory.dart:14) and the webview screens use StagingEnv.authUrl ?? '' / kycUrl ?? ''. When staging isn't configured this yields a Dio/WebView pointed at '' rather than failing fast.
In practice this is mostly shielded: the StagingExchangeGuard blocks UI navigation into the exchange on testnet, and ExchangeNotificationDatasource.connect() (exchange_notification_datasource.dart:66) early-returns when there's no API key. But it relies on every entry point being guarded and on the API-key check — an unguarded/programmatic access would silently hit an empty URL. Consider either not registering the testnet datasources when !StagingEnv.isConfigured, or asserting a non-empty URL, so misconfiguration surfaces loudly instead of as confusing empty-host errors.
🟢 Latent bug fixed: basic-auth no longer sent on mainnet
Previously onHttpAuthRequest in exchange_auth_screen.dart and exchange_kyc_screen.dart unconditionally replied to HTTP auth challenges with the staging bbadmin credentials — including on mainnet webviews. The new isTestnet ? ... : null makes it testnet-only and cancels cleanly when creds are missing. Good catch/fix.
🔵 Minor: isConfigured couples API URL to basic-auth creds
isConfigured requires all five vars (URLs and username/password) to be set. A dev who sets only the URLs in .env (as .env.example ships username/password empty) gets a fully-blocked exchange and '' API base URLs. That's probably fine since the staging proxy fronts everything with basic auth, but the coupling means partial config fails opaquely. The STAGING ENV MISSING snackbar helps; a more specific message ("set STAGING_BASIC_AUTH_*") would aid onboarding.
🔵 Minor: blank webview if a guard is ever bypassed
exchange_auth_screen.dart:42 / exchange_kyc_screen.dart:35 load StagingEnv.authUrl ?? '' / kycUrl ?? ''. If either screen is reached on testnet without staging configured (e.g. a future deep-link/redirect path not covered by the guard), the WebView loads a blank URL rather than showing the missing-env message. Low risk given current entry points are all guarded, but a defensive check inside the screens would be more robust than relying solely on callers.
Summary
No blocking correctness bugs or compile breaks. The main action item is rotating the previously-committed staging credentials (they live in git history regardless of this PR). The empty-string ?? '' fallbacks are safe today only because of the navigation guard + API-key check — worth hardening so misconfiguration fails loudly. Nice incidental fix of mainnet webviews sending staging credentials.
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
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.
Move all staging envs to a .env
Ensures env is not used for release builds and only for debug or profile builds