Repository navigation
fix(browser) : refuse javascript:/data: URLs in BrowserServiceImpl.navigate (#911) - #932
Closed
abhinavlenka wants to merge 89 commits into
Closed
abhinavlenka wants to merge 89 commits into
abhinavlenka wants to merge 89 commits into
Conversation
…h 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.
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
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
…arch-selected-row-highlight fix(search): highlight the selected row in the sectioned global search list
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.
…te stats and project writes (risa-labs-inc#860)
… a compound or quoted credential cannot survive
…e catalog sha256 (risa-labs-inc#798 follow-up)
…or confirmation boss://terminal?command= has consulted DeepLinkOrigin since risa-labs-inc#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.
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 risa-labs-inc#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.
setFiles wrote _allFiles and then derived _recentFiles, and risa-labs-inc#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 risa-labs-inc#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.
…isa-labs-inc#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 risa-labs-inc#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>
…risa-labs-inc#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>
…hot-manager-confinement fix(orchestrator): confine snapshot paths and persist atomically with owner permissions (risa-labs-inc#873)
…manifest-read fix(plugin): bound the manifest entry reads before signature verification
…e-link-origin fix(deep-link): hold an external workspace link's terminal commands for confirmation
…set (risa-labs-inc#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 risa-labs-inc#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 risa-labs-inc#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>
Contributor
|
Cross-referencing against #911 (our filing, 08:39Z) — four PRs now sit on it (#916, #931, #932, #955). Two substantive points on this approach:
Also for the maintainer's triage: this PR has zero CI runs (statusCheckRollup empty — no check has ever run on it, while the same fork runs CI on #943), and the base is |
abhinavlenka
added a commit
to abhinavlenka/BossConsole
that referenced
this pull request
Sep 18, 2026
…daction (risa-labs-inc#911) Review on PR risa-labs-inc#932 (choksi2212) found two bypasses in the original fix: 1. dangerousUrlScheme split on the first ':' and set-tested {javascript, data} after trim().lowercase(). "jav\tascript:alert(1)" didn't match that literal set, but a real URL parser strips embedded tab/newline/CR before reading the scheme and still runs it as javascript:. Replaced the blocklist with an allowlist (http, https, file, ftp) and made scheme extraction WHATWG-faithful: strip embedded tab/newline/CR and leading C0 controls/space before reading the scheme, then validate it against RFC 3986's grammar. This also refuses view-source:, about:, schemeless and other unknown schemes by default, matching PR risa-labs-inc#931's approach (referenced in the same review thread as the other candidate fix for issue risa-labs-inc#911). 2. redactUrlUserInfo's regex masked only up to the first '@', so a password containing its own '@' leaked its tail into the log ("admin:p@ss@w0rd@internal-host" only masked up to the first '@'). Rewritten to split on the LAST '@' within the authority component, matching the WHATWG URL standard's userinfo/host boundary. Also wired into getFavicon's log line, which had the same unredacted leak. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…daction (risa-labs-inc#911) Review on PR risa-labs-inc#932 (choksi2212) found two bypasses in the original fix: 1. dangerousUrlScheme split on the first ':' and set-tested {javascript, data} after trim().lowercase(). "jav\tascript:alert(1)" didn't match that literal set, but a real URL parser strips embedded tab/newline/CR before reading the scheme and still runs it as javascript:. Replaced the blocklist with an allowlist (http, https, file, ftp) and made scheme extraction WHATWG-faithful: strip embedded tab/newline/CR and leading C0 controls/space before reading the scheme, then validate it against RFC 3986's grammar. This also refuses view-source:, about:, schemeless and other unknown schemes by default, matching PR risa-labs-inc#931's approach (referenced in the same review thread as the other candidate fix for issue risa-labs-inc#911). 2. redactUrlUserInfo's regex masked only up to the first '@', so a password containing its own '@' leaked its tail into the log ("admin:p@ss@w0rd@internal-host" only masked up to the first '@'). Rewritten to split on the LAST '@' within the authority component, matching the WHATWG URL standard's userinfo/host boundary. Also wired into getFavicon's log line, which had the same unredacted leak. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
abhinavlenka
force-pushed
the
fix/browser-javascript-scheme-gate
branch
from
September 20, 2026 11:13
3a8a747 to
f19d930
Compare
Contributor
Author
|
Thanks - both points were right, and both are fixed here. 1. Blocklist -> allowlist. 2. Multi- 3. The base. You were right that the branch was cut from
|
kshivang
force-pushed
the
dev
branch
2 times, most recently
from
September 22, 2026 04:42
bdcf620 to
bc9aa3b
Compare
Contributor
|
@abhinavlenka — thank you for the thorough work here. Your v2 (allowlist + WHATWG-faithful scheme extraction, last- Why it's superseded
What's genuinely yours and credited
If you'd like to re-engage (fully welcome, the issue is still open for future schemes like |
kshivang
marked this pull request as draft
September 22, 2026 11:36
Contributor
Author
|
Closing in favour of #931, which has since landed on |
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
14 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.
📋 Pull Request
Description
BrowserServiceImpl.navigate()(modules/boss-app-browser) had no scheme gate: ajavascript:or
data:URL was accepted, logged verbatim at INFO (including any embedded credentials, e.g.https://user:pass@host), and reported back as a completed navigation. This adds a schemeallowlist (
http,https,file,ftp) checked before any state/logging/events happen,and masks URL userinfo in the one log line that still prints a URL.
An allowlist rather than a blocklist, after @choksi2212's review below: a
{javascript, data}set-test is bypassed by
jav\tascript:, because a real URL parser strips embedded tab, newlineand CR before reading the scheme and still executes it. The scheme is now extracted the way the
WHATWG parser does - tab/newline/CR removed from anywhere in the string, then leading C0 controls
and spaces trimmed, then validated against RFC 3986's scheme grammar - so
jav\tascript:,jav\nascript:, a leading NUL byte andview-source:data:...are all refused, as is any schemenobody has thought of yet. The same review noted that the redaction regex masked only up to the
first
@, leaving the tail ofhttps://admin:p@ss@host/in the log; it now splits on the last@within the authority, which is what the WHATWG parser treats as the userinfo boundary.Also fixes a companion bug the issue calls out in its own title:
reload()setisLoading = trueand nothing ever reset it, so
getPageInfo()reported "loading" forever after the first reload.Fixed by mirroring the synchronous STARTED -> COMPLETED pattern
navigate()already uses.Note on the suggested fix: the issue suggests routing logging through the existing
LogSanitizer.redactUrlUserInfo. That class lives in composeApp;boss-app-browseris aseparate GraalVM-native microkernel module with no dependency on the desktop app module, so I
wrote a small local equivalent (
redactUrlUserInfo) scoped to exactly what this log line needs,rather than adding a cross-module dependency for one function.
🔄 Type of Change
📦 Version Impact
x.x.+1x.+1.0+1.0.0✅ Testing Checklist
🧪 Local Testing
🖥️ Platform Testing
🏗️ Build Verification
🔍 Code Quality
🚀 CI/CD Integration
📸 Screenshots/Media
N/A
🔗 Related Issues
Fixes #911
📝 Additional Context
🎯 Motivation and Context
Issue #911: the microkernel browser service accepted, executed-in-name (it's a stub that doesn't
actually run JxBrowser, but the RPC contract behaves as if it did), and logged
javascript:/data:URLs verbatim, including any credentials embedded in a URL's userinfo. The issue's titlealso calls out a stuck-
isLoadingcompanion bug inreload().🧠 Implementation Details
extractUrlScheme(url),navigationRefusalReason(url)andredactUrlUserInfo(url)are smalltop-level pure functions in
BrowserServiceImpl.kt, unit-tested directly.ALLOWED_URL_SCHEMESis the allowlist; anything else, including a URL with no syntactically valid scheme, is refused.
URL, so a caller can log it unconditionally.
navigate()now checks the scheme before touchingwindowStatesor emitting anynavigationEvents- a refused URL leaves no trace beyond a warning log naming the scheme, notthe payload.
reload()now setsisLoading = truethenfalsearound its STARTED/COMPLETED event pair,matching
navigate()'s own synchronous pattern, instead of leaving it permanentlytrue.BrowserServiceImplTest, including the bypass vectors from the review: a tab, anewline and a leading NUL byte in the scheme;
view-source:as a nested scheme;about:, whichthe old blocklist never named; and a password containing
@in the redaction cases.dev(it was branched frommain, which put two unrelated release-note files inthe diff);
:boss-app-browser:test,:boss-app-browser:ktlintCheckand:boss-app-browser:detektpass on that base.
boss-app-browser) has no consumers outside itself except its ownmain()- it'sa standalone GraalVM-native process, not linked into composeApp - so its own full test suite
(
:boss-app-browser:test) is the right scope here rather than a full monorepo build.None. A
javascript:/data:URL now returnssuccess = falsewith an error message instead ofsuccess = true- any caller currently relying on those schemes "succeeding" was relying on thebug this fixes.
🔮 Future Considerations
None identified.
👀 Review Focus Areas
🚨 Pre-merge Checklist
Thank you for contributing to BOSS! 🎉