fix(desktop): validate generic oauth token responses

[tt-a1i]

What this PR does

• Rejects generic OAuth exchange responses that do not include a non-empty access_token.
• Reuses the same non-empty access_token validation for generic OAuth refresh responses.
• Parses expires_in from JSON and form-encoded token responses without partial parsing.
• Preserves expires_in: 0 as an immediate expiry instead of treating it as missing.
• Adds focused coverage for generic OAuth exchange and refresh token responses.

Why it's needed

SourceCredentialManager.exchangeAndStore() saves result.accessToken! after a successful OAuth exchange. The generic OAuth exchange path could return success: true even when the token response was missing access_token, which could mark a source authenticated with an invalid credential.

The generic OAuth path also used parseInt for expires_in, so malformed values like "3600abc" were accepted as 3600. Token lifetimes should only accept whole, safe, non-negative seconds.

Reviewer Test Plan

How to verify

• Review exchangeGenericOAuth: missing or empty access_token should return success: false.
• Review refreshGenericOAuthToken: missing or empty access_token should still throw.
• Review expires_in parsing: JSON numbers and form-encoded strings should be accepted only when they are safe non-negative integers.
• Review expires_in: 0: it should produce an immediate expiresAt, not undefined.
• Run bun test packages/desktop/packages/shared/src/auth/__tests__/generic-oauth.test.ts from the repo root.
• Run bun run typecheck:shared from packages/desktop.
• Run npx prettier --check packages/desktop/packages/shared/src/auth/generic-oauth.ts packages/desktop/packages/shared/src/auth/__tests__/generic-oauth.test.ts.
• Run git diff --check.

Evidence (Before & After)

Before: a generic OAuth token exchange response like {"refresh_token":"r"} could be treated as successful, and expires_in: "3600abc" was accepted as 3600. After: missing access_token fails exchange/refresh, malformed expires_in is rejected, and valid JSON/form-encoded token responses keep working.

Tested on

OS	Status
🍏 macOS	✅ tested
🪟 Windows	⚠️ not tested locally; covered by CI
🐧 Linux	⚠️ not tested locally; covered by CI

Environment (optional)

Local desktop workspace after bun install --cwd packages/desktop --frozen-lockfile; root npm ci --ignore-scripts was used for tooling.

Risk & Scope

• Main risk or tradeoff: malformed generic OAuth token responses that were previously tolerated are now rejected.
• Not validated / out of scope: changing provider-specific Google, Slack, Microsoft, or MCP OAuth flows.
• Breaking changes / migration notes: generic OAuth providers must return a non-empty access_token; expires_in, when present, must be a whole non-negative second count.

Linked Issues

Fixes #5510

中文说明

What this PR does

• generic OAuth exchange response 缺少非空 access_token 时直接拒绝。
• generic OAuth refresh response 复用同一套非空 access_token 校验。
• 对 JSON 和 form-encoded token response 的 expires_in 做严格解析，不再部分解析。
• 保留 expires_in: 0 为立即过期，而不是当成缺失。
• 增加 generic OAuth exchange/refresh token response 的聚焦测试。

Why it's needed

SourceCredentialManager.exchangeAndStore() 会在 OAuth exchange 成功后保存 result.accessToken!。generic OAuth exchange 路径以前即使 token response 缺少 access_token，也可能返回 success: true，从而把 source 标记为已认证并保存无效 credential。

generic OAuth 还用 parseInt 解析 expires_in，所以 "3600abc" 这类异常值会被当成 3600。token lifetime 应该只接受安全、非负、完整的秒数整数。

Reviewer Test Plan

How to verify

• 检查 exchangeGenericOAuth：缺少或为空的 access_token 应该返回 success: false。
• 检查 refreshGenericOAuthToken：缺少或为空的 access_token 仍应该抛错。
• 检查 expires_in 解析：JSON number 和 form-encoded string 只有在安全非负整数时才接受。
• 检查 expires_in: 0：应该生成立即过期的 expiresAt，而不是 undefined。
• 运行英文部分列出的测试、typecheck、prettier 和 git diff --check。

Evidence (Before & After)

修复前：{"refresh_token":"r"} 这类 generic OAuth exchange response 可能被当作成功，expires_in: "3600abc" 会被当成 3600。修复后：缺少 access_token 的 exchange/refresh 会失败，格式错误的 expires_in 会被拒绝，合法 JSON/form-encoded token response 继续可用。

Tested on

OS	Status
🍏 macOS	✅ tested
🪟 Windows	⚠️ not tested locally; covered by CI
🐧 Linux	⚠️ not tested locally; covered by CI

Environment (optional)

本地 desktop workspace，已运行 bun install --cwd packages/desktop --frozen-lockfile；为了 tooling 也运行了 root npm ci --ignore-scripts。

Risk & Scope

• 主要风险或取舍：以前被容忍的 malformed generic OAuth token response 现在会被拒绝。
• 未验证 / 不在范围内：修改 Google、Slack、Microsoft 或 MCP 这些 provider-specific OAuth flow。
• Breaking changes / migration notes：generic OAuth provider 必须返回非空 access_token；如果返回 expires_in，必须是完整的非负秒数整数。

Linked Issues

Fixes #5510

AI Assistance Disclosure

I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.

[wenshao]

📌 Same maintainer note applies here — full version on #5509.

#5507, #5509, and this PR are the desktop arm of the same fix series (path-boundary / strict-parse / validation). Please fold them into a single "desktop hardening" PR where it makes sense, and reuse existing shared helpers rather than adding new per-file copies (duplication list is in the #5509 comment). The empty-access_token rejection here looks reasonable on its face — this is purely about grouping + DRY, not correctness.

Thanks! 🙏

中文说明

📌 同一条维护者说明也适用于此 —— 完整版见 #5509。

#5507、#5509 和本 PR 是同一系列修复在 desktop 端的分支(路径边界 / 严格解析 / 校验)。请在合适处把三个合成一个 "desktop hardening" PR,并复用已有共享 helper,不要每个文件再加一份新副本(重复清单见 #5509 的评论)。这里对空 access_token 的拒绝本身看起来是合理的 —— 这条纯粹是关于归并 + DRY,不涉及正确性。

谢谢!🙏

[wenshao]

@qwen-code /triage

[qwen-code-ci-bot]

Thanks for the PR!

Template looks good ✓ — all required sections present, bilingual, clear test plan.

On direction: this is a straightforward correctness/security fix — exchangeGenericOAuth silently succeeding with a missing access_token is a real bug, and parseInt("3600abc") === 3600 is a real parsing laxity. Squarely within scope. The linked issue (#5510) describes the problem accurately.

On approach: the scope feels right for what it claims — +212/-13 across 2 files, focused entirely on OAuth token response validation. New helpers (readStringField, readAccessToken, parseExpiresIn, expiresAtFromSeconds) are small and private to the file. The parseTokenResponse return type widening from Record<string, string> to Record<string, unknown> is the correct move since JSON responses can have non-string values like expires_in: 3600.

One note: the existing maintainer comment on this PR already flags the consolidation ask across #5507/#5509/#5511. That's a process concern, not a correctness one — this PR stands on its own merits. Whether to merge it separately or fold it into a broader desktop-hardening PR is a maintainer call.

Moving on to code review. 🔍

中文说明

感谢贡献！

模板完整 ✓ —— 所有必填段落齐全，双语，测试计划清晰。

方向：这是一个直接的正确性/安全修复 —— exchangeGenericOAuth 在缺少 access_token 时仍然返回成功是一个真实的 bug，parseInt("3600abc") === 3600 是真实的解析宽松问题。完全在范围内。关联的 issue (#5510) 准确描述了问题。

方案：对于所声称的目标来说范围合理 —— 2 个文件，+212/-13，完全聚焦于 OAuth token response 校验。新增的 helper（readStringField、readAccessToken、parseExpiresIn、expiresAtFromSeconds）都是文件内私有小函数。parseTokenResponse 返回类型从 Record<string, string> 放宽到 Record<string, unknown> 是正确的，因为 JSON response 中可以有非字符串值（如 expires_in: 3600）。

注意：此 PR 上已有的维护者评论提出了 #5507/#5509/#5511 合并的要求。这是流程上的考虑，不是正确性问题 —— 本 PR 本身是有价值的。是单独合并还是并入更广泛的 desktop-hardening PR，由维护者决定。

进入代码审查 🔍

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Code Review

Before reading the diff, my independent proposal for fixing "generic OAuth accepts malformed token responses" would have been:

1. After parsing the token response, add a non-empty-string check on data.access_token before returning success.
2. Replace parseInt(data.expires_in, 10) with a stricter parser that only accepts safe non-negative integers — either a number type passing Number.isSafeInteger, or a string matching /^\d+$/.
3. Fix the expires_in: 0 bug — the old data.expires_in ? parseInt(...) : undefined treats 0 as falsy, silently dropping an explicit immediate-expiry signal.
4. Add tests covering all the above.

The PR's approach matches and exceeds this baseline. The helpers are small, focused, and private to the file. The parseTokenResponse return type widening from Record<string, string> to Record<string, unknown> is the right call — JSON responses can contain non-string values like expires_in: 3600. The String() coercion on error fields is defensive and correct. The expiresAtFromSeconds helper cleanly separates the time computation from the parsing logic.

No critical blockers. No AGENTS.md violations. The code is minimal, focused, and does exactly what the PR says it does.

Testing

This PR modifies internal OAuth token response validation — not user-facing TUI behavior. There's no way to drive OAuth token exchange through the CLI without a real OAuth provider, so unit tests and type/lint checks are the appropriate verification here.

=== PR #5511: Generic OAuth token response validation ===

--- Running unit tests ---
bun test v1.3.14 (0d9b296a)

✓ generic OAuth token responses > exchanges JSON token responses with numeric expires_in [0.77ms]
✓ generic OAuth token responses > exchanges form-encoded token responses with string expires_in [0.25ms]
✓ generic OAuth token responses > rejects exchange responses without access_token [0.13ms]
✓ generic OAuth token responses > rejects exchange responses with malformed expires_in [0.18ms]
✓ generic OAuth token responses > rejects unsafe JSON number expires_in values [0.15ms]
✓ generic OAuth token responses > preserves zero-second expiries instead of treating them as missing [0.13ms]
✓ generic OAuth token responses > rejects refresh responses without access_token [0.26ms]
✓ generic OAuth token responses > refreshes JSON token responses with refresh_token and numeric expires_in [0.14ms]
✓ generic OAuth token responses > preserves zero-second expiries on refresh responses [0.13ms]
✓ generic OAuth token responses > rejects refresh responses with malformed expires_in [0.11ms]

 10 pass
 0 fail
 25 expect() calls
Ran 10 tests across 1 file. [50.00ms]

--- Running typecheck ---
$ cd packages/shared && bun run tsc --noEmit

--- Running prettier check ---
Checking formatting...
All matched files use Prettier code style!

=== All checks complete ===

All green — 10/10 tests pass, typecheck clean, prettier clean.

中文说明

代码审查

在读 diff 之前，我对"generic OAuth 接受格式错误的 token response"这个问题的独立方案是：

1. 解析 token response 后，对 data.access_token 做非空字符串检查再返回成功。
2. 用更严格的解析器替换 parseInt(data.expires_in, 10) —— 只接受安全的非负整数（number 类型通过 Number.isSafeInteger，或字符串匹配 /^\d+$/）。
3. 修复 expires_in: 0 的 bug —— 旧的 data.expires_in ? parseInt(...) : undefined 把 0 当 falsy，丢弃了明确的立即过期信号。
4. 添加覆盖以上场景的测试。

PR 的方案与基线一致并有所超越。helper 函数小而专注，文件内私有。parseTokenResponse 返回类型从 Record<string, string> 放宽到 Record<string, unknown> 是正确的 —— JSON response 中可以有非字符串值。错误字段上的 String() 强制转换是防御性且正确的。expiresAtFromSeconds 干净地把时间计算与解析逻辑分开。

无关键阻塞。无 AGENTS.md 违规。代码最小、聚焦，完全实现了 PR 所声称的功能。

测试

本 PR 修改的是内部 OAuth token response 校验逻辑，非用户可见的 TUI 行为。无法在没有真实 OAuth provider 的情况下通过 CLI 驱动 OAuth token exchange，因此单元测试和类型/lint 检查是合适的验证方式。

全部通过 —— 10/10 测试通过，typecheck 干净，prettier 干净。

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Stepping back to look at the whole picture.

This is a clean, focused security fix. The bug is real — exchangeGenericOAuth could return success: true with a missing access_token, and parseInt silently accepted "3600abc" as 3600. The fix is minimal (2 files, no scope creep), the helpers are well-named and do one thing each, and the tests cover all the edge cases I'd want to see (missing token, malformed expires_in, unsafe integers, zero-second expiry, form-encoded fallback, refresh path).

My independent proposal before reading the diff was essentially the same approach — the PR didn't miss anything obvious, and it added a couple of niceties I wouldn't have thought of (the expiresAtFromSeconds separation, the String() coercion on error fields).

The existing maintainer comment flags consolidation across #5507/#5509/#5511 as a process preference. That's fair — but it's a grouping decision, not a code quality concern. This PR stands on its own merits.

Approving. ✅

中文说明

退后一步看全貌。

这是一个干净、聚焦的安全修复。bug 是真实存在的 —— exchangeGenericOAuth 可能在缺少 access_token 的情况下返回 success: true，parseInt 默默接受了 "3600abc" 作为 3600。修复最小化（2 个文件，无范围蔓延），helper 命名合理且各司其职，测试覆盖了所有我想看到的边界情况（缺少 token、格式错误的 expires_in、不安全整数、零秒过期、form-encoded 回退、refresh 路径）。

读 diff 前我的独立方案基本是一样的思路 —— PR 没有遗漏明显的东西，还加了一些我不会想到的细节（expiresAtFromSeconds 的分离、错误字段上的 String() 强制转换）。

已有的维护者评论提出了 #5507/#5509/#5511 合并的流程偏好。这合理 —— 但这是归组决定，不是代码质量问题。本 PR 本身是有价值的。

批准 ✅

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅