fix: 14 bug-scan findings across persistence, engine, and API routes - #55
Merged
Conversation
partialize only whitelisted `state.state`, so every field a feature slice adds at the store's top level (profiles, drills, trainer notes, certification checklist, missions, reports, bulk, integrity) was silently dropped from localStorage on every reload — a trainee's progress vanished the moment they refreshed the page. Persist every non-function field instead of a hardcoded key list, so the next feature slice added to the store doesn't quietly repeat this bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
Every target, negative, productAd, and ad was hardcoded to the first new ad group's id (newAgId) regardless of which ad group it originally belonged to. Duplicating a campaign with multiple ad groups silently merged everything into one, leaving the other ad groups empty. Build a map from old ad group id to new ad group id and use it to re-attach each item to its own ad group instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
Deleting the only remaining profile reset activeProfileId to the literal 'p-default' without reinserting a default profile into the roster, leaving profiles: [] and activeProfileId pointing at nothing — any selector doing profiles.find(p => p.id === activeProfileId) would come back undefined. Reseed defaultProfile() when the roster would otherwise go empty. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
PUT and DELETE checked ownership via a separate findFirst({ id,
userId }) query, but the actual update/delete calls used where: { id
} alone. Not exploitable today since the check and mutation run in
the same request, but any future refactor that separates them (a
queued job, a transfer feature, reordered code) could silently
reintroduce cross-user access with no compiler or test signal.
Prisma's extended where-unique-input lets id and userId be combined
directly on the mutation, so the mutation itself now proves
ownership instead of relying entirely on the preceding check.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
- setTargetBid (and adjustTargetBid, which calls it) never validated newBid, unlike sibling addTarget — a NaN or negative bid was silently clamped via Math.max instead of throwing ValidationError, contradicting the codebase's fail-fast convention. Added the same assertFiniteNonNegative guard addTarget already uses. - pauseTarget didn't check whether targetId matched an existing target before proceeding, unlike removeTarget/adjustTargetBid/ setTargetStatus — an unknown id pushed a blank string into history and returned a new object reference for what should have been a no-op. Now early-returns the campaign unchanged, matching its sibling functions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
updateCampaignSettings never validated dailyBudget/defaultBid (unlike normalizeCampaign, which clamps them), so a negative budget or a NaN bid was written straight into the campaign and even rendered into history text like "$-50.00". The store slice compounded this by typing `updates` as Record<string, unknown>, discarding the compile- time protection the function's own signature already provided. Added assertFiniteNonNegative checks matching the rest of the engine's fail-fast convention, and tightened the slice's type to the function's actual Partial<Pick<Campaign, ...>> signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
…ndary Both actions called an engine function that throws ValidationError on blank input (assertNonEmpty) from inside a Zustand set() updater, where the exception propagates uncaught — a blank note or profile rename would crash the calling event handler instead of failing gracefully. The engine functions are correct to throw (fail-fast is the intended contract there); the gap was the store not guarding the boundary before calling them, unlike the equivalent UI-level checks elsewhere in the app. Added the same non-empty guard at the store action level so blank input is a no-op regardless of caller. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
Unlike the analogous startDrill (which falls back to createSession() for an unknown id), startMission built a session for any id with no existence check. Since completeMissionStep looks the mission back up by id on every call, an unresolvable id left the session permanently stuck at step 0 with no error — indistinguishable from a UI freeze. Mirrors startDrill's pattern: fall back to an empty/idle session instead of a session that can never resolve. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
Only 'campaign'/'adGroup'/'target' were implemented; the two other valid, user-selectable ReportTypes silently returned rows: [] with a status of 'completed' and no error — requesting a searchTerm or placement report produced an apparently successful but empty export. ReportRow has no per-type schema, so the existing simulated-row generator now covers every type in REPORT_TYPES instead of a hardcoded subset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
The exclusion condition was `st.orders >= minOrders && st.sales > 0` with a default minOrders of 0. Since orders can never be negative, `st.orders >= 0` is always true, so the condition collapsed to `st.sales > 0` — any search term with even $0.01 of sales was excluded before its ACOS was ever checked, hiding genuinely catastrophic-ACOS terms from negation. Mirrors the sibling getHarvestCandidates function: a single `st.orders >= minOrders` gate with minOrders defaulting to 1 (already converted at least once → protected from blanket negation; everything else proceeds to the ACOS check). Also adds the test coverage this function had none of. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
GET /api/campaigns, GET /api/campaigns/[id], and GET /api/sync all called JSON.parse directly on 10 stored JSON columns with no try/catch, while the sync write path is careful to validate input. A single row with a corrupted or truncated JSON value (from a prior bug, a direct DB edit, or column truncation) threw an uncaught SyntaxError, 500ing the entire list rather than just that row. Added a shared safeJsonParse helper that falls back to the same empty defaults already used for null columns, so one bad row no longer takes down every other campaign in the response. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
Registration accepted any non-empty password (a single character was enough) and any string as an "email" with no format check. Added a basic email-format check and an 8-character minimum password length. Left the user-enumeration behavior (differing response for an existing email) unchanged for now — fixing that properly means changing the registration UX to not confirm success/failure by status code, which is a larger behavioral change than this pass, and lower real-world severity for an offline training simulator with no real user data at stake. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
…trics No validation existed on days — a negative value flowed unclamped into spend (Math.min picked the more-negative term), decreasing a campaign's cumulative spend/impressions/clicks below their prior values with a nonsensical history entry. Not reachable through the current UI (every call site uses the days=7 default), but it's a public engine function with no guard, unlike the rest of the engine's fail-fast convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
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 |
Contributor
|
Contributor
Analysis CompleteGenerated ECC bundle from 13 commits | Confidence: 65% View Pull Request #56Repository Profile
Changed Files (26)
Top hotspots
Top directories
Analysis Depth Readiness (commit-history, 14%)ECC Tools uses this to decide whether recommendations should stay at commit-history/setup guidance or expand into CI, security, harness, reference-set, AI-routing, and team backlog work.
Reference Set Readiness (0/7, 0%)
Likely Future Issues (4)
Suggested Follow-up Work (4)
Copy-ready bodies test: add integration coverage for src/app/api/auth/register/tests/route.test.ts + src/app/api/auth/register/route.ts ## Summary
- Add integration or end-to-end coverage for the recently changed API surface.
## Why
- Backfill integration or end-to-end coverage for the changed API surface before more contract changes land.
## Touched paths
- `src/app/api/auth/register/__tests__/route.test.ts`
- `src/app/api/auth/register/route.ts`
## Validation
- Add or extend integration / e2e coverage for the changed API, route, or contract surface.
- Exercise the touched endpoints or route handlers against realistic request / response flows.docs: sync API contract for src/app/api/auth/register/tests/route.test.ts + src/app/api/auth/register/route.ts ## Summary
- Update the API contract artifact that should reflect the recently changed implementation surface.
## Why
- Backfill the missing API contract or spec update before another implementation change lands on top of the same surface.
## Touched paths
- `src/app/api/auth/register/__tests__/route.test.ts`
- `src/app/api/auth/register/route.ts`
## Validation
- Update the relevant OpenAPI, GraphQL, or contract/spec artifact used by this repo.
- Run the contract validation, docs generation, or API verification flow that depends on that artifact.test: add auth coverage for src/app/api/auth/register/tests/route.test.ts + src/app/api/auth/register/route.ts ## Summary
- Add auth, session, or permission regression coverage for the recently changed security-sensitive surface.
## Why
- Backfill auth or permission regression coverage before another access-control change lands on the touched surface.
## Touched paths
- `src/app/api/auth/register/__tests__/route.test.ts`
- `src/app/api/auth/register/route.ts`
## Validation
- Add or extend integration / e2e coverage for the changed auth, session, middleware, or permission surface.
- Exercise allowed and denied flows, invalid or expired credentials, or equivalent access-control boundary cases.security: add scanner evidence for src/app/api/auth/register/tests/route.test.ts + src/app/api/auth/register/route.ts ## Summary
- Add security scanner or code-scanning evidence for the recently changed security-sensitive surface.
## Why
- Backfill explicit scanner or code-scanning evidence before another security-sensitive change lands on the touched surface.
## Touched paths
- `src/app/api/auth/register/__tests__/route.test.ts`
- `src/app/api/auth/register/route.ts`
## Validation
- Run or add the relevant security scanner, code scanning, secret scanning, or dependency/security review check for the touched surface.
- Attach the scanner output, SARIF/code-scanning result, or focused security regression test to the follow-up PR.
- Confirm the changed auth, billing, webhook, secret-handling, agent, or CI surface has an explicit pass/fail gate.Detected Workflows (2)
Generated Instincts (25)
After merging, import with: Files
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a set of bug-scan findings across the ad-console persistence layer, core engine logic, and API routes to prevent data loss/corruption, enforce fail-fast validation, and harden multi-user access boundaries.
Changes:
- Fix Zustand persistence to avoid silently dropping feature-slice state on reload by persisting all non-function fields (with regression tests).
- Correct/validate core engine behaviors (campaign duplication across ad groups, bid/days/budget validation, report generation coverage, mission/profile edge cases) with targeted unit tests.
- Harden API routes by safely parsing stored JSON columns and scoping campaign mutations by
userId, plus add basic registration input validation with tests.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/json.ts | Adds safeJsonParse helper to prevent corrupted JSON from crashing API responses. |
| src/lib/tests/json.test.ts | Unit tests for safeJsonParse behavior (valid/invalid/empty inputs). |
| src/engine/ad-console/store.ts | Fixes persistence partialize to include all non-function state (prevents feature slice loss). |
| src/engine/ad-console/features/trainer/store.ts | Guards blank note submissions to prevent uncaught validation errors in Zustand updater. |
| src/engine/ad-console/features/reports/engine.ts | Ensures all report types generate rows (including searchTerm/placement). |
| src/engine/ad-console/features/reports/tests/engine.test.ts | Adds regression tests for searchTerm and placement report generation. |
| src/engine/ad-console/features/profiles/store.ts | Prevents blank rename crash; reseeds default profile when deleting the last profile. |
| src/engine/ad-console/features/missions/engine.ts | Validates mission id existence by falling back to an empty session for unknown ids. |
| src/engine/ad-console/features/missions/tests/engine.test.ts | Adds regression test for unknown mission id fallback behavior. |
| src/engine/ad-console/core/slices/core.ts | Tightens updateCampaignSettings typing to preserve compile-time safety for allowed fields. |
| src/engine/ad-console/core/simulation.ts | Adds fail-fast validation for days to prevent negative/NaN simulation inputs. |
| src/engine/ad-console/core/engine/target.ts | Validates bid inputs; makes pauseTarget a no-op for unknown ids (prevents blank history). |
| src/engine/ad-console/core/engine/negative.ts | Fixes negative-candidate filtering defaults/logic so ACOS checks are reachable. |
| src/engine/ad-console/core/engine/campaign.ts | Fixes duplicateCampaign to preserve per-ad-group relationships; validates campaign setting updates. |
| src/engine/ad-console/core/tests/simulation.test.ts | Regression tests for invalid days values. |
| src/engine/ad-console/core/tests/engine.test.ts | Regression tests for multi-ad-group duplication, bid validation, and pauseTarget unknown-id behavior. |
| src/engine/ad-console/core/tests/campaignGoal.test.ts | Adds tests for negative/harvest candidate selection and campaign settings validation. |
| src/engine/ad-console/tests/persistence.test.ts | Regression tests ensuring partialize includes feature slice state and excludes functions. |
| src/engine/ad-console/tests/feature-stores.test.ts | Regression tests for profile reseeding and no-op behaviors on blank inputs. |
| src/app/api/sync/route.ts | Uses safeJsonParse to avoid 500s when a single row has corrupted JSON fields. |
| src/app/api/sync/tests/route.test.ts | Test ensuring corrupted JSON in one row doesn’t break the entire sync list response. |
| src/app/api/campaigns/route.ts | Uses safeJsonParse when listing campaigns to prevent list-wide failures on corrupted JSON. |
| src/app/api/campaigns/[id]/route.ts | Uses safeJsonParse for single campaign fetch; scopes PUT/DELETE mutations by userId. |
| src/app/api/campaigns/[id]/tests/route.test.ts | Tests mutation scoping for PUT/DELETE to defend against cross-user updates by id. |
| src/app/api/auth/register/route.ts | Adds basic email format and password length validation to registration. |
| src/app/api/auth/register/tests/route.test.ts | Tests register-route input validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
152
to
+156
| bidStrategy: c.bidStrategy, | ||
| placements: c.placements ? JSON.parse(c.placements) : { top: 0, product: 0, rest: 0 }, | ||
| products: c.products ? JSON.parse(c.products) : [], | ||
| creative: c.creative ? JSON.parse(c.creative) : null, | ||
| metrics: c.metrics ? JSON.parse(c.metrics) : { impressions: 0, clicks: 0, spend: 0, sales: 0, orders: 0 }, | ||
| adGroups: c.adGroups ? JSON.parse(c.adGroups) : [], | ||
| targets: c.targets ? JSON.parse(c.targets) : [], | ||
| searchTerms: c.searchTerms ? JSON.parse(c.searchTerms) : [], | ||
| negatives: c.negatives ? JSON.parse(c.negatives) : [], | ||
| budgetRules: c.budgetRules ? JSON.parse(c.budgetRules) : [], | ||
| history: c.history ? JSON.parse(c.history) : [], | ||
| placements: safeJsonParse(c.placements, { top: 0, product: 0, rest: 0 }), | ||
| products: safeJsonParse(c.products, []), | ||
| creative: safeJsonParse(c.creative, null), | ||
| metrics: safeJsonParse(c.metrics, { impressions: 0, clicks: 0, spend: 0, sales: 0, orders: 0 }), |
Comment on lines
+16
to
+24
| if (typeof email !== 'string' || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email)) { | ||
| return NextResponse.json( | ||
| { error: 'Enter a valid email address' }, | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| if (typeof password !== 'string' || password.length < 8) { | ||
| return NextResponse.json( |
Comment on lines
+21
to
+25
| placements: safeJsonParse(c.placements, null), | ||
| products: safeJsonParse(c.products, []), | ||
| creative: safeJsonParse(c.creative, null), | ||
| metrics: safeJsonParse(c.metrics, null), | ||
| adGroups: safeJsonParse(c.adGroups, []), |
Comment on lines
+31
to
+35
| placements: safeJsonParse(campaign.placements, null), | ||
| products: safeJsonParse(campaign.products, []), | ||
| creative: safeJsonParse(campaign.creative, null), | ||
| metrics: safeJsonParse(campaign.metrics, null), | ||
| adGroups: safeJsonParse(campaign.adGroups, []), |
3 tasks
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
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.
Summary
13 atomic fixes for issues surfaced by a code-review pass over the engine, store, and API layers. Each commit is independently tested and scoped to one bug.
partializeonly persistedstate.state, silently dropping every feature slice (profiles, drills, trainer, missions, reports, bulk, integrity) on reload — trainee progress vanished on every refresh. Now persists everything non-function, so future slices don't repeat the bug.duplicateCampaign: hardcoded all targets/negatives/ads to the first new ad group regardless of origin, silently losing data on multi-ad-group campaigns.PUT/DELETEverified ownership via a separate query but mutated by bareid— now scoped byuserIdon the mutation itself (defense-in-depth against future refactors).deleteProfile: could leaveactiveProfileIdpointing at nothing after deleting the only profile.setTargetBid/adjustTargetBid: silently acceptedNaN/negative bids instead of failing fast, unlike sibling engine functions.pauseTarget: pushed a blank history entry for an unknown target id instead of no-op'ing.updateCampaignSettings: no validation ondailyBudget/defaultBid, and the store slice typedupdatesasRecord<string, unknown>, discarding compile-time safety.addNote/renameProfile: threw uncaughtValidationErrorfrom inside a Zustandset()updater on blank input, crashing the caller.startMission: didn't validate the mission id exists, unlike the analogousstartDrill.generateReport: silently produced 0 rows for the validsearchTerm/placementreport types.getNegativeCandidates: its defaultminOrders: 0made the ACOS check unreachable for any term with sales.JSON.parseon stored campaign columns meant one corrupted row 500'd the entire list.simulateDays: no validation ondays, allowing negative values to corrupt cumulative metrics (not reachable via current UI, but a latent API gap).Two related findings were deliberately not fixed here (called out in the corresponding commit messages): missions' lack of a real action-correctness check (would require wiring action-tracking into every mission-relevant UI event, a feature-sized change) and registration's email-enumeration behavior (a UX-contract change, lower severity for an offline training simulator).
Test plan
npm run type-check— cleannpm test— 624/624 passing (new tests added for every fix)npm run build— succeedsGenerated by Claude Code