feat: scoped Personal Access Token auth for browser-extension stock writes#3786
Merged
Conversation
…rites Adds DB-backed Personal Access Tokens so the browser extension can authorize stock writes without a cookie session (Option D). The owner mints a token from the web UI; the extension sends `Authorization: Bearer nsx_pat_<raw>` on the two stock-write endpoints only. Cookie login is untouched. Server backend (E1/E4/E5/E6/E7/E8/E9/E12/E14): - prisma: PersonalAccessToken model (tokenHash unique, tokenSuffix, name, userId, lastUsedAt, expiresAt, revokedAt) + User.personalAccessTokens[]; additive migration adds personal_access_tokens only (existing tables unchanged). - authSession.ts: rename hashRefreshToken -> generic exported hashToken (shared by refresh tokens and PATs, no second hash fn). New authenticateStockPatToken validates a PAT in ONE atomic query (revokedAt null + non-expired) and hydrates the full session user (SESSION_USER_SELECT) in the same round-trip; it never reads or clears auth cookies. - auth.ts: new authenticateStockRequest middleware dispatches on the Authorization header -- absent -> the unchanged cookie middleware (U7); present -> the PAT-only path that never clears the owner's cookies on failure (U5 confused-deputy guard) and returns 401 (not 500) for a malformed header (U6). - routes/personalAccessToken.ts: mint (201, raw token shown once, only the hash + last-4 suffix stored), list (masked; never selects tokenHash), revoke (owner- scoped, idempotent). Cookie-gated via isAuthorized; mint rate-limited 10/hr in prod. - stock.ts: /push_stock + /stock/exists now use authenticateStockRequest (E8/E9); /stocklist + DELETE stay cookie-only. - Logger.ts: extend redaction paths (rawToken/pat/personalAccessToken + uppercase Authorization) so a raw PAT can never reach logs (E5). Tests (DB-free, server vitest project): 13 new unit tests with mocked prisma -- 7 middleware (happy / full-hydrate / atomic-query / lastUsedAt / cookie-fallback / no-cookie-clear / malformed-401) replacing the PR1 it.todo placeholders, plus 6 route-handler tests (mint stores hash-not-raw, list never selects hash, revoke active / 404 / idempotent). Full suite green (92), typecheck + lint clean. Refs #3784 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(E13) Adds a Settings → Extension Token tab where the owner manages browser-extension Personal Access Tokens via the PR backend: - RTK Query endpoints (getPersonalAccessTokens / mintPersonalAccessToken / revokePersonalAccessToken) + a PersonalAccessTokens cache tag so mint/revoke refresh the list. Response types added to @types/response.d.ts. - ExtensionToken.tsx: generate (name -> token), a show-once reveal panel with copy-to-clipboard and an inline "Copied!" confirmation (no SnackBar — it uppercases, auto-dismisses, and its close button is unwired), a masked list (`nsx_pat_…<suffix>` only — the raw value never re-renders), and a per-row revoke control. Hand-rolled with the existing Button + Tailwind (no shadcn/radix). A failed mint/revoke shows an inline error and preserves the form. Tests (web vitest project, MSW): 4 component tests via TestRenderer + a fresh RTK Query cache per test — reveals the raw token once, copies it to the clipboard, lists tokens masked, and keeps the form + typed name when mint fails with a 5xx. Refs #3784 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…5/X1-X4)
Wires the browser extension into the server-side scoped-PAT auth: the popup reads a
Personal Access Token from chrome.storage.local and sends it as
`Authorization: Bearer nsx_pat_...` on both the existence check and the save POST.
- E3: rename VITE_API_URL -> VITE_API_ENDPOINT in .env.{development,production}
(origin only -- buildPushStockApiUrl appends the path; a full path would
double-append to /api/push_stock/api/push_stock). Un-ignore those two
non-secret build configs (public API origins) so CI/prod builds inject the
endpoint instead of silently falling back to the localhost default. Unskips the
NSX-80 e2e (the prod build would otherwise point the extension at localhost).
- Paste/connect UI (additive): a connect panel when no token is stored, a
"Connected" status + Disconnect when connected, and a reconnect alert when a
stored token is rejected with 401. The save checkbox and POST stay
unconditional, so the existing e2e flow -- including token-less 401 -> "Failed"
-- is unchanged; reconnect only triggers on a token-present 401.
- Bearer attaches to BOTH /stock/exists and /push_stock (both now behind
authenticateStockRequest server-side).
- E5: replace console.error(JSON.stringify(error)) with a redacted logger that
logs only HTTP status + message, never the axios config (it carries the header).
- E15: narrow host_permissions from <all_urls> to the NSX API origins; the active
tab's URL/title comes from the activeTab/tabs permissions.
Tests (extension vitest, Testing Library + happy-dom): 4 popup component tests --
shows the paste panel with no token, persists the pasted token to
chrome.storage.local and reveals the connected state, attaches the Bearer header
on save, and prompts reconnect on a 401. setup.ts switches to
@testing-library/jest-dom/vitest so DOM matchers type-check under tsc.
Refs #3784
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two P2 findings from the pre-landing adversarial review:
- authSession.ts: the post-auth `lastUsedAt` bookkeeping write was awaited with
no try/catch despite its "best-effort" comment. A transient DB write failure
threw past the already-successful authentication and surfaced as a 500, so a
valid PAT's stock save would fail on a momentary DB hiccup. Wrap it in
try/catch (Logger.error + continue) so it is genuinely best-effort.
- personalAccessToken.ts (revoke): Number.parseInt('5abc', 10) === 5 silently
treated a malformed id as token 5, contradicting the 400-for-non-numeric-id
contract. Guard with /^\d+$/ before parsing so trailing garbage is rejected
before any DB lookup (still owner-scoped, so never exploitable — just honest).
Regression tests for both: a valid PAT still authenticates (next() called, no
500) when the lastUsedAt write rejects, and a "5abc" revoke returns 400 without
touching the database. Full vitest suite 96 -> 98.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR implements end-to-end Personal Access Token (PAT) support: DB schema and migration, server auth and CRUD routes, logger redaction, browser-extension popup token state and auth-aware requests, web Settings UI and RTK endpoints, tests, and build/manifest updates. ChangesPersonal Access Token (PAT) System
Sequence Diagram(s) sequenceDiagram
participant Popup as Browser Extension Popup
participant AuthBuilder as buildStockRequestConfig
participant StockAPI as Server /push_stock or /stock/exists
participant AuthDispatcher as authenticateStockRequest
participant Prisma as Database (Prisma)
Popup->>AuthBuilder: attach Bearer header when token present
Popup->>StockAPI: request (with/without Authorization)
StockAPI->>AuthDispatcher: middleware invoked
AuthDispatcher->>Prisma: findFirst(tokenHash, not revoked, not expired) if Authorization present
Prisma-->>AuthDispatcher: user or null
AuthDispatcher-->>StockAPI: set req.authenticatedUser or return 401
StockAPI-->>Popup: 200/201 or 401/409
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
ESLint install failed: dependency version conflict. Check your lock file or package.json. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
browser-extension/tests/e2e/api-integration.spec.ts (1)
167-202:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix E2E test to not “happy-path” save without auth (and to actually assert success)
File:browser-extension/tests/e2e/api-integration.spec.ts(around lines 167-202)
POST /api/push_stockis protected: whenAuthorizationis absent,authenticateStockRequestfalls back to cookie-session auth (isAuthorized), which returns 401 when no cookies/JWT are present.- The popup save flow only sets
Authorization: Bearer <token>whenchrome.storage.localcontainsnsx_pat(buildStockRequestConfig(token)returns{}whentokenisnull). This spec never connects/pastes a PAT, andopenPopup()only stubs/api/stock/exists—so the real save request is very likely unauthenticated (401).- The test doesn’t fail even if the save fails: it calls
await verifySuccessMessage(popupPage)but does not assert the returned boolean.Suggested adjustment: either seed a valid
nsx_pat(or mock successful auth) and assertverifySuccessMessage(...) === true, or change the test to explicitly expect the unauthenticated/401 path.🤖 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 `@browser-extension/tests/e2e/api-integration.spec.ts` around lines 167 - 202, The test currently happy-paths a save without seeding authentication and never asserts the result, so either seed a valid PAT in extension storage before opening the popup or mock the auth/save endpoints and also assert the success boolean; specifically, before calling openPopup(...) set chrome.storage.local nsx_pat (so buildStockRequestConfig(token) attaches Authorization) or stub the POST /api/push_stock to return 200, then replace await verifySuccessMessage(popupPage) with const ok = await verifySuccessMessage(popupPage); expect(ok).toBe(true) to ensure the save actually succeeded (see openPopup, verifySuccessMessage, buildStockRequestConfig, authenticateStockRequest, nsx_pat).
🧹 Nitpick comments (1)
browser-extension/src/entrypoints/popup/style.css (1)
21-76: ⚖️ Poor tradeoffPAT panel styles lack dark-mode support.
The new
.pat-*rules use fixed light-theme hex colors (e.g. borders, label/notice text) with no dark variants, so the panel won't adapt in dark mode. Add dark-mode handling (Tailwind dark variants/config or aprefers-color-schemeblock) consistent with the main app.As per coding guidelines: "Use TailwindCSS for styling in popup UI, with support for dark mode using the same config as main app".
🤖 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 `@browser-extension/src/entrypoints/popup/style.css` around lines 21 - 76, The PAT panel CSS uses hard-coded light-theme hex colors; update the selectors (.pat-connect/.pat-connected, .pat-input, .pat-connect-btn/.pat-disconnect-btn, .pat-connected-label, .pat-reconnect-notice, .pat-connect-label) to support dark mode by using the same approach as the main app: replace fixed hex values with the app's Tailwind dark variants or shared CSS variables and/or add a prefers-color-scheme: dark block that sets the dark equivalents (borders, background, label/notice colors, button background/opacity) consistent with the main app theme; ensure disabled state (.pat-connect-btn:disabled) also adapts in dark mode.
🤖 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 `@browser-extension/src/entrypoints/popup/usePersonalAccessToken.ts`:
- Around line 43-53: The useEffect that calls readStoredPatToken() can reject
and leave isLoading true; update the promise chain for readStoredPatToken()
inside the effect to add a .catch handler that (when isActive) sets isLoading
false (and optionally clears token or logs error), and keep the existing success
path (setToken and setIsLoading(false)) and the isActive guard; i.e., modify the
readStoredPatToken().then(...) call to readStoredPatToken().then(...).catch(err
=> { if (!isActive) return; setIsLoading(false); /* optional: handle/log err */
}) so failures no longer pin loading state.
In `@server/lib/authSession.ts`:
- Around line 349-358: The extractBearerToken function currently only matches a
case-sensitive single-space "Bearer " prefix; update its parsing to accept
case-insensitive scheme and flexible whitespace by using a case-insensitive
regex (e.g. /^Bearer\s+(.+)$/i) or equivalent splitting logic on the first
whitespace, then trim the captured token and return null for empty tokens;
update the matching line in extractBearerToken to use the new regex and keep the
same null-return behavior for missing/malformed tokens.
In `@src/pages/Dashboard/Setting/ExtensionToken.tsx`:
- Around line 139-146: The token name input currently uses only a placeholder
and needs a stable accessible label; update the input with an accessible name by
either adding aria-label="Token name" to the input element that has value={name}
and data-testid="token-name-input" or add a visible <label
htmlFor="token-name-input">Token name</label> and give the input
id="token-name-input" so screen readers receive a permanent label; ensure the
onChange handler (setName) remains unchanged and the test id is preserved.
- Around line 68-72: The handleCopy function currently calls
navigator.clipboard.writeText(mintedToken.token) without handling
navigator.clipboard being undefined or writeText rejecting; wrap the copy logic
in a try/catch, check for navigator.clipboard before calling writeText, and on
failure fall back to a DOM-based copy (e.g., create a temporary textarea, set
its value to mintedToken.token, select and document.execCommand('copy'), then
remove it); always call setDidCopy(true) on successful copy and provide a
failure path that sets an error state or triggers user feedback when both
clipboard and fallback fail, referencing handleCopy, mintedToken,
navigator.clipboard, writeText, setDidCopy and the fallback
textarea/document.execCommand approach.
---
Outside diff comments:
In `@browser-extension/tests/e2e/api-integration.spec.ts`:
- Around line 167-202: The test currently happy-paths a save without seeding
authentication and never asserts the result, so either seed a valid PAT in
extension storage before opening the popup or mock the auth/save endpoints and
also assert the success boolean; specifically, before calling openPopup(...) set
chrome.storage.local nsx_pat (so buildStockRequestConfig(token) attaches
Authorization) or stub the POST /api/push_stock to return 200, then replace
await verifySuccessMessage(popupPage) with const ok = await
verifySuccessMessage(popupPage); expect(ok).toBe(true) to ensure the save
actually succeeded (see openPopup, verifySuccessMessage,
buildStockRequestConfig, authenticateStockRequest, nsx_pat).
---
Nitpick comments:
In `@browser-extension/src/entrypoints/popup/style.css`:
- Around line 21-76: The PAT panel CSS uses hard-coded light-theme hex colors;
update the selectors (.pat-connect/.pat-connected, .pat-input,
.pat-connect-btn/.pat-disconnect-btn, .pat-connected-label,
.pat-reconnect-notice, .pat-connect-label) to support dark mode by using the
same approach as the main app: replace fixed hex values with the app's Tailwind
dark variants or shared CSS variables and/or add a prefers-color-scheme: dark
block that sets the dark equivalents (borders, background, label/notice colors,
button background/opacity) consistent with the main app theme; ensure disabled
state (.pat-connect-btn:disabled) also adapts in dark mode.
🪄 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: 1bd42db9-ae48-4bbb-8c1c-61937d2b7466
📒 Files selected for processing (31)
@types/response.d.tsbrowser-extension/.env.developmentbrowser-extension/.env.productionbrowser-extension/.gitignorebrowser-extension/src/entrypoints/popup/App.tsxbrowser-extension/src/entrypoints/popup/constants.tsbrowser-extension/src/entrypoints/popup/style.cssbrowser-extension/src/entrypoints/popup/usePersonalAccessToken.tsbrowser-extension/src/entrypoints/popup/utils/buildStockRequestConfig.tsbrowser-extension/src/entrypoints/popup/utils/isUnauthorizedResponse.tsbrowser-extension/src/entrypoints/popup/utils/logStockRequestError.tsbrowser-extension/src/entrypoints/popup/utils/patStorage.tsbrowser-extension/tests/e2e/api-integration.spec.tsbrowser-extension/tests/setup.tsbrowser-extension/tests/unit/entrypoints/popup/App.test.tsxbrowser-extension/wxt.config.tsprisma/migrations/20260530141710_add_personal_access_tokens/migration.sqlprisma/schema.prismaserver/api.tsserver/auth.tsserver/lib/Logger.tsserver/lib/authSession.tsserver/lib/authenticateStockRequest.contract.test.tsserver/lib/requestSchemas.tsserver/routes/personalAccessToken.test.tsserver/routes/personalAccessToken.tsserver/routes/stock.tssrc/pages/Dashboard/Setting/ExtensionToken.test.tsxsrc/pages/Dashboard/Setting/ExtensionToken.tsxsrc/pages/Dashboard/Setting/index.tsxsrc/redux/API.ts
- usePersonalAccessToken: clear isLoading via .finally so a failed chrome.storage read can't pin isLoading=true and stall the existence check - authSession.extractBearerToken: accept case-insensitive Bearer scheme and flexible whitespace (RFC 7235) so 'bearer nsx_pat_...' parses - ExtensionToken.handleCopy: handle clipboard.writeText rejection / undefined navigator.clipboard with a manual-copy fallback message - ExtensionToken: add aria-label to the token-name input for screen readers - e2e api-integration: stub POST /api/push_stock 200 and assert verifySuccessMessage so the URL test no longer masks a 401 (auth added in PR)
Collaborator
Author
|
Addressed CodeRabbit review findings in ce5ca3a: Fixed (4 inline threads resolved):
Fixed (outside-diff, Major):
Skipped with reason (nitpick):
@coderabbitai review |
Contributor
|
The dark-mode deferral rationale is sound — adding dark-mode rules only to Triggering a fresh review now. (´• ω •`) 🐇 ✅ Actions performedReview triggered.
|
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
Lets the browser extension save reading-list "stocks" without a cookie login, using a scoped, DB-backed Personal Access Token (PAT). The owner mints a token from the web Settings UI (raw
nsx_pat_<64hex>shown once, SHA-256 hash stored), pastes it into the extension, and the extension sendsAuthorization: Bearer nsx_pat_...on the two stock-write endpoints only. Cookie login for the web app is untouched.Server (
96b9fa31,53434887)PersonalAccessTokenPrisma model + migration (tokenHashUNIQUE, FK → authorsON DELETE CASCADE,revokedAt/expiresAt/lastUsedAt).authenticateStockPatTokenvalidates a PAT in one atomic query (not revoked, not expired) and hydrates the full session user. TheauthenticateStockRequestmiddleware dispatches:Authorizationheader → PAT path, otherwise → unchanged cookie session. A bad PAT never reads or clears auth cookies (U5 confused-deputy guard)./api/push_stock+/api/stock/existsnow sit behindauthenticateStockRequest;/stocklistandDELETE /stock/:idstay cookie-only.console.error(JSON.stringify(error))that serialized the Bearer header; pino redactsauthorization/tokenpaths.Web (
0bf3203c)Extension (
610feb38)chrome.storage.localand attaches it as a Bearer header on both stock calls. Additive connect / connected / reconnect UI; the save checkbox + POST stay unconditional, so the existing token-less e2e flow is unchanged — reconnect only triggers on a token-present 401.VITE_API_URL→VITE_API_ENDPOINT(origin only —buildPushStockApiUrlappends the path). Un-ignored.env.{development,production}so CI/prod builds inject the endpoint instead of falling back to localhost. Verified: the prod build's popup chunk targetsnsx.malloc.tokyo(localhost count 0).host_permissionsnarrowed from<all_urls>to the two NSX origins (E15).Test Coverage
personalAccessToken.test.ts(mint / list / revoke, incl. new"5abc" → 400 before any DB lookupregression),authenticateStockRequest.contract.test.ts(U1–U8 confused-deputy / scope / cookie-fallback + newlastUsedAt write failure still authenticatesregression).ExtensionToken.test.tsx(W1–W3).App.test.tsx(X1–X4: paste panel shown w/o token, persist + connect, Bearer attach on save, 401 → reconnect) — runs in the browser-extension CI (vitest unit).typecheck+lintclean.Pre-Landing Review (adversarial, fresh context)
53434887):lastUsedAtmade genuinely best-effort — it previously 500'd a valid token's save on a transient DB write failure; revoke id now^\d+$-guarded —Number.parseInt('5abc', 10)had silently parsed to5.Known gaps / verification
200+ DB row): server tests mock Prisma, extension tests mock axios, and the success-path e2e (NSX-81) stays skipped pending real-DB PAT provisioning. Token format / hash alignment checks out on paper. The real verification is a post-deploy prod smoke test.Test plan
typecheck+lintclean (max-warnings=0)nsx.malloc.tokyo(not localhost)200+personal_access_tokensrow🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Enhancements
Tests