fix(logging): keep query and fragment secrets out of browser URL log lines by nikhil-jangir-iitk · Pull Request #691 · risa-labs-inc/BossConsole

nikhil-jangir-iitk · 2026-09-15T00:04:29Z

Summary

BrowserServiceImpl logged a browser tab's full URL at INFO, the default level, as "Browser created via BrowserService". That line runs on every tab creation, and the fluck-browser plugin creates the browser at the tab's current page URL, so it also runs on every hibernation wake and crash recovery. DeepLinkHandler logged every incoming link the same way: six lines, at INFO or WARN, including every https link BOSS receives as the default browser.

A tab URL is where an OAuth code, an implicit-flow #access_token, a magic-link token or a presigned X-Amz-Signature lives, and a deep link's query can be a whole URL (boss://url?url=) or a command (boss://terminal?command=). Where the line goes:

BossLogger appends the data map to the line as-is and writes it to SLF4J, which prints to System.out (simplelogger.properties).
GlobalLogCapture keeps System.out for the Console panel, and every plugin reads it through PluginContext.logDataProvider.
The default console plugin serves that capture to MCP clients as console_tail and console_search. Neither is in McpMutatingToolCatalog, so both resolve to defaultReadOnlyAction = ALLOW.

Run on dev: a URL logged through BossLogger.info with the exact "Browser created" message reached LogDataProviderImpl.logs and exportLogs() with its code intact. Seven synthetic secrets sent through the real DeepLinkHandler.processDeepLink and processCommandLineArgs all reached LogDataProviderImpl.logs; after this change none does.

Evidence: what the probes captured

Throwaway tests (not in this PR) started a DesktopLogCapture, built LogDataProviderImpl over it, and either logged the production message through BossLogger or called the real DeepLinkHandler entry point.

Browser line, raw URL, captured verbatim:

INFO boss - [BROWSER] BrowserServiceImpl: Browser created via BrowserService | {handleId=h1, url=https://app.example.com/oauth/callback?code=PROBE-INFO-CODE-123&state=xyz, profile=default, activeBrowsers=1}

Check	Result
raw URL at INFO reached `logs`	true
raw URL at INFO in `exportLogs()`	true
raw URL at DEBUG reached `logs`	false

Deep links, before and after:

Input	Lines holding the canary on `dev`	After
`boss://url?url=https%3A%2F%2Fapp.example.com%2Fcb%3FoobCode%3DCANARY1`	1 (INFO "Processing deep link")	0
`boss://url?url=https://app.example.com/cb?code=CANARY2`	1	0
`boss://auth/verify?oobCode=CANARY3` (unrouted)	2 (INFO + WARN)	0
argv `https://b.s3.amazonaws.com/f.pdf?X-Amz-Signature=CANARY4`	3 (command line INFO, processing INFO, unrouted WARN)	0
`boss://terminal?command=export%20API_TOKEN%3DCANARY5`	1	0
`boss://plugin?id=probe-none&action=open&invite=CANARY6`	1	0
`boss://url?url=example.com%2Fpath%3Fq%3DCANARY7`	1	0

One captured line on dev, verbatim:

INFO boss - [SYSTEM] DeepLinkHandler: Processing deep link | {uri=boss://url?url=https%3A%2F%2Fapp.example.com%2Fcb%3FoobCode%3DCANARY1, origin=EXTERNAL}

describeUri / maskUriParams output for the browser URL shapes below:

Input	`describeUri`	`maskUriParams`
`https://app.example.com/callback?code=SECRETVAL&state=abc`	`https://app.example.com/callback (with query params)`	`https://app.example.com/callback?code=[REDACTED]&state=abc`
`https://b.s3.amazonaws.com/f.pdf?X-Amz-Credential=AKIA&X-Amz-Signature=SECRETVAL`	`https://b.s3.amazonaws.com/f.pdf (with query params)`	unchanged, signature visible
`https://x.firebaseapp.com/__/auth/action?mode=resetPassword&oobCode=SECRETVAL`	`https://x.firebaseapp.com/__/auth/action (with query params)`	unchanged, `oobCode` visible
`https://x-access-token:SECRETVAL@github.com/o/r.git`	`https://github.com/o/r.git`	unchanged, token visible
`https://hooks.slack.com/services/T000/B000/SECRETVAL`	unchanged, token in path visible	unchanged, token in path visible

Fix

Every site below now logs LogSanitizer.describeUri(...), which keeps scheme, host and path and replaces the query and fragment with a note that they were there:

File	Lines	Level
`BrowserServiceImpl`	"Browser created via BrowserService"; "Discarding browser from a recycled engine - retrying"; "Cookie rejected"	INFO; WARN; WARN
`DeepLinkHandler`	"Received deep link" from the macOS, Windows and Linux open-URI handlers and from the command line; "Processing deep link"; "Deep link host is not routed"; "URL command queued"	INFO (6); WARN
`CLICommandHandler`	"Invalid URL"	WARN

describeUri no longer prints null://. It interpolated the scheme, so a reference with no scheme read as null://localhost, which is exactly what "Invalid URL" logs for input normalizeAndValidateUrl rejected, and what "URL command queued" logged for boss://url?url=example.com. It now renders localhost, //host/path, and (with query params) without a leading space. Absolute URIs are described as before.

Why describeUri and not maskUriParams. maskUriParams redacts by exact parameter name (10 names). Measured on 14 credential-bearing URL shapes:

Shape	`maskUriParams` hides it	`describeUri` hides it
OAuth `?code=`, `#access_token=`, magic-link `?token=`, hash-routed `#/reset?token=`	yes	yes
Firebase `oobCode`, S3 `X-Amz-Signature`, Azure SAS `sig`, Zoom `pwd`, `client_secret`, `apikey`, `sessionId`, userinfo `x-access-token:T@`	no	yes
token in the path (Slack webhook, `/reset-password/<token>`)	no	no
Total	4 / 14	12 / 14

For deep links, a name list cannot work at all: inside boss://url?url=https://app/cb?code=S the only parameter is url. Keeping parameter names but not values was considered and dropped, because a name is caller-chosen text too (boss://x?<token>).

Guard

BrowserUrlLogConventionTest scans the three files for logger.<level>(...) calls with a "...url", "...uri", "...link" or "...href" data key whose value is not LogSanitizer.describeUri(...). maskUriParams does not count.
- It parses whole calls, skipping string literals, character literals and comments, so multi-line calls count and a quote in a comment does not end a call.
- A self-check pins single-line, multi-line and suffixed keys, the describeUri-only rule, parentheses inside string literals, quotes in comments and character literals, and has*/is* flag keys.
- Its KDoc lists what it cannot see: a receiver not named logger, a key that does not end in url/uri/link/href, a URL built into the message string, a raw string containing a single ".
DescribeUriSecretShapesTest pins what describeUri returns for the shapes above, for deep links carrying a URL or a command, and for references with no scheme. DescribeUriReferenceTest pins the scheme-less rendering in plugin-logging itself, so the module's own test run covers it.

Scoped to three files on purpose. A tree-wide scan would have to allowlist lines that open PRs are about to move:

fix(auth): keep credentials out of authentication logs #567 changes the authentication and passkey logging sites;
fix: route external URLs to focused window #671 rewrites the URLHandlerService URL routing, including its two raw URL lines;
fix(auth): bind the passkey browser view to its composition #639 changes the lines next to the one in DesktopPasskeyBrowserView.

Worth a reviewer's attention

Deep-link log lines no longer show parameter values, so a successful boss://plugin?id=...&action=... no longer shows its id and action at INFO. DeepLinkActionRegistryImpl still warns with both when no handler exists or the handler declines. Each route's handler still logs what it logged before (the path for file, folder and workspace links, the panel id for a panel link).
describeUri is in plugin-logging, so the scheme-less rendering applies to plugins through the host copy as well. Only references with no scheme change.

Verified

On dev d22d2bfeb:

Red first: with the tests added and production unchanged, 5 tests fail. BrowserUrlLogConventionTest names exactly 7 sites (the six DeepLinkHandler lines and "Cookie rejected"); the scheme-less describeUri pins fail with null://.
Tests: :plugin-platform:plugin-logging:desktopTest, and :composeApp:desktopTest for ai.rever.boss.utils.*, ai.rever.boss.cli.* and ai.rever.boss.plugin.browser.*: 1048 tests, 0 failures, 7 skipped.
Lint: ktlintCheck and detekt clean for composeApp and plugin-logging.
Collision check: a 3-way merge of the seven changed files against every open PR that changes one of them (91 PR/file pairs, 95 open PRs) adds 0 conflicts. fix(logging): redact credentials embedded in a URL's userinfo before logging #640 also edits plugin-logging's LogSanitizer.kt; merging the two branches gives 0 conflicts.

Mutation-checked, on the final code:

Mutation	Caught by
each of the four original sites back to the raw URL (4 mutations)	convention scan (original run)
"Processing deep link", the macOS handler line, the command-line line, "Cookie rejected", "Browser created" back to `maskUriParams` (5 mutations)	convention scan
scan accepts any `LogSanitizer` call	self-check
scan does not skip line comments	self-check, convention scan
scan does not skip character literals	self-check
scan: string literals not skipped; `has`/`is` keys not exempt	self-check (original run)
`describeUri` keeps the query; keeps the fragment; keeps userinfo; returns raw text on a parse error	pins (original run)
`describeUri` interpolates a null scheme again	4
`describeUri` drops the `//` of a network-path reference	2
`describeUri` keeps the leading space for a query or fragment alone	1

Not covered

A credential in the URL path is not hidden by describeUri or maskUriParams. For a reference with no scheme that is the whole input, so text that is not a URL at all, passed to open-url, is still logged by "Invalid URL".
The three OS open-URI handlers were not executed (they are Desktop.setOpenURIHandler callbacks); their lines are the same one-call change and are in the scan. "Invalid URL" was not reached in the probe; its rendering is pinned through describeUri.
URLHandlerService still logs the raw URL in "No usable window registered" (WARN) and "Emitted URL open event" (DEBUG). fix: route external URLs to focused window #671 rewrites both lines; masking them here adds 2 merge conflicts with it.
BrowserServiceImpl "Failed to create browser" logs error = e. I found no JxBrowser message that names the URL, but the scan does not look at exception messages.
CloneProjectDialog "Starting clone operation" is left alone. DesktopGitService.cloneRepository logs the same URL at INFO through maskUriParams, whose userinfo handling is fix(logging): redact credentials embedded in a URL's userinfo before logging #640.
describeUri costs some detail: it drops the port and writes an opaque URI such as about:blank as about://. Input java.net.URI rejects becomes [uri-parse-error].
The console plugin's MCP tools were not run (another repository); what they return is read from ConsoleMcpTools.kt.
Not exercised in a running app.

Update after review (2026-09-15)

Rebased onto dev d22d2bfeb. The original commit applied with no conflicts and its changed lines are identical, so the reviewer merge 66fd6f3ef (and the duplicate trackpad commits the review noted) are no longer in this branch.
Review finding 1 (the whole link is logged earlier through maskUriParams): addressed at all six DeepLinkHandler lines, not the five listed, plus "Cookie rejected"; the guard no longer accepts maskUriParams.
Review finding 2 (null://): fixed in describeUri.
Review finding 3 and 4 (scan holes, missing pins): comments and character literals are skipped, the remaining limits are in the KDoc, and the relative-reference and about:blank cases are pinned.

kshivang · 2026-09-15T01:37:14Z

@claude Please review the URL logging change at dc4f8d2 against current dev. Focus on the four changed logging sites and regression coverage; identify inherited trackpad history separately.

kshivang · 2026-09-15T01:43:19Z

@ninja-lifts Your four call-site changes correctly remove query, fragment and userinfo values from routine browser/URL diagnostics, and the secret-shape cases make the intended boundary clear. I independently compiled the actual production sanitizer and host facade with your two new test classes: all 6 tests pass. The documented limitation for credentials in URL paths remains; this does not claim universal URL secret redaction.

I repaired the inherited-history conflict in 66fd6f3ef by merging dev and preserving dev's entire BrowserHandleImpl, including the new exact-frame authority and editor-command API. Your original commit is retained. The resulting five-file diff is byte-identical to your original logging patch; no other dev behavior was replaced. A subsequent merge-tree against newer dev 5dc82116e is clean. This integration repair is reviewer work; the logging change and tests are yours.

Current-head upstream Build run 34918240211 is approved but incomplete, and the one requested Claude review has not supplied a result yet. I am placing this in draft for validation handoff, not because pending CI is an author defect. Please let those checks finish, address independently verified findings if any, and refresh current-dev integration before marking ready. Run the focused logging tests plus the required platform/quality checks on that final head; exercise tab creation, recycled-engine retry, deep-link URL opening and invalid CLI URLs with synthetic query/fragment canaries when manual app testing is available. No running-app check was performed here.

#567 (auth logs), #640 (userinfo sanitizer), and #671 (window routing) are complementary work, not duplicates. Preserve their behavior on future integration. I requested Claude once and am not starting another repair/polling loop. Updated integration evidence is also in the PR body.

claude · 2026-09-15T01:48:27Z

Claude finished @kshivang's task in 7m 44s —— View job

Review: URL logging change at `dc4f8d2`

Locate dc4f8d2 and read its production diff
Review the four changed logging sites
Review BrowserUrlLogConventionTest regression coverage
Review DescribeUriSecretShapesTest pins against LogSanitizer.describeUri
Check HEAD state still matches after the merges
Separate out the inherited trackpad/swipe history

Verdict

The four call-site edits are correct and the guard is a genuinely good one - a source scan is the right shape for "one missing call in an argument list". Two things I'd want addressed before merge, one of them because it makes the guard read stronger than it is:

for the boss://url path the same URL is still logged in full at INFO two hops earlier, in a file this PR guards;
the CLI site now prints null://… for the most common input it receives.

Premises I checked rather than took from the description - all hold:

BossLogger appends the data map and error.message verbatim (plugin-logging/.../BossLogger.kt:511-517); no LogSanitizer call anywhere in it, so nothing sanitizes a map for you.
describeUri had zero production callers before this PR (only 4 pins in LogSanitizerTest:346-370). This PR is what puts it on a hot path, which matters for finding 2.
All four sites are still sanitized at branch HEAD (66fd6f3), and no merge since dc4f8d2 added a new url-keyed log line to the three guarded files.

I could not run :composeApp:desktopTest or ktlint/detekt here (the sandbox declined the toolchain commands), so the test results in the description are unverified by me; the analysis below is static plus java.net.URI semantics.

1. The `boss://url` leak is still open upstream of the line that was fixed (`DeepLinkHandler`)

handleUrlLink (DeepLinkHandler.kt:710) is only reached through processDeepLink, which logs the whole link first:

// DeepLinkHandler.kt:363
logger.info(
    LogCategory.SYSTEM,
    "Processing deep link",
    mapOf("uri" to LogSanitizer.maskUriParams(uri), "origin" to origin.name),
)

maskUriParams redacts by exact parameter name, and url is not in sensitiveUriParamNames (LogSanitizer.kt:351-363: token, access_token, refresh_token, code, error_description, id_token, session_token, api_key, key, secret). The documented link shape is boss://url?url=https%3A%2F%2Fexample.com (DeepLinkHandler.kt:708), so the target URL rides inside one non-listed parameter and is logged whole, at INFO, with its nested %3Fcode%3D… intact. Unencoded forms fare no better - maskParamsInSegment splits on &/=, so url=https://app/cb?code=S is one param named url.

Same line at :198, :269, :325 and :381 (five INFO/WARN sites in total, all on the reachable path).

So for the boss://url route, dc4f8d2 removes the third copy of the URL from the log while the first still carries it in full - and CLICommandHandler then hands the normalized URL to URLHandlerService, which logs it raw at :145 (WARN, i.e. above the default threshold - the "not DEBUG, so it prints by default" argument in the description applies there too, not only to the INFO sites). The end-to-end reduction for this path is smaller than the table suggests.

This is also the one place where the new guard actively misleads: BrowserUrlLogConventionTest accepts any value starting with LogSanitizer., so DeepLinkHandler passes the scan at 11/11 sites while five of them log a nested credential-bearing URL. The mutation table lists "a site switched to maskUriParams" as not caught, by design - that design choice is exactly what hides this. I'd either narrow the exemption (allow maskUriParams only where the query is provably not URL-valued) or state in the KDoc that maskUriParams is treated as sufficient and is not sufficient for a link whose query carries another URL.

Fix this →

2. `CLICommandHandler:236` now prints `null://localhost` for its commonest input

The value at that site is, by construction, a string CLISecurityValidator.normalizeAndValidateUrl just rejected - and it rejects exactly three classes (CLISecurityValidator.kt:29-55). describeUri builds "${parsed.scheme}://${parsed.host ?: ""}${parsed.path ?: ""}", and java.net.URI happily parses a relative reference with a null scheme:

Rejected input	What the WARN now says
empty / blank	`[empty]` ✅
no dot (`localhost`, `app?code=S`)	`null://localhost`, `null://app (with query params)` - literal `null://`
contains a space/newline (`my site.com`)	`[uri-parse-error]`

Two of the three rejection classes now produce a value that tells the operator nothing about what was wrong, and one of them leaks the word null into an operator-facing log line. Sanitizing the site is right - handleOpenUrl does receive the decoded boss://url target, so a secret can reach it - but the rendering wants a fix: either handle a null scheme in describeUri (it lives in the published plugin-logging module, so that also fixes it for plugins) or log the rejection reason plus a bounded shape here. Worth pinning either way: DescribeUriSecretShapesTest pins nine shapes and not the relative-URI case that this exact site hits, so nothing flags it.

Fix this →

3. The two `BrowserServiceImpl` sites - no objection

:657 and :671 are the right two lines, config.url is the right value, and the inline comment explains the choice where the next reader needs it. describeUri handles the nullable input, and the nearby :1067 cookie line stays on maskUriParams, which is correct there (a cookie URL's query is not the credential and the domain detail is the diagnostic).

One unguarded channel in the same hunk, for the record rather than as a request: the sibling logger.error(LogCategory.BROWSER, "Failed to create browser", error = e) at :674 appends e.message raw, and a JxBrowser creation failure plausibly names the URL it failed on. Not a confirmed leak - I did not find a message format that includes it - but the scan cannot see that channel at all, and the KDoc's "not seen" list mentions only the message string, not the exception.

4. Regression coverage

Good on the thing it aims at: all four changed sites live in the three scanned files, so reverting any one of them fails the scan, and the self-check pins the parser's interesting cases (multi-line calls, suffixed keys, a ( inside a string literal, has*/is* keys). repoRoot() is the shared helper, not a fourth copy. Holes worth a line in the KDoc rather than code:

Key must end in url/uri/link/href. urlKeyed is "[A-Za-z_]*(?:url|uri|link|href)", so "urlString", "linkTarget", "uriPath" are invisible. None exist in the three files today; a scan that says "url-keyed" should say which half of the key it matches.
closingParen counts bare " only. A raw string ("""), a '"' char literal, or a comment with one unbalanced quote inside a logger call desyncs inString. Usually that fails loudly (error("unbalanced call…")), but the odd-quote-then-)-in-a-message case truncates the argument region early and silently drops later pairs. Note this PR just added a comment inside one of these argument lists, which is the shape that would trip it.
Only a receiver literally named logger. \blogger\. misses log.info(, browserLogger.warn(, and BossLogger.forComponent(…).info( at a call site. Fine for these three files, relevant when the file list grows.
DescribeUriSecretShapesTest pins are accurate against the implementation and are the right nine shapes for the "names maskUriParams does not list" argument. Add the relative-URI and about:blank cases (see finding 2) - the KDoc already describes about://, so pinning it costs one line.

The scoping argument (three files, not a tree scan, because #567/#671/#639 are moving the other sites) is sound and I'd keep it.

Inherited trackpad history (separate, as asked)

The branch carries a second copy of PR #650's three swipe/trackpad commits, applied on top of the merge that already contained them:

Re-applied on this branch	Already in the base `1c887b6` (merge of #650)
`783021c` Respect horizontal scrollers and commit trackpad navigation on release	`2fa6063`
`7169d28` Fix swipe cancellation, observer lifecycle and terminal delivery	`62200fc`
`9373641` Publish prior contact termination for home swipe attribution	`a445e5f`

Identical diffstats (11 files, +913/-207 each pair). They are content no-ops: git diff 1c887b6 HEAD -- MacOSScrollGesturePhases.kt swipe-nav.js scripts/test/test-swipe-nav.js is empty, so the merge resolved to the same tree the base already had. GitHub also reports this PR as 5 files / +240/-4, which is dc4f8d2 exactly - nothing from the trackpad work, eccd65e (#693) or the seven merged dev commits (10f3e05, 3c5953a, bc5da50, c2479ae, 56ce78f, 61941e3, 6004233) reaches the reviewable diff.

So: no functional impact, history noise only. Two consequences worth knowing - git log on dev will show the swipe work twice, and if #650 were ever reverted on dev these copies would silently re-land it. A squash merge (or a rebase of dc4f8d2 onto current dev) removes both. Caveat on my verification: this runner's origin/dev is a depth-1 fetch (5dc8211 only, no merge base), and fetching more was declined, so I compared against the branch's own base 1c887b6 and GitHub's computed PR diff rather than diffing dev directly.

Summary: the intent and the two BrowserServiceImpl sites are right, the guard is worth keeping, and I'd fix the DeepLinkHandler upstream INFO line (finding 1) and the null:// rendering (finding 2) before merge. No changes pushed - this is review feedback only.
· branch fix/log-browser-url-query

nikhil-jangir-iitk · 2026-09-15T10:19:02Z

Thanks @kshivang, and for the integration repair. I rebased onto dev d22d2bfeb instead of keeping the merge: the original commit applied unchanged, so the duplicate trackpad commits are gone too.

Follow-up 5ca5be1a7 addresses the review:

Whole link logged earlier through maskUriParams: real, and it was six DeepLinkHandler lines, not five (the Windows open-URI handler too). Sending seven synthetic secrets through the real processDeepLink / processCommandLineArgs (a nested boss://url target, an unrouted oobCode, an argv https link with X-Amz-Signature, a terminal command, a plugin action parameter) put all seven in LogDataProvider.logs; after the change, none. All six lines and "Cookie rejected" now use describeUri, and the guard accepts only describeUri.
null://: fixed in describeUri, which also removes it from "URL command queued" for boss://url?url=example.com.
Scan holes: comments and character literals are skipped; the other limits are in the KDoc; relative-reference and about:blank cases are pinned.

Red first (7 sites named), 1048 tests green, ktlint/detekt clean for composeApp and plugin-logging, 11/11 mutations caught, 0 added conflicts across 91 PR/file pairs. One cost is in the description: a successful plugin action link no longer shows its id/action at INFO. Not run in an app.

kshivang · 2026-09-15T10:45:21Z

@claude Please review the current head of #691 against dev, including contributor follow-ups and prior finding dispositions. This is the current morning batch review; do not repeat repaired findings. Focus on remaining correctness, credential exposure, regressions and focused test coverage. Earlier reviewer repairs remain separately attributed.

claude · 2026-09-15T10:45:53Z

Claude finished @kshivang's task in 7m 54s —— View job

Review: current head of #691 vs `dev`

Fetch dev, get the reviewable diff at current head
Verify prior findings 1-4 dispositions (not re-reported)
Review the seven changed production log sites
Review describeUri scheme-less rendering
Review the guard scan and the three new test classes
Look for remaining credential exposure / regressions

Verdict

The branch is now a clean two-commit stack on dev 77c4a40 (no merge, no duplicated trackpad history - that disposition is confirmed below). All four earlier findings are genuinely addressed, and the maskUriParams -> describeUri conversion is complete rather than partial: all 11 url-keyed log entries in the three guarded files go through describeUri, and maskUriParams survives in those files only inside comments. I verified that by grep rather than trusting the description. Build & Test run 34958956245 is green on this head, which settles the test/lint claim my previous pass could not check.

One new finding I would fix before merge (#1, a host-dropping case that defeats the stated purpose of describeUri at the very site that changed from maskUriParams), one cosmetic correctness nit, one coverage recommendation, and a short nits list.

I could not execute anything again - the sandbox declined ./gradlew, javac, java and python3. Everything below is static reading plus java.net.URI.Parser semantics; where that matters I give the one-line test that settles it.

Prior findings: dispositions (confirmed, not re-reported)

Earlier finding	Disposition
1 - whole link logged upstream through `maskUriParams`	Fixed, and wider than reported: 6 `DeepLinkHandler` lines (`:201`, `:246`, `:273`, `:329`, `:373`, `:388`) plus `BrowserServiceImpl:1069` "Cookie rejected". The guard no longer accepts `maskUriParams` - the self-check at `BrowserUrlLogConventionTest.kt:87` pins it as an offender.
2 - `null://localhost` from `CLICommandHandler:236`	Fixed in `describedBase` (`LogSanitizer.kt:222-226`), so it is fixed for plugins too. Pinned in both modules.
3 - scan holes (comments, char literals)	`endOfCommentOrChar` (`:179-192`) handles `//`, `/* /`, `'x'` and `'\''`; the remaining limits (receiver named `logger`, key must end* in url/uri/link/href, raw string with a single `"`) are in the KDoc at `:34-41`.
4 - missing relative-URI and `about:blank` pins	Both present (`DescribeUriSecretShapesTest:87-95`, `DescribeUriReferenceTest`).
Inherited trackpad history	Gone. `git log origin/dev..HEAD` is exactly the two logging commits; `origin/dev` is the parent of `52db907`.

The four existing describeUri pins in LogSanitizerTest:349-370 are all absolute URIs, so the describedBase change cannot have silently rewritten an older expectation.

1. `describeUri` drops the host entirely whenever `java.net.URI` will not parse the authority as server-based

describedBase reads parsed.host:

// LogSanitizer.kt:222-226
private fun describedBase(parsed: URI): String {
    val scheme = parsed.scheme?.let { "$it://" }
    val authority = parsed.host?.let { if (scheme == null) "//$it" else it }
    return scheme.orEmpty() + authority.orEmpty() + parsed.path.orEmpty()
}

URI.getHost() is null for any authority that fails parseServer, in which case URI falls back to a registry-based authority: authority is set and host, userInfo and port are left null. parseHostname scans L_ALPHANUM plus - and ., so two everyday shapes land there:

Input	What `describeUri` returns
`https://my_host.internal/a?t=S` (underscore - legal in DNS, ubiquitous in Docker/K8s/internal names)	`https:///a (with query params)`
`https://例え.jp/a` (IDN, not punycoded)	`https:///a (with query params)`

So the log line loses the host, which is the one fact these lines exist to report, and prints a bare https:///. The path is unaffected because parseHierarchical parses it after the authority regardless.

This is a straight regression at BrowserServiceImpl:1069, the one site that moved off maskUriParams: maskUriParams passes the authority through verbatim, so "Cookie rejected" used to name the domain, and your own earlier note said the domain is the diagnostic there. It also hits "Browser created via BrowserService" (:657), which runs on every tab creation, hibernation wake and crash recovery.

Note the local is named authority while it holds the host - that naming is the confusion itself. Falling back to the real authority with userinfo stripped keeps the fix safe (a registry-based authority keeps userinfo inline, so dropping everything before the last @ is what removes it):

val hostText = parsed.host ?: parsed.authority?.substringAfterLast('@')

Settles in one line either way: assertDescribed("https://my_host.internal/a (with query params)", "https://my_host.internal/a?t=S"). On the current code I expect that to produce https:///a (with query params).

Fix this →

2. `describeUri("#")` and `describeUri("?")` return the empty string

Non-blank input, so the [empty] guard at :197 does not fire. URI("#") parses with path == "" and rawFragment == "", and "" is blank, so hasFragment is false; base is empty and suffix is empty, and :213 returns suffix.trimStart() = "". The line then renders url= with nothing after it, which is indistinguishable from a key whose value was dropped.

Reachable from the site this PR added the call to: CLISecurityValidator.normalizeAndValidateUrl rejects input with no dot, so boss open-url '#' takes the Invalid URL branch, and boss://url?url=%23 reaches "URL command queued" the same way. One-line fix, if (base.isEmpty() && suffix.isEmpty()) "[empty]", and it belongs next to the ?code=secret pin that is already in DescribeUriReferenceTest:35-38.

Fix this →

3. Coverage: the only regression protection in the PR is a text scan

BrowserUrlLogConventionTest is the right shape for the defect it targets, its parser is careful, and the self-checks pin the interesting cases (multi-line calls, suffixed keys, ( inside a literal, a quote in a comment, a '"' char literal). repoRoot() is the shared helper and check(file.isFile) catches a rename. No complaint about what it does.

The gap is that the behavioural evidence - seven canaries through the real processDeepLink / processCommandLineArgs into LogDataProviderImpl.logs - stayed throwaway, so nothing in the repo asserts the outcome. A text scan cannot survive the refactors most likely to reintroduce this: a key renamed to "target" (invisible by the suffix rule), the line moved to a receiver not called logger, or the URL folded into the message string. All three are in the KDoc as limits, which is honest, but you already wrote the test that covers all three at once. Promoting one canary case is the highest-value addition left here.

One channel the guard structurally cannot see and that the KDoc does not list: error = e. BossLogger appends error.message verbatim, and DeepLinkHandler:816 logs logger.warn(..., "Error decoding URL", error = e) for a value that is a deep-link parameter - i.e. inside a guarded file, for exactly the strings this PR is about. Not a leak today (URLDecoder's IllegalArgumentException names only the offending escape characters, not the input), and the same is true of BrowserServiceImpl:674, which the description already discusses. Worth one line in the KDoc's limits list so the next reader does not read "guarded file" as "every channel in this file".

Fix this →

4. Nits, none blocking

describeUri's KDoc first line is now stale. LogSanitizer.kt:189 still says "Returns the scheme and host only for auth URIs", written when the function had zero production callers. It has 11 now, across browser tabs, cookies, the CLI and every incoming deep link. The paragraph you added below it is accurate; the summary line above it is not.
The describeUri( check is a prefix match. value.startsWith("LogSanitizer.describeUri(") at :143 accepts LogSanitizer.describeUri(url) + "?" + rawQuery, and conversely flags a bare describeUri(uri) after a member import as an offender. Fail-safe in the direction that matters; the first half is worth a KDoc clause.
booleanKey exempts on the name, not the value. "hasUrl" to url passes silently (:132, :143). No such site exists today; it is a hole by construction rather than a bug, and belongs in the same limits list.
urlKeyed needs the suffix to be the literal end of the key, so "url2" and "urlString" are invisible. The KDoc names "urlString"; a digit suffix is the same class.
Every opaque URI collapses to scheme://. about:blank and about:crash are the same line, and data:text/html;base64,... becomes data://. Safe, and dropping a data: payload is a bonus - but about:// asserts an authority that does not exist, which is the same interpolation defect as the null:// you just fixed. It is now pinned in two tests, so it reads as intended rather than as an oversight; if you touch describedBase for finding 1, about: costs nothing extra.

What I checked and had no objection to

All 11 url-keyed entries in the three guarded files use describeUri; nothing in those files still hands a url-keyed value to maskUriParams, so the scan should be green (and CI says it is).
describeUri is a single implementation: composeApp's ai.rever.boss.utils.logging.LogSanitizer:27 is a facade delegating to the plugin-logging copy, so there is no second implementation to keep in step, and DescribeUriSecretShapesTest exercising the facade covers the delegation.
Userinfo is dropped for the shapes that matter, and it fails safe for the awkward ones: https://user:p@ss@host/x sends parseServer into the registry-based fallback, so the credential is dropped along with the host rather than printed.
boss:url?url=SECRET (opaque, no //) renders boss:// - getRawQuery() is null for an opaque URI, so the suffix goes quiet, but nothing leaks.
The disclosed losses are correctly disclosed: the port, a credential in the path, and the id/action of a successful boss://plugin link (DeepLinkActionRegistryImpl still warns with both on the failure paths, and openPluginPanel:647 still logs panelId).
The auth diagnostics loss from "Processing deep link" is small: maskUriParams already redacted token, access_token and code, origin is still logged beside the shape, and fix(auth): keep credentials out of authentication logs #567 owns that area.
The three-file scope, and the reasons for it (fix(auth): keep credentials out of authentication logs #567, fix: route external URLs to focused window #671, fix(auth): bind the passkey browser view to its composition #639), still read correctly against the current tree.

Summary: findings 1-4 from the earlier pass are properly repaired and the conversion is more complete than described. I would fix the registry-based-authority host loss (finding 1) before merge, since it removes the host from a line whose stated purpose is to keep it and regresses "Cookie rejected" relative to maskUriParams; finding 2 is a one-liner in the same function. Finding 3 is a recommendation, not a blocker. No changes pushed - review feedback only. Integration repairs from the earlier pass remain attributed to @kshivang.
· branch fix/log-browser-url-query

kshivang · 2026-09-15T11:02:00Z

@ninja-lifts The current Claude review confirms your earlier four findings and rebase are properly addressed. I reproduced the two new diagnostic cases against the real Kotlin implementation: the added tests failed for registry-based authorities (underscored/Unicode hosts) and empty ?/# references.

Local reviewer commit 74ae3621b fixes those two cases, preserves raw authority encoding while stripping everything through the last @, and documents the source guard's exception/prefix/boolean-key limits. All 95 focused production sanitizer/facade/guard tests pass after the repair. The optional registry-authority fallback retains its port; ordinary server-based URI rendering remains unchanged. The original opaque URI rendering stays pinned rather than adding an unrelated format change. Promoting a running-host log-capture test remains useful optional coverage, not a demonstrated current leak.

The repair is local and awaiting reviewer-side authorization to push to your fork, so published head 840b940 remains unchanged. Its ten hosted checks are green, but they do not validate unpublished 74ae3621b. A shared-lock quality run for the repair is queued. No second Claude loop requested. Your implementation and follow-ups retain their attribution and original rating; these small fixes are reviewer work.

This is an authorization/validation checkpoint. No merge is attempted while the repair remains unpublished. Manual browser/OS/deep-link canary checks are still unperformed.

kshivang · 2026-09-15T22:12:18Z

@ninja-lifts Your browser/deep-link redaction and earlier review follow-ups remain valuable and correctly scoped. I refreshed the PR today: published head is still 840b940, all ten checks are green, and the completed Claude review is unchanged.

The published code still reproduces the two diagnostic cases from that review: underscored/Unicode authority names disappear from the description (including Cookie rejected), and a lone ? or # becomes an empty description. The local reviewer fix 74ae3621b has two red-first regressions and 95 passing focused Kotlin tests, but its push remains explicitly blocked by reviewer-side external-fork authorization. I have not retried or bypassed that rejection.

I am marking this draft as a concrete code-review handoff for the still-unrepaired published behavior, not because of CI and not as a penalty for the authorization block. To return it to ready, publish a fix that preserves those hostnames while removing userinfo and gives empty references an explicit description, retain the existing query/fragment and scheme-less tests, and run focused logging tests plus applicable ktlint/detekt and final-head CI. The already-tested local fix remains available if its exact push is authorized. No new feature or broad URL parser rewrite is requested.

Your original 9/10 rating and all contributor credit remain unchanged. Reviewer repairs are separately attributed. Manual browser/deep-link synthetic-canary checks remain unperformed.

nikhil-jangir-iitk · 2026-09-16T00:39:46Z

@kshivang Published the fix at 565341a2d. It covers both diagnostic cases you named, keeps the existing tests, and does not touch the URL parser beyond the one branch that was wrong.

Also rebased onto dev b915182bc. dev had been force-rewritten again (the #713 integration collapsed the individual PR commits, so cf21742df is no longer an ancestor), which is why this PR was showing 86 files. git range-diff reports both earlier patches identical (=); the PR is 7 files now.

Case 1: the authority disappeared

describedBase read URI.getHost() alone, and that is null whenever the authority is not a legal RFC 2396 hostname or IP literal. Measured on the previous head, JDK 17:

Input	Description
`https://my_host.example.com/a?code=x`	`https:///a (with query params)`
`http://web_server/status`	`http:///status`
`https://x_y.internal:8443/p#t=1`	`https:///p (with fragment)`
`https://münchen.example.com/a?code=x`	`https:///a (with query params)`
`//my_host.example.com/f?sig=x`	`/f (with query params)`

describedBase now falls back to the raw authority when getHost() is null.

The parser offers no help with userinfo in exactly these cases, which is the part worth checking. For https://x-access-token:SECRET@my_host.example.com/o/r.git, JDK 17 reports host=null, getRawUserInfo()=null and getPort()=-1, while getRawAuthority() is x-access-token:SECRET@my_host.example.com. So naming the authority means reading that raw string, and the credential is inside it. The previous head is safe here only because it discards the authority entirely.

hostFromRawAuthority therefore splits it explicitly:

everything up to the last @ is userinfo and is dropped. A private HTTPS clone URL is https://x-access-token:<token>@host/..., and splitting on the first @ would keep the tail of a password containing one;
a trailing :<digits> is a port and is dropped. Requiring digits keeps a colon that is not a port (x_y.internal:80a) rather than cutting the name at it;
an authority that is only userinfo (//user:pass@/a) yields null, so the scheme-less branch does not emit a bare //.

The fallback runs only when getHost() is null, so every URL that parses today is described exactly as before, the dropped port included. That is deliberate: it is what keeps https://example.com:8443/a/b?token=S#state=S reading https://example.com/a/b (with query and fragment), which DescribeUriSecretShapesTest pins.

An IPv6 literal is not a case here. [::1], [::1]:8080 and a zone id such as [fe80::1%25eth0] all parse, so the fallback is never reached, and a malformed one such as [::1 throws out of URI before it. Measured rather than reasoned, because my first version of the KDoc claimed the digit test protected IPv6 and that was wrong.

Case 2: the empty description

?, # and ?# each have an empty base, an empty query and an empty fragment, so every branch contributed nothing and the line read uri= with no value, which cannot be told from a line that logged nothing. It now reads [empty reference], kept distinct from the existing [empty] that reports an absent or blank input.

Tests

Red first on the unchanged source:

DescribeUriReferenceTest: 7 tests completed, 3 failed
  expected:<https://[my_host.example.com]/a (with query params)> but was:<https://[]/a (with query params)>
  expected:<https://[x_y.internal]/p (with fragment)>            but was:<https://[]/p (with fragment)>
  expected:<[[empty reference]]>                                 but was:<[]>

Green on the final code:

Suite	Result
`:plugin-platform:plugin-logging:desktopTest` (whole module, includes #640's `UriUserInfoTest` and `UrlUserInfoInTextTest`)	56 tests, 0 failures, 0 errors, 0 skipped
`:composeApp:desktopTest` for `utils.logging.*` and `GitCloneProgressLogTest`	113 tests, 0 failures, 0 errors, 0 skipped

The userinfo guard is asserted in DescribeUriSecretShapesTest beside the existing userinfo is dropped case, since naming the authority is what makes that credential reachable in the first place. Every query/fragment and scheme-less assertion is retained unchanged.

Mutations on the final code

#	Mutation	Result
1	no raw-authority fallback	CAUGHT 3/56
2	userinfo not stripped	CAUGHT 2/56
3	split on the first `@` not the last	CAUGHT 1/56
4	port not stripped	CAUGHT 1/56
5	digit guard dropped from the port test	CAUGHT 1/56
6	empty host not mapped to null	CAUGHT 1/56
7	no explicit empty-reference marker	CAUGHT 1/56

Survivors: 0. Mutations 5 and 6 exist because two guards in the first version of the fix were covered by no test; both shapes were probed and found reachable rather than assumed unreachable, then pinned.

Lint

:plugin-platform:plugin-logging:ktlintCheck and :composeApp:ktlintDesktopTestSourceSetCheck clean with --rerun. :plugin-platform:plugin-logging:detekt and :composeApp:detekt, the tasks build.yml runs, clean.

One note so it is not mistaken for a regression: the type-resolution variant detektDesktopMain, which CI does not run, fails on BossLogger.kt:139 InjectDispatcher. That file is not touched by this branch and the failure reproduces with these changes stashed.

Not covered

No live BOSS run. This is a pure string function; the manual browser and deep-link synthetic-canary checks you listed remain unperformed.
A credential in the URL path is still described verbatim, unchanged and already recorded in DescribeUriSecretShapesTest's KDoc.
An opaque URI such as about:blank still reads about://, unchanged and already pinned.
This names a host, it does not validate one. An authority reaching the fallback is by definition one the parser rejected, so the result is the text between the delimiters. Percent-encoded labels and multi-level public suffixes are not interpreted.

Final-head CI is running. Happy to adjust the [empty reference] wording if you would rather it matched another placeholder in that file.

nikhil-jangir-iitk · 2026-09-16T00:51:47Z

Final-head CI is green on 565341a2d: 10/10 checks pass, code-quality (detekt + ktlint) included. That covers the four conditions in your handoff, so this is ready for your review whenever it suits you.

kshivang · 2026-09-16T01:22:51Z

@ninja-lifts Thank you for publishing follow-up 565341a and documenting the raw-authority/userinfo handling, empty-reference case, rebase and final-head validation. This supersedes the old handoff's statement that those repairs were unpublished; please do not repeat that work or push the older local reviewer commit.

For the next review, verify the actual follow-up diff and retained userinfo/query/fragment regressions, refresh current dev integration and exact-head CI, then restore ready if the review gates are satisfied. Your reported 56 module tests, 113 targeted host tests and ten green checks should be checked against that head rather than treating the old 840b940 findings as still unresolved. This final handoff pass has not independently reviewed or executed the new implementation, so it does not claim those new results as reviewer-run validation.

Per the user's request to finish with stalled-work draft handoffs, I am leaving the existing draft state intact. No additional code change is requested here. Your contribution/follow-up credit and prior 9/10 rating are preserved; the earlier reviewer publishing restriction was infrastructure, not poor quality. Manual live-browser/deep-link canary checks remain the documented limit.

nikhil-jangir-iitk · 2026-09-17T00:40:17Z

@kshivang Thank you for the merges tonight. This is the last one of mine open, so here is the current state against the gate you set for the next pass, to save you gathering it.

Your gate	State now
verify the actual follow-up diff	published head `565341a2d`, 7 files
retained userinfo / query / fragment regressions	retained and asserted: `DescribeUriSecretShapesTest` keeps #640's `userinfo is dropped` and adds the unparsed-authority case; every query/fragment and scheme-less assertion in `DescribeUriReferenceTest` is unchanged
refresh current dev integration	`git merge-tree` against dev `f240b049c` is clean; GitHub reports MERGEABLE / CLEAN
exact-head CI	ten checks green on `565341a2d`

Measured on that head: 56 plugin-logging tests and 113 targeted composeApp logging tests, zero failures, and 7 mutations with no survivors. ktlintCheck and the detekt tasks build.yml runs are clean. As recorded before, detektDesktopMain fails on BossLogger.kt:139, which this branch does not touch and which reproduces with the changes stashed.

No code change is requested and I have not pushed anything since that head, so the validation above still describes the exact commit. Entirely your call whether the review gates are satisfied; if you would rather it stay draft, that is fine and I will leave it alone.

kshivang · 2026-09-18T02:06:53Z

@claude Please review current head b291fbe against current dev. Prior findings were addressed across rebases; focus only on remaining correctness/security issues in URL shape logging, raw-authority/userinfo handling, and the retained regression tests. Distinguish current-head findings from obsolete earlier heads.

claude · 2026-09-18T02:07:25Z

Claude finished @kshivang's task in 6m 28s —— View job

Review: current head `b291fbe` vs `dev`

Fetch dev, get the reviewable diff at current head
Review describeUri / describedBase / raw-authority + userinfo handling
Review the three production log sites
Review the retained regression tests and the guard scan
Separate current-head findings from obsolete earlier-head findings

Verdict

Current head is b291fbe, three commits (cdd5f8e, 1ef4328, b291fbe) on base 571e3df. dev is exactly one commit ahead of that base (a9c2b65, an unrelated WorkspaceMcpToolProviderTest change), so integration is clean by construction, and there is no inherited trackpad history. Diff is 7 files, +508/-13.

Every finding from the earlier heads is genuinely closed (table below), including the two the last pass blocked on. The raw-authority work in b291fbe is the right shape and the userinfo split is correct for the cases I could reason through, including the awkward ones.

One new finding I would fix before merge, and it is specific to this head rather than carried over: describedBase appends parsed.path, which java.net.URI returns percent-decoded, so a %0A in a URL path reaches the log line as a real newline. That is a log-injection channel that maskUriParams did not have, into exactly the capture this PR's own KDoc says plugins and MCP clients read. Two smaller items after it, both carried over and neither blocking.

I could not execute anything: the sandbox declined git fetch, java and gh, and the checkout is shallow (no merge base with origin/dev). So the analysis is static reading plus java.net.URI semantics, and CI status at this head is unverified by me. Where execution matters I give the one-line test that settles it.

Obsolete: prior findings, confirmed closed at this head

Earlier finding	State at `b291fbe`
whole link logged upstream through `maskUriParams`	Closed. I grepped the three guarded files for `"…url\|uri\|link\|href" to` : 11 entries, all `LogSanitizer.describeUri(...)`, none on `maskUriParams`.
`null://localhost` from the CLI site	Closed in `describedBase` (`LogSanitizer.kt:321-326`), pinned in both modules.
scan holes (comments, character literals)	Closed. `endOfCommentOrChar` (`:179-192`) handles `//`, `/* */`, `'x'` and `'\''`, and `inString -> Unit` correctly stops a comment being detected inside a string.
registry-based authority dropped the host	Closed by `hostFromRawAuthority` (`:357-369`).
`describeUri("?")` returned the empty string	Closed: `[empty reference]`, distinct from `[empty]`, pinned for `?`, `#`, `?#`.
duplicated trackpad commits	Gone.

The three new/changed test classes are accurate against the implementation as written; repoRoot() is the shared testsupport helper, not a fourth copy.

1. `describedBase` appends the decoded path, so a `%0A` in a URL path forges a log line

// LogSanitizer.kt:321-326
private fun describedBase(parsed: URI): String {
    val scheme = parsed.scheme?.let { "$it://" }
    val host = parsed.host ?: hostFromRawAuthority(parsed.rawAuthority)
    val authority = host?.let { if (scheme == null) "//$it" else it }
    return scheme.orEmpty() + authority.orEmpty() + parsed.path.orEmpty()
}

URI.getPath() returns the decoded path component; getRawPath() is the undecoded one. %0A is a legal escaped octet, so URI("https://example.com/a%0A…") constructs fine and getPath() gives /a + a literal newline.

That value goes straight into the data map, and BossLogger appends the map verbatim and terminates each entry with \n (BossLogger.kt:513-533). So:

Input reaching a guarded site	Logged value
`https://example.com/a%0A2026-09-18%20[INFO%20]%20[AUTH]%20forged`	two lines, the second indistinguishable from a real entry
`https://x.example/a%3Fcode%3DSECRET`	`https://x.example/a?code=SECRET` - reads as a query `describeUri` claims to have dropped, and the value is printed in full

This is a new channel, not a pre-existing one: maskUriParams operates on the raw input string and never decodes (maskParamsInSegment, :228-258), and the two BrowserServiceImpl sites previously logged config.url raw, which is likewise still-encoded. describeUri had no production callers before this PR, so the decode was latent until these three files were pointed at it.

Reachability is the same set this PR is about: any program can ask the OS to open boss://x/%0A…, and a page can navigate a tab to a path with an encoded control character before "Browser created via BrowserService" logs it at INFO. The consumer is the Console capture that console_tail / console_search serve, i.e. the exact audience the guard's KDoc names.

The fix is one word, parsed.rawPath, and it also removes an inconsistency the new code introduced: the authority now comes from rawAuthority (undecoded) while the path comes from path (decoded), so one description mixes both conventions. Every current pin is escape-free, so switching keeps all of them green; the two worth adding are a %0A path and a %3F path.

Fix this →

2. The guard's KDoc limits list is still missing three items

BrowserUrlLogConventionTest's KDoc (:34-41) lists three limits: receiver named logger, key must end in url/uri/link/href, and a raw string containing a single ". Three more exist in the code and are not there. These were raised on the earlier head and documented in the reviewer's local commit that was never pushed, so they are unrepaired at this head rather than re-litigated:

The acceptance check is a prefix match. value.startsWith("LogSanitizer.describeUri(") at :143 accepts LogSanitizer.describeUri(url) + "?" + rawQuery, and conversely flags a bare describeUri(uri) after a member import. Fail-safe in the direction that matters, but worth stating.
booleanKey exempts on the key's name, not its value. "hasUrl" to url passes silently (:132, :143). No such site exists today; it is a hole by construction.
error = e is invisible to the scan, and two such lines sit inside guarded files: DeepLinkHandler.kt:816 "Error decoding URL", error = e (whose input is a deep-link parameter) and BrowserServiceImpl.kt:674 "Failed to create browser", error = e. Neither is a leak today, but "guarded file" should not read as "every channel in this file" - BossLogger appends error.message verbatim too.

Fix this →

3. Coverage: still only a text scan (recommendation, not a blocker)

Unchanged from the earlier pass and unchanged in my assessment. The seven-canary evidence through the real processDeepLink / processCommandLineArgs stayed throwaway, so nothing in the repo asserts the outcome. The three refactors most likely to reintroduce this - a key renamed to "target", the line moved to a receiver not called logger, the URL folded into the message string - are all invisible to the scan and all caught at once by one capture test. Also note finding 1 is a case a canary test would not have caught either, because it is the sanitizer itself, not a missing call.

Fix this →

4. Nits, none blocking

An authority that is only a port keeps the colon. hostFromRawAuthority(":8080") takes the portSeparator > 0 else-branch and returns ":8080" as the host. The > 0 guard is there to avoid an empty host, which is right, but the result is inconsistent with the //user:pass@/a decision one bullet above it in the same KDoc, which deliberately returns null rather than a bare marker. One ?: would make the two agree.
describeUri's summary line is still stale. LogSanitizer.kt:277 says "Returns the scheme and host only for auth URIs", written when the function had zero callers. It has 11 now, across browser tabs, cookies, the CLI and every incoming deep link. The paragraphs added below it are accurate; the one-line summary above them is not.
A hierarchical URI with no authority gains one. file:/tmp/x renders file:///tmp/x. Harmless (the two forms are equivalent) and pre-existing, and it is the same interpolation shape as the already-pinned about://.

What I checked and had no objection to

The userinfo split is right, including the cases the parser gives no help with. substringAfterLast('@') is correct per RFC (the last @ is the delimiter), a percent-encoded %40 inside userinfo stays encoded in rawAuthority so it cannot be mistaken for the delimiter, and rawAuthority cannot contain /, ? or #. https://user@name:SECRET@web_server/a -> https://web_server/a is the right answer and is pinned in both test classes.
The digits-only port test is safe for IPv6 even though it is never reached. [::1] -> last : is followed by 1], not all digits, so the literal survives whole; [::1]:8080 -> the trailing 8080 is digits and the bracketed literal survives. So the KDoc's "never reached" claim and the guard are independently correct, which is the better of the two ways to be right.
afterUserInfo = "host:" (trailing colon, empty port) takes the port branch, because all {} on an empty sequence is true, and yields host. Correct, and quietly non-obvious.
Scheme-less references that look like credentials collapse safely. user:pass@host/path parses as an opaque URI with scheme user, so it renders user:// and the password is dropped - the opaque collapse this PR discloses as a detail loss is a security win here.
hasQuery / hasFragment read the raw components, so an encoded value cannot make a present query look absent.
Single implementation: composeApp's LogSanitizer:29 is a one-line delegate, so DescribeUriSecretShapesTest exercising the facade covers the delegation and there is no second copy to keep in step.
The parser in the scan is sound where it claims to be: inString -> Unit is ordered before the comment/char branch so a // inside a string literal is not treated as a comment, valueExpression checks depth == 0 before decrementing so a nested ) does not terminate early, and an unbalanced call fails loudly rather than silently truncating.
The three-file scope and its reasons (fix(auth): keep credentials out of authentication logs #567, fix: route external URLs to focused window #671, fix(auth): bind the passkey browser view to its composition #639) still read correctly against the current tree, and the disclosed losses (the port, a credential in the path, the id/action of a successful boss://plugin link) are correctly disclosed.

Summary: prior findings are all closed and the raw-authority/userinfo work is correct. The one thing I would fix before merge is finding 1, the decoded path - it is new at this head, it is a log-injection channel maskUriParams did not have, and rawPath closes it without touching a single existing pin. Findings 2-4 are documentation and coverage. No changes pushed; this is review feedback only.
· branch fix/log-browser-url-query

kshivang · 2026-09-18T02:16:19Z

@ninja-lifts Thank you for the careful follow-ups; the fresh Claude pass confirms every earlier finding is closed and the raw-authority/userinfo handling is correct. It found one new current-head blocker: describeUri appends URI.path, which percent-decodes the path. A legal %0A therefore becomes a real newline in the INFO/WARN log capture, and %3Fcode%3D... renders like a query value even though the description claims queries were removed. This is a new log-injection/detail-exposure channel at the call sites this PR activates. The bounded fix is to use rawPath and pin %0A/%3F paths. I prepared that locally as reviewer commit cbda64547, along with accurate guard-limit KDoc, but have not published it while the explicit #876 release/integration gate remains open. Focused current-head sanitizer/facade/guard tests passed before this small repair; final integrated-head validation remains required. I am marking this draft for that concrete code fix, not for inherited Windows CI: the hosted Windows failure is WorkspaceMcpToolProviderTest:447, already fixed on current dev by a9c2b6580, and macOS was fail-fast cancelled. Your original contribution and follow-ups retain the existing 9/10 credit; reviewer repair remains separate.

kshivang · 2026-09-18T02:24:20Z

Integrated onto current remote dev bce812bc1 and published reviewer repair e406579a1. The contributor’s three commits remain separate and intact; the reviewer commit alone changes parsed.path to parsed.rawPath, pins %0A and %3F path escapes, updates the stale describeUri summary, and documents the guard’s known prefix/boolean/error-channel limits. Shared-lock validation on the integrated head passed: full plugin-logging desktop tests, focused composeApp DescribeUri/BrowserUrlLogConvention tests, plugin-logging ktlint/detekt, and composeApp desktop-test ktlint/detekt. No live app/deep-link canary run. The PR is restored ready; fresh exact-head CI is now required. @claude Please review integrated head e406579 against current dev, focusing on the rawPath repair and whether the prior decoded-path blocker is closed. Do not repeat findings already resolved on b291fbe.

claude · 2026-09-18T02:26:38Z

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

Review: integrated head `e406579` vs `dev`

Establish the reviewable diff at e406579
Read the reviewer repair commit in isolation
Verify the decoded-path blocker is closed
Check for new issues introduced by the switch to raw components
Check the KDoc / limits documentation changes

Verdict

The decoded-path blocker is closed, and closed properly rather than narrowed. The repair is 3 files / +8/-2, the production change is one word, and after it there is no decoded component left in the description - it is now built entirely from scheme + rawAuthority + rawPath, which is also the internal consistency the previous head lacked. No new blocker. I have four nits, all documentation-sized, and one carried-over coverage recommendation.

Reviewable diff at this head is 7 files / +515/-14 across four commits on bce812b; the contributor's three are unchanged and e406579 is the only reviewer commit. I did not re-review anything closed at b291fbe.

I could not execute anything again - git fetch, java and gradle were all declined, and the checkout is depth-1 on origin/dev (local origin/dev is 628f152, already past the bce812b this was integrated onto), so I compared against the branch's own base. Everything below is static reading plus java.net.URI parser semantics. CI at this head (run 35299175330) is queued, so the exact-head result is still unverified by me as well as by you.

The blocker: closed

// LogSanitizer.kt:325
return scheme.orEmpty() + authority.orEmpty() + parsed.rawPath.orEmpty()

I checked the chain rather than accepting "rawPath is the undecoded one", because rawPath only helps if a raw newline cannot reach the path by some other door:

URI(String) → parseHierarchical → checkChars(..., L_PATH, H_PATH, "Illegal character in path"), and checkChars is scan + a throw on the first unmatched character.
scan accepts a character either by mask or via scanEscape. For c <= 128 scanEscape accepts only a well-formed %XX pair; the unescaped-non-ASCII branch is guarded by c > 128 && !Character.isSpaceChar(c) && !Character.isISOControl(c).
So every line terminator is rejected before describeUri ever sees a URI: \n/\r are < 128 and not in L_PATH; U+0085 NEL is isISOControl; U+2028/U+2029 are isSpaceChar. All three end in [uri-parse-error].
The only surviving route for a newline was therefore an escaped %0A decoded on the way out, which is exactly what getPath() did and getRawPath() does not.

That makes the fix complete for the channel, not merely narrower: rawPath is the whole of it, and the %3F half of the finding goes with it (…/a%3Fcode%3Dsecret now reads as an escaped path rather than as a query the same line claims to have dropped, so the description no longer contradicts itself).

The same reasoning covers the authority, which 5338913 had already switched to rawAuthority, so the description is now raw end to end. scheme cannot carry an escape (parseScheme is alphanum plus +-.), and rawQuery/rawFragment are only read for isNullOrBlank, so an encoded value cannot make a present query look absent.

Every pre-existing pin stays green by construction: I grepped all describeUri assertions in both modules (LogSanitizerTest:346-370, DescribeUriReferenceTest, DescribeUriSecretShapesTest) and every input is escape-free, so path and rawPath are the same string for all of them. The two new pins are the only ones that can tell the change apart, and they are the right two. unsanitizedUrlEntries is untouched, and all 11 url-keyed log entries in the three guarded files still route through describeUri (grepped, not assumed) - maskUriParams survives in those files only inside two comments.

The three KDoc limits added to BrowserUrlLogConventionTest:40-42 are the three I asked for, worded accurately, including the direction the prefix match fails in.

Prior findings (dispositions only, not re-reviewed)

Finding	State at `e406579`
decoded `parsed.path` (the `b291fbe` blocker)	Closed - `rawPath`, pinned for `%0A` and `%3F`
guard KDoc missing prefix / `booleanKey` / `error = e` limits	Closed - `BrowserUrlLogConventionTest:40-42`
stale `describeUri` summary line	Closed - `LogSanitizer.kt:277`
everything closed at or before `b291fbe` (upstream `maskUriParams`, `null://`, scan holes, registry authority, empty reference, trackpad history)	unchanged, not re-examined

Nits, none blocking

1. The summary this commit rewrote now overstates the port. LogSanitizer.kt:277 and :286 both say the port is always dropped, but hostFromRawAuthority requires portSeparator > 0 (:363) specifically to avoid an empty host - so an authority that is only a port keeps it: describeUri("https://:8080/a") returns https://:8080/a. Harmless as output, and the > 0 guard is the right call, but it is now the one case the text does not cover, and it reads inconsistently beside the //user:pass@/a decision one bullet above it, which deliberately returns null rather than a bare marker. Either a ?: there or a clause in the sentence.

Fix this →

2. The weaker cousin of the channel just closed, worth one line in the limits rather than code. A newline is now impossible, but L_PATH/L_REG_NAME admit ,, =, ;, &, : and + unescaped, and BossLogger renders the data map as {key=value, key=value}. So https://x.example/a,profile=attacker describes as itself and reads inside that one line as if it carried a second map entry. It cannot forge a line, which is the difference that matters and the reason this is a nit and not a repeat of the finding - but the guard's KDoc now claims the Console capture is what these calls protect, and intra-line map confusion is the residue.

3. Preserving escapes costs readability, and that is not in the disclosed losses. DescribeUriSecretShapesTest's "Not covered" paragraph lists the path credential, the port and about://, but not that a legitimately encoded path (/docs/%E6%97%A5%E6%9C%AC%E8%AA%9E) now prints escaped where it used to print readably. It is the right trade and it should be on that list beside the others.

4. Grammar, BrowserUrlLogConventionTest:42: "an URL carried by error = e" → "a URL". The other two bullets added in the same hunk read cleanly.

Carried over: still only a text scan (recommendation)

Unchanged from the last pass and still not a blocker. The seven-canary evidence through the real processDeepLink / processCommandLineArgs remains throwaway, so a key renamed to "target", a receiver not called logger, or a URL folded into the message string are all invisible to the repo's own guard while being the three likeliest ways this regresses. Worth noting against my own recommendation, though: the blocker fixed here was inside the sanitizer, so a canary test would not have caught it either - the two kinds of coverage are complements, and DescribeUriReferenceTest is the one that caught this class.

What else I checked and had no objection to

The repair touches nothing but the one expression, the one summary line, two pins and four KDoc bullets - no behaviour change beyond the escape preservation, so the 5338913 raw-authority/userinfo work I already reviewed is untouched.
rawPath is null for an opaque URI, and .orEmpty() handles it, so about:blank still reads about:// and stays pinned in both modules.
münchen.example.com still reaches hostFromRawAuthority and still renders as itself: the unescaped-non-ASCII branch of scanEscape admits it (c > 128, not a space char, not an ISO control), parseHostname rejects it, so getHost() is null and the pin at DescribeUriReferenceTest:52 holds under rawAuthority.
Single implementation confirmed: composeApp's LogSanitizer:29 is a one-line delegate, so the facade cannot drift and DescribeUriSecretShapesTest exercising it covers the delegation.
The three-file scope and its reasons (fix(auth): keep credentials out of authentication logs #567, fix: route external URLs to focused window #671, fix(auth): bind the passkey browser view to its composition #639) still read correctly against this tree, and check(file.isFile) still catches a rename of any guarded file.

Summary: the rawPath repair closes the blocker cleanly and leaves no decoded component in the description; every prior finding stays closed; all four nits are documentation. My one reservation about calling this done is procedural rather than technical - exact-head CI is still queued and no live app / deep-link canary run exists, both of which are already recorded as limits. No changes pushed; review feedback only. Reviewer repair e406579 is attributed to @kshivang, the three logging commits to @ninja-lifts.
· branch fix/log-browser-url-query

kshivang · 2026-09-18T02:37:46Z

Reintegrated the unchanged three contributor commits plus the reviewed raw-path repair onto post-release dev 628f152dc; new exact head is 8f332d330. Shared-lock validation passed plugin-logging desktop tests, focused composeApp DescribeUri/BrowserUrlLogConvention tests, plugin-logging ktlint/detekt, and composeApp desktop-test ktlint/detekt. @claude Please confirm the prior review remains closed at exact head 8f332d330 after this ancestry-only integration. Fresh hosted CI is required before merge.

claude · 2026-09-18T02:50:25Z

Claude finished @kshivang's task in 2m 21s —— View job

Confirmation: prior review remains closed at exact head `8f332d3`

Confirm ancestry: head, base, commit list
Confirm the reviewable diff is unchanged from e406579
Re-verify the rawPath repair and the rest of describedBase
Re-verify the three production sites and the guard/test pins
Report what is closed and what remains open

Verdict

Confirmed: ancestry-only. Nothing I reviewed at e406579 moved, and every finding stays closed. The one thing I'd flag is procedural and you already named it - hosted CI at this head is queued, not green.

Ancestry

Check	Result
exact head	`8f332d33008f148a7f1d81673e25ec7e5fe1f72d`
`git merge-base origin/dev HEAD`	`628f152dc` - identical to `origin/dev`, so the branch is linear on dev tip and zero commits behind
commits ahead	4, no merge commit
reviewable diff	7 files, +515/-14 - the same shape as `e406579`

8f332d3 Shivang       | fix(logging): preserve escaped path bytes in URI descriptions
441fa5c Nikhil jangir | fix(logging): name the host when java.net.URI will not parse the authority
6b4581a Nikhil jangir | fix(logging): log deep links by shape and describe scheme-less references
dcdd453 Nikhil jangir | fix(logging): keep query and fragment secrets out of browser URL log lines

Attribution survived the reintegration: three contributor commits, one reviewer commit, and 8f332d3 is still 3 files / +8/-2 - byte-for-byte the repair I reviewed, not a re-do. e406579 and b291fbe are not fetchable in this shallow checkout, so I re-derived equivalence from content rather than from a commit-to-commit diff; the line numbers below are the clincher, since they are the same ones I cited last pass.

The repair is intact

What I checked at `e406579`	State at `8f332d3`
`parsed.rawPath` in `describedBase`	`LogSanitizer.kt:325` - unchanged, still the only path read
no decoded component left in the description	confirmed: `scheme` + `rawAuthority`-derived host + `rawPath`; `rawQuery`/`rawFragment` read only for `isNullOrBlank`
`%0A` and `%3F` path pins	`DescribeUriReferenceTest.kt:44-45`, both present
stale `describeUri` summary rewritten	`LogSanitizer.kt:277`
three guard KDoc limits (prefix match, `booleanKey` by name, `error = e`)	present in `BrowserUrlLogConventionTest`'s limits list
raw-authority / userinfo work	`hostFromRawAuthority:357-369` unchanged, `portSeparator > 0` + digits guard at `:363`
`[empty reference]`	`:312`
facade is a one-line delegate	`composeApp/.../LogSanitizer.kt:29`, still `= delegate.describeUri(uri)`

Post-release dev did not add a url-keyed line to a guarded file. I re-grepped rather than assuming: the three files hold 11 "…url|uri|link|href" to entries, and all 11 are LogSanitizer.describeUri(...). maskUriParams survives in those files only inside the two explanatory comments (BrowserServiceImpl.kt:656, DeepLinkHandler.kt:372). So the guard scan should still be green, and there is no new site to sanitize from the #863 merge.

LogSanitizerTest is not in the diff, so the four pre-existing describeUri pins are untouched and remain escape-free, i.e. unaffected by rawPath.

Findings status

Finding	State
decoded `parsed.path` (the `b291fbe` blocker)	closed, unchanged
guard KDoc missing prefix / `booleanKey` / `error = e` limits	closed, unchanged
stale `describeUri` summary	closed, unchanged
everything closed at or before `b291fbe`	unchanged, not re-examined
nit 1 - port-only authority keeps its port (`https://:8080/a` → `https://:8080/a`, because the `portSeparator > 0` guard at `:363` declines it while `:277`/`:286` say the port is always dropped)	still open, still cosmetic
nit 3 - escaped-path readability is not on the disclosed-losses list in `DescribeUriSecretShapesTest`'s KDoc	still open
nit 4 - "an URL carried by `error = e`" → "a URL"	still open
coverage recommendation - the seven canaries through the real `processDeepLink` are still throwaway	still open, still not a blocker

None of those four was requested for this pass and none blocks merge.

Limits of this confirmation

I could not execute anything - ./gradlew, java and git fetch are all declined in this sandbox - so your shared-lock results (plugin-logging desktop tests, the focused composeApp DescribeUri/BrowserUrlLogConvention tests, and both ktlint/detekt pairs) are unverified by me. Hosted CI at this head is run 35300100682, queued. No live app / deep-link canary run exists, which stays the documented limit.

Everything above is static reading of the working tree at 8f332d3.
· branch fix/log-browser-url-query

…lines "Browser created via BrowserService" logged a tab's full URL at INFO on every tab creation, hibernation wake and crash recovery, and "URL command queued" logged every boss://url link the same way. The data map is appended to the line as-is, the line reaches the Console capture that every plugin reads through logDataProvider, and the console plugin serves that capture to MCP clients through console_tail and console_search. An OAuth code, a sign-in token or a presigned signature in the URL went with it. Log LogSanitizer.describeUri (scheme, host and path) at the four sites in BrowserServiceImpl, DeepLinkHandler and CLICommandHandler. maskUriParams redacts by exact parameter name and hid 4 of 14 credential-bearing URL shapes measured; describeUri hid 12, missing only a token carried in the path. BrowserUrlLogConventionTest scans those three files for url-keyed log data without LogSanitizer; DescribeUriSecretShapesTest pins the describeUri output the sites now rely on.

…nces From review of the browser URL logging change: DeepLinkHandler logged every incoming link through maskUriParams, at INFO or WARN, in six places: the three OS open-URI handlers, "Received deep link from command line", "Processing deep link" and the unrouted-host warning. maskUriParams redacts by parameter name, and a link's query is often not a listed name: boss://url?url= carries a whole URL with its own query, boss://terminal?command= a command, and an https link BOSS receives as the default browser any credential under any name. Run through the real processDeepLink and processCommandLineArgs, seven synthetic secrets in those shapes all reached LogDataProvider.logs; after this change none does. The six sites and the "Cookie rejected" line now use describeUri. describeUri interpolated the scheme, so a reference with no scheme read as null://localhost. That is exactly what CLICommandHandler logs for input normalizeAndValidateUrl rejected, and what a boss://url link can carry. It now renders localhost, //host/path, and "(with query params)" without a leading space. BrowserUrlLogConventionTest now accepts only describeUri for a url-keyed value in the guarded files, so switching a site to maskUriParams is caught. Its call parser skips comments and character literals, and its KDoc lists what it cannot see.

…ority describeUri built its description from URI.getHost(), which is null whenever the authority is not a legal RFC 2396 hostname or IP literal. An underscore is ordinary in intranet and container names, and a non-ASCII label is ordinary anywhere, so those URLs were logged with the authority missing entirely: https://my_host.example.com/a?code=x -> https:///a (with query params) http://web_server/status -> http:///status https://muenchen.example.com/a -> https:///a The "Cookie rejected" line in BrowserServiceImpl describes a cookie URL this way, so it named no host at all for exactly the hosts whose names are least guessable. describedBase now falls back to the raw authority when getHost() is null. The parser is no help there: getRawUserInfo() is null and getPort() is -1 in exactly the same cases, while getRawAuthority() still carries the userinfo verbatim, so hostFromRawAuthority splits it by hand and removes both. Everything up to the LAST @ is userinfo, because a private HTTPS clone URL is https://x-access-token:<token>@host/... and splitting on the first @ would keep the tail of a password containing one. A trailing :<digits> is a port; requiring digits keeps a colon that is not a port rather than cutting the name at it. The fallback runs only when getHost() is null, so every URL that parses today is described exactly as before, the dropped port included. A reference that is only delimiters ("?", "#", "?#") has an empty base, an empty query and an empty fragment, so describeUri returned "" and the line read "uri=" with no value, indistinguishable from a line that logged nothing. It now reads [empty reference], distinct from the [empty] that reports an absent input. Tests: 56 in plugin-logging and 113 focused composeApp logging tests, all green; the three new assertions fail first on the unchanged source. The userinfo guard is pinned beside the existing risa-labs-inc#640 assertion, since naming the authority is what makes that secret reachable. 7 mutations on the final code, 0 survivors.

kshivang · 2026-09-18T02:55:43Z

Dev advanced through #821/#826/#834 while hosted jobs queued. Replayed the same three contributor commits and reviewed raw-path repair onto current dev 84407bca0; exact head is now babee6734. The replay was conflict-free and diff --check is clean; the previously completed focused validation applies to the unchanged patch. @claude Please finish confirmation against exact head babee6734; only this head’s hosted CI will be used for merge.

claude · 2026-09-18T02:57:42Z

Claude finished @kshivang's task in 1m 44s —— View job

Confirmation: prior review remains closed at exact head `babee673`

Confirm ancestry: head, base, commit list
Confirm the reviewable diff is unchanged from the reviewed patch
Re-verify the rawPath repair and the rest of describedBase
Re-verify the guarded log sites and test pins
Check whether newer dev (Merge instead of assigning on the recent-browser-pages load #821/Derive the displayed recent-files list outside mutationLock #826/fix(auth): fence user_data.json saves against a logout that interleaves the lock #834) added a url-keyed line to a guarded file
Report hosted CI state at this head

Verdict

Confirmed: ancestry-only, and nothing I reviewed moved. The replay is linear on dev tip, the reviewable diff is the same shape as the head I reviewed, and the rawPath repair is byte-for-byte the one I signed off. Every finding stays closed. The only thing outstanding is the one you named: hosted CI at this head is queued.

Ancestry

Check	Result
exact head	`babee67341ac3f60da4c2fc90197d315b9e9a575`
`git merge-base origin/dev HEAD`	`84407bca0` - identical to `origin/dev`, so the branch is on dev tip and zero commits behind
commits ahead	4, no merge commit
reviewable diff	7 files, +515/-14 - the same shape as `8f332d3` and `e406579`

babee67 Shivang       | fix(logging): preserve escaped path bytes in URI descriptions
012da41 Nikhil jangir | fix(logging): name the host when java.net.URI will not parse the authority
408a9c4 Nikhil jangir | fix(logging): log deep links by shape and describe scheme-less references
cbbea56 Nikhil jangir | fix(logging): keep query and fragment secrets out of browser URL log lines
84407bc (dev) Merge pull request #834 from choksi2212/fix/userdatastorage-races-v2

Attribution survived the replay: three contributor commits, one reviewer commit, and babee67 is still 3 files / +8/-2. I read that commit's full diff rather than trusting the stat - it is the same three hunks (parsed.path → parsed.rawPath, the rewritten summary line, the two path-escape pins, the three KDoc limits), with no additional edit smuggled in by the replay.

The repair is intact

What I verified at `8f332d3`	State at `babee67`
`parsed.rawPath` in `describedBase`	`LogSanitizer.kt:325` - unchanged, still the only path read
no decoded component left in the description	confirmed: `scheme` + `rawAuthority`-derived host + `rawPath`; `rawQuery`/`rawFragment` read only for `isNullOrBlank`
`%0A` and `%3F` path pins	`DescribeUriReferenceTest.kt:44-45`, both present
stale summary rewritten	`LogSanitizer.kt:277` - "scheme, host and path without query, fragment, port or userinfo"
three guard KDoc limits (prefix match, `booleanKey` by name, `error = e`)	`BrowserUrlLogConventionTest.kt:40-42`
raw-authority / userinfo work	`hostFromRawAuthority:357` unchanged, `portSeparator > 0` + digits guard intact
`[empty reference]`	`:312`
facade is a one-line delegate	`composeApp/src/commonMain/.../LogSanitizer.kt:29`, still `= delegate.describeUri(uri)`, not in the diff

Every line number is the one I cited last pass, which is the clincher that the replay did not shift the patch.

Newer dev did not touch this surface

git diff --name-only 628f152dc..origin/dev is five files - UserDataStorage.kt, RecentFilesManager.kt, RecentBrowserPagesManager.kt and two tests - so #821/#826/#834 overlap none of this PR's seven files and none of the three guarded files. Consistent with your conflict-free replay.

I re-grepped rather than assuming: the three guarded files hold 11 "…url|uri|link|href" to entries and all 11 are LogSanitizer.describeUri(...) (CLICommandHandler:236; BrowserServiceImpl:657, :671, :1069; DeepLinkHandler:201, :246, :273, :329, :373, :388, :736). maskUriParams survives in those files only inside the two explanatory comments (BrowserServiceImpl:656, DeepLinkHandler:372). So the guard scan has no new site to catch and should still be green.

Findings status

Finding	State
decoded `parsed.path` (the `b291fbe` blocker)	closed, unchanged
guard KDoc missing prefix / `booleanKey` / `error = e` limits	closed, unchanged
stale `describeUri` summary	closed, unchanged
everything closed at or before `b291fbe` (upstream `maskUriParams`, `null://`, scan holes, registry authority, empty reference, trackpad history)	unchanged, not re-examined
nit 1 - a port-only authority keeps its port (`https://:8080/a`), because the `portSeparator > 0` guard declines it while the KDoc says the port is always dropped	still open, cosmetic
nit 3 - escaped-path readability loss is not on the disclosed-losses list in `DescribeUriSecretShapesTest`'s KDoc	still open, cosmetic
nit 4 - "an URL carried by `error = e`" → "a URL" (`BrowserUrlLogConventionTest:42`)	still open, cosmetic
coverage recommendation - the seven canaries through the real `processDeepLink` are still throwaway	still open, not a blocker

None of those four blocks merge, and none was requested for this pass.

Limits of this confirmation

Static verification only: this is a shallow checkout and ./gradlew/java are not usable here, so the focused validation you carried forward (plugin-logging desktop tests, the DescribeUri/BrowserUrlLogConvention composeApp tests, both ktlint/detekt pairs) is unverified by me - though it does apply to an unchanged patch, which the commit-content check above supports. Hosted CI at this head is run 35301258274, queued as of writing, so the merge gate you named is not yet satisfied. No live app / deep-link canary run exists, which stays the documented limit.

No changes pushed. Reviewer repair babee67 is attributed to @kshivang, the three logging commits to @ninja-lifts.
· branch fix/log-browser-url-query

…ut of browser URL log lines and redact JWT tokens (risa-labs-inc#691, risa-labs-inc#855)

…lines (#691) * fix(logging): keep query and fragment secrets out of browser URL log lines "Browser created via BrowserService" logged a tab's full URL at INFO on every tab creation, hibernation wake and crash recovery, and "URL command queued" logged every boss://url link the same way. The data map is appended to the line as-is, the line reaches the Console capture that every plugin reads through logDataProvider, and the console plugin serves that capture to MCP clients through console_tail and console_search. An OAuth code, a sign-in token or a presigned signature in the URL went with it. Log LogSanitizer.describeUri (scheme, host and path) at the four sites in BrowserServiceImpl, DeepLinkHandler and CLICommandHandler. maskUriParams redacts by exact parameter name and hid 4 of 14 credential-bearing URL shapes measured; describeUri hid 12, missing only a token carried in the path. BrowserUrlLogConventionTest scans those three files for url-keyed log data without LogSanitizer; DescribeUriSecretShapesTest pins the describeUri output the sites now rely on. * fix(logging): log deep links by shape and describe scheme-less references From review of the browser URL logging change: DeepLinkHandler logged every incoming link through maskUriParams, at INFO or WARN, in six places: the three OS open-URI handlers, "Received deep link from command line", "Processing deep link" and the unrouted-host warning. maskUriParams redacts by parameter name, and a link's query is often not a listed name: boss://url?url= carries a whole URL with its own query, boss://terminal?command= a command, and an https link BOSS receives as the default browser any credential under any name. Run through the real processDeepLink and processCommandLineArgs, seven synthetic secrets in those shapes all reached LogDataProvider.logs; after this change none does. The six sites and the "Cookie rejected" line now use describeUri. describeUri interpolated the scheme, so a reference with no scheme read as null://localhost. That is exactly what CLICommandHandler logs for input normalizeAndValidateUrl rejected, and what a boss://url link can carry. It now renders localhost, //host/path, and "(with query params)" without a leading space. BrowserUrlLogConventionTest now accepts only describeUri for a url-keyed value in the guarded files, so switching a site to maskUriParams is caught. Its call parser skips comments and character literals, and its KDoc lists what it cannot see. * fix(logging): name the host when java.net.URI will not parse the authority describeUri built its description from URI.getHost(), which is null whenever the authority is not a legal RFC 2396 hostname or IP literal. An underscore is ordinary in intranet and container names, and a non-ASCII label is ordinary anywhere, so those URLs were logged with the authority missing entirely: https://my_host.example.com/a?code=x -> https:///a (with query params) http://web_server/status -> http:///status https://muenchen.example.com/a -> https:///a The "Cookie rejected" line in BrowserServiceImpl describes a cookie URL this way, so it named no host at all for exactly the hosts whose names are least guessable. describedBase now falls back to the raw authority when getHost() is null. The parser is no help there: getRawUserInfo() is null and getPort() is -1 in exactly the same cases, while getRawAuthority() still carries the userinfo verbatim, so hostFromRawAuthority splits it by hand and removes both. Everything up to the LAST @ is userinfo, because a private HTTPS clone URL is https://x-access-token:<token>@host/... and splitting on the first @ would keep the tail of a password containing one. A trailing :<digits> is a port; requiring digits keeps a colon that is not a port rather than cutting the name at it. The fallback runs only when getHost() is null, so every URL that parses today is described exactly as before, the dropped port included. A reference that is only delimiters ("?", "#", "?#") has an empty base, an empty query and an empty fragment, so describeUri returned "" and the line read "uri=" with no value, indistinguishable from a line that logged nothing. It now reads [empty reference], distinct from the [empty] that reports an absent input. Tests: 56 in plugin-logging and 113 focused composeApp logging tests, all green; the three new assertions fail first on the unchanged source. The userinfo guard is pinned beside the existing #640 assertion, since naming the authority is what makes that secret reachable. 7 mutations on the final code, 0 survivors. * fix(logging): preserve escaped path bytes in URI descriptions --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>

@kshivang

…orites (#935) * fix(search): highlight the selected row in the sectioned global search list The sectioned (All-category) view of the double-shift search never lit up the selected row. SearchResultsList advanced a loop-mutated var per section and the items(...) composable lambda read it by reference - the lambda runs only when the LazyColumn composes the slot, after the walk has finished, so the var held the list's total count and no row's computed global index could ever equal selectedIndex. The flat single-category view passes its index straight in and always highlighted, which is why the bug survived: only the default view, the one double-shift opens, was affected. Compute each section's start up front in sectionStartsFor (a pure function mirroring the drawing loop's walk, in the style of listItemIndexFor) and capture that value by value into the row lambda. Selection math becomes sectionStart + localIndex == selectedIndex, which is also what Enter already indexes, so the highlighted row is now the row Enter opens. Five tests pin the walk: first-occurrence starts, bijection of section ranges over the result indices, absent categories, and the empty list. * refactor(search): address review feedback on section starts Address the Claude diff review on the global-search highlight fix: - sectionStartsFor now takes the drawing loop's own grouping instead of re-grouping the list, so there is one source of truth and getValue is safe by construction - pin the getFilteredResults -> sectionStartsFor coupling with a test that feeds the real producer's output through the walk - pin the walk's input contract (enum-order walk over counts) with an explicit interleaved-input test - add a consistency test: section-derived row positions agree with listItemIndexFor for every row - drop the overstated KDoc claim; state the two tests that pin the coupling * style(search): satisfy detekt MaxLineLength on the grouped helper detekt caps code lines at 120 (ktlint allows 140), and ktlint wants the helper as a one-line expression body - the annotated signature exceeded 120. Simplify instead of fighting both linters: - the flat branch never reads groupedResults (it draws results directly), so its map was dead weight - emptyMap() and a non-null-key map type - sectionStartsFor takes Map<SearchCategory, List<SearchResult>> again - the test helper is a plain expression body with no type annotation * Merge instead of assigning on the recent-browser-pages load loadAsync assigned data.pages straight into _recentPages, and bootstrapFromBrowserHistory did the same with its converted entries. init launches that load and recordPageVisit can land while the read is in flight - which is exactly why recordPageVisit uses update{} - so the page the user had just visited was dropped from the list and from the save scheduled for it. The bootstrap path is worse: it writes immediately, so the assignment persisted over the racing visit. This is the defect #795 fixed for the sibling RecentFilesManager, which was modelled on this class for its lock discipline, debounce swap and atomic write. Those three were already right here; this fourth one was not. mergeRecordedPages differs from RecentFilesManager's mergeRecorded because visitCount accumulates: the racing entry is created fresh with a count of 1 because the list was empty, so taking the newer entry wholesale would reset a long-lived page from its real count to 1. Counts are summed, the same way UrlHistoryManager merges duplicates; the newer lastVisited supplies the title, and the favicon falls back to the stored entry when the racing visit has none. Summing is not idempotent, which is safe while loadAsync runs once from init and is documented as a constraint on adding any reload. 7 unit tests for the merge contract. SuggestionDismissalTest and SearchSourceRegistrarTest, which drive this object, still pass 7/7 each. * fix(browser): preserve startup dismissals during load * Derive the displayed recent-files list outside mutationLock setFiles wrote _allFiles and then derived _recentFiles, and #795 put the whole thing inside mutationLock. The derive calls File.exists() once per entry - up to 20 - and on an unmounted or disconnected share, the exact case _allFiles exists for, each can block for seconds. Every recordFileOpen, removeFile and clearAll then queued behind one slow mount. The exists() cost is pre-existing; being inside a critical section is not. Before #795 setFiles was called with no lock held, so a slow share made one mutation slow rather than serialising all of them. setFiles is split: mutationLock now covers only the recorded write, and refreshVisible() does the derive under its own visibilityLock. The two locks are never nested. The recorded list is read inside visibilityLock, so of two concurrent derives the one that finishes last is the one that read the freshest list. The displayed list now lags the recorded one for the length of the derive. That is not new - nothing re-derives between mutations either, so a file deleted outside BOSS already stays visible until the next mutation - the window is just wider. No new tests: the change is about lock scope, which needs a blocking File.exists() to observe. RecentFileVisibilityTest, RecentFilesMergeTest and RecentFilesLoadTest pass 15/15 unchanged. * test(recent-files): order reset with visibility refresh * fix(auth): fence user_data.json saves against a logout that interleaves the lock Follow-up on the merged #795 (issues #762/#764), closing the one review item it did not include: a save that entered before logout but acquires fileLock only after clearUserData ran still recreated user_data.json with the logged-out user's identity - the mutex cannot order a caller that already passed it. saveUserData now captures a clear-generation counter before acquiring the lock and re-checks it inside (delegating to an internal doSaveUserData seam that takes the entry generation, so the regression test drives the capture-clear-acquire interleaving deterministically); clearUserData bumps the counter under the lock. A save entered before logout observes the bump and skips; a save entered after logout has a fresh generation and proceeds normally (pinned by the test). The fence test rides the merged suite's hermetic resetForTesting fixture. 4/4 + detekt + ktlint green in one run. * test(auth): exercise logout fence through public save * fix(persist): pin atomicWriteText permissions to owner-only and migrate stats and project writes (#860) * fix(security): fail closed on permission hardening * fix(render): let plugin quarantine settle before narrowing (#750) * fix(render): let plugin quarantine settle before narrowing * fix(render): bound settling refunds across an incident * test(render): derive escalation probe length from policy --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * fix(persist): flush debounced recent-files and user-data saves on exit (#795 follow-up) * fix(review): acknowledge recent-files manager breadth * style(persist): format flush test chain * test(search): pin the global search service contract * style(search): wrap service test fixtures * style(search): format match fixture helper * style(search): use block body for fixture helper * style(search): extract match fixture constructor * fix(settings): serialize mutations and persist atomically with owner permissions (#871) * fix(security): fail closed on settings permissions * fix(security): redact sensitive JWT tokens in Supabase realtime log lines (#855) (#870) * fix(security): redact sensitive JWT tokens in Supabase realtime log lines (#855) * test(logging): isolate Supabase logger state --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * test(mcp): pin the risk evaluator contract for tool approval * test(mcp): make risk contract claims explicit * fix(logging): keep query and fragment secrets out of browser URL log lines (#691) * fix(logging): keep query and fragment secrets out of browser URL log lines "Browser created via BrowserService" logged a tab's full URL at INFO on every tab creation, hibernation wake and crash recovery, and "URL command queued" logged every boss://url link the same way. The data map is appended to the line as-is, the line reaches the Console capture that every plugin reads through logDataProvider, and the console plugin serves that capture to MCP clients through console_tail and console_search. An OAuth code, a sign-in token or a presigned signature in the URL went with it. Log LogSanitizer.describeUri (scheme, host and path) at the four sites in BrowserServiceImpl, DeepLinkHandler and CLICommandHandler. maskUriParams redacts by exact parameter name and hid 4 of 14 credential-bearing URL shapes measured; describeUri hid 12, missing only a token carried in the path. BrowserUrlLogConventionTest scans those three files for url-keyed log data without LogSanitizer; DescribeUriSecretShapesTest pins the describeUri output the sites now rely on. * fix(logging): log deep links by shape and describe scheme-less references From review of the browser URL logging change: DeepLinkHandler logged every incoming link through maskUriParams, at INFO or WARN, in six places: the three OS open-URI handlers, "Received deep link from command line", "Processing deep link" and the unrouted-host warning. maskUriParams redacts by parameter name, and a link's query is often not a listed name: boss://url?url= carries a whole URL with its own query, boss://terminal?command= a command, and an https link BOSS receives as the default browser any credential under any name. Run through the real processDeepLink and processCommandLineArgs, seven synthetic secrets in those shapes all reached LogDataProvider.logs; after this change none does. The six sites and the "Cookie rejected" line now use describeUri. describeUri interpolated the scheme, so a reference with no scheme read as null://localhost. That is exactly what CLICommandHandler logs for input normalizeAndValidateUrl rejected, and what a boss://url link can carry. It now renders localhost, //host/path, and "(with query params)" without a leading space. BrowserUrlLogConventionTest now accepts only describeUri for a url-keyed value in the guarded files, so switching a site to maskUriParams is caught. Its call parser skips comments and character literals, and its KDoc lists what it cannot see. * fix(logging): name the host when java.net.URI will not parse the authority describeUri built its description from URI.getHost(), which is null whenever the authority is not a legal RFC 2396 hostname or IP literal. An underscore is ordinary in intranet and container names, and a non-ASCII label is ordinary anywhere, so those URLs were logged with the authority missing entirely: https://my_host.example.com/a?code=x -> https:///a (with query params) http://web_server/status -> http:///status https://muenchen.example.com/a -> https:///a The "Cookie rejected" line in BrowserServiceImpl describes a cookie URL this way, so it named no host at all for exactly the hosts whose names are least guessable. describedBase now falls back to the raw authority when getHost() is null. The parser is no help there: getRawUserInfo() is null and getPort() is -1 in exactly the same cases, while getRawAuthority() still carries the userinfo verbatim, so hostFromRawAuthority splits it by hand and removes both. Everything up to the LAST @ is userinfo, because a private HTTPS clone URL is https://x-access-token:<token>@host/... and splitting on the first @ would keep the tail of a password containing one. A trailing :<digits> is a port; requiring digits keeps a colon that is not a port rather than cutting the name at it. The fallback runs only when getHost() is null, so every URL that parses today is described exactly as before, the dropped port included. A reference that is only delimiters ("?", "#", "?#") has an empty base, an empty query and an empty fragment, so describeUri returned "" and the line read "uri=" with no value, indistinguishable from a line that logged nothing. It now reads [empty reference], distinct from the [empty] that reports an absent input. Tests: 56 in plugin-logging and 113 focused composeApp logging tests, all green; the three new assertions fail first on the unchanged source. The userinfo guard is pinned beside the existing #640 assertion, since naming the authority is what makes that secret reachable. 7 mutations on the final code, 0 survivors. * fix(logging): preserve escaped path bytes in URI descriptions --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * feat: added MCP operation ledger and CLI * fix(mcp): create the operation ledger with owner-only permissions * fix(mcp): harden ledger verification and persistence * fix(mcp): mask the credential in an Authorization header, not just its label * fix(mcp): absorb repeated auth schemes and reach the quoted value, so a compound or quoted credential cannot survive * fix(mcp): redact arbitrary authorization schemes * fix(mcp): scope session trust to the provider the operator approved (#815) * fix(mcp): scope session trust to the provider the operator approved Session trust was keyed by tool name only, so trusting one provider's tool for the session silently granted the same auto-run to every other provider shipping a same-named tool - an unvetted plugin could inherit an approval the operator never gave it. The engine already treats (toolName, providerId) as a tool's identity everywhere else; session trust was the one surface that forgot. - trustForSession now takes the provider, with no default, so a caller cannot grant name-only trust by accident - policyFor matches a session grant only when the provider in hand is the one that earned it; with no provider it never applies - confirmInvocation had the providerId all along and now passes it through; a grant without a provider runs the one approved call but does not stick - revokeSessionTrust keeps a name-wide mode (null provider) because persisted rules are name-keyed and their resets cannot know a grant's provider; that direction over-removes trust, which fails closed - section writes drop only the (providerId, toolName) pairs they changed, so same-named tools trusted for other providers keep their own grants New McpSessionTrustScopingTest pins the leak: a same-named tool from an untrusted provider keeps prompting. * test(mcp): pin session-trust edge cases for #815 * docs(agents): pin the provider-scoped session-trust contract (#815) * docs(agents): scope section-save trust invalidation to the changed pairs (#815) * style(mcp): remove leading blank line in trust edge cases --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * perf(ipc): Optimize MCP tool schema parsing with string-based cache (#867) * perf(ipc): Cache MCP tool schema parsing to reduce latency * test(ipc): Add regression coverage for MCP schema caching * fix(ipc): bound and verify tool schema cache --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * fix(plugins): log dropped protected dev jars and duplicate jar picks (#781) * fix(plugins): add diagnostic logging for deduplicated and dropped dev jars * style(test): order imports alphabetically for ktlint compliance * refactor(plugin): remove unreachable protected dev penalty branch * fix(plugins): preserve dedupe scan and test isolation --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * test(boss-native-files): skip symlink tests when symlink creation is unavailable (#839) * test(boss-native-files): skip symlink tests when symlink creation is unavailable * test(native-files): keep symlink probe failures visible * style(native-files): order test imports --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * fix(browser): parse the focused field details as JSON, not with a regex The page builds this payload with `JSON.stringify` and `getFormFieldInfoFromJS` read it back with `"key":"([^"]*)"`, once per key. That is not a JSON parser in two ways, and both of them reach a credential: - `[^"]*` stops at the first quote, and `JSON.stringify` writes a quote inside a value as `\"`. So the password `a"b` arrived as `a\` - truncated at the quote, and carrying a backslash nobody typed. - Nothing decoded an escape, so a textarea's newline arrived as the two characters `\n`, and the same for `\\`, `\t` and `\uXXXX`. The parse moves to `formFieldInfoFrom` in its own file so it can be exercised without a live browser frame, the way `cloneProgressLogMessage` is in `DesktopGitService`. `BrowserHandleImpl` keeps the script and the frame call. `Json.Default` is right here and this is not the case AGENTS.md's `supabaseJson` rule is about: that rule exists because `decodeFromString<T>` treats an undeclared column as a hard error. There is no model here - `parseToJsonElement` builds a tree and each key is read off it, so the script's `className`, which nothing reads, is simply not looked at. One deliberate behaviour change: a payload that is not a JSON object now yields null rather than a `FormFieldInfo` whose every field is empty. The old shape claimed to describe a text field it had never read; the caller treats null as "no autofill detail for this menu", which is what it is. The classification `when` is split into `declaredFieldType` and `namedFieldType`. That split is not cosmetic and not free: the block had been inside a function baselined for `CyclomaticComplexMethod` and `LongMethod`, so moving it out lost the suppression. Decomposing by source of evidence - what the element declares outranks what it is named - keeps the branch order exactly as it was and lets both baseline entries be deleted instead of re-baselined. Three tests pin that precedence, which nothing covered before. * test(plugin): pin the artifact cleanup deletion-safety contract (#843) * test(plugin): pin the artifact cleanup deletion-safety contract * test(plugin): make cleanup symlink coverage portable --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * fix(plugin): verify system-plugin GitHub downloads before install * fix(plugin): use the canonical pinned system-plugin repo * test(cli): pin the CLI security validator contract * test(cli): make validator contract assertions honest * test(cli): clarify downstream URL and path contracts * test(run): pin update-payload integrity and crash-write safety for run configs (#824) * test(run): pin concurrent-update and crash-write safety for run configs (#754 follow-up) * style: ktlint fixes for the run-config concurrency tests * test(run): skip unsupported permission failure injection * test(run): make atomic-write observer deterministic * test(run): keep atomic observer within quality limits * test(run): stabilize persisted timestamps * fix(run): preserve timestamps and detect empty writes * test(run): require the reader to sample before writes * test(run): make reader readiness fail fast --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * fix(secrets): stop clients importing ciphertext into rows they can decrypt `authenticated` held table-level INSERT and UPDATE on public.secrets and public.secret_metadata, and the row policies let an owner update their own rows. A signed-in user could therefore PATCH any ciphertext into their own password_encrypted or recovery_codes_encrypted and read it back decrypted through get_user_secrets, since every value is under the one global key: a leaked dump or backup plus any account decrypts every password in it. It is the attack 20260909000000_encrypt_totp.sql already closes for twofa_secret, and it also hands an owned row out as a padding oracle against the unauthenticated CBC ciphertext (#618 defect 3). Replace each client table-level INSERT/UPDATE with a column-level grant of the same privilege on every column except the ciphertext ones, reading the column list from the catalog and never granting a privilege a role did not already hold. Every legitimate writer is a SECURITY DEFINER RPC and is unaffected; no client in this repository writes either column. The new pgTAP suite reproduces the import end to end (it fails on the unpatched schema) and pins the paths that must keep working: create and update through the RPCs, direct edits of ordinary columns, and plaintext TOTP input through its trigger. Full pgTAP suite: 38 files, 900 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(secrets): fail closed on inherited ciphertext grants * fix(updater): verify the chromium engine fallback download against the catalog sha256 (#798 follow-up) * docs(browser): describe fallback checksum provenance * Keep RecentFilesLoadTest teardown off the user's recent-files.json (#817) * Keep RecentFilesLoadTest teardown off the user's recent-files.json resetForTesting reloads after repointing settingsFile, so restoring the real path in teardown reads the developer's own recent-files.json. If that file is denormalised (duplicate paths, over-cap, hand-edited order) the merge differs from the decoded contents, persist is true, and the debounced save lands on the real file well within a full desktopTest run. CI never sees it: a runner has no ~/.boss, so the load early-returns. - resetForTesting takes reload: Boolean = true; teardown passes false. The restore only needs the pointer back in place, and nothing else in the suite drives this object. - saveJob is cancelled before the settingsFile swap rather than after. saveImmediately reads settingsFile without the lock, so a job already past its delay could write the just-cleared list to the new path - which on the restore call is the user's own file. Raised in the automated review on #795 after it merged, alongside the measured ~/.boss writes from other singletons (separate issue). * test: await recent-files reset jobs before path swap --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * fix(security): make /auth/challenge enumeration-safe and rate-limited (#769) * fix(security): make /auth/challenge enumeration-safe and rate-limited POST /passkey/auth/challenge runs with verify_jwt=false and returns three distinguishable outcomes for an email probe: 400 'User not found', 400 'No passkeys found for user', or a 200 whose allowCredentials leaks each stored credential_id to the unauthenticated caller. Migration 20260910000000 already called the same lookup an 'unauthenticated-grade enumeration oracle for the whole user base' when it revoked find_user_by_email's client grants, but this route performs the lookup over HTTP and returns distinguishable errors, so the oracle survived through the surface that migration did not cover. - pre-auth failures (unknown email, no passkeys, lookup error) now all return the same inert challenge: a fresh random value that is never stored - so /auth/complete can never verify against it - with an empty allowCredentials, so the client's credential request fails locally exactly as it would for a genuine no-credentials user. Status, body shape, and content are identical for every email a prober tries; the real allow list is returned only when a passkey row actually exists. - the route is rate-limited with the organisation function's in-memory fixed-window shape (copied, since edge functions share no import path): 20 challenges per client per hour, 429 with the new OpenAPI 429 response documented. Validation: full passkey deno test suite 162 passed / 0 failed; type-check clean. The two enumeration tests now pin the inert response; five new rate-limit tests cover the window, retry-after, reset, and per-client budgets. * fix(security): close the inverted store-failure oracle, bound the limiter, and pin the route contract Review follow-ups on this PR (by @kshivang): - storeChallenge failure previously returned success:false - a state reachable only for an enrolled account (we already passed the passkey lookup), which inverts the oracle exactly when the store hiccups. It now returns the same inert challenge as every other pre-auth failure. - The route's per-client budget moves from 20/hour to 60/hour: the desktop sign-in flow spends up to three challenge calls per successful sign-in, so 20 was under three sign-in attempts. - The limiter's MAX_KEYS memory bound (already ported from the organisation implementation) now has its flood test: a 10,050-key spray is evicted and fresh keys still get fresh windows. - Route-level tests pin what only the route can prove: the 61st probe from one client is a 429 with ZERO Supabase lookups spent on it (limit-before-lookup, proven via the mock's query history), and the serialized responses for unknown-email and no-passkeys are key-for-key identical with empty allow lists and per-request random challenges. - resetRateLimiter exposed so route tests isolate limiter state. The residual enrollment oracle is real and stays: enrolled accounts still receive their real credential IDs in allowCredentials, because legacy (non-discoverable) credentials cannot complete the ceremony without them. Closing it needs a client-side discoverable-credentials migration; this PR narrows the claim to the pre-auth failure states it seals and documents that boundary in the issue. Full passkey deno suite: 165 passed / 0 failed; type-check clean. * fix(security): enforce limiter cap and narrow enumeration claims --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * fix(orchestrator): confine snapshot paths and persist atomically with owner permissions (#873) * fix(orchestrator): reject snapshot symlink escapes * fix(plugin): bound the manifest entry reads before signature verification * style(plugin): fix bounded manifest test formatting * test(plugin): pin the manifest bound and keep the caps in sync Final review asks on #858: - split readBoundedManifest(stream) out of readManifestContent so the byte bound is directly testable without a JAR, and pin it with a fake InputStream that fails the moment a reader asks for more than cap + 1 bytes; an unbounded readText-then-check-length implementation now fails deterministically instead of passing every fixture test - add real zip-bomb fixtures to both bounded-read suites: a plugin.json entry inflating to 1 GiB from a tiny compressed payload, streamed into the JAR, asserted rejected within the byte bounds - pin PluginManifestReader.MAX_MANIFEST_BYTES against DevPluginArtifacts.MAX_MANIFEST_BYTES with an explicit assertEquals in a composeApp desktopTest (the RetiredPluginsTest convention), and document the deliberate null-vs-throw failure modes in the KDoc - narrow the PluginManifestReader KDoc: the bound covers this reader's own manifest read only, not the JDK's JarVerifier, which reads META-INF signature entries unbounded on the first getInputStream * fix(plugin): satisfy detekt on the bounded manifest reads Wrap the oversize fail message in the bounded-read test and suppress TooManyFunctions on PluginManifestReader, whose eleven read/validate steps belong to one fail-closed order. * fix(deep-link): hold an external workspace link's terminal commands for confirmation boss://terminal?command= has consulted DeepLinkOrigin since #60, but boss://workspace?path= reaches the same shell - applying a Space types each terminal tab's initialCommand - and dispatch never passed it an origin. An external workspace link therefore started every command in the file with no prompt. Carry the origin on CLICommand.LoadWorkspace (default EXTERNAL), through the cold-start readiness queue, onto WorkspaceLoadEvent.requiresConfirmation. The window, which is the only place the file is parsed, decides with spaceLoadDisposition: an external load of a Space that carries terminal commands is shown in SpaceLoadApprovalDialog first, and dropped when a command is malformed or the commands cannot all be shown. The operator's own `boss workspace` and Spaces without terminal commands load as before. * fix(deep-link): harden workspace confirmation feedback * fix(cli): reject hidden command formatting characters * fix(cli): reject supplementary format controls * fix(deep-link): complete hidden character filtering * fix(security): close the search_path on the passkey SECURITY DEFINER set (#773) * fix(security): close the search_path on the passkey SECURITY DEFINER set Since 20260802000000 the repo convention for SECURITY DEFINER functions is SET search_path TO '' plus fully-qualified table references - a hardening that migration itself calls a security fix and the Supabase linter enforces. Three passkey lifecycle functions predate it and were never upgraded, so each still resolves its tables through the caller-influenced search_path: - clean_expired_passkey_challenges() - create_mobile_registration_session(text, text, text) - get_session_status(text) Not a live exploit today - turning a mutable search_path into privilege escalation needs CREATE on a searched schema, which the PostGREST roles lack - but a future CREATE grant or restored PUBLIC default would silently convert them into escalation primitives, and the bodies would not error before that day. Same drift class 20260910000000 closed ('the repository was WEAKER than production'). clean_expired_passkey_challenges also still carried EXECUTE grants for anon and authenticated (20251023000014; the 20260908030000 sweep stripped only anon/PUBLIC). Nothing invokes it - the probabilistic trigger inlines its own DELETE - so the grants are dead weight on a SECURITY DEFINER function any self-registered user could call. The migration carries each original body verbatim with only two mechanical changes: the SET search_path TO '' clause and schema qualification of table references (auth.users, public.passkey_challenges, pg_catalog.now()). Signatures, return types, logic and owners unchanged; pgTAP pins the closed form via pg_proc.proconfig and the revoked grants. Validation: fresh local DB applied the full chain through this migration cleanly; supabase test db - new suite 12/0, all other suites unchanged (the only failure is the pre-existing secret_totp_encryption_test fixture, absent on a fresh clone). * test(security): correct the plan count to the 12 assertions that run plan(13) was written before the two grant-keeping assertions were merged into one, so pgTAP aborted the file with 'Bad plan: planned 13, ran 12' despite every assertion passing. plan(12) matches the file; suite now reports ok. * fix(security): close the nested-trigger search_path regression in the passkey hardening Review follow-up (PR review by @kshivang): the hardened create_mobile_registration_session inserts under search_path = '', and passkey_challenges' AFTER INSERT trigger then runs trigger_cleanup_expired_challenges(), which had no local search_path and referenced passkey_challenges unqualified - so the 10% cleanup branch inherited the empty path and aborted registration with relation-not-found. The migration now redefines the trigger chain to be self-contained, in the same shape as open #572 (20260911093022) so whichever lands second is a textual no-op replace of identical semantics: - clean_expired_passkey_challenges(): bounded cleanup (256 rows, FOR UPDATE SKIP LOCKED), SECURITY INVOKER, SET search_path TO ''. Defined here so the delegate resolves whether or not #572 has landed; when it has, this replaces it with the identical body. - trigger_cleanup_expired_challenges(): SECURITY INVOKER delegate with its own empty search_path, public.-qualified, replacing the old unbounded unqualified probabilistic body. pgTAP updates: membership testing ('search_path=""' = any(proconfig)) instead of positional proconfig[1]; pins on the trigger's own empty search_path and INVOKER status; live-path proofs that the cleanup RPC and get_session_status still execute under the closed path; 16/16 ok, full suite 785 tests green. * fix(security): preserve passkey cleanup semantics under closed path * test(security): use the pgTAP pattern assertion * test(security): pin the passkey trigger repair --------- Co-authored-by: Shivang <shivang.iitk@gmail.com> * fix(home): place Home in the Favorites grid (#933) * fix(browser): wire printing and make top-bar zoom badge opt-in * test(spotlight): classify the handled browser print command * fix(mcp): keep ledger telemetry ordered with durable writes * fix(review): handle Windows persistence contention and flush recent pages * fix(test): await recent-page cleanup without locking its file --------- Co-authored-by: Shivanshu <shivanshu@risalabs.ai> Co-authored-by: Aryan Bajaj <aryan.bajaj.met22@itbhu.ac.in> Co-authored-by: choksi2212 <manaschoksiwork@gmail.com> Co-authored-by: Rohith Pariki <rohithpariki@gmail.com> Co-authored-by: Aryan Raj <aryanraj2789@gmail.com> Co-authored-by: Mihir-Rabari <mihirrabari2604@gmail.com> Co-authored-by: RohithPariki <159949499+RohithPariki@users.noreply.github.com> Co-authored-by: Nikhil jangir <nikhil23@iitk.ac.in> Co-authored-by: harshwardhan singh bais <titanx276@gmail.com> Co-authored-by: nitin gupta <nitin_g@me.iitr.ac.in> Co-authored-by: Kanak Khandelwal <152528927+Kanakk23@users.noreply.github.com> Co-authored-by: Rounak Kumar Agarwal <agarwalrounak2005@gmail.com> Co-authored-by: Atharv Gaur <atharvanna71@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: aryanbajaj868 <117334799+aryanbajaj868@users.noreply.github.com> Co-authored-by: Manas Choksi <162600900+choksi2212@users.noreply.github.com> Co-authored-by: Vidhaankhare16 <vidhankhare16@gmail.com>

…lines (risa-labs-inc#691) * fix(logging): keep query and fragment secrets out of browser URL log lines "Browser created via BrowserService" logged a tab's full URL at INFO on every tab creation, hibernation wake and crash recovery, and "URL command queued" logged every boss://url link the same way. The data map is appended to the line as-is, the line reaches the Console capture that every plugin reads through logDataProvider, and the console plugin serves that capture to MCP clients through console_tail and console_search. An OAuth code, a sign-in token or a presigned signature in the URL went with it. Log LogSanitizer.describeUri (scheme, host and path) at the four sites in BrowserServiceImpl, DeepLinkHandler and CLICommandHandler. maskUriParams redacts by exact parameter name and hid 4 of 14 credential-bearing URL shapes measured; describeUri hid 12, missing only a token carried in the path. BrowserUrlLogConventionTest scans those three files for url-keyed log data without LogSanitizer; DescribeUriSecretShapesTest pins the describeUri output the sites now rely on. * fix(logging): log deep links by shape and describe scheme-less references From review of the browser URL logging change: DeepLinkHandler logged every incoming link through maskUriParams, at INFO or WARN, in six places: the three OS open-URI handlers, "Received deep link from command line", "Processing deep link" and the unrouted-host warning. maskUriParams redacts by parameter name, and a link's query is often not a listed name: boss://url?url= carries a whole URL with its own query, boss://terminal?command= a command, and an https link BOSS receives as the default browser any credential under any name. Run through the real processDeepLink and processCommandLineArgs, seven synthetic secrets in those shapes all reached LogDataProvider.logs; after this change none does. The six sites and the "Cookie rejected" line now use describeUri. describeUri interpolated the scheme, so a reference with no scheme read as null://localhost. That is exactly what CLICommandHandler logs for input normalizeAndValidateUrl rejected, and what a boss://url link can carry. It now renders localhost, //host/path, and "(with query params)" without a leading space. BrowserUrlLogConventionTest now accepts only describeUri for a url-keyed value in the guarded files, so switching a site to maskUriParams is caught. Its call parser skips comments and character literals, and its KDoc lists what it cannot see. * fix(logging): name the host when java.net.URI will not parse the authority describeUri built its description from URI.getHost(), which is null whenever the authority is not a legal RFC 2396 hostname or IP literal. An underscore is ordinary in intranet and container names, and a non-ASCII label is ordinary anywhere, so those URLs were logged with the authority missing entirely: https://my_host.example.com/a?code=x -> https:///a (with query params) http://web_server/status -> http:///status https://muenchen.example.com/a -> https:///a The "Cookie rejected" line in BrowserServiceImpl describes a cookie URL this way, so it named no host at all for exactly the hosts whose names are least guessable. describedBase now falls back to the raw authority when getHost() is null. The parser is no help there: getRawUserInfo() is null and getPort() is -1 in exactly the same cases, while getRawAuthority() still carries the userinfo verbatim, so hostFromRawAuthority splits it by hand and removes both. Everything up to the LAST @ is userinfo, because a private HTTPS clone URL is https://x-access-token:<token>@host/... and splitting on the first @ would keep the tail of a password containing one. A trailing :<digits> is a port; requiring digits keeps a colon that is not a port rather than cutting the name at it. The fallback runs only when getHost() is null, so every URL that parses today is described exactly as before, the dropped port included. A reference that is only delimiters ("?", "#", "?#") has an empty base, an empty query and an empty fragment, so describeUri returned "" and the line read "uri=" with no value, indistinguishable from a line that logged nothing. It now reads [empty reference], distinct from the [empty] that reports an absent input. Tests: 56 in plugin-logging and 113 focused composeApp logging tests, all green; the three new assertions fail first on the unchanged source. The userinfo guard is pinned beside the existing risa-labs-inc#640 assertion, since naming the authority is what makes that secret reachable. 7 mutations on the final code, 0 survivors. * fix(logging): preserve escaped path bytes in URI descriptions --------- Co-authored-by: Shivang <shivang.iitk@gmail.com>

nikhil-jangir-iitk requested a review from kshivang as a code owner September 15, 2026 00:04

kshivang force-pushed the dev branch from 15a4ee3 to 6004233 Compare September 15, 2026 01:33

This was referenced Sep 15, 2026

feat(security): project secret scanner (hardcoded-credential detection) #688

Closed

fix(auth): keep credentials out of authentication logs #567

Open

kshivang marked this pull request as draft September 15, 2026 01:43

kshivang force-pushed the dev branch from 5dc8211 to 236937c Compare September 15, 2026 02:43

nikhil-jangir-iitk force-pushed the fix/log-browser-url-query branch from 66fd6f3 to 5ca5be1 Compare September 15, 2026 10:15

nikhil-jangir-iitk mentioned this pull request Sep 15, 2026

fix(logging): redact credentials embedded in a URL's userinfo before logging #640

Merged

kshivang force-pushed the dev branch from d22d2bf to 77c4a40 Compare September 15, 2026 10:27

nikhil-jangir-iitk marked this pull request as ready for review September 15, 2026 10:31

nikhil-jangir-iitk force-pushed the fix/log-browser-url-query branch from 5ca5be1 to 840b940 Compare September 15, 2026 10:37

kshivang marked this pull request as draft September 15, 2026 22:12

kshivang force-pushed the dev branch from f32537f to f8ccb86 Compare September 15, 2026 22:55

nikhil-jangir-iitk force-pushed the fix/log-browser-url-query branch from 840b940 to 565341a Compare September 16, 2026 00:38

kshivang mentioned this pull request Sep 16, 2026

fix(auth): read a pasted magic link the way the redirect function does #740

Merged

Mihir-Rabari mentioned this pull request Sep 17, 2026

test(cli): pin the CLI security validator contract #841

Merged

kshivang force-pushed the dev branch from 0020ecc to 571e3df Compare September 17, 2026 23:31

nikhil-jangir-iitk force-pushed the fix/log-browser-url-query branch from 565341a to b291fbe Compare September 18, 2026 00:54

nikhil-jangir-iitk marked this pull request as ready for review September 18, 2026 01:24

kshivang marked this pull request as draft September 18, 2026 02:16

kshivang force-pushed the fix/log-browser-url-query branch from b291fbe to e406579 Compare September 18, 2026 02:24

kshivang marked this pull request as ready for review September 18, 2026 02:24

kshivang force-pushed the dev branch from 75ebc71 to 628f152 Compare September 18, 2026 02:25

kshivang force-pushed the fix/log-browser-url-query branch from e406579 to 8f332d3 Compare September 18, 2026 02:37

nikhil-jangir-iitk and others added 4 commits September 17, 2026 19:54

fix(logging): preserve escaped path bytes in URI descriptions

babee67

kshivang force-pushed the fix/log-browser-url-query branch from 8f332d3 to babee67 Compare September 18, 2026 02:55

kshivang merged commit 164a12e into risa-labs-inc:dev Sep 18, 2026
10 checks passed

kshivang mentioned this pull request Sep 18, 2026

Integrate dev: security hardening, reliable persistence, and Home Favorites #935

Merged

KhushiTrivediii added a commit to KhushiTrivediii/BossConsole that referenced this pull request Sep 20, 2026

Merge fix/log-url-secret-redaction: keep query and fragment secrets o…

54e6376

…ut of browser URL log lines and redact JWT tokens (risa-labs-inc#691, risa-labs-inc#855)

Conversation

nikhil-jangir-iitk commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Guard

Worth a reviewer's attention

Verified

Not covered

Update after review (2026-09-15)

Uh oh!

kshivang commented Sep 15, 2026

Uh oh!

kshivang commented Sep 15, 2026

Uh oh!

claude Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: URL logging change at dc4f8d2

Verdict

1. The boss://url leak is still open upstream of the line that was fixed (DeepLinkHandler)

2. CLICommandHandler:236 now prints null://localhost for its commonest input

3. The two BrowserServiceImpl sites - no objection

4. Regression coverage

Inherited trackpad history (separate, as asked)

Uh oh!

nikhil-jangir-iitk commented Sep 15, 2026

Uh oh!

kshivang commented Sep 15, 2026

Uh oh!

claude Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: current head of #691 vs dev

Verdict

Prior findings: dispositions (confirmed, not re-reported)

1. describeUri drops the host entirely whenever java.net.URI will not parse the authority as server-based

2. describeUri("#") and describeUri("?") return the empty string

3. Coverage: the only regression protection in the PR is a text scan

4. Nits, none blocking

What I checked and had no objection to

Uh oh!

kshivang commented Sep 15, 2026

Uh oh!

kshivang commented Sep 15, 2026

Uh oh!

nikhil-jangir-iitk commented Sep 16, 2026

Case 1: the authority disappeared

Case 2: the empty description

Tests

Mutations on the final code

Lint

Not covered

Uh oh!

nikhil-jangir-iitk commented Sep 16, 2026

Uh oh!

kshivang commented Sep 16, 2026

Uh oh!

nikhil-jangir-iitk commented Sep 17, 2026

Uh oh!

kshivang commented Sep 18, 2026

Uh oh!

claude Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: current head b291fbe vs dev

Verdict

Obsolete: prior findings, confirmed closed at this head

1. describedBase appends the decoded path, so a %0A in a URL path forges a log line

2. The guard's KDoc limits list is still missing three items

3. Coverage: still only a text scan (recommendation, not a blocker)

4. Nits, none blocking

What I checked and had no objection to

Uh oh!

kshivang commented Sep 18, 2026

Uh oh!

kshivang commented Sep 18, 2026

Uh oh!

claude Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: integrated head e406579 vs dev

Verdict

The blocker: closed

nikhil-jangir-iitk commented Sep 15, 2026 •

edited

Loading

claude Bot commented Sep 15, 2026 •

edited

Loading

Review: URL logging change at `dc4f8d2`

1. The `boss://url` leak is still open upstream of the line that was fixed (`DeepLinkHandler`)

2. `CLICommandHandler:236` now prints `null://localhost` for its commonest input

3. The two `BrowserServiceImpl` sites - no objection

claude Bot commented Sep 15, 2026 •

edited

Loading

Review: current head of #691 vs `dev`

1. `describeUri` drops the host entirely whenever `java.net.URI` will not parse the authority as server-based

2. `describeUri("#")` and `describeUri("?")` return the empty string

claude Bot commented Sep 18, 2026 •

edited

Loading

Review: current head `b291fbe` vs `dev`

1. `describedBase` appends the decoded path, so a `%0A` in a URL path forges a log line

claude Bot commented Sep 18, 2026 •

edited

Loading

Review: integrated head `e406579` vs `dev`

claude Bot commented Sep 18, 2026 •

edited

Loading

Confirmation: prior review remains closed at exact head `8f332d3`

claude Bot commented Sep 18, 2026 •

edited

Loading

Confirmation: prior review remains closed at exact head `babee673`