对齐凭据存储策略与威胁模型#277
Conversation
PR Reviewer Guide 🔍(Review updated until commit 0505c2f)Here are some key observations to aid the review process:
|
H-Chris233
force-pushed
the
fix/issue-230-credential-storage-model
branch
from
e7beea5 to
6296e3d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6296e3d0ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let json = serde_json::to_string(&cleaned).context("encode credentials failed")?; | ||
| keyring_entry()? | ||
| .set_password(&json) |
There was a problem hiding this comment.
On Windows this set_password call writes the entire multi-provider JSON into a single Windows Credential Manager blob; that backend is limited to 2560 bytes per credential blob. Once a user has configured several LLM/ASR presets, long API keys, or extra headers, the serialized root can exceed that limit and every subsequent credential save/migration fails with a vault write error, leaving the settings page unable to persist credentials. Splitting accounts into separate keyring entries or using a Windows-safe chunking strategy avoids this Windows-only regression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
已处理：保留逻辑上的 provider JSON payload，但实际写入系统凭据库时改为 manifest + bounded chunks，避免 Windows Credential Manager 单条 credential blob 限制。这里只兼容本 PR 之前的明文 credentials.json 迁移，不兼容本 PR 过程中未发布过的单条 keyring JSON 临时格式。验证：cargo check、目标单测、npm build、git diff --check。
|
Persistent review updated to latest commit 6296e3d |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Persistent review updated to latest commit 0a1bdf2 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a83a8d2f94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn load_keyring_credentials() -> Result<Option<CredsRoot>> { | ||
| let Some(json_or_manifest) = get_keyring_password(KEYRING_CREDENTIALS_ACCOUNT)? else { | ||
| return Ok(None); | ||
| }; |
There was a problem hiding this comment.
When upgrading an install that already has Keychain/credential-vault entries under the documented CredentialAccount names (for example ark.api_key) but no new credentials.v1 manifest yet, this returns Ok(None) and load_credentials falls through to the legacy JSON/default path. That silently ignores the existing vault credentials even though CredentialAccount::keyring_account still advertises those accounts as upgrade-readable, so affected users have to re-enter provider settings. Please add a fallback that reads the old per-account vault entries and writes the new chunked manifest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
已处理：没有 credentials.v1 manifest 时会先读取旧 per-account vault entries（如 ark.api_key / volcengine.app_key / asr.model），组装为当前 CredsRoot，成功写入新的 chunked manifest 后清理旧 entries。另将 chunk 写入改为 generation 方案，先写新 generation chunks，再切换 manifest，降低 partial write 混合旧/新 chunk 的风险。验证：cargo check、目标单测、npm build、git diff --check。
|
Persistent review updated to latest commit a83a8d2 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 508eaf40a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let legacy_vault = load_legacy_keyring_credentials(); | ||
| if !legacy_vault.providers.asr.is_empty() || !legacy_vault.providers.llm.is_empty() { | ||
| if let Err(e) = save_credentials(&legacy_vault) { | ||
| log::warn!("[vault] legacy vault credential migration failed: {e}"); | ||
| } else { | ||
| remove_legacy_keyring_credentials(); | ||
| } | ||
| return legacy_vault; |
There was a problem hiding this comment.
When an existing Tauri user has the current %APPDATA%/OpenLess/credentials.json or ~/.openless/credentials.json plus leftover per-account Keychain entries from an older Swift install, this branch migrates and returns the old vault values before ever reading the plaintext JSON that the previous release actually used. That can replace the user's current multi-provider settings with stale Swift-era credentials and make ASR/LLM appear unconfigured or use old keys; this is fresh evidence after the earlier migration comment because the new fallback now exists, but it runs before the legacy JSON source.
Useful? React with 👍 / 👎.
| Err(e) => { | ||
| log::warn!("[vault] read {} failed: {}", path.display(), e); | ||
| return CredsRoot::default(); | ||
| log::warn!("[vault] system credential read failed: {e}"); | ||
| return load_legacy_credentials().unwrap_or_default(); |
There was a problem hiding this comment.
If a chunked vault payload exists but one chunk is missing or temporarily unreadable, this error path returns the legacy JSON/default root; the mutating callers (set, remove, set_active_*) then save that partial/default root as a new manifest, which can permanently discard the rest of the user's credentials. In this scenario the read failure should be propagated to write callers rather than treated as an empty credential set.
Useful? React with 👍 / 👎.
|
Persistent review updated to latest commit 508eaf4 |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Persistent review updated to latest commit 3b15c21 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76c1ef63f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [target.'cfg(all(unix, not(target_os = "macos")))'.dependencies.keyring] | ||
| version = "3.6.3" | ||
| default-features = false | ||
| features = ["linux-native"] |
There was a problem hiding this comment.
When this is built for the Linux target, features = ["linux-native"] selects keyring's keyutils-only store; the keyring docs describe that store as in-memory and not persistent across reboots (https://docs.rs/keyring/3.6.3/keyring/keyutils/). Since this PR now removes the legacy JSON file after a successful vault write, Linux users can lose all configured provider credentials after reboot/key expiry with no fallback to reload them; use one of the persistent Secret Service-backed features or keep a Linux reload fallback before deleting plaintext.
Useful? React with 👍 / 👎.
|
Persistent review updated to latest commit 76c1ef6 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1536d5fe0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match get_keyring_password(legacy_account) { | ||
| Ok(Some(value)) => write_account(&mut root, *account, Some(value)), | ||
| Ok(None) => {} | ||
| Err(e) => log::warn!("[vault] read legacy vault {legacy_account} failed: {e}"), |
There was a problem hiding this comment.
When upgrading an install with no chunked manifest and no plaintext JSON fallback, this branch logs a read failure for one old per-account vault entry and keeps migrating any other entries that did read successfully; migrate_legacy_sources_for_update then saves that partial root and calls remove_legacy_keyring_credentials(), which can delete the unread legacy secret. In the scenario where one Keychain/Credential Manager entry is temporarily inaccessible or corrupt but another entry is readable, this silently drops the failed entry instead of preserving the old vault state for a retry.
Useful? React with 👍 / 👎.
|
Persistent review updated to latest commit 1536d5f |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
H-Chris233
force-pushed
the
fix/issue-230-credential-storage-model
branch
from
b6f92ed to
b5f281d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5f281da35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| remove_legacy_credentials_file_best_effort(); |
There was a problem hiding this comment.
When a Windows dev/test machine has been migrated, this removes %APPDATA%\OpenLess\credentials.json after the first successful vault save. I checked openless-all/app/scripts/windows-smoke-suite.ps1: it still runs windows-runtime-smoke.ps1 with RequireCredentials = true, and that script only checks credentials.json before launching the app, so the default smoke suite fails even though the credentials now live in Credential Manager. Please update the smoke scripts to query the vault or stop requiring the legacy JSON file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
已处理：\n- windows-smoke-suite 不再默认传入 RequireCredentials，避免迁移后因缺失 legacy credentials.json 直接失败。\n- windows-runtime-smoke / windows-real-regression 在 RequireCredentials 分支改为 warning（不阻断），明确说明当前凭据来源为 OS credential vault。\n\n提交：c06db84\n验证：git diff --check（通过）；脚本冲突标记检查（通过）。
Provider credentials should follow the platform credential-store threat model instead of continuing normal plaintext JSON writes. The vault now stores the existing multi-provider credentials payload in the OS credential store, migrates the legacy JSON file only when the vault has no entry, removes the legacy file after a successful write, and keeps raw credential IPC restricted to the main settings window. Constraint: User explicitly chose Keychain/Credential Manager/Linux keyring for issue Open-Less#230. Constraint: Preserve the existing provider schema and active-provider routing with minimal frontend churn. Rejected: Keep plaintext JSON as the documented product choice | user rejected this direction. Rejected: Split every credential field into separate vault entries | larger migration surface and risks losing active provider state. Confidence: medium Scope-risk: moderate Directive: Security-model and storage-strategy choices must be confirmed with the user before implementation. Tested: cargo check --manifest-path src-tauri/Cargo.toml; npm run -s build; git diff --check; grep for stale plaintext/Keychain fallback wording. Not-tested: Manual migration on macOS Keychain, Windows Credential Manager, or Linux desktop keyring. Related: Open-Less#230
The OS vault migration keeps provider state as one logical JSON payload, but Windows Credential Manager limits each credential blob. Store the logical payload as a manifest plus bounded chunks so long API keys, multiple providers, or extra headers do not make future saves fail on Windows. Constraint: Windows Credential Manager generic credential blob is capped and keyring stores passwords as UTF-16 on Windows. Rejected: Split each provider field into independent vault entries | broader schema rewrite and more migration surface than needed for this review fix. Confidence: high Scope-risk: narrow Directive: Keep the legacy plaintext credentials.json path migration-only; do not add compatibility for transient PR-internal vault formats unless the release shipped them. Tested: cargo check --manifest-path src-tauri/Cargo.toml Tested: cargo test --manifest-path src-tauri/Cargo.toml credential_payload_chunks_stay_under_windows_blob_limit Tested: npm run -s build Tested: git diff --check Not-tested: Live Windows Credential Manager write with oversized real provider payload. Related: Open-Less#230
Users upgrading from the plaintext credentials file should not lose access to existing settings just because the OS vault backend is temporarily unavailable or migration cannot be written. Keep the old file migration-only, but return its contents as the compatibility source when vault reads or writes fail. Constraint: credentials.json remains a legacy compatibility source only, not the normal write target. Rejected: Return defaults on vault read/write errors | hides existing upgrade data and makes configured providers appear empty. Confidence: high Scope-risk: narrow Directive: Do not reintroduce plaintext writes; fallback reads are only for pre-PR legacy migration compatibility. Tested: cargo check --manifest-path src-tauri/Cargo.toml Tested: cargo test --manifest-path src-tauri/Cargo.toml credential_payload_chunks_stay_under_windows_blob_limit Tested: npm run -s build Tested: git diff --check Not-tested: Manual vault-unavailable upgrade run on Windows/macOS/Linux. Related: Open-Less#230
Credential vault saves now write a fresh generation of chunks before switching the manifest, so failed chunk writes or crashes do not overwrite chunks still referenced by the previous manifest. The same change also migrates documented per-account vault entries into the chunked manifest before falling back to legacy JSON. Constraint: Windows Credential Manager requires chunking, and legacy credentials may exist either as plaintext JSON or documented per-account vault entries. Rejected: Overwrite stable chunk names before manifest update | can mix old and new payloads after partial writes. Confidence: high Scope-risk: narrow Directive: Treat the manifest as the commit pointer; write new-generation chunks before updating it and delete old chunks only after manifest write succeeds. Tested: cargo check --manifest-path src-tauri/Cargo.toml Tested: cargo test --manifest-path src-tauri/Cargo.toml credential_payload_chunks_stay_under_windows_blob_limit Tested: npm run -s build Tested: git diff --check Not-tested: Manual crash injection during OS vault writes. Related: Open-Less#230
The migration path must prefer the current legacy JSON source over older per-account vault leftovers, clean those leftovers after a valid manifest is available, and avoid write operations overwriting a damaged or unreadable vault payload with fallback defaults. Constraint: credentials.json is migration-only, but it was the previous Tauri source of truth.\nRejected: Use read fallback for mutating updates | can overwrite unreadable vault data with defaults.\nConfidence: high\nScope-risk: narrow\nDirective: Mutating credential operations must propagate chunked vault read errors instead of saving fallback roots.\nTested: cargo check --manifest-path src-tauri/Cargo.toml\nTested: cargo test --manifest-path src-tauri/Cargo.toml credential_payload_chunks_stay_under_windows_blob_limit\nTested: npm run -s build\nTested: git diff --check\nNot-tested: Manual migration with simultaneous JSON and stale per-account vault entries.\nRelated: Open-Less#230
Vault migration should not silently report success when credentials were written to the OS vault but the previous plaintext credentials file could not be removed. Propagating that cleanup error on write/update paths makes the remaining plaintext exposure visible instead of treating the migration as complete. Constraint: credentials.json is allowed only as a migration source, not a durable fallback after a successful vault write.\nRejected: Keep deletion best-effort only | can leave plaintext credentials indefinitely with no user-visible failure.\nConfidence: high\nScope-risk: narrow\nDirective: Do not hide legacy plaintext deletion failures on credential write paths.\nTested: cargo check --manifest-path src-tauri/Cargo.toml\nTested: cargo test --manifest-path src-tauri/Cargo.toml credential_payload_chunks_stay_under_windows_blob_limit\nTested: npm run -s build\nTested: git diff --check\nNot-tested: Manual permission-denied legacy file cleanup on installed apps.\nRelated: Open-Less#230
Linux credential storage must survive reboot after the legacy plaintext file is removed. The keyring linux-native feature uses kernel keyutils, so the Linux build now enables the persistent Secret Service-backed combo and the migration read fallback also considers older per-account vault entries when a current manifest is unreadable. Constraint: credentials.json must be removed after successful vault migration.\nConstraint: keyring linux-native/keyutils is an in-memory cache across reboot.\nRejected: Keep plaintext JSON as Linux fallback | violates the vault storage boundary.\nConfidence: high\nScope-risk: moderate\nDirective: Linux credential storage must remain persistent if legacy plaintext cleanup is enforced.\nTested: cargo check --manifest-path src-tauri/Cargo.toml\nTested: cargo test --manifest-path src-tauri/Cargo.toml credential_payload_chunks_stay_under_windows_blob_limit\nTested: npm run -s build\nTested: git diff --check\nNot-tested: Manual Secret Service read/write on a desktop Linux keyring session.\nRelated: Open-Less#230
Legacy per-account vault migration must be all-or-nothing. If one old entry is temporarily unreadable, saving the readable subset and deleting the original entries can silently drop the failed secret, so update-path migration now aborts on any legacy vault read error while the non-mutating read fallback remains conservative. Constraint: migration cleanup deletes old per-account vault entries after saving the chunked manifest.\nRejected: Migrate the readable subset | can discard an unreadable legacy secret before retry.\nConfidence: high\nScope-risk: narrow\nDirective: Do not remove legacy per-account vault entries unless every legacy account read completed without error.\nTested: cargo check --manifest-path src-tauri/Cargo.toml\nTested: cargo test --manifest-path src-tauri/Cargo.toml credential_payload_chunks_stay_under_windows_blob_limit\nTested: npm run -s build\nTested: git diff --check\nNot-tested: Manual transient per-account vault read failure during upgrade.\nRelated: Open-Less#230
When the current legacy credentials.json source wins migration, older per-account vault entries should not remain as stale fallback data. After the JSON payload is successfully written to the chunked vault and plaintext cleanup succeeds, the migration now removes those older per-account entries as well. Constraint: credentials.json is the previous Tauri source of truth and must win over stale Swift-era vault entries.\nRejected: Leave per-account entries for fallback | keeps stale secrets reachable after successful migration.\nConfidence: high\nScope-risk: narrow\nDirective: Clean old per-account vault entries after any successful authoritative migration source is saved.\nTested: cargo check --manifest-path src-tauri/Cargo.toml\nTested: cargo test --manifest-path src-tauri/Cargo.toml credential_payload_chunks_stay_under_windows_blob_limit\nTested: npm run -s build\nTested: git diff --check\nNot-tested: Manual upgrade with both credentials.json and stale per-account vault entries.\nRelated: Open-Less#230
Credential writes should not fail only because an old migration source cannot be deleted. Once current vault data is readable and writable, legacy plaintext cleanup remains best-effort and logged so users can still save or remove credentials even if credentials.json is locked or read-only.\n\nThe rebase also brought the new Japanese and Korean locale files onto this branch, so the credential storage notice key is filled there to keep the frontend type contract intact.\n\nConstraint: credentials.json is obsolete migration data after vault write.\nRejected: Propagate legacy JSON deletion failures on every vault save | blocks credential updates despite a valid vault.\nConfidence: high\nScope-risk: narrow\nDirective: Do not make credential updates depend on deleting legacy plaintext files.\nTested: cargo check --manifest-path src-tauri/Cargo.toml\nTested: cargo test --manifest-path src-tauri/Cargo.toml credential_payload_chunks_stay_under_windows_blob_limit\nTested: npm run -s build\nTested: git diff --check\nNot-tested: Manual locked/read-only credentials.json cleanup.\nRelated: Open-Less#230
H-Chris233
force-pushed
the
fix/issue-230-credential-storage-model
branch
from
b5f281d to
deb1196
Compare
|
Persistent review updated to latest commit b5f281d |
|
Persistent review updated to latest commit deb1196 |
Windows smoke scripts should no longer fail solely because credentials.json was migrated into the OS credential vault. The default suite now runs runtime startup without the obsolete JSON gate, and explicit legacy JSON checks warn instead of blocking vault-backed test machines.\n\nConstraint: Provider credentials now live in the OS credential vault after migration.\nRejected: Query Windows Credential Manager from smoke scripts | larger platform-specific parsing surface for a smoke gate that only needs to avoid the obsolete JSON blocker.\nConfidence: high\nScope-risk: narrow\nDirective: Do not make Windows smoke defaults depend on migrated plaintext credentials.json.\nTested: git diff --check\nTested: Python sanity check for conflict markers in edited PowerShell scripts\nNot-tested: PowerShell parser unavailable in this Linux environment.\nRelated: Open-Less#230
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c06db84d3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ($RequireCredentials -and (-not $credentialStatus.VolcengineConfigured -or -not $credentialStatus.ArkConfigured)) { | ||
| throw "Real regression requires configured Volcengine ASR and Ark LLM credentials." | ||
| Write-Warning "Legacy credentials.json is incomplete; continuing because the app uses the OS credential vault." |
There was a problem hiding this comment.
When this script is run with -RequireCredentials on a clean Windows machine that has neither legacy JSON nor vault credentials, this branch now only warns and continues. In the -InsertionFallback/real-ASR path that can exercise OpenLess' missing-Volcengine mock fallback instead of real ASR, so the regression can stop proving that configured credentials are present. Please either query Credential Manager here or keep -RequireCredentials as a hard abort when credentials cannot be confirmed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
已处理：恢复了 windows-real-regression.ps1 中 -RequireCredentials 的硬门禁（缺少凭据时直接 throw），避免“看起来跑过了 real regression 但其实没有真实凭据”的假阳性。\n\n同时保留上一个修复：默认 smoke suite 不再强制 RequireCredentials，避免仅因 legacy credentials.json 被迁移就阻断基础启动冒烟。\n\n提交：1d04131
|
Persistent review updated to latest commit c06db84 |
Real regression runs that explicitly pass -RequireCredentials should still fail fast when credentials cannot be confirmed. This keeps the real-ASR signal strict while leaving the default runtime smoke suite decoupled from legacy credentials.json.\n\nConstraint: default smoke suite must not depend on migrated plaintext JSON.\nRejected: Keep warning-only behavior in real regression mode | allows credential-missing runs to appear as valid real-ASR regressions.\nConfidence: high\nScope-risk: narrow\nDirective: Keep -RequireCredentials in windows-real-regression as a hard gate unless credential-vault probing is added.\nTested: git diff --check\nNot-tested: Running Windows PowerShell regression scripts in Linux environment.\nRelated: Open-Less#230
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Persistent review updated to latest commit 1d04131 |
用户实测：首次允许 Keychain 后仍反复弹窗十几次。
根因：
1. save_credentials 每次都生成新 UUID 作为 chunk account 命名前缀
(credentials.v1.chunk.<NEW_UUID>.0/.1)。macOS Keychain 的「始终允许」
绑定到 (binary 签名, entry 名)，每次保存设置都让 chunk entry 名变 →
之前 ACL 全失效 → 重新弹。
2. load_credentials / load_credentials_for_update 每次都调
remove_legacy_keyring_credentials() —— 对 9 个旧 stable account
各做一次 keyring delete。delete 在 macOS 上仍触发 ACL 检查，
每次 load 都弹 9 次「OpenLess 想删除 X」。
合计每次 IPC 触发的 load 12 次弹窗，跟实测吻合。
修复（数据安全优先）：
A. chunk account 命名稳定化
- chunk_account(generation: Option<&str>, idx) 接受可选 generation
- save_credentials 总用 None（稳定 credentials.v1.chunk.{idx}）
- 新 manifest 不写 generation 字段（serde skip_serializing_if=None）
- 旧 manifest 仍可读（generation: Option<String>，含 UUID 时按旧格式
读）→ 一次 save 后无缝迁移到新格式 + 删除旧 UUID chunks
B. partial-write 安全
- 仍保留「先写所有 chunks，再写 manifest」顺序
- chunks 数量减少时主动删除 idx >= new count 的旧 entries
- 旧 UUID chunks 保留删除路径（迁移用）
C. legacy keyring delete 仅在 migration 路径触发
- load_credentials / load_credentials_for_update 不再调用
remove_legacy_keyring_credentials()
- migrate_legacy_sources_for_update 内部仍调用一次 → 首次成功迁移
就清干净；之后 load 不再尝试 delete 不存在的 entries
- legacy 文件 delete 不需 ACL，best-effort 调用保留
D. 不影响 Windows
- Windows Credential Manager 没有 ACL 弹窗概念
- 仍走 chunked storage（2560 byte 限制需要分块）
修复后体感：
- 首次启动迁移：~3 次弹（manifest + 2 chunks），用户点 Always Allow
- 之后启动 / 保存设置：0 次弹（ACL 长期有效）
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0505c2fc54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (index, chunk) in chunks.iter().enumerate() { | ||
| let account = chunk_account(None, index); | ||
| keyring_entry_for(&account)? | ||
| .set_password(chunk) | ||
| .with_context(|| format!("write system credential vault chunk {index}"))?; |
There was a problem hiding this comment.
When updating an already-migrated vault, these writes reuse the stable credentials.v1.chunk.{index} accounts before the manifest is replaced. If any later chunk write or the manifest write fails (for example a Keychain ACL denial/cancel or transient Credential Manager error), the old manifest is still present but now points at a mix of new and old chunk contents; with the legacy JSON removed after the previous successful save, subsequent reads fail to decode the vault and credential updates/readbacks are effectively broken. Use a staged generation or otherwise avoid overwriting the currently-referenced chunks until the new manifest can be committed.
Useful? React with 👍 / 👎.
|
Persistent review updated to latest commit 0505c2f |
主要修复 ───────── - fix(windows): TSF DLL 静态链接 CRT，防 host 进程（QQ 等）私有 MSVCP140 劫持导致切微软拼音崩溃 0xc0000005（PR #287 + #289 + Codex P1 review）。 - fix(windows): 移除 DisableThreadLibraryCalls — /MT 下 CRT 需要 thread attach/detach 通知做 per-thread TLS 清理，禁用反而触发崩溃。 - fix(recording): 录音条出现时 mic 已 capture，按 Option 不再吞开头 字。emit Recording 加 race 检查避免短按时覆盖 stop/cancel UI 信号。 - fix(startup): splash 透明背景 + 卡片化，避免长启动时左半白屏。 - fix(vault): keyring chunks 用稳定 account name —— macOS Keychain Always-Allow 不再因 UUID 轮换失效；不再每次 load 都尝试删 9 个 legacy entries 触发 ACL 弹窗（与 PR #277 一起）。 新功能 ───────── - feat(security): 凭据从 plaintext JSON 迁移到平台 credential vault （macOS Keychain / Windows Credential Manager / Linux libsecret）。 Windows 2560 byte 限制下用 chunked storage；partial-write safe。 legacy 多源迁移完成后自动清理（PR #277）。 工程 ───────── - chore(ci): 暂时搁置 macos-13（Intel mac）matrix —— GH Actions runner pool 紧张到每次 dispatch queue 1-2h（已观测 4 次）。Apple Silicon 用户 dmg 仍发；Intel mac 用户保留 v1.2.20 dmg + Rosetta 说明（issue #299）。 - chore(devex): CI 加 macOS 矩阵 + 5-way 版本号一致性校验 + scripts/bump-version.sh 一行同步 5 处版本号。.gitignore 加 promo-openless-v2 / node_modules / dist / target 全局兜底（PR #294）。 - docs: docs/audit-2026-05-06.md 工程审计基线，覆盖架构 / UI bug / 多端一致性 / IPC 1:1 校验 / P0/P1/P2 改进路径。 - chore: 9 个改进 issue (#295-#303) 跟踪 audit 列出的所有 P1/P2/P3 问题。
2 participants
User description
变更
credentials.json迁移到系统凭据库：macOS Keychain、Windows Credential Manager、Linux keyring。credentials.json仅作为升级迁移/兼容来源：成功写入系统凭据库后删除；如果 vault 暂不可用或迁移写入失败，则保留旧文件并读取其内容，避免升级用户凭据变空。ark.api_key、volcengine.app_key），迁移成功后清理旧 per-account entries。credentials.json→ 更旧的 per-account vault entries，避免 stale vault entries 覆盖上一版 Tauri 的当前配置。credentials.json清理失败会在写入/迁移路径上返回错误；已有 vault manifest 的普通读取路径会继续尝试 best-effort 清理并记录警告。read_credential/set_credential增加窗口 label gate，仅允许主窗口访问 raw credential IPC，避免 QA / capsule 等辅助窗口默认暴露。验证
cargo check --manifest-path src-tauri/Cargo.toml（通过，保留既有 unused warnings）cargo test --manifest-path src-tauri/Cargo.toml credential_payload_chunks_stay_under_windows_blob_limitnpm run -s build（通过，保留既有 Vite dynamic/static import warnings）git diff --check未验证
Closes #230
PR Type
Bug fix, Enhancement, Documentation
Description
Migrate credentials to the OS vault
Restrict raw secret access to main window
Chunk vault payloads for Windows limits
Update docs, settings copy, and smoke scripts
Diagram Walkthrough
File Walkthrough
5 files
Gate credential IPC to main windowMigrate credentials into chunked vault storageUpdate smoke warning for vault-backed credentialsRemove outdated credential-required failureStop forcing credentials in smoke suite6 files
Add credential storage notice textAdd credential storage notice textAdd credential storage notice textAdd credential storage notice textAdd credential storage notice textSurface credential storage notice in settings4 files
Document platform vault credential storageDocument platform vault credential storageExplain vault-backed credential migrationExplain vault-backed credential migration1 files
Add platform keyring dependencies