Refactored XPC for FinderSync Extension - #10492
Conversation
i2h3
added
bug
os: 🍎 macOS
github-project-automation
Bot
moved this to 🧭 Planning evaluation (don't pick)
in 💻 Desktop Clients team
i2h3
moved this from 🧭 Planning evaluation (don't pick)
to 🏗️ In progress
in 💻 Desktop Clients team
|
/backport to stable-34.0 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
claucambra
left a comment
claucambra
left a comment
There was a problem hiding this comment.
The code changes look fine.
I still think the quantity and length of comments in these changes is excessive. I understand providing context around certain additions motivated by solved problems is valuable. We should certainly seek to preserve what we learn during the development process. I just don't think doing so in code files is the best approach. Comments like the ones in FinderSyncBrokerProtocol.h are for me bordering on a code smell; they are lengthy, reference issues and problems that are not familiar to the reader, they increase cognitive load, they distract from the actual implementation of the method itself... I think a lot of these comments are better served living in a dedicated document for e.g. the broker extension. That way we can follow progressive disclosure and offer information where/when it is relevant for both humans and the agents.
Given the urgency of the issue, and since the functional changes in this PR are fine, we can go ahead and merge
| * The broker exists because neither the app nor the extension is allowed to publish | ||
| * a named XPC endpoint. Only a launchd job may do that: the Mach service name has to | ||
| * be declared in a launchd.plist, and `xpc_connection_create(3)` states that "new | ||
| * service names may NOT be dynamically registered" and that the allowances XPC makes | ||
| * for this "in debug scenarios" will "absolutely NOT be made in the production | ||
| * scenario". That is why a plain `-[NSXPCListener initWithMachServiceName:]` in the | ||
| * app works under Xcode and fails once the app is launched from Finder — the failure | ||
| * mode that left FinderSync completely inert in 34.0.0. | ||
| * | ||
| * A Service Management login item *is* a launchd job, so it can vend a name that both | ||
| * sandboxed peers are permitted to look up: the broker's Mach service name equals its | ||
| * own bundle identifier, which is prefixed by the shared App Group identifier, and the | ||
| * App Sandbox grants `mach-lookup` for any name under an App Group the process claims. | ||
| * No `temporary-exception` entitlement is involved on any side. | ||
| * | ||
| * The broker is only a rendezvous point. It stores the endpoint of the app's anonymous | ||
| * listener and hands it to the extension; all FinderSync traffic then flows directly | ||
| * between app and extension over that endpoint, so no message is ever relayed here. | ||
| * | ||
| * @note `NSXPCListenerEndpoint` needs no `-setClasses:forSelector:argumentIndex:ofReply:` | ||
| * allow-list. That API constrains the contents of *collection* arguments; a directly | ||
| * typed parameter takes its class from the method signature, and endpoints conform to | ||
| * `NSSecureCoding`. Passing an endpoint as `id`, or nested inside a dictionary, would | ||
| * require an allow-list — so do not do that. | ||
| */ |
There was a problem hiding this comment.
I think this is a good example of context that hinders rather than informs. It's a big information dump. This would be better served in a detailed doc explaining the issue, separated from the code
| * `-[NSXPCListener resume]` returns void and reports activation failure only to the XPC | ||
| * subsystem's own log, and `initWithMachServiceName:` never returns nil, so neither | ||
| * proves anything about the channel. Only a completed round-trip does: a reply here | ||
| * establishes that the Mach name really is in the bootstrap namespace and that the | ||
| * broker's listener delegate is accepting connections. |
There was a problem hiding this comment.
Again, I think this exemplifies too much information being offered up front
| NSString *appName = [[NSBundle mainBundle] objectForInfoDictionaryKey:@"CFBundleDisplayName"] | ||
| ?: [[NSBundle mainBundle] objectForInfoDictionaryKey:@"CFBundleName"] ?: @"Nextcloud"; | ||
| NSMenu *menu = [[NSMenu alloc] initWithTitle:@""]; | ||
| NSMenuItem *item = [menu addItemWithTitle:[NSString stringWithFormat:@"%@ — not connected", appName] | ||
| action:nil | ||
| keyEquivalent:@""]; |
There was a problem hiding this comment.
I think we should constify these pointers and fix the alignment of the selector parameters
| // Carry the command on the item itself rather than an index into _menuItems. The | ||
| // menu outlives the array it was built from: a later GET_MENU_ITEMS round trip can | ||
| // reset and repopulate _menuItems before the user clicks, after which an index | ||
| // either selects the wrong command or is out of bounds and throws — crashing the | ||
| // extension. identifier is a plain string property from | ||
| // NSUserInterfaceItemIdentification, so unlike representedObject it survives | ||
| // whatever copying Finder does to the menu. |
There was a problem hiding this comment.
I think this comment is far too wordy. It increases cognitive load. Seems to me we can explain this far more simply with
// Store the command on the menu item instead of an index into _menuItems. Finder may
// rebuild _menuItems before the user clicks, making the index invalid or pointing to
// the wrong command. identifier is copied with the menu item, so it remains valid.
| // No -setCodeSigningRequirement: on our connection to the broker. That is deliberate, and it | ||
| // is the one direction of the three where peer validation cannot be done. | ||
| // | ||
| // It was tried. A Finder Sync extension evaluating the broker's signature fails the check even | ||
| // when the broker is correctly signed and satisfies the very same requirement statically: | ||
| // | ||
| // codesign --verify -R <req> …FinderSyncBroker.app -> satisfies | ||
| // runtime, from this process: | ||
| // "Received message forbidden due to code signing requirement: | ||
| // xpc_support_check_token: anchor apple generic and | ||
| // certificate leaf[subject.OU] = "…" status: -67030" (errSecCSReqFailed) | ||
| // | ||
| // The client, also sandboxed, validates the same broker over the same protocol without any | ||
| // trouble, so what differs is this process: app extensions run in a custom sandbox that, as | ||
| // Apple DTS puts it, "doesn't always align with the standard App Sandbox used by apps" | ||
| // (developer.apple.com/forums/thread/802817). Setting a requirement that cannot succeed is | ||
| // worse than setting none — the connection is invalidated on the first reply, which is exactly | ||
| // how this manifested: the broker handed over the endpoint and we then rejected it, forever. | ||
| // | ||
| // The security property is still covered from both other directions: | ||
| // * the broker requires *us* to satisfy its requirement before it accepts our connection, | ||
| // and it does the same for the client; | ||
| // * the client validates the broker before publishing an endpoint to it. | ||
| // And the broker's Mach service name lives inside the shared App Group, so registering it at | ||
| // all means being a login item whose bundle identifier is prefixed with our team identifier. |
There was a problem hiding this comment.
This extended comment doesn't help explain the code below. It justifies a broader technical decision based on a past decision not made by the reader. This belongs in a separate technical doc (which can be referenced here). If I am a reader trying to understand what this code does, I don't think it's valuable to know what didn't work. I would want to know what the code below does and have the option of reading more somewhere if I need to, rather than having those impede my understanding of this class implementation
| // - the install destination in shell_integration/MacOSX/CMakeLists.txt, because a login | ||
| // item's wrapper filename must equal its bundle identifier | ||
| // - the extension's own derivation from NCApplicationGroupIdentifier | ||
| return appGroupIdentifier() + QStringLiteral(".FinderSyncBroker"); |
There was a problem hiding this comment.
This is a nice example of a comment that is useful as it explains something that is not explicit to this code but does directly affect this specific code
Signed-off-by: Iva Horn <iva.horn@nextcloud.com>
744d677 to
6963914
Compare
make Codesign.swift discover branded login-item names dynamically and derive the broker entitlement manifest from the existing FinderSync entitlement path. This avoids changing the CLI or Brander command. Signed-off-by: Rello <Rello@users.noreply.github.com>
…ed from clang-tidy Assisted-by: Codex:GPT-5 Signed-off-by: Rello <github@scherello.de>
|
Artifact containing the AppImage: nextcloud-appimage-pr-10492.zip Digest: To test this change/fix you can download the above artifact file, unzip it, and run it. Please make sure to quit your existing Nextcloud app and backup your data. |
|
Checklist
AI (if applicable)