Builder MP port#23
Conversation
- Remove unauthenticated /bills/api/auth/debug route that leaked NEXTAUTH_URL and secret-presence to anonymous callers - Require isAdmin on /bills/api/users POST so allowlisted users can no longer self-expand the allowlist (privilege escalation); add isAdmin field to the User model - Gate DEV_OPEN_ACCESS on an explicit BILLS_DEV_OPEN_ACCESS=true opt-in in addition to non-production NODE_ENV, so open access can never be enabled by accident; share the flag via env.ts and use it for sign-in auto-allow as well Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clear all 42 ESLint errors flagged on the PR: - Replace no-explicit-any casts with proper types (auth, API routes, services, utils); add image field to the User model so auth no longer needs any-casts (also fixes the field being silently dropped on save) - Escape apostrophes in FAQ copy (react/no-unescaped-entities) - Satisfy React Compiler rules: move cache reset out of the Home component (react-hooks/globals), derive collapsed state from props instead of syncing via effect in FilterSection, and justify the remaining intentional setState-in-effect (BillShare) and Math.random skeleton width (sidebar) with scoped disables Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mikaalnaik
left a comment
mikaalnaik
left a comment
There was a problem hiding this comment.
Code review — Builder MP port
Reviewed at 0b69199. The three critical security issues flagged earlier are now resolved in this branch: the unauthenticated /bills/api/auth/debug leak is gone, /bills/api/users is admin-gated (isAdmin added to the User model), and DEV_OPEN_ACCESS now requires an explicit BILLS_DEV_OPEN_ACCESS=true opt-in so it can't be enabled by a misconfigured NODE_ENV. Lint is clean (0 errors). Nice.
What remains is 🟡 medium / cleanup — none blocking, but worth resolving before merge. Inline comments below.
Operational follow-up (no line to pin)
- First-admin bootstrap.
isAdminnow defaults tofalsefor everyone, so in production/bills/api/userswill 403 for all callers until an admin exists. You'll need a migration or one-off DB update to seed the first admin — otherwise the allowlist can never be managed via the API. - Unused dependencies.
@google/genaiand@google/generative-aiare both inpackage.jsonbut have no imports anywhere insrc/(the AI services use OpenAI). Two Google AI SDKs plus OpenAI is a lot of dependency surface — drop them unless they're about to be used. - Test coverage. No tests accompany the auth guard, the admin gate, or the non-trivial body-parsing in
api/[id]/route.ts(comma-split / question-grouping). Given the auth sensitivity, at minimum add a test asserting production mode denies non-allowlisted/non-admin users on each mutating route.
Quality (non-blocking)
- The three ~70-line fallback tenet blocks in
billApi.ts(no-key / parse-fail / error) are near-identical copy-paste — extract aFALLBACK_TENETSconstant +makeFallback(reason)helper. - LLM output is parsed with
JSON.parse+ regex fallbacks; since you're on the OpenAI SDK, prefer a structured-output /response_formatschema for robustness.
| _request: NextRequest, | ||
| context: { params: Promise<{ email: string }> }, | ||
| ) { | ||
| const session = { user: { email: null } }; |
There was a problem hiding this comment.
Dead / broken endpoint. session is hardcoded to { user: { email: null } }, so the guard on the next line always returns 401 — this route can never do anything. It fails closed (safe), but it's misleading dead code.
Either delete the route, or implement it properly with getServerSession(authOptions) and an isAdmin check (granting allowlist access is the same privilege-escalation surface we just locked down on /api/users).
|
|
||
| export async function POST(_request: Request) { | ||
| // if (env.NODE_ENV !== "development") { | ||
| return NextResponse.json({ error: "Not found" }, { status: 404 }); |
There was a problem hiding this comment.
This endpoint is stubbed to unconditionally return 404 with ~70 lines of commented-out implementation above/below. Please remove the dead code (and the file if the route isn't needed) before merge — commented-out blocks rot quickly and obscure the real behavior.
| export default function DevUsersPage() { | ||
| return ( | ||
| <div> | ||
| howdy |
There was a problem hiding this comment.
Placeholder page renders literally howdy with the real form commented out. Remove it or finish it — it shouldn't ship as-is.
| billId: string, | ||
| ): Promise<ApiBillDetail | null> { | ||
| const URL = `${env.CIVICS_PROJECT_BASE_URL}/canada/bills/${CANADIAN_PARLIAMENT_NUMBER}/${billId}`; | ||
| console.log({URL}); |
There was a problem hiding this comment.
Leftover debug logging (console.log({URL})) — this runs on every Civics Project fetch and will spam production logs. Remove it (same for the console.log("Raw response:", responseText) further down, which also logs full AI output / bill text).
| cached.promise = mongoose.connect(MONGO_URI, { | ||
| dbName: DATABASE_NAME, | ||
| serverSelectionTimeoutMS: 3000, | ||
| socketTimeoutMS: 3000, |
There was a problem hiding this comment.
socketTimeoutMS: 3000 is aggressive: combined with bufferCommands: false, any single DB operation taking >3s (cold Atlas connection, a slower aggregate, the reprocess path that writes after a long AI call) will have its socket killed mid-flight and fail. Keep serverSelectionTimeoutMS short for fast failover, but consider raising the socket timeout (e.g. 30s+).
1 participant
No description provided.