fix(security): confine DownloadDataProvider open and reveal to tracked downloads

[Antriksh1984]

The gap

DownloadDataProvider.openFile(path) hands path to FileSystemUtils.openFile, which asks the OS to open it with its default application. For an executable, "open" and a double-click are the same action, and nothing on this path asks for consent. DownloadServiceBridge already documents exactly this hazard and closes it for the kernel-facing RPCs by confining both openFile and revealInFolder to a path the provider is currently tracking as a download.

The provider itself was never confined. PluginContext.downloadDataProvider is available to every installed plugin, and DownloadDataProviderImpl.openFile / revealInFolder accepted any string. So the confinement existed for a caller that had to be authenticated and did not exist for one that is already inside the process. A plugin (or a compromised one) could call openFile("C:\\path\\to\\payload.exe") and launch it, with no download involved.

Fix

The rule moves into DownloadDataProviderImpl, where the bridge and the in-process API meet it, so the two cannot disagree:

• openFile and revealInFolder act only on a path that is a tracked download's destinationPath, compared canonically (a .. segment or a link cannot name a tracked file from outside it).
• Anything else is refused and logged (Refused openFile: the path is not a tracked download).
• A blank path is never tracked, even when a download's own destination is blank.

The Downloads panel only ever passes destinationPath values it read from this same list, so nothing legitimate changes. The provider gets two constructor seams (opener, revealer, defaulting to the real FileSystemUtils calls) so the rule is testable without launching anything; the public no-argument constructor is unchanged. The comparison lives in an internal top-level isTrackedDownload rather than as members, to stay under the class's function ceiling.

Tests

DownloadOpenConfinementTest (5). Run against the provider with the seams added and no confinement (the previous behavior), 3 failed: an untracked path was opened and revealed, a .. path that resolves to an untracked file was opened, and blank paths were passed through. The two that pin the unchanged path (a tracked download opens and reveals; a tracked name with spaces and parentheses reaches the launcher) passed either way. After the change all 5 pass, and the existing DownloadDataProviderImplTest (11) still passes. ktlintCheck and detekt are clean.

One unrelated test in this module, DesktopFileScannerTest, fails on this Windows machine (A required privilege is not held by the client creating a symlink), and it does so without this change.

Not covered

• A tracked download that is itself an executable still opens on request: the download-time consent step (ExecutableDownloadConsent) is where that decision is made, and it has not changed.
• Symlink cases are covered by using canonical paths, but the test for a symlink pointing out of the tracked set needs a privilege this machine does not grant, so it is not in the suite.

[kshivang]

Thanks @Antriksh1984 for the focused provider-side hardening and the preserved contributor/test coverage. I reviewed the net two-file patch against the current dev; the large apparent diff is branch-history drift, not additional PR content. I also checked the merged overlap in #550: that established the same confinement at the IPC bridge, while this PR correctly closes the in-process DownloadDataProvider path.

I’m not marking this ready for merge yet because the provider-side guard reads _downloads.value, which is not the live tracked-download set. DownloadManager.downloads is explicitly .sample(150.milliseconds), and DownloadDataProviderImpl.removeDownload already documents that this snapshot can lag and therefore uses getDownload(id) instead. The new openFile/revealInFolder methods reintroduce that stale snapshot for the security decision:

• a newly added download can be refused until the sampled/collector snapshot catches up, even though the manager already tracks it;
• after removeDownload/clearCompleted, the stale snapshot can continue to authorize the old path briefly (and revealInFolder can still act on that path), which is the opposite direction from the intended “currently tracked” rule.

Please fix the guard to query the manager’s live map (for example, add a narrowly scoped live lookup/predicate by canonical destination path and use it from both methods), while retaining the blank-path rejection and canonical comparison. Please add focused regressions that (1) add a download and call the provider before the sampled flow emits, and (2) remove/clear a tracked download and assert both operations are refused immediately; keep the existing tracked, .., untracked, and blank-path cases. The blank-path test should also actually include a tracked item whose destinationPath is blank, since its current fixture only adds a nonblank item.

CI is green and I found no separate Claude review attached to #1407; the prior Claude feedback on the overlapping #550 was resolved there and specifically called out this sampled-snapshot limitation. Once the live-state fix and focused tests are in, please rerun the provider tests plus ktlintCheck/detekt. I’m converting this to draft so the issue stays visible while preserving your authorship and commit credit.

[Antriksh1984]

Fixed and force-pushed a clean rebuild on current dev.

DownloadManager.allDownloads() is a new live accessor (_downloadsMap.value.values.toList(), no sampling), the same way getDownload(id) already bypasses the throttled flow. openFile/revealInFolder now build their tracked-set from that instead of _downloads.value.

Added three tests: a download is trusted the instant addDownload returns, without waiting on the throttled flow; a download is refused the instant removeDownload runs, even after the throttled flow had caught up and showed it as tracked (so the test is racing against a snapshot that really did say "tracked"); the same for clearCompleted. All three fail on the unfixed code (checked by temporarily reverting to _downloads.value and rerunning). Also fixed the blank-destination test to actually include a tracked item with a blank destinationPath, per your note.

DownloadOpenConfinementTest (8 tests) and DownloadDataProviderImplTest (11 tests) pass; ktlintCheck/detekt clean.

[fluck-boss]

@claude please preliminary triage-review #1407 at exact head 2c6f494. Focus on the authors own commits, not the polluted dev compare. Compare current dev for supersession, flag correctness/test/security blockers, and suggest viable rebase, specific fixes, or closure recommendation. This is triage, not merge approval; full gate follows a clean rebase.

[claude]

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

---

Triage review of #1407 at 2c6f494

• Gather context (author's own commit vs polluted branch history)
• Check current dev for supersession
• Review correctness + security of the live tracked-set guard
• Review test veracity
• Post triage verdict

Verdict: keep it open, rebase it. The gap is real and still open on dev, the design is right, and the author correctly addressed the sampled-snapshot feedback. But the branch will not compile on current dev (the file it extends was rewritten), and there are two substantive fixes plus one test-veracity fix to make on the way through. Not a closure candidate; not mergeable as-is.

---

Scope: what is actually the author's work

The branch is 20+ unrelated commits deep (fix(test): await recent-page cleanup, fix(mcp): keep ledger telemetry ordered, …) — that is history drift, not PR content. The only commit that belongs to this PR is the tip:

2c6f494 fix(security): confine DownloadDataProvider open and reveal to tracked downloads, read live
  DownloadManager.kt                  +8/-0
  DownloadDataProviderImpl.kt        +70/-33
  DownloadOpenConfinementTest.kt    +216/-0

I reviewed that commit only.

Supersession check: not superseded

origin/dev (c3b2b5a) still has the unconfined provider:

// origin/dev:DownloadDataProviderImpl.kt
override fun revealInFolder(path: String) { FileSystemUtils.revealInFolder(path) }
override fun openFile(path: String) { FileSystemUtils.openFile(path) }

DownloadServiceBridge on dev has the confinement for the gRPC path only, over provider.downloads.value, and its own KDoc documents the staleness it accepts. So #550's overlap is exactly as the author and @kshivang described: the in-process PluginContext.downloadDataProvider path is still wide open on dev. The vulnerability this PR closes is live.

---

Blockers

1. allDownloads() does not compile against current dev, and the naive rebase is a data race.

DownloadManager was rewritten on dev (the mutationTick / sortedSnapshot change). The field this PR reads no longer exists:

branch base	current dev
private val _downloadsMap = MutableStateFlow<Map<String, DownloadItem>>(...) + Mutex	private val downloadsById = mutableMapOf<...>() + private val stateLock = Any() + mutationTick
fun getDownload(id) = _downloadsMap.value[id]	fun getDownload(id) = synchronized(stateLock) { downloadsById[id] }

So fun allDownloads(): List<DownloadItem> = _downloadsMap.value.values.toList() is an unresolved reference after rebase. The rebased form must take the lock, and that is not cosmetic — downloadsById is a plain mutableMapOf, so an unsynchronized values.toList() races every mutator and can throw ConcurrentModificationException or read a torn map. liveDownloads() is not inside a runCatching, so that throw would propagate straight out of openFile into the calling plugin. Correct rebase:

fun allDownloads(): List<DownloadItem> = synchronized(stateLock) { downloadsById.values.toList() }

Fix this →

2. The path that is validated is not the path that is launched.

openFile canonicalizes path to compare, then hands the caller's raw string to opener(path). The bridge deliberately does the opposite — trackedDownloadPathOrRefuse returns checkNotNull(requestedCanonical).path. Two consequences:

• A check-then-launch gap: an intermediate directory component of the raw path can be re-pointed (symlink swap) between the canonicalization and the exec, so the process that launches is not provably the file that was validated. Local-write-access only, so low severity — but the fix costs nothing.
• It keeps caller-controlled text on a cmd /c start command line. FileSystemUtils.openFile builds arrayOf("cmd", "/c", "start", "", file.absolutePath) (FileSystemUtils.kt:50), and Runtime.exec re-joins those into one command string on Windows. The canonical-equality guard does refuse a path carrying &-style payloads today, so this is defence-in-depth rather than an open hole — but launching the tracked item's own destinationPath removes the question entirely.

Strongest form: have the predicate return the matched tracked item (or its canonical file) and launch that, not the argument. Also note test the launcher gets the path as given by the tracked entry does not currently test what its name claims — it passes the caller's x/../a b (1).pdf string and then compares canonically, so it passes either way.

Fix this →

3. Two of the new race tests pass silently when their own premise fails.

awaitTracked (and providerTracking) poll for 5s and then fall through with no assertion:

while (provider.downloads.value.none { it.destinationPath == path } && System.currentTimeMillis() < deadline) {
    Thread.sleep(20)
}

The KDoc on a download is refused the instant it stops being tracked says the wait exists "so the removal below is a real race against a snapshot that still says tracked - not a case where the stale snapshot happened to be empty either way". If the flow never catches up (slow CI, a future change to the sampling), the wait expires, the premise evaporates, and the test goes green having tested nothing. Per this repo's own Test Veracity Rules, that needs an explicit assertTrue(...) / fail() on the deadline in both helpers.

Fix this →

---

Non-blocking, worth folding into the rebase

• The bridge and the provider still disagree, in the refusing direction. DownloadServiceBridge.trackedDownloadPathOrRefuse keeps guarding on provider.downloads.value (sampled). Post-PR the provider is the authority and the bridge can only be stricter, so a kernel caller can still be refused a just-added download. That is fail-closed and safe, but the PR body's "so the two cannot disagree" isn't quite true. The bridge's class KDoc also still says the rule lives there — it should now point at the provider as the authority and describe itself as the outer, snapshot-based check. (Sharing one predicate is awkward: the bridge holds the DownloadDataProvider interface, which has no live accessor.)
• The List<DownloadItemData> parameter re-exposes the exact seam that caused review round 1. An internal predicate taking a list invites the next caller to pass a stale snapshot again. A member isTrackedLive(path) (or a predicate taking the manager) makes the stale variant unrepresentable.
• liveDownloads allocates a full DTO per download per call just to read destinationPath, and that mapping is the only reason the two toData() converters had to be lifted from class members to file-level privates — roughly 45 of the 70 added lines and most of the rebase conflict surface. Taking List<DownloadItem> (or List<String> of destinations) removes the mapping, the move, and the churn.
• Test placement: ai.rever.boss.downloadopen vs the sibling DownloadDataProviderImplTest in ai.rever.boss.components.plugin.panels.left_top. Colocate.
• Provider instances are never disposed in the new tests — each leaks an eagerly-shared stateIn collector plus a DownloadManager scope. Harmless, tidy-up only.
• The "nothing legitimate changes" claim is not verifiable in this repo. After grepping, the only in-repo caller of DownloadDataProviderImpl.openFile/revealInFolder is DownloadServiceBridge; the Downloads panel lives in the browser plugin repo. Someone should check that plugin for an "open/reveal the downloads folder" action that passes a directory — that is now silently refused with only a logger.warn. If such an action exists, this is a behavioural break in an API 33 plugin repos consume, and it needs either an explicit allowance or a release note.
• Repo convention: an invariant of this shape ("both the bridge and the in-process provider confine to the live tracked set, and here is why it reads live") normally gets a short AGENTS.md paragraph. Worth adding while the reasoning is fresh.

What the author got right

• allDownloads() as a live accessor mirroring the existing getDownload(id) bypass is the correct answer to @kshivang's review, and the two staleness directions (refuse-a-fresh-download, authorize-a-removed-one) are both covered by tests that wait for the throttled flow to genuinely show "tracked" before racing it. That is the harder version of the test and the right one.
• The blank-destination fixture was fixed as asked (item("blank", "") is now actually tracked alongside a real one).
• Canonical comparison on both sides, blank rejected before canonicalization, runCatching around both canonicalFile calls, DownloadItem shape unchanged on dev so the fixtures still compile, and the added constructor params are defaulted so the existing DownloadDataProviderImplTest is untouched.

Suggested rebase

The PR content is three files and does not conflict semantically with anything on dev — only DownloadManager.kt conflicts textually. Cleanest path:

git fetch origin dev
git checkout -b fix/download-provider-open-confinement-v2 origin/dev
git cherry-pick 2c6f494        # resolve DownloadManager.kt per blocker 1
# then fold in blockers 2 and 3
./gradlew :composeApp:ktlintFormat
./gradlew :composeApp:desktopTest --tests '*DownloadOpenConfinementTest' --tests '*DownloadDataProviderImplTest' --tests '*DownloadServiceBridgeTest'
./gradlew ktlintCheck detekt

Include DownloadServiceBridgeTest in that run — it uses a stub provider so it should be unaffected, but it is the other half of this rule and is cheap to confirm.

I did not build or run tests here: this is a shallow checkout on the pre-rewrite base, so a compile would be testing code that cannot merge anyway. Blocker 1 is from reading origin/dev's DownloadManager.kt directly, not from a build.

Triage only — full gate after a clean rebase, as requested.
· branch fix/download-provider-open-confinement

[fluck-boss]

Preliminary triage on head 2c6f494: keep this provider confinement fix; current dev still allows the in-process download provider to open/reveal arbitrary paths. Re-cut the author commit onto current dev, where DownloadManager has a stateLock and downloadsById map rather than _downloadsMap; the live accessor must copy under that lock. Have the predicate return the matched tracked destination and launch that path, not the caller’s raw string. Make the 5-second test waits fail on timeout instead of passing without the race premise, and assert the exact launched tracked path. Claude’s current-head review above details the conflicts and tests. This is triage, not merge approval. Thanks @AntrikshSingh850.