feat: migrate to remix-run/auth-middleware#132
Conversation
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
|
|
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 15 minutes and 32 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. WalkthroughRefactors authentication from a session/async-context approach to remix/auth credentials-based auth, adds credential provider and session parsing helpers, updates middleware wiring to use function references, changes controller action signatures to accept a context object, and adjusts redirects and session shape for auth state. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant AuthCtl as Auth Controller
participant PasswordProv as Password Provider
participant DB as Database
participant Session as SessionStore
participant Router as Router
Client->>AuthCtl: POST /login (form data)
AuthCtl->>PasswordProv: verifyCredentials(passwordProvider, context)
PasswordProv->>DB: find user by email
DB-->>PasswordProv: user record
PasswordProv->>PasswordProv: bcrypt.compare(password, hash)
alt credentials valid
PasswordProv-->>AuthCtl: identity
AuthCtl->>Session: completeAuth(context) / session.set("auth",{userId})
AuthCtl->>Router: getPostAuthRedirect(context.url)
AuthCtl-->>Client: Redirect to target
else invalid / error
AuthCtl->>Session: flash error
AuthCtl-->>Client: Redirect to /login with returnTo
end
Client->>Router: GET /protected
Router->>Session: read "auth" session value
Session-->>Router: AuthSession { userId }
Router->>DB: load user by id
DB-->>Router: user (no password)
Router-->>ProtectedHandler: attach AuthIdentity to request
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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 |
mcansh
force-pushed
the
logan/auth-middleware
branch
from
78ec409 to
604ea4b
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates authentication to remix/auth-middleware, updates security header behavior by request method, removes legacy user-context/session helpers, and extracts a reusable keyboard UI that’s now shown on historical games.
Changes:
- Migrate auth to
remix/auth-middlewarewith session-based scheme + credentials provider, and update controllers to readAuthidentity. - Add
skip()support usage for security headers based on request method. - Extract the on-screen keyboard into
app/components/keyboard.tsxand reuse it on home + historical game pages.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| app/utils/local-schema.ts | Adds @ts-expect-error suppressions around internal ~run calls while iterating keys/items/variants. |
| app/utils/local-form-schema.ts | Introduces a local Standard Schema-compatible form data parser that plugs into local-schema. |
| app/utils/context.ts | Removes async-context-based current-user storage utilities. |
| app/utils/auth-session.ts | Adds auth session normalization/parsing helpers (e.g., parseAuthSession, normalizeEmail). |
| app/router.ts | Adds loadAuth() middleware globally and configures security headers skipping by method. |
| app/models/user.ts | Removes unused login schema/types and getUserById. |
| app/middleware/auth.ts | Replaces old middleware with auth() scheme + credentials provider + requireAuth. |
| app/home/page.tsx | Reuses extracted Keyboard component; updates tab behavior for read-only inputs. |
| app/home/controller.tsx | Switches auth access from context-user to Auth identity; updates middleware usage. |
| app/history/game.tsx | Shows the keyboard on historical game pages (replacing the prior submit UI). |
| app/history/controller.tsx | Switches auth access to Auth identity; updates middleware usage. |
| app/components/keyboard.tsx | New extracted keyboard UI component. |
| app/auth/controller.tsx | Updates login flow to use credentials provider + auth middleware session handling. |
Comments suppressed due to low confidence (4)
app/models/user.ts:18
getUserByIdwas removed from this module, but it is still referenced inapp/home/controller.test.ts(used in avi.mockreturn object). This will break type-checking/build of tests. Update the test to mock the new auth/session-based flow (or remove the unused mock) so it no longer expectsgetUserByIdto exist.
export async function getUserByEmail(email: User["email"]) {
return db.user.findUnique({ where: { email } })
}
app/utils/local-schema.ts:701
- Avoid using a bare
// @ts-expect-errorhere. It suppresses the real typing issue (indexingshapewith astring) and can hide unrelated future errors. Prefer typing the keys (e.g. iterateObject.keys(shape) as Array<keyof shape>) or otherwise narrowkeysoshape[key]is aSchema, and include a short justification if suppression is still required.
app/utils/local-schema.ts:975 - Avoid using a bare
// @ts-expect-errorhere. The underlying issue is thatitems[index]can’t be typed safely whenindexis anumber. Consider iterating with a typed index (for (const [index, schema] of items.entries())) or otherwise narrowing/casting so the call toschema["~run"]doesn’t require suppressing errors, and include a justification if suppression remains necessary.
app/utils/local-schema.ts:1072 - Avoid using a bare
// @ts-expect-errorhere. The casttag as keyof variantsis already narrowing, so it’s better to refine the generic constraints / discriminant typing sovariants[tag]is known to be aSchemaandschema["~run"]can be called without suppressing type errors. If suppression is required, add a short explanation so future changes don’t accidentally rely on it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/router.ts (1)
42-45:⚠️ Potential issue | 🟠 MajorDon't drop security headers on every non-GET response.
forgotPassword.actionandresetPassword.actionstill render HTML on POST, so this strips CSP, framing, and HSTS protections from real document responses. If the goal is to exempt API/file responses, decide that from the response type rather than the request method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/router.ts` around lines 42 - 45, The current skip callback on securityHeaders drops security headers for all non-GET requests, which removes CSP/HSTS for POST handlers that still return HTML (e.g., forgotPassword.action and resetPassword.action); change the skip(context) logic to decide based on response type rather than request method — e.g., only return true (skip) for API/file responses by checking the response Content-Type or the request Accept header (or Content-Disposition) and ensure document/html responses (or routes known to render HTML like forgotPassword.action/resetPassword.action) keep security headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/auth/controller.tsx`:
- Around line 193-194: The flashed error isn't consumed because
register.actions.index never reads the Session; update register.actions.index to
read the Session (e.g., via the same Session/getSession utility used elsewhere),
extract the "error" flash key and pass it into the register page's loader/props
so the UI can render it, or alternatively stop redirecting from the controller
and return the register page response directly with an inline error prop;
specifically adjust the controller code that calls session.flash(...) and
routes.auth.register.index.href and the register.actions.index loader/renderer
to exchange the Session flash "error" so duplicate-email submissions show the
message.
- Around line 197-199: The session is being set to a bare string
(session.set("auth", user.id)) but parseAuthSession() expects an object shape {
userId: string }, so update the session assignment after createUser(result) to
store the correct shape (an object containing userId) so subsequent requests are
recognized as authenticated; locate the session.set call in the auth controller
and replace the bare string with the object form { userId: user.id } (or the
exact key name parseAuthSession expects) to persist the proper auth session
shape.
In `@app/history/controller.tsx`:
- Around line 23-25: The auth fallback in the history controller currently
redirects unauthenticated requests with redirect(routes.auth.login.index.href())
which loses the original URL; update the branch that checks auth (where you call
context.get(Auth) and test auth.ok === false) to include the return-to query by
using getReturnToQuery(context.url) when building the login redirect (i.e.,
append the helper's output to routes.auth.login.index.href()); also mirror this
pattern for any other auth fallbacks in this controller so the requested history
URL is preserved post-login.
- Around line 20-25: The index action currently checks auth.ok and redirects to
login; update index to pass the original requested URL as a returnTo param when
calling redirect(routes.auth.login.index.href()) so the user returns after
sign-in, and add the same defensive auth validation to the game action: retrieve
auth from context.get(Auth) and guard on (auth.ok === false) returning the same
redirect with returnTo; reference the Auth type and GoodAuth/BadAuth union and
the game and index action handlers to implement the checks consistently.
In `@app/middleware/auth.ts`:
- Around line 29-40: The verify handler in createSessionAuthScheme is returning
the full Prisma User row (from db.user.findFirst) so protected handlers receive
sensitive fields like password; change the query in verify to explicitly select
only the public fields needed for authentication (e.g., id, email, name,
createdAt) instead of the entire record, build and return a redacted identity
object (e.g., { user: { id, email, name } }) and update the AuthIdentity
type/signature to match that sanitized shape; keep parseAuthSession and
AuthSession usage intact but ensure verify returns the narrowed AuthIdentity
rather than the full User.
---
Outside diff comments:
In `@app/router.ts`:
- Around line 42-45: The current skip callback on securityHeaders drops security
headers for all non-GET requests, which removes CSP/HSTS for POST handlers that
still return HTML (e.g., forgotPassword.action and resetPassword.action); change
the skip(context) logic to decide based on response type rather than request
method — e.g., only return true (skip) for API/file responses by checking the
response Content-Type or the request Accept header (or Content-Disposition) and
ensure document/html responses (or routes known to render HTML like
forgotPassword.action/resetPassword.action) keep security headers.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1215b16a-da55-49de-973d-5ae9eab49435
📒 Files selected for processing (10)
app/auth/controller.tsxapp/history/controller.tsxapp/home/controller.tsxapp/middleware/auth.tsapp/models/user.tsapp/router.tsapp/utils/auth-session.tsapp/utils/context.tsapp/utils/local-form-schema.tsapp/utils/local-schema.ts
💤 Files with no reviewable changes (1)
- app/utils/context.ts
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
mcansh
force-pushed
the
logan/auth-middleware
branch
from
fd203d5 to
22a9abc
Compare
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
app/utils/local-schema.ts:700
- New
// @ts-expect-erroris masking a typing issue caused by indexingshapewith astringkey. Prefer narrowingkeytokeyof shape(e.g., a typedObject.keyscast orfor...in) so the call to~runtype-checks without suppressing errors; this keeps future refactors from accidentally hiding real schema type problems.
app/utils/local-schema.ts:973 - New
// @ts-expect-erroris suppressing a type-safety issue when indexingitems/valuebyindex. It would be better to adjust the loop typing (or castitems[index]toSchema) so the~runcall is checked by TypeScript rather than silenced.
app/utils/local-schema.ts:1072 - New
// @ts-expect-erroris hiding a typing mismatch aroundvariants[tag]andschema["~run"]. Consider tightening the discriminator/key typing soschemais known to be aSchemaand the~runcall can be made without suppressing type errors.
| export const joinSchema = f.object({ | ||
| email: f.field(s.string().pipe(email())), | ||
| username: f.field(s.string().pipe(minLength(1))), | ||
| password: f.field(s.string().pipe(minLength(10))), | ||
| }) | ||
|
|
||
| export type JoinData = s.InferOutput<typeof joinSchema> | ||
| export type LoginData = s.InferOutput<typeof loginSchema> | ||
|
|
||
| export const loginSchema = f.object({ | ||
| email: f.field(s.string().pipe(email())), | ||
| password: f.field(s.string().pipe(minLength(10))), | ||
| }) | ||
|
|
||
| export async function getUserById(id: User["id"]) { | ||
| return db.user.findUnique({ where: { id } }) | ||
| } | ||
|
|
||
| export async function getUserByEmail(email: User["email"]) { | ||
| return db.user.findUnique({ where: { email } }) | ||
| } |
There was a problem hiding this comment.
getUserById was removed, but there are still tests referencing it (e.g. app/home/controller.test.ts mocks getUserById). Please update or remove those usages/mocks to avoid TypeScript/test failures.
| setCurrentUser(user) | ||
| if (typeof user.password === "string" && user.password !== "") { | ||
| let verified = await bcrypt.compare(input.password, user.password) | ||
| return verified ? user : null |
There was a problem hiding this comment.
passwordProvider.verify returns the full User record (including the hashed password) on success. Even if it’s not currently exposed, returning password-bearing objects increases the chance of accidental leakage; consider returning a user object with password omitted/stripped after verification (while still fetching it for bcrypt comparison).
| return verified ? user : null | |
| if (verified === false) { | |
| return null | |
| } | |
| let { password: _password, ...safeUser } = user | |
| return safeUser |
| let error = session.get("error") | ||
|
|
||
| return render( | ||
| <Document url={url} head={<title>Login - Remix Wordle</title>}> |
There was a problem hiding this comment.
The register page title is still set to "Login - Remix Wordle". Since this hunk was updated, it’s a good opportunity to correct the title to match the register page to avoid confusing users and improve accessibility (page title is announced by screen readers).
| <Document url={url} head={<title>Login - Remix Wordle</title>}> | |
| <Document url={url} head={<title>Register - Remix Wordle</title>}> |
| <main class="h-dvh"> | ||
| <form | ||
| method="POST" | ||
| action={routes.auth.register.action.href()} |
There was a problem hiding this comment.
The register form action does not preserve the returnTo query param (unlike the login form). If a user is sent to register from a protected page, the post-register redirect can’t send them back; consider including getReturnToQuery(url) when building the register form action URL.
| action={routes.auth.register.action.href()} | |
| action={routes.auth.register.action.href(undefined, getReturnToQuery(url))} |
| let user = await createUser(result) | ||
|
|
||
| session.set("auth", {auth: user.id}) | ||
| session.set("auth", { userId: user.id }) | ||
|
|
||
| return redirect(routes.home.index.href()) |
There was a problem hiding this comment.
After successful registration, the code sets session.set("auth", ...) but does not call completeAuth(context) (used in the login flow) to finalize auth/session state (e.g., session id regeneration to prevent session fixation). Consider using the same completeAuth + getPostAuthRedirect pattern here for consistency and security.
|
✅ Created PR with unit tests: #134 |
Summary by CodeRabbit
Bug Fixes
Refactor