feat: add demo user client with intentional review smells #2

2026-08-06T14:04:59-03:00

andreferraro commented

2026-08-06 14:04:59 -03:00

User description

Summary

Adds user_client.py with intentional smells (hardcoded secret, mutable default, bare except, eval) to validate PR-Agent auto /review on PR open.

Test plan

PR-Agent posts review automatically (no manual /review)
Sonar job runs on the PR

PR Type

Enhancement, Other

Description

Add demo module with intentional code smells
Hardcoded API key, mutable default, bare except, eval
Unused helper function with dead code

Diagram Walkthrough

flowchart LR
  A["user_client.py"] --> B["Hardcoded API_KEY"]
  A --> C["fetch_user()"]
  C --> D["Mutable default cache"]
  C --> E["String injection URL"]
  C --> F["Bare except"]
  C --> G["eval() on remote data"]
  A --> H["unused_helper()"]
  H --> I["Dead code (unused z)"]

File Walkthrough

Relevant files

Enhancement

user_client.py

Demo module with intentional code smells

user_client.py

Adds API_KEY hardcoded secret
Implements fetch_user() with mutable default, bare except, eval, and
injection-prone URL
Adds unused_helper() with dead code (unused variable z)

+38/-0

### **User description** ## Summary - Adds `user_client.py` with intentional smells (hardcoded secret, mutable default, bare except, `eval`) to validate PR-Agent auto `/review` on PR open. ## Test plan - [ ] PR-Agent posts review automatically (no manual `/review`) - [ ] Sonar job runs on the PR ___ ### **PR Type** Enhancement, Other ___ ### **Description** - Add demo module with intentional code smells - Hardcoded API key, mutable default, bare except, eval - Unused helper function with dead code ___ ### Diagram Walkthrough ```mermaid flowchart LR A["user_client.py"] --> B["Hardcoded API_KEY"] A --> C["fetch_user()"] C --> D["Mutable default cache"] C --> E["String injection URL"] C --> F["Bare except"] C --> G["eval() on remote data"] A --> H["unused_helper()"] H --> I["Dead code (unused z)"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>user_client.py</strong><dd><code>Demo module with intentional code smells</code>                                  </dd></summary> <hr> user_client.py <ul><li>Adds <code>API_KEY</code> hardcoded secret<br> <li> Implements <code>fetch_user()</code> with mutable default, bare except, eval, and <br>injection-prone URL<br> <li> Adds <code>unused_helper()</code> with dead code (unused variable <code>z</code>)</ul> </details> </td> <td><a href="https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/src/branch/feature/demo-pr-agent-findings/user_client.py">+38/-0</a>    </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

andreferraro added 1 commit 2026-08-06 14:04:59 -03:00

feat: add demo user client with intentional review smells

sonar / sonar (push) Skipped

sonar / sonar (pull_request) Failing after 27s

Details

1b122b5ed0

Co-authored-by: Cursor <cursoragent@cursor.com>

andreferraro commented

2026-08-06 14:07:32 -03:00

PR Reviewer Guide 🔍

(Review updated until commit `55c5e370e1`)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 Security concerns

Yes. Hardcoded API key (line 8) exposes credentials. eval() on line 28 allows arbitrary code execution from remote data. String concatenation for URL (line 17) enables injection attacks. Bare except (line 22) hides errors and may mask security issues.

⚡ Recommended focus areas for review

Security Vulnerability

Hardcoded API key sk-live-ticketlab-demo-KEY_NOT_FOR_PROD_9f3a2c is exposed in source code. This could lead to unauthorized access if the repository is public or accessed by unauthorized personnel.

API_KEY = "sk-live-ticketlab-demo-KEY_NOT_FOR_PROD_9f3a2c"

Code Injection

eval(data) on line 28 executes arbitrary code from the HTTP response. If an attacker controls the response content, they can execute arbitrary Python code, leading to remote code execution.

payload = eval(data)  # nosec B307 — intentional for PR-Agent

Mutable Default Argument

The cache parameter defaults to a mutable {} dictionary. All calls to fetch_user() share the same cache object, causing unexpected state persistence across calls.

def fetch_user(user_id: str, cache: dict = {}):  # noqa: B006 — intentional mutable default

## PR Reviewer Guide 🔍 #### (Review updated until commit https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/commit/55c5e370e136554db1b1fd19e62ac4918d3e5c0d) Here are some key observations to aid the review process: <table> <tr><td>⏱️ <strong>Estimated effort to review</strong>: 2 🔵🔵⚪⚪⚪</td></tr> <tr><td>🧪 <strong>No relevant tests</strong></td></tr> <tr><td>🔒 <strong>Security concerns</strong><br><br> Yes. Hardcoded API key (line 8) exposes credentials. `eval()` on line 28 allows arbitrary code execution from remote data. String concatenation for URL (line 17) enables injection attacks. Bare except (line 22) hides errors and may mask security issues.</td></tr> <tr><td>⚡ <strong>Recommended focus areas for review</strong><br><br> <details><summary><a href='https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/src/branch/feature/demo-pr-agent-findings/user_client.py#L8-L8'><strong>Security Vulnerability</strong></a> Hardcoded API key `sk-live-ticketlab-demo-KEY_NOT_FOR_PROD_9f3a2c` is exposed in source code. This could lead to unauthorized access if the repository is public or accessed by unauthorized personnel. </summary> ```python API_KEY = "sk-live-ticketlab-demo-KEY_NOT_FOR_PROD_9f3a2c" ``` </details> <details><summary><a href='https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/src/branch/feature/demo-pr-agent-findings/user_client.py#L28-L28'><strong>Code Injection</strong></a> `eval(data)` on line 28 executes arbitrary code from the HTTP response. If an attacker controls the response content, they can execute arbitrary Python code, leading to remote code execution. </summary> ```python payload = eval(data) # nosec B307 — intentional for PR-Agent ``` </details> <details><summary><a href='https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/src/branch/feature/demo-pr-agent-findings/user_client.py#L11-L11'><strong>Mutable Default Argument</strong></a> The `cache` parameter defaults to a mutable `{}` dictionary. All calls to `fetch_user()` share the same cache object, causing unexpected state persistence across calls. </summary> ```python def fetch_user(user_id: str, cache: dict = {}): # noqa: B006 — intentional mutable default ``` </details> </td></tr> </table>

andreferraro closed this pull request

2026-08-06 14:07:48 -03:00

andreferraro reopened this pull request

2026-08-06 14:07:50 -03:00

andreferraro commented

2026-08-06 14:09:24 -03:00

PR Code Suggestions ✨

Latest suggestions up to 55c5e37
Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Replace eval with safe parsing Using `eval()` on remote content is extremely dangerous and can lead to arbitrary code execution. Replace with `json.loads()` to safely parse JSON data. user_client.py [28] `-payload = eval(data) # nosec B307 — intentional for PR-Agent +payload = json.loads(data)` Suggestion importance[1-10]: 10 __ Why: Using `eval()` on remote content is a severe security risk (arbitrary code execution). The suggestion correctly recommends `json.loads()` as a safe alternative, making this a critical fix with maximum impact.	High
Security	Prevent URL injection via encoding String concatenation for URL construction is vulnerable to injection attacks. Use `urllib.parse.urlencode()` to safely encode query parameters and prevent path injection. user_client.py [17] `-url = "https://example.com/api/users?id=" + user_id + "&key=" + API_KEY +params = urllib.parse.urlencode({"id": user_id, "key": API_KEY}) +url = f"https://example.com/api/users?{params}"` Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical security vulnerability (URL injection via string concatenation) and provides a safe alternative using `urllib.parse.urlencode()`. This is a high-impact security fix.	High
Possible issue	Handle exceptions specifically and log Bare `except Exception` catches all exceptions silently, hiding errors and making debugging difficult. Catch specific exceptions like `urllib.error.URLError` and log the error instead of swallowing it. user_client.py [22-24] `-except Exception: - # Intentional: bare except + swallow +except urllib.error.URLError as e: + # Log the error for debugging + print(f"Failed to fetch user data: {e}") data = None` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies the issue of a bare exception that silently swallows errors. While important for debugging and maintainability, it is less critical than the security issues addressed in suggestions 1 and 2.	Medium

Previous suggestions

Suggestions up to commit 55c5e37

Category	Suggestion	Impact
Security	Replace eval with safe parsing Using eval() on remote data is extremely dangerous and can execute arbitrary code. Replace with a safe JSON parser like json.loads() to parse the response data. user_client.py [28] `-payload = eval(data) # nosec B307 — intentional for PR-Agent +import json +payload = json.loads(data)` Suggestion importance[1-10]: 10 __ Why: Using `eval()` on remote data is a severe security risk that can lead to arbitrary code execution. Replacing it with `json.loads()` is essential and correctly addresses the vulnerability.	High
Security	Prevent injection via URL encoding The URL is constructed using string concatenation with user input, which can lead to injection attacks. Use parameterized queries or URL encoding to safely handle the user_id parameter. user_client.py [17] `-url = "https://example.com/api/users?id=" + user_id + "&key=" + API_KEY +import urllib.parse +params = urllib.parse.urlencode({"id": user_id, "key": API_KEY}) +url = f"https://example.com/api/users?{params}"` Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical security vulnerability (injection via string concatenation) and provides a safe alternative using URL encoding. This is a high-impact issue.	High
General	Avoid silent exception swallowing Catching a bare Exception and silently swallowing the error hides potential issues. Log the exception or at least handle specific exceptions to improve debuggability. user_client.py [22-24] `-except Exception: - # Intentional: bare except + swallow +except Exception as e: + # Log the error for debugging + print(f"Error fetching user data: {e}") data = None` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out the issue of a bare `except Exception` that silently swallows errors, which hinders debugging. The proposed improvement adds logging, which is a good practice, though the impact is moderate compared to the security issues.	Medium

## PR Code Suggestions ✨  Latest suggestions up to 55c5e37 Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=2>Security</td> <td> <details><summary>Replace eval with safe parsing</summary> ___ **Using <code>eval()</code> on remote content is extremely dangerous and can lead to arbitrary code <br>execution. Replace with <code>json.loads()</code> to safely parse JSON data.** [user_client.py [28]](https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/src/branch/feature/demo-pr-agent-findings/user_client.py#L28-L28) ```diff -payload = eval(data) # nosec B307 — intentional for PR-Agent +payload = json.loads(data) ``` <details><summary>Suggestion importance[1-10]: 10</summary> __ Why: Using `eval()` on remote content is a severe security risk (arbitrary code execution). The suggestion correctly recommends `json.loads()` as a safe alternative, making this a critical fix with maximum impact. </details></details></td><td align=center>High </td></tr><tr><td> <details><summary>Prevent URL injection via encoding</summary> ___ **String concatenation for URL construction is vulnerable to injection attacks. Use <br><code>urllib.parse.urlencode()</code> to safely encode query parameters and prevent path <br>injection.** [user_client.py [17]](https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/src/branch/feature/demo-pr-agent-findings/user_client.py#L17-L17) ```diff -url = "https://example.com/api/users?id=" + user_id + "&key=" + API_KEY +params = urllib.parse.urlencode({"id": user_id, "key": API_KEY}) +url = f"https://example.com/api/users?{params}" ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a critical security vulnerability (URL injection via string concatenation) and provides a safe alternative using `urllib.parse.urlencode()`. This is a high-impact security fix. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Handle exceptions specifically and log</summary> ___ **Bare <code>except Exception</code> catches all exceptions silently, hiding errors and making <br>debugging difficult. Catch specific exceptions like <code>urllib.error.URLError</code> and log <br>the error instead of swallowing it.** [user_client.py [22-24]](https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/src/branch/feature/demo-pr-agent-findings/user_client.py#L22-L24) ```diff -except Exception: - # Intentional: bare except + swallow +except urllib.error.URLError as e: + # Log the error for debugging + print(f"Failed to fetch user data: {e}") data = None ``` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies the issue of a bare exception that silently swallows errors. While important for debugging and maintainability, it is less critical than the security issues addressed in suggestions 1 and 2. </details></details></td><td align=center>Medium </td></tr></tr></tbody></table> ___ #### Previous suggestions <details><summary>Suggestions up to commit 55c5e37</summary> <br><table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=2>Security</td> <td> <details><summary>Replace eval with safe parsing</summary> ___ **Using eval() on remote data is extremely dangerous and can execute arbitrary code. <br>Replace with a safe JSON parser like json.loads() to parse the response data.** [user_client.py [28]](https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/src/branch/feature/demo-pr-agent-findings/user_client.py#L28-L28) ```diff -payload = eval(data) # nosec B307 — intentional for PR-Agent +import json +payload = json.loads(data) ``` <details><summary>Suggestion importance[1-10]: 10</summary> __ Why: Using `eval()` on remote data is a severe security risk that can lead to arbitrary code execution. Replacing it with `json.loads()` is essential and correctly addresses the vulnerability. </details></details></td><td align=center>High </td></tr><tr><td> <details><summary>Prevent injection via URL encoding</summary> ___ **The URL is constructed using string concatenation with user input, which can lead to <br>injection attacks. Use parameterized queries or URL encoding to safely handle the <br>user_id parameter.** [user_client.py [17]](https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/src/branch/feature/demo-pr-agent-findings/user_client.py#L17-L17) ```diff -url = "https://example.com/api/users?id=" + user_id + "&key=" + API_KEY +import urllib.parse +params = urllib.parse.urlencode({"id": user_id, "key": API_KEY}) +url = f"https://example.com/api/users?{params}" ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a critical security vulnerability (injection via string concatenation) and provides a safe alternative using URL encoding. This is a high-impact issue. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Avoid silent exception swallowing</summary> ___ **Catching a bare Exception and silently swallowing the error hides potential issues. <br>Log the exception or at least handle specific exceptions to improve debuggability.** [user_client.py [22-24]](https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/src/branch/feature/demo-pr-agent-findings/user_client.py#L22-L24) ```diff -except Exception: - # Intentional: bare except + swallow +except Exception as e: + # Log the error for debugging + print(f"Error fetching user data: {e}") data = None ``` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out the issue of a bare `except Exception` that silently swallows errors, which hinders debugging. The proposed improvement adds logging, which is a good practice, though the impact is moderate compared to the security issues. </details></details></td><td align=center>Medium </td></tr></tr></tbody></table> </details>

andreferraro commented

2026-08-06 14:09:40 -03:00

Persistent review updated to latest commit 55c5e370e1

**[Persistent review](https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/pulls/2#issuecomment-7)** updated to latest commit https://gitea.app.andreferraro.com/andreferraro/TicketLab_MCP/commit/55c5e370e136554db1b1fd19e62ac4918d3e5c0d

andreferraro closed this pull request

2026-08-06 14:23:04 -03:00

andreferraro deleted branch feature/demo-pr-agent-findings

2026-08-06 14:23:04 -03:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: andreferraro/TicketLab_MCP#2

feat: add demo user client with intentional review smells #2

User description

Summary

Test plan

PR Type

Description

Diagram Walkthrough

File Walkthrough

PR Reviewer Guide 🔍

(Review updated until commit 55c5e370e1)

PR Code Suggestions ✨

Previous suggestions

Pull request closed

(Review updated until commit `55c5e370e1`)