feat: one shot fix auth - #24
Conversation
Greptile SummaryThis PR replaces the previous session-cookie authentication system with a standard OAuth2 bearer JWT flow and introduces a shared-password "access gate" that locks the entire site (both the Next.js frontend and the FastAPI backend) behind a single password before users can reach the login page or any API endpoint. Key changes:
Confidence Score: 4/5Safe to merge after replacing the != comparison in access_gate.py with hmac.compare_digest. The overall architecture is sound and the implementation is clean. One P1 security issue remains: the primary password-check endpoint uses a non-constant-time comparison which is a timing-attack vector. Previously flagged issues (localStorage JWT, no-op signout, open redirect) are acknowledged trade-offs for a dev tool. The missing ?next= redirect is a minor UX gap. backend/routers/access_gate.py — swap != for hmac.compare_digest on the password check before merging. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as Browser
participant NX as Next.js (Server)
participant FA as FastAPI
Note over U,FA: Dev Access Gate flow
U->>NX: GET / (no dev_access_granted cookie)
NX->>NX: ProtectedLayout: hasAccessGateCookie() → false
NX-->>U: redirect /dev-access
U->>FA: POST /api/access-gate/unlock {password}
FA->>FA: compare password (== operator ⚠️)
FA-->>U: 200 OK + Set-Cookie: dev_access_granted=sha256
U->>NX: GET /
NX->>NX: hasAccessGateCookie() → true
NX-->>U: 200 Render page
Note over U,FA: JWT Auth flow
U->>FA: POST /api/auth/token (form: username, password)
FA->>FA: _authenticate_user(): verify password hash
FA-->>U: {access_token, token_type: bearer}
U->>U: localStorage.setItem(pyrmit_access_token, token)
Note over U,FA: Authenticated API request
U->>FA: GET /api/auth/me Authorization: Bearer token
FA->>FA: oauth2_scheme → decode_access_token → get_current_user
FA-->>U: {id, name, email}
Note over U,FA: Sign-out
U->>FA: POST /api/auth/signout (Bearer token)
FA-->>U: {message: Signed out} (token still valid ⚠️)
U->>U: localStorage.removeItem(pyrmit_access_token)
Reviews (3): Last reviewed commit: "feat: fix dev-access gate + remove nextj..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| export default function DevAccessPage() { | ||
| const [password, setPassword] = useState(''); | ||
| const [error, setError] = useState(''); |
There was a problem hiding this comment.
next parameter
The nextPath value is read directly from the URL query string and passed to router.replace() without any validation. If an attacker crafts a link like /dev-access?next=https://evil.com, a legitimate user who enters the correct password will be silently redirected to an external site. This is a classic open-redirect / phishing vector.
Only allow relative paths (those that begin with /):
| const [error, setError] = useState(''); | |
| const rawNext = searchParams.get('next') ?? '/'; | |
| const nextPath = rawNext.startsWith('/') ? rawNext : '/'; |
| const ACCESS_TOKEN_KEY = 'pyrmit_access_token'; | ||
|
|
||
| export function getStoredAccessToken(): string | null { | ||
| if (typeof window === 'undefined') { | ||
| return null; | ||
| } | ||
| return window.localStorage.getItem(ACCESS_TOKEN_KEY); | ||
| } | ||
|
|
||
| export function storeAccessToken(token: string) { | ||
| window.localStorage.setItem(ACCESS_TOKEN_KEY, token); | ||
| } | ||
|
|
||
| export function clearAccessToken() { |
There was a problem hiding this comment.
localStorage is XSS-accessible
localStorage is readable by any JavaScript running on the page. If a dependency or user-generated content introduces an XSS vulnerability, the bearer token can be exfiltrated and used to impersonate the user until it expires. The conventional mitigation is to store the token in an httpOnly cookie (which JavaScript cannot read), similar to the previous session_token approach.
For a development tool this risk is lower, but if the app ever moves to production this will need to be revisited. Consider at minimum documenting the trade-off in a comment here so it isn't overlooked.
| def signup(request: schemas.SignUpRequest, db: Session = Depends(get_db)): | ||
| existing_user = db.query(models.User).filter(models.User.email == request.email).first() |
There was a problem hiding this comment.
signout is a no-op — issued tokens remain valid until expiry
The endpoint now returns a success response without doing anything server-side. Because JWTs are stateless, a token that was issued before logout is still cryptographically valid for up to ACCESS_TOKEN_EXPIRE_MINUTES minutes (30 by default). If a token is leaked or copied before logout, it can continue to be used after the user believes they've signed out.
Common mitigations include:
- Maintaining a server-side token revocation/blocklist (e.g. a small Redis set of
jtivalues) - Reducing the
ACCESS_TOKEN_EXPIRE_MINUTESvalue significantly (e.g. 5–10 min) and pairing it with a longer-lived refresh token
For a development tool the current risk is acceptable, but it's worth documenting so consumers of this endpoint understand it doesn't actually invalidate the token.
| if submitted_password != configured_password: | ||
| raise HTTPException(status_code=401, detail="Invalid access password") |
There was a problem hiding this comment.
!= short-circuits on the first mismatched character, leaking information about how many leading characters of the submitted password match the configured one. Since this is the primary user-facing endpoint for entering the password, an attacker can repeatedly call it and measure response latencies to brute-force the secret one character at a time.
Use hmac.compare_digest for a constant-time comparison — add import hmac at the top of the file and replace the check:
| if submitted_password != configured_password: | |
| raise HTTPException(status_code=401, detail="Invalid access password") | |
| if not hmac.compare_digest(submitted_password, configured_password): |
1 participant
No description provided.