[abhinavlenka]

Fixes #1591.

Problem

BrowserServiceImpl.navigateRefusal gated only the scheme. A Navigate over IPC could still carry an authority that composeApp's UrlOpenValidation (the deep-link gate) refuses, including https://apple.com@evil.example, where the credentials disguise the real host.

Change

After the scheme check, Navigate now applies the same authority rule as UrlOpenValidation:

• // must follow the scheme (https:example.com is refused, as the deep-link gate does).
• The authority (up to the first /, ? or #) must be non-empty and free of spaces, NBSP, backslashes, quotes, angle brackets and control characters.
• No @. Credentials are refused, not stripped.
• The host must be non-empty, and any port must be : or 0–65535 in ASCII digits. Bracketed IPv6 is handled, and fullwidth digits are rejected.

It judges the URL as an engine reads it, with tabs and newlines stripped, the same form the scheme check already used. So https://apple.com\t@evil.example is refused as the credential URL it becomes. Refusal messages name the rule and never echo the URL, keeping #911's "never the URL" logging contract.

modules/boss-app-browser can't depend on composeApp, so the rule is written here as file-scope functions, as #916 did. Adapted from @KhushiTrivediii's authorityError in #916, as the issue suggests.

Tests

BrowserServiceImplTest: 15/15, plus ktlintCheck and detekt.

• New: credential URLs are refused and their message leaks no part of the URL (including the tab-hidden @); 14 malformed authorities are refused; well-formed ones still navigate, including those a stricter parser would reject (localhost:3000, [::1]:8080, my_service, a bare :); and a refused authority emits no event and leaves the page alone.
• Changed: "the response contract is not redacted" now carries its secret in the query and fragment (the parts describeUri drops from the log), since userinfo can no longer reach the contract.
• Mutation-checked: dropping the @ check, the port check or the whole authority gate each fail the matching tests.

[fluck-boss]

@claude please review this exact head (f887104) for merge into dev. Check that the change is correct and complete for its stated goal, that tests cover the risk being fixed, and flag any security issues (secrets, injection, auth or approval bypass, path handling). Keep the review focused on this diff.

[claude]

Claude finished @fluck-boss's task in 4m 2s —— View job

---

Review of f887104 (merge into dev)

• Read the diff
• Compare against composeApp's UrlOpenValidation (the rule being mirrored)
• Check correctness/completeness of the authority gate
• Check test coverage of the risk
• Security pass (logging leaks, bypasses, injection)

Verdict: correct and complete for its stated goal. No blocking issues. One real test gap (NBSP), and three low-severity notes.

---

What I verified

The gate is in the right place. navigate is the only URL entry point in modules/boss-app-browser (goBack/goForward/reload/getPageInfo replay stored state, which can now only hold a gated URL), and navigateRefusal still returns before the windowStates write, before tryEmit, and before the describeUri log line. a refused authority leaves no event and no page change pins that directly.

The index arithmetic is safe. authorityRefusal(engineForm(url), scheme.length + 1) depends on the scheme token starting at index 0 of engineForm(url) — true, because navigableScheme takes normalized.substring(0, normalized.indexOf(':')) and SCHEME_SYNTAX is ASCII-only, so .lowercase() cannot change the length. Correct, but it is an implicit coupling across two functions: if navigableScheme ever grows a leading-whitespace strip or any other normalisation, the offset silently points into the middle of the authority. Cheapest hardening is for navigableScheme to return the offset alongside the scheme (or a one-line require), rather than recomputing it at the call site. Low severity.

hostOf / isValidPortSuffix are behaviourally equivalent to the UrlOpenValidation originals. Walked case by case: [] → refused, [::1 (unclosed) → refused, [::1]x → refused, [::1]:80:90 → refused, a.example: → accepted (default port), :8080 → accepted port but empty host → refused, :000000 → accepted, :-1/:99999/20-digit → refused, fullwidth digits → refused. removeSurrounding("[", "]") is safe here because that branch is only reached when both brackets were found.

Not a bypass (checked the obvious ones):

• https:\\evil.example — WHATWG treats \ as / for special schemes, so an engine reads this as https://evil.example; the // check refuses it, fail-closed, matching the deep-link gate.
• https://apple.com%40evil.example — @ is a forbidden domain code point after percent-decoding, so the engine refuses it; not something this gate has to catch.
• https://[not-ipv6]/ passes the gate (IPv6 literal contents are unvalidated) — fail-open but inert, the engine refuses, and it matches UrlOpenValidation.
• Leading-whitespace forms (" https://…") still fall out at SCHEME_SYNTAX, unchanged.

Security/logging. Every new refusal string is a compile-time constant; nothing derived from the URL reaches the log or errorMessage. The credentials test asserts the absence of secret/evil/apple, including for the tab-hidden form, so #911's "never the URL" contract holds on the new branch. The re-pointed the response contract is not redacted test still covers what it was for (query + fragment survive into finalUrl and events) — the right adjustment, since userinfo can no longer reach that path. every url logged by the service goes through describeUri is unaffected (the new code adds no logger call). No injection, path handling or approval surface in this diff. ktlint max_line_length = 140: no new line exceeds it.

---

Findings

1. ' ' in FORBIDDEN_IN_AUTHORITY is untested — the one member of the array no test pins. (test gap, low)

BrowserServiceImpl.kt:262 lists NBSP, and NBSP is not an isISOControl character, so it is only refused by that array entry. The malformed-authority table exercises a plain space, \, ", <, > and ``, but nothing exercises NBSP — delete ' ' from the array and all 15 tests still pass. The composeApp copy does pin it (`UrlOpenValidationTest.kt:123`, `refuses("http://exa mple.com")`), so this is exactly the drift the copy is exposed to. One row in the existing table closes it:

"https://a b.example/",

Fix this →

2. The KDoc says "the same rule" where it is the same rule applied to a different string. (doc accuracy, low)

authorityRefusal's KDoc and the PR body both say this is "the same rule composeApp's UrlOpenValidation applies". It is the same predicate, run on engineForm rather than the raw string, and the two genuinely disagree in one direction: https://ho\tst.example/ is refused by the deep-link gate (control character in the authority) and accepted here, because after the strip it is host.example — which is what an engine actually loads. That is the right call and it is what makes the https://apple.com\t@evil.example case work, but a future reader comparing the two files will read the divergence as a bug. Worth one clause ("…applied to the engine form, so a tab cannot hide an @ and cannot invent one either").

3. Nothing pins the two copies together, and only one side knows the other exists. (maintainability, low)

FORBIDDEN_IN_AUTHORITY, hostOf and isValidPortSuffix are now verbatim second copies. The duplication itself is unavoidable (modules/ can't depend on composeApp, and #916 set this precedent), but UrlOpenValidation.kt has no idea a copy exists, so a fix made there — the kind its own KDoc records having needed twice — reaches only one gate. A path-level cross-reference in both KDocs is the cheap version; this repo's house style (RetiredPluginIds pinned against RetiredPlugins.ALL) would suggest a shared fixture list eventually, which is more than this PR should take on.

---

Not blocking, noted for completeness

• engineForm(url) is computed twice per navigate (once inside navigableScheme, once at the call site). Immaterial at this call rate.
• The refusal messages distinguish "no authority" from "no valid host and port", but only the credentials wording is asserted. Fine as is — the other two are not load-bearing for a caller.
• I could not run :boss-app-browser:test, ktlintCheck or detekt here: Gradle invocation is outside this job's allowed tools, so the 15/15 in the PR body is unverified by me. Everything above is from reading the code and hand-evaluating the tables against the implementation.
  • browser-authority-gate

[fluck-boss]

CI green on the refreshed head (ubuntu+windows), Claude review on the current head found no blocking or security issues, and a maintainer diff review is clean. Squash-merging to dev.