fix: scope my-usage ip geo lookups#1034
Conversation
📝 WalkthroughWalkthrough新增只读受限服务器动作 Changes
估算代码审查工作量🎯 3 (Moderate) | ⏱️ ~22 minutes 可能相关的 PR
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
github-actions
Bot
added
bug
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![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 introduces a restricted IP geolocation lookup feature for the usage logs. It adds a new server action, getMyIpGeoDetails, which ensures that users can only access geolocation data for IPs that have appeared in their own usage history. The useIpGeo hook and UI components like IpDetailsDialog and VirtualizedLogsTable were updated to support a lookupMode property for routing these requests. Feedback was provided to simplify the return type of the new server action by utilizing the existing IpGeoLookupResponse type.
| type UsageLogSummary, | ||
| type UsageLogsBatchResult, | ||
| } from "@/repository/usage-logs"; | ||
| import type { IpGeoLookupResult, IpGeoPrivateMarker } from "@/types/ip-geo"; |
There was a problem hiding this comment.
Consider importing IpGeoLookupResponse as well to simplify the return type definition of getMyIpGeoDetails.
| import type { IpGeoLookupResult, IpGeoPrivateMarker } from "@/types/ip-geo"; | |
| import type { IpGeoLookupResponse, IpGeoLookupResult, IpGeoPrivateMarker } from "@/types/ip-geo"; |
| export async function getMyIpGeoDetails(params: { ip: string; lang?: string }): Promise< | ||
| ActionResult<{ | ||
| status: "ok" | "private" | "error"; | ||
| data?: IpGeoLookupResult | IpGeoPrivateMarker; | ||
| error?: string; | ||
| }> | ||
| > { |
There was a problem hiding this comment.
The return type of getMyIpGeoDetails can be simplified by using the IpGeoLookupResponse type directly, which already represents the union of successful, private, and error states for IP geolocation lookups.
export async function getMyIpGeoDetails(params: { ip: string; lang?: string }): Promise<
ActionResult<IpGeoLookupResponse>
> {![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| expect(visible.response.status).toBe(200); | ||
| expect(visible.json).toMatchObject({ ok: true }); | ||
|
|
There was a problem hiding this comment.
ipGeoLookupEnabled setting
expect(visible.json).toMatchObject({ ok: true }) passes only when ipGeoLookupEnabled is true in the test DB and the upstream geo service responds without error. If the feature flag is off (common in a fresh test DB), getMyIpGeoDetails returns { ok: false, error: "IP geolocation disabled" } — causing the assertion to fail even though the scope check itself is correct.
The test's primary goal is to verify scoping (Key A can look up its own IP; Key B's IP is denied). Consider asserting on the error string rather than ok: true so the test is independent of the geo-lookup feature flag:
// Assert that the visible IP is not blocked by scoping:
expect(visible.json).not.toMatchObject({
ok: false,
error: "IP not found in current key usage logs",
});
// Assert the hidden IP IS blocked by scoping:
expect(hidden.json).toMatchObject({
ok: false,
error: "IP not found in current key usage logs",
});Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/api/my-usage-readonly.test.ts
Line: 350-352
Comment:
**Visible-IP assertion couples scoping test to `ipGeoLookupEnabled` setting**
`expect(visible.json).toMatchObject({ ok: true })` passes only when `ipGeoLookupEnabled` is `true` in the test DB *and* the upstream geo service responds without error. If the feature flag is off (common in a fresh test DB), `getMyIpGeoDetails` returns `{ ok: false, error: "IP geolocation disabled" }` — causing the assertion to fail even though the scope check itself is correct.
The test's primary goal is to verify scoping (Key A can look up its own IP; Key B's IP is denied). Consider asserting on the error string rather than `ok: true` so the test is independent of the geo-lookup feature flag:
```typescript
// Assert that the visible IP is not blocked by scoping:
expect(visible.json).not.toMatchObject({
ok: false,
error: "IP not found in current key usage logs",
});
// Assert the hidden IP IS blocked by scoping:
expect(hidden.json).toMatchObject({
ok: false,
error: "IP not found in current key usage logs",
});
```
How can I resolve this? If you propose a fix, please make it concise.![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| body: { ip: visibleIp, lang: "en" }, | ||
| }); | ||
| expect(visible.response.status).toBe(200); | ||
| expect(visible.json).toMatchObject({ ok: true }); |
There was a problem hiding this comment.
[Medium] [TEST-BRITTLE] Visible-IP assertion couples scoping test to ipGeoLookupEnabled setting
Why this is a problem: The test name states it verifies key-scoped IP access, but expect(visible.json).toMatchObject({ ok: true }) also depends on (1) ipGeoLookupEnabled being true in system settings and (2) the upstream geo service returning successfully. If the feature flag is off in the test DB, the action returns { ok: false, error: "IP geolocation disabled" } and the assertion fails -- even though the scoping logic itself is correct.
Suggested fix:
// Assert the visible IP is NOT blocked by scoping (the actual concern of this test).
// This passes regardless of ipGeoLookupEnabled or upstream service status.
expect(visible.json).not.toMatchObject({
ok: false,
error: "IP not found in current key usage logs",
});
// Assert the hidden IP IS blocked by scoping (unchanged).
expect(hidden.json).toMatchObject({
ok: false,
error: "IP not found in current key usage logs",
});
There was a problem hiding this comment.
Code Review Summary
This PR introduces a scoped getMyIpGeoDetails server action for the my-usage page, with proper key-scoped IP verification, readonly access support, and adequate unit test coverage. The implementation is architecturally sound -- the scope check correctly gates lookups to IPs that appear in the requesting key's own non-deleted usage logs, preventing cross-key IP enumeration. Component prop threading through IpDetailsDialog and VirtualizedLogsTable is clean and backward-compatible via defaults.
PR Size: M
- Lines changed: 351
- Files changed: 10
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority Issues (Should Fix)
- [TEST-BRITTLE]
tests/api/my-usage-readonly.test.ts:351-- The visible-IP assertionexpect(visible.json).toMatchObject({ ok: true })couples the scoping test toipGeoLookupEnabledbeing true and the upstream geo service being available. The test's stated purpose is verifying key-scoped IP access control, so the assertion should verify that scoping does not block the visible IP (usingnot.toMatchObjectwith the scope-error message), not that the full geo lookup succeeds.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10) -- parameterized queries, no injection risk, proper scope gating
- Error handling -- try/catch with logging, user-facing error messages
- Type safety -- IpGeoLookupMode union type, ActionResult generic properly typed
- Documentation accuracy
- Test coverage -- 5 unit tests + 1 integration test covering auth, validation, scope, and settings
- Code clarity
Automated review by Claude AI
| authToken: keyA.key, | ||
| body: { ip: hiddenIp, lang: "en" }, | ||
| }); | ||
| expect(hidden.response.status).toBe(200); |
There was a problem hiding this comment.
[High] [TEST-MISSING-CRITICAL] Wrong HTTP status assertion for ActionResult ok:false
Why this is a problem: The Actions adapter maps { ok: false, error: ... } to HTTP 400 (business error): return c.json({ ok: false, error: result.error, ... }, 400); (src/lib/api/action-adapter-openapi.ts). As written, tests/api/my-usage-readonly.test.ts:359 expects 200 and will fail.
Suggested fix:
expect(hidden.response.status).toBe(400);
expect(hidden.json).toMatchObject({
ok: false,
error: "IP not found in current key usage logs",
});
| headers: { "content-type": "application/json" }, | ||
| body: JSON.stringify({ ip, lang: locale }), | ||
| }); | ||
| if (!response.ok) throw new Error(`my-ip-geo fetch failed: ${response.status}`); |
There was a problem hiding this comment.
[High] [LOGIC-BUG] useIpGeo my-usage branch drops ActionResult error details on HTTP 400
Why this is a problem: /api/actions/* returns HTTP 400 for { ok: false, error: ... } results, but src/hooks/use-ip-geo.ts:30 throws on !response.ok before parsing the JSON body. That makes the later if (!result.ok) block unreachable for business errors and loses the server-provided error message.
Suggested fix (parse the ActionResult even on 400 and normalize into IpGeoLookupResponse so the dialog can render details):
const response = await fetch("/api/actions/my-usage/getMyIpGeoDetails", {
method: "POST",
headers: { "content-type": "application/json" },
body: JSON.stringify({ ip, lang: locale }),
});
const result = (await response.json()) as ActionResult<IpGeoLookupResponse>;
if (!result.ok) {
return { status: "error", error: result.error };
}
return result.data;
There was a problem hiding this comment.
Code Review Summary
PR adds a my-usage-scoped IP geolocation lookup action and wires the IP details dialog to use it from the my-usage logs table. The remaining issues are around Actions adapter HTTP semantics (400 vs 200) not matching the new test and the client hook.
PR Size: M
- Lines changed: 351
- Files changed: 10
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 1 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 1 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
tests/api/my-usage-readonly.test.ts:359: asserts HTTP 200 for{ ok: false }ActionResult, but the Actions adapter returns HTTP 400 for business errors.src/hooks/use-ip-geo.ts:30: throws on!response.okbefore parsing the ActionResult JSON, dropping server error details on HTTP 400.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/actions/my-usage.ts`:
- Around line 713-733: Move the ip-lookup feature flag check before the DB query
and make the visibility filter consistent with my-usage by adding the warmup
exclusion: first call getSystemSettings() and return when
settings.ipGeoLookupEnabled is false, then run the select on messageRequest;
when building the where clause for the query (currently using
isNull(messageRequest.deletedAt), eq(messageRequest.key, session.key.key),
eq(messageRequest.clientIp, ip)) also include the EXCLUDE_WARMUP_CONDITION used
elsewhere (e.g., in getMyStatsSummary) so IPs only from warmup requests are
excluded and the visibility semantics match the my-usage list.
- Around line 699-705: The return type of getMyIpGeoDetails should use the
existing discriminated union IpGeoLookupResponse instead of the inline type;
update the function signature from Promise<ActionResult<{ status: "ok" |
"private" | "error"; ... }>> to Promise<ActionResult<IpGeoLookupResponse>> and
adjust any imports/exports as needed so IpGeoLookupResponse is available (this
matches the type returned by lookupIp()).
In `@src/hooks/use-ip-geo.ts`:
- Around line 24-37: The my-usage branch in use-ip-geo.ts currently throws new
Error(result.error) when ActionResult<IpGeoLookupResponse> has ok: false, which
yields an "undefined" message if result.error is missing; change the error throw
to include a safe fallback message and/or include serialized result so the Error
always has useful text (e.g. use result.error ?? "Unknown IP geo lookup error"
or include JSON.stringify(result) when constructing the Error) — update the code
paths that reference result (the constant named result inside the mode ===
"my-usage" block) so any failure throws a descriptive Error instead of
undefined.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91386831-c8d9-4134-8c20-b5aef4115c56
📒 Files selected for processing (10)
src/actions/my-usage.tssrc/app/[locale]/dashboard/_components/ip-details-dialog.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/app/[locale]/my-usage/_components/usage-logs-section.test.tsxsrc/app/[locale]/my-usage/_components/usage-logs-section.tsxsrc/app/api/actions/[...route]/route.tssrc/hooks/use-ip-geo.tstests/api/api-actions-integrity.test.tstests/api/my-usage-readonly.test.tstests/unit/actions/my-usage-ip-geo.test.ts
| export async function getMyIpGeoDetails(params: { ip: string; lang?: string }): Promise< | ||
| ActionResult<{ | ||
| status: "ok" | "private" | "error"; | ||
| data?: IpGeoLookupResult | IpGeoPrivateMarker; | ||
| error?: string; | ||
| }> | ||
| > { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '\b(IpGeoLookupResult|IpGeoPrivateMarker)\b' --type=ts
ast-grep --pattern 'export function lookupIp($$$) { $$$ }'
ast-grep --pattern 'export async function lookupIp($$$) { $$$ }'Repository: ding113/claude-code-hub
Length of output: 13458
🏁 Script executed:
# Check ActionResult type definition
rg -nP 'export (type|interface)\s+ActionResult' --type=ts
# Check the implementation of getMyIpGeoDetails
sed -n '699,750p' src/actions/my-usage.ts
# Check if IpGeoLookupResponse is used elsewhere
rg -nP 'IpGeoLookupResponse' --type=tsRepository: ding113/claude-code-hub
Length of output: 3594
返回类型应改用已有的 IpGeoLookupResponse，而非 IpGeoLookupResult
当前内联的 { status: "ok" | "private" | "error"; data?: ...; error?: ... } 与实现中 lookupIp() 返回的已有类型 IpGeoLookupResponse 重复。后者已是正确的判别联合，完整包含三个分支：{ status: "ok"; data: IpGeoLookupResult } | { status: "private"; data: IpGeoPrivateMarker } | { status: "error"; error: string }。应直接使用 ActionResult<IpGeoLookupResponse> 替代内联定义。
♻️ 建议修改
-export async function getMyIpGeoDetails(params: { ip: string; lang?: string }): Promise<
- ActionResult<{
- status: "ok" | "private" | "error";
- data?: IpGeoLookupResult | IpGeoPrivateMarker;
- error?: string;
- }>
-> {
+export async function getMyIpGeoDetails(
+ params: { ip: string; lang?: string }
+): Promise<ActionResult<IpGeoLookupResponse>> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function getMyIpGeoDetails(params: { ip: string; lang?: string }): Promise< | |
| ActionResult<{ | |
| status: "ok" | "private" | "error"; | |
| data?: IpGeoLookupResult | IpGeoPrivateMarker; | |
| error?: string; | |
| }> | |
| > { | |
| export async function getMyIpGeoDetails( | |
| params: { ip: string; lang?: string } | |
| ): Promise<ActionResult<IpGeoLookupResponse>> { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/my-usage.ts` around lines 699 - 705, The return type of
getMyIpGeoDetails should use the existing discriminated union
IpGeoLookupResponse instead of the inline type; update the function signature
from Promise<ActionResult<{ status: "ok" | "private" | "error"; ... }>> to
Promise<ActionResult<IpGeoLookupResponse>> and adjust any imports/exports as
needed so IpGeoLookupResponse is available (this matches the type returned by
lookupIp()).
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
- Identified open PR
#1034(“fix: scope my-usage ip geo lookups”) and gathered diff + changed files. - Applied size label:
size/M(351 lines, 10 files). - Posted 2 inline review comments (test status expectation +
useIpGeomy-usage error handling). - Submitted the required review summary via
gh pr review --comment.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cf1fd93fc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const [visibleLog] = await db | ||
| .select({ id: messageRequest.id }) | ||
| .from(messageRequest) | ||
| .where( | ||
| and( |
There was a problem hiding this comment.
The new authorization gate only checks message_request rows (and also applies EXCLUDE_WARMUP_CONDITION), but the my-usage table is populated via getMyUsageLogsBatchFull → findUsageLogsBatch, which can surface rows from other visible sources (notably usage_ledger fallback) and rows not filtered the same way. In those cases, users can see an IP in the table but getMyIpGeoDetails will always return IP not found in current key usage logs, so the IP details dialog fails for visible records.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/api/my-usage-readonly.test.ts (1)
355-365:⚠️ Potential issue | 🟡 Minorhidden IP 断言仍依赖
ipGeoLookupEnabled当前 action 会先检查系统开关；如果测试 DB 中
ipGeoLookupEnabled=false，这里会返回"IP geolocation disabled"，而不是进入 key-scoped not-found 分支。建议在该用例 setup 中显式开启 IP geo，或隔离/mock 系统设置，否则 scoping 断言仍不稳定。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/my-usage-readonly.test.ts` around lines 355 - 365, The test's assertion for the "IP not found in current key usage logs" branch is flaky because the system feature flag ipGeoLookupEnabled may be false; before invoking callActionsRoute for /api/actions/my-usage/getMyIpGeoDetails with keyA.key and hiddenIp you must ensure IP geolocation is enabled in the test setup (e.g. set ipGeoLookupEnabled = true in the test DB or mock the feature flag) so the action reaches the key-scoped not-found branch; alternatively explicitly mock the system setting lookup used by the action to return true prior to calling callActionsRoute.
🧹 Nitpick comments (1)
tests/api/my-usage-readonly.test.ts (1)
297-297: 确认新增 DB-backed API 测试的目录约定这个用例属于 API/集成覆盖，但新增在
tests/api/。如果项目没有对tests/api/的例外约定，建议迁移到tests/integration/，或在规范中明确 API 测试目录。As per coding guidelines,
**/*.test.{ts,tsx}: Unit tests should be located in tests/unit/, integration tests in tests/integration/, and source-adjacent tests in src/**/*.test.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/my-usage-readonly.test.ts` at line 297, This test file defines an integration/API test ("只读 Key：只能查询当前 key 日志里出现过的 IP 详情") placed under tests/api/ which violates the repo's test-directory conventions; move the test file to tests/integration/ (or update the project test-directory convention if API tests are intentionally separated) and update any import paths or test runner config that reference tests/api/my-usage-readonly.test.ts; ensure the test name and exported test code (the test(...) block) remain unchanged so test discovery still finds it under tests/integration/.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/actions/my-usage.ts`:
- Around line 736-749: The logs in getMyIpGeoDetails are recording raw IPs
(variables ip and params.ip) via logger.warn/logger.error; remove the raw IP
from both log payloads and instead record a non-sensitive identifier (e.g.
params.messageRequestId or session.user.id) or omit the field entirely to avoid
storing personal data. Locate the lookupIp call and the catch block (symbols:
lookupIp, getMyIpGeoDetails, logger.warn, logger.error, ip, params.ip) and
replace the ip/params.ip entries with messageRequestId (if available) or other
internal IDs, or drop them; keep existing error/result details but do not
include the original IP.
---
Duplicate comments:
In `@tests/api/my-usage-readonly.test.ts`:
- Around line 355-365: The test's assertion for the "IP not found in current key
usage logs" branch is flaky because the system feature flag ipGeoLookupEnabled
may be false; before invoking callActionsRoute for
/api/actions/my-usage/getMyIpGeoDetails with keyA.key and hiddenIp you must
ensure IP geolocation is enabled in the test setup (e.g. set ipGeoLookupEnabled
= true in the test DB or mock the feature flag) so the action reaches the
key-scoped not-found branch; alternatively explicitly mock the system setting
lookup used by the action to return true prior to calling callActionsRoute.
---
Nitpick comments:
In `@tests/api/my-usage-readonly.test.ts`:
- Line 297: This test file defines an integration/API test ("只读 Key：只能查询当前 key
日志里出现过的 IP 详情") placed under tests/api/ which violates the repo's test-directory
conventions; move the test file to tests/integration/ (or update the project
test-directory convention if API tests are intentionally separated) and update
any import paths or test runner config that reference
tests/api/my-usage-readonly.test.ts; ensure the test name and exported test code
(the test(...) block) remain unchanged so test discovery still finds it under
tests/integration/.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ced6b177-d734-4561-a97e-985cbf573e23
📒 Files selected for processing (4)
src/actions/my-usage.tssrc/hooks/use-ip-geo.tstests/api/my-usage-readonly.test.tstests/unit/actions/my-usage-ip-geo.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/use-ip-geo.ts
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/actions/my-usage-ip-geo.test.ts (1)
109-125: 冗余的 mock 设置与断言自相矛盾第 114 行设置了
dbLimit.mockResolvedValueOnce([{ id: 42 }])，但第 123 行又断言dbLimit未被调用。根据实现（getSystemSettings检查先于 DB 查询），该 mock 不会被消费，会残留到下一个测试用例的队列中，虽然beforeEach中的vi.clearAllMocks()会清理，但阅读时仍造成混淆。建议移除该行以使测试意图更清晰。建议的清理
mocks.getSession.mockResolvedValueOnce({ user: { id: 1 }, key: { id: 10, key: "sk-test" }, }); - mocks.dbLimit.mockResolvedValueOnce([{ id: 42 }]); mocks.getSystemSettings.mockResolvedValueOnce({ ipGeoLookupEnabled: false, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/my-usage-ip-geo.test.ts` around lines 109 - 125, The test for getMyIpGeoDetails sets a redundant mocks.dbLimit.mockResolvedValueOnce([{ id: 42 }]) while also asserting mocks.dbLimit was not called; remove the mocks.dbLimit.mockResolvedValueOnce(...) setup from the "系统关闭 IP 查询时返回禁用错误" test so the mock queue isn't polluted and the expectation that dbLimit is not called remains consistent with the getSystemSettings mock (ipGeoLookupEnabled: false); keep the rest of the test (mocks.getSession, mocks.getSystemSettings, import of getMyIpGeoDetails, and the assertions) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/actions/my-usage-ip-geo.test.ts`:
- Around line 109-125: The test for getMyIpGeoDetails sets a redundant
mocks.dbLimit.mockResolvedValueOnce([{ id: 42 }]) while also asserting
mocks.dbLimit was not called; remove the
mocks.dbLimit.mockResolvedValueOnce(...) setup from the "系统关闭 IP 查询时返回禁用错误" test
so the mock queue isn't polluted and the expectation that dbLimit is not called
remains consistent with the getSystemSettings mock (ipGeoLookupEnabled: false);
keep the rest of the test (mocks.getSession, mocks.getSystemSettings, import of
getMyIpGeoDetails, and the assertions) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12b39d05-bfca-41e5-8a2b-e27ee66ae907
📒 Files selected for processing (2)
src/actions/my-usage.tstests/unit/actions/my-usage-ip-geo.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/actions/my-usage.ts
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3dc50e296
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .where( | ||
| and( | ||
| isNull(messageRequest.deletedAt), | ||
| EXCLUDE_WARMUP_CONDITION, |
There was a problem hiding this comment.
After this commit’s new getMyIpGeoDetails gate was added, the message_request scope check still hard-filters with EXCLUDE_WARMUP_CONDITION, but the my-usage table that opens this dialog (getMyUsageLogsBatchFull → findUsageLogsBatch) does not apply that warmup exclusion on message_request rows. In practice, a warmup log row can be visible in my-usage while the IP details action rejects it as “not found,” so users get a broken dialog for visible rows.
Useful? React with 👍 / 👎.
| .where(and(eq(usageLedger.key, session.key.key), eq(usageLedger.clientIp, ip))) | ||
| .limit(1); |
There was a problem hiding this comment.
The new ledger-only fallback authorizes by usage_ledger.key + usage_ledger.clientIp only, but visible my-usage ledger logs are filtered with LEDGER_BILLING_CONDITION (blocked_by IS NULL) in findUsageLogsBatch. This means blocked ledger rows that are hidden from the table can still satisfy authorization here, allowing IP lookups for data outside the visible my-usage log scope.
Useful? React with 👍 / 👎.
1 participant
Summary
getMyIpGeoDetails) gated by the current key's visible usage logs/api/ip-geo/[ip]admin-only and route my-usage IP dialogs through the scoped actionProblem
PR #1027 introduced IP geolocation lookup via the admin-only
/api/ip-geo/[ip]endpoint. PR #1029 then reusedVirtualizedLogsTablein the my-usage page but left IP detail lookups disconnected (the detail dialog was disabled). Readonly/API-only key users had no way to view IP geo details for their own usage logs, while the admin endpoint was not suitable for non-admin access.Related PRs:
IpDetailsDialogwith alookupModepropSolution
Introduce a new server action
getMyIpGeoDetailsin the my-usage module that:allowReadOnlyAccess: truemessage_requesttable)lookupIpclientThe
useIpGeohook gains amodeparameter ("default"|"my-usage") that routes my-usage requests to the new scoped action instead of the admin endpoint. TheIpDetailsDialogandVirtualizedLogsTablecomponents propagate this mode through props.Changes
src/actions/my-usage.tsgetMyIpGeoDetailsaction with key-scoped IP verification (+56)src/hooks/use-ip-geo.tsIpGeoLookupModetype; route"my-usage"mode through scoped action (+26/-2)src/app/[locale]/dashboard/_components/ip-details-dialog.tsxlookupModeprop (+11/-5)src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxipLookupModeprop (+10/-1)src/app/[locale]/my-usage/_components/usage-logs-section.tsxipLookupMode="my-usage"to table (+1)src/app/api/actions/[...route]/route.tsgetMyIpGeoDetailsOpenAPI route withallowReadOnlyAccess(+24)tests/unit/actions/my-usage-ip-geo.test.tstests/api/my-usage-readonly.test.tstests/api/api-actions-integrity.test.tssrc/app/[locale]/my-usage/_components/usage-logs-section.test.tsxipLookupMode="my-usage"is passed through (+3)Security
The action enforces that only IPs appearing in the requesting key's own non-deleted
message_requestrows can be queried. A readonly key cannot probe arbitrary IPs or discover IPs from other keys' traffic.Verification
bun run buildbun run lintbun run lint:fixbun run typecheckbun run testbunx vitest run tests/unit/actions/my-usage-ip-geo.test.ts 'src/app/[locale]/my-usage/_components/usage-logs-section.test.tsx' tests/api/api-actions-integrity.test.ts 'src/app/[locale]/dashboard/_components/ip-details-dialog.test.tsx'bunx vitest run --config tests/configs/integration.config.ts tests/api/my-usage-readonly.test.ts(fails locally in this environment becauseDSNis not set; existing DB-backed tests in that file cannot run without DB env)Description enhanced by Claude AI
Greptile Summary
This PR closes the access gap left by #1029 by introducing a key-scoped
getMyIpGeoDetailsserver action that lets readonly/API-only users look up IP geo details for IPs that appear in their own usage logs, while keeping the admin/api/ip-geo/[ip]endpoint off-limits to non-admins. The scope check — verifying the requested IP appears in non-deleted, non-warmupmessage_requestrows for the current key — is well-structured and the prop-threading throughVirtualizedLogsTable→IpDetailsDialog→useIpGeois clean.Confidence Score: 5/5
Safe to merge; all remaining findings are P2 style suggestions that do not affect correctness or security
The security model is sound: IP lookups are gated behind session auth, a key-scoped DB existence check, and the global feature flag. The usageLedger.requestId column is NOT NULL so the ledger-only fallback is correct. The disableDetailDialog + ipLookupMode combination is intentional (they gate different dialogs). Unit and integration test coverage is thorough. The two flagged items are a reversed check order (dead code) and a minor filter inconsistency in the ledger fallback — neither affects production behavior.
src/hooks/use-ip-geo.ts (check order) and src/actions/my-usage.ts (ledger fallback warmup filter) are worth a second look, but neither blocks merge
Important Files Changed
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "fix: align my-usage ip visibility checks" | Re-trigger Greptile