Fix remaining security scan regressions#349
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR integrates validation, security, and error-handling improvements across the routing API, travel data, cost tracking, UI components, and weather service. A new proxy-secret validation mechanism gates OSRM forwarded-host access; route-points are normalized in batch and single updates; YNAB transaction dates are validated and invalid entries are skipped; components now properly clear state on errors; and weather caching is optimized to skip persisting empty results. ChangesValidation, Security, and Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/cost-tracking/[id]/ynab-process/route.ts (1)
158-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInvalid-date transactions consume legacy hash counts and are mislabeled as “already imported.”
Two related concerns in the preview loop:
consumeLegacyImportedHash(legacyImportedHashCounts, hash)runs at Line 161, before the date validation at Line 167. When a transaction is dropped because of an invalid date, its legacy-hash count has already been decremented. A later valid duplicate with the same hash could then escape legacy-duplicate detection (or, conversely, the invalid one "uses up" a slot it shouldn't).- Invalid-date transactions are bucketed into
alreadyImportedCount, which is surfaced to the UI as "already imported" in the response message at Line 251. That's misleading — these were skipped because of a parse failure, not because they were imported before.Consider moving date validation above the legacy-hash consumption and exposing a separate counter (e.g.
invalidDateCount) so the UI can distinguish the two cases.🤖 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/api/cost-tracking/`[id]/ynab-process/route.ts around lines 158 - 171, The loop currently calls consumeLegacyImportedHash(legacyImportedHashCounts, hash) and computes hasLegacyImportedHash before validating the transaction date, which causes invalid-date transactions to consume legacy hash counts and be miscounted as already imported; fix by first calling getValidYnabISODate(transaction.Date) and if the date is invalid increment a new invalidDateCount (do not modify legacyImportedHashCounts or existingHashSet), then only after the date is valid compute hash via createTransactionHash and check existingHashSet and call consumeLegacyImportedHash to detect legacy duplicates; ensure alreadyImportedCount remains for true duplicates and map lookup (mappings.find(...)) logic stays the same.
🧹 Nitpick comments (4)
src/app/__tests__/unit/weatherService.test.ts (1)
32-45: 💤 Low valueOptional: rename the “expires empty cache entries after the negative cache TTL” test.
Since
isFreshEmptyCacheEntrynow unconditionally returnsfalse, this test no longer exercises a TTL boundary — it’s asserting the same behavior as the “recent empty entries” test above. Consider renaming/merging it (e.g. “returns false regardless of fetchedAt age”) so the description matches the new semantics and doesn’t imply a TTL that no longer exists.🤖 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__/unit/weatherService.test.ts` around lines 32 - 45, The test titled "expires empty cache entries after the negative cache TTL" is now misleading because isFreshEmptyCacheEntry (in weatherServiceTestUtils) always returns false; rename the test to reflect the new semantics (e.g., "returns false regardless of fetchedAt age") or merge it with the "recent empty entries" test so the description matches what is being asserted and no TTL boundary is implied; update only the it(...) description string where that test is defined.src/app/services/weatherService.ts (1)
434-439: 💤 Low valueOptional: collapse the now-dead branches in
isFreshEmptyCacheEntryand its caller.With the function unconditionally returning
false, the length check on Line 436 and the entirecache:negative-hitbranch at Lines 774–780 (including thelog('cache:negative-hit', …)call) are unreachable. Consider either inlining thefalseresult at the call site and dropping the helper, or keeping the helper as a thin no-op with a comment so the intent (negative caching disabled) is explicit and the dead branches/log don’t mislead future readers.♻️ One possible cleanup
-function isFreshEmptyCacheEntry(cached: CacheEntry, now: Date): boolean { - void now; - if (cached.summary.dailyWeather.length > 0) return false; - - return false; -} +// Negative caching for empty weather lookups is disabled; kept exported +// for test compatibility. +function isFreshEmptyCacheEntry(_cached: CacheEntry, _now: Date): boolean { + return false; +}And in
getWeatherForLocation, theif (isFreshEmptyCacheEntry(cached, now)) { … 'cache:negative-hit' … }block can be removed.🤖 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/services/weatherService.ts` around lines 434 - 439, The helper isFreshEmptyCacheEntry currently always returns false, making its length check and the 'cache:negative-hit' branch in getWeatherForLocation unreachable; either remove the helper and delete the entire if block that logs 'cache:negative-hit' in getWeatherForLocation, or keep isFreshEmptyCacheEntry as an explicit no-op (return false) but add a clarifying comment and remove the dead log/branch (the cache:negative-hit check and log call) so readers aren't misled—locate the function isFreshEmptyCacheEntry and the caller in getWeatherForLocation to apply the change.src/app/__tests__/api/osrm-routing-validation.test.ts (1)
68-87: ⚡ Quick winConsider adding a test for missing/mismatched header when
ADMIN_PROXY_SECRETis configured.This test covers the case where
ADMIN_PROXY_SECRETis unset (helper returnsfalsevia the early return). The more realistic bypass-attempt scenario — operator has configured the secret, but the inbound request lacksx-travel-tracker-admin-proxy-secretor sends a wrong value — isn't currently asserted. Adding it would lock in the header comparison branch inhasTrustedForwardedHostSecret.🧪 Suggested additional test case
+ it('rejects forwarded admin hosts from local proxy hops with a missing or mismatched proxy secret', async () => { + process.env.ADMIN_DOMAIN = 'admin.example.test'; + process.env.ADMIN_PROXY_SECRET = 'proxy-secret'; + const fetchSpy = jest.spyOn(global, 'fetch'); + + const response = await GET( + new NextRequest( + 'http://127.0.0.1:3000/api/routing/osrm?profile=car&fromLat=51.5&fromLng=-0.1&toLat=48.8&toLng=2.3', + { + headers: { + host: '127.0.0.1:3000', + 'x-forwarded-host': 'admin.example.test', + 'x-travel-tracker-admin-proxy-secret': 'wrong-secret' + } + } + ) + ); + + await expect(response.json()).resolves.toEqual({ error: 'Admin domain required' }); + expect(response.status).toBe(403); + expect(fetchSpy).not.toHaveBeenCalled(); + });🤖 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/osrm-routing-validation.test.ts` around lines 68 - 87, Add a test that simulates the case where ADMIN_PROXY_SECRET is set but the incoming request either omits or supplies a wrong x-travel-tracker-admin-proxy-secret header so the hasTrustedForwardedHostSecret branch is exercised; in the new test (in src/app/__tests__/api/osrm-routing-validation.test.ts) set process.env.ADMIN_DOMAIN and process.env.ADMIN_PROXY_SECRET, call GET with x-forwarded-host set to the admin domain but either no x-travel-tracker-admin-proxy-secret header or a mismatched value, then assert the response is 403 with { error: 'Admin domain required' } and that global.fetch was not called to verify the header comparison branch in hasTrustedForwardedHostSecret is covered.src/app/api/cost-tracking/[id]/ynab-process/route.ts (1)
49-52: 💤 Low valueHelper can be simplified.
convertYnabDateToISOalready returns a falsy value on failure, soisoDate ? isoDate : nullis equivalent toconvertYnabDateToISO(ynabDate) || null. The wrapper currently only exists to coerceundefined/''tonull; a one-liner or inlining is fine.Proposed simplification
-function getValidYnabISODate(ynabDate: string): string | null { - const isoDate = convertYnabDateToISO(ynabDate); - return isoDate ? isoDate : null; -} +function getValidYnabISODate(ynabDate: string): string | null { + return convertYnabDateToISO(ynabDate) || null; +}🤖 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/api/cost-tracking/`[id]/ynab-process/route.ts around lines 49 - 52, The helper getValidYnabISODate is redundant because convertYnabDateToISO already yields a falsy value on failure; replace the body with a one-liner that returns convertYnabDateToISO(ynabDate) || null, or remove getValidYnabISODate and inline convertYnabDateToISO(ynabDate) || null wherever the helper is used so the call coerces undefined/empty to null directly; update all references to the helper (getValidYnabISODate) accordingly.
🤖 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/__tests__/integration/cost-tracking-validation.integration.test.ts`:
- Around line 184-185: The test creates an unused NextRequest instance in the
variable named request and then calls costTrackingListGET with a freshly
constructed NextRequest; either remove the unused const request declaration or
reuse it by passing request into costTrackingListGET instead of creating a
second NextRequest. Update the lines around the NextRequest construction so only
one NextRequest is created and used with costTrackingListGET to eliminate the
unused variable.
In `@src/app/admin/components/TravelItemSelector.tsx`:
- Line 349: The effect in TravelItemSelector is depending on the object-identity
of initialValue (a TravelLinkInfo) which causes repeated refetches when parents
pass inline object literals; change the dependency list to use stable primitives
instead (e.g. initialValue?.id and initialValue?.type) rather than initialValue
itself for the fetch effect (the one currently depending on [initialValue,
tripId, reloadToken]) and also update the hydration effect deps (the effect
around lines 180-185) to use the same stable fields so the effects only re-run
on meaningful changes.
---
Outside diff comments:
In `@src/app/api/cost-tracking/`[id]/ynab-process/route.ts:
- Around line 158-171: The loop currently calls
consumeLegacyImportedHash(legacyImportedHashCounts, hash) and computes
hasLegacyImportedHash before validating the transaction date, which causes
invalid-date transactions to consume legacy hash counts and be miscounted as
already imported; fix by first calling getValidYnabISODate(transaction.Date) and
if the date is invalid increment a new invalidDateCount (do not modify
legacyImportedHashCounts or existingHashSet), then only after the date is valid
compute hash via createTransactionHash and check existingHashSet and call
consumeLegacyImportedHash to detect legacy duplicates; ensure
alreadyImportedCount remains for true duplicates and map lookup
(mappings.find(...)) logic stays the same.
---
Nitpick comments:
In `@src/app/__tests__/api/osrm-routing-validation.test.ts`:
- Around line 68-87: Add a test that simulates the case where ADMIN_PROXY_SECRET
is set but the incoming request either omits or supplies a wrong
x-travel-tracker-admin-proxy-secret header so the hasTrustedForwardedHostSecret
branch is exercised; in the new test (in
src/app/__tests__/api/osrm-routing-validation.test.ts) set
process.env.ADMIN_DOMAIN and process.env.ADMIN_PROXY_SECRET, call GET with
x-forwarded-host set to the admin domain but either no
x-travel-tracker-admin-proxy-secret header or a mismatched value, then assert
the response is 403 with { error: 'Admin domain required' } and that
global.fetch was not called to verify the header comparison branch in
hasTrustedForwardedHostSecret is covered.
In `@src/app/__tests__/unit/weatherService.test.ts`:
- Around line 32-45: The test titled "expires empty cache entries after the
negative cache TTL" is now misleading because isFreshEmptyCacheEntry (in
weatherServiceTestUtils) always returns false; rename the test to reflect the
new semantics (e.g., "returns false regardless of fetchedAt age") or merge it
with the "recent empty entries" test so the description matches what is being
asserted and no TTL boundary is implied; update only the it(...) description
string where that test is defined.
In `@src/app/api/cost-tracking/`[id]/ynab-process/route.ts:
- Around line 49-52: The helper getValidYnabISODate is redundant because
convertYnabDateToISO already yields a falsy value on failure; replace the body
with a one-liner that returns convertYnabDateToISO(ynabDate) || null, or remove
getValidYnabISODate and inline convertYnabDateToISO(ynabDate) || null wherever
the helper is used so the call coerces undefined/empty to null directly; update
all references to the helper (getValidYnabISODate) accordingly.
In `@src/app/services/weatherService.ts`:
- Around line 434-439: The helper isFreshEmptyCacheEntry currently always
returns false, making its length check and the 'cache:negative-hit' branch in
getWeatherForLocation unreachable; either remove the helper and delete the
entire if block that logs 'cache:negative-hit' in getWeatherForLocation, or keep
isFreshEmptyCacheEntry as an explicit no-op (return false) but add a clarifying
comment and remove the dead log/branch (the cache:negative-hit check and log
call) so readers aren't misled—locate the function isFreshEmptyCacheEntry and
the caller in getWeatherForLocation to apply the change.
🪄 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: 606d0513-df23-48e9-bc9f-e1d59e8edc42
📒 Files selected for processing (12)
src/app/__tests__/api/osrm-routing-validation.test.tssrc/app/__tests__/api/travel-data-auth-cache.test.tssrc/app/__tests__/integration/cost-tracking-validation.integration.test.tssrc/app/__tests__/unit/MultiRouteLinkManager.test.tsxsrc/app/__tests__/unit/TravelItemSelector.loadFailure.test.tsxsrc/app/__tests__/unit/weatherService.test.tssrc/app/admin/components/MultiRouteLinkManager.tsxsrc/app/admin/components/TravelItemSelector.tsxsrc/app/api/cost-tracking/[id]/ynab-process/route.tssrc/app/api/routing/osrm/route.tssrc/app/api/travel-data/route.tssrc/app/services/weatherService.ts
💤 Files with no reviewable changes (1)
- src/app/admin/components/MultiRouteLinkManager.tsx
![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 enhances security and data integrity by introducing a proxy secret for OSRM routing, improving YNAB transaction import logic with date validation, and refining batch route updates. It also disables negative caching for empty weather results and improves error handling in the TravelItemSelector UI component. Review feedback identifies a misleading counter increment for invalid YNAB dates and warns against including initialValue in a useEffect dependency array to prevent redundant network requests.
Confidence Score: 5/5Safe to merge. All changes are targeted bug fixes with direct test coverage; no regressions identified in the changed paths. Every fix is well-scoped: the proxy secret check fails closed (no env var → no forwarded-host trust), date validation happens before hash computation so it cannot pollute alreadyImportedCount, and null route points are normalized by the existing validateRoutePoints helper rather than by ad-hoc checks. The test suite covers the happy path, the rejection paths, and the edge cases (mismatched secret, invalid date, instance-keyed duplicate) for each change. No files require special attention. Operators upgrading who use a local reverse proxy for the OSRM endpoint must set ADMIN_PROXY_SECRET — without it the forwarded-host fallback is now unconditionally blocked. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming OSRM GET] --> B{isAdminHost\nhost header?}
B -- yes --> ALLOW[Proxy to OSRM upstream]
B -- no --> C{isLocalProxyHost?}
C -- no --> DENY[403 Admin domain required]
C -- yes --> D{ADMIN_PROXY_SECRET\nenv set?}
D -- no --> DENY
D -- yes --> E{x-travel-tracker-admin-proxy-secret\nmatches?}
E -- no --> DENY
E -- yes --> F{isAdminHost\nx-forwarded-host?}
F -- no --> DENY
F -- yes --> ALLOW
Reviews (2): Last reviewed commit: "Address review feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Tests