Skip to content

[test] PR #668 scenario B — bot-only CR (should be ignored) #103

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

Closed
Eeshu-Yadav-1 wants to merge 2 commits into from
Closed

[test] PR #668 scenario B — bot-only CR (should be ignored) #103

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
1eae9b9
[test] Scenario B: bot-only CR for PR #668
Eeshu-Yadav-1 May 11, 2026
c925729
[test] Add intentionally bad code to provoke bot CR
Eeshu-Yadav-1 May 12, 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
1 change: 1 addition & 0 deletions TEST_PR_668_B.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test scenario B: bot-only CR should not trigger stale pipeline.

@coderabbitai coderabbitai Bot May 13, 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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use an H1 heading for the first line.

Line 1 should be a top-level markdown heading to satisfy MD041 and keep docs consistent.

Proposed fix 
-Test scenario B: bot-only CR should not trigger stale pipeline.
+# Test scenario B: bot-only CR should not trigger stale pipeline
📝 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.

Suggested change
Test scenario B: bot-only CR should not trigger stale pipeline.
# Test scenario B: bot-only CR should not trigger stale pipeline
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 1-1: First line in a file should be a top-level heading

(MD041, first-line-heading, first-line-h1)

🤖 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 `@TEST_PR_668_B.md` at line 1, The first line of TEST_PR_668_B.md is not a
top-level heading; change Line 1 to an H1 markdown heading (prefix with "# ") so
the file begins with a single top-level heading to satisfy MD041 and doc
consistency; update the first line content ("Test scenario B: bot-only CR should
not trigger stale pipeline.") to become "# Test scenario B: bot-only CR should
not trigger stale pipeline." and keep the rest unchanged.

35 changes: 35 additions & 0 deletions bad_code_for_review.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
"""Intentionally bad code for bot reviewers to flag."""

import os
import subprocess

# Hardcoded credentials — should trigger a security flag.
API_KEY = "sk_live_4242424242424242"
DATABASE_PASSWORD = "admin123"
Comment on lines +6 to +8

@coderabbitai coderabbitai Bot May 13, 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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove committed secrets from source control.

This adds a Stripe-like live key and a plaintext database password to the repository. Move both to injected runtime configuration, and if either value was ever real, rotate it immediately.

🔐 Minimal fix 
-# Hardcoded credentials — should trigger a security flag.
-API_KEY = "sk_live_4242424242424242"
-DATABASE_PASSWORD = "admin123"
+# Inject secrets at runtime instead of committing them.
+API_KEY = os.environ["API_KEY"]
+DATABASE_PASSWORD = os.environ["DATABASE_PASSWORD"]
🧰 Tools
🪛 Betterleaks (1.2.0)

[high] 7-7: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.

(stripe-access-token)

🤖 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 `@bad_code_for_review.py` around lines 6 - 8, The file contains hardcoded
secrets in API_KEY and DATABASE_PASSWORD; remove these literals and read both
values from injected runtime configuration (e.g., environment variables or a
secrets/credentials provider) instead, update any initialization code that
references API_KEY or DATABASE_PASSWORD to fetch process/runtime config (e.g.,
os.environ or your config service) and fail fast with a clear error if missing,
purge the committed secrets from the repo history and rotate them if they were
ever real, and ensure you add guidance/docs or .env.example (without real
values) so developers know which keys to supply.



def run_user_command(user_input):
# Command injection — passing user input to shell with shell=True.
return subprocess.check_output(user_input, shell=True)
Comment on lines +11 to +13

@coderabbitai coderabbitai Bot May 13, 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.

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

Do not execute raw user input via a shell.

Passing untrusted input to subprocess.check_output(..., shell=True) is direct command injection. This needs an allowlisted command map and shell=False, not a raw shell entrypoint.

🛡️ Safer shape 
+ALLOWED_COMMANDS = {
+    "list_tmp": ["ls", "/tmp"],
+}
+
-def run_user_command(user_input):
-    # Command injection — passing user input to shell with shell=True.
-    return subprocess.check_output(user_input, shell=True)
+def run_user_command(command_name):
+    return subprocess.check_output(ALLOWED_COMMANDS[command_name], shell=False)
🤖 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 `@bad_code_for_review.py` around lines 11 - 13, The run_user_command function
currently executes raw user input with subprocess.check_output(..., shell=True),
which allows command injection; replace this with an allowlisted command map and
call subprocess without shell (shell=False) using a list/sequence of arguments:
validate the user's requested command against an allowlist (e.g., a dict mapping
safe command names to fixed argument templates or executable paths), construct
the argv list deterministically (no string concatenation of user input), and
call subprocess.check_output(argv, shell=False); if the input is not
allowlisted, raise an error or return a safe failure response.



def fetch_user(user_id):
# SQL injection — string concatenation into a query.
query = "SELECT * FROM users WHERE id = " + str(user_id)
return query
Comment on lines +16 to +19

@coderabbitai coderabbitai Bot May 13, 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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Stop building tainted SQL strings.

Any caller that executes this returned query inherits SQL injection. Keep user data out of the SQL text and use bound parameters through the database driver instead of concatenation.

🤖 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 `@bad_code_for_review.py` around lines 16 - 19, The fetch_user function
currently builds a tainted SQL string via concatenation; change it to use
parameterized queries/bound parameters instead of embedding user_id into the SQL
text. Replace the string concatenation in fetch_user with a SQL statement using
a placeholder (e.g., SELECT * FROM users WHERE id = ?) and return or execute
that statement with the user_id supplied as a separate parameter (or use your DB
driver's execute method with the query plus params). Ensure no user input is
interpolated into the SQL string in fetch_user to prevent SQL injection.



def parse_config(data):
# eval on untrusted input.
return eval(data)
Comment on lines +22 to +24

@coderabbitai coderabbitai Bot May 13, 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.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Replace eval with a real parser.

eval(data) is arbitrary code execution, and the __main__ path feeds raw stdin straight into it, so exploitation is one prompt away. Parse a constrained format instead of executing the input.

Also applies to: 34-35

🤖 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 `@bad_code_for_review.py` around lines 22 - 24, The use of eval in
parse_config(data) is executing untrusted input; replace eval(data) with a
proper parser for a constrained format (e.g., json.loads or a strict YAML
parser) and validate the resulting structure, and make the same replacement for
the other occurrence referenced at lines 34-35; update the parse_config function
to return the parsed/validated dict and add input validation/error handling to
reject or sanitize unexpected types/fields.



def bare_except():
try:
os.remove("/etc/passwd")
except:
pass
Comment on lines +27 to +31

@coderabbitai coderabbitai Bot May 13, 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.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Delete the destructive /etc/passwd removal path.

This helper attempts to remove a system file and then suppresses every failure. If it ever runs with elevated privileges, it can damage the host while hiding the incident from logs and callers.

🧹 Simplest fix 
-def bare_except():
-    try:
-        os.remove("/etc/passwd")
-    except:
-        pass
📝 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.

Suggested change
def bare_except():
try:
os.remove("/etc/passwd")
except:
pass
🤖 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 `@bad_code_for_review.py` around lines 27 - 31, The function bare_except
contains a destructive call os.remove("/etc/passwd") and swallows all errors
with a bare except: pass; remove the hardcoded deletion immediately. If the
helper is meant to delete files, change bare_except to accept a validated path
parameter, validate against unsafe targets, call os.remove only on safe paths,
and replace the bare except with specific handlers (e.g., FileNotFoundError,
PermissionError) that either log the error or return a failure status instead of
silencing it; ensure you reference the bare_except function, the os.remove call,
and the bare except block when making the change.



if __name__ == "__main__":
parse_config(input("config: "))
Loading