Skip to content

fix(WalletWebViewClient): restrict externally-launched URL schemes (refs #5)#55

Open
jim-daf wants to merge 1 commit intowwWallet:mainfrom
jim-daf:fix/security-issue-5-scheme-allowlist
Open

fix(WalletWebViewClient): restrict externally-launched URL schemes (refs #5)#55
jim-daf wants to merge 1 commit intowwWallet:mainfrom
jim-daf:fix/security-issue-5-scheme-allowlist

Conversation

@jim-daf
Copy link
Copy Markdown

@jim-daf jim-daf commented Apr 22, 2026

Restrict the schemes we forward to startActivity (refs #5)

Scope

Issue #5 itself is about minifying / obfuscating injectjs.js.
This PR lands a small, narrowly scoped hardening of the closely related navigation
path inside WalletWebViewClient, which is the entry point
the injected script ultimately depends on for safe routing.

What changes

WalletWebViewClient.shouldOverrideUrlLoading() currently does:

if (request.url.scheme != baseUrl.scheme ||
    request.url.host   != baseUrl.host) {
    activity.startActivity(Intent(Intent.ACTION_VIEW, request.url))
    return true
}

i.e. any URL whose host or scheme differs from the wallet's
own origin is handed straight to startActivity. That happily
accepts intent://...#Intent;...end, content://, file://,
javascript: etc., which is the classic Android-WebView
intent-smuggling / deep-link attack surface (CWE-939).

We add an explicit allow-list:

val allowed = scheme == "https" || scheme == "http" ||
              scheme == "eid"  || scheme == "tel"  ||
              scheme == "mailto"
if (!allowed) return true
  • https/http covers the normal external-link case.
  • eid is preserved because the existing comment names it as
    the Ausweis App handover.
  • tel and mailto are preserved because they are the only
    other URI schemes a normal page is expected to launch.

Anything else is dropped silently (return true), the same
way other audited Android wallet apps handle it.

Why bundle it under #5

The injection script and the URL-forwarding policy are the
two halves of the wallet's WebView trust boundary. #5 is the
open issue tracking work on that boundary.

…wWallet#5)

Refs wwWallet#5

WalletWebViewClient.shouldOverrideUrlLoading() forwards every
non-base-host request to startActivity(Intent.ACTION_VIEW, ...).
That includes any scheme the loaded page chooses — `intent://`,
`content://`, `file://`, `javascript:` etc. — which is the
classic Android-WebView intent-smuggling / deep-link attack
surface (CWE-939) on a page that runs inside a credential
wallet.

Add an explicit allow-list of `https`, `http`, `eid`, `tel`,
`mailto` schemes before the startActivity call. Anything else
is dropped (`return true`).

Issue wwWallet#5 itself is about minifying `injectjs.js`; this PR is
an opportunistic hardening of the closely related navigation
path and is intentionally out of scope of that minification
work.
@jim-daf jim-daf marked this pull request as ready for review April 22, 2026 14:11
Copilot AI review requested due to automatic review settings April 22, 2026 14:11
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Hardens WalletWebViewClient.shouldOverrideUrlLoading() by restricting which non-wallet URL schemes are forwarded to startActivity, reducing Android WebView intent/deeplink attack surface when navigating away from the wallet origin.

Changes:

  • Adds an explicit scheme allow-list (http/https/eid/tel/mailto) before calling startActivity for non-wallet origins.
  • Silently blocks disallowed schemes instead of forwarding them to the host activity.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +31 to +33
val scheme = request.url.scheme?.lowercase()
val allowed = scheme == "https" ||
scheme == "http" ||
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

request.url.scheme can be null (e.g., for some relative navigations or malformed URLs). With the current scheme?.lowercase() + allow-list, a null scheme becomes allowed == false and the WebView navigation is swallowed (return true), which can break in-wallet navigation unexpectedly. Consider explicitly handling scheme == null (e.g., let super.shouldOverrideUrlLoading handle it, or resolve against baseUrl before applying the allow-list).

Copilot uses AI. Check for mistakes.
@@ -23,6 +23,20 @@ class WalletWebViewClient(
// Open all foreign web pages and app schemes like "eid" for the AusweisApp
// externally. Only wwWallet code is allowed inside the app.
if (request.url.scheme != baseUrl.scheme || request.url.host != baseUrl.host) {
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

The foreign-origin check only compares scheme + host. This treats https://example.com:444/... as same-origin as https://example.com/... (ports are ignored), which can allow loading non-wallet services on the same host inside the privileged WebView. Consider including an (effective) port comparison (or compare authority) when deciding whether a URL is allowed to stay in-app.

Copilot uses AI. Check for mistakes.
Comment on lines 40 to 41
activity.startActivity(Intent(Intent.ACTION_VIEW, request.url))
return true
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

startActivity(Intent(ACTION_VIEW, ...)) can throw ActivityNotFoundException (notably for eid: if AusweisApp isn’t installed, but also for mailto: on minimal devices). Consider guarding with resolveActivity(packageManager) and/or catching the exception and surfacing a user-visible error via onErrorReceived instead of crashing.

Copilot uses AI. Check for mistakes.
Comment on lines +32 to +36
val allowed = scheme == "https" ||
scheme == "http" ||
scheme == "eid" ||
scheme == "tel" ||
scheme == "mailto"
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

This new scheme allow-list is security-critical routing logic but currently has no unit tests. Consider adding tests that exercise shouldOverrideUrlLoading for allowed schemes (http/https/eid/tel/mailto) and denied schemes (intent/content/file/javascript), plus edge cases like null scheme/host and differing ports.

Copilot uses AI. Check for mistakes.
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