fix(knowledge): require a direct local request for the native folder picker

[leonlaiyc]

Problem / Motivation

POST /api/knowledge/pick-folder opens a native macOS folder dialog on the
gateway host. Its gate is:

def _folder_picker_available(request: web.Request) -> bool:
    return sys.platform == "darwin" and bool(request.app.get("local_only", False))

local_only describes how the gateway was started, not where this request
came from — and those come apart in exactly the deployment this project ships.
dashboard/origin.py's own helper states why:

A loopback TCP peer alone is NOT sufficient: the gateway binds loopback and
remote access is delivered via a same-host tunnel or reverse proxy, so a
forwarded remote request also arrives from 127.0.0.1.

So on a local_only gateway published through tailscale serve, an AEA tunnel,
nginx or Caddy, a remote user's request satisfies this gate. The consequences
are all on the operator's machine, not the requester's:

• a native modal opens on the gateway operator's physical screen, put there
  by someone else;
• it blocks there for up to _FOLDER_DIALOG_TIMEOUT (180 s), and the handler
  holds an executor thread for the duration;
• whatever the operator picks — or is socially engineered into picking — is
  returned to the remote caller as an absolute host path, which is a small
  filesystem-layout disclosure even though add_source re-validates the path
  before using it.

The same gate also drives GET /api/knowledge/config's folder_picker flag, so
a remote dashboard currently advertises the button as available.

Why it matters

This is the unfixed sibling the review of #9233 counted. That PR added
exactly this predicate to the project picker in handlers/files.py, and its
review recorded:

Proxy-remote hardening via is_direct_local_request — justified; sibling
knowledge._folder_picker_available (checks only local_only) lacks it

Two host-side native dialogs, one hardened and one not, is the state that helper
exists to prevent. Fixing only the first left the same capability reachable one
route over.

The scope is deliberately not overstated: reaching the endpoint still requires a
valid dashboard token. This closes the gap between "a token holder on the far
end of the tunnel" and "someone sitting at the machine", which is the same
boundary the is_direct_local_request docstring says it exists to hold — it is
not, and does not claim to be, protection against a host-level actor.

What changed (motivation → approach → change)

Root cause: a per-deployment flag was standing in for a per-request property.

• _folder_picker_available gains is_direct_local_request(request) as a
  third conjunct — loopback peer and no forwarding headers. One import, one
  early return, one call.
• local_only is kept, not replaced. It is still the right switch for
  "this deployment offers host-side dialogs at all", and dropping it would
  widen the gate on a gateway deliberately started without it. The check order
  also preserves the existing fail-closed behaviour when local_only is unset.
• The comment points back at feat(dashboard): native folder picker for project selection #9233, so the pair stays visible to whoever
  touches either next.

Behaviour is unchanged for a genuine local browser, which is loopback with no
forwarding headers — pinned by a test that flips only that one header.

Deliberately not widened. No other local_only reader is touched. Most are
correct as they stand (they gate deployment capability, not a host-side side
effect), and auditing all of them is a different change from fixing the sibling a
reviewer already counted.

Tests

Extended TestFolderPickerAvailable in test/test_knowledge_add_source.py.

The existing _fake_request helper carried only app, which is no longer enough
— the real predicate reads request.remote and request.headers. Rather than
patch is_direct_local_request out (which would assert the gate against a mock
of itself), the fixture now supplies a genuine loopback peer and real headers,
so the production predicate actually runs and the new cases flip it by changing
one field.

Red-before (production file reverted to origin/main, tests kept):

FAILED ...::TestFolderPickerAvailable::test_unavailable_when_a_proxy_forwarded_the_request
  AssertionError: assert True is False
FAILED ...::TestFolderPickerAvailable::test_unavailable_when_the_peer_is_not_loopback
  AssertionError: assert True is False
FAILED ...::TestFolderPickerAvailable::test_the_forwarding_header_is_what_flips_it
  AssertionError: assert True is False

assert True is False is the defect stated plainly: the gate reports the host
dialog as available to a proxied remote request.

• test_unavailable_when_a_proxy_forwarded_the_request — the reported case: a
  loopback peer carrying X-Forwarded-For.
• test_unavailable_when_the_peer_is_not_loopback — a directly-bound remote
  peer, which local_only alone also failed to exclude.
• test_the_forwarding_header_is_what_flips_it — guard the guard: asserts the
  allowed and denied requests differ only by that one header, so the denial
  cannot be passing because the fixture is malformed.

All four pre-existing tests in the class are unchanged and still pass, including
test_fail_closed_when_local_only_unset, which passes a bare SimpleNamespace
with no peer at all — the check order keeps that reaching False before the new
predicate is consulted.

Green-after: 417 passed / 2 skipped across
test_knowledge_add_source.py, test_dashboard_origin.py, test_knowledge.py
and test_knowledge_budget.py.

Gates: flake8, isort --check-only, mypy --platform linux and
scripts/check_black_formatting.py all pass. The one black finding in
test_knowledge_add_source.py is a pre-existing baseline entry in a region this
PR does not touch, so the file was not reformatted.

Manual verification

N/A — unit coverage sufficient: the gate is a pure predicate over
request.remote and request.headers, and the tests drive the real predicate
with the exact peer/header combinations a tunnel and a direct browser produce.
Reproducing it by hand would need a macOS host behind a reverse proxy, and would
assert nothing these do not.

Related Issues

None filed. Found by mining merged-PR review threads for counted-but-unfixed
siblings; this one is named explicitly in the review of #9233.

Contention checked immediately before opening: handlers/knowledge.py is touched
by 5 open PRs (#2937, #8345, #8985, #9222, #9333), and none of them has a hunk
in the _folder_picker_available / pick_folder region — the nearest are at
lines ~519–661 and ~919+.

Pattern harvest

Rule candidate: review-prompt

Pattern: a deployment flag is not a request property. local_only answers
"how was this gateway started", and it was being used to answer "is the person
making this request at the keyboard". Those diverge the moment a tunnel or
reverse proxy is in front — which is not an edge case here, it is the shipped
remote-access design, and it is why is_direct_local_request exists. Any gate
protecting a host-side side effect (a native dialog, a local file write, a
desktop notification, a shell) needs the per-request check; the deployment flag
can only decide whether the capability is offered at all.

Second lesson: when a shared predicate is introduced to fix one call site,
the other call sites of the old weaker check are the change's real scope.
#9233's review counted this sibling correctly, and it stayed open because
counting is not fixing. A helper added to close a class leaves the class open
until every member is migrated — which argues for landing the sibling in the
same cycle, or filing it, rather than only naming it in a review thread.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

🤖 Generated with Claude Code

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed cdc2a7983355b08b52af8bc76b0a3cb855f64fdd via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] cdc2a79

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of cdc2a7983355b08b52af8bc76b0a3cb855f64fdd via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The change is verified against the base: is_direct_local_request exists in dashboard/origin.py:90 with exactly the rationale the PR quotes, and handlers/files.py:176 already gates its own host-side native action (Finder reveal / project picker from #9233) on the same predicate. The PR applies the identical, fail-closed pattern to the one remaining host-side dialog, keeps local_only as the outer deployment switch, and the folder_picker flag in /api/knowledge/config flips consistently so a remote dashboard stops advertising the button. The predicate itself is platform-independent and the tests exercise it on CI by monkeypatching sys.platform, so the "Manual verification: N/A" claim is actually covered by unit tests rather than untested platform branching. No scope creep, no contract break, trivially reversible.

Design-Verdict: PASS

Closes a counted, real remote-reaches-host-dialog gap with the exact fail-closed predicate its sibling already uses — right layer, minimal surface, tested red-before.

[DESIGN-REVIEWED] cdc2a79

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed cdc2a7983355b08b52af8bc76b0a3cb855f64fdd via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] cdc2a79

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of cdc2a7983355b08b52af8bc76b0a3cb855f64fdd via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks complete. The fix itself is verified against the base: the weak gate exists at knowledge.py:831, the helper and its documented tunnel/proxy premise exist at origin.py:90, the new tests verifiably fail on base by reading the base predicate, and the description's claim that no other local_only handler gate exists holds (only knowledge.py:831). But the class the PR itself names — host-side native dialogs — has one more member: POST /api/upload (files.py:918, routed at routes/taskrunner.py:54) spawns the same osascript Finder dialog with only a platform check, no locality gate at all. That's a counted unfixed sibling, which per the contract is CONCERNS, not BLOCK.

First-Principles-Verdict: CONCERNS

The class this PR names stays open by one member: POST /api/upload (files.py:918) spawns the same native dialog with no locality gate at all.

What this change ships

Inventory (3 items)

Intent: stop a proxied/tunneled remote token-holder from popping a native folder dialog on the gateway operator's screen — a FIX.

1. A proxied or non-loopback request can no longer open the native folder picker (403 → typed-path fallback) — justified
2. A remote dashboard's folder_picker config flag now reports false, hiding the button — justified
3. Picker-gate tests now drive the real locality predicate with a genuine peer and headers — justified

Watch

• Point patch on a counted class. Grepped osascript|choose folder|NSOpenPanel|tkinter under src/kiro_crew/dashboard/: 2 dialog-spawn sites. This PR guards knowledge.py:839; files.py:924 (api_upload, routed at routes/taskrunner.py:54) is gated only on sys.platform == "darwin" — no local_only, no is_direct_local_request, though files.py:55 already imports the helper. The description's scope audit ("No other local_only reader is touched") scanned the wrong axis: the root cause is host-side dialogs, not local_only readers, and its own harvested pattern ("a helper added to close a class leaves the class open until every member is migrated") indicts this member.
  Clears when: api_upload gains the same gate in this PR or a filed issue names it.

[FIRST-PRINCIPLES-REVIEWED] cdc2a79