Fix 11 bugs: security, logic, error handling, and code quality#41
Fix 11 bugs: security, logic, error handling, and code quality#41devin-ai-integration[bot] wants to merge 1 commit intomainfrom
Conversation
Security fixes: - Add auth check and user_id filter to deleteTailoredJob (was completely unauthenticated) - Add user_id filter to deleteJob delete/select queries (allowed cross-user deletion) Functional fixes: - Remove stub subscription actions that silently returned undefined - Fix countResumes returning -1 instead of 0 (|| vs ?? operator) - Add error handling to cover letter streaming IIFE (prevents hanging streams) - Fix getSubscriptionStatus returning 'Free' (capitalized) instead of 'free' - Fix testApiKey using OpenRouter model ID with OpenAI SDK (always failed) - Add job deletion to deleteUserAccount (left orphaned records) - Handle failed code exchange in auth callback (was silently ignored) Code quality: - Remove debug console.log statements leaking user IDs - Remove unnecessary void expressions - Clean up unused MODEL_DESIGNATIONS import Co-Authored-By: Alex Olyaiy <alexolyaiy@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
JiwaniZakir
left a comment
There was a problem hiding this comment.
The hardcoded model string 'gpt-5-mini-2025-08-07' in settings/actions.ts is a regression — the MODEL_DESIGNATIONS constant was removed specifically to introduce a magic string, which defeats the purpose of maintaining a central constants file. If this model name needs to change or is wrong (and gpt-5-mini-2025-08-07 doesn't match any currently known OpenAI model identifier), it now has to be tracked down and updated in-place rather than changed in one location.
The countResumes fix in resumes/actions.ts from count || -1 to count ?? 0 is a meaningful correctness fix — the original would return -1 when count is 0 (a valid result), silently misreporting an empty result as an error sentinel.
In jobs/actions.ts, deleteTailoredJob now correctly gates on user_id, but deleteJob still fetches affectedResumes without a corresponding ownership check on the resume update step — only the final delete call has the user_id guard added. If there's downstream logic that acts on affectedResumes, it could still operate on resumes belonging to other users if the RLS policies don't cover it.
The cover-letter/actions.ts change wraps the async IIFE in a try/catch and propagates errors via stream.error(...), but it also removes the onFinish usage tracking callback without any replacement, which may affect observability for token usage monitoring.
Summary
Fixes 11 bugs found during a systematic code review, spanning security issues, logic errors, missing error handling, and code quality problems.
Security (authorization gaps in job mutations):
deleteTailoredJobhad no authentication or ownership check — addedgetUser()call anduser_idfilterdeleteJobauthenticated the user but did not filter queries byuser_id— addeduser_idfilter to both the affected-resumes select and the delete queryLogic / functional fixes:
countResumesreturned-1when count was0due to||vs??— changed tocount ?? 0getSubscriptionStatusreturned'Free'(capitalized) in the no-subscription fallback, while the rest of the codebase uses lowercase'free'testApiKeypassed an OpenRouter model ID (deepseek/deepseek-v3.2:nitro) to the OpenAI SDK — replaced withgpt-5-mini-2025-08-07and reducedmax_tokensto 256 for a simple connectivity testsubscriptions/actions.tsthat silently returnedundefinedError handling:
stream.done()never called, leaving clients hanging. Added try/catch withstream.error()exchangeCodeForSession— now redirects to/auth/login?error=auth_callback_failedData integrity:
deleteUserAccountdeleted profiles and resumes but left orphaned job records — added job deletion stepCode quality:
console.logstatements that leaked user IDs and subscription details to server logsvoidexpressions used to suppress lint warningsMODEL_DESIGNATIONSimportReview & Testing Checklist for Human
deleteUserAccountdeletion order doesn't violate FK constraints — jobs are now deleted between profiles and resumes. If there's a foreign key fromresumes.job_id → jobs.id, deleting jobs first could fail. Check your DB schema for CASCADE/SET NULL behavior on that FK.stream.error()is a valid method oncreateStreamableValue— used in the cover letter error handler. If this method doesn't exist in your version ofai/rsc, the catch block would itself throw.?error=auth_callback_failed— the auth callback now redirects there on failure. If the login page doesn't read this param, users will see the login form with no error message (still better than silent redirect, but not ideal).jobstable — if RLS policies filter byauth.uid(), the security bugs indeleteJob/deleteTailoredJobmay not have been exploitable in practice, though the server-side checks are still good defense-in-depth.testApiKeyfunction with a real OpenAI API key to confirmgpt-5-mini-2025-08-07is a valid model ID for your OpenAI account.Recommended test plan: Exercise the core flows that touch these changes — delete a job, delete a tailored job, generate a cover letter (and interrupt it mid-stream), delete a user account, trigger the auth callback with an invalid code, and call
testApiKeyfrom settings.Notes
testApiKeymodel is now hardcoded rather than using a constant. This was intentional sinceMODEL_DESIGNATIONSdidn't have a suitable OpenAI-native model designation.countResumesfallback changed from-1(null sentinel) to0— this is arguably more correct but is a subtle semantic change.Link to Devin session: https://app.devin.ai/sessions/df6a7139bf3f4945b2043bf89ab4df2e
Requested by: @olyaiy