Share 10-min Touch ID context across org operations#390
Conversation
- Add shared LAContext with 600s reuse for org rename operations - Single Touch ID prompt covers all org renames within 10 minutes - Fix MCP tool descriptions: org_add and org_remove don't need Touch ID - Rebuild MCP bundle Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR implements LAContext caching with a 10-minute expiry window in SocketServer.swift to reduce redundant Touch ID evaluations. Corresponding user-facing documentation in the MCP server files is updated to reflect that Touch ID is no longer required for organization add/remove operations, but required once for rename with context reuse. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@NoxKey/SocketServer.swift`:
- Around line 27-35: orgContext is currently a global cached LAContext shared
across all socket clients which lets any same-UID caller reuse another caller's
auth for 10 minutes; change the cache to be per-caller identity (keyed by the
same identity used elsewhere: resolvedPid and resolvedStartTime or a single
combined CallerID) instead of a single orgContext. Replace the
single-orgContext/orgContextExpiry with a dictionary mapping CallerID ->
(LAContext, expiry) protected by orgContextLock, update orgAuthContext(reason:)
to look up and return the caller-specific LAContext (and to store a new context
under that caller key when created), and update any invalidation logic to remove
only that caller’s entry rather than clearing a global context.
- Around line 42-49: The current evaluatePolicy callback blocks indefinitely on
sem.wait(), risking a permanent hang while holding orgContextLock; change the
synchronous wait to a bounded wait using DispatchSemaphore.wait(timeout:) with
the same timeout used by authenticateForMutation and treat a timeout as failure
(return nil). Specifically, replace the unconditional sem.wait() with a timed
wait (DispatchTime.now() + the authenticateForMutation timeout constant/value)
and check the wait result before the existing guard ok else { return nil } so
that if the semaphore times out you fail closed.
- Around line 1571-1574: The auth flow is still prompting too early: update
handleOrg to stop calling authenticateForMutation for the "add" and "remove"
actions and to avoid the outer authenticateForMutation before invoking orgRename
so that orgRename can solely rely on orgAuthContext; specifically, remove or
short-circuit the authenticateForMutation invocation in handleOrg for cases
"add" and "remove" and ensure orgRename continues to call
orgAuthContext(reason:) (and only falls back to sendResponse on nil), so that
add/remove use the inner orgAuthContext behavior and rename uses the cached
orgAuthContext without an extra outer prompt.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c553aebd-6184-41c5-bcfe-8d9a29c61d44
📒 Files selected for processing (3)
NoxKey/SocketServer.swiftNoxKey/noxkey-mcp.mjsmcp-server/src/index.ts
| private var orgContext: LAContext? | ||
| private var orgContextExpiry: Date = .distantPast | ||
| private let orgContextLock = NSLock() | ||
|
|
||
| private func orgAuthContext(reason: String) -> LAContext? { | ||
| orgContextLock.lock() | ||
| defer { orgContextLock.unlock() } | ||
| if let ctx = orgContext, Date() < orgContextExpiry { | ||
| return ctx |
There was a problem hiding this comment.
Scope the cached LAContext to the caller, not the whole server.
orgContext is shared across every socket client. Once one process authenticates, any other same-UID caller can reuse that context for the next 10 minutes and rename org secrets without its own prompt. That’s a broader trust boundary than the rest of this file, which keys access by resolvedPid and resolvedStartTime. Cache this per caller identity instead of process-wide.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NoxKey/SocketServer.swift` around lines 27 - 35, orgContext is currently a
global cached LAContext shared across all socket clients which lets any same-UID
caller reuse another caller's auth for 10 minutes; change the cache to be
per-caller identity (keyed by the same identity used elsewhere: resolvedPid and
resolvedStartTime or a single combined CallerID) instead of a single orgContext.
Replace the single-orgContext/orgContextExpiry with a dictionary mapping
CallerID -> (LAContext, expiry) protected by orgContextLock, update
orgAuthContext(reason:) to look up and return the caller-specific LAContext (and
to store a new context under that caller key when created), and update any
invalidation logic to remove only that caller’s entry rather than clearing a
global context.
| var ok = false | ||
| let sem = DispatchSemaphore(value: 0) | ||
| ctx.evaluatePolicy(policy, localizedReason: reason) { success, _ in | ||
| ok = success | ||
| sem.signal() | ||
| } | ||
| sem.wait() | ||
| guard ok else { return nil } |
There was a problem hiding this comment.
Add a timeout to the LocalAuthentication wait path.
This helper blocks on sem.wait() indefinitely while still holding orgContextLock. If evaluatePolicy never calls back, one hung auth attempt stalls all later org renames. Match authenticateForMutation here and fail closed after a bounded timeout.
Suggested fix
var ok = false
let sem = DispatchSemaphore(value: 0)
ctx.evaluatePolicy(policy, localizedReason: reason) { success, _ in
ok = success
sem.signal()
}
- sem.wait()
- guard ok else { return nil }
+ guard sem.wait(timeout: .now() + 60) == .success, ok else { return nil }
orgContext = ctx
orgContextExpiry = Date().addingTimeInterval(600)
return ctx📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var ok = false | |
| let sem = DispatchSemaphore(value: 0) | |
| ctx.evaluatePolicy(policy, localizedReason: reason) { success, _ in | |
| ok = success | |
| sem.signal() | |
| } | |
| sem.wait() | |
| guard ok else { return nil } | |
| var ok = false | |
| let sem = DispatchSemaphore(value: 0) | |
| ctx.evaluatePolicy(policy, localizedReason: reason) { success, _ in | |
| ok = success | |
| sem.signal() | |
| } | |
| guard sem.wait(timeout: .now() + 60) == .success, ok else { return nil } | |
| orgContext = ctx | |
| orgContextExpiry = Date().addingTimeInterval(600) | |
| return ctx |
🧰 Tools
🪛 ast-grep (0.42.1)
[info] 43-43: The application was observed to leverage biometrics via Local
Authentication, which returns a simple boolean result for authentication.
This design is subject to bypass with runtime tampering tools such as
Frida, Substrate, and others. Although this is limited to rooted
(jailbroken) devices, consider implementing biometric authentication the
reliable way - via Keychain Services.
Context: ctx.evaluatePolicy
Note: [CWE-305] Authentication Bypass by Primary Weakness [REFERENCES]
- https://mobile-security.gitbook.io/mobile-security-testing-guide/ios-testing-guide/0x06f-testing-local-authentication
- https://shirazkhan030.medium.com/biometric-authentication-in-ios-6c53c54f17df
(insecure-biometrics-swift)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NoxKey/SocketServer.swift` around lines 42 - 49, The current evaluatePolicy
callback blocks indefinitely on sem.wait(), risking a permanent hang while
holding orgContextLock; change the synchronous wait to a bounded wait using
DispatchSemaphore.wait(timeout:) with the same timeout used by
authenticateForMutation and treat a timeout as failure (return nil).
Specifically, replace the unconditional sem.wait() with a timed wait
(DispatchTime.now() + the authenticateForMutation timeout constant/value) and
check the wait result before the existing guard ok else { return nil } so that
if the semaphore times out you fail closed.
| // Use shared org auth context (10-min reuse window — single Touch ID for batch org operations) | ||
| guard let context = orgAuthContext(reason: "Rename organization \"\(fromOrg)\" → \"\(toOrg)\" — \(caller.label)") else { | ||
| sendResponse(clientSocket, ok: false, error: "Authentication failed for secret migration") | ||
| return |
There was a problem hiding this comment.
This still won’t deliver the advertised org auth behavior.
orgRename now has a cached context, but handleOrg still runs authenticateForMutation for every action except list before this code is reached. So rename still incurs the outer prompt, and add / remove still require Touch ID. To match the new behavior, remove the outer auth gate for add and remove, and let rename rely on orgAuthContext alone.
Suggested fix outside this hunk
private func handleOrg(_ clientSocket: Int32, _ request: [String: Any], _ caller: CallerInfo) {
let action = request["action"] as? String ?? "list"
- // "list" needs no auth — everything else gets one Touch ID prompt
- if action != "list" {
+ // Only actions that mutate Keychain-backed secrets need auth here.
+ // `rename` performs its own cached auth via `orgAuthContext`.
+ if action == "some_future_mutation_requiring_global_auth" {
guard authenticateForMutation(reason: "Manage organizations — \(caller.label)") else {
sendResponse(clientSocket, ok: false, error: "Authentication canceled")
return
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NoxKey/SocketServer.swift` around lines 1571 - 1574, The auth flow is still
prompting too early: update handleOrg to stop calling authenticateForMutation
for the "add" and "remove" actions and to avoid the outer
authenticateForMutation before invoking orgRename so that orgRename can solely
rely on orgAuthContext; specifically, remove or short-circuit the
authenticateForMutation invocation in handleOrg for cases "add" and "remove" and
ensure orgRename continues to call orgAuthContext(reason:) (and only falls back
to sendResponse on nil), so that add/remove use the inner orgAuthContext
behavior and rename uses the cached orgAuthContext without an extra outer
prompt.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a shared authentication context in SocketServer.swift to allow Touch ID reuse for organization management within a 10-minute window. It also updates the MCP server documentation and tool descriptions to reflect that certain organization operations no longer require immediate biometric authentication. Feedback suggests logging specific authentication errors for better diagnostics and addresses a potential thread starvation issue caused by synchronous blocking during the authentication process.
| var authError: NSError? | ||
| guard ctx.canEvaluatePolicy(policy, error: &authError) else { return nil } |
There was a problem hiding this comment.
It would be beneficial to log the authError if canEvaluatePolicy fails. This information is crucial for diagnosing why biometric authentication might be unavailable (e.g., biometry locked out, not enrolled, or hardware issues), which is more helpful than a generic failure message.
| private func orgAuthContext(reason: String) -> LAContext? { | ||
| orgContextLock.lock() | ||
| defer { orgContextLock.unlock() } | ||
| if let ctx = orgContext, Date() < orgContextExpiry { | ||
| return ctx | ||
| } | ||
| let ctx = LAContext() | ||
| ctx.touchIDAuthenticationAllowableReuseDuration = 600 // 10 minutes | ||
| let policy = KeychainManager.authPolicy | ||
| var authError: NSError? | ||
| guard ctx.canEvaluatePolicy(policy, error: &authError) else { return nil } | ||
| var ok = false | ||
| let sem = DispatchSemaphore(value: 0) | ||
| ctx.evaluatePolicy(policy, localizedReason: reason) { success, _ in | ||
| ok = success | ||
| sem.signal() | ||
| } | ||
| sem.wait() | ||
| guard ok else { return nil } | ||
| orgContext = ctx | ||
| orgContextExpiry = Date().addingTimeInterval(600) | ||
| return ctx | ||
| } |
There was a problem hiding this comment.
The orgAuthContext method uses a synchronous wait (sem.wait()) while holding an NSLock. While this correctly prevents multiple simultaneous Touch ID prompts, it blocks the calling thread (likely from clientQueue) for the entire duration of the user interaction. In a server environment, this can lead to thread starvation if multiple requests are pending. Consider using an asynchronous approach or ensuring that the thread pool is sufficiently sized to handle these long-running blocking operations.
1 participant
Summary
LAContextwith 600s reuse window for org rename operations — one Touch ID prompt covers all renames within 10 minutesorg_addandorg_removedon't touch Keychain, so no Touch ID neededTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation