Skip to content

fix(pages): security hardening: only serve to our origin - #22156

Merged
mikehardy merged 3 commits into
ankidroid:mainfrom
david-allison:anki-26-09-2-part-7
Oct 1, 2026
Merged

mikehardy merged 3 commits into
ankidroid:mainfrom
david-allison:anki-26-09-2-part-7

Conversation

@david-allison

Copy link
Copy Markdown
Member

Note

Assisted-by: GPT-6 - proposed and implemented while updating the backend, I cleaned it up

Purpose / Description

Ensure that pages such as 'Deck Options' cannot be used to load external web pages which can then load internal anki assets from the backend, or call into Anki APIs

Fixes

Approach

  • define isInternalUrl
    • Don't serve assets if this is false
  • Open webpages externally if they're not in an allowlist to ensure they can't access our APIs
    • Except docs.ankiweb

How Has This Been Tested?

Tests added

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@david-allison david-allison added the Review High Priority Request for high priority review label Sep 28, 2026
@criticalAY
criticalAY self-requested a review September 28, 2026 20:16

@mikehardy mikehardy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Local CI-parity on a rebase onto current main: jacocoUnitTestReport failed (twice, including after :AnkiDroid:clean) with:

PrefsSearchBarTest → Could not resolve fragment for xml/preferences_review_reminders

Investigation notes:

  • This PR only touches pages/WebView hardening (no preferences changes)
  • GitHub unit CI is green
  • Isolated PrefsSearchBarTest passes; full suite fails
  • Same local failure pattern as #22141 on this host
  • Root suspicion: HeaderFragment.configureSearchBar indexes R.xml.preferences_review_reminders when Prefs.newReviewRemindersEnabled, but getFragmentFromXmlRes has no branch for that XML — so if any earlier test leaves the flag enabled, this assertion fails (suite-order pollution)

Holding Approve/enqueue until local full unit is green (or the prefs mapping / test isolation is fixed). Lint + emulator local lanes were green.

@mikehardy mikehardy added the Needs Author Reply Waiting for a reply from the original author label Sep 30, 2026
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Snapshot diff report: Previous regressions were resolved by the latest commit.

@david-allison

Copy link
Copy Markdown
Member Author

@mikehardy mikehardy mentioned this pull request Sep 30, 2026
3 of 5 tasks
@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Sep 30, 2026
@david-allison
david-allison dismissed mikehardy’s stale review September 30, 2026 22:48

Flake has been split off; requesting re-review

Define 'isInternalUrl' for future use

Part of 21926

Assisted-by: GPT-6
Serve bundled assets only for URLs on the configured local-server origin.

Part of 21926

Assisted-by: GPT-6
These WebViews have an exception and are able to access our backend.

If we load an untrusted page, it would retain this backend access, probably
 fine unless we're specifically targeted, but not worth the risk.

The manual is fine (via Deck Options), so trust it.

Part of 21926.

Assisted-by: GPT-6
@mikehardy
mikehardy force-pushed the anki-26-09-2-part-7 branch from 2d9c8b0 to bbf80e5 Compare October 1, 2026 04:42

@mikehardy mikehardy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey David 👋

LGTM — origin-locked asset serving and fail-closed external navigation match the hardening slice for #21926, and the PageWebViewClient / DeckOptions tests lock that in.

Local lint / unit / package / emulator are green on a rebase onto current main (full unit suite; PrefsSearchBar suite-order red is gone after #22209). Happy to merge.

@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Review High Priority Request for high priority review Needs Review labels Oct 1, 2026
@mikehardy
mikehardy enabled auto-merge October 1, 2026 05:00
@criticalAY
criticalAY removed their request for review October 1, 2026 05:13
@mikehardy
mikehardy added this pull request to the merge queue Oct 1, 2026
Merged via the queue into ankidroid:main with commit cf692fe Oct 1, 2026
23 of 24 checks passed
@github-actions github-actions Bot added this to the 2.26 release milestone Oct 1, 2026
@github-actions github-actions Bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Oct 1, 2026
@david-allison
david-allison deleted the anki-26-09-2-part-7 branch October 1, 2026 07:37
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