feat(api): add PAM auth and token utilities#16
Merged
Merged
Conversation
- Add PAM-based login flow using a Python PAM verifier and a new pam-verify.py script - Implement token-based auth: sign/verify access tokens, rotate and store refresh tokens in SQLite - Add refresh_tokens table, Drizzle migration, and related utilities to manage tokens - Auto-generate and load a JWT secret on first API startup; initialize auth at boot - Expose /auth/login, /auth/logout, /auth/refresh, and /auth/me endpoints - Update Dockerfile to install Python3 for PAM script execution - Add frontend login page and route; integrate with auth client and layout - Add docs pages for authentication and installation; include new auth.md - Add unit tests for auth utilities and DB integrations - Update server-info endpoint to check base domain status
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds PAM-based login, JWT access and refresh tokens, auth routes and middleware, a web login page with route and guard updates, DNS base-domain status reporting, and related container, docs, and test changes. ChangesAuthentication System
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
apps/web/src/api/client.ts (1)
346-356: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winTighten auth response typing to match backend success/error contracts.
Lines 347 and 356 currently model success-only fields as always present, while
/auth/loginand/auth/refreshcan return{ error: string }on failure. Prefer a union type so callers must branch safely.♻️ Proposed refactor
+type AuthSuccess = { ok: true; username: string }; +type AuthError = { error: string; ok?: false }; + export const login = (username: string, password: string) => - apiFetch<{ ok: boolean; username: string; error?: string }>("/auth/login", { + apiFetch<AuthSuccess | AuthError>("/auth/login", { method: "POST", body: JSON.stringify({ username, password }), }); export const refreshSession = () => - apiFetch<{ ok: boolean; username: string }>("/auth/refresh", { method: "POST" }); + apiFetch<AuthSuccess | AuthError>("/auth/refresh", { method: "POST" });🤖 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 `@apps/web/src/api/client.ts` around lines 346 - 356, The type definitions for the login function and refreshSession function currently use optional error fields (error?: string) rather than proper union types that reflect the backend's contract of returning either a success response or an error response. Replace the generic object types with union types that distinguish between success cases (ok: true with username field) and error cases (ok: false with error field), so that callers are forced to safely handle both branches based on the ok boolean field.apps/api/src/api/index.ts (2)
21-49: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDynamic imports on every request add unnecessary latency.
import("../utils/auth")andimport("../db/repo")are called on each authenticated request. While Bun caches modules, the async import overhead accumulates. Consider moving these to top-level imports.Proposed fix
+import { verifyAccessToken } from "../utils/auth"; +import { validateApiKey } from "../db/repo"; import { authRoutes } from "./auth"; // ... const authMiddleware = (app: Elysia) => app.onBeforeHandle(async ({ request, set, path }) => { if (BYPASS_PATHS.has(path)) return; const cookie = request.headers.get("cookie") || ""; const match = cookie.match(/(?:^|;\s*)dequel_session=([^;]+)/); if (match) { - const { verifyAccessToken } = await import("../utils/auth"); const payload = await verifyAccessToken(match[1]); if (payload) return; set.status = 401; return { error: "Invalid session" }; } const authHeader = request.headers.get("authorization"); if (authHeader?.startsWith("Bearer ")) { const token = authHeader.slice(7); if (token) { - const { validateApiKey } = await import("../db/repo"); const key = await validateApiKey(token);🤖 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 `@apps/api/src/api/index.ts` around lines 21 - 49, The authMiddleware function is performing dynamic imports of `import("../utils/auth")` and `import("../db/repo")` on every request, which adds unnecessary latency. Move these imports to the top level of the file by importing the verifyAccessToken function and validateApiKey function at the module level instead of dynamically importing them inside the onBeforeHandle callback. This way the modules are only loaded once rather than on every authenticated request.
26-26: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueCookie value should be URL-decoded.
The regex extracts the raw cookie value without decoding. If
dequel_sessioncontains URL-encoded characters (which JWT tokens with base64url typically don't, but cookie values might), validation could fail unexpectedly.const match = cookie.match(/(?:^|;\s*)dequel_session=([^;]+)/); if (match) { - const payload = await verifyAccessToken(match[1]); + const payload = await verifyAccessToken(decodeURIComponent(match[1]));🤖 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 `@apps/api/src/api/index.ts` at line 26, The cookie value extracted from the regex match for dequel_session is not being URL-decoded. After capturing the cookie value using the regex match on the cookie string, apply decodeURIComponent() to the extracted value to properly decode any URL-encoded characters before using it for validation or further processing.apps/api/src/index.ts (1)
33-33: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider logging cleanup errors for observability.
The empty
.catch(() => {})silently swallows all errors fromcleanupExpiredTokens. While cleanup failures aren't critical, logging them would help diagnose issues in production.- setInterval(() => { cleanupExpiredTokens().catch(() => {}); }, 60_000); + setInterval(() => { cleanupExpiredTokens().catch((e) => console.error("Token cleanup failed:", e)); }, 60_000);🤖 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 `@apps/api/src/index.ts` at line 33, The empty catch handler in the setInterval callback for cleanupExpiredTokens is silently ignoring all errors, which prevents observability and makes production debugging difficult. Modify the catch handler to log the error details using an appropriate logger instead of an empty function. This way, cleanup failures will be visible in logs for monitoring and troubleshooting purposes while the interval continues to run.
🤖 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 `@apps/api/src/api/auth/index.ts`:
- Around line 66-86: The try-catch block around the blacklistRefreshToken(rt)
call on line 77 silently ignores all errors, creating a security race condition
where the old refresh token remains valid if blacklisting fails. Remove the
empty catch block and either propagate the error by letting exceptions from
blacklistRefreshToken bubble up, or explicitly catch the error and return a 500
error response to the client, ensuring that token rotation fails safely if the
blacklist operation cannot complete successfully.
- Around line 28-34: The COOKIE_OPTS constant currently uses a maxAge of 7 days
for both the access token and refresh token cookies, but the access token JWT
expires after 15 minutes (900 seconds), creating a mismatch where the cookie
remains valid long after the token becomes invalid. Create a separate cookie
options object specifically for the dequel_session access token with a shorter
maxAge of approximately 15-20 minutes (900-1200 seconds) to match the JWT
expiry, while keeping the existing COOKIE_OPTS with the 7-day maxAge for the
dequel_refresh cookie since refresh tokens also expire after 7 days in the
database. Update the cookie configuration to use the appropriate options object
when setting each cookie type.
In `@apps/api/src/utils/__tests__/auth.test.ts`:
- Around line 48-56: The test for verifying expired tokens in the 'rejects
expired token' test uses a fake signature ('fakesig'), causing verifyAccessToken
to return null due to signature validation failure before it can check the
expiration time. To properly exercise the expiration branch, create a valid JWT
token with a proper signature (either by using the actual signing key or by
mocking the signature verification) while keeping the expired timestamp (exp:
1), so that the test can verify the function correctly rejects expired tokens
rather than just invalid signatures.
In `@apps/api/src/utils/auth.ts`:
- Around line 93-98: The expiration timestamp comparison in the refresh token
validation query has a format mismatch issue. The refreshTokens.expiresAt column
stores timestamps in ISO 8601 format, but the sql template string compares it
directly against datetime('now') which returns a different format, causing
incorrect lexical string comparisons. To fix this, wrap the
refreshTokens.expiresAt column reference with the datetime() function in the sql
template string (in the condition checking `${refreshTokens.expiresAt} >
datetime('now')`) to normalize both sides of the comparison to the same format.
In `@apps/docs/src/content/docs/auth.md`:
- Around line 43-52: The `/api/auth/logout` endpoint in the endpoints table is
marked as requiring "Session" auth, but the actual handler does not validate
that session cookies are present—it succeeds regardless of auth state. Either
update the Auth column in the table for the `/api/auth/logout` row from
"Session" to "None" to accurately reflect that the endpoint does not enforce
authentication, or modify the endpoint handler to validate and enforce the
presence of session cookies before allowing the logout operation to complete.
In `@apps/web/src/api/client.ts`:
- Line 345: Remove the non-essential comment "// Auth" from the client.ts file.
This section comment does not provide critical information about the code
functionality and violates the coding guideline that comments in source code
should only be used when absolutely necessary. Delete the entire line containing
this comment to maintain code cleanliness and comply with the project's
standards.
In `@apps/web/src/components/Layout.tsx`:
- Around line 21-33: The useProjects hook (referenced at lines 92-101) executes
on every render, including before the authentication check in the useEffect can
redirect unauthenticated users, causing unnecessary protected API calls. Gate
the useProjects hook with the same authentication conditions used in the
redirect logic: only call it when me?.authenticated is true and the current
location is not the login path. Either wrap the hook call in a conditional that
checks these conditions, or if useProjects supports an enabled/disabled option,
pass a condition that evaluates to true only when the user is authenticated and
not on the /login route.
In `@apps/web/src/routes/Login.tsx`:
- Around line 32-223: Remove all decorative and non-essential JSX comments
throughout the Login component in the file. This includes structural comments
such as those labeling the background glow section, center content container,
left column, logo branding, form, header mockup, sidebar mockup, main area
mockup, footer info, and other similar comments that describe component layout
but don't provide necessary information about code logic or behavior. Keep only
comments that are absolutely required for understanding complex logic or
important implementation details.
In `@docker-compose.yml`:
- Around line 51-53: The docker-compose.yml is currently mounting the host's
/etc/shadow file directly into the API container, which exposes password hashes
if the API is compromised. Remove the /etc/shadow mount from the API container's
volumes section. Instead, create a separate tightly scoped host-side PAM helper
or sidecar service that handles authentication verification. Configure the API
container to communicate with this helper service through a secure interface
(such as a Unix socket or authenticated network endpoint) rather than directly
accessing sensitive host files, ensuring the API process never needs direct
access to /etc/shadow.
In `@scripts/pam-verify.py`:
- Around line 44-45: The libpam variable assignment uses
ctypes.util.find_library("pam") which can return None if the PAM library is not
found, causing LoadLibrary(None) to fail with an unclear error message. Check if
find_library("pam") returns a non-None value before passing it to LoadLibrary,
and raise a descriptive exception with a clear error message if the PAM library
cannot be found on the system.
- Around line 93-102: The group membership verification using getent group only
checks supplementary group members but misses users whose primary group is
GRP_NAME. To fix this, after parsing the supplementary members from
result.stdout, also call subprocess.run with getent passwd username to retrieve
the user's primary GID, then parse the GID from that output. Compare the user's
primary GID with the group's GID (the third colon-separated field from the
getent group output). Update the membership check logic to return success if
either the username is found in the supplementary members list or the user's
primary GID matches the group's GID.
---
Nitpick comments:
In `@apps/api/src/api/index.ts`:
- Around line 21-49: The authMiddleware function is performing dynamic imports
of `import("../utils/auth")` and `import("../db/repo")` on every request, which
adds unnecessary latency. Move these imports to the top level of the file by
importing the verifyAccessToken function and validateApiKey function at the
module level instead of dynamically importing them inside the onBeforeHandle
callback. This way the modules are only loaded once rather than on every
authenticated request.
- Line 26: The cookie value extracted from the regex match for dequel_session is
not being URL-decoded. After capturing the cookie value using the regex match on
the cookie string, apply decodeURIComponent() to the extracted value to properly
decode any URL-encoded characters before using it for validation or further
processing.
In `@apps/api/src/index.ts`:
- Line 33: The empty catch handler in the setInterval callback for
cleanupExpiredTokens is silently ignoring all errors, which prevents
observability and makes production debugging difficult. Modify the catch handler
to log the error details using an appropriate logger instead of an empty
function. This way, cleanup failures will be visible in logs for monitoring and
troubleshooting purposes while the interval continues to run.
In `@apps/web/src/api/client.ts`:
- Around line 346-356: The type definitions for the login function and
refreshSession function currently use optional error fields (error?: string)
rather than proper union types that reflect the backend's contract of returning
either a success response or an error response. Replace the generic object types
with union types that distinguish between success cases (ok: true with username
field) and error cases (ok: false with error field), so that callers are forced
to safely handle both branches based on the ok boolean field.
🪄 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
Run ID: 25bbe8ec-cc79-4c8b-b9e0-59ab341474fc
📒 Files selected for processing (23)
apps/api/Dockerfileapps/api/src/api/auth/index.tsapps/api/src/api/index.tsapps/api/src/api/server-info/index.tsapps/api/src/db/__tests__/auth.test.tsapps/api/src/db/migrations/0001_refresh_tokens.sqlapps/api/src/db/migrations/meta/_journal.jsonapps/api/src/db/schema.tsapps/api/src/index.tsapps/api/src/utils/__tests__/auth.test.tsapps/api/src/utils/auth.tsapps/api/src/utils/config.tsapps/api/src/utils/dns.tsapps/api/src/utils/secrets.tsapps/docs/src/content/docs/auth.mdapps/docs/src/content/docs/installation.mdapps/web/src/api/client.tsapps/web/src/components/ConfigWarnings.tsxapps/web/src/components/Layout.tsxapps/web/src/routes/Login.tsxapps/web/src/routes/index.tsxdocker-compose.ymlscripts/pam-verify.py
Comment on lines
+43
to
+52
| ## Endpoints | ||
|
|
||
| | Method | Path | Auth | Description | | ||
| |--------|------|------|-------------| | ||
| | `POST` | `/api/auth/login` | None | Sign in with username/password | | ||
| | `POST` | `/api/auth/logout` | Session | Sign out, blacklist refresh token | | ||
| | `POST` | `/api/auth/refresh` | Session | Rotate refresh token | | ||
| | `GET` | `/api/auth/me` | None | Check current session status | | ||
| | `GET` | `/api/health` | None | Health check | | ||
|
|
There was a problem hiding this comment.
Minor: /auth/logout endpoint auth annotation inconsistency.
The table marks /auth/logout as requiring "Session" auth, but the handler does not validate the presence of session cookies—it silently succeeds even if they are absent. While the endpoint uses the cookies (if present), it does not reject requests without them.
Either clarify that logout succeeds regardless of auth state, or add validation to the endpoint to enforce the "Session" requirement. As written, "None" would be more accurate than "Session".
🤖 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 `@apps/docs/src/content/docs/auth.md` around lines 43 - 52, The
`/api/auth/logout` endpoint in the endpoints table is marked as requiring
"Session" auth, but the actual handler does not validate that session cookies
are present—it succeeds regardless of auth state. Either update the Auth column
in the table for the `/api/auth/logout` row from "Session" to "None" to
accurately reflect that the endpoint does not enforce authentication, or modify
the endpoint handler to validate and enforce the presence of session cookies
before allowing the logout operation to complete.
Comment on lines
+32
to
+223
| {/* Background glow using Dequel orange */} | ||
| <div className="absolute top-0 left-0 w-[400px] h-[400px] bg-orange-500/[0.02] blur-[100px] rounded-full pointer-events-none -translate-x-1/2 -translate-y-1/2" /> | ||
|
|
||
| {/* Center content container for closer alignment */} | ||
| <div className="w-full max-w-4xl flex flex-col lg:flex-row items-center justify-center gap-12 lg:gap-16 z-10"> | ||
|
|
||
| {/* Left Column: Login Card & Heading */} | ||
| <div className="w-full max-w-sm flex flex-col justify-between min-h-[480px] space-y-6"> | ||
| {/* Logo Branding */} | ||
| <div className="flex items-center gap-2"> | ||
| <DequelLogo className="h-6 w-6" /> | ||
| <span className="font-display font-bold text-base tracking-wider text-zinc-200"> | ||
| dequel | ||
| </span> | ||
| <span className="text-[9px] px-1.5 py-0.5 rounded-full bg-orange-500/10 text-orange-500 border border-orange-500/20 font-mono font-bold"> | ||
| v0.1 | ||
| </span> | ||
| </div> | ||
|
|
||
| {/* Form */} | ||
| <div className="space-y-5"> | ||
| <div className="space-y-1"> | ||
| <h1 className="text-2xl font-bold tracking-tight text-zinc-100"> | ||
| Sign in to Dequel | ||
| </h1> | ||
| <p className="text-zinc-500 text-xs leading-normal"> | ||
| Enter your credentials to access your self-hosted deployment engine. | ||
| </p> | ||
| </div> | ||
|
|
||
| <div className="relative rounded-xl border border-[#1a1a1f] bg-[#0c0c0e] p-5 shadow-xl space-y-5"> | ||
| <div className="flex items-center justify-between border-b border-[#1a1a1f] pb-3"> | ||
| <span className="text-[10px] font-bold uppercase tracking-wider text-zinc-400 flex items-center gap-1.5"> | ||
| <Shield className="h-3.5 w-3.5 text-zinc-400" /> | ||
| Security Gateway | ||
| </span> | ||
| <span className="flex items-center gap-1 text-[9px] text-emerald-400 font-medium"> | ||
| <span className="w-1 h-1 rounded-full bg-emerald-500 animate-pulse" /> | ||
| Active | ||
| </span> | ||
| </div> | ||
|
|
||
| <form onSubmit={handleSubmit} className="space-y-3.5"> | ||
| <div className="space-y-1.5"> | ||
| <label className="block text-[10px] font-bold text-zinc-500 uppercase tracking-wider">Username</label> | ||
| <div className="relative"> | ||
| <User className="absolute left-3 top-2.5 h-4 w-4 text-zinc-655" /> | ||
| <input | ||
| type="text" | ||
| value={username} | ||
| onChange={e => setUsername(e.target.value)} | ||
| className="w-full pl-9 pr-4 py-2 rounded-lg border border-[#222227] bg-[#121214] text-zinc-200 text-xs placeholder-zinc-700 focus:outline-none focus:border-orange-500 focus:ring-1 focus:ring-orange-500/20 transition-all" | ||
| placeholder="Linux username" | ||
| autoFocus | ||
| required | ||
| /> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="space-y-1.5"> | ||
| <label className="block text-[10px] font-bold text-zinc-500 uppercase tracking-wider">Password</label> | ||
| <div className="relative"> | ||
| <Lock className="absolute left-3 top-2.5 h-4 w-4 text-zinc-655" /> | ||
| <input | ||
| type="password" | ||
| value={password} | ||
| onChange={e => setPassword(e.target.value)} | ||
| className="w-full pl-9 pr-4 py-2 rounded-lg border border-[#222227] bg-[#121214] text-zinc-200 text-xs placeholder-zinc-700 focus:outline-none focus:border-orange-500 focus:ring-1 focus:ring-orange-500/20 transition-all" | ||
| placeholder="Linux password" | ||
| required | ||
| /> | ||
| </div> | ||
| </div> | ||
|
|
||
| {error && ( | ||
| <p className="text-[11px] text-rose-400 bg-rose-500/5 rounded-lg px-3 py-2 border border-rose-500/10 leading-normal"> | ||
| {error} | ||
| </p> | ||
| )} | ||
|
|
||
| <button | ||
| type="submit" | ||
| disabled={loading} | ||
| className="w-full py-2 px-4 rounded-lg bg-orange-500 hover:bg-orange-600 text-white text-xs font-semibold transition-all active:scale-[0.98] disabled:opacity-50 disabled:cursor-not-allowed flex items-center justify-center gap-1.5 group" | ||
| > | ||
| {loading ? ( | ||
| <> | ||
| <RefreshCw className="h-3.5 w-3.5 animate-spin" /> | ||
| Signing in... | ||
| </> | ||
| ) : ( | ||
| <> | ||
| Sign In | ||
| <ArrowRight className="h-3.5 w-3.5 group-hover:translate-x-0.5 transition-transform" /> | ||
| </> | ||
| )} | ||
| </button> | ||
| </form> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Footer Info */} | ||
| <div className="text-[10px] text-zinc-600 font-mono pt-4"> | ||
| © {new Date().getFullYear()} Dequel. | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Right Column: Clean, Low-contrast CSS Mockup of Dequel Dashboard */} | ||
| <div className="hidden lg:flex w-full max-w-[480px] aspect-[16/10] rounded-xl border border-[#1a1a1f] bg-[#0c0c0e] shadow-xl flex flex-col overflow-hidden shrink-0"> | ||
| {/* Header Mockup */} | ||
| <div className="h-8 bg-[#101012] border-b border-[#1a1a1f] flex items-center px-3 justify-between"> | ||
| <div className="flex gap-1 items-center"> | ||
| <span className="w-2 h-2 rounded-full bg-zinc-800" /> | ||
| <span className="w-2 h-2 rounded-full bg-zinc-800" /> | ||
| <span className="w-2 h-2 rounded-full bg-zinc-800" /> | ||
| </div> | ||
| <div className="w-48 h-4 rounded bg-[#070708] border border-[#1a1a1f] flex items-center justify-center text-[8px] text-zinc-600 font-mono"> | ||
| dequel.local/dashboard | ||
| </div> | ||
| <div className="w-8" /> | ||
| </div> | ||
|
|
||
| {/* Content Layout Mockup */} | ||
| <div className="flex-1 flex overflow-hidden"> | ||
| {/* Sidebar Mockup */} | ||
| <div className="w-32 border-r border-[#1a1a1f] bg-[#09090b] p-2.5 flex flex-col justify-between text-[8px]"> | ||
| <div className="space-y-3"> | ||
| <div className="flex items-center gap-1 px-1 pb-1.5 border-b border-[#1a1a1f]/60"> | ||
| <DequelLogo className="h-3 w-3" /> | ||
| <span className="font-bold text-zinc-400">dequel</span> | ||
| </div> | ||
|
|
||
| <div className="space-y-1"> | ||
| <div className="h-5 rounded bg-orange-500/10 flex items-center px-1.5 text-orange-500 gap-1 font-semibold"> | ||
| <Activity className="h-2.5 w-2.5 text-orange-500" /> | ||
| Overview | ||
| </div> | ||
| <div className="h-5 rounded flex items-center px-1.5 text-zinc-650 gap-1"> | ||
| <Terminal className="h-2.5 w-2.5" /> | ||
| Logs | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Sidebar Footer Widget Mockup */} | ||
| <div className="rounded bg-[#101012] border border-[#1a1a1f] p-1.5 space-y-1 text-[7px]"> | ||
| <div className="flex items-center justify-between text-zinc-500 pb-1 border-b border-[#1a1a1f]"> | ||
| <span className="font-bold">STATUS</span> | ||
| <span className="text-emerald-500 flex items-center gap-0.5"> | ||
| <span className="w-0.5 h-0.5 rounded-full bg-emerald-500 animate-pulse" /> | ||
| Live | ||
| </span> | ||
| </div> | ||
| <div className="flex justify-between text-zinc-600"> | ||
| <span>Services</span> | ||
| <span className="font-mono text-zinc-400">3</span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Main Area Mockup */} | ||
| <div className="flex-1 bg-[#070708] p-3 flex flex-col gap-3"> | ||
| <div className="flex justify-between items-center pb-1.5 border-b border-[#1a1a1f]"> | ||
| <div> | ||
| <h3 className="text-[10px] font-bold text-zinc-300">Overview</h3> | ||
| <p className="text-[7px] text-zinc-600">Manage and monitor cluster resources.</p> | ||
| </div> | ||
| <div className="h-4.5 px-2 rounded bg-orange-500 text-white text-[7px] font-bold flex items-center justify-center"> | ||
| + New Project | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Metric stats card */} | ||
| <div className="grid grid-cols-2 gap-2 text-[8px]"> | ||
| <div className="rounded border border-[#1a1a1f] bg-[#0c0c0e] p-2 flex items-center justify-between"> | ||
| <div className="space-y-0.5"> | ||
| <div className="text-zinc-650 font-bold uppercase text-[6px]">API Traffic</div> | ||
| <div className="text-zinc-300 font-mono font-bold">148 <span className="text-[6px] text-zinc-600 font-normal">reqs</span></div> | ||
| </div> | ||
| <Activity className="h-2.5 w-2.5 text-orange-500 animate-pulse" /> | ||
| </div> | ||
|
|
||
| <div className="rounded border border-[#1a1a1f] bg-[#0c0c0e] p-2 flex items-center justify-between"> | ||
| <div className="space-y-0.5"> | ||
| <div className="text-zinc-650 font-bold uppercase text-[6px]">Deployments</div> | ||
| <div className="text-zinc-300 font-mono font-bold">3 <span className="text-[6px] text-zinc-600 font-normal">active</span></div> | ||
| </div> | ||
| <Shield className="h-2.5 w-2.5 text-rose-500" /> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Projects list */} |
There was a problem hiding this comment.
Drop decorative JSX comments in this TSX file.
Lines 32-223 include many non-essential comments; these should be removed to comply with repository JS/TS rules.
As per coding guidelines, "No comments in source code unless absolutely necessary".
🤖 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 `@apps/web/src/routes/Login.tsx` around lines 32 - 223, Remove all decorative
and non-essential JSX comments throughout the Login component in the file. This
includes structural comments such as those labeling the background glow section,
center content container, left column, logo branding, form, header mockup,
sidebar mockup, main area mockup, footer info, and other similar comments that
describe component layout but don't provide necessary information about code
logic or behavior. Keep only comments that are absolutely required for
understanding complex logic or important implementation details.
Source: Coding guidelines
Comment on lines
+44
to
+45
| libpam = ctypes.cdll.LoadLibrary(ctypes.util.find_library("pam")) | ||
| libc = ctypes.cdll.LoadLibrary("libc.so.6") |
There was a problem hiding this comment.
Handle missing PAM library gracefully.
ctypes.util.find_library("pam") returns None if the library isn't found, causing LoadLibrary(None) to fail with an unclear error.
Proposed fix
def verify(username: str, password: str) -> dict:
- libpam = ctypes.cdll.LoadLibrary(ctypes.util.find_library("pam"))
- libc = ctypes.cdll.LoadLibrary("libc.so.6")
+ pam_path = ctypes.util.find_library("pam")
+ if not pam_path:
+ return {"ok": False, "error": "PAM library not found"}
+ libpam = ctypes.cdll.LoadLibrary(pam_path)
+ libc = ctypes.cdll.LoadLibrary(ctypes.util.find_library("c") or "libc.so.6")📝 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.
Suggested change
| libpam = ctypes.cdll.LoadLibrary(ctypes.util.find_library("pam")) | |
| libc = ctypes.cdll.LoadLibrary("libc.so.6") | |
| def verify(username: str, password: str) -> dict: | |
| pam_path = ctypes.util.find_library("pam") | |
| if not pam_path: | |
| return {"ok": False, "error": "PAM library not found"} | |
| libpam = ctypes.cdll.LoadLibrary(pam_path) | |
| libc = ctypes.cdll.LoadLibrary(ctypes.util.find_library("c") or "libc.so.6") |
🤖 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 `@scripts/pam-verify.py` around lines 44 - 45, The libpam variable assignment
uses ctypes.util.find_library("pam") which can return None if the PAM library is
not found, causing LoadLibrary(None) to fail with an unclear error message.
Check if find_library("pam") returns a non-None value before passing it to
LoadLibrary, and raise a descriptive exception with a clear error message if the
PAM library cannot be found on the system.
Comment on lines
+93
to
+102
| result = subprocess.run( | ||
| ["getent", "group", GRP_NAME], | ||
| capture_output=True, text=True, timeout=10, | ||
| ) | ||
| if result.returncode != 0: | ||
| return {"ok": False, "error": f"Group '{GRP_NAME}' does not exist"} | ||
|
|
||
| members = result.stdout.strip().split(":")[-1].split(",") if ":" in result.stdout else [] | ||
| if username not in members: | ||
| return {"ok": False, "error": f"User '{username}' is not in the '{GRP_NAME}' group"} |
There was a problem hiding this comment.
Group membership check misses users whose primary group is dequel.
The getent group command returns supplementary group members only. If a user's primary group is dequel (as shown in /etc/passwd), they won't appear in the members list returned by getent group dequel.
Consider also checking the user's primary GID:
Proposed fix
+import pwd
+import grp
result = subprocess.run(
["getent", "group", GRP_NAME],
capture_output=True, text=True, timeout=10,
)
if result.returncode != 0:
return {"ok": False, "error": f"Group '{GRP_NAME}' does not exist"}
members = result.stdout.strip().split(":")[-1].split(",") if ":" in result.stdout else []
- if username not in members:
+ try:
+ user_info = pwd.getpwnam(username)
+ grp_info = grp.getgrgid(user_info.pw_gid)
+ is_primary = grp_info.gr_name == GRP_NAME
+ except KeyError:
+ is_primary = False
+ if username not in members and not is_primary:
return {"ok": False, "error": f"User '{username}' is not in the '{GRP_NAME}' group"}📝 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.
Suggested change
| result = subprocess.run( | |
| ["getent", "group", GRP_NAME], | |
| capture_output=True, text=True, timeout=10, | |
| ) | |
| if result.returncode != 0: | |
| return {"ok": False, "error": f"Group '{GRP_NAME}' does not exist"} | |
| members = result.stdout.strip().split(":")[-1].split(",") if ":" in result.stdout else [] | |
| if username not in members: | |
| return {"ok": False, "error": f"User '{username}' is not in the '{GRP_NAME}' group"} | |
| import pwd | |
| import grp | |
| result = subprocess.run( | |
| ["getent", "group", GRP_NAME], | |
| capture_output=True, text=True, timeout=10, | |
| ) | |
| if result.returncode != 0: | |
| return {"ok": False, "error": f"Group '{GRP_NAME}' does not exist"} | |
| members = result.stdout.strip().split(":")[-1].split(",") if ":" in result.stdout else [] | |
| try: | |
| user_info = pwd.getpwnam(username) | |
| grp_info = grp.getgrgid(user_info.pw_gid) | |
| is_primary = grp_info.gr_name == GRP_NAME | |
| except KeyError: | |
| is_primary = False | |
| if username not in members and not is_primary: | |
| return {"ok": False, "error": f"User '{username}' is not in the '{GRP_NAME}' group"} |
🧰 Tools
🪛 Ruff (0.15.17)
[error] 93-93: subprocess call: check for execution of untrusted input
(S603)
[error] 94-94: Starting a process with a partial executable path
(S607)
🤖 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 `@scripts/pam-verify.py` around lines 93 - 102, The group membership
verification using getent group only checks supplementary group members but
misses users whose primary group is GRP_NAME. To fix this, after parsing the
supplementary members from result.stdout, also call subprocess.run with getent
passwd username to retrieve the user's primary GID, then parse the GID from that
output. Compare the user's primary GID with the group's GID (the third
colon-separated field from the getent group output). Update the membership check
logic to return success if either the username is found in the supplementary
members list or the user's primary GID matches the group's GID.
- Replace in-process PAM verification with HTTP PAM service at pam-auth - Use pam-auth:4567 for authentication requests - Rename and relocate PAM server script to scripts/auth/pam-server.py - Update tests to reflect new PAM flow and token handling
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/api/src/utils/auth.ts (1)
116-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
cleanupExpiredTokensstill has the timestamp format mismatch.
expires_atis stored in ISO 8601 (...T..Z) but compared lexically againstdatetime('now')(YYYY-MM-DD HH:MM:SS). TheTseparator (0x54) sorts after the space (0x20), so tokens that expired earlier the same day compare as not less-than-now and survive cleanup. The companion fix was applied tovalidateRefreshToken(Line 98) but not here — wrapexpires_atindatetime()for consistency.🛠️ Proposed fix
- db.run(`DELETE FROM refresh_tokens WHERE expires_at < datetime('now')`); + db.run(`DELETE FROM refresh_tokens WHERE datetime(expires_at) < datetime('now')`);🤖 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 `@apps/api/src/utils/auth.ts` at line 116, In the cleanupExpiredTokens function, the DELETE statement compares expires_at (stored in ISO 8601 format with T separator) directly against datetime('now') (YYYY-MM-DD HH:MM:SS format), causing a lexical mismatch where tokens with T character sort after the space character, preventing proper cleanup. Wrap the expires_at column with the datetime() function in the WHERE clause condition to convert it to the same format as datetime('now'), matching the fix that was already applied to validateRefreshToken for consistency.
🧹 Nitpick comments (3)
scripts/auth/pam-server.py (1)
160-163: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider
ThreadingHTTPServerfor concurrent auth requests.
HTTPServerhandles requests serially; a slow PAM call blocks all other login attempts (and the healthcheck). Usinghttp.server.ThreadingHTTPServerlets concurrent logins proceed.🤖 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 `@scripts/auth/pam-server.py` around lines 160 - 163, The main() function currently uses HTTPServer which processes requests serially, causing slow PAM authentication calls to block other login attempts and healthchecks. Replace HTTPServer with ThreadingHTTPServer (from the http.server module) to enable concurrent request handling. Ensure ThreadingHTTPServer is imported at the top of the file if it is not already imported, then update the server instantiation in main() to use ThreadingHTTPServer instead of HTTPServer while keeping the same arguments.docker-compose.yml (2)
76-80: 🧹 Nitpick | 🔵 TrivialInstalling packages at container start is fragile.
Running
apt-get update && apt-get install libpam-moduleson every startup adds a hard runtime dependency on apt mirrors and slows boot/restarts. Prefer baking a small dedicated image (Dockerfile based onpython:3-slimwithlibpam-modulespreinstalled) so the sidecar starts deterministically and offline.🤖 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 `@docker-compose.yml` around lines 76 - 80, The docker-compose.yml command block is installing libpam-modules at container startup using apt-get, which creates a fragile runtime dependency on apt mirrors and slows boot times. Instead, create a dedicated Dockerfile based on python:3-slim that pre-installs libpam-modules and sets up the pam-server.py runtime during image build time. Then update the docker-compose.yml to build and use this custom image instead of the inline apt-get installation command, ensuring the sidecar starts deterministically without network dependencies.
53-55: 🧹 Nitpick | 🔵 TrivialAdd
pam-authto theapiservice'sdepends_on.
apicallspam-auth:4567for login but does not wait for it. Until the sidecar is healthy,/auth/loginreturns "Auth service unavailable". Adding aservice_healthydependency (the sidecar already defines a healthcheck) avoids this startup window.🛠️ Proposed change
depends_on: buildkit: condition: service_started + pam-auth: + condition: service_healthy🤖 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 `@docker-compose.yml` around lines 53 - 55, The api service in docker-compose.yml currently only declares a dependency on buildkit but does not wait for pam-auth to be healthy before starting, even though api calls pam-auth on port 4567 for login functionality. Add pam-auth to the depends_on section of the api service with a condition of service_healthy (since pam-auth already defines a healthcheck) to ensure the authentication service is ready before the api service starts accepting login requests.
🤖 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 `@apps/api/src/api/auth/index.ts`:
- Around line 7-19: The callPam function lacks a timeout on the fetch request to
PAM_AUTH_URL, which can cause the /auth/login endpoint to hang indefinitely if
the pam-auth service accepts the connection but stalls. Add an AbortSignal with
a 5-second timeout to the fetch options by including a signal property that uses
AbortSignal.timeout(5000). This will cause the fetch to abort if the response
takes longer than 5 seconds, triggering the existing catch block and returning
the "Auth service unavailable" error response.
In `@scripts/auth/pam-server.py`:
- Around line 54-71: The libc.malloc and libc.strdup function calls in the conv
function are returning truncated pointers on 64-bit systems because ctypes
defaults the restype to c_int. Before the malloc call at line 57 and the strdup
call at line 64, explicitly set the restype for these functions to the correct
pointer type: set libc.malloc.restype to ctypes.c_void_p and libc.strdup.restype
to ctypes.c_char_p. This ensures the returned pointers are not truncated and the
subsequent operations using buf and pw_buf operate on correct memory addresses.
In `@scripts/auth/pam-verify.py`:
- Around line 120-122: The return statement on line 122 is unreachable code
since the function always returns on line 120 with the error response.
Additionally, the entire pam-verify.py file appears to be dead code with no
references in the codebase and its functionality has been moved to
pam-server.py. Either remove the unreachable return statement on line 122, or
better yet, remove the entire pam-verify.py file from the repository if there
are no active dependencies on it. If the file needs to be retained for
historical or legacy reasons, document its status clearly in a comment
explaining why it is no longer actively used.
---
Duplicate comments:
In `@apps/api/src/utils/auth.ts`:
- Line 116: In the cleanupExpiredTokens function, the DELETE statement compares
expires_at (stored in ISO 8601 format with T separator) directly against
datetime('now') (YYYY-MM-DD HH:MM:SS format), causing a lexical mismatch where
tokens with T character sort after the space character, preventing proper
cleanup. Wrap the expires_at column with the datetime() function in the WHERE
clause condition to convert it to the same format as datetime('now'), matching
the fix that was already applied to validateRefreshToken for consistency.
---
Nitpick comments:
In `@docker-compose.yml`:
- Around line 76-80: The docker-compose.yml command block is installing
libpam-modules at container startup using apt-get, which creates a fragile
runtime dependency on apt mirrors and slows boot times. Instead, create a
dedicated Dockerfile based on python:3-slim that pre-installs libpam-modules and
sets up the pam-server.py runtime during image build time. Then update the
docker-compose.yml to build and use this custom image instead of the inline
apt-get installation command, ensuring the sidecar starts deterministically
without network dependencies.
- Around line 53-55: The api service in docker-compose.yml currently only
declares a dependency on buildkit but does not wait for pam-auth to be healthy
before starting, even though api calls pam-auth on port 4567 for login
functionality. Add pam-auth to the depends_on section of the api service with a
condition of service_healthy (since pam-auth already defines a healthcheck) to
ensure the authentication service is ready before the api service starts
accepting login requests.
In `@scripts/auth/pam-server.py`:
- Around line 160-163: The main() function currently uses HTTPServer which
processes requests serially, causing slow PAM authentication calls to block
other login attempts and healthchecks. Replace HTTPServer with
ThreadingHTTPServer (from the http.server module) to enable concurrent request
handling. Ensure ThreadingHTTPServer is imported at the top of the file if it is
not already imported, then update the server instantiation in main() to use
ThreadingHTTPServer instead of HTTPServer while keeping the same arguments.
🪄 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
Run ID: c1f733e0-1939-4957-8441-730f01407f71
📒 Files selected for processing (8)
.dockerignoreapps/api/src/api/auth/index.tsapps/api/src/db/__tests__/auth.test.tsapps/api/src/utils/__tests__/auth.test.tsapps/api/src/utils/auth.tsdocker-compose.ymlscripts/auth/pam-server.pyscripts/auth/pam-verify.py
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/db/tests/auth.test.ts
- apps/api/src/utils/tests/auth.test.ts
Comment on lines
+120
to
+122
| return {"ok": False, "error": f"User '{username}' is not in the '{GRP_NAME}' group"} | ||
|
|
||
| return {"ok": True, "username": username} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any references to pam-verify(.py) across the repo
rg -n --hidden -g '!**/.git/**' 'pam-verify'Repository: Lftobs/dequel
Length of output: 151
🏁 Script executed:
#!/bin/bash
# Check if the file exists and read the relevant lines
if [ -f "scripts/auth/pam-verify.py" ]; then
echo "=== File exists, reading lines 115-125 ==="
sed -n '115,125p' scripts/auth/pam-verify.py | cat -n
else
echo "File not found at scripts/auth/pam-verify.py"
fi
# List files in the scripts/auth directory
echo ""
echo "=== Files in scripts/auth/ ==="
ls -la scripts/auth/ 2>/dev/null || echo "Directory not found"
# Search more broadly for pam references
echo ""
echo "=== All pam references ==="
rg -n 'pam' --type py | head -30Repository: Lftobs/dequel
Length of output: 947
🏁 Script executed:
#!/bin/bash
# Check if pam-verify is referenced in shell scripts, documentation, or config files
echo "=== References in shell scripts ==="
rg 'pam-verify' --type sh 2>/dev/null || echo "No references in shell scripts"
echo ""
echo "=== References in documentation/config ==="
rg 'pam-verify' --type md --type txt --type yaml --type json 2>/dev/null || echo "No references in docs/config"
echo ""
echo "=== All file references to pam-verify (any file type) ==="
rg 'pam-verify' 2>/dev/null || echo "No references found anywhere"
echo ""
echo "=== Check imports/usage of pam in pam-verify.py ==="
head -60 scripts/auth/pam-verify.py | cat -nRepository: Lftobs/dequel
Length of output: 535
🏁 Script executed:
#!/bin/bash
# The previous output was cut off, let me read the full log
cat /tmp/coderabbit-shell-logs/shell-output-IthsrgRepository: Lftobs/dequel
Length of output: 535
🏁 Script executed:
#!/bin/bash
# Read the full output from the log
tail -100 /tmp/coderabbit-shell-logs/shell-output-qMwBm5Repository: Lftobs/dequel
Length of output: 535
🏁 Script executed:
#!/bin/bash
# Read the pam-verify.py file directly
echo "=== Full pam-verify.py file ==="
cat scripts/auth/pam-verify.py
echo ""
echo ""
echo "=== Search for any pam-verify references (exit code will indicate if found) ==="
if grep -r 'pam-verify' . --exclude-dir=.git 2>/dev/null; then
echo "Found references to pam-verify"
else
echo "No references to pam-verify found (exit code: $?)"
fiRepository: Lftobs/dequel
Length of output: 4897
Remove unused dead code or clarify if pam-verify.py is still needed.
Line 122 is unreachable—the function always returns on line 120. Additionally, no references to pam-verify.py exist anywhere in the codebase, making this file dead code that should be removed or its continued use should be documented, especially since PAM verification moved to the pam-server.py HTTP sidecar.
🤖 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 `@scripts/auth/pam-verify.py` around lines 120 - 122, The return statement on
line 122 is unreachable code since the function always returns on line 120 with
the error response. Additionally, the entire pam-verify.py file appears to be
dead code with no references in the codebase and its functionality has been
moved to pam-server.py. Either remove the unreachable return statement on line
122, or better yet, remove the entire pam-verify.py file from the repository if
there are no active dependencies on it. If the file needs to be retained for
historical or legacy reasons, document its status clearly in a comment
explaining why it is no longer actively used.
- Add AbortSignal.timeout(5000) to PAM authentication request in apps/api/src/api/auth/index.ts - Fetch projects only when authenticated and not on /login in apps/web/src/components/Layout.tsx - Update useProjects to accept options and pass through to useQuery in apps/web/src/hooks/useProjects.ts - Harden PAM Python scripts by loading libc via find_library and setting malloc/strdup restype in scripts/auth/pam-server.py - Symmetrically update pam-verify.py to use find_library for libc and set malloc/strdup restype - Remove premature success return from pam-verify.py to fix flow
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.
Description
Fixes #(issue)
Type of Change
How Has This Been Tested?
bun testinapps/api/)Checklist
bun testinapps/api/and all tests passbun run sync-versions)Summary by CodeRabbit
New Features
/auth/login,/auth/logout,/auth/refresh, and/auth/me/loginpage with automatic redirect behavior and gated access to app data/server/ipDocumentation
Chores