Skip to content

Fix Apple OAuth continuation ownership and cleanup - #1907

Open
HarshMN2345 wants to merge 1 commit into
mainfrom
codex/fix-6486-apple-oauth-continuation
Open

HarshMN2345 wants to merge 1 commit into
mainfrom
codex/fix-6486-apple-oauth-continuation

Conversation

@HarshMN2345

@HarshMN2345 HarshMN2345 commented Sep 14, 2026

Copy link
Copy Markdown
Member

Starting a second OAuth login replaced the first pending callback, leaking its Swift continuation. Cancellation kept completed callbacks registered, so later cleanup could resume them again.

Reject an overlapping login while preserving the active one. Clear callback and session state before invoking callbacks, cancel the completed browser session, and ignore stale session completions. A browser session that fails to start also completes its callback. Completion handlers can safely begin another login.

The Apple E2E script now covers same-scheme and different-scheme overlaps, repeated cancellation, and starting another login from success or failure. The tests run on the existing Linux Apple SDK runner without opening a browser.

Validation: regenerated Apple/client and Swift/server; both generated trees pass swift-format. Generation suite passes (87 tests, 1,132 assertions, four existing skips), and the changed Twig template passes lint. A local Swift callback harness runs the three regression methods successfully; the original implementation produces 11 assertion failures. Native browser presentation is not exercised by that harness. CI AppleSwift61 passes: all four Swift tests, including the three new OAuth regressions, complete with zero failures; the mock API harness passes 185 assertions. Swift61 and all generation, formatting, Twig, and refactor checks also pass.

Addresses appwrite/appwrite#6486.

The Apple SDK must be regenerated and released for applications to receive this change.

@HarshMN2345
HarshMN2345 marked this pull request as ready for review September 14, 2026 12:31
@greptile-apps

greptile-apps Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The implementation appears behaviorally sound, but the PR should not merge until its new coverage satisfies the repository requirement to test observable behavior rather than internal OAuth bookkeeping.

Fix All in Claude CodeFindings

  1. P2 Tests Bypass Public OAuth
Fix with agent prompt
### Issue 1
tests/e2e/languages/apple/Tests.swift:6
Changing this fixture to `@testable import Appwrite` lets it call the internal `WebAuthComponent.authenticate` method and synthesize lifecycle events through `onCallback`. That bypasses the public OAuth continuation wrapper, its main-queue dispatch, and native session integration, so these tests can pass while the user-visible OAuth operation still leaks a continuation or mishandles cleanup. This violates the repository directive to test observable behavior instead of mirroring internal implementation, and the requirement must be satisfied before merging.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

  • Adds explicit authentication ownership through a per-login UUID.
  • Makes cleanup remove callback and session state before invoking completion handlers.
  • Adds Linux regression coverage for overlap, repeated cancellation, and callback reentrancy.
  • The added tests directly exercise internal bookkeeping rather than the observable public OAuth API.

Reviews (1) · Last reviewed commit: "fix(apple): complete OAuth callbacks exa..."

import FoundationNetworking
#endif
import Appwrite
@testable import Appwrite

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Tests Bypass Public OAuth

Changing this fixture to @testable import Appwrite lets it call the internal WebAuthComponent.authenticate method and synthesize lifecycle events through onCallback. That bypasses the public OAuth continuation wrapper, its main-queue dispatch, and native session integration, so these tests can pass while the user-visible OAuth operation still leaks a continuation or mishandles cleanup. This violates the repository directive to test observable behavior instead of mirroring internal implementation, and the requirement must be satisfied before merging.

Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)

Knowledge Base Used: End-to-end SDK suites

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/languages/apple/Tests.swift
Line: 6

Comment:
**Tests Bypass Public OAuth**

Changing this fixture to `@testable import Appwrite` lets it call the internal `WebAuthComponent.authenticate` method and synthesize lifecycle events through `onCallback`. That bypasses the public OAuth continuation wrapper, its main-queue dispatch, and native session integration, so these tests can pass while the user-visible OAuth operation still leaks a continuation or mishandles cleanup. This violates the repository directive to test observable behavior instead of mirroring internal implementation, and the requirement must be satisfied before merging.

**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

**Knowledge Base Used:** [End-to-end SDK suites](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/sdk-generator/-/docs/end-to-end-sdk-suites.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant