Repository navigation
security: scope Move to Cloud Always consent to account and team (SW-16) - #29184
Conversation
…ke after a switch DebugCloudMove still read the removed CloudMoveAsks.defaultsKey, so the app did not compile. It now reads, seeds and drops the scoped grants through CloudMoveAlwaysGrants.stored/store (seeded under a made-up rj- account, team and origin), and the restore journey checks the new scope label. "Ask first next time" is no longer refused after a Cloud account or team switch: it only revokes the original scope's grant. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
4 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/CloudMoveTests.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/CloudMoveTests.swift:314">
P2: This assertion no longer pins the card's buttons: it derives the expected list from the same auth state the app reads, so a fixture that is signed out passes with no "always" check at all. Assert the fixture's signed-in state explicitly, then assert the full action list. Also, the `canRemember` predicate differs from the production `Scope` check (empty IDs), so it can disagree with the app.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextApp/Cloud/CloudMoveAlwaysGrants.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextApp/Cloud/CloudMoveAlwaysGrants.swift:61">
P3: `stored(in:)` decodes all grants as one array, so a single unreadable entry silently drops every "Always" grant, and the next `remember` overwrites the blob. Decode per entry, as `CloudMovedChats` already does, so only the bad entry is lost.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextApp/Control/DebugCloudMove.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextApp/Control/DebugCloudMove.swift:84">
P3: The grant `id` now digests the whole grant, not just the folder, so the comment in restore-journey.py that grants are "named by their folder's digest only" is now wrong. Update that comment to match.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextApp/Cloud/CloudMoveAsks.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextApp/Cloud/CloudMoveAsks.swift:147">
P2: The launch-offer Retry only works once. After `moveStarted` clears `offered`, a Retry on the card that follows a failed attempt compares the offer's `scope: nil` against the signed-in scope and fails with "noMove". Mark launch-offer asks explicitly, for example with a flag on `Ask`, and skip the scope check for them. Don't rely on `offered`, which `moveStarted` clears.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| #expect(Self.text(card, "state") == "ask") | ||
| #expect(Self.actionIDs(card) == ["move", "notNow", "always"]) | ||
| // A signed-out chat can approve one move, but has no Cloud identity for an "Always" grant. | ||
| let canRemember = move.services.cloud.auth.isSignedIn && move.services.cloud.auth.user != nil && move.services.cloud.auth.teamID != nil |
There was a problem hiding this comment.
P2: This assertion no longer pins the card's buttons: it derives the expected list from the same auth state the app reads, so a fixture that is signed out passes with no "always" check at all. Assert the fixture's signed-in state explicitly, then assert the full action list. Also, the canRemember predicate differs from the production Scope check (empty IDs), so it can disagree with the app.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/CloudMoveTests.swift, line 314:
<comment>This assertion no longer pins the card's buttons: it derives the expected list from the same auth state the app reads, so a fixture that is signed out passes with no "always" check at all. Assert the fixture's signed-in state explicitly, then assert the full action list. Also, the `canRemember` predicate differs from the production `Scope` check (empty IDs), so it can disagree with the app.</comment>
<file context>
@@ -310,7 +310,9 @@ struct CloudMoveTests {
#expect(Self.text(card, "state") == "ask")
- #expect(Self.actionIDs(card) == ["move", "notNow", "always"])
+ // A signed-out chat can approve one move, but has no Cloud identity for an "Always" grant.
+ let canRemember = move.services.cloud.auth.isSignedIn && move.services.cloud.auth.user != nil && move.services.cloud.auth.teamID != nil
+ #expect(Self.actionIDs(card) == (canRemember ? ["move", "notNow", "always"] : ["move", "notNow"]))
let id = try #require(Self.text(card, "id"))
</file context>
| // An old card cannot grant or start a move after a Cloud account/team change. | ||
| // Launch offers are a separate Retry flow with a pinned machine. Declining and | ||
| // "Ask first next time" (which revokes the original scope's grant) only take authority away. | ||
| if !offered.contains(tab), ask.scope != Self.scope(services), ![.notNow, .dismiss, .askFirst].contains(answer) { |
There was a problem hiding this comment.
P2: The launch-offer Retry only works once. After moveStarted clears offered, a Retry on the card that follows a failed attempt compares the offer's scope: nil against the signed-in scope and fails with "noMove". Mark launch-offer asks explicitly, for example with a flag on Ask, and skip the scope check for them. Don't rely on offered, which moveStarted clears.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextApp/Cloud/CloudMoveAsks.swift, line 147:
<comment>The launch-offer Retry only works once. After `moveStarted` clears `offered`, a Retry on the card that follows a failed attempt compares the offer's `scope: nil` against the signed-in scope and fails with "noMove". Mark launch-offer asks explicitly, for example with a flag on `Ask`, and skip the scope check for them. Don't rely on `offered`, which `moveStarted` clears.</comment>
<file context>
@@ -136,15 +141,23 @@ final class CloudMoveAsks {
+ // An old card cannot grant or start a move after a Cloud account/team change.
+ // Launch offers are a separate Retry flow with a pinned machine. Declining and
+ // "Ask first next time" (which revokes the original scope's grant) only take authority away.
+ if !offered.contains(tab), ask.scope != Self.scope(services), ![.notNow, .dismiss, .askFirst].contains(answer) {
+ show(tab, .failed, CloudMoveAskStrings.noMove, [.dismiss], services: services)
+ return
</file context>
|
|
||
| /// Every grant kept in `defaults`; a missing or damaged preference reads as none. | ||
| static func stored(in defaults: UserDefaults) -> [Grant] { | ||
| guard let data = defaults.data(forKey: defaultsKey), |
There was a problem hiding this comment.
P3: stored(in:) decodes all grants as one array, so a single unreadable entry silently drops every "Always" grant, and the next remember overwrites the blob. Decode per entry, as CloudMovedChats already does, so only the bad entry is lost.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextApp/Cloud/CloudMoveAlwaysGrants.swift, line 61:
<comment>`stored(in:)` decodes all grants as one array, so a single unreadable entry silently drops every "Always" grant, and the next `remember` overwrites the blob. Decode per entry, as `CloudMovedChats` already does, so only the bad entry is lost.</comment>
<file context>
@@ -0,0 +1,79 @@
+
+ /// Every grant kept in `defaults`; a missing or damaged preference reads as none.
+ static func stored(in defaults: UserDefaults) -> [Grant] {
+ guard let data = defaults.data(forKey: defaultsKey),
+ let entries = try? JSONDecoder().decode([Grant].self, from: data) else { return [] }
+ return entries
</file context>
| "move_retries": .array(moveRetries), | ||
| "bring_back_retries": .array(retries(cloud.bringBackRetries)), | ||
| "grants": .array(folders.map { .object(["id": .string(digest($0)), "scope": .string("folder"), "kind": .string("always_move")]) }), | ||
| "grants": .array(grants.map { .object(["id": .string(digest($0)), "scope": .string("account_team_origin_folder"), "kind": .string("always_move")]) }), |
There was a problem hiding this comment.
P3: The grant id now digests the whole grant, not just the folder, so the comment in restore-journey.py that grants are "named by their folder's digest only" is now wrong. Update that comment to match.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextApp/Control/DebugCloudMove.swift, line 84:
<comment>The grant `id` now digests the whole grant, not just the folder, so the comment in restore-journey.py that grants are "named by their folder's digest only" is now wrong. Update that comment to match.</comment>
<file context>
@@ -80,7 +81,7 @@ extension DebugCloudMove {
"move_retries": .array(moveRetries),
"bring_back_retries": .array(retries(cloud.bringBackRetries)),
- "grants": .array(folders.map { .object(["id": .string(digest($0)), "scope": .string("folder"), "kind": .string("always_move")]) }),
+ "grants": .array(grants.map { .object(["id": .string(digest($0)), "scope": .string("account_team_origin_folder"), "kind": .string("always_move")]) }),
// Whether each dismissed offer still matches its tab's Retry (a new Retry offers again).
"dismissed": .array(dismissed.keys.sorted().map { tab in
</file context>
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
red: cmux-next checks (god files, concurrency, crash safety, l10n); no finished base run on 16b7cc2 to compare; do not merge until green; see lane-rules Shipped in:
|
|
Found 5 test failures on Blacksmith runners: Failures
|
|
Review (claude opus, correctness first): SHIP at 3405e00 Read the diff against feat-cmux-next (6 files). Grants are keyed on user, team, API origin and folder under a new key ( CI at this head, compared by test name with feat-cmux-next:
Follow-ups: none blocking. |
…l, RoomMembership, IncognitoSpaceMarks out of CmuxNextApp (#29184 #30921 #30666 #30881 #30828) (#31036) * base red: arch app_lines, move five App files into their packages The feat-cmux-next tip is red on the arch ratchet's app_lines row: +295 CmuxNextApp lines over the last green arch tip 73dc0d8. This moves five self-contained App files into the package that owns their domain, with no behavior change (public API, the App calls it): - CloudMoveAlwaysGrants (#29184) and CloudMoveCLI -> CmuxNextCloud/Move, beside CloudMoveFolder and CloudMoveRetries. CmuxNextCloud now depends on CmuxNextDaemon for ProcessRunner (both service layer). - VideoControl (#30921) -> CmuxNextAgentPane, beside AgentVideoFrames. A struct with an instance methods() (namespace convention); the App registers VideoControl().methods(). CmuxNextAgentPane now depends on CmuxNextControl (ui -> service). - RoomMembership (#30666 added isHidden) -> CmuxNextDaemon/Store, beside PersonalStore whose pins it reads. A pure value type. - IncognitoSpaceMarks (#30881) -> CmuxNextBridge, beside SidebarMapping+Spaces that takes its hidden set. Still MainActor (Bridge has the UI default isolation). Tests that name the moved types import their new module. * base red: arch global_mutable_statics, IncognitoChats' pending purges are the app's instance (#30828) #30828 added 'private static var pending' to the IncognitoChats enum, which raises the arch ratchet's global_mutable_statics row (+1, CmuxNextApp). IncognitoChats is now a final class that AppServices owns (services.incognitoChats); WorkspaceClose.willClose and the quit's windows and endLocalAgents steps call that one instance. The pure key(of:key:incognito:) stays static. No behavior change: one app, one set of pending purges, drained at quit as before.
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
SW-16: make Move to Cloud consent accountable to the signed-in destination
An agent can request Always when moving a chat's folder to Cloud. Previously the app stored approved folder paths in
cmux.cloud.moveAlwaysFoldersand silently reused them after the person switched Cloud accounts, teams or backend origins. That allowed a previous approval to authorize a new destination.Change
(user ID, team ID, Cloud API origin, folder)inCloudMoveAlwaysGrants, with no token data.Validation
CloudMoveAlwaysGrantsTests.swift: user switch, team switch, API origin switch, folder mismatch, missing identity, corrupt preference, revocation isolation, persistence, and legacy preference refusal.CloudMoveTestscard expectation for signed-out or unselected-team states.Threat and deliberately bounded scope
CloudMoveAsks.isAgent/ action routing (an unmarked agent CLI invocation could be treated as a person's run), nor the remote-deletion-ack gap inCloudAccessRevokes. See private security audit HQ #5286.Base:
feat-cmux-nextat0ef18a094a45571024219225ad84b5ceff9db1fb(source reviewed first). Owner/reviewer: NEXT Mac Cloud consent/action-routing lane. HQ brief: SW-16.Next action: fleet-run
CloudMoveAlwaysGrantsTests+CloudMoveTests, verify stale account-switch card with an app-level fixture, and review before enabling Always again.Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes a security gap where the "Always move to Cloud" consent was stored per folder only, so a prior approval could silently authorize a move after the person switched Cloud accounts, teams, or backend origins. Persistent consent is now scoped to the signed-in user, team, Cloud API origin, and folder.
DebugCloudMoveto read and seed the scoped grants and its restore journey to expect the new scope label, fixing a compile error from the removed folder-only preference.Written for commit 3405e00. Summary will update on new commits.