Repository navigation
fix(logging): redact credentials embedded in a URL's userinfo before logging - #640
Merged
kshivang merged 2 commits intoSep 15, 2026
Merged
Conversation
Contributor
|
@claude Please review the current head for correctness, credential redaction coverage, regressions, and integration with dev. |
|
Claude finished @kshivang's task in 4m 45s —— View job Review: PR #640 - redact userinfo before logging
VerdictThe change is correct and I found no regression. The premise checks out too: I confirmed The one thing I'd want resolved before calling the git-clone case covered is finding #1 below: the same function fixed here logs the token anyway, ~100 lines later, from git's own stderr. Correctness -
|
Contributor
|
@ninja-lifts Thank you for identifying the concrete userinfo leak and supplying focused positive/negative tests. The authority redaction and query-index recomputation are correct. I independently compiled the actual sanitizer plus host delegate: all 10 new tests and 80 existing host tests pass. Against current dev, exactly the six userinfo cases fail, reproducing your red/green evidence. I am moving this PR to draft for a bounded follow-up, with your original commit and authorship unchanged. The latest Claude review identified an end-to-end gap I verified at Actionable remaining work:
Validation status is separate from author defects: I approved the existing fork CI run https://github.com/risa-labs-inc/BossConsole/actions/runs/34798899078. Quality, shell, version, AI-contract and database jobs passed; platform builds/compatibility were still running at this checkpoint. Local module Current head: |
kshivang
marked this pull request as draft
September 14, 2026 04:06
kshivang
force-pushed
the
dev
branch
2 times, most recently
from
September 14, 2026 08:43
7dfb90e to
a3f4b7f
Compare
nikhil-jangir-iitk
force-pushed
the
fix/log-uri-userinfo
branch
from
September 14, 2026 09:37
9193597 to
695d8a2
Compare
Contributor
Author
|
@kshivang Thanks for the bounded list. All three items are addressed in a new commit, 1. Git clone outputBefore changing anything I checked what git actually prints. On git 2.43, against a local HTTP server, a clone URL with a token produced no credential in any The leak is still real through three other sources, all merged into the same stream:
One limit on reach: these lines are logged at DEBUG, and Fix: the loop now logs Tests: 2. Nested URLsEvery 3. Pins and boundariesPinned One correction to the review: the no-path case does not depend on Verification (final head, Windows, JDK 17)
I did not run the full composeApp suite locally, only the tests above, so full CI on this head is still the gate. Not changed
|
nikhil-jangir-iitk
marked this pull request as ready for review
September 14, 2026 09:38
Contributor
|
@claude Please review contributor update 695d8a2 against current dev, especially the clone progress sink, multiple/nested URL scan, and the new regression tests. The previous handoff was addressed by @ninja-lifts; please assess the new code rather than carrying forward the old draft conclusion. Fork CI run 34828969760 has been approved. The published API copy and broad free-text sanitizer ordering are explicitly outside this change; the host facade delegates to the updated parent-first host module. |
|
Claude finished @kshivang's task in 7m 54s —— View job Review: contributor update
|
Contributor
|
@ninja-lifts Thank you for addressing the earlier handoff with measured Git trace/server-output evidence, the clone sink repair, nested-URL handling and targeted regression/mutation tests. Those follow-up changes are your work; I made no code changes to this PR. Final reviewed head: The low-severity review notes are not merge blockers: the scan's bounds/progress proof makes the unguarded helper total today; comma-adjacent emails and free-text nested-query separators can over-redact (diagnostic fidelity, not credential disclosure); using the shared test-root helper is optional cleanup; the source assertion pins this exact call, not every possible future logging expression. Keep those limits explicit. Protocol-relative/encoded nested URLs, literal ampersands inside nested userinfo and browser backslash normalization retain the documented scope limits. #659 is a distinct draft timeout change to the same clone loop and must preserve this redaction on later integration; it was not modified here. The sole final handoff blocker is hosted macOS CI still queued, not the old findings and not an author test failure. Linux, Windows, quality, compatibility, database, shell, version and contracts checks have passed at this snapshot: https://github.com/risa-labs-inc/BossConsole/actions/runs/34828969760 . The fork run was approved during this review. Next: let macOS and the build summary finish, address any actual failure if one appears, restore ready, refresh dev and mergeability, then merge under the shared dev lock using this reviewed SHA. No new implementation or repeated Claude request is required by the present review. I am converting the open PR to draft solely under the requested bounded-review policy because that remaining gate cannot be bypassed. |
kshivang
marked this pull request as draft
September 14, 2026 10:08
kshivang
force-pushed
the
dev
branch
2 times, most recently
from
September 14, 2026 11:16
cd88490 to
5a2e818
Compare
This was referenced Sep 15, 2026
nikhil-jangir-iitk
force-pushed
the
fix/log-uri-userinfo
branch
from
September 15, 2026 10:15
695d8a2 to
35777ab
Compare
Contributor
Author
|
Rebased onto dev |
nikhil-jangir-iitk
marked this pull request as ready for review
September 15, 2026 10:32
…logging
maskUriParams masked only sensitive query and fragment parameters, so a
credential carried in a URL's authority (scheme://user:password@host) was
returned verbatim and written to the log. A private HTTPS git remote is
https://x-access-token:<token>@github.com/..., and DesktopGitService logs the
clone URL at INFO and elsewhere branches on repositoryUrl.contains("@") for its
auth-failure message, so the URL it logs is expected to carry a token. Browser
navigation and deep-link sites pass URLs through the same masker.
redactUserInfo now replaces the userinfo with [REDACTED], keeping the scheme,
host, port and path. Only an @ inside the authority is treated as a delimiter -
the authority ends at the first / ? or # after :// - so an @ in a path or a
query value (an email) is left alone, and a URL with no :// is unchanged. The
query index is now read off the redacted string, since the redaction can change
its length.
Review of the first commit found two more places a URL credential reaches the log. cloneRepository merges git's stderr into stdout and logged each line raw at DEBUG. git 2.22 and later strip the credential from their own fatal messages (measured on git 2.43 for 404, 401, 403, 500, redirect and connection failures), but three sources still print it: git older than 2.22, whose release notes say remote-http "did not anonymize URLs reported in its error messages at places"; GIT_TRACE=1 inherited from the environment, which printed the full clone URL four times on git 2.43; and text a server sends back, which git prints verbatim after "remote error:". These lines reach the log only when DEBUG is enabled. The line now goes through cloneProgressLogMessage, which calls the new LogSanitizer.redactUrlUserInfo. It removes only the userinfo, so the failure text, host and port stay readable; sanitizeLogMessage would replace the whole URL and its host. maskUriParams examined only the first authority, so https://sso.example/login?next=https://u:p@internal/ kept its credential. Every :// is now examined. A nested URL's authority also ends at &, so a later &contact=a@b.com is not read as its userinfo. The no-path and repeated-@ cases were already correct and are now pinned, along with an @ as the last authority character. Input without ://, percent-encoded nested URLs and backslash normalization are documented as not handled.
nikhil-jangir-iitk
force-pushed
the
fix/log-uri-userinfo
branch
from
September 15, 2026 10:37
35777ab to
bf6a17e
Compare
Contributor
|
@claude Please review the current head of #640 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. |
nikhil-jangir-iitk
added a commit
to nikhil-jangir-iitk/sec_boss
that referenced
this pull request
Sep 18, 2026
…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
pushed a commit
to nikhil-jangir-iitk/sec_boss
that referenced
this pull request
Sep 18, 2026
…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
pushed a commit
to nikhil-jangir-iitk/sec_boss
that referenced
this pull request
Sep 18, 2026
…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
pushed a commit
to nikhil-jangir-iitk/sec_boss
that referenced
this pull request
Sep 18, 2026
…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
added a commit
that referenced
this pull request
Sep 18, 2026
…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>
nitininhouse
added a commit
to nitininhouse/BossConsole
that referenced
this pull request
Sep 18, 2026
7 of 20 tasks
This was referenced Sep 18, 2026
Closed
24 of 44 tasks
kshivang
added a commit
that referenced
this pull request
Sep 22, 2026
…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
added a commit
that referenced
this pull request
Sep 22, 2026
…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>
choksi2212
pushed a commit
to choksi2212/BossConsole
that referenced
this pull request
Sep 22, 2026
…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>
choksi2212
pushed a commit
to choksi2212/BossConsole
that referenced
this pull request
Sep 22, 2026
…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>
kshivang
pushed a commit
to nitininhouse/BossConsole
that referenced
this pull request
Sep 22, 2026
kshivang
added a commit
to KhushiTrivediii/BossConsole
that referenced
this pull request
Sep 22, 2026
…d sanitizer Agent review fix for risa-labs-inc#916 (not contributor credit): - The PR's own redactUrlUserInfo regex stopped at the FIRST '@' in the URL authority, so 'https://user:p@ss@host/' logged the credential tail 'ss@host'. Delegate to LogSanitizer.redactUrlUserInfo (risa-labs-inc#640), which redacts at the LAST '@' per WHATWG URL parsing, and add regression tests for the at-sign password and for non-URL at-signs.
kshivang
added a commit
to KhushiTrivediii/BossConsole
that referenced
this pull request
Sep 22, 2026
…PEM blocks in the argument sanitizer Review repair for risa-labs-inc#886 (agent work, not credited to the contributor): - URI userinfo: call LogSanitizer.redactUrlUserInfo (merged in risa-labs-inc#640) instead of a local regex. The local regex stopped at the first @ in the authority, so a password containing an @ left its tail (ss@host) in the approval dialog and the operation ledger. Issue risa-labs-inc#886's suggested fix calls for exactly this reuse. - PEM private keys: mask the whole BEGIN..END block (a header-only mask left the key material on the line) and still mask truncated fragments to end of line. - curl -u/--user: username part may not contain /, :, or @, and the flag must not be mid-token, so a bare URL argument (-u http://host) is left untouched while a real user:pass credential is masked whole (passwords may contain colons). - Tests: pin the last-@ delimiter case, path-@ case, colon-in-password case, and full-block PEM masking.
nikhil-jangir-iitk
added a commit
to nikhil-jangir-iitk/sec_boss
that referenced
this pull request
Sep 22, 2026
The free-text sanitizers left a password in the line when the URL's host was an IP literal. Measured on dev 5dda86e, through the real sanitizer: in : Failed to connect to proxy http://alice:pass@10.0.0.5:3128 out: Failed to connect to proxy http:[PATH]:pass@10.0.0.5:3128 filePathPattern's (?:/[^\s:]+)+ reaches //alice before urlPattern can match the whole URL, so the URL is never recognised as one and the credential survives. risa-labs-inc#640 closed this for maskUriParams and recorded the free-text path as a separate change. This is that change. The bound is narrower than risa-labs-inc#640 stated, and I measured it rather than assuming it: the leak needs an IP-literal host. Against a DNS name, emailPattern matches s3cr3t@proxy.corp.example and masks the password by accident. The DNS cases are in the test as guards, not as evidence, and the test says so. Where this lands is the reason it matters. These functions are where text BOSS did not write arrives, and two call sites delegate that trust to them in their own comments: McpToolRegistryImpl.executeUncapped puts a plugin's exception message through sanitizeExceptionMessage before returning it to the agent, and CrashReport puts every captured log entry through sanitizeLogMessage on the way into a report built to be submitted. Neither can predict what it is handed. I found no string BOSS itself builds that triggers this; git clone output is already covered by risa-labs-inc#640's cloneProgressLogMessage. The fix is one line. redactUrlUserInfo already exists and already has eight tests from risa-labs-inc#640; it was simply never wired into this chain. It runs after the query-parameter pass, which must see the original text, and before the location passes, which need the authority still intact. Only the credential is removed; what survives is masked as a location exactly as before. Verified by running it. Red: four of the seven new tests fail against unmodified dev, and the three that pass are the guards, so they are not passing vacuously. Green: 63 tests in plugin-logging and 267 in composeApp's sanitizer and crash suites, with no existing test modified. Mutation: removing the one line fails the same four by name and leaves the guards green. detekt and ktlint pass. Not covered: boss-plugin-api ships its own 306-line copy of this class with the same pass ordering and no redactUrlUserInfo at all. The host copy shadows it parent-first inside plugin classloaders, so the host path is closed, but that repo needs its own change. A protocol-relative //user:pass@host is still unchanged, as redactUrlUserInfo's own KDoc records.
kshivang
added a commit
to KhushiTrivediii/BossConsole
that referenced
this pull request
Sep 22, 2026
…PEM blocks in the argument sanitizer Review repair for risa-labs-inc#886 (agent work, not credited to the contributor): - URI userinfo: call LogSanitizer.redactUrlUserInfo (merged in risa-labs-inc#640) instead of a local regex. The local regex stopped at the first @ in the authority, so a password containing an @ left its tail (ss@host) in the approval dialog and the operation ledger. Issue risa-labs-inc#886's suggested fix calls for exactly this reuse. - PEM private keys: mask the whole BEGIN..END block (a header-only mask left the key material on the line) and still mask truncated fragments to end of line. - curl -u/--user: username part may not contain /, :, or @, and the flag must not be mid-token, so a bare URL argument (-u http://host) is left untouched while a real user:pass credential is masked whole (passwords may contain colons). - Tests: pin the last-@ delimiter case, path-@ case, colon-in-password case, and full-block PEM masking.
atharvgaur1845
pushed a commit
to atharvgaur1845/BossConsole
that referenced
this pull request
Sep 22, 2026
…dialog or the ledger McpArgumentSanitizer masked a value when its key was sensitive, when it was an assignment, when it was an Authorization header or when its shape was a known token. A password inside a URI authority is none of those, so `psql postgres://admin:hunter2@host/app` reached the approval dialog and McpOperationLedger, which appends it to disk, verbatim (risa-labs-inc#886). risa-labs-inc#640 had already added LogSanitizer.redactUrlUserInfo for the logging path; the MCP path never called it. sanitizeMessage now runs that helper first, so the userinfo of every URI becomes [REDACTED] while the scheme, host, port and path stay readable, which is what an operator approves a command by. Three more shapes the issue measured leaking get a rule each: curl basic auth (`-u user:pass`, requiring the colon so `git push -u origin` and a URI after `-u` are left alone), an AWS access key id in its documented form, and a PEM private key body. McpArgumentSanitizerUriCredentialTest covers the issue's measured lines, the look-alikes that must survive (an email in --author, /@handle, a bare `-u admin`, a public key block) and that the existing rules still apply. Closes risa-labs-inc#886 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
choksi2212
added a commit
to choksi2212/BossConsole
that referenced
this pull request
Sep 24, 2026
…L userinfo in logs, and complete the reload cycle Fixes risa-labs-inc#911. The browser service accepted any non-blank URL on a plugin-exposed IPC surface: a javascript: URL was executed in the page context with no approval gate, and the raw URL - including user:password@host credentials - was logged verbatim at INFO. A refused scheme now short-circuits before any state is touched; allowed schemes are http/https/file/ftp/about. URL logging redacts userinfo per the risa-labs-inc#640 shape. reload() also stranded the window in loading=true forever (STARTED emitted, COMPLETED never): the bookkeeping-only reload now completes the cycle and clears the flag. 4 regression tests, none collecting unbounded flows. (cherry picked from commit 1adee41)
fluck-boss
added a commit
that referenced
this pull request
Sep 24, 2026
… close the cells it fails (#1019) * fix(mcp): mask a credential in a URI authority before it reaches the dialog or the ledger McpArgumentSanitizer masked a value when its key was sensitive, when it was an assignment, when it was an Authorization header or when its shape was a known token. A password inside a URI authority is none of those, so `psql postgres://admin:hunter2@host/app` reached the approval dialog and McpOperationLedger, which appends it to disk, verbatim (#886). #640 had already added LogSanitizer.redactUrlUserInfo for the logging path; the MCP path never called it. sanitizeMessage now runs that helper first, so the userinfo of every URI becomes [REDACTED] while the scheme, host, port and path stay readable, which is what an operator approves a command by. Three more shapes the issue measured leaking get a rule each: curl basic auth (`-u user:pass`, requiring the colon so `git push -u origin` and a URI after `-u` are left alone), an AWS access key id in its documented form, and a PEM private key body. McpArgumentSanitizerUriCredentialTest covers the issue's measured lines, the look-alikes that must survive (an email in --author, /@handle, a bare `-u admin`, a public key block) and that the existing rules still apply. Closes #886 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(mcp): state the argument sanitizer's contract as a property, and close the cells it fails Four sanitizer leak reports in a week (#836, #837, #886, and the shapes below) were each one hand-found cell of the same table: how a command labels a credential, crossed with how the text is wrapped on its way through a tool argument. McpArgumentSanitizerFuzzTest walks the table with a seeded generator: 26 shapes (assignments and their `_`/`-` built names, YAML and JSON keys, Authorization and cookie headers, long and short flags, URI userinfo, npm's token line, vendor tokens, a JWT, an AWS key id, a PEM block) times 10 contexts (quoting, `export`, chained and piped lines, a multiline script, a JSON string with its escapes, prose, unicode neighbours), plus a nested-argument pass, asserting the random value never survives into what the dialog and the ledger receive; and 20 benign commands times the same contexts asserting byte-identical output, because the operator approves what they can read. On the sanitizer as of #979 the property failed in 303 cells across 11 shapes. Each is one rule now: sensitive names extended by `_`/`-` words (`AWS_SECRET_ACCESS_KEY=`, `SECRET_KEY_BASE=`), a quoted or JSON-escaped key before the `:`/`=` (`{"password":"x"}` in a curl body, also inside a JSON string), long secret flags with a space (`--password x`, `--token x`), the short `-p` only after the programs whose `-p` is a password (sshpass, mysql tools, mongo, docker login, az login) and `-a` after redis-cli, npm's `_authToken x`, a `Cookie:` header, and a PEM body carrying JSON-escaped newlines. `max_tokens=4096` and `--tokenizer=` stay readable: only whole words joined by `_`/`-` extend a name, since `tokens` is not `token`. Whole ai.rever.boss.mcp package: 269 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mcp): redact encrypted PEM bodies, un-vacuous the aws fuzz cell The pemPrivateKey body class excludes '-', so a traditionally encrypted PEM - Proc-Type:/DEK-Info: headers between the BEGIN line and the base64 - stops the match four characters in and the entire encrypted key body survives into the approval dialog and the ledger. That is openssl rsa -aes256 and ssh-keygen -m PEM with a passphrase. Let the rule absorb RFC 1421 header lines, and add the encrypted-PEM shape as the 27th fuzz shape (the harness could not see the leak: its pem block shape generates a headerless block). Also make the aws access key id cell assert what it means: the shape renders AKIA + an uppercased, truncated secret, so contains(secret) was vacuously true. Probe for the rendered key body instead. * fix(mcp): close the encrypted-PEM cell when the command travels in a JSON string The RFC 1421 header arm anchored on real newlines only: when the whole command is wrapped as a JSON string value the newlines arrive as '\' + 'n', the arm cannot start, and the match stops four characters into Proc-Type - the encrypted key body survives into the approval dialog and the ledger (BOSS_SANITIZER_FUZZ_SEED=20260919, cell: encrypted pem block / json string value). Let header lines end at a real or an escaped newline and stop line content at either. --------- Co-authored-by: Atharv Gaur <atharvanna71@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: fluck-boss <fluck-boss@users.noreply.github.com>
fluck-boss
pushed a commit
that referenced
this pull request
Sep 24, 2026
The free-text sanitizers left a password in the line when the URL's host was an IP literal. Measured on dev 5dda86e, through the real sanitizer: in : Failed to connect to proxy http://alice:pass@10.0.0.5:3128 out: Failed to connect to proxy http:[PATH]:pass@10.0.0.5:3128 filePathPattern's (?:/[^\s:]+)+ reaches //alice before urlPattern can match the whole URL, so the URL is never recognised as one and the credential survives. #640 closed this for maskUriParams and recorded the free-text path as a separate change. This is that change. The bound is narrower than #640 stated, and I measured it rather than assuming it: the leak needs an IP-literal host. Against a DNS name, emailPattern matches s3cr3t@proxy.corp.example and masks the password by accident. The DNS cases are in the test as guards, not as evidence, and the test says so. Where this lands is the reason it matters. These functions are where text BOSS did not write arrives, and two call sites delegate that trust to them in their own comments: McpToolRegistryImpl.executeUncapped puts a plugin's exception message through sanitizeExceptionMessage before returning it to the agent, and CrashReport puts every captured log entry through sanitizeLogMessage on the way into a report built to be submitted. Neither can predict what it is handed. I found no string BOSS itself builds that triggers this; git clone output is already covered by #640's cloneProgressLogMessage. The fix is one line. redactUrlUserInfo already exists and already has eight tests from #640; it was simply never wired into this chain. It runs after the query-parameter pass, which must see the original text, and before the location passes, which need the authority still intact. Only the credential is removed; what survives is masked as a location exactly as before. Verified by running it. Red: four of the seven new tests fail against unmodified dev, and the three that pass are the guards, so they are not passing vacuously. Green: 63 tests in plugin-logging and 267 in composeApp's sanitizer and crash suites, with no existing test modified. Mutation: removing the one line fails the same four by name and leaves the guards green. detekt and ktlint pass. Not covered: boss-plugin-api ships its own 306-line copy of this class with the same pass ordering and no redactUrlUserInfo at all. The host copy shadows it parent-first inside plugin classloaders, so the host path is closed, but that repo needs its own change. A protocol-relative //user:pass@host is still unchanged, as redactUrlUserInfo's own KDoc records.
kshivang
pushed a commit
that referenced
this pull request
Sep 24, 2026
The free-text sanitizers left a password in the line when the URL's host was an IP literal. Measured on dev 5dda86e, through the real sanitizer: in : Failed to connect to proxy http://alice:pass@10.0.0.5:3128 out: Failed to connect to proxy http:[PATH]:pass@10.0.0.5:3128 filePathPattern's (?:/[^\s:]+)+ reaches //alice before urlPattern can match the whole URL, so the URL is never recognised as one and the credential survives. #640 closed this for maskUriParams and recorded the free-text path as a separate change. This is that change. The bound is narrower than #640 stated, and I measured it rather than assuming it: the leak needs an IP-literal host. Against a DNS name, emailPattern matches s3cr3t@proxy.corp.example and masks the password by accident. The DNS cases are in the test as guards, not as evidence, and the test says so. Where this lands is the reason it matters. These functions are where text BOSS did not write arrives, and two call sites delegate that trust to them in their own comments: McpToolRegistryImpl.executeUncapped puts a plugin's exception message through sanitizeExceptionMessage before returning it to the agent, and CrashReport puts every captured log entry through sanitizeLogMessage on the way into a report built to be submitted. Neither can predict what it is handed. I found no string BOSS itself builds that triggers this; git clone output is already covered by #640's cloneProgressLogMessage. The fix is one line. redactUrlUserInfo already exists and already has eight tests from #640; it was simply never wired into this chain. It runs after the query-parameter pass, which must see the original text, and before the location passes, which need the authority still intact. Only the credential is removed; what survives is masked as a location exactly as before. Verified by running it. Red: four of the seven new tests fail against unmodified dev, and the three that pass are the guards, so they are not passing vacuously. Green: 63 tests in plugin-logging and 267 in composeApp's sanitizer and crash suites, with no existing test modified. Mutation: removing the one line fails the same four by name and leaves the guards green. detekt and ktlint pass. Not covered: boss-plugin-api ships its own 306-line copy of this class with the same pass ordering and no redactUrlUserInfo at all. The host copy shadows it parent-first inside plugin classloaders, so the host path is closed, but that repo needs its own change. A protocol-relative //user:pass@host is still unchanged, as redactUrlUserInfo's own KDoc records.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LogSanitizer.maskUriParamsis the masker used before a URL is logged, at ~22 call sites (git clone, browser navigation, deep links). It masked only sensitive query and fragment parameters and returned the rest of the URL verbatim - including the userinfo component,scheme://user:password@host, where a credential is routinely carried:https://x-access-token:<token>@github.com/..., andDesktopGitService.cloneRepositorylogs the clone URL at INFO ("Starting git clone") and later branches onrepositoryUrl.contains("@")for its auth-failure message - so the URL it logs is expected to carry a token;nav.url/url.So
https://x-access-token:ghs_...@github.com/o/r.gitwas written to the log, and to the plugin-readable log capture, in cleartext.Fix
redactUserInforeplaces the userinfo with[REDACTED], keeping the scheme, host, port and path:https://x-access-token:ghs_...@github.com/o/r.git->https://[REDACTED]@github.com/o/r.git.Only an
@inside the authority is treated as a userinfo delimiter. The authority ends at the first/,?or#after://, so:@in a path (/@handle) or in a query value (an email,?to=a@b.com) is left alone;://(amailto:address) is returned unchanged;:, so a bare-token userinfo (https://ghp_...@host) is covered too.The query index is now read off the redacted string rather than the original, because the redaction can change the string's length; a userinfo credential and a sensitive query parameter in the same URL are both removed.
Worth a reviewer's attention
redactUserInforeturns the original string untouched whenever there is no://or no@in the authority, so every URL that has no userinfo is byte-for-byte unchanged - the existing behaviour is preserved for everything the masker already handled.maskUriParamsstays purely text-based (noURIparsing), matching the rest of the function: it must not throw on a malformed input and must keep the path/query for debugging.Verified
Red first:
UriUserInfoTest(10 tests) against the unchangedmaskUriParamsfails 6 of 10 (the userinfo-redaction cases); the 4 negative cases (email in query,@in path,mailto:, plain URL) already pass, proving the fix does not over-redact.Full
:plugin-platform:plugin-logging:desktopTestpasses (32 tests, 0 failures, including the module's publish-path guard tests), and thecomposeAppLogSanitizerTest, which exercises the re-export delegate, passes (80, 0).ktlintCheckanddetektare clean.Mutation-checked, each mutation caught by a named test:
@)@in path,@in query):redacted, username keptNot covered (found while doing this, separate changes)
sanitizeLogMessage/sanitizeStackTrace) still leaves a userinfo password in a bare proxy message such asFailed to connect to proxy http://alice:pass@10.0.0.5:3128:filePathPatternconsumes//alicebeforeurlPatterncan match the whole URL, leavinghttp:[PATH]:pass@10.0.0.5. That is a different function with a pass-ordering interaction and is left for a separate change.maskUriParams' sensitive-parameter allow-list (10 names) is unchanged here; broadening it (e.g.client_secret, presigned-URL signatures, SSOticket) is a separate question about which names count as sensitive.Contributor follow-up (2026-09-14)
@ninja-lifts supplied commit
695d8a2aaddressing the previous review: every plain nested URL is examined; free-text clone progress goes throughredactUrlUserInfo; last-@, no-path and malformed authority boundaries are pinned. New tests include inherited GIT_TRACE and server-echo canaries. These repairs and all implementation/tests are contributor work, not reviewer code changes. The original description above records the initial submission; clone output and plain nested URLs are now covered by this follow-up. Protocol-relative and percent-encoded nested URLs, literal ampersands in nested userinfo, and browser backslash normalization remain documented limitations.Rebase onto dev (2026-09-15)
Rebased both commits onto
devd22d2bfeb. The first applied cleanly. The second conflicted inDesktopGitService.cloneRepositorywith fix(git): make clone timeout cancellable #660, which moved the clone loop intorunCloneProcesswith a cancellable timeout.Resolution: dev's fix(git): make clone timeout cancellable #660 loop is kept as it is, and its progress callback's debug line goes through
cloneProgressLogMessage, the same one-line change this PR made to the old loop:GitCloneProcess.kt(new in fix(git): make clone timeout cancellable #660) has no logger calls, so that line is still the only place clone output reaches a log. The diff againstdevis the same six files, +430/-5, as before the rebase.Verified on the rebased head:
:plugin-platform:plugin-logging:desktopTest48, 0 failures;:composeApp:desktopTestforai.rever.boss.utils.logging.*andai.rever.boss.git.*233, 0 failures, includingGitCloneProgressLogTest, whose source assertion pins that exact call;ktlintCheckanddetektclean for both modules.Collision check: a 3-way merge of the six files against every open PR that changes them (37 PR/file pairs) adds 0 conflicts, except against fix(logging): keep query and fragment secrets out of browser URL log lines #691's current remote branch, which still carries a reviewer merge of an older
devclone loop; fix(logging): keep query and fragment secrets out of browser URL log lines #691's rebased branch does not touchDesktopGitService.kt, and merging it with this branch gives 0 conflicts.The low-severity review notes are unchanged, as agreed in the review.
Morning review boundary (2026-09-15)
The overlapping marker shape
https://host://u:p@in/is not scanned as a second URL because://starts one character before the outer authority end. Theu:p@intext is a path in the outer URL, not its navigation credentials. This unusual nested/free-text shape remains an explicit limitation; this is not a universal sanitizer for arbitrary credential-shaped path text. The current clone output reader inGitCloneProcess.kthas no logging calls, and both live output and the trailing drain use the redacting callback. A future logging sink there would need its own regression guard.Current-head review: #640 (comment) . All implementation and contributor follow-ups remain @ninja-lifts's work; this batch made no source changes.