fix(security): authenticate marketplace writes with GitHub OAuth - #816

Logical verdict: REQUEST_CHANGES (submitted as COMMENT because the authenticated account is also the PR author).

I reviewed exact head 292057ebb79adadfffc9ca7948fc3bc12347e607 against beta, including all Marketplace call sites and the backend PR #7 compatibility boundary.

Blocking findings

1. [P1] Android persists the OAuth token as reversible Base64, not encrypted storage. credentials.rs:350-377 explicitly calls the Android format a stub, Base64-decodes/encodes the entire CredsRoot, and writes it to credentials.enc.json. This PR adds marketplace.githubAccessToken to that root, so any reader of the app data file can recover the GitHub bearer token. Android must use a real Keystore-backed implementation. Until that exists, an acceptable fail-secure fallback is to keep only the Marketplace token in process memory and require login again after restart; it must never enter credentials.enc.json.

2. [P1] Closing/cancelling device flow can still persist a token, and the device credential is exposed to TypeScript. GithubLoginModal.tsx:45-52 returns the raw deviceCode to JS; :77-105 only cancels the JS timer/result handling. If github_device_flow_poll is already in flight, Rust continues through github_oauth.rs:133-157 and writes the token after the user closes the modal. Keep the device code in Rust, return an opaque flow_id/generation, add cancel IPC, and atomically verify that the flow is current, unexpired, and not cancelled immediately before saving. Add a regression that pauses the mock /user response, cancels, resumes it, and proves no credential was written.

3. [P2] Device Flow ignores GitHub supplied polling interval and expiry. The start response exposes interval and expiresIn, but GithubLoginModal.tsx:47-52 drops both and :72-96 hard-codes 5 seconds with no deadline. GitHub requires the minimum interval returned by step 1 and adds 5 seconds after slow_down; polling ends when the device code expires. See https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps. Carry these values through the flow, enforce expiry in Rust as well as UI, and test interval, repeated slow_down, expiry, malformed responses, denial, and timeout.

4. [P2] Authenticated redirects and real HTTP/IPC behavior are not fail-closed or tested. Both GitHub /user and all Marketplace Bearer requests use the shared client, whose default follows redirects. The added tests only build a helper request and test status/serialization helpers; they do not execute the real upload/delete/like/my-likes/my-packs/OAuth paths, assert destination hosts, prove 3xx behavior, or prove token non-exposure through errors. Use a security-specific client/policy that rejects authenticated 3xx responses (at minimum never forwards credentials away from the exact GitHub/Marketplace origin), sanitize authenticated endpoint errors before IPC, and cover real mock HTTP endpoints including cross-origin redirects and 401/403/5xx behavior.

Additional correctness issue

After a 401 clears the vault token, prefs.marketplaceDevLogin remains stale; Marketplace.tsx:884-910 still renders the account chip from that cache, while :262-297 allows a logged-out like click and only rolls back after the Rust call fails. Treat vault status as authority for every write control, offer sign-in instead of optimistic mutation when logged out, and clear/reconcile legacy display metadata on credential invalidation/account switch.

Verified positive boundaries

• The access token is not part of the serialized poll result or CredentialsSnapshot.
• Desktop storage uses the existing OS keyring path and Debug redacts the new token wrapper.
• Upload, like/unlike-toggle, delete, my-likes, my-packs, and Style publishing all route through Rust Bearer auth; no production Marketplace call site sends X-Dev-User or X-Admin.
• Public list/detail/download/install remain anonymous, and no token is attached to the download URL.
• Exact-range gitleaks found no leak; npm audit --audit-level=high found 0; cargo audit found 0 vulnerabilities with 18 pre-existing allowed maintenance/unsoundness warnings.
• Linux, macOS, Android, and PR-agent checks were green when reviewed; Windows was still pending. Independent focused Rust compilation could not reach tests because this detached worktree does not contain the local untracked vendor/qwen-asr sources, then its dedicated target was removed.

Keep this PR Draft and blocked after the code fixes too: it must not merge or release until private backend PR #7 is independently approved, merged, securely deployed, and app/backend compatibility is validated against the live deployment.

Independent re-review — logical verdict: REQUEST_CHANGES

Reviewed exact head 9bb09a0 against PR #816 and rechecked every blocker from review 4692095378.

The OAuth flow itself is materially improved: TypeScript now receives only an opaque flow ID; Rust owns the raw device code; interval, repeated slow_down, expiry, cancellation and the in-flight cancel/save race are enforced; credential-bearing clients reject redirects; authenticated Marketplace calls use Bearer; stale frontend login/like state is cleared. Public list/detail/install remain anonymous through net::http at commands/marketplace.rs:159-163, 189-193 and 222-248.

[P1] Android legacy token scrubbing is not fail-closed across write/crash failures

persistence/credentials.rs:371-394 detects a legacy Base64 bearer and calls the replacement writer, but persistence/credentials.rs:403-417 removes the original file only after rename fails. A temporary-file write failure at line 410 returns immediately and leaves the original credentials.enc.json, including the recoverable token, untouched. There is no fsync stage; a crash after writing the temporary file but before rename likewise leaves the old token recoverable.

The actual Android Marketplace getter/setter at persistence/credentials.rs:1065-1085 only touches process memory and never triggers legacy scrubbing. The generic Android loader also caches an empty fallback after a scrub/read failure at persistence/credentials.rs:725-736, suppressing retries for the process. The only regression at persistence/credentials.rs:1280-1311 directly calls the helper and covers only the happy path.

Acceptance:

• Make the scrub path guarantee that a failure at temporary write, fsync, or rename cannot leave the old bearer recoverable; best-effort delete the legacy secret on every failure and define crash-safe ordering.
• Exercise the real startup or Marketplace getter/status path so scrubbing cannot depend on an unrelated generic credential read, and do not cache a fallback that suppresses retry after scrub failure.
• Add injected write, fsync, rename and crash-boundary regression cases, plus a real getter/startup-path test.

[P2] A rejected token can remain active for the rest of the process when vault deletion fails

commands/marketplace.rs:100-125 logs and ignores credential-clear failure after a 401. remove_marketplace_github_token clears a cloned root and calls save_credentials at persistence/credentials.rs:1094-1106, while save_credentials updates the process cache only after every durable keyring write succeeds at persistence/credentials.rs:809-873. If Keychain or another vault write fails, the old cached token remains. marketplace_auth_status then reports signed_in=true at commands/github_oauth.rs:500-505, and the next write reads and sends the rejected bearer again through commands/marketplace.rs:75-94.

Acceptance:

• Add a process-local rejected-token tombstone or equivalent invalidation that is committed before durable deletion and is authoritative for both marketplace_access_token and marketplace_auth_status.
• After a simulated vault-delete failure, auth_status must be false, display prefs must be cleared, and upload/like/delete/my-likes/my-packs must not send any request or bearer. Durable cleanup may be retried separately.

[P2] Marketplace coverage still constructs helpers instead of executing the real authenticated paths

commands/marketplace.rs:450-547 contains synchronous RequestBuilder/status-helper tests. It has no mock server, no .send path, and does not invoke upload, like, delete, my-likes or my-packs. Therefore the PR description claim of real HTTP/IPC coverage for Marketplace request shapes is not established.

Acceptance:

• Execute every authenticated endpoint against a local mock server and assert method/path, exactly one Bearer header, absence of X-Dev-User and X-Admin, and that public reads remain anonymous.
• Cover 3xx without redirect following, 401 cleanup, 403 and 5xx for each real command/send path.
• Assert that response bodies, bearer values and raw OAuth secrets never enter IPC-visible results or error strings. If Coordinator coupling prevents this, extract injectable send/orchestration helpers and test those actual helpers rather than rebuilding equivalent RequestBuilders in the test.

Independent release gate

Keep this app PR Draft. Backend PR Open-Less/openless-marketplace#7 is still Draft and its exact head 774b3a22e7b670227344ed1714a62c3b7c0b1268 currently has logical REQUEST_CHANGES review 4692388983 for an admin clickjacking blocker, with no CI checks. Even after the app findings are fixed, do not merge or release until the backend is independently approved, merged, securely deployed, and live compatibility is validated.

Independent checks on this head: TypeScript typecheck passed; npm audit reports 0 vulnerabilities; cargo audit reports 0 vulnerabilities with 18 allowed pre-existing warnings; gitleaks reports no leaks; git diff --check is clean. GitHub CI currently has Android, Linux, macOS and PR-Agent green; Windows is still running. I did not launch a separate cold cargo test build because this host had about 1.5 GiB free; the security and control-flow blockers above are directly evidenced in source and are not contingent on CI.

Because the authenticated account is also the PR author, GitHub does not allow a formal REQUEST_CHANGES review; this COMMENTED review is semantically blocking.

Independent security re-review — logical verdict: APPROVE

Reviewed exact head ed7ec896de3fe6adf4a6e3927193b3178fa8b8de against exact beta base 145a0ea17d94b93a178b54f92b74d20808ecbf3a, and rechecked every blocker from review 4692544338. I found no remaining blocking finding.

Previous blockers are resolved

• Android legacy credential scrubbing is fail-closed. The real getter/startup path goes through ensure_android_marketplace_legacy_scrubbed_at before reading process memory; scrub completion is recorded only after success, and load_android_credentials_into_cache_with does not cache a fallback after load failure. The legacy file is truncated and synced before removal/replacement, parent-directory durability is enforced, and every error path calls fail-closed cleanup. The I/O-fault, crash-boundary, real-getter retry, and startup retry regressions are at persistence/credentials.rs:1613-1724.
• A rejected token remains process-locally signed out even when durable deletion fails. The tombstone is set and process/Android token memory is cleared before durable removal. The getter honors the tombstone, auth status reports signed out, and a newly verified durable save is the only path that clears it. The regression at commands/marketplace.rs:857-901 exercises the production token resolver for all five authenticated endpoint variants after an injected deletion failure and proves zero listener accepts/network sends.
• The HTTP contract tests exercise the shared production executor. MarketplaceAuthenticatedEndpoint and execute_authenticated_marketplace_with are the same path used by upload, like/unlike, delete, my-likes, and my-packs. Tests at commands/marketplace.rs:717-852 cover exact method/path, exactly one Bearer header, like/unlike responses, 200/3xx/401/403/5xx, sanitized errors, and anonymous public list/detail/download. Only 401 clears auth. No authenticated endpoint follows redirects.

Additional security checks

• net::credential_http uses Policy::none(); the redirect regression proves credentials are not forwarded.
• OAuth device codes stay in Rust and redact under Debug; opaque flow IDs, polling interval/expiry/slow-down, cancellation, single consumption, and cancel-before-save are covered.
• Authenticated errors do not echo response bodies, request bodies, or tokens. gitleaks scanned all three PR commits / 98.47 KB with no leaks.
• The OAuth/auth-status/logout/cancel commands and all marketplace commands are wired in both desktop and mobile Tauri handlers.
• Backend gate comment 4967491126 is consistent with source-control and live evidence: backend PR #7 exact reviewed head 10f64f91... merged as c799a6a..., dependency PR #8 merged as 116acb7..., security checks succeeded, live /healthz returned 200, and forged user/admin plus wrong agent credentials each returned 401.

Independent verification on this head

• cargo test --locked --lib commands::github_oauth::tests -- --nocapture: 6 passed
• cargo test --locked --lib commands::marketplace::tests -- --nocapture: 4 passed
• cargo test --locked --lib persistence::credentials::tests::android_ -- --nocapture: 6 passed
• npm ci: 0 vulnerabilities; npm run build: passed
• git diff --check: passed
• Exact-head GitHub checks: Android cargo check, macOS, Windows, Linux, and PR-Agent are all SUCCESS

Because the authenticated account is the PR author, this is submitted as COMMENTED; semantically the verdict is APPROVE for merge readiness. This review does not merge, undraft, or deploy the PR.

Reviewed exact head 5637ced0998da504a4143b4284530849d65bcd3c against base c645b16851727bb09d3a5274854bc1277da4bd9e. I also range-diffed the previously approved ed7ec896de3fe6adf4a6e3927193b3178fa8b8de series against this rebased series, with special attention to the Marketplace conflict with #819.

Strengths

• The #819 bounded archive reader survived the rebase correctly: marketplace_install passes the response to read_marketplace_archive_response, which validates Content-Length, enforces the same limit while streaming, and uses the unique create_new/Drop temporary archive path.
• The earlier security blockers remain fixed: Android keeps the OAuth token process-memory-only and fail-closed scrubs legacy envelopes; OAuth uses opaque flow IDs and enforces cancellation, expiry, interval, and slow_down; credential-bearing GitHub/Marketplace calls use the no-redirect credential client; 401 publishes a process-local tombstone before durable deletion; authenticated endpoint tests cover real HTTP requests and redact errors.
• Frontend auth gating now derives capability from Rust vault state rather than the display-only login cache, and the frontend build succeeds.

Critical

None.

Important

1. Blocking — the public Marketplace executor is not no-redirect, despite the rebase resolution claiming that protection is preserved. execute_public_marketplace_with sends list/detail/download through net::http() (marketplace.rs:325-329). That client has no redirect policy (net.rs:23-34), and reqwest 0.12 defaults to Policy::limited(10); only net::credential_http() sets Policy::none() (net.rs:38-47). As a result, a 3xx from the Marketplace API is followed and accepted if the target returns 2xx, including for archive downloads. The current public endpoint test discards followed_redirect at marketplace.rs:954, so it cannot detect this.

   Please route public Marketplace requests through a dedicated anonymous no-redirect client (or an equivalent policy), reject 3xx explicitly, and add a real 302 test for list/detail/download that asserts the redirect target is never contacted. The archive size bound itself is intact; this request-source/redirect invariant is what is missing.

Minor

None.

Verification

• git diff --check c645b168...5637ced — passed
• cargo test --locked --lib commands::github_oauth::tests — 6 passed
• cargo test --locked --lib commands::marketplace:: — 7 passed
• cargo test --locked --lib persistence::credentials::tests::android_ — 6 passed
• npx tsx src/lib/githubOauth.test.ts — passed
• npm ci, npm run build, npm audit --audit-level=high — passed; 0 npm vulnerabilities
• cargo audit — exit 0; 18 allowed dependency warnings, with no Cargo.lock change in this PR
• gitleaks git --redact --log-opts='c645b168...5637ced' — no leaks
• Live backend: health/public list returned 200; unauthenticated, forged legacy/admin headers, and invalid Bearer /me/packs requests all returned 401
• Exact-head checks: macOS, Windows, Linux, Android, and PR-Agent all succeeded

Ready verdict

Not ready; logical REQUEST_CHANGES. Keep the PR in Draft. Once the public Marketplace no-redirect invariant and regression coverage are restored, this should be re-reviewed at the new exact head.

Independent security re-review — logical verdict: APPROVE

Reviewed exact head e3245bfa266f73611c57bdf04f390e977ce440a1 against the current beta tip 9991b325b8ea035ab167d486c5ba3c71688418a9. The conflict-free merge tree is 0d8912464634bb92457b71528615212dcb66ded7.

Critical

None.

Important

None.

Minor

None.

Security evidence

• The public marketplace executor now uses the dedicated anonymous client with redirects disabled and explicitly rejects every 3xx status. Production list, detail, download, and install paths all route through that executor.
• The regression test exercises real list, detail, and download requests through a 302 source and a live redirect target. It proves the target is never contacted; no Authorization, X-Dev-User, or X-Admin header is sent; and the fixed error does not disclose the Location value or response-body secret.
• Authenticated marketplace write paths retain the separate no-redirect credential client. Their real-request matrix covers success, redirect, 401, 403, and 500 behavior, including single Bearer injection, header hygiene, sanitized errors, and clearing credentials only on 401.
• OAuth lifecycle coverage still verifies opaque flow IDs, server-controlled timing/expiry, repeated slow_down, cancellation/generation leases, sanitized redirect/error behavior, and no token save after in-flight cancellation.
• Android token handling remains process-memory-only with fail-closed legacy-secret scrubbing, tombstone-first deletion, and retry/crash-boundary coverage. Durable-delete failure prevents all five authenticated marketplace operations from reaching the network.
• PR #819 ZIP/SSRF hardening remains intact: no-redirect marketplace transport, compressed-size/stream bounds, exclusive temporary archive creation with cleanup, and bounded style-pack import.

Verification

• Focused Rust suites: marketplace 8/8, OAuth 6/6, Android credentials 6/6, style-pack archive 4/4, style-pack 17/17.
• Full Rust library suite: 672/672 passed.
• cargo check --locked --lib, npm run build, all 9 standalone frontend test files, and current-beta macOS/Windows/Android CI contract checks passed.
• npm audit --audit-level=high: 0 vulnerabilities. cargo audit: 0 denied vulnerabilities (18 repository-allowed warnings). Gitleaks found no leaks across the PR's four commits. git diff --check passed.
• All five exact-head GitHub checks are green: Android cargo check, Linux checks, Windows checks, macOS checks, and pr_agent_job.

The GitHub CI run predates the two newest beta test-infrastructure commits, so I also ran the local validation above on the current merge tree. The PR remains a draft and was not merged. Because the authenticated account is the PR author, this is submitted as COMMENTED; the semantic review verdict is APPROVE.