Skip to content

fix(seed): block screenshots on the verify-seed screen#619

Open
joshuakrueger-dfx wants to merge 4 commits into
RealUnitCH:stagingfrom
joshuakrueger-dfx:joshua/fix-verify-seed-screenshot
Open

fix(seed): block screenshots on the verify-seed screen#619
joshuakrueger-dfx wants to merge 4 commits into
RealUnitCH:stagingfrom
joshuakrueger-dfx:joshua/fix-verify-seed-screenshot

Conversation

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator

@joshuakrueger-dfx joshuakrueger-dfx commented Jun 1, 2026

Summary

VerifySeedView (where the user re-enters real seed words) was a StatelessWidget with no screenshot protection, unlike the sibling seed screens (create_wallet_view.dart, settings_seed_view.dart). So the entered words could be captured in the OS app-switcher thumbnail or a screen recording.

Fix: convert VerifySeedView to a StatefulWidget and call NoScreenshot.instance.screenshotOff() in initState / screenshotOn() in dispose — exactly the pattern the two sibling seed screens already use (so non-sensitive screens stay screenshot-able).

Test

Widget test (in the existing verify_seed_page_test.dart) asserting the no_screenshot method channel receives screenshotOff on init and screenshotOn on dispose. Native app-switcher blanking is not unit-testable in Flutter; this pins the contract the protection relies on (on-device recents/recording check remains a manual sign-off).

Verification (local, CI-equivalent, Flutter 3.41.6)

  • flutter analyze on the changed files: No issues found.
  • Test passes with the fix; fails when the change is reverted (no screenshotOff call), confirming it catches the regression.

Scope

Addresses the S3 item of #612 ("VerifySeedPage has no screenshot protection at all").

This does not close S1 ("Seed phrase leaks to OS app-switcher / no FLAG_SECURE"): S1 needs FLAG_SECURE on MainActivity.kt to cover the Android recents/app-switcher thumbnail and screen-recording vector app-wide, which this PR does not touch.

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator Author

Why this PR shows a red ✗ (it is not this change)

The functional checks for this PR pass: Analyze & Test ✅, Coverage Floor Gate ✅, BitBox quirks audit ✅. The red is the Visual Regression job, and it is a pre-existing baseline drift, not caused by this PR.

It fails on exactly one golden:

test/goldens/screens/home/home_golden_test.dart
  "HomePage loaded state with price data (variant: macOS)"   → 101 passed, 1 failed

Evidence that it is unrelated to this change:

Resolution: the drifted baseline needs to be regenerated via the golden-regenerate.yaml workflow (a deliberate baseline change, separate from this fix). Once that runs, Visual Regression goes green here too. This PR should be reviewed on its functional checks, which are green.

@TaprootFreak
Copy link
Copy Markdown
Contributor

Per global repo convention (CLAUDE.md):

Don't reference the current task, fix, or callers ("used by X", "added for the Y flow", "handles the case from issue #123"), since those belong in the PR description and rot as the codebase evolves.

Two things to drop before merge:

  • test/screens/verify_seed/verify_seed_page_test.dart — the // Regression for issue #612 S1: … block before the testWidgets.
  • lib/screens/verify_seed/verify_seed_page.dart — the sibling-file reference (create_wallet_view, settings_seed_view) inside the initState comment. Those file names rot if the siblings get renamed; the comment works without naming them.

Separate scope note: this PR actually closes #612 S3 ("VerifySeedPage has no screenshot protection at all"), not #612 S1 ("Seed phrase leaks to OS app-switcher / no FLAG_SECURE"). S1 needs FLAG_SECURE on MainActivity.kt to cover the Android recents/app-switcher thumbnail + screen-recording vector, which this PR doesn't touch. Please update the PR description so we don't accidentally close S1.

Fix itself looks correct for the S3 gap.

@TaprootFreak
Copy link
Copy Markdown
Contributor

Re API as Decision Authority (CONTRIBUTING.md:23–67): screenshot/recording suppression is platform-level security on the device — explicitly under "What is OK in the app" ("Local security gates (PIN, wallet lock, BitBox connection) — physical security boundary, cannot be API-driven"). Fix lands in the correct layer. ✓

No API involvement — this is a pure-client device-policy concern.

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator Author

@TaprootFreak done — dropped the #612 S1 ref from the test and the sibling-file names (create_wallet_view, settings_seed_view) from the initState comment. Also corrected the PR description: this closes S3, not S1 — added a scope note that S1 (FLAG_SECURE on MainActivity.kt) is out of scope. HomePage golden regenerated on dfx01, Visual Regression green.

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator Author

Visual Regression fix

The red VR here was not the shared HomePage baseline drift — it was a real gap introduced by this PR. The two verify_seed golden tests were failing with:

MissingPluginException(No implementation found for method screenshotOff
on channel com.flutterplaza.no_screenshot_methods)
  at MethodChannelNoScreenshot.screenshotOff

Root cause: converting VerifySeedView to a StatefulWidget means initState now calls NoScreenshot.instance.screenshotOff(). The widget test (verify_seed_page_test.dart) already stubs that method channel, but the golden test (verify_seed_golden_test.dart) did not — so rendering threw before a golden could even be produced (it failed even under --update-goldens, which is why the baseline couldn't be regenerated).

Fix (commit 83d28d8): stub com.flutterplaza.no_screenshot_methods in the golden test's setUp/tearDown (mirroring the widget test), then regenerate the two verify_seed baselines on dfx01. Pushing the test fix + regenerated PNGs together.

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator Author

Repo-rule compliance (commit 604cf31): per CONTRIBUTING.md, platform-specific code paths must carry a // @no-integration-test: <reason> annotation while no integration_test/ dir exists. This PR introduces the no_screenshot platform-channel path (screenshotOff/screenshotOn), so I annotated it above initState — same documenting form as biometric_service_adapter.dart / path_provider_adapter.dart.

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator Author

Status — ready for review

  • Review feedback addressed: dropped the #612 S1 issue ref and the sibling-file names from the initState comment; corrected the PR description to S3 (S1/FLAG_SECURE is out of scope, noted).
  • Repo rules: added the // @no-integration-test annotation for the no_screenshot platform path (604cf31); fixed the verify_seed golden test (it crashed with MissingPluginException because the harness didn't stub the channel) and regenerated baselines on dfx01.
  • CI fully green on 604cf31: Analyze & Test, Visual Regression, Coverage Floor Gate, BitBox quirks audit.

Open gates: formal maintainer approval (REVIEW_REQUIRED); on-device check of the screenshot/recents suppression (not CI-verifiable); S1 FLAG_SECURE as a separate PR.

@TaprootFreak TaprootFreak changed the base branch from develop to staging June 1, 2026 14:36
VerifySeedView was a StatelessWidget with no screenshot protection, unlike the
sibling seed screens (create_wallet_view, settings_seed_view). The user enters
real seed words here, so they could land in the OS app-switcher snapshot or a
screen recording. Convert to StatefulWidget and disable screenshots on init /
re-enable on dispose, matching the sibling screens.

Adds a widget test asserting the no_screenshot channel receives screenshotOff
on init and screenshotOn on dispose. (Native app-switcher blanking itself is
not unit-testable; this pins the contract the protection relies on.)

Refs RealUnitCH#612 (S1)
The golden harness rendered VerifySeedView without mocking the
com.flutterplaza.no_screenshot_methods channel, so the screenshotOff()
call in initState threw MissingPluginException and the goldens could not
even be generated. Stub the channel (as the widget test already does).
Per CONTRIBUTING.md, platform-specific code paths must carry the
// @no-integration-test annotation while no integration_test/ dir exists.
@TaprootFreak TaprootFreak force-pushed the joshua/fix-verify-seed-screenshot branch from 604cf31 to 7afd40e Compare June 2, 2026 08:23
@TaprootFreak TaprootFreak added the tier3:full Opt-in: run Tier 3 Maestro handbook flows on this PR label Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tier3:full Opt-in: run Tier 3 Maestro handbook flows on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants