security: ensure COUNT(*) selects have restricted format#3765
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change tightens SQL validation by updating Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/assertSafeQuery.test.ts`:
- Line 47: Replace the fragile test entry in test/unit/assertSafeQuery.test.ts
with a query that isolates the COUNT alias check (e.g. "SELECT COUNT(*) as count
FROM _content_content") so it only exercises the COUNT(...) as count restriction
and cannot fail for unrelated multi-column/select-from issues, and ensure the
test mapping line ends with the required trailing comma to satisfy linting;
target the test case string that currently contains "COUNT(*) as c" and update
it to use "COUNT(...) as count" and include the trailing comma.
🪄 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: 071c5dd8-fa66-4819-a292-c3d7a187cd65
📒 Files selected for processing (2)
src/runtime/internal/security.tstest/unit/assertSafeQuery.test.ts
| 'SELECT * FROM _content_test WHERE (1=\' \\\' OR id IN (SELECT id FROM _content_docs) OR 1!=\'\') ORDER BY id ASC': false, | ||
| 'SELECT "id", "id" FROM _content_docs WHERE (1=\' \\\') UNION SELECT tbl_name,tbl_name FROM sqlite_master-- \') ORDER BY id ASC': false, | ||
| 'SELECT "id" FROM _content_test WHERE (x=$\'$ OR x IN (SELECT BLAH) OR x=$\'$) ORDER BY id ASC': false, | ||
| 'SELECT COUNT(*) as c,(SELECT group_concat(name,\'|\') FROM sqlite_master WHERE type=\'table\') AS leak FROM _content_content ORDER BY stem ASC': false |
There was a problem hiding this comment.
Line 47 test does not isolate the COUNT-alias guard (and currently fails lint).
This case can throw for unrelated reasons (FROM _content_content mismatch and multi-column select), so it doesn’t prove the new COUNT(...) as count restriction. It also misses the required trailing comma, which is failing CI lint.
🔧 Suggested fix
- 'SELECT COUNT(*) as c,(SELECT group_concat(name,\'|\') FROM sqlite_master WHERE type=\'table\') AS leak FROM _content_content ORDER BY stem ASC': false
+ 'SELECT COUNT(*) as c FROM _content_test ORDER BY stem ASC': false,
+ 'SELECT COUNT(*) as count FROM _content_test ORDER BY stem ASC': true,📝 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.
| 'SELECT COUNT(*) as c,(SELECT group_concat(name,\'|\') FROM sqlite_master WHERE type=\'table\') AS leak FROM _content_content ORDER BY stem ASC': false | |
| 'SELECT COUNT(*) as c FROM _content_test ORDER BY stem ASC': false, | |
| 'SELECT COUNT(*) as count FROM _content_test ORDER BY stem ASC': true, |
🧰 Tools
🪛 GitHub Actions: ci
[error] 47-47: ESLint error from 'pnpm lint' (eslint .): 47:156 Missing trailing comma (@stylistic/comma-dangle)
🪛 GitHub Check: ubuntu
[failure] 47-47:
Missing trailing comma
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/assertSafeQuery.test.ts` at line 47, Replace the fragile test entry
in test/unit/assertSafeQuery.test.ts with a query that isolates the COUNT alias
check (e.g. "SELECT COUNT(*) as count FROM _content_content") so it only
exercises the COUNT(...) as count restriction and cannot fail for unrelated
multi-column/select-from issues, and ensure the test mapping line ends with the
required trailing comma to satisfy linting; target the test case string that
currently contains "COUNT(*) as c" and update it to use "COUNT(...) as count"
and include the trailing comma.
commit: |
1 participant
🔗 Linked issue
❓ Type of change
📚 Description
📝 Checklist