Conversation
- Add file size limits and stdin timeout to usage-import (DoS prevention) - Sanitize SQLite table names in cursor/windsurf scanners (SQLi prevention) - Add response size limits and structure validation to pricing data fetch - Add SHA-256 integrity verification for encryption key file - Restrict install scripts via pnpm.onlyBuiltDependencies - Enforce signed tags in release workflow - Add Dependabot config for npm and GitHub Actions - Add readStreamWithLimit helper for bounded network reads
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThis PR hardens key handling, pricing fetch validation, and usage import input limits in subtrack, adds scanner comments, and updates Dependabot, release tag verification messaging, and pnpm package metadata. ChangesSubtrack input/data hardening
CI and dependency configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 (3)
subtrack/src/windsurf-scanner.ts (1)
96-111: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSame dead-code guard as in
cursor-scanner.ts.
tableNameat Line 100 comes fromknownTables.find((t) => tableNames.includes(t)), so it is always drawn from the fixedknownTableslist, never from the untrustedtableNamesarray. The added!knownTables.includes(tableName)check at Line 107 can therefore never be true and doesn't provide real protection — see the corresponding comment incursor-scanner.tsfor the same pattern.🧹 Proposed cleanup
- // Sanitize table name: must match exactly a known table name (prevents SQL injection from malicious DB) - if (!knownTables.includes(tableName)) { - consola.warn(`Windsurf DB has suspicious table name "${tableName}" — skipping`) - return { source: "windsurf", entries: [] } - } + // tableName is guaranteed to be one of `knownTables` by construction + // (see the `.find` above), so no further identifier validation is needed here.🤖 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 `@subtrack/src/windsurf-scanner.ts` around lines 96 - 111, Remove the dead-code sanitization guard in windsurf-scanner.ts: tableName is already constrained by knownTables.find(...) to a fixed allowlist, so the subsequent !knownTables.includes(tableName) branch in the scan flow is unreachable. Keep the existing knownTables lookup and the no-table-found early return, and delete the redundant suspicious-table warning check to match the cleanup done in cursor-scanner.ts.subtrack/src/cursor-scanner.ts (1)
104-119: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdded "sanitize" check is unreachable —
tableNamecan never fail it.
tableNameis computed asknownTables.find((t) => tableNames.includes(t))— it iterates over the fixedknownTablesarray, so any non-undefinedresult is always one of"cursorDiskKV"/"ItemTable"by construction, never an attacker-controlled value fromtableNames. Consequently!knownTables.includes(tableName)at Line 115 can never be true; the new guard is dead code and doesn't add protection against a malicious DB (the existing derivation was already safe).Consider removing the redundant check (or replacing it with a comment clarifying the existing derivation is already safe), so the code doesn't imply a runtime validation that isn't actually happening.
🧹 Proposed cleanup
- // Sanitize table name: must match exactly a known table name (prevents SQL injection from malicious DB) - if (!knownTables.includes(tableName)) { - consola.warn(`Cursor DB has suspicious table name "${tableName}" — skipping`) - return { source: "cursor", entries: [] } - } + // tableName is guaranteed to be one of `knownTables` by construction + // (see the `.find` above), so no further identifier validation is needed here.🤖 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 `@subtrack/src/cursor-scanner.ts` around lines 104 - 119, The new sanitize guard in cursor-scanner’s table lookup is unreachable because cursorTableName/tableName is derived only from the fixed knownTables list, so the extra includes check can never fail. Remove the redundant runtime validation (or replace it with a short comment) in cursor-scanner’s table-name selection logic so the code accurately reflects that the safe derivation already prevents untrusted table names from being used.subtrack/src/usage-import.ts (1)
172-190: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid reopening the file between the size check and the read
statSync(safeFile)andreadFileSync(safeFile, "utf-8")use the path twice, so a writable file underhome/tmpcan be replaced after the size check and still be read. Open it once and use the same file descriptor for both operations.🤖 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 `@subtrack/src/usage-import.ts` around lines 172 - 190, Avoid reopening the target file in usage-import.ts: the current statSync(safeFile) followed by readFileSync(safeFile, "utf-8") can be raced if the file is replaced between calls. Update the file-reading flow in the usage-import logic to open the file once and perform both the size check and content read through the same file descriptor, keeping the existing MAX_FILE_SIZE, safeFile, and related error handling paths intact.
🧹 Nitpick comments (2)
subtrack/package.json (1)
37-39: 🔒 Security & Privacy | 🔵 TrivialKeep this empty allowlist intentional. Add any future dependency with install/postinstall scripts to
onlyBuiltDependencies, and consider a short note here so the policy is obvious.🤖 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 `@subtrack/package.json` around lines 37 - 39, The pnpm onlyBuiltDependencies allowlist is currently empty and should be made intentionally documented. Update the package.json pnpm configuration around onlyBuiltDependencies to keep the empty list explicit, and add a short note/comment nearby explaining that any future dependency with install/postinstall scripts must be added there.subtrack/src/pricing.ts (1)
69-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancel the stream reader when the size limit is exceeded.
When
totalBytes > MAX_PRICING_JSON_BYTES(Lines 79-81), the code throws without callingreader.cancel(), so the underlying HTTP response stream isn't explicitly released — it relies on the 15sFETCH_TIMEOUT_MS/GC to eventually clean up the connection rather than terminating it immediately.🔧 Proposed fix
if (totalBytes > MAX_PRICING_JSON_BYTES) { + reader.cancel().catch(() => {}) throw new Error(`Pricing data too large (exceeded ${MAX_PRICING_JSON_BYTES / 1024 / 1024} MB)`) }🤖 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 `@subtrack/src/pricing.ts` around lines 69 - 103, In the pricing JSON fetch/parsing flow, the size-limit failure path throws before the response stream is explicitly released. Update the reader loop in the logic that uses MAX_PRICING_JSON_BYTES so that when totalBytes exceeds the limit, the code first cancels the reader (and then raises the same error) before exiting. Keep the cleanup consistent with the existing timeout handling and finally block around the fetch/read path.
🤖 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 @.github/dependabot.yml:
- Around line 1-33: The npm Dependabot entry in the dependabot config still
overlaps with Renovate for the same subtrack dependencies, which can create
duplicate patch/minor PRs. Update the existing npm update rule in the dependabot
configuration so it is scoped away from the dependencies Renovate already
manages, using the relevant updates entry for the /subtrack package-ecosystem
block and its grouping settings.
In `@subtrack/src/usage-import.ts`:
- Around line 141-162: The stdin timeout handling in usage-import.ts is
terminating the whole process directly from the setTimeout callback, which makes
the read path abrupt and hard to test. Update the stdin read logic around the
timeout timer and the for await loop over process.stdin so the callback stops
the stream instead of calling process.exit(1), allowing the loop to throw and be
handled by the existing try/finally (and any caller-level catch). Keep the
behavior localized to the stdin-reading block and preserve the clear timeout
error message via consola.error.
---
Outside diff comments:
In `@subtrack/src/cursor-scanner.ts`:
- Around line 104-119: The new sanitize guard in cursor-scanner’s table lookup
is unreachable because cursorTableName/tableName is derived only from the fixed
knownTables list, so the extra includes check can never fail. Remove the
redundant runtime validation (or replace it with a short comment) in
cursor-scanner’s table-name selection logic so the code accurately reflects that
the safe derivation already prevents untrusted table names from being used.
In `@subtrack/src/usage-import.ts`:
- Around line 172-190: Avoid reopening the target file in usage-import.ts: the
current statSync(safeFile) followed by readFileSync(safeFile, "utf-8") can be
raced if the file is replaced between calls. Update the file-reading flow in the
usage-import logic to open the file once and perform both the size check and
content read through the same file descriptor, keeping the existing
MAX_FILE_SIZE, safeFile, and related error handling paths intact.
In `@subtrack/src/windsurf-scanner.ts`:
- Around line 96-111: Remove the dead-code sanitization guard in
windsurf-scanner.ts: tableName is already constrained by knownTables.find(...)
to a fixed allowlist, so the subsequent !knownTables.includes(tableName) branch
in the scan flow is unreachable. Keep the existing knownTables lookup and the
no-table-found early return, and delete the redundant suspicious-table warning
check to match the cleanup done in cursor-scanner.ts.
---
Nitpick comments:
In `@subtrack/package.json`:
- Around line 37-39: The pnpm onlyBuiltDependencies allowlist is currently empty
and should be made intentionally documented. Update the package.json pnpm
configuration around onlyBuiltDependencies to keep the empty list explicit, and
add a short note/comment nearby explaining that any future dependency with
install/postinstall scripts must be added there.
In `@subtrack/src/pricing.ts`:
- Around line 69-103: In the pricing JSON fetch/parsing flow, the size-limit
failure path throws before the response stream is explicitly released. Update
the reader loop in the logic that uses MAX_PRICING_JSON_BYTES so that when
totalBytes exceeds the limit, the code first cancels the reader (and then raises
the same error) before exiting. Keep the cleanup consistent with the existing
timeout handling and finally block around the fetch/read path.
🪄 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: 7d8f3590-ac7e-440d-b35d-c572bfcd3bff
📒 Files selected for processing (8)
.github/dependabot.yml.github/workflows/release.ymlsubtrack/package.jsonsubtrack/src/crypto.tssubtrack/src/cursor-scanner.tssubtrack/src/pricing.tssubtrack/src/usage-import.tssubtrack/src/windsurf-scanner.ts
| # Dependabot configuration for automated dependency updates. | ||
| # Also managed by Renovate (GitHub App). Dependabot provides | ||
| # additional security advisories and PR labeling. | ||
|
|
||
| version: 2 | ||
| updates: | ||
| - package-ecosystem: "npm" | ||
| directory: "/" | ||
| schedule: | ||
| interval: "weekly" | ||
| day: "monday" | ||
| time: "09:00" | ||
| timezone: "Asia/Tokyo" | ||
| open-pull-requests-limit: 5 | ||
| labels: | ||
| - "dependencies" | ||
| - "security" | ||
| # Only security updates — regular version bumps are handled by Renovate | ||
| allow: | ||
| - dependency-type: "all" | ||
| reviewers: | ||
| - "nazozokc" | ||
| assignees: | ||
| - "nazozokc" | ||
|
|
||
| - package-ecosystem: "npm" | ||
| directory: "/subtrack" | ||
| schedule: | ||
| interval: "weekly" | ||
| day: "monday" | ||
| time: "09:00" | ||
| timezone: "Asia/Tokyo" | ||
| open-pull-requests-limit: 5 | ||
| versioning-strategy: increase | ||
| labels: | ||
| - "dependencies" | ||
| - "security" | ||
| allow: | ||
| - dependency-type: "all" | ||
| reviewers: | ||
| - "nazozokc" | ||
| assignees: | ||
| - "nazozokc" | ||
| - "npm" | ||
| open-pull-requests-limit: 10 | ||
| commit-message: | ||
| prefix: "chore" | ||
| prefix-development: "chore" | ||
| groups: | ||
| production-dependencies: | ||
| dependency-type: "production" | ||
| update-types: | ||
| - "patch" | ||
| - "minor" | ||
| dev-dependencies: | ||
| dependency-type: "development" | ||
| update-types: | ||
| - "patch" | ||
| - "minor" | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if a root package.json exists with dependencies
fd -HI '^package\.json$' -d 1 . && cat package.json 2>/dev/null | jq '{dependencies, devDependencies}'Repository: nazozokc/subtrack
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dependabot =="
cat -n .github/dependabot.yml
echo
echo "== renovate files =="
fd -HI 'renovate.*(\.json|\.json5|\.js|\.mjs|\.cjs)?$' .github . 2>/dev/null || true
echo
echo "== renovate config snippets =="
for f in $(fd -HI 'renovate.*(\.json|\.json5|\.js|\.mjs|\.cjs)?$' .github . 2>/dev/null); do
echo "--- $f ---"
cat -n "$f"
doneRepository: nazozokc/subtrack
Length of output: 15728
Avoid overlapping npm updates
The root package.json is empty, so dropping directory: "/" is fine. The /subtrack npm entry still overlaps with Renovate, so both bots will open patch/minor PRs for the same dependencies unless one side is scoped out.
🤖 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 @.github/dependabot.yml around lines 1 - 33, The npm Dependabot entry in the
dependabot config still overlaps with Renovate for the same subtrack
dependencies, which can create duplicate patch/minor PRs. Update the existing
npm update rule in the dependabot configuration so it is scoped away from the
dependencies Renovate already manages, using the relevant updates entry for the
/subtrack package-ecosystem block and its grouping settings.
- cursor-scanner.ts: remove dead-code table-name guard (already safe by construction) - windsurf-scanner.ts: same cleanup - usage-import.ts: fix TOCTOU race (open file once for stat+read via fd) - usage-import.ts: replace process.exit(1) with stdin.destroy() in timeout - pricing.ts: add reader.cancel() before throw on size limit
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@subtrack/src/usage-import.ts`:
- Around line 189-191: The file-reading logic in usage-import should not use
readSync plus Buffer.alloc because a short read can leave zero-filled bytes in
content and corrupt parsing. Update the read path in usage-import.ts to read the
file descriptor directly with readFileSync(fd, "utf-8") and assign that result
to content, keeping the surrounding logic in place.
🪄 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: 0ee916d2-5dbc-45d8-bfe1-ed5663d589f8
📒 Files selected for processing (4)
subtrack/src/cursor-scanner.tssubtrack/src/pricing.tssubtrack/src/usage-import.tssubtrack/src/windsurf-scanner.ts
✅ Files skipped from review due to trivial changes (2)
- subtrack/src/cursor-scanner.ts
- subtrack/src/windsurf-scanner.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- subtrack/src/pricing.ts
| const buffer = Buffer.alloc(st.size) | ||
| readSync(fd, buffer, 0, st.size, 0) | ||
| content = buffer.toString("utf-8") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js fs.readSync partial read regular file may return fewer bytes than requested
💡 Result:
Yes, the Node.js fs.readSync function (and its asynchronous counterpart fs.read) can return fewer bytes than the length requested [1]. This behavior is expected and is not a bug [1]. ### Explanation When you call fs.readSync, Node.js acts as a wrapper around the underlying operating system's read system call (e.g., read() or pread() on POSIX/Linux) [2][1]. These system-level operations do not guarantee that they will fill the entire requested buffer in a single call [2][1]. Several scenarios can cause a partial read where bytesRead < length: * End of File (EOF): If the number of bytes remaining in the file is less than the requested length, the function will read only until the end of the file and return the number of bytes actually read [3]. * System-Level Limitations: Factors such as reading from slow network filesystems, interrupted system calls, or pipe/device-specific behaviors can cause the kernel to return fewer bytes than requested [2][1]. ### How to Handle Partial Reads Because fs.readSync does not automatically loop to fill the buffer, your application code must handle the possibility of partial reads if your logic strictly requires a specific amount of data [1]. If you need to ensure a buffer is completely filled (or until EOF is reached), you should wrap fs.readSync in a loop that continues to read until the desired number of bytes has been reached or the end of the file is signaled (indicated by bytesRead === 0) [2][1]. Example of correct loop implementation: javascript const fs = require('fs'); const fd = fs.openSync(path, 'r'); const buffer = Buffer.alloc(BUFFER_SIZE); let totalBytesRead = 0; while (totalBytesRead < BUFFER_SIZE) { const bytesRead = fs.readSync(fd, buffer, totalBytesRead, BUFFER_SIZE - totalBytesRead); if (bytesRead === 0) break; // EOF reached totalBytesRead += bytesRead; } fs.closeSync(fd);
Citations:
- 1: What does
fs.read'slengthargument indicate? nodejs/node#52447 - 2: https://stackoverflow.com/questions/72321444/does-node-js-guarantee-that-fs-read-always-fills-the-buffer
- 3: https://stackoverflow.com/questions/51032857/reading-data-a-block-at-a-time-synchronously
Use readFileSync(fd, "utf-8") here. readSync can return fewer bytes than requested, so decoding the zero-filled tail can corrupt content and break parsing. Reading directly from the fd avoids the partial-read hazard.
🤖 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 `@subtrack/src/usage-import.ts` around lines 189 - 191, The file-reading logic
in usage-import should not use readSync plus Buffer.alloc because a short read
can leave zero-filled bytes in content and corrupt parsing. Update the read path
in usage-import.ts to read the file descriptor directly with readFileSync(fd,
"utf-8") and assign that result to content, keeping the surrounding logic in
place.
The monthly and yearly date comparison used strict greater-than (>), so when today is exactly the billing day, the next date was incorrectly pushed to the following month/year. Use >= to correctly return today when it matches the billing day. Fixes the pre-existing test failure: 'handleNotify dry-run shows upcoming bills'
1 participant
概要
サプライチェーンセキュリティとアプリケーションセキュリティを包括的に診断・改善。あわせてドキュメントの不足を全補完。
変更内容
サプライチェーンセキュリティ
アプリケーションセキュリティ
>→>=に修正（同日請求のバグfix）ドキュメント全補完
テスト
pnpm test: 396 passed ✅（既存不具合も解消）pnpm lint:types: OK ✅pnpm build: OK ✅docs build: OK ✅