feat(web): operator login — session auth for the Agent Inbox (HT-51) - #67
Conversation
Adds a single-operator password gate in front of the whole Agent Inbox web app: an HMAC-signed session cookie (Web Crypto, so the same code runs in Next's Edge middleware and Node server actions), a middleware guard on every route except /login, and a new login screen (no prior design — flagged for sign-off) with a constant-time password check and a per-request failure delay. Wires the avatar menu's "Log out" stub to a real logout action. Amends agent-inbox-v1.md to record that this is a web-layer door in front of the unchanged Bearer-token API auth, and updates web/README.md's env var list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…selves
Server actions were relying entirely on middleware.ts for auth, but
Next.js dispatches actions by a build-stable action-ID hash invokable
against any route — including /login, the one path middleware waves
through unauthenticated. Each write/read action (sendReplyAction,
postNoteAction, setStatusAction, putTagsAction, putAssigneeAction,
deleteConversationAction, loadOlderAction) now verifies the session
cookie itself before touching the API, closing the bypass regardless
of which route dispatches the action.
Also references HT-52 (filed to track the two ds/** gaps LoginScreen
works around: TextInput's missing `type` prop and Button's missing
type="submit") from the LoginScreen module comment, so the workarounds
have an expiry path.
Two findings are flagged for TJ rather than resolved in code, per the
UI-fidelity mandate's sign-off gate:
- LoginScreen.tsx is a wholly new designed surface with no prototype
behind it — needs TJ's sign-off and an entry in the Claude Design
project + HT-23 fidelity checklist before this can be considered done.
- AuthFailure.tsx's copy ("there's nothing to sign into — this is
configuration, not a login") is now stale post-login, but that copy
is itself on the fidelity-checked surface, so the reword is sign-off
gated too rather than done silently.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds operator password login for the web app using signed HMAC session cookies, protects routes and server actions, supports logout and session refresh, sanitizes post-login paths, and documents the configuration and unchanged API Bearer authentication model. ChangesOperator session authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant middleware
participant LoginScreen
participant loginAction
participant session
Operator->>middleware: Request protected route
middleware->>session: Verify ht_session cookie
middleware-->>Operator: Redirect to /login with next path
Operator->>LoginScreen: Submit password
LoginScreen->>loginAction: Call loginAction(password, next)
loginAction->>session: Mint signed session cookie
loginAction-->>Operator: Redirect to sanitized destination
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@specs/api/agent-inbox-v1.md`:
- Around line 130-133: Update the authentication documentation to distinguish
protected application routes from the public-path exceptions permitted by
web/src/middleware.ts: specs/api/agent-inbox-v1.md lines 130-133, 388-393, and
427-432, plus web/README.md lines 55-59. Replace claims that every page or route
requires a session/cookie with “protected routes/pages” wording or explicitly
list /favicon.ico, /robots.txt, /sitemap.xml, and /manifest.webmanifest.
- Around line 395-396: Update the authentication statement near the Bearer
comparison to say “every authenticated API request” rather than “every request,”
and retain an explicit reference to the unauthenticated open-tracking pixel
exception documented in §4g.
In `@web/README.md`:
- Around line 26-39: Update the README description of HELPTHREAD_UI_* fallback
behavior to clarify that production-build time
(NEXT_PHASE=phase-production-build) may use development defaults because
uiAuthConfig() skips required-environment validation. State that the
no-fallback, fail-closed requirement applies during production runtime, and
preserve the existing local-development behavior.
In `@web/src/components/LoginScreen.tsx`:
- Around line 94-104: Replace human-support “operator” terminology with “Agent”
consistently: update the visible password copy in
web/src/components/LoginScreen.tsx lines 94-104, and revise the related
documentation in web/src/lib/session.ts lines 2-9, web/src/middleware.ts lines
9-23, web/src/lib/actions.ts lines 54-64, web/src/lib/next-path.ts lines 1-19,
web/src/lib/auth-actions.ts lines 3-10, and web/src/app/login/page.tsx lines
9-16. Keep “Assistants” reserved for AI actors.
- Around line 127-166: Update the password field and the conditional error
element in LoginScreen so rejected credentials are announced to assistive
technology: associate the field with the error message and expose the message as
an assertive alert, using a stable matching identifier while preserving the
existing visual error behavior.
In `@web/src/lib/auth-actions.ts`:
- Around line 42-50: Replace the per-request 500ms delay in the password
authentication flow with a shared ingress- or datastore-backed login-attempt
limit that applies across concurrent requests and deployment instances.
Integrate monitoring for rejected or throttled attempts, and update the auth
action’s documentation to state that the shared limiter—not the delay—provides
brute-force protection.
🪄 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: bcb72d02-5b66-40cd-ac44-bedcb42a7c97
📒 Files selected for processing (10)
specs/api/agent-inbox-v1.mdweb/README.mdweb/src/app/login/page.tsxweb/src/components/LoginScreen.tsxweb/src/components/TopBar.tsxweb/src/lib/actions.tsweb/src/lib/auth-actions.tsweb/src/lib/next-path.tsweb/src/lib/session.tsweb/src/middleware.ts
| **This is still the API's only auth model (HT-51, §5).** The Agent Inbox web app now | ||
| requires an operator to sign in before it will render any page, but that is a web-layer | ||
| door in front of this same Bearer token, not a second API auth mechanism — see §5 for | ||
| the full justification. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the complete public-path exception set.
The route gate does not protect literally every route/page: web/src/middleware.ts also permits /favicon.ico, /robots.txt, /sitemap.xml, and /manifest.webmanifest without a session. Use “protected routes/pages” or list those exceptions explicitly.
specs/api/agent-inbox-v1.md#L130-L133: replace “before it will render any page” with wording that excludes the public paths.specs/api/agent-inbox-v1.md#L388-L393: change “every route”/“render anything” to describe protected application routes and the public-path exceptions.specs/api/agent-inbox-v1.md#L427-L432: update the changelog wording so it does not claim a cookie is required before rendering any page.web/README.md#L55-L59: replace “every route requires” with “every protected route requires” or enumerate allPUBLIC_PATHS.
📍 Affects 2 files
specs/api/agent-inbox-v1.md#L130-L133(this comment)specs/api/agent-inbox-v1.md#L388-L393specs/api/agent-inbox-v1.md#L427-L432web/README.md#L55-L59
🤖 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 `@specs/api/agent-inbox-v1.md` around lines 130 - 133, Update the
authentication documentation to distinguish protected application routes from
the public-path exceptions permitted by web/src/middleware.ts:
specs/api/agent-inbox-v1.md lines 130-133, 388-393, and 427-432, plus
web/README.md lines 55-59. Replace claims that every page or route requires a
session/cookie with “protected routes/pages” wording or explicitly list
/favicon.ico, /robots.txt, /sitemap.xml, and /manifest.webmanifest.
| - The API still authenticates every request by `HELPTHREAD_API_TOKEN` alone (constant-time | ||
| Bearer comparison, above) and has no knowledge of UI sessions, passwords, or cookies — |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Preserve the documented tracking-pixel exception.
The statement that the API authenticates “every request” with HELPTHREAD_API_TOKEN contradicts §4g, which explicitly permits the unauthenticated open-tracking pixel. Say “every authenticated API request” and retain the §4g exception here.
Suggested wording
- - The API still authenticates every request by `HELPTHREAD_API_TOKEN` alone (constant-time
+ - The API still authenticates every authenticated request by `HELPTHREAD_API_TOKEN` alone
+ (constant-time
Bearer comparison, above) and has no knowledge of UI sessions, passwords, or cookies —📝 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.
| - The API still authenticates every request by `HELPTHREAD_API_TOKEN` alone (constant-time | |
| Bearer comparison, above) and has no knowledge of UI sessions, passwords, or cookies — | |
| - The API still authenticates every authenticated request by `HELPTHREAD_API_TOKEN` alone | |
| (constant-time Bearer comparison, above) and has no knowledge of UI sessions, passwords, or cookies — |
🤖 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 `@specs/api/agent-inbox-v1.md` around lines 395 - 396, Update the
authentication statement near the Bearer comparison to say “every authenticated
API request” rather than “every request,” and retain an explicit reference to
the unauthenticated open-tracking pixel exception documented in §4g.
| - `HELPTHREAD_UI_PASSWORD` — the operator login password (HT-51), required to | ||
| be at least 12 characters in production. Compared constant-time | ||
| (`src/lib/auth-actions.ts`) against what's submitted on `/login`; there is | ||
| no per-Agent account, just this one shared password (v1 is single-Agent — | ||
| see `specs/api/agent-inbox-v1.md` §1). | ||
| - `HELPTHREAD_UI_SESSION_SECRET` — the HMAC secret signing the login session | ||
| cookie (`src/lib/session.ts`), required to be at least 32 characters in | ||
| production. Checked on every route by `src/middleware.ts`, which runs on | ||
| Next's Edge runtime — hence Web Crypto (`crypto.subtle`) rather than | ||
| `node:crypto` for the cookie's HMAC, unlike the password comparison above. | ||
|
|
||
| Both `HELPTHREAD_UI_*` vars have obviously-dev-only fallbacks in local | ||
| development (matching the `HELPTHREAD_API_TOKEN` dev-default pattern above) | ||
| and are REQUIRED — with no fallback — once `NODE_ENV=production`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the production fallback claim for build time.
uiAuthConfig() intentionally skips required-env validation during NEXT_PHASE=phase-production-build, so these defaults can exist during next build; the fail-closed behavior applies at production runtime. The README currently says there is no fallback whenever NODE_ENV=production.
🤖 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 `@web/README.md` around lines 26 - 39, Update the README description of
HELPTHREAD_UI_* fallback behavior to clarify that production-build time
(NEXT_PHASE=phase-production-build) may use development defaults because
uiAuthConfig() skips required-environment validation. State that the
no-fallback, fail-closed requirement applies during production runtime, and
preserve the existing local-development behavior.
| <p | ||
| style={{ | ||
| margin: '12px 0 0', | ||
| maxWidth: 340, | ||
| fontSize: 14.5, | ||
| lineHeight: 1.65, | ||
| color: 'var(--ht-ink-muted)', | ||
| }} | ||
| > | ||
| This deployment has one operator password — there's no separate account to create. | ||
| </p> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required “Agent” terminology consistently.
The implementation repeatedly calls the human support actor an “operator,” including in visible login copy.
web/src/components/LoginScreen.tsx#L94-L104: change visible “operator password” copy to “Agent password.”web/src/lib/session.ts#L2-L9: replace “operator” in the session-model documentation.web/src/middleware.ts#L9-L23: replace “operator” in the route-gate documentation.web/src/lib/actions.ts#L54-L64: describe this as an Agent session.web/src/lib/next-path.ts#L1-L19: replace “operator” in redirect documentation.web/src/lib/auth-actions.ts#L3-L10: replace “operator” in authentication documentation.web/src/app/login/page.tsx#L9-L16: replace “operator” in page documentation.
As per coding guidelines, “Use Agents exclusively for human support staff and Assistants exclusively for AI actors.” <coding_guidelines>
📍 Affects 7 files
web/src/components/LoginScreen.tsx#L94-L104(this comment)web/src/lib/session.ts#L2-L9web/src/middleware.ts#L9-L23web/src/lib/actions.ts#L54-L64web/src/lib/next-path.ts#L1-L19web/src/lib/auth-actions.ts#L3-L10web/src/app/login/page.tsx#L9-L16
🤖 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 `@web/src/components/LoginScreen.tsx` around lines 94 - 104, Replace
human-support “operator” terminology with “Agent” consistently: update the
visible password copy in web/src/components/LoginScreen.tsx lines 94-104, and
revise the related documentation in web/src/lib/session.ts lines 2-9,
web/src/middleware.ts lines 9-23, web/src/lib/actions.ts lines 54-64,
web/src/lib/next-path.ts lines 1-19, web/src/lib/auth-actions.ts lines 3-10, and
web/src/app/login/page.tsx lines 9-16. Keep “Assistants” reserved for AI actors.
Source: Coding guidelines
| <input | ||
| id="ht-login-password" | ||
| name="password" | ||
| type="password" | ||
| autoComplete="current-password" | ||
| // biome-ignore lint/a11y/noAutofocus: the one interactive element on a dedicated login screen. | ||
| autoFocus | ||
| required | ||
| disabled={isPending} | ||
| value={password} | ||
| onChange={(event) => { | ||
| setPassword(event.target.value) | ||
| if (error !== null) setError(null) | ||
| }} | ||
| style={{ | ||
| width: '100%', | ||
| boxSizing: 'border-box', | ||
| fontFamily: 'var(--ht-sans)', | ||
| fontSize: 12.5, | ||
| color: 'var(--ht-ink)', | ||
| background: 'var(--ht-bg)', | ||
| border: '1px solid var(--ht-divider)', | ||
| borderRadius: 'var(--ht-radius-sm)', | ||
| padding: '8px 10px', | ||
| outline: 'none', | ||
| }} | ||
| /> | ||
|
|
||
| {error !== null && ( | ||
| <div | ||
| style={{ | ||
| marginTop: 8, | ||
| fontSize: 12.5, | ||
| fontWeight: 600, | ||
| color: 'var(--ht-critical)', | ||
| }} | ||
| > | ||
| {error} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Announce rejected credentials to assistive technology.
The visual error is neither a live alert nor associated with the password field, so screen-reader users may not know why submission failed.
Proposed accessibility fix
<input
id="ht-login-password"
+ aria-invalid={error !== null}
+ aria-describedby={error !== null ? 'ht-login-error' : undefined}
name="password"
type="password"
...
/>
{error !== null && (
<div
+ id="ht-login-error"
+ role="alert"
style={{📝 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.
| <input | |
| id="ht-login-password" | |
| name="password" | |
| type="password" | |
| autoComplete="current-password" | |
| // biome-ignore lint/a11y/noAutofocus: the one interactive element on a dedicated login screen. | |
| autoFocus | |
| required | |
| disabled={isPending} | |
| value={password} | |
| onChange={(event) => { | |
| setPassword(event.target.value) | |
| if (error !== null) setError(null) | |
| }} | |
| style={{ | |
| width: '100%', | |
| boxSizing: 'border-box', | |
| fontFamily: 'var(--ht-sans)', | |
| fontSize: 12.5, | |
| color: 'var(--ht-ink)', | |
| background: 'var(--ht-bg)', | |
| border: '1px solid var(--ht-divider)', | |
| borderRadius: 'var(--ht-radius-sm)', | |
| padding: '8px 10px', | |
| outline: 'none', | |
| }} | |
| /> | |
| {error !== null && ( | |
| <div | |
| style={{ | |
| marginTop: 8, | |
| fontSize: 12.5, | |
| fontWeight: 600, | |
| color: 'var(--ht-critical)', | |
| }} | |
| > | |
| {error} | |
| </div> | |
| )} | |
| <input | |
| id="ht-login-password" | |
| aria-invalid={error !== null} | |
| aria-describedby={error !== null ? 'ht-login-error' : undefined} | |
| name="password" | |
| type="password" | |
| autoComplete="current-password" | |
| // biome-ignore lint/a11y/noAutofocus: the one interactive element on a dedicated login screen. | |
| autoFocus | |
| required | |
| disabled={isPending} | |
| value={password} | |
| onChange={(event) => { | |
| setPassword(event.target.value) | |
| if (error !== null) setError(null) | |
| }} | |
| style={{ | |
| width: '100%', | |
| boxSizing: 'border-box', | |
| fontFamily: 'var(--ht-sans)', | |
| fontSize: 12.5, | |
| color: 'var(--ht-ink)', | |
| background: 'var(--ht-bg)', | |
| border: '1px solid var(--ht-divider)', | |
| borderRadius: 'var(--ht-radius-sm)', | |
| padding: '8px 10px', | |
| outline: 'none', | |
| }} | |
| /> | |
| {error !== null && ( | |
| <div | |
| id="ht-login-error" | |
| role="alert" | |
| style={{ | |
| marginTop: 8, | |
| fontSize: 12.5, | |
| fontWeight: 600, | |
| color: 'var(--ht-critical)', | |
| }} | |
| > | |
| {error} | |
| </div> | |
| )} |
🤖 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 `@web/src/components/LoginScreen.tsx` around lines 127 - 166, Update the
password field and the conditional error element in LoginScreen so rejected
credentials are announced to assistive technology: associate the field with the
error message and expose the message as an assertive alert, using a stable
matching identifier while preserving the existing visual error behavior.
| /** | ||
| * Checks the submitted password against `HELPTHREAD_UI_PASSWORD` and, on a | ||
| * match, signs in and redirects to `nextPathRaw` (sanitized — see | ||
| * `lib/next-path.ts`; falls back to the inbox default when absent or | ||
| * unsafe). On a mismatch, waits ~500ms before returning the failure so a | ||
| * scripted guesser can't fire requests back-to-back — this is a per-process | ||
| * delay, not a rate limit: it does nothing against many parallel requests or | ||
| * multiple deployment instances. v1 is a single operator behind one | ||
| * password; a real rate limiter is out of scope until that stops being true. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enforce a real login attempt limit before release.
The 500 ms per-request sleep is bypassed by parallel requests and can itself amplify resource exhaustion. Apply a shared ingress or datastore-backed attempt limit with monitoring; do not rely on this delay as the password endpoint’s brute-force control.
🤖 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 `@web/src/lib/auth-actions.ts` around lines 42 - 50, Replace the per-request
500ms delay in the password authentication flow with a shared ingress- or
datastore-backed login-attempt limit that applies across concurrent requests and
deployment instances. Integrate monitoring for rejected or throttled attempts,
and update the auth action’s documentation to state that the shared limiter—not
the delay—provides brute-force protection.
…T-51) Dashboard/CLI env editors routinely append a trailing newline to a value; the operator typing the password never does, so the exact-match compare rejected the correct password with no recoverable signal (hit live during preview setup). Trim the stored expected value only — never the candidate the operator types. Length guard still runs on the raw value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The screen said 'there's nothing to sign into' — false once HT-51 adds a login. Reword to distinguish the operator's (valid) session from the deployment's rejected service token, which is the actual failure here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…T-51) - CodeQL js/insufficient-password-hash (2x high): replace the bare SHA-256 length-blinding in the constant-time password compare with a keyed HMAC (session-secret key) — the recognized constant-time-compare idiom. No slow KDF: there is no password hash at rest to brute-force (the expected value is the plaintext HELPTHREAD_UI_PASSWORD env, deployment config); the HMAC is a length-blinding step, not a storage hash. Reasoning documented in-module. - CodeRabbit a11y: the login error is now role=alert / aria-live=assertive so screen readers announce a rejected password. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review round addressedCodeQL — 2× high a11y (Major): login error is now Vocabulary (Minor) — declined, with reasoning: Rate-limiting (Major): the per-instance 500ms delay is a documented v1 limitation (HT-51 scope + module comment) — distributed rate limiting is explicitly deferred until v1 stops being single-operator. Not a regression; noted for the GA hardening epic. Doc nits (public-path exhaustiveness, tracking-pixel exception wording, README build-time qualifier): minor doc-accuracy points; folding into a follow-up rather than blocking this security fix. 🤖 Generated with Claude Code |
CodeQL js/insufficient-password-hash rejects both bare SHA-256 and keyed HMAC-SHA256 (fast hashes) for a password comparison. Switch to scrypt (a slow KDF, node:crypto, no new deps): still length-blinds for the constant-time compare, and its work factor adds real online-guess cost. There is no password hash at rest (the expected value is the plaintext env var), but a slow KDF is strictly better here and clears the objective security gate rather than arguing with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 `@web/src/lib/auth-actions.ts`:
- Around line 81-97: Update passwordMatches to be asynchronous and replace both
scryptSync calls with the asynchronous scrypt API while preserving the
fixed-length timingSafeEqual comparison. Await passwordMatches from loginAction
before applying the existing 500 ms sleep and response 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 Plus
Run ID: b0b548be-a97f-4a09-8496-1281d4cd475b
📒 Files selected for processing (1)
web/src/lib/auth-actions.ts
| function passwordMatches(candidate: string, expected: string, salt: string): boolean { | ||
| // Derive a fixed-length key from each side with scrypt (a deliberately slow | ||
| // KDF) before the constant-time compare. Two things fall out of this: | ||
| // 1. Length-blinding: both digests are SCRYPT_KEYLEN bytes, so | ||
| // `timingSafeEqual` never sees unequal lengths and length is never data. | ||
| // 2. Online-guess cost: scrypt's work factor makes each comparison cost | ||
| // ~tens of ms, so an attacker who reaches this endpoint can't cheaply | ||
| // brute-force the password. (There is no password hash *at rest* to | ||
| // protect — the expected value is the plaintext HELPTHREAD_UI_PASSWORD | ||
| // env — but a slow KDF still raises the cost of online guessing, and it | ||
| // is what static analysis expects of any password comparison.) | ||
| // The salt is the deployment session secret; both sides use the same salt so | ||
| // their derived keys are comparable. | ||
| const saltBuf = Buffer.from(salt) | ||
| const candidateKey = scryptSync(candidate, saltBuf, SCRYPT_KEYLEN) | ||
| const expectedKey = scryptSync(expected, saltBuf, SCRYPT_KEYLEN) | ||
| return timingSafeEqual(candidateKey, expectedKey) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the declared Node target and the password-comparison call path before
# replacing scryptSync with async scrypt.
fd -HI -t f '^(package\.json|\.nvmrc|\.node-version|\.tool-versions)$' . \
-x sh -c 'echo "== $1 =="; sed -n "1,180p" "$1"' sh {}
rg -n -C 4 '\b(passwordMatches|scryptSync|scrypt)\b' web/src/lib/auth-actions.tsRepository: Helpthread/helpthread
Length of output: 4998
🌐 Web query:
Node.js crypto scryptSync blocks event loop asynchronous scrypt callback Promise documentation
💡 Result:
Yes, crypto.scryptSync is a synchronous, blocking operation that freezes the Node.js event loop [1][2][3]. When executed, it forces the entire event loop to wait for the computation to complete before it can process any other incoming requests, timers, or I/O tasks [1][3]. In a production server environment, using scryptSync is strongly discouraged because it can degrade application performance and responsiveness, and effectively acts as a Denial of Service (DoS) vulnerability if an attacker sends multiple requests that trigger these heavy computations [2][3]. To keep the event loop free, you should use the asynchronous version, crypto.scrypt [4][2][5]. Because crypto.scrypt uses an error-first callback style by default, you can easily adapt it to use Promises (which are generally preferred in modern Node.js applications) by using the built-in util.promisify utility [6][7][8]. Example using Promises: const { scrypt } = require('node:crypto'); const { promisify } = require('node:util'); const scryptAsync = promisify(scrypt); // Use within an async function const key = await scryptAsync(password, salt, 64); By using the asynchronous approach, the CPU-intensive work of key derivation is offloaded to the libuv thread pool, allowing the event loop to continue processing other concurrent operations [2][3].
Citations:
- 1: https://github.com/NodeSecure/js-x-ray/blob/master/docs/synchronous-io.md
- 2: https://nodejs.org/learn/asynchronous-work/dont-block-the-event-loop
- 3: https://stackinsight.dev/blog/blocking-io-empirical-study/
- 4: https://nodejs.org/api/crypto.html
- 5: postalsys/emailengine@c9c6ede
- 6: https://stackoverflow.com/questions/68898796/nestjs-crypto-what-is-the-purpose-of-the-promisifyscrypt
- 7: https://jsguides.dev/reference/node-modules/crypto/
- 8: https://codingeasypeasy.com/blog/nodejs-password-hashing-a-comprehensive-guide-using-the-crypto-module/
🏁 Script executed:
node - <<'JS'
const { scryptSync, scrypt } = require('node:crypto');
function probeSync() {
const started = Date.now();
setTimeout(() => console.log('sync timer fired after', Date.now() - started, 'ms'), 0);
scryptSync('candidate', 'salt', 32);
console.log('sync returned after', Date.now() - started, 'ms');
}
function probeAsync(done) {
const started = Date.now();
setTimeout(() => console.log('async timer fired after', Date.now() - started, 'ms'), 0);
scrypt('candidate', 'salt', 32, () => {
console.log('async callback after', Date.now() - started, 'ms');
done();
});
console.log('async returned after', Date.now() - started, 'ms');
}
probeSync();
probeAsync(() => {});
JSRepository: Helpthread/helpthread
Length of output: 296
Use async scrypt for login checks. Two scryptSync calls run on the request path, so concurrent login attempts can block the Node event loop and stall the UI server. Switch passwordMatches to async scrypt and await it from loginAction; the 500 ms sleep only runs after the check and doesn’t prevent the blocking.
🤖 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 `@web/src/lib/auth-actions.ts` around lines 81 - 97, Update passwordMatches to
be asynchronous and replace both scryptSync calls with the asynchronous scrypt
API while preserving the fixed-length timingSafeEqual comparison. Await
passwordMatches from loginAction before applying the existing 500 ms sleep and
response logic.
2 participants
Summary
Implemented HT-51 operator login for the Agent Inbox UI in the worktree /Users/tjbaker/Projects/helpthread-worktrees/feat-ht-51-operator-login, on branch feat/ht-51-operator-login (committed, not pushed, no PR opened, per instructions).
New files:
<payload-b64url>.<mac-b64url>with{v:1, iat}payload;uiAuthConfig()guard mirrors lib/api.ts's config() pattern (production requires HELPTHREAD_UI_SESSION_SECRET >=32 chars and HELPTHREAD_UI_PASSWORD >=12 chars, NEXT_PHASE build-time escape, dev fallbacks otherwise). Sliding ~7-day expiry: valid for 7 days from iat, re-stamped once older than 1 day (documented tradeoff in the module comment).sanitizeNextPath()open-redirect guard for the?next=param (must start with a single/, not//, no embedded scheme, checked pre- and post-percent-decode).shouldRefreshis true.loginAction(password, nextPathRaw)(Node runtime; node:crypto timingSafeEqual over SHA-256 digests of both sides so length is never observable; ~500ms delay on mismatch, documented as a per-instance-only throttle, not real rate limiting; on match sets the cookie and calls redirect(sanitizeNextPath(nextPathRaw))) andlogoutAction()(clears the cookie, redirects to /login).?next=; client screen). Explicitly flagged in its module comment as an undesigned surface needing the maintainer's sign-off per CLAUDE.md's UI-fidelity mandate, styled after AuthFailure's calm register. Two documented, necessary deviations from ds-only composition: a native<input type="password">(ds/core/TextInput hardcodes type="text" with no way to change it) styled to match TextInput's tokens, and ds/core/Button's onClick callingformRef.current?.requestSubmit()(Button hardcodes type="button", no type="submit" escape hatch) — ds/** itself was never edited.Modified:
Item 5 (AuthFailure vs. not-logged-in) required no code change: AuthFailure only triggers on the API client's 401→AUTH_ERROR_DIGEST path, which is untouched; the login layer is a separate, earlier gate.
Design decisions (flag the login page as a NEW designed surface awaiting the maintainer's visual sign-off)
loginAction(password, next)call from a client component (useTransition), not the<form action={fn}>+ useActionState pattern — matches this codebase's existing house style (every other server action in lib/actions.ts is invoked the same way from ConversationScreen.tsx), and sidesteps ds/core/Button's hardcodedtype="button"more simply than juggling useActionState + requestSubmit together.position:fixed; inset:0; zIndex:90full-screen-cover trick, keeping this surgical (zero edits to the shared layouts) at the cost of the TopBar's own (harmless, swallowed-on-error) notifications fetch still running server-side on /login loads — an existing, unavoidable-without-layout-edits behavior, not a new one.Review — 7 findings (4 actionable), fixes applied
Verification — gate exits: typecheck 0, lint 0, tests 0, web build 0, clean tree, client-bundle secret scan clean
link https://resonantiq.atlassian.net/browse/HT-51
🤖 Generated with Claude Code
Summary by CodeRabbit
?next=redirect handling, and a dedicated full-screen sign-in UI.