fix: use clearAll() instead of clear() in CredentialsManager error paths - #1249
fix: use clearAll() instead of clear() in CredentialsManager error paths#1249NandanPrabhu wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesCredential Reset and Error Handling
Xcode Build Graph Maintenance
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Auth0/CredentialsManager.swift`:
- Line 804: Update both clearAll() call sites around the DPoP error handling to
use try? instead of try, matching the existing session-expired paths. Preserve
the subsequent throw of CredentialsManagerError.dpopKeyMissing or
.dpopKeyMismatch so those specific errors are reached when clearAll() fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 24d043d1-f3a8-4983-83d1-e4a0fc3082ac
📒 Files selected for processing (1)
Auth0/CredentialsManager.swift
57aef32 to
5557e0d
Compare
e57c823
Ensures DPoP key mismatch/missing and expired-session cleanup remove all stored credentials, not just the default set.
clearAll() now calls storage.deleteAllEntries(), which the default protocol extension implements as an assertionFailure(). Without this, CredentialsManagerDPoPSpec crashes the test process instead of failing the individual test.
e57c823 to
672fc97
Compare
Map failures in retrieveCredentialsWithRetry, retrieveSSOCredentials, and retrieveAPICredentials to .renewFailed/.ssoExchangeFailed/.apiExchangeFailed instead of the generic .noCredentials, and use try? for DPoP cleanup so a failed clearAll() doesn't mask the original DPoP error. Updates CredentialsManagerSpec expectations to match the new error codes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Auth0.xcodeproj/project.pbxproj`:
- Around line 1224-1227: Remove the duplicate PBXFileSystemSynchronizedRootGroup
entry for AppTests, keeping only one of D4919A702F55A38C007EA4B0 and
D4919A762F55A3BD007EA4B0. Delete the discarded identifier from the main group
and AppTests target references so the AppTests folder is synchronized exactly
once.
In `@Auth0/CredentialsManager.swift`:
- Around line 804-812: Update both clearAll() call sites in the DPoP validation
flow to propagate cleanup errors instead of suppressing them with try?. Preserve
the existing subsequent dpopKeyMissing and dpopKeyMismatch throws only when
clearAll() succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e32b052-dc8b-4c26-a0bf-b043ef455fbe
📒 Files selected for processing (5)
Auth0.xcodeproj/project.pbxprojAuth0/Auth0Authentication.swiftAuth0/CredentialsManager.swiftAuth0Tests/CredentialsManagerDPoPSpec.swiftAuth0Tests/CredentialsManagerSpec.swift
💤 Files with no reviewable changes (1)
- Auth0/Auth0Authentication.swift
| D424EBAA2F15D6A500052186 /* MFA */ = {isa = PBXFileSystemSynchronizedRootGroup; explicitFileTypes = {}; explicitFolders = (); path = MFA; sourceTree = "<group>"; }; | ||
| D4919A702F55A38C007EA4B0 /* AppTests */ = {isa = PBXFileSystemSynchronizedRootGroup; explicitFileTypes = {}; explicitFolders = (); path = AppTests; sourceTree = "<group>"; }; | ||
| D4919A762F55A3BD007EA4B0 /* AppTests */ = {isa = PBXFileSystemSynchronizedRootGroup; explicitFileTypes = {}; explicitFolders = (); path = AppTests; sourceTree = "<group>"; }; | ||
| D4ABFCF72F0F8D78000C828C /* App */ = {isa = PBXFileSystemSynchronizedRootGroup; exceptions = (D4ABFE5E2F0FA132000C828C /* PBXFileSystemSynchronizedBuildFileExceptionSet */, D424EDA82F1668CB00052186 /* PBXFileSystemSynchronizedBuildFileExceptionSet */, D424EDA62F1668C200052186 /* PBXFileSystemSynchronizedBuildFileExceptionSet */, D424EDA72F1668C800052186 /* PBXFileSystemSynchronizedBuildFileExceptionSet */, ); explicitFileTypes = {}; explicitFolders = (); path = App; sourceTree = "<group>"; }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='Auth0.xcodeproj/project.pbxproj'
echo '=== Relevant section around claimed lines ==='
sed -n '1210,1240p' "$file" | cat -n
echo
echo '=== AppTests synchronized group references ==='
rg -n 'AppTests|PBXFileSystemSynchronizedRootGroup|fileSystemSynchronizedGroups' "$file"
echo
echo '=== Main group children around references ==='
sed -n '1518,1540p' "$file" | cat -n
echo
echo '=== AppTests target section around fileSystemSynchronizedGroups ==='
sed -n '2208,2230p' "$file" | cat -nRepository: auth0/Auth0.swift
Length of output: 6897
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== AppTests files tracked in git ==='
git ls-files 'AppTests/**' | sed 's#^`#-` #'
echo
echo '=== Other synchronized root groups / duplicated paths in pbxproj ==='
python3 - <<'PY'
from pathlib import Path
import re
text = Path('Auth0.xcodeproj/project.pbxproj').read_text()
# collect root groups path -> ids
groups = re.findall(r'\b([A-F0-9]{24}) /\* ([^*]+) \*/ = \{isa = PBXFileSystemSynchronizedRootGroup;.*?path = ([^;]+);', text, re.S)
from collections import defaultdict
m = defaultdict(list)
for gid, label, path in groups:
m[path].append((gid, label))
for path, items in m.items():
if len(items) > 1:
print(path, '=>', items)
PYRepository: auth0/Auth0.swift
Length of output: 425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== AppTests files tracked in git ==='
git ls-files 'AppTests/**' | sed 's#^`#-` #'
echo
echo '=== Duplicate synchronized root groups in pbxproj ==='
python3 - <<'PY'
from pathlib import Path
import re
from collections import defaultdict
text = Path('Auth0.xcodeproj/project.pbxproj').read_text()
groups = re.findall(r'\b([A-F0-9]{24}) /\* ([^*]+) \*/ = \{isa = PBXFileSystemSynchronizedRootGroup;.*?path = ([^;]+);', text, re.S)
m = defaultdict(list)
for gid, label, path in groups:
m[path].append((gid, label))
for path, items in m.items():
if len(items) > 1:
print(f'{path} => {items}')
PYRepository: auth0/Auth0.swift
Length of output: 410
🌐 Web query:
PBXFileSystemSynchronizedRootGroup fileSystemSynchronizedGroups duplicate path Xcode
💡 Result:
The PBXFileSystemSynchronizedRootGroup is a file structure type introduced in Xcode 16 that represents directories as native "synced folders" rather than traditional virtual PBXGroup containers [1][2]. Issues involving duplicate paths or "Unknown ISA" errors related to this object typically arise from compatibility conflicts with external build tools or incorrect project structure logic [3][2][4]. Common causes and resolutions for these issues include: 1. Compatibility Errors (CocoaPods/Xcodeproj) If you encounter a RuntimeError: PBXGroup attempted to initialize an object with unknown ISA PBXFileSystemSynchronizedRootGroup, it is because the version of the xcodeproj gem or CocoaPods in use does not recognize this newer object type [3][2]. - Solution: Update your tools to versions that support Xcode 16 features (e.g., xcodeproj 1.26.0 or later) [2]. - Workaround: Some users manually edit the project.pbxproj file to replace occurrences of PBXFileSystemSynchronizedRootGroup with PBXGroup, though updating the gem is the recommended, permanent fix [1][2]. 2. Duplicate Paths in Project Generation (e.g., XcodeGen) When using project generation tools like XcodeGen, referencing the same directory path across multiple targets can cause the tool to incorrectly generate redundant PBXFileSystemSynchronizedRootGroup objects, leading to duplicate entries in the project navigator [5][4]. - Resolution: Ensure your project generator is configured to deduplicate these references. Recent patches in tools like XcodeGen have introduced caching mechanisms (e.g., syncedGroupsByPath) to identify existing synced root groups and reuse them across targets rather than creating duplicates [5][6][7]. 3. Duplicate Exception Sets If you are programmatically manipulating synced folders, creating multiple PBXFileSystemSynchronizedBuildFileExceptionSet objects for the same target will result in duplicate exception sets in the project file [8]. - Resolution: Logic must be implemented to check for an existing exception set for a specific target before adding new files, appending to the existing set instead [8].
Citations:
- 1: https://stackoverflow.com/questions/78984326/cocoapods-pod-init-fails-with-unknown-isa-pbxfilesystemsynchronizedrootgroup-e
- 2: https://bleepingswift.com/blog/pod-install-fails-with-unknow-isa
- 3: PBXFileSystemSynchronizedRootGroup causing pod init to fail with CocoaPods 1.15.2a CocoaPods/CocoaPods#12627
- 4: Synced folders:
includessilently ignored + no deduplication across targets yonaskolb/XcodeGen#1602 - 5: Fix issue 1602 - Synced folders: includes silently ignored + no deduplication across targets yonaskolb/XcodeGen#1604
- 6: Fix synced folder issues: configFiles duplicate, group insertion, and directory-level membershipExceptions yonaskolb/XcodeGen#1607
- 7: yonaskolb/XcodeGen@df251b2
- 8: toba/xc-mcp@a7e8d49
Remove the duplicate AppTests synchronized root group
D4919A702F55A38C007EA4B0 and D4919A762F55A3BD007EA4B0 both point to path = AppTests and are both added to the main group and the AppTests target. Keep only one entry and drop the extra references so the folder is synchronized once.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Auth0.xcodeproj/project.pbxproj` around lines 1224 - 1227, Remove the
duplicate PBXFileSystemSynchronizedRootGroup entry for AppTests, keeping only
one of D4919A702F55A38C007EA4B0 and D4919A762F55A3BD007EA4B0. Delete the
discarded identifier from the main group and AppTests target references so the
AppTests folder is synchronized exactly once.
| try? self.clearAll() | ||
| throw CredentialsManagerError.dpopKeyMissing | ||
| } | ||
|
|
||
| // Hash the current thumbprint to compare against the stored hash | ||
| let currentThumbprint = try dpop.jkt() | ||
| if let stored = storedThumbPrintValue { | ||
| if stored != currentThumbprint { | ||
| try self.clear() | ||
| try? self.clearAll() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and inspect the surrounding section.
git ls-files 'Auth0/CredentialsManager.swift'
wc -l Auth0/CredentialsManager.swift
sed -n '780,840p' Auth0/CredentialsManager.swift
# Inspect nearby error definitions and doc comments for the mentioned cases.
rg -n "dpopKeyMissing|dpopKeyMismatch|clearAll|noCredentials|renewFailed|ssoExchangeFailed|apiExchangeFailed" Auth0/CredentialsManager.swift Auth0 -g '!**/*.xc*'Repository: auth0/Auth0.swift
Length of output: 9055
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and inspect the surrounding section.
git ls-files 'Auth0/CredentialsManager.swift'
wc -l Auth0/CredentialsManager.swift
sed -n '780,840p' Auth0/CredentialsManager.swift
# Inspect nearby error definitions and doc comments for the mentioned cases.
rg -n "dpopKeyMissing|dpopKeyMismatch|clearAll|noCredentials|renewFailed|ssoExchangeFailed|apiExchangeFailed" Auth0/CredentialsManager.swift Auth0 -g '!**/*.xcx'Repository: auth0/Auth0.swift
Length of output: 9055
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '200,260p' Auth0/CredentialsManager.swift
sed -n '1,170p' Auth0/CredentialsManagerError.swiftRepository: auth0/Auth0.swift
Length of output: 11118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation of clearAll and any related helper methods.
sed -n '260,360p' Auth0/CredentialsManager.swift
sed -n '360,460p' Auth0/CredentialsManager.swiftRepository: auth0/Auth0.swift
Length of output: 9509
Don’t swallow clearAll() failures here. clearAll() is documented to throw, so try? hides a real cleanup failure and replaces it with .dpopKeyMissing / .dpopKeyMismatch. Let the error propagate from both call sites.
Fix
- try? self.clearAll()
+ try self.clearAll()
throw CredentialsManagerError.dpopKeyMissing
...
- try? self.clearAll()
+ try self.clearAll()
throw CredentialsManagerError.dpopKeyMismatch🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Auth0/CredentialsManager.swift` around lines 804 - 812, Update both
clearAll() call sites in the DPoP validation flow to propagate cleanup errors
instead of suppressing them with try?. Preserve the existing subsequent
dpopKeyMissing and dpopKeyMismatch throws only when clearAll() succeeds.
4 participants
Summary
clear()withclearAll()inCredentialsManagererror/cleanup paths (DPoP key missing, DPoP key mismatch, and expired-session handling) so all stored credentials are removed, not just the default set.Test plan
swift testswiftlint lintSummary by CodeRabbit