fix(android): reject self-provider picker roots - #446
Conversation
Obiente previewNC Native · Obiente updates this comment as the preview changes. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 538e12112f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72c0b6edf5
ℹ️ 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".
7dfaeb8 to
d963ee4
Compare
72c0b6e to
548a50d
Compare
d963ee4 to
b2c2188
Compare
548a50d to
8bacb4d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e91eb6921
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bcc25f9d0
ℹ️ 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".
6057f39 to
a712498
Compare
5bcc25f to
d50a4e9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d50a4e9048
ℹ️ 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".
cc5f869 to
337b65f
Compare
d50a4e9 to
156ee6f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 156ee6febf
ℹ️ 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".
1caca52 to
6c1d647
Compare
62a3f5b to
4dee821
Compare
There was a problem hiding this comment.
💡 Codex Review
Require authoritative listings during recovery
When a recovery-authorized query encounters an offline or 5xx response but an older directory cache exists, this fallback returns the cached children as if they were current. AndroidSafDownloadPublisher can consequently interpret a stage or backup omitted from that stale listing as physically absent, remove its ownership row, and let pair/account removal succeed while the recovery file remains on the server; this loses the durable recovery path and can expose a partial stage later. Require an authoritative network listing for recovery, or retain the transaction when only cached state is available.
AGENTS.md reference: AGENTS.md:L399-L403
ℹ️ 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".
| // Missing credentials cannot safely repair legacy self-provider downloads before removal. | ||
| // Commit first; durable owned-state cleanup remains fail-closed and can resume after re-add. |
There was a problem hiding this comment.
When an unavailable account is removed with a legacy self-provider download pending, the promised recovery after re-add cannot run: saveSession invokes retryPendingAccountRemovalCleanup before persisting the new session, and that retry reaches retireAndroidFileSyncAccountPairs without a providerRecoverySession; meanwhile the cleanup journal itself prevents loadSession from restoring this account for NextcloudDocumentsProvider. Fresh evidence after the credential-free commit change is that the newly supplied credential is never bound to the deferred provider recovery, so the retry fails and blocks the account from being added again.
AGENTS.md reference: AGENTS.md:L428-L430
Useful? React with 👍 / 👎.
| prepare = { prepareAndroidAccountRemoval(appContext, session) }, | ||
| revalidate = { preflightAndroidAccountRemoval(appContext, session) }, | ||
| action = action, |
There was a problem hiding this comment.
If a folder-sync run starts after prepare() releases its recovery lease and finishes before removal acquires the next lease, it can fail after persisting a new local SAF download transaction. Fresh evidence after adding in-lease revalidation is that preflightAndroidAccountRemoval checks document writebacks and fileSyncOwnedUploads only, so this new local ownership row is invisible and credentials can still be committed as removed, leaving subsequent self-provider cleanup without a usable session. Reconcile or fail closed on pending SAF ownership again while holding the final removal lease.
AGENTS.md reference: AGENTS.md:L428-L430
Useful? React with 👍 / 👎.
| return providerRecovery.run( | ||
| document.uri, | ||
| AndroidDocumentsProviderRecoveryOperation.OpenRead, |
There was a problem hiding this comment.
When account-removal recovery must hash a non-empty stage or backup that is not already in an offline or virtual cache, this recovery OpenRead reaches NextcloudDocumentsProvider.openVirtualFileProxy, which calls openFileRangeSession; openTrackedAndroidFileRangeSession then blocks acquiring the same non-reentrant account guard already held by withAndroidFileSyncAccountRecoveryLease. Fresh evidence after the query/listing lease bypass is that the range-open path still has no held-lease mode, so removal hangs while authenticating typical uncached recovery content. Propagate the held lease through range opening or use a recovery read path that does not reacquire it.
AGENTS.md reference: AGENTS.md:L428-L430
Useful? React with 👍 / 👎.
6c1d647 to
50db150
Compare
4dee821 to
70a4c3e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70a4c3ecf3
ℹ️ 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".
| val services = AndroidNextcloudServices(context.applicationContext) | ||
| withAndroidFileSyncAccountRecoveryLease( | ||
| expectedSession = providerRecoverySession, | ||
| resolveSession = { services.loadSession(providerRecoverySession.accountId) }, |
There was a problem hiding this comment.
When clearSession() handles IndependentRecoveryUnavailable (for example, a malformed or missing registry with a recoverable independent credential slot), the new production prepare callback reaches this lookup, but services.loadSession(accountId) can only restore an account present in a valid registry and therefore returns null in precisely this state. withExactAccountSession then throws before deleting the slot, even when the account has no folder-sync pairs, leaving the user unable to complete the explicit credential reset. Use the already recovered slot session for this reset path, or avoid acquiring the sync-recovery lease when there are no applicable pairs.
Useful? React with 👍 / 👎.
…picker-provider-feedback
…picker-provider-feedback
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 606098fa0c
ℹ️ 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".
| prepare() | ||
| return withAndroidAccountRemovalLease(accountIdentity, guard) { |
There was a problem hiding this comment.
When ordinary account removal races a newly opened seekable DocumentsProvider file, prepare() now runs before the exclusive removal lease. AndroidFileRangeSessionCoordinator.quiesce() snapshots existing registrations, while openTrackedAndroidFileRangeSession() holds the account guard only until it registers the range session; an open that registers after that snapshot can release the guard before removal calls tryWithAccount. The quiesce pass then misses (and removes the registry entry for) this session, allowing credential deletion to proceed while the descriptor can continue issuing authenticated range requests with the retained app password. Keep range-session admission fenced through quiescing and credential removal, or re-quiesce under the final lease.
Useful? React with 👍 / 👎.
| if (recoveryAuthorized) services.listFilesWhileAccountLeaseHeld(session, account.userId, parent.path) | ||
| else services.listFiles(session, account.userId, parent.path) |
There was a problem hiding this comment.
When legacy self-provider recovery runs while the server is offline or returns a 5xx, this call can silently return a cached listing because listFilesWhileAccountLeaseHeld() discards NextcloudFileListingSource, and the surrounding provider code also falls back to offline directory contents. If that cached snapshot predates an interrupted stage or backup, AndroidSafDownloadPublisher.reconcile() treats the missing recovery documents as authoritative and can delete the durable ownership row; pair or account removal then completes while the stage or protected original remains stranded under a reserved recovery name. Recovery-authorized listings must fail closed unless they came from the network.
AGENTS.md reference: AGENTS.md:L428-L430
Useful? React with 👍 / 👎.
| buildDocumentUri = { id -> DocumentsContract.buildDocumentUri(authority, id) }, | ||
| buildChildDocumentsUri = { id -> DocumentsContract.buildChildDocumentsUri(authority, id) }, | ||
| ) | ||
| return withAndroidDocumentsProviderRecoveryPermit(bound, documentId, operation) { |
There was a problem hiding this comment.
When a legacy self-provider tree URI carries an Android user prefix for another profile, such as content://10@<own-authority>/tree/..., this permit is installed only in the caller profile's process and thread, while the rebuilt URI dispatches to the provider instance in user 10. That provider cannot see the ThreadLocal permit and its Binder caller UID is not the caller process UID, so recovery succeeds only accidentally if the other profile currently has the same account active; otherwise the first query or mutation fails and the pending ownership row permanently blocks pair or account removal. The rejection parser explicitly recognizes user-prefixed own authorities, so legacy retirement also needs a provider-independent cross-user path or an explicit safe retirement policy.
AGENTS.md reference: AGENTS.md:L428-L430
Useful? React with 👍 / 👎.
1 participant
Summary
Validation