Conversation
Greptile SummaryThis PR adds VPN/proxy/Tor/relay/hosting detection to the free-mode country gate by calling the IPinfo privacy API and blocking allowlisted-country requests that carry anonymous-network signals. The gate is applied consistently to both the chat completions and freebuff session endpoints, with a 30-minute in-process cache and graceful fallback (allow) on lookup failure. Three P2 items worth a follow-up: the Confidence Score: 5/5Safe to merge; all findings are P2 style/reliability suggestions that don't block correctness. No P0 or P1 defects. The three issues (dead parameter, unbounded cache, VPN skip on missing IP) are all P2: the first is cosmetic, the second is a long-running-process concern unlikely to manifest in practice, and the third is a theoretical bypass only possible if Cloudflare is misconfigured or bypassed (at which point CF-IPCountry itself is already untrustworthy). web/src/server/free-mode-country.ts and web/src/app/api/v1/freebuff/session/_handlers.ts have the noted issues but nothing blocking.
|
| Filename | Overview |
|---|---|
| web/src/server/free-mode-country.ts | Core VPN-blocking logic: adds IPinfo privacy lookup with a 30-min module-level cache; cache grows unbounded and VPN check is skipped when clientIp is absent on an allowlisted CF-country path. |
| web/src/app/api/v1/freebuff/session/_handlers.ts | Makes countryBlockedResponse async and adds VPN gating; the deps parameter is accepted but entirely unused — simplification opportunity. |
| web/src/app/api/v1/chat/completions/_post.ts | Awaits the now-async getFreeModeCountryAccess and passes fetch + ipinfoToken; logging extended with ipPrivacySignals. Clean change. |
| packages/internal/src/env-schema.ts | Adds IPINFO_TOKEN as a required string field; minor reformatting of existing Stripe price ID and FREEBUFF session length fields. |
| packages/internal/src/env.ts | Fixes CI env-injection to work correctly by building envInput before parse; adds test defaults for IPINFO_TOKEN and the three Stripe subscription price IDs. |
| web/src/server/tests/free-mode-country.test.ts | Good coverage of VPN blocking, privacy lookup failure tolerance, and IPinfo signal parsing; tests are well-structured with injectable deps. |
| web/src/app/api/v1/freebuff/session/route.ts | Extracts shared freebuffSessionDeps constant to avoid duplication across GET/POST; straightforward cleanup. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: web/src/app/api/v1/freebuff/session/_handlers.ts
Line: 26-28
Comment:
**Unused `deps` parameter in `countryBlockedResponse`**
`deps: FreebuffSessionDeps` is accepted but never referenced in the function body — the implementation reads `env.IPINFO_TOKEN` directly from the module-level import. The parameter is dead weight that misleads readers into thinking the function uses injected dependencies. Removing it also removes the need for callers to pass `deps` to a function that ignores it, which is the simplification opportunity here.
```suggestion
async function countryBlockedResponse(
req: NextRequest,
): Promise<NextResponse | null> {
```
**Context Used:** Find ways to simplify the implementation ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/src/server/free-mode-country.ts
Line: 72-76
Comment:
**Module-level cache grows without bound**
`ipinfoPrivacyCache` never evicts stale entries — it only skips writing them on a cache hit. In a long-running process with many unique client IPs, every distinct IP accumulates an entry that persists in memory indefinitely even after its 30-minute TTL passes. Consider either periodically sweeping expired entries, applying a max-size eviction policy, or using a small LRU structure (e.g. a plain `Map` capped at a fixed count with `FIFO` eviction on `set`).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/src/server/free-mode-country.ts
Line: 221-229
Comment:
**VPN check silently skipped when CF country is present but no client IP**
When a request carries `cf-ipcountry: US` but no `x-forwarded-for` / `x-real-ip` header, `clientIp` is `undefined`, so `getIpPrivacy` returns `null` immediately (line 137) and the request is allowed through without any VPN lookup. While Cloudflare normally populates `x-forwarded-for`, a misconfiguration or an origin-direct request with a forged `cf-ipcountry` would bypass the VPN gate entirely. Consider checking `CF-Connecting-IP` as a fallback, or treating a missing IP on an allowlisted CF-country path as requiring an explicit policy decision rather than silently allowing.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Block free mode VPN traffic" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async function countryBlockedResponse( | ||
| req: NextRequest, | ||
| deps: FreebuffSessionDeps, |
There was a problem hiding this comment.
deps parameter in countryBlockedResponse
deps: FreebuffSessionDeps is accepted but never referenced in the function body — the implementation reads env.IPINFO_TOKEN directly from the module-level import. The parameter is dead weight that misleads readers into thinking the function uses injected dependencies. Removing it also removes the need for callers to pass deps to a function that ignores it, which is the simplification opportunity here.
| async function countryBlockedResponse( | |
| req: NextRequest, | |
| deps: FreebuffSessionDeps, | |
| async function countryBlockedResponse( | |
| req: NextRequest, | |
| ): Promise<NextResponse | null> { |
Context Used: Find ways to simplify the implementation (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/app/api/v1/freebuff/session/_handlers.ts
Line: 26-28
Comment:
**Unused `deps` parameter in `countryBlockedResponse`**
`deps: FreebuffSessionDeps` is accepted but never referenced in the function body — the implementation reads `env.IPINFO_TOKEN` directly from the module-level import. The parameter is dead weight that misleads readers into thinking the function uses injected dependencies. Removing it also removes the need for callers to pass `deps` to a function that ignores it, which is the simplification opportunity here.
```suggestion
async function countryBlockedResponse(
req: NextRequest,
): Promise<NextResponse | null> {
```
**Context Used:** Find ways to simplify the implementation ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.| const IPINFO_PRIVACY_CACHE_TTL_MS = 30 * 60 * 1000 | ||
| const ipinfoPrivacyCache = new Map< | ||
| string, | ||
| { expiresAt: number; privacy: FreeModeIpPrivacy | null } | ||
| >() |
There was a problem hiding this comment.
ipinfoPrivacyCache never evicts stale entries — it only skips writing them on a cache hit. In a long-running process with many unique client IPs, every distinct IP accumulates an entry that persists in memory indefinitely even after its 30-minute TTL passes. Consider either periodically sweeping expired entries, applying a max-size eviction policy, or using a small LRU structure (e.g. a plain Map capped at a fixed count with FIFO eviction on set).
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/server/free-mode-country.ts
Line: 72-76
Comment:
**Module-level cache grows without bound**
`ipinfoPrivacyCache` never evicts stale entries — it only skips writing them on a cache hit. In a long-running process with many unique client IPs, every distinct IP accumulates an entry that persists in memory indefinitely even after its 30-minute TTL passes. Consider either periodically sweeping expired entries, applying a max-size eviction policy, or using a small LRU structure (e.g. a plain `Map` capped at a fixed count with `FIFO` eviction on `set`).
How can I resolve this? If you propose a fix, please make it concise.| const ipPrivacy = await getIpPrivacy(clientIp, options) | ||
| if (ipPrivacy?.signals.length) { | ||
| return { | ||
| ...baseAccess, | ||
| allowed: false, | ||
| blockReason: 'anonymous_network', | ||
| ipPrivacy, | ||
| } | ||
| } |
There was a problem hiding this comment.
When a request carries cf-ipcountry: US but no x-forwarded-for / x-real-ip header, clientIp is undefined, so getIpPrivacy returns null immediately (line 137) and the request is allowed through without any VPN lookup. While Cloudflare normally populates x-forwarded-for, a misconfiguration or an origin-direct request with a forged cf-ipcountry would bypass the VPN gate entirely. Consider checking CF-Connecting-IP as a fallback, or treating a missing IP on an allowlisted CF-country path as requiring an explicit policy decision rather than silently allowing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/server/free-mode-country.ts
Line: 221-229
Comment:
**VPN check silently skipped when CF country is present but no client IP**
When a request carries `cf-ipcountry: US` but no `x-forwarded-for` / `x-real-ip` header, `clientIp` is `undefined`, so `getIpPrivacy` returns `null` immediately (line 137) and the request is allowed through without any VPN lookup. While Cloudflare normally populates `x-forwarded-for`, a misconfiguration or an origin-direct request with a forged `cf-ipcountry` would bypass the VPN gate entirely. Consider checking `CF-Connecting-IP` as a fallback, or treating a missing IP on an allowlisted CF-country path as requiring an explicit policy decision rather than silently allowing.
How can I resolve this? If you propose a fix, please make it concise.
|
Addressed the Greptile follow-ups in
Re-ran:
|
2 participants
Summary
IPINFO_TOKENand use IPinfo privacy signals in free-mode country gatingValidation
bun test web/src/server/__tests__/free-mode-country.test.tsCI=true bun test web/src/app/api/v1/freebuff/session/__tests__/session.test.tsCI=true bun test web/src/app/api/v1/chat/completions/__tests__/completions.test.tsbun run typecheckinwebbun run typecheckinpackages/internal