Skip to content

fix(dashboard): harden keyset cursor parsing and timestamp precision #819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ding113 merged 2 commits into from
Feb 23, 2026
Merged

fix(dashboard): harden keyset cursor parsing and timestamp precision #819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
deef12b
fix(dashboard): harden keyset cursor parsing and timestamp precision
ding113 Feb 23, 2026
8218507
refactor(cache): rename ambiguous clearAllSessionsCache / clearAllSes…
ding113 Feb 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/actions/active-sessions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,12 +864,12 @@ export async function terminateActiveSession(sessionId: string): Promise<ActionR
}

// 4. 清除缓存
const { clearActiveSessionsCache, clearSessionDetailsCache, clearAllSessionsCache } =
const { clearActiveSessionsCache, clearSessionDetailsCache, clearAllSessionsQueryCache } =
await import("@/lib/cache/session-cache");

clearActiveSessionsCache();
clearSessionDetailsCache(sessionId);
clearAllSessionsCache();
clearAllSessionsQueryCache();

logger.info("Session terminated by user", {
sessionId,
Expand Down Expand Up @@ -1012,11 +1012,11 @@ export async function terminateActiveSessionsBatch(
const failedCount = allowedFailedCount + unauthorizedCount + missingCount;

// 4. 清除缓存
const { clearActiveSessionsCache, clearAllSessionsCache, clearSessionDetailsCache } =
const { clearActiveSessionsCache, clearAllSessionsQueryCache, clearSessionDetailsCache } =
await import("@/lib/cache/session-cache");

clearActiveSessionsCache();
clearAllSessionsCache();
clearAllSessionsQueryCache();

// 清除每个终止 Session 的详情缓存
for (const sid of allowedSessionIds) {
Expand Down
4 changes: 3 additions & 1 deletion src/actions/users.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,9 @@ export async function getUsersUsageBatch(
return { ok: true, data: { usageByKeyId: {} } };
}

const sanitizedIds = Array.from(new Set(userIds)).filter((id) => Number.isInteger(id));
const sanitizedIds = Array.from(new Set(userIds)).filter(
(id) => Number.isInteger(id) && id > 0
);
if (sanitizedIds.length === 0) {
return { ok: true, data: { usageByKeyId: {} } };
}
Expand Down
4 changes: 2 additions & 2 deletions src/lib/cache/session-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ export function clearActiveSessionsCache() {
/**
* 清空所有 Sessions 的缓存（包括活跃和非活跃）

@github-actions github-actions Bot Feb 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] [COMMENT-INACCURATE] Comment does not match function behavior

Why this is a problem: The comment states the function clears all sessions cache including active and inactive, but the function only deletes the 'all_sessions' key. This is misleading and could cause developers to misunderstand the function's scope.

Suggested fix:

/**
 * 清空所有 Sessions 查询缓存
 */
export function clearAllSessionsQueryCache() {
  activeSessionsCache.delete("all_sessions");
}

*/
export function clearAllSessionsCache() {
export function clearAllSessionsQueryCache() {
activeSessionsCache.delete("all_sessions");
}

Expand All @@ -197,7 +197,7 @@ export function clearSessionDetailsCache(sessionId: string) {
/**
* 清空所有 Session 缓存
*/
export function clearAllSessionCache() {
export function clearAllCaches() {
activeSessionsCache.clear();
sessionDetailsCache.clear();
}
Expand Down
21 changes: 16 additions & 5 deletions src/repository/user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,13 @@ interface KeysetCursor {
}

function parseKeysetCursor(raw: string): KeysetCursor | null {
if (raw.length > 1024) return null;

@github-actions github-actions Bot Feb 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] [TEST-MISSING-CRITICAL] Cursor hardening + keyset pagination changes are not covered by unit tests

Why this is a problem: This PR changes pagination behavior (cursor length cap, id validation, createdAt truncation, keyset-to-offset fallback) but adds no tests. This violates the repo guideline: CLAUDE.md: "Test Coverage - All new features must have unit test coverage of at least 80%".

Suggested fix (extract pure cursor helpers + add fast unit tests):

// src/repository/_lib/keyset-cursor.ts
export interface KeysetCursor {
  v: string;
  id: number;
}

export function parseKeysetCursor(raw: string): KeysetCursor | null {
  if (raw.length > 1024) return null;
  try {
    const parsed = JSON.parse(raw);
    if (typeof parsed === "object" && parsed !== null && "v" in parsed && "id" in parsed) {
      const id = Number((parsed as { id: unknown }).id);
      if (!Number.isFinite(id) || !Number.isInteger(id) || id <= 0) return null;
      return { v: String((parsed as { v: unknown }).v), id };
    }
  } catch {
    // not JSON
  }
  return null;
}
// tests/unit/repository/keyset-cursor.test.ts
import { describe, expect, it } from "vitest";
import { parseKeysetCursor } from "@/repository/_lib/keyset-cursor";

describe("parseKeysetCursor", () => {
  it("rejects oversize cursors", () => {
    expect(parseKeysetCursor("x".repeat(1025))).toBeNull();
  });

  it("rejects non-positive ids", () => {
    expect(parseKeysetCursor(JSON.stringify({ v: "a", id: 0 }))).toBeNull();
  });
});

try {
const parsed = JSON.parse(raw);
if (typeof parsed === "object" && parsed !== null && "v" in parsed && "id" in parsed) {
return { v: String(parsed.v), id: Number(parsed.id) };
const id = Number(parsed.id);
if (!Number.isFinite(id) || !Number.isInteger(id) || id <= 0) return null;

@gemini-code-assist gemini-code-assist Bot Feb 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The !Number.isFinite(id) check is redundant here. The Number.isInteger() method returns false for non-finite values such as Infinity, -Infinity, and NaN. Therefore, !Number.isInteger(id) already covers the check for finiteness, making the explicit !Number.isFinite(id) check unnecessary. Removing it simplifies the condition.

      if (!Number.isInteger(id) || id <= 0) return null;

return { v: String(parsed.v), id };
}
} catch {
// not JSON - treat as offset
Expand Down Expand Up @@ -299,18 +302,20 @@ export async function findUserListBatch(
// Keyset cursor: seek past the last row of the previous page.
// ASC: ORDER BY col ASC, id ASC -> WHERE (col, id) > (cv, cid)
// DESC: ORDER BY col DESC, id ASC -> mixed direction, must decompose manually
// Truncate timestamps to millisecond precision to match JS Date encoding.
let offset = 0;
if (cursor && useKeyset) {
const keyset = parseKeysetCursor(cursor);
if (keyset) {
if (sortBy === "createdAt") {
const d = new Date(keyset.v);
if (!Number.isNaN(d.getTime())) {
const truncCol = sql`date_trunc('milliseconds', ${sortColumn})`;

@github-actions github-actions Bot Feb 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] [LOGIC-BUG] Keyset predicate truncates createdAt but ORDER BY still uses microsecond precision

Why this is a problem: Keyset pagination requires the seek predicate and ORDER BY to use the same sort keys. Here the predicate switches to date_trunc('milliseconds', created_at) (millisecond buckets) while orderByClause remains users.createdAt (microseconds). If multiple rows share the same millisecond but differ in microseconds, the query can return rows in an order the cursor cannot represent, which can lead to skipped rows across page boundaries.

Suggested fix (use the same truncCol in both the predicate and ORDER BY when sortBy === "createdAt"):

const keysetSortColumn =
  sortBy === "createdAt" ? sql`date_trunc('milliseconds', ${sortColumn})` : sortColumn;

const orderByClause = sortOrder === "asc" ? asc(keysetSortColumn) : sql`${keysetSortColumn} DESC`;

// ... and reuse keysetSortColumn in the keyset predicate
conditions.push(sql`(${keysetSortColumn}, ${users.id}) > (${d}, ${keyset.id})`);

if (sortOrder === "asc") {
conditions.push(sql`(${sortColumn}, ${users.id}) > (${d}, ${keyset.id})`);
conditions.push(sql`(${truncCol}, ${users.id}) > (${d}, ${keyset.id})`);
} else {
conditions.push(
sql`(${sortColumn} < ${d} OR (${sortColumn} = ${d} AND ${users.id} > ${keyset.id}))`
sql`(${truncCol} < ${d} OR (${truncCol} = ${d} AND ${users.id} > ${keyset.id}))`
);
}
}
Comment on lines 310 to 321

@greptile-apps greptile-apps Bot Feb 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if keyset.v contains an invalid date string (e.g. corrupted cursor), the Number.isNaN(d.getTime()) check fails silently and no condition is added. this causes the query to fetch from the beginning instead of the intended page. consider falling back to offset or returning early:

Suggested change
if (sortBy === "createdAt") {
const d = new Date(keyset.v);
if (!Number.isNaN(d.getTime())) {
const truncCol = sql`date_trunc('milliseconds', ${sortColumn})`;
if (sortOrder === "asc") {
conditions.push(sql`(${sortColumn}, ${users.id}) > (${d}, ${keyset.id})`);
conditions.push(sql`(${truncCol}, ${users.id}) > (${d}, ${keyset.id})`);
} else {
conditions.push(
sql`(${sortColumn} < ${d} OR (${sortColumn} = ${d} AND ${users.id} > ${keyset.id}))`
sql`(${truncCol} < ${d} OR (${truncCol} = ${d} AND ${users.id} > ${keyset.id}))`
);
}
}
if (sortBy === "createdAt") {
const d = new Date(keyset.v);
if (Number.isNaN(d.getTime())) {
// Invalid date in cursor - fall back to offset
offset = Math.max(Number(cursor) || 0, 0);
} else {
const truncCol = sql`date_trunc('milliseconds', ${sortColumn})`;
if (sortOrder === "asc") {
conditions.push(sql`(${truncCol}, ${users.id}) > (${d}, ${keyset.id})`);
} else {
conditions.push(
sql`(${truncCol} < ${d} OR (${truncCol} = ${d} AND ${users.id} > ${keyset.id}))`
);
}
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/repository/user.ts
Line: 310-321

Comment:
if `keyset.v` contains an invalid date string (e.g. corrupted cursor), the `Number.isNaN(d.getTime())` check fails silently and no condition is added. this causes the query to fetch from the beginning instead of the intended page. consider falling back to offset or returning early:

```suggestion
      if (sortBy === "createdAt") {
        const d = new Date(keyset.v);
        if (Number.isNaN(d.getTime())) {
          // Invalid date in cursor - fall back to offset
          offset = Math.max(Number(cursor) || 0, 0);
        } else {
          const truncCol = sql`date_trunc('milliseconds', ${sortColumn})`;
          if (sortOrder === "asc") {
            conditions.push(sql`(${truncCol}, ${users.id}) > (${d}, ${keyset.id})`);
          } else {
            conditions.push(
              sql`(${truncCol} < ${d} OR (${truncCol} = ${d} AND ${users.id} > ${keyset.id}))`
            );
          }
        }
```

How can I resolve this? If you propose a fix, please make it concise.

Expand All @@ -323,6 +328,9 @@ export async function findUserListBatch(
);
}
}
} else {
// Cursor is not valid keyset JSON -- fall back to offset
offset = Math.max(Number(cursor) || 0, 0);

@gemini-code-assist gemini-code-assist Bot Feb 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

In the findUserListBatch function, when a cursor is provided but is not a valid keyset JSON, the code falls back to treating the cursor as a numeric offset. The line offset = Math.max(Number(cursor) || 0, 0); converts the user-provided cursor string to a number without any upper-bound validation. An authenticated administrator could exploit this by providing a very large number (e.g., 9999999999) as the cursor, which would be passed directly to the database query's OFFSET clause. A large offset can cause severe performance degradation or a denial of service by forcing the database to scan and discard a massive number of rows.

      const MAX_OFFSET = 10000; // Define a reasonable upper limit for the offset
      // Cursor is not valid keyset JSON -- fall back to offset
      offset = Math.min(Math.max(Number(cursor) || 0, 0), MAX_OFFSET);

@github-actions github-actions Bot Feb 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] [LOGIC-BUG] Offset fallback accepts Infinity/non-integers and can break the query

Why this is a problem: Number(cursor) accepts scientific notation and can return Infinity (e.g. cursor="1e309") or non-integers. That value then flows into query.offset(offset), which can generate invalid SQL/params and crash pagination for malformed cursors.

Suggested fix:

function parseOffsetCursor(raw: string): number {
  if (raw.length > 32) return 0;

  const parsed = Number.parseInt(raw, 10);
  if (!Number.isFinite(parsed) || !Number.isSafeInteger(parsed) || parsed < 0) return 0;

  return parsed;
}

// ...
offset = parseOffsetCursor(cursor);

}
} else if (cursor) {
// Offset fallback for nullable columns
Expand Down Expand Up @@ -369,8 +377,11 @@ export async function findUserListBatch(
if (hasMore) {
if (useKeyset) {
const lastRow = usersToReturn[usersToReturn.length - 1];
const rawValue = sortBy === "name" ? lastRow.name : lastRow.createdAt;
nextCursor = encodeKeysetCursor(rawValue, lastRow.id);
const keysetRowValue: Record<string, unknown> = {
name: lastRow.name,
createdAt: lastRow.createdAt,
};
nextCursor = encodeKeysetCursor(keysetRowValue[sortBy], lastRow.id);
} else {
nextCursor = String(offset + effectiveLimit);
}
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/actions/active-sessions-special-settings.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ vi.mock("@/lib/cache/session-cache", () => ({
setSessionDetailsCache: setSessionDetailsCacheMock,
clearActiveSessionsCache: vi.fn(),
clearSessionDetailsCache: vi.fn(),
clearAllSessionsCache: vi.fn(),
clearAllSessionsQueryCache: vi.fn(),
}));

vi.mock("@/lib/logger", () => ({
Expand Down
16 changes: 8 additions & 8 deletions tests/unit/lib/cache/session-cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ async function loadSessionCache() {
getSessionDetailsCache: mod.getSessionDetailsCache,
setSessionDetailsCache: mod.setSessionDetailsCache,
clearActiveSessionsCache: mod.clearActiveSessionsCache,
clearAllSessionsCache: mod.clearAllSessionsCache,
clearAllSessionsQueryCache: mod.clearAllSessionsQueryCache,
clearSessionDetailsCache: mod.clearSessionDetailsCache,
clearAllSessionCache: mod.clearAllSessionCache,
clearAllCaches: mod.clearAllCaches,
startCacheCleanup: mod.startCacheCleanup,
stopCacheCleanup: mod.stopCacheCleanup,
getCacheStats: mod.getCacheStats,
Expand Down Expand Up @@ -125,9 +125,9 @@ describe("SessionCache（Session 数据缓存层）", () => {
getSessionDetailsCache,
setSessionDetailsCache,
clearActiveSessionsCache,
clearAllSessionsCache,
clearAllSessionsQueryCache,
clearSessionDetailsCache,
clearAllSessionCache,
clearAllCaches,
} = await loadSessionCache();

setActiveSessionsCache(
Expand Down Expand Up @@ -182,14 +182,14 @@ describe("SessionCache（Session 数据缓存层）", () => {
clearActiveSessionsCache();
expect(getActiveSessionsCache()).toBeNull();

clearAllSessionsCache();
// clearAllSessionsCache 删除 all_sessions，而非 active_sessions
clearAllSessionsQueryCache();
// clearAllSessionsQueryCache 删除 all_sessions，而非 active_sessions
expect(getActiveSessionsCache("all_sessions")).toBeNull();

clearSessionDetailsCache("s_1");
expect(getSessionDetailsCache("s_1")).toBeNull();

// 再次写入后，clearAllSessionCache 应清空两类缓存
// 再次写入后，clearAllCaches 应清空两类缓存
setActiveSessionsCache([], "active_sessions");
setSessionDetailsCache("s_2", {
sessionId: "s_2",
Expand All @@ -213,7 +213,7 @@ describe("SessionCache（Session 数据缓存层）", () => {
cacheTtlApplied: null,
});

clearAllSessionCache();
clearAllCaches();
expect(getActiveSessionsCache()).toBeNull();
expect(getSessionDetailsCache("s_2")).toBeNull();
});
Expand Down
Loading