Add Touch ID denial cooldown and improve agent error messages#286
Open
JasperNoBoxDev wants to merge 6 commits intomainfrom
Open
Add Touch ID denial cooldown and improve agent error messages#286JasperNoBoxDev wants to merge 6 commits intomainfrom
JasperNoBoxDev wants to merge 6 commits intomainfrom
Conversation
Three fixes: 1. **Denial cooldown (5 min)**: When the owner cancels a Touch ID prompt, subsequent requests for the same prefix are blocked for 5 minutes without showing another prompt. Prevents background processes from spamming Touch ID dialogs after the owner says no. 2. **Clearer agent-blocked errors**: `--raw`, `--copy`, `load`, `env`, `export`, and `bundle` now say "permanently blocked" and "cannot be bypassed with unlock, tokens, or any other method" — stops agents from hallucinating workarounds like unlock + --raw. 3. **Update command sudo fallback**: `noxkey update` now falls back to the macOS Installer GUI when sudo is unavailable (non-interactive terminals, Claude Code, scripts). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 26 minutes and 47 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded a 5-minute owner-denial cooldown tracked in SessionManager and enforced across SocketServer unlock flows; expanded UnlockResult semantics to mark cancellations; updated noxkey-cli messages and macOS install fallback behavior. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Server as SocketServer
participant Session as SessionManager
participant Keychain as Keychain
Client->>Server: Request (GET / unlock / meta / batch) with prefix
Server->>Session: denialRemaining(prefix)
alt Denial active
Session-->>Server: Remaining seconds > 0
Server-->>Client: ok: false, "do not retry — try again in X minutes" (denied_cooldown)
else No denial
Server->>Keychain: Attempt Touch ID / auth unlock
alt Unlock success
Keychain-->>Server: Credentials
Server->>Session: clearDenial(prefix)
Server-->>Client: ok: true, secret/metadata
else User cancelled (LAError -> count == -2)
Server->>Session: recordDenial(prefix)
Session-->>Server: recorded timestamp
Server-->>Client: ok: false, "Access denied by owner… cooldown is Y minutes"
else Auth failed (other)
Server-->>Client: ok: false, "Authentication failed"
end
end
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 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 |
![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 denial cooldown mechanism to prevent repeated authentication prompts after a user denies access. It adds logic to SessionManager to track and enforce a 5-minute cooldown period for specific prefixes, with the SocketServer updated to check for this cooldown during GET and UNLOCK requests. Additionally, the noxkey-cli is improved with clearer error messages for AI agents and a more robust installation process that falls back to the GUI installer if sudo is unavailable. I have no feedback to provide as there were no review comments to evaluate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-cli`:
- Around line 648-656: The script is being terminated by set -e when sudo
installer returns non-zero, so _install_ok is never set and the GUI fallback
never runs; change the sudo invocation logic in the installation block that sets
_install_ok to explicitly capture failures without letting set -e exit the
script — e.g., wrap the call in a conditional (use if sudo installer ...; then
_install_ok=0; else _install_ok=$?; fi) or otherwise append a safe fallback (||
_install_ok=$?) so that the exit status of sudo installer is recorded into
_install_ok and the subsequent GUI fallback code (the block referencing
_install_ok) can execute as intended.
In `@NoxKey/SessionManager.swift`:
- Around line 297-317: The current helpers recordDenial(prefix:) and
denialRemaining(prefix:) assume callers can distinguish an explicit owner-cancel
from other auth failures, but unlock(...) still collapses all failures into
count == -1; change unlock(...) to return a distinct outcome (e.g. an enum
AuthOutcome { success, failure, ownerDenied } or a dedicated ownerDenied
sentinel) instead of overloading -1, then only call recordDenial(prefix:) when
unlock(...) returns the ownerDenied outcome; update any call sites that
currently check count == -1 to handle the new enum/sentinel and preserve
existing logic for ordinary failures and successful unlocks so cooldowns are
only recorded for explicit owner denials.
In `@NoxKey/SocketServer.swift`:
- Line 612: The server error texts must retain the literal "canceled" sentinel
so clients (noxkey-cli parse_response) can detect auth denial; update the
sendResponse calls in SocketServer.swift that currently use "cancelled" to
include "canceled" (e.g., the sendResponse(...) call sending "Access denied by
owner. The owner cancelled authentication..." and the other two similar
sendResponse(...) occurrences) so the message contains the exact word "canceled"
while preserving the rest of the message and pluralization logic; ensure all
three locations (the sendResponse invocations referenced in the review) are
changed from "cancelled" to "canceled" so parse_response can mirror the
denial/cooldown text.
- Around line 608-614: The cooldown denial logic (session.denialRemaining /
audit.log / sendResponse) must be centralized behind a single shared unlock
wrapper so all code paths (current GET/unlock and other callers like
handleConvert, handleBatchGet, and handleMeta) go through the same enforcement;
implement a new function (e.g., SessionManager.unlockWithCooldown or
SocketServer.performUnlockWithCooldown) that checks session.denialRemaining,
logs the denied_cooldown audit, sends the denial response, and only if allowed
calls session.unlock and handles recording/clearing denials, then replace direct
calls to session.unlock in handleConvert, handleBatchGet, handleMeta and
existing GET/unlock handlers with this wrapper. Ensure the wrapper returns the
same success/error semantics so callers do not need extra handling.
🪄 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: c98e5e15-202d-47aa-b0a7-4872c491ae04
📒 Files selected for processing (3)
NoxKey/SessionManager.swiftNoxKey/SocketServer.swiftnoxkey-cli
Comment on lines
+297
to
+317
| /// Record that the owner denied access for this prefix. | ||
| func recordDenial(prefix: String) { | ||
| lock.lock() | ||
| denials[prefix] = Date() | ||
| lock.unlock() | ||
| } | ||
|
|
||
| /// Check if a prefix is in denial cooldown. Returns remaining seconds, or nil if not denied. | ||
| func denialRemaining(prefix: String) -> TimeInterval? { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| for (deniedPrefix, deniedAt) in denials { | ||
| if prefix.hasPrefix(deniedPrefix) || deniedPrefix.hasPrefix(prefix) { | ||
| let elapsed = Date().timeIntervalSince(deniedAt) | ||
| let remaining = Self.denialCooldown - elapsed | ||
| if remaining > 0 { return remaining } | ||
| denials.removeValue(forKey: deniedPrefix) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Make “owner denied” an explicit unlock outcome.
These helpers are only correct if the caller can tell an explicit owner cancel from every other auth failure. unlock(...) (Lines 219-230) still collapses all unsuccessful authentication paths into count == -1, so the new cooldown can be recorded for ordinary auth failures too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NoxKey/SessionManager.swift` around lines 297 - 317, The current helpers
recordDenial(prefix:) and denialRemaining(prefix:) assume callers can
distinguish an explicit owner-cancel from other auth failures, but unlock(...)
still collapses all failures into count == -1; change unlock(...) to return a
distinct outcome (e.g. an enum AuthOutcome { success, failure, ownerDenied } or
a dedicated ownerDenied sentinel) instead of overloading -1, then only call
recordDenial(prefix:) when unlock(...) returns the ownerDenied outcome; update
any call sites that currently check count == -1 to handle the new enum/sentinel
and preserve existing logic for ordinary failures and successful unlocks so
cooldowns are only recorded for explicit owner denials.
Comment on lines
+608
to
+614
| // Check if owner recently denied access to this prefix | ||
| if let remaining = session.denialRemaining(prefix: prefix) { | ||
| let mins = Int(ceil(remaining / 60)) | ||
| audit.log(operation: "GET", account: account, result: "denied_cooldown", caller: caller.name, isAgent: caller.isAgent) | ||
| sendResponse(clientSocket, ok: false, error: "Access denied by owner. The owner cancelled authentication for this prefix. Do not retry — try again in \(mins) minute\(mins == 1 ? "" : "s").") | ||
| return | ||
| } |
There was a problem hiding this comment.
Enforce the cooldown in one shared unlock path.
These guards only cover get and unlock. handleConvert (Line 766), handleBatchGet (Line 940), and the unlock path in handleMeta (Line 1130) still call session.unlock(...) directly, so an agent can keep surfacing Touch ID prompts after a denial by switching commands. Move the denial check/record/clear flow behind a shared unlock wrapper.
Also applies to: 1182-1188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NoxKey/SocketServer.swift` around lines 608 - 614, The cooldown denial logic
(session.denialRemaining / audit.log / sendResponse) must be centralized behind
a single shared unlock wrapper so all code paths (current GET/unlock and other
callers like handleConvert, handleBatchGet, and handleMeta) go through the same
enforcement; implement a new function (e.g., SessionManager.unlockWithCooldown
or SocketServer.performUnlockWithCooldown) that checks session.denialRemaining,
logs the denied_cooldown audit, sends the denial response, and only if allowed
calls session.unlock and handles recording/clearing denials, then replace direct
calls to session.unlock in handleConvert, handleBatchGet, handleMeta and
existing GET/unlock handlers with this wrapper. Ensure the wrapper returns the
same success/error semantics so callers do not need extra handling.
…all commands - Distinguish user cancellation (count=-2) from generic auth failure (count=-1) using LAError codes — only record denial cooldown on explicit user cancel - Add denial cooldown checks to handleConvert, handleBatchGet, and handleMeta so agents can't bypass cooldown by switching commands - Fix 'cancelled' → 'canceled' spelling to match CLI parse_response pattern Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Linter kept reverting the evaluatePolicy callback change. This commit re-applies the LAError detection so only explicit user cancellation triggers the denial cooldown, not hardware/biometric failures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Under set -euo pipefail, sudo installer failing inside an if/elif body exits the script before reaching the GUI fallback. Use '&& var=0 || var=$?' to capture the exit code without triggering early exit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 tasks
…leanup - SessionManager: fix clearDenial — collect keys to array before mutating dict (Dictionary.filter returns tuples array, not Dictionary) - SocketServer: _ = semaphore.wait(timeout:) to silence unused-result warnings; distinguish -2 (cancel) from other negative auth failures - noxkey-cli: re-read mtime inside pgrep check to fix race where installer closes before we sample; add rm -f before timeout exit Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
Addressed all review issues from Gemini and CodeRabbit:
Build: succeeded with no errors or warnings (excluding expected Spotlight build phase advisory). |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
noxkey-cli (1)
71-79:⚠️ Potential issue | 🟠 MajorBlocked-agent guidance still won’t reach stdout-only callers, and
--copynever uses it.Line 403 later in this script already mirrors auth-denial text to stdout because agent callers read stdout, but this helper only writes to stderr. Also,
getstill only routes--rawthroughblock_ai_agent, so the new “permanently blocked” message never appears for--copyat all.Possible fix
block_ai_agent() { local cmd_name="$1" local reason="$2" if is_ai_agent; then - echo "Error: '$cmd_name' is permanently blocked for AI agents — it $reason." >&2 - echo "This cannot be bypassed with unlock, tokens, or any other method." >&2 - echo "Instead, use: eval \"\$(noxkey get <org/proj/KEY>)\"" >&2 - echo "This loads the secret as an env var without exposing the value." >&2 + local msgs=( + "Error: '$cmd_name' is permanently blocked for AI agents — it $reason." + "This cannot be bypassed with unlock, tokens, or any other method." + "Instead, use: eval \"\$(noxkey get <org/proj/KEY>)\"" + "This loads the secret as an env var without exposing the value." + ) + printf '%s\n' "${msgs[@]}" + printf '%s\n' "${msgs[@]}" >&2 exit 1 fi }- if [[ "$get_mode" == "raw" ]]; then - block_ai_agent "noxkey get --raw" "outputs the raw secret value" - fi + case "$get_mode" in + raw) block_ai_agent "noxkey get --raw" "outputs the raw secret value" ;; + copy) block_ai_agent "noxkey get --copy" "copies the secret value to the clipboard" ;; + esac🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@noxkey-cli` around lines 71 - 79, The block_ai_agent helper only writes the "permanently blocked" guidance to stderr and is only invoked for --raw, so stdout-only agent callers and the --copy code path never see the guidance; update block_ai_agent (the block_ai_agent() function) to mirror the three guidance echo lines to stdout as well as stderr (write the same messages to both), and modify the get command path that handles --copy so it routes through or calls block_ai_agent the same way --raw does (ensure the code path for handling --copy invokes block_ai_agent before exiting) so the blocked guidance is shown for both stdout-only callers and --copy.NoxKey/SocketServer.swift (1)
1128-1162:⚠️ Potential issue | 🟠 Major
metastill bypasses the cooldown once a session already exists.Lines 1150-1162 only enforce the denial flow in the
!hasSessionbranch. ThehasSessionbranch above still shows Touch ID directly, ignores any active denial, and on cancel just returns"Authentication canceled"withoutsession.recordDenial(prefix:). A background caller with an existing session can therefore keep surfacing dialogs throughmeta.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NoxKey/SocketServer.swift` around lines 1128 - 1162, The hasSession branch bypasses owner-denial checks and never records a denial on canceled re-auth; before calling LAContext.evaluatePolicy in the hasSession path, check session.denialRemaining(prefix:) and if non-nil return the same "Access denied by owner…" response used in the !hasSession branch, and if evaluatePolicy returns false call session.recordDenial(prefix:) before sendResponse so canceled Touch ID attempts are enforced into cooldown (refer to session.validateSession, session.denialRemaining(prefix:), session.recordDenial(prefix:), LAContext.evaluatePolicy and SessionManager.denialCooldown to mirror the existing denial logic).
🤖 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/SessionManager.swift`:
- Around line 223-234: The guard currently treats LAError.appCancel and
LAError.systemCancel as owner denials; change the logic in the
context.evaluatePolicy completion closure (where authSuccess, userCanceled and
semaphore are set) to only set userCanceled = true when the caught error is
.userCancel (do not include .appCancel or .systemCancel), leaving other
cancellation/error cases to fall through so the final guard returns
UnlockResult(count: userCanceled ? -2 : -1, context: nil) with userCanceled true
only for .userCancel; update the evaluation block around
context.evaluatePolicy(...) and the LAError check accordingly.
---
Outside diff comments:
In `@noxkey-cli`:
- Around line 71-79: The block_ai_agent helper only writes the "permanently
blocked" guidance to stderr and is only invoked for --raw, so stdout-only agent
callers and the --copy code path never see the guidance; update block_ai_agent
(the block_ai_agent() function) to mirror the three guidance echo lines to
stdout as well as stderr (write the same messages to both), and modify the get
command path that handles --copy so it routes through or calls block_ai_agent
the same way --raw does (ensure the code path for handling --copy invokes
block_ai_agent before exiting) so the blocked guidance is shown for both
stdout-only callers and --copy.
In `@NoxKey/SocketServer.swift`:
- Around line 1128-1162: The hasSession branch bypasses owner-denial checks and
never records a denial on canceled re-auth; before calling
LAContext.evaluatePolicy in the hasSession path, check
session.denialRemaining(prefix:) and if non-nil return the same "Access denied
by owner…" response used in the !hasSession branch, and if evaluatePolicy
returns false call session.recordDenial(prefix:) before sendResponse so canceled
Touch ID attempts are enforced into cooldown (refer to session.validateSession,
session.denialRemaining(prefix:), session.recordDenial(prefix:),
LAContext.evaluatePolicy and SessionManager.denialCooldown to mirror the
existing denial logic).
🪄 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: dcf99428-aecf-47fb-966e-fb34d8a7f196
📒 Files selected for processing (3)
NoxKey/SessionManager.swiftNoxKey/SocketServer.swiftnoxkey-cli
…ystemCancel System and app interruptions (device lock, programmatic cancel) should not trigger the 5-minute denial cooldown. Only the owner tapping Cancel in the Touch ID dialog (.userCancel) is an intentional denial. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
Addressed all open review feedback: CodeRabbit major issues:
Gemini critical/high issues:
|
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
1 participant
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.
Summary
--rawand friends now say "permanently blocked" and "cannot be bypassed" — prevents agents from hallucinating workarounds (unlock + --raw, tokens, etc.)noxkey updatefalls back to the macOS Installer GUI when sudo is unavailable (non-interactive terminals like Claude Code)Supersedes #284.
Test plan
noxkey get --rawfrom an AI agent context — verify "permanently blocked" messagenoxkey updatein a non-interactive terminal — verify GUI installer opens🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes