Repository navigation
Conversation
i2h3
added
bug
os: 🍎 macOS
Collaborator
Author
|
/backport to stable-34.0 |
Collaborator
Author
|
/backport to stable-35.0 |
i2h3
changed the title
i2h3
marked this pull request as ready for review
October 2, 2026 09:23
i2h3
requested review from
camilasan,
claucambra,
mgallien and
nilsding
as code owners
October 2, 2026 09:23
i2h3
force-pushed
the
i2h3/9865-keychain-scope
branch
2 times, most recently
from
October 2, 2026 14:11
1cb5951 to
d16fa95
Compare
6 of 12 tasks
i2h3
force-pushed
the
i2h3/9865-keychain-scope
branch
from
October 4, 2026 11:18
d16fa95 to
7e665b1
Compare
i2h3
force-pushed
the
i2h3/9865-keychain-scope
branch
from
October 13, 2026 08:53
7e665b1 to
aad263a
Compare
11 of 12 tasks
4 of 8 tasks
The file provider extension stored the account password as a generic password and scoped it with kSecAttrServer. That attribute belongs to internet passwords and macOS ignores it on generic passwords, so every lookup and update matched on the account name alone. Saving the password overwrote the generic passwords of other applications using the same account name, as reported in #9865, and a lookup could return one of them. Key the item by the file provider domain identifier as service and the user name as account, and show the domain display name as its label. Items written by earlier versions are left alone because they cannot be told apart from the items of other applications. Refs #9865 Co-authored-by: Jonathan Stucklen <stuckj@gmail.com> Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Iva Horn <iva.horn@nextcloud.com>
… out removeAccountConfig now also deletes every keychain item of the domain. The main app sends it when the account is signed out, needs credentials again, is redirected or must accept the terms of service. Without this, Finder could authenticate the domain again with a password the user signed out of, and every account removed and added again would leave one more item behind now that items are keyed by domain. Refs #9865 Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Iva Horn <iva.horn@nextcloud.com>
The extension cannot run once its domain is removed, so the password it stored would stay in the keychain. Add a request through which the main app asks the extension of a domain to delete its stored password, and send it as the first step of every domain removal. The main app waits at most one second for the reply and removes the domain in any case. The request is a new method instead of a changed removeAccountConfig because an extension still running an older build drops a selector it does not know, which would also drop the existing sign-out handling. Refs #9865 Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Iva Horn <iva.horn@nextcloud.com>
…eProviderKit The keychain regression tests skip themselves when the default keychain is not usable. In CI that would hide them silently, so require them there, and limit how long the test job can run in case the keychain waits for user interaction. Refs #9865 Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Iva Horn <iva.horn@nextcloud.com>
i2h3
force-pushed
the
i2h3/9865-keychain-scope
branch
from
October 19, 2026 10:53
aad263a to
33a8c62
Compare
|
claucambra
requested changes
Oct 19, 2026
Comment on lines
+129
to
+132
| if (!xpc) { | ||
| qCInfo(lcMacFileProvider) << "Not deleting password because XPC is not configured." << fileProviderDomainIdentifier; | ||
| return std::nullopt; | ||
| } |
Collaborator
There was a problem hiding this comment.
Startup orphan removal skips password cleanup -> returns immediately without XPC. The settings controller removes orphaned domains before calling configureXPC(), so that path removes domains without sending a deletion request, leaving scoped credentials behind. It needs to acquire the domain’s service before removal. This finding is based on source inspection.
Comment on lines
+45
to
49
| public func deletePassword(completionHandler: ((Bool) -> Void)?) { | ||
| logger.info("Received request to delete the stored password.") | ||
| let deleted = keychain.deletePassword() | ||
| completionHandler?(deleted) | ||
| } |
Collaborator
There was a problem hiding this comment.
Queued setup can recreate a deleted password -> deletes independently of setupChain. This is reproducible: hold authentication A, queue configuration B, delete successfully, then release A—B stores its password afterward. Deletion needs to invalidate pending setup and coordinate with credential writes. The sign-out path has the same ordering problem.
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.
Intentionally skipping 34.0.5 and 35.0.0 and to be ported back later! Fixes #10976. Replaces #10858. Relates to #9865.
Thanks to @stuckj for clearing this issue up and taking the initiative! I probably would not have seen the problem here without their help and first draft.
Checklist
AI (if applicable)