fix(security): refuse prohibited schemes, redact URL credentials in logs, and fix reload state in BrowserServiceImpl (#911)

[KhushiTrivediii]

Fixes #911

Changes

• Refuses prohibited URL schemes in BrowserServiceImpl.navigate (http/https allowlist matching the host's UrlOpenValidation, with authority checks that also block the control-character and embedded-@ bypasses)
• Redacts URL credentials in logging via the shared LogSanitizer.redactUrlUserInfo
• Fixes reload loading/title state in BrowserServiceImpl
• Adds unit tests.

Scope history (review 2026-09-22 sweep, codeq)

This branch originally bundled four commits. The review deduplicated the rest, keeping only the #911 work:

• fix(security): close the search_path on the passkey SECURITY DEFINER set (fix(security): close the search_path on the passkey SECURITY DEFINER set #773) — already merged to dev; provenance verified, no re-landing needed.
• fix(security): redact URI credentials, curl user flags, AWS keys, and PEM keys in McpArgumentSanitizer (Security: a credential inside a URI authority reaches the approval dialog and the ledger unmasked #886) — byte-identical (patch-id) to the head of @KhushiTrivediii's other PR fix(security): redact URI credentials, curl user flags, AWS keys, and PEM keys in McpArgumentSanitizer (#886) #898, which already carries this change; credited there.
• fix(security): implement atomic save and canonical path validation in EditorServiceImpl (Security: the out-of-process EditorService saveFile tears user files on crash and its path gate misses Windows system paths and symlink escapes #885) — superseded by PR fix(editor): atomic saveFile + stricter path gate (fixes #885) #1404 (@choksi2212), which implements the same fix with more refined path validation; fix(editor): atomic saveFile + stricter path gate (fixes #885) #1404 is the surviving implementation.

The remaining commit is the #911 gRPC browser-service fix, rebased onto dev. Review follow-ups (allowlist instead of denylist, malformed-authority refusal, reload title retention) are in the subsequent commits.

[kshivang]

@claude review

[claude]

Claude finished @kshivang's task in 3m 39s —— View job

---

Review of #916

• Read the PR diff against origin/dev
• Review BrowserServiceImpl.kt (scheme gate, redaction, reload state)
• Review the new tests and the build file change
• Post review

The direction is right and the redaction half is done the way this repo asks for (one shared sanitizer, not a second regex dialect). The scheme gate is where I'd push back hardest, and there's one behaviour regression in the refactor.

---

1. The gate is a three-entry denylist where the repo already has a reasoned allowlist (high)

BrowserServiceImpl.kt:46-51 refuses javascript:, data:, vbscript: and lets everything else through - so file:///etc/passwd, blob:, filesystem:, view-source:javascript:..., about:, chrome: and intent: all pass.

This exact question is already answered in composeApp/src/desktopMain/kotlin/ai/rever/boss/services/UrlOpenValidation.kt, whose KDoc says in so many words:

the scheme is http or https and nothing else. file:, javascript: and data: in particular must never reach a browser tab from here.

It also handles the authority (control characters, spaces, backslash, embedded credentials, IPv6 literals, the non-ASCII-digit port trick) and has UrlOpenValidationTest behind it. It's internal to composeApp so boss-app-browser can't import it as-is - but the rules should be mirrored, or the object lifted into a module both sides can see, rather than a second, weaker dialect growing here. A denylist in one process and an allowlist in another is exactly the "both readers must agree" failure mode AGENTS.md keeps recording.

Fix this →

2. Embedded control characters walk straight past the prefix test (high)

url = request.url.trim() only strips leading/trailing whitespace. Chromium strips ASCII tab and newline anywhere in a URL and strips leading C0 controls, so all of these reach the browser as javascript: while startsWith("javascript:") is false:

"java\tscript:alert(1)"
"java\nscript:alert(1)"
"javascript:alert(1)"      // trim() doesn't touch NUL - isWhitespace('') is false

An allowlist fixes this for free (none of them start with http://), which is another reason to prefer it. If you keep a denylist, it needs control characters stripped before the comparison, the way UrlOpenValidation rejects on isISOControl().

(lowercase() is fine, by the way - Kotlin's no-arg overload is invariant-locale, so there's no Turkish-I problem here.)

3. emitNavEvent drops the real title (medium)

BrowserServiceImpl.kt:77-93 hardcodes .setTitle(url). reload previously sent .setTitle(state.title) (old line ~163). They're equal today only because navigate happens to set title = url; the moment a real title lands in PageState, reload starts broadcasting the URL as the title and nothing will point at this line. Take title as a parameter.

4. redactUrlUserInfo is a test-only wrapper, and the tests duplicate existing coverage (medium)

BrowserServiceImpl.kt:31 adds an internal fun that does nothing but delegate, and it's part of what pushed the class over detekt's function ceiling, hence the new @Suppress("TooManyFunctions") at line 21. Meanwhile three of the six new tests (URL userinfo credentials are redacted, a password containing an at sign..., an at sign outside a URL authority...) re-assert behaviour that plugin-platform/plugin-logging/src/desktopTest/.../UrlUserInfoInTextTest.kt already pins - including the last-@ rule, whitespace termination, scp-style remotes and malformed input.

Suggest: call LogSanitizer.redactUrlUserInfo(...) inline at the two call sites, drop the wrapper, drop the @Suppress, and delete the three delegating tests. If you want service-level coverage, assert that navigate's log line is redacted (via a capturing appender) - that's the thing this file actually owns.

5. The scheme tests can't fail the way they look like they can (medium)

assertTrue(jsResponse.errorMessage.contains("javascript:"))
...
assertTrue(dataResponse.errorMessage.contains("data:"))

The error message is a constant containing both substrings, so each assertion passes for either input - swap the two URLs and the test still goes green. More importantly, missing entirely:

• vbscript: (refused by the code, untested)
• uppercase JavaScript:alert(1) (the lowercase() path)
• blank URL (the pre-existing branch that got restructured)
• that a refused navigation leaves windowStates untouched and emits no navigation events. That's the security-relevant invariant - what stops a later refactor from recording or broadcasting the refused URL - and nothing currently pins it.

6. Smaller things

• BrowserServiceImpl.kt:69 - the message names javascript: and data: but vbscript: is refused too, and it never says which scheme was rejected. Echoing just the scheme token (never the full URL) would be more useful and stays safe.
• BrowserServiceImpl.kt:202 - state.copy(isLoading = false) writes back a value that is already false; after this PR isLoading is never true anywhere in the service, so PageInfoResponse.isLoading is a constant. That's a defensible choice for a synchronous stub, but it deserves a one-line comment, otherwise a client polling loading state gets a field that can never be observed and nobody knows it was deliberate.
• BrowserServiceImpl.kt:200 - reload still takes windowStates.values.firstOrNull() with no warning, while getPageInfo warns when more than one window is tracked. Reload now emits events for that arbitrarily chosen window, so the same warning (or a windowId on the RPC) applies more than it did before.
• BrowserServiceImpl.kt:58-75 - validateUrl logs as a side effect inside a when expression whose value is the error string. Works, reads oddly; logging at the call site or returning a small result type would be clearer.
• build.gradle.kts:36 - depending on :plugin-platform:plugin-logging from a kotlinJvm module should resolve (modules/boss-app-editor already consumes :plugin-platform:plugin-language-types, same KMP-with-one-jvm("desktop")-target shape). Worth confirming nativeCompile is still green though - this module is a GraalVM --no-fallback target and the new jar comes with it. LogSanitizer uses only java.net.URI and regex, so no reachability metadata should be needed.

What's good

• Routing through LogSanitizer.redactUrlUserInfo rather than writing a local regex is the right call, and the KDoc at line 25-30 explains why the last-@ rule matters - that's the house style.
• getFavicon was redacted too, not just navigate.
• Extracting emitNavEvent removed a genuine duplication, and making reload emit STARTED/COMPLETED as a pair is consistent with navigate.

Not verified

I could not run ./gradlew :boss-app-browser:test, ktlintCheck or detekt here - the Gradle invocation isn't in my allowed tools - so compilation, the KMP-from-JVM resolution in item 6, and the new tests are reasoned about rather than executed.

--- • branch fix/browser-service-scheme-gate-and-url-redaction

[kshivang]

@claude please review this PR (head 40123d3). Focus: the scheme-gate + authority validation in modules/boss-app-browser (security), the LogSanitizer redaction delegation, and the reload-state fix. The earlier review on c4baca8 is superseded by 5ba9e07 + 40123d3.

[claude]

Claude finished @kshivang's task in 5m 8s —— View job

---

Review of #916 (head 40123d3)

• Read the diff against origin/dev
• Review the scheme gate + authority validation
• Compare against UrlOpenValidation (the host's allowlist)
• Review the LogSanitizer redaction delegation
• Review the reload-state fix and the tests
• Post review

The three follow-ups I'd have insisted on are done: the denylist is an allowlist, emitNavEvent takes a title rather than hardcoding the url, the redactUrlUserInfo wrapper and its @Suppress("TooManyFunctions") are gone, and the tautological contains("javascript:") assertions are replaced. The redaction half I have no notes on - LogSanitizer.redactUrlUserInfo at both call sites, no second regex dialect, getFavicon covered too. The remaining findings are about the gate's parity with the host and about what the new tests can actually fail on.

---

1. The KDoc claims parity with UrlOpenValidation; the code does not deliver it (medium-high)

BrowserServiceImpl.kt:14-19 says "UrlOpenValidation in the host process enforces the same allowlist … so the two readers agree". The scheme allowlist and the FORBIDDEN_IN_AUTHORITY/isISOControl/@ rules are faithfully mirrored, but authorityError stops there. UrlOpenValidation continues into hostOf and isValidPortSuffix, and those carry rules that its own tests pin. Every one of these is refuses(...) in UrlOpenValidationTest and success = true here:

URL	UrlOpenValidation	this gate
http://:8080/ (empty host)	refuses (hostOf → null)	accepts
http://[::1 (unclosed bracket)	refuses	accepts
http://host:notaport/	refuses	accepts
http://localhost:٣٣/ (Arabic-Indic digits)	refuses	accepts
http://localhost:65536/	refuses	accepts
http://localhost:-1/	refuses	accepts

The non-ASCII-digit one is not hypothetical - UrlOpenValidation's own comment records it as a bypass that was found and closed there (in '0'..'9', not Char.isDigit()). None of these is a scheme escape, so the blast radius is smaller than the javascript: case this PR closes; the problem is that a KDoc asserting agreement is exactly how the two quietly drift, which is the failure mode AGENTS.md keeps recording (installedAndOnDisk, DefaultHandlerState.reduce, paneGlyphs - one predicate, two readers).

Two ways out, and I'd take the second if the build allows it:

• mirror hostOf + isValidPortSuffix here and add a test that runs the same fixture list through both answers; or
• lift UrlOpenValidation into a module both composeApp and modules/boss-app-* can see, and delete this copy. It is pure, has no Compose or host dependency, and this PR has already established that modules/boss-app-browser can consume :plugin-platform:*.

Failing either, the honest minimum is to soften the KDoc to say which of the host's rules are mirrored and which are not - a comment that overstates the guarantee is worse than no comment.

Fix this →

2. ignoreCase = true folds U+017F to s, so httpſ:// matches the https:// prefix (low-medium)

url.startsWith(it, ignoreCase = true) at BrowserServiceImpl.kt:59 compares through String.regionMatches(true, …), which is a Character.toUpperCase comparison - and the Unicode simple uppercase mapping of U+017F LATIN SMALL LETTER LONG S is U+0053 S. So "httpſ://evil.example/x" satisfies the allowlist while its actual scheme is httpſ, not https. scheme.length is then still 8, so the authority is parsed correctly and the URL passes as if it were https.

Downstream this is a stub (JxBrowser is in composeApp), so nothing loads it today - but an allowlist that can be satisfied by a string that is not on the list is worth closing, and the fix is smaller than the bug: url.lowercase().startsWith(prefix). lowercase() maps ASCII S→s and leaves ſ alone, so the long-s form correctly fails, and Kotlin's no-arg overload is root-locale so there is no Turkish-I problem.

Worth saying plainly: UrlOpenValidation has the same line and the same hole. That is a separate issue rather than something to pile onto this PR - but if you take option 2 above and share one object, fixing it once fixes both.

3. reload re-broadcasts the tracked title, not the url cannot fail (medium)

BrowserServiceImplTest.kt:104-126. navigate is the only writer of PageState.title and it writes title = url, so state.title == state.url for every state this service can hold. The assertion assertEquals("https://example.com", it.title) therefore passes for setTitle(state.title), setTitle(state.url) and the original setTitle(url) the earlier review flagged. Mutate state.title → state.url in reload and the test stays green - which is the same shape of defect the scheme tests were rewritten to remove.

Nothing in the public API can set a title independently today, so the options are: assert the property that is observable (both events carry the same url and title as getPageInfo, and STARTED/COMPLETED arrive as a pair), or rename the test to what it checks. As it stands it documents a guarantee it does not hold.

4. Still nothing pins the security-relevant invariant of a refusal (medium)

A refused navigate must leave windowStates untouched and emit no navigation events. The code does that (the early return precedes both), but no test says so - and that is precisely the line a later refactor moves. Three of the four scheme/authority tests assert only success == false and a nonempty message; add, for one refused URL:

• getPageInfo() still reports an empty url after the refusal (and still reports the previous url if one was navigated first - the stronger version);
• a subscribed collector receives nothing.

Also missing, and cheap: uppercase HTTPS://example.com succeeds (nothing currently exercises the ignoreCase path in the accepting direction), and the blank-URL branch returns its own "URL must not be blank" message rather than the scheme message.

Fix this →

5. reload's write-back is a no-op that can undo a concurrent navigate (low)

BrowserServiceImpl.kt:209 - windowStates[state.windowId] = state.copy(isLoading = false). Nothing in the service ever sets isLoading = true any more, so this writes a value the state already holds, and PageInfoResponse.isLoading is now a constant false no client can ever observe as true. Two consequences:

• It is a read-modify-write on a ConcurrentHashMap outside computeIfPresent, so a navigate landing between the read at line 207 and the write at 209 is reverted - the window's url goes back to the previous page. Pre-existing shape, but the line now buys nothing in exchange for keeping the race.
• The earlier review asked for a one-line comment saying the constant is deliberate; it is not there. Either drop the write entirely, or make it the honest pair (true before STARTED, false before COMPLETED) so the field means something once this stops being a stub.

6. Smaller things

• reload still takes windowStates.values.firstOrNull() with no warning while getPageInfo warns on size > 1 (:189). Reload now mutates state and emits events for that arbitrarily chosen window, so the case for the same warning is stronger than it was before this PR, not weaker.
• The gate is only reachable through gRPC. authorityError is a top-level private function that takes logger: Logger as a parameter, which reads as what it is - a way around detekt's TooManyFunctions. An internal object BrowserUrlGate with its own logger and a fun refusalFor(url: String): String? would satisfy detekt for the same reason, be directly unit-testable without building a NavigateBrowserRequest, and be the obvious seam to later swap for the shared UrlOpenValidation.
• A refusal log can name the wrong host. redactUrlUserInfo's KDoc records that https://evil.example\@good.example/x is logged as https://[REDACTED]@good.example/x because \ does not end an authority for the sanitizer. This gate refuses that URL (backslash is in FORBIDDEN_IN_AUTHORITY), so the warn line at :221 reports refusing a URL at good.example when the refused one pointed at evil.example. Nothing to fix in the sanitizer; worth knowing when reading these logs.
• charArrayOf(' ', …) under a comment that calls them "the printable characters" - the host writes ' ', which reads better and makes a diff between the two copies trivial.
• isLoading = false at :123 restates the PageState default; harmless, arguably documents intent now that reload writes the same value.

Build change

implementation(project(":plugin-platform:plugin-logging")) matches the existing modules/boss-app-editor → :plugin-platform:plugin-language-types precedent exactly (same kotlinJvm-consumes-KMP-with-one-jvm("desktop")-target shape), so resolution should be fine. LogSanitizer is in desktopMain and imports only org.slf4j.LoggerFactory and java.net.URI, so the GraalVM --no-fallback image should need no new reachability metadata. plugin-logging applies the Compose compiler with Compose runtime as compileOnly, so nothing Compose reaches this module's runtime classpath either.

Not verified

Gradle is not in my allowed tools here, so :boss-app-browser:test, ktlintCheck, detekt and nativeCompile are reasoned about rather than executed. The U+017F claim in item 2 rests on the documented Unicode simple uppercase mapping (U+017F → U+0053) and String.regionMatches(ignoreCase = true) semantics - I could not run a JVM snippet in this sandbox to demonstrate it, so confirm it locally before acting on it.
• branch fix/browser-service-scheme-gate-and-url-redaction

[fluck-boss]

Thank you for this - the scheme allowlist with the tab/newline bypass analysis, the credential redaction in logs, and the reload isLoading fix were all the right calls.

While this was in review, #931 landed on dev covering the same #911 ground: the scheme gate (with the WHATWG tab/newline stripping handled the same way you did), logging that never echoes a refused URL, LogSanitizer redaction for accepted URLs, and the reload isLoading=false invariant with the STARTED/COMPLETED pair. The two implementations converge almost exactly, so I am closing this one as superseded rather than asking you to rebase onto a now-duplicate change.

One piece of your PR is not yet on dev: the authority checks - refusing empty/malformed authorities and credentials in the authority, matching what the host deep-link gate does. I filed a follow-up issue for that delta and pointed at your authorityError implementation and tests as the starting point. A PR for that would be very welcome. Thanks again!