Conversation
Add server-verified protected-route sessions, secure cookie-backed refresh/logout, locale persistence, and accessible registration validation and strength feedback. Closes CodeGirlsInc#1305 Closes CodeGirlsInc#1306 Closes CodeGirlsInc#1307 Closes CodeGirlsInc#1308
|
@khaadish is attempting to deploy a commit to the Mftee's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@khaadish Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Resolve auth-session.ts: keep this PR's DEFAULT_POST_LOGIN_PATH="/" redesign (middleware/tests consistently treat "/" as the protected landing page), carry forward main's hasStoredSession() from CodeGirlsInc#1414 (decoupled, dashboardHref comes from an explicit prop, not this constant). Resolve robots.ts: keep main's per-locale disallowedPaths from CodeGirlsInc#1413; this PR's own edit here was unrelated to its scope and would have blanket-disallowed the whole site.
mftee
left a comment
mftee
left a comment
There was a problem hiding this comment.
Reviewed. This is a substantial, well-integrated rework: live password confirmation with accessible strength feedback on registration (with real requirement rules mirroring the backend DTO's validation), locale persistence via a cookie instead of an authenticated-only backend call, and a genuinely stronger protected-session model — the middleware now verifies the access cookie against the backend on every protected-route request (fail-closed with a 503 rather than silently letting traffic through when the auth service is unreachable), falls back to a dedicated session-refresh redirect when only a refresh cookie is present, and guards against open-redirect via the param (rejects protocol-relative and control-character payloads, strips locale prefixes). Extensive new middleware test coverage backs all of this.
This PR conflicted with both #1413 (robots.ts) and #1414 (auth-session.ts) — the auth-session.ts conflict was substantive, not just textual: this PR deliberately redefines DEFAULT_POST_LOGIN_PATH from '/dashboard' to '/' as part of making '/' the actual protected landing page (consistent throughout the new middleware and its test suite). I kept that redesign and carried forward #1414's hasStoredSession() helper alongside it (confirmed it's decoupled — the 404 page's dashboard link uses an explicit homeHref prop, not this constant). Separately, this PR's own robots.ts edit looked like an unrelated accidental regression (would have blanket-disallowed the whole site) and was out of scope for its linked issues, so I discarded it and kept #1413's correct per-locale disallow list. CI failures reproduce identically on main. Approving.
mftee
left a comment
mftee
left a comment
There was a problem hiding this comment.
Reviewed. This is a substantial, well-integrated rework: live password confirmation with accessible strength feedback on registration (with real requirement rules mirroring the backend DTO's validation), locale persistence via a cookie instead of an authenticated-only backend call, and a genuinely stronger protected-session model — the middleware now verifies the access cookie against the backend on every protected-route request (fail-closed with a 503 rather than silently letting traffic through when the auth service is unreachable), falls back to a dedicated session-refresh redirect when only a refresh cookie is present, and guards against open-redirect via the redirect param (rejects protocol-relative and control-character payloads, strips locale prefixes). Extensive new middleware test coverage backs all of this.
This PR conflicted with both #1413 (robots.ts) and #1414 (auth-session.ts) — the auth-session.ts conflict was substantive, not just textual: this PR deliberately redefines DEFAULT_POST_LOGIN_PATH from '/dashboard' to '/' as part of making '/' the actual protected landing page (consistent throughout the new middleware and its test suite). I kept that redesign and carried forward #1414's hasStoredSession() helper alongside it (confirmed it's decoupled — the 404 page's dashboard link uses an explicit homeHref prop, not this constant). Separately, this PR's own robots.ts edit looked like an unrelated accidental regression (would have blanket-disallowed the whole site) and was out of scope for its linked issues, so I discarded it and kept #1413's correct per-locale disallow list. CI failures reproduce identically on main. Approving.
- register-auth.dto.ts / lib/schemas/auth.ts: keep contract-driven minLength (this PR's design) combined with main's password complexity rules and translated message keys (from CodeGirlsInc#1415) rather than either side alone. - Bumped the stale api-contracts.json register.password.minLength (6->8) and fullName.minLength (1->2) to match the password policy already merged in CodeGirlsInc#1415, and re-ran contracts/sync.mjs to propagate to both copies. - Kept app/schemas/register.schema.ts and e2e/dialog.spec.ts, which this PR deleted for reasons unrelated to its own scope (predates CodeGirlsInc#1415's RegisterForm.tsx, which still imports the former; the latter's coverage isn't replaced elsewhere in this PR).
Centralizes shared API constraints into contracts/api-contracts.json, adds dispute-filing E2E coverage across three browsers, and includes app-level code in Jest coverage. Fixed a stale contract value and two unrelated file deletions found while resolving conflicts with #1415. Closes #1325, #1326, #1327, #1328.
Both CodeGirlsInc#1415 (already merged) and this PR independently rewrote api-client.ts and auth-session.ts with different, incompatible session-refresh designs. Synthesized rather than picked a side: - Kept this PR's authGeneration/AbortController invalidation mechanism (invalidateRefresh/RefreshInvalidatedError) since this PR's own auth-session.ts (storeSession) depends on it, non-conflicting call sites confirm the sync clearSession() -> SessionStateResult contract (e.g. settings/security/page.tsx checks .ok synchronously, would not type-check against CodeGirlsInc#1415's Promise<boolean>), and the resume-aware redirectToLogin (appends ?resume= from a pending session-state group) is real, tested functionality from this PR's own scope. - Kept CodeGirlsInc#1415's refresh-token ROTATION handling (persisting the server's rotated refresh_token) and its RefreshError(status, definitive)/ isDefinitiveRefreshError distinction + cross-tab refresh adoption, since dropping either would regress against CodeGirlsInc#1398's already-merged reuse-detection/family-revocation, and SessionRefreshClient.tsx (already merged, not touched by this PR) imports isDefinitiveRefreshError directly. - clearSession is now synchronous (SessionStateResult, matching this PR's real non-conflicting call sites) but still fires the backend logout call fire-and-forget so CodeGirlsInc#1415's server-side revocation still happens. - Removed a dead cookie-clearing line (document.cookie = 'token=...') present in both sides' versions of this logic — nothing in the codebase ever sets a cookie named 'token'; the real session cookies are named via ACCESS_COOKIE_NAME/REFRESH_COOKIE_NAME in auth-cookie.ts. - Rewrote test-utils/api-client.test.ts and LoginForm.test.tsx assertions that depended on the losing side's implementation details (async boolean clearSession, /dashboard redirect, relative request URLs). Could not run tsc or jest locally to verify this (no node_modules on this machine) — CI is the real check for this commit.
2 participants
Summary
Issues
Validation
Closes #1305
Closes #1306
Closes #1307
Closes #1308