feat: add security notifications for remote access#17
Merged
Conversation
added 8 commits
May 24, 2026 10:29
📝 WalkthroughWalkthroughThis PR implements remote-access security: stable client device IDs, relay durable security state, first-time device detection with multi-channel alerts, emergency lockdown with single-use action tokens, client-side persistence and UI for security events, push registration, and a CLI unlock flow. ChangesRemote Access Security: Device Tracking, Notifications, and Lockdown
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app/lib/client-device.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. app/lib/push-registration.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. app/lib/relay-transport.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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: 8
🧹 Nitpick comments (4)
relay/src/security-delivery.ts (2)
72-79: ⚡ Quick winSilent failure on push delivery may hide critical alert failures.
Same concern as email: the Expo push call doesn't check
res.ok. Push notifications are a key alert channel for security events.Proposed fix to add error logging
- await fetch("https://exp.host/--/api/v2/push/send", { + const res = await fetch("https://exp.host/--/api/v2/push/send", { method: "POST", headers: { Accept: "application/json", "Content-Type": "application/json", }, body: JSON.stringify(messages), }); + if (!res.ok) { + console.error(`Security push delivery failed: ${res.status} ${res.statusText}`); + }🤖 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 `@relay/src/security-delivery.ts` around lines 72 - 79, The Expo push fetch currently ignores the response, causing silent failures; update the push-sending logic (the fetch call that posts JSON.stringify(messages)) to await and capture the response, check response.ok, and on failure call the existing logger/error handler with contextual details (status, statusText and response body or text) and surface or throw an error so callers know delivery failed; ensure you reference the same messages payload and preserve headers while adding the response check and logging.
31-44: ⚡ Quick winSilent failure on email delivery may hide critical alert failures.
The
fetchcall to Resend doesn't checkres.okor log failures. For security alerts, silent failures could mean users never learn about unauthorized access attempts. Consider logging errors for observability.Proposed fix to add error logging
- await fetch("https://api.resend.com/emails", { + const res = await fetch("https://api.resend.com/emails", { method: "POST", headers: { Authorization: `Bearer ${input.env.RESEND_API_KEY}`, "Content-Type": "application/json", }, body: JSON.stringify({ from: input.env.SECURITY_EMAIL_FROM, to: [email], subject: input.event.title, html, text: securityEmailText(input.event, input.emergencyUrl), }), }); + if (!res.ok) { + console.error(`Security email delivery failed: ${res.status} ${res.statusText}`); + }🤖 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 `@relay/src/security-delivery.ts` around lines 31 - 44, The fetch call to Resend that sends the security email should check the HTTP response and surface failures instead of silently ignoring them; update the logic around the existing fetch(...) call to capture the response, verify res.ok, and on non-ok responses log the status and body (or error) along with contextual info (e.g., input.event.title, email, and input.emergencyUrl) using the project's logger (or console.error if no logger is available); also handle fetch exceptions with a try/catch to log the thrown error so failed deliveries for securityEmailText notifications are observable.relay/src/relay-object.ts (1)
382-388: 💤 Low valueMissing deviceId always causes rejection when policy is "enforce".
When
SECURITY_DEVICE_POLICY === "enforce", a client without a deviceId (e.g., older client versions) will have all requests rejected with 403. This could break backward compatibility. Consider whether this is intentional or if you want a more graceful fallback.🤖 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 `@relay/src/relay-object.ts` around lines 382 - 388, The current shouldRejectClientRequest in relay-object.ts rejects any client without a deviceId when SECURITY_DEVICE_POLICY === "enforce", breaking older clients; update shouldRejectClientRequest (and references to clientDeviceIds and deviceIdFromTags) so that a missing deviceId does not automatically cause rejection—return false (allow) instead of true for absent deviceId, and optionally log a warning; keep the existing loadSecurityState and isDeviceApproved flow for cases where deviceId exists so approved devices remain enforced.src/relay-client.test.ts (1)
31-50: ⚡ Quick winAdd a negative-path test for non-
client_connectedsecurity events.This ensures the notification bridge only fires for the intended event kind and prevents regressions as new security event kinds are added.
Suggested test addition
+ it("does not notify for other security_event kinds", async () => { + const daemon = { routeRequest: vi.fn() } as unknown as AimuxDaemon; + const client = new RelayClient("wss://relay.aimux.app/", "token", daemon); + const message = JSON.stringify({ + type: "security_event", + event: { + kind: "lockdown_enabled", + title: "Lockdown enabled", + body: "All remote clients were disconnected", + createdAt: new Date().toISOString(), + }, + }); + + await (client as unknown as { handleMessage(data: string): Promise<void> }).handleMessage(message); + expect(notifyRemoteClientConnected).not.toHaveBeenCalled(); + });🤖 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 `@src/relay-client.test.ts` around lines 31 - 50, Add a negative-path unit test that ensures RelayClient.handleMessage does not call notifyRemoteClientConnected for security_event messages whose event.kind is not "client_connected": create a RelayClient instance (as in the existing test), craft a JSON message with type "security_event" and event.kind set to some other value (e.g., "client_disconnected"), call (client as unknown as { handleMessage(data: string): Promise<void> }).handleMessage(message), and assert notifyRemoteClientConnected was not called (and reset or spy on the mock as needed). This should reference the RelayClient class, its handleMessage method, and the notifyRemoteClientConnected mock used in the test.
🤖 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 `@app/lib/client-device.ts`:
- Around line 30-36: getOrCreateDeviceId can race when called concurrently;
introduce a module-level cache/in-flight guard (e.g., cachedDeviceId and
deviceIdPromise) so concurrent callers share the same resolution: first check
cachedDeviceId and return if present, else if deviceIdPromise exists return it,
otherwise set deviceIdPromise to an async flow that calls readDeviceId,
generates next via randomId, writes via writeDeviceId, assigns cachedDeviceId,
clears deviceIdPromise, and returns the id; update references to
getOrCreateDeviceId to rely on this memoized behavior.
In `@app/lib/push-registration.ts`:
- Around line 19-21: Before calling Notifications.getExpoPushTokenAsync in
app/lib/push-registration.ts, ensure Android notification channel is configured
by calling Notifications.setNotificationChannelAsync first (only on Android).
Update the registration flow to detect Android (Platform.OS === 'android') and
call Notifications.setNotificationChannelAsync('default', { name: 'default',
importance: Notifications.AndroidImportance.MAX, vibrationPattern: [0, 250, 250,
250], lightColor: '`#FF231F7C`' }) or similar channel settings, then call
Notifications.getExpoPushTokenAsync (preserving the existing projectId param) so
token retrieval won't be blocked on Android.
In `@app/lib/relay-transport.ts`:
- Around line 207-213: The handler trusts msg.event as a SecurityEventRecord and
may pass malformed payloads to this.securityEventListeners; update the block
that checks msg.type === "security_event" to validate that msg.event is a
non-null object and that required string fields defined by SecurityEventRecord
are present (e.g., check typeof field === "string" for each required property),
only then call listener(msg.event); otherwise skip notifying listeners and emit
a warning or debug log (use the existing logger) so invalid payloads are dropped
safely.
- Around line 92-93: connect() currently awaits this.getDeviceInfo() without
catching rejection, which can cause an unhandled rejection and skip the normal
disconnect/reconnect/status updates; wrap the getDeviceInfo() call in a
try/catch inside connect(), and on error set the transport status (e.g. via
this.setStatus or this.status) to a failed/disconnected state, call the same
disconnect/error handling path you use elsewhere (e.g. this.disconnect() or
this.handleDisconnect()), and ensure you schedule a reconnect (e.g.
this.scheduleReconnect()) before rethrowing or returning so the retry logic runs
consistently; keep the subsequent use of this.clientConnectUrl(deviceInfo)
inside the try block so it only runs when deviceInfo was obtained successfully.
In `@relay/src/security.ts`:
- Around line 149-153: hashIpAddress currently uses a raw SHA-256
(sha256Base64Url) which is reversible via dictionaries; change it to a keyed
HMAC using a secret key so IPs are pseudonymized. Replace the direct call to
sha256Base64Url inside hashIpAddress with an HMAC-SHA256 computed with a
securely-provided key (e.g. injected config or process.env IP_HASH_KEY) and
return the base64url result; ensure the function validates the presence of the
key and fails/throws or returns undefined if the key is missing, and keep the
normalization logic intact so callers still call hashIpAddress(ip).
- Around line 113-122: normalizeSecurityState currently preserves all entries in
state.actions causing unbounded growth; update normalizeSecurityState to compact
actions by filtering out expired or already-used/consumed actions and limit the
retained entries. Specifically, iterate over state.actions (the object passed
into normalizeSecurityState), remove any action where a TTL/expiry (e.g.,
expiresAt) is in the past or a used/consumed flag is true, and keep only the
most recent N actions (introduce a MAX_SECURITY_ACTIONS constant) before
returning the normalized actions map; this will prevent the token lookup routine
that scans actions from iterating over stale entries.
- Line 141: Don't collapse invalid/missing device IDs into the shared string
"unknown"; instead set deviceId to null/undefined (e.g., deviceId:
sanitizeId(input?.deviceId) ?? null) or generate a per-request distinct
placeholder and update the merge logic that currently combines clients (the
block that sets/merges approvedAt/blockedAt for device records) so it treats
null/undefined (or distinct placeholders) as non-equal and does not merge those
entries. Update any checks that detect "first-time-device" to rely on the
presence of a real deviceId (not equality to "unknown") and ensure sanitizeId,
deviceId, and the merging code that touches approvedAt/blockedAt are adjusted
accordingly.
In `@src/notify.ts`:
- Around line 96-98: The notifyRemoteClientConnected handler currently uses
truthy fallback (input.title || ...) and can pass non-string truthy values into
downstream AppleScript escaping (escapeAppleScriptString) causing runtime
crashes; change notifyRemoteClientConnected so it explicitly validates/coerces
input.title and input.body to strings before calling sendSecurity (e.g., check
typeof input.title === "string" and typeof input.body === "string" and otherwise
use the default literals), and ensure sendSecurity/AppleScript dispatch always
receives plain strings (reference notifyRemoteClientConnected, sendSecurity, and
escapeAppleScriptString when making the fix).
---
Nitpick comments:
In `@relay/src/relay-object.ts`:
- Around line 382-388: The current shouldRejectClientRequest in relay-object.ts
rejects any client without a deviceId when SECURITY_DEVICE_POLICY === "enforce",
breaking older clients; update shouldRejectClientRequest (and references to
clientDeviceIds and deviceIdFromTags) so that a missing deviceId does not
automatically cause rejection—return false (allow) instead of true for absent
deviceId, and optionally log a warning; keep the existing loadSecurityState and
isDeviceApproved flow for cases where deviceId exists so approved devices remain
enforced.
In `@relay/src/security-delivery.ts`:
- Around line 72-79: The Expo push fetch currently ignores the response, causing
silent failures; update the push-sending logic (the fetch call that posts
JSON.stringify(messages)) to await and capture the response, check response.ok,
and on failure call the existing logger/error handler with contextual details
(status, statusText and response body or text) and surface or throw an error so
callers know delivery failed; ensure you reference the same messages payload and
preserve headers while adding the response check and logging.
- Around line 31-44: The fetch call to Resend that sends the security email
should check the HTTP response and surface failures instead of silently ignoring
them; update the logic around the existing fetch(...) call to capture the
response, verify res.ok, and on non-ok responses log the status and body (or
error) along with contextual info (e.g., input.event.title, email, and
input.emergencyUrl) using the project's logger (or console.error if no logger is
available); also handle fetch exceptions with a try/catch to log the thrown
error so failed deliveries for securityEmailText notifications are observable.
In `@src/relay-client.test.ts`:
- Around line 31-50: Add a negative-path unit test that ensures
RelayClient.handleMessage does not call notifyRemoteClientConnected for
security_event messages whose event.kind is not "client_connected": create a
RelayClient instance (as in the existing test), craft a JSON message with type
"security_event" and event.kind set to some other value (e.g.,
"client_disconnected"), call (client as unknown as { handleMessage(data:
string): Promise<void> }).handleMessage(message), and assert
notifyRemoteClientConnected was not called (and reset or spy on the mock as
needed). This should reference the RelayClient class, its handleMessage method,
and the notifyRemoteClientConnected mock used in the test.
🪄 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 Plus
Run ID: 9bc065c9-e609-4ba7-8a1a-3dade0d33821
⛔ Files ignored due to path filters (1)
app/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (27)
app/app/(main)/(tabs)/(inbox)/notifications.tsxapp/app/(main)/_layout.tsxapp/app/cli-auth.tsxapp/components/MobileTabBar.tsxapp/components/NotificationBellButton.tsxapp/components/ProjectSidebar.tsxapp/lib/client-device.tsapp/lib/push-registration.tsapp/lib/relay-transport.tsapp/package.jsonapp/stores/security.test.tsapp/stores/security.tsdocs/deployment.mddocs/notification-system.mddocs/security-notifications.mdrelay/src/daemon-token.tsrelay/src/index.tsrelay/src/relay-object.tsrelay/src/security-delivery.tsrelay/src/security.tsrelay/src/types.tsrelay/wrangler.tomlsrc/login-flow.tssrc/main.tssrc/notify.tssrc/relay-client.test.tssrc/relay-client.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/notify.ts (1)
96-99: Rebuilddist/before validating this runtime change.Since this is a
src/*.tsCLI/runtime change, runyarn buildat repo root before runtime verification, and restart/reload any running daemon/runtime to avoid stale behavior.As per coding guidelines: “changes to src/*.ts Node CLI require running yarn build at the repo root to update dist/ … [and] restart or reload the relevant runtime after the build.”
🤖 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 `@src/notify.ts` around lines 96 - 99, The runtime change in src/notify.ts affecting the function notifyRemoteClientConnected requires rebuilding the compiled outputs and restarting any running daemon to avoid stale behavior; run yarn build at the repo root to update dist/, then restart or reload the relevant CLI/runtime process so the updated notifyRemoteClientConnected (and its use of sendSecurity) is picked up before validating the change.
🤖 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.
Nitpick comments:
In `@src/notify.ts`:
- Around line 96-99: The runtime change in src/notify.ts affecting the function
notifyRemoteClientConnected requires rebuilding the compiled outputs and
restarting any running daemon to avoid stale behavior; run yarn build at the
repo root to update dist/, then restart or reload the relevant CLI/runtime
process so the updated notifyRemoteClientConnected (and its use of sendSecurity)
is picked up before validating the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 649c803d-7c2c-4624-a253-da2773b809b8
📒 Files selected for processing (9)
.env.exampleapp/lib/client-device.tsapp/lib/push-registration.tsapp/lib/relay-transport.tsdocs/deployment.mdrelay/src/relay-object.tsrelay/src/security.tsrelay/src/types.tssrc/notify.ts
✅ Files skipped from review due to trivial changes (1)
- .env.example
This was referenced May 24, 2026
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
Verification
Summary by CodeRabbit
New Features
Documentation