Vulnerable sample for Action test#2
Conversation
WalkthroughAdds new file app.js defining two functions: runUserCode(code) executes eval on input; getUser(db, id) builds a SQL string using the id and calls db.query. Includes a comment noting eval and SQL concatenation. No additional logic, validation, or error handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant App as app.js
participant JS as eval(...)
participant DB as db.query(...)
rect rgb(240,248,255)
note over Caller,App: runUserCode flow
Caller->>App: runUserCode(code)
App->>JS: eval(code)
JS-->>App: result
App-->>Caller: result
end
rect rgb(245,255,250)
note over Caller,DB: getUser flow
Caller->>App: getUser(db, id)
App->>App: q = "SELECT * FROM users WHERE id = '<id>'"
App->>DB: db.query(q)
DB-->>App: rows
App-->>Caller: rows
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Comment |
🔐 Secure Code Review (AI)Risk Summary: High (2)
Safeguards Checklist:
The diff is small and focused, but it contains critical security issues that need immediate attention. Models can make mistakes. Verify before merging. |
Micro-Learning Topic: Injection attack (Detected by phrase)Matched on "Injection attack"Injection flaws, such as SQL, NoSQL, OS, and LDAP injection, occur when untrusted data is sent to an interpreter as part of a command or query. The attacker’s hostile data can trick the interpreter into executing unintended commands or accessing data without proper authorization. Source: https://www.owasp.org/index.php/Category:OWASP_Top_Ten_Project Try a challenge in Secure Code WarriorHelpful references
Micro-Learning Topic: SQL injection (Detected by phrase)Matched on "SQL Injection"This is probably one of the two most exploited vulnerabilities in web applications and has led to a number of high profile company breaches. It occurs when an application fails to sanitize or validate input before using it to dynamically construct a statement. An attacker that exploits this vulnerability will be able to gain access to the underlying database and view or modify data without permission. Try a challenge in Secure Code WarriorHelpful references
|
![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
🧹 Nitpick comments (1)
app.js (1)
1-6: Add guardrails to prevent regressions.
- Enforce no-eval in lints (Biome/ESLint) and block SQL string concatenation in queries.
- Add a simple unit test asserting the query is parameterized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app.js(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
app.js
[error] 2-2: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
| @@ -0,0 +1,6 @@ | |||
| // BAD: eval + SQLi | |||
| function runUserCode(code) { return eval(code); } | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Block eval and remove arbitrary code execution.
eval enables RCE and bypasses CSP; there’s no safe way to expose it here.
Apply this minimal hardening:
-function runUserCode(code) { return eval(code); }
+function runUserCode(/* code */) {
+ throw new Error("Disabled: eval is prohibited");
+}If you truly need expression support, I can replace this with a safe, whitelisted evaluator. Want me to draft that?
📝 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.
| function runUserCode(code) { return eval(code); } | |
| function runUserCode(/* code */) { | |
| throw new Error("Disabled: eval is prohibited"); | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 2-2: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🤖 Prompt for AI Agents
In app.js around line 2, the runUserCode function currently uses eval(code)
which allows remote code execution; remove the eval usage and stop executing
arbitrary strings. Replace it with one of these safe options: 1) if you only
need to evaluate simple expressions, use a whitelisted expression evaluator
(e.g., expression-eval or a small custom parser) and validate input against an
allowlist; 2) if you must run JS snippets, execute them in a strict sandboxed
environment (e.g., Node VM with time/memory limits or vm2) and explicitly
disallow globals and I/O; or 3) if execution isn’t required, throw an error and
reject the request. Ensure inputs are validated, limit execution time/memory,
and log/audit attempts. Implement the chosen change and remove the eval line.
| function getUser(db, id) { | ||
| const q = "SELECT * FROM users WHERE id = '" + id + "'"; | ||
| return db.query(q); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix SQL injection: use parameterized queries (and avoid quoting the parameter).
String concatenation of id is exploitable. Bind parameters and optionally constrain id.
MySQL/sqlite-style placeholders:
-function getUser(db, id) {
- const q = "SELECT * FROM users WHERE id = '" + id + "'";
- return db.query(q);
-}
+function getUser(db, id) {
+ const q = "SELECT * FROM users WHERE id = ? LIMIT 1";
+ return db.query(q, [id]);
+}Postgres-style placeholders:
-function getUser(db, id) {
- const q = "SELECT * FROM users WHERE id = '" + id + "'";
- return db.query(q);
-}
+function getUser(db, id) {
+ const q = "SELECT * FROM users WHERE id = $1 LIMIT 1";
+ return db.query(q, [id]);
+}To confirm which variant to use, run:
🏁 Script executed:
#!/bin/bash
# Detect DB driver and find db.query usages
rg -n -C2 '"(pg|mysql2|mssql|sqlite3|better-sqlite3|knex|sequelize|typeorm|prisma)"' package.json || true
rg -n -C3 --type=js --type=ts '\bdb\.query\s*\('Length of output: 204
Fix SQL injection: use parameterized queries
String concatenation of id is exploitable; bind parameters and constrain id. Location: app.js lines 3-6.
MySQL/sqlite-style placeholders:
-function getUser(db, id) {
- const q = "SELECT * FROM users WHERE id = '" + id + "'";
- return db.query(q);
-}
+function getUser(db, id) {
+ const q = "SELECT * FROM users WHERE id = ? LIMIT 1";
+ return db.query(q, [id]);
+}Postgres-style placeholders:
-function getUser(db, id) {
- const q = "SELECT * FROM users WHERE id = '" + id + "'";
- return db.query(q);
-}
+function getUser(db, id) {
+ const q = "SELECT * FROM users WHERE id = $1 LIMIT 1";
+ return db.query(q, [id]);
+}📝 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.
| function getUser(db, id) { | |
| const q = "SELECT * FROM users WHERE id = '" + id + "'"; | |
| return db.query(q); | |
| } | |
| function getUser(db, id) { | |
| const q = "SELECT * FROM users WHERE id = ? LIMIT 1"; | |
| return db.query(q, [id]); | |
| } |
| function getUser(db, id) { | |
| const q = "SELECT * FROM users WHERE id = '" + id + "'"; | |
| return db.query(q); | |
| } | |
| function getUser(db, id) { | |
| const q = "SELECT * FROM users WHERE id = $1 LIMIT 1"; | |
| return db.query(q, [id]); | |
| } |
🤖 Prompt for AI Agents
In app.js around lines 3-6, the current function builds the SQL via string
concatenation causing SQL injection; fix by using a parameterized query and
validating/constraining id before calling the DB. Ensure you switch the query to
use the appropriate placeholder for your driver (use ? for MySQL/SQLite or $1
for Postgres), pass id as a bound parameter array/params object to db.query, and
validate id (e.g., parseInt and verify numeric range or validate UUID format)
before executing so only valid ids are allowed.
1 participant
This PR intentionally adds risky patterns for testing ai-secure-code-review-action.
Summary by CodeRabbit
New Features
Chores