Resolve remaining security scan comments#350
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements security and caching improvements across several areas. It disables public CDN caching for host-sensitive map and calendar routes to prevent leaking admin data and adds a corresponding unit test. The YNAB transaction sync API now includes category ID deduplication, trimming, and a limit of 25 categories per request to prevent excessive fan-out. Additionally, URL safety checks were integrated into the map and embed pages for social media and blog links. I have no feedback to provide as there were no review comments.
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 50 minutes and 19 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR hardens security and safety across multiple layers: it enforces no-store caching for admin-sensitive routes, validates external post URLs before rendering links, enforces input limits on YNAB transaction categories, and simplifies logging messages. ChangesSecurity & Safety Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Confidence Score: 5/5Safe to merge — all changes are targeted security hardening with no logic regressions. Every changed path has direct test coverage: cache header tests exercise the next.config rules, the YNAB boundary tests verify the cap, dedup, and no-store header on error responses, and the URL validation utility is straightforward with no side effects. The normalization logic in the transactions route correctly trims and deduplicates category IDs before the YNAB call, and the lookup map keys match what YNAB returns. No regressions are introduced. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "Resolve remaining security scan comments" | Re-trigger Greptile |
bdamokos
force-pushed
the
codex/resolve-security-comments-2026-05-23
branch
from
f19c7e1 to
a5b59f3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/app/__tests__/api/ynab-token-boundary.test.ts (1)
248-280: ⚡ Quick winAdd one regression case for whitespace-normalized category IDs.
This test verifies de-duplication, but not trim normalization. Add one mapping like
' cat-1 'and assertgetTransactionsByCategoriesreceives'cat-1'to lock in normalization behavior.🤖 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 `@src/app/__tests__/api/ynab-token-boundary.test.ts` around lines 248 - 280, Add a regression case to ensure whitespace-normalized category IDs are deduped: in the test for ynabTransactionsPOST add a categoryMappings entry with a padded ID like ' cat-1 ' (alongside the existing 'cat-1') and keep the other mappings; then assert mockGetTransactionsByCategories is called with the trimmed ID array ['cat-1','cat-3'] so the test verifies trimming + dedup behavior for categoryMappings before calling getTransactionsByCategories.
🤖 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 `@src/app/api/ynab/transactions/route.ts`:
- Around line 61-62: The POST route's error responses are inconsistent: include
PRIVATE_JSON_HEADERS on every error NextResponse.json call in the POST handler
(not just the two 400 returns). Update the error paths that currently return
401, 404, 429, 500 (and the earlier 79-83 error returns) to pass
PRIVATE_JSON_HEADERS in the response options so all admin-sensitive error
responses use the same private/no-store headers; look for NextResponse.json(...)
calls inside the POST handler in route.ts and add the headers argument ({
headers: PRIVATE_JSON_HEADERS }) to each error response.
- Around line 66-77: The category normalization is done into mappedCategoryIds
but the later categoryMappingLookup still reads raw mapping.ynabCategoryId and
mappingType, which breaks lookups for whitespace-padded IDs and allows invalid
mappingType values; update the code that builds categoryMappingLookup to first
validate and normalize each CategoryMapping (check typeof mapping === 'object',
typeof mapping.ynabCategoryId === 'string' and typeof mapping.mappingType ===
'string' and mapping.mappingType !== 'none'), trim and reject empty
ynabCategoryId, then use the trimmed ID as the key when constructing
categoryMappingLookup so lookups match mappedCategoryIds and malformed mappings
are excluded.
---
Nitpick comments:
In `@src/app/__tests__/api/ynab-token-boundary.test.ts`:
- Around line 248-280: Add a regression case to ensure whitespace-normalized
category IDs are deduped: in the test for ynabTransactionsPOST add a
categoryMappings entry with a padded ID like ' cat-1 ' (alongside the existing
'cat-1') and keep the other mappings; then assert
mockGetTransactionsByCategories is called with the trimmed ID array
['cat-1','cat-3'] so the test verifies trimming + dedup behavior for
categoryMappings before calling getTransactionsByCategories.
🪄 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: c5703c9b-434d-4564-ba00-090d8b8d8534
📒 Files selected for processing (8)
AGENTS.mdnext.config.jssrc/app/__tests__/api/ynab-token-boundary.test.tssrc/app/__tests__/unit/nextConfigCache.test.tssrc/app/api/ynab/transactions/route.tssrc/app/embed/[id]/page.tsxsrc/app/lib/unifiedDataService.tssrc/app/map/[id]/page.tsx
bdamokos
force-pushed
the
codex/resolve-security-comments-2026-05-23
branch
from
a5b59f3 to
fc5984c
Compare
1 participant
Summary
Cache-Control: no-storeso admin/planning data is not eligible for shared CDN caching.Validation
bun run test:unit -- src/app/__tests__/api/ynab-token-boundary.test.ts src/app/__tests__/unit/nextConfigCache.test.ts src/app/__tests__/lib/serverPrivacyUtils.test.ts --modulePathIgnorePatterns="<rootDir>/.worktrees" --modulePathIgnorePatterns="<rootDir>/.next"bun run lint(passes with existing warnings)bun run buildSummary by CodeRabbit
New Features
Improvements
Tests