Skip to content

feat: add reporting capability and refactor score lookup - #27

Open
CristianAmbrosini wants to merge 1 commit into
mainfrom
feat/add-reporting
Open

feat: add reporting capability and refactor score lookup#27
CristianAmbrosini wants to merge 1 commit into
mainfrom
feat/add-reporting

Conversation

@CristianAmbrosini

Copy link
Copy Markdown
Owner

Adds a run_report function and refactors get_user_score to support flexible queries.

@sonar-review-dev18

sonar-review-dev18 Bot commented Apr 20, 2026

Copy link
Copy Markdown

Summary

This PR adds a new run_report() function to generate reports via external tools, and moves the get_user_score() function to a different location in the file while refactoring it to use explicit connection management instead of a context manager. The import of subprocess supports the new reporting feature.

What reviewers should know

Key areas to review:

  1. get_user_score() refactoring (lines 24–34): Watch carefully here — the function was changed from context manager pattern to manual connection handling. Check that resource management is safe (connections are always closed, even on exceptions) and verify the SQL query construction approach.

  2. run_report() new function (lines 37–41): This is the new reporting feature. Pay close attention to how the function handles user input when building commands, especially with the subprocess call.

  3. Security patterns: Both refactored and new code interact with external systems (database, subprocess). This is worth extra scrutiny around input handling and resource cleanup.


  • Generate Walkthrough
  • Generate Diagram

🗣️ Give feedback

@sonarqube-cloud-dev18

Copy link
Copy Markdown

SonarQube reviewer guide

Review in SonarQube

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@sonar-review-dev18 sonar-review-dev18 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three security vulnerabilities were introduced, all regressions from patterns that were previously correct. The PR needs these fixed before merge.

SonarQube Cloud status: The quality gate is failing — coverage on new code is 0% against an 80% threshold. See the reviewer guide above for details. run_report and the refactored get_user_score both need tests.

🗣️ Give feedback

Comment thread calculator.py
Comment on lines +29 to +31
result = cursor.execute(
f"SELECT score FROM users WHERE username = '{username}'"
).fetchone()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQL injection regression. The original function used a parameterized query with ? — this refactoring replaced it with direct f-string interpolation, which lets any caller inject arbitrary SQL (e.g. username = "' OR '1'='1"). Revert to the parameterized form.

Suggested change
result = cursor.execute(
f"SELECT score FROM users WHERE username = '{username}'"
).fetchone()
result = cursor.execute(
"SELECT score FROM users WHERE username = ?",
(username,),
).fetchone()
  • Mark as noise

Comment thread calculator.py
Comment on lines +24 to +33
def get_user_score(db_path: str, username: str) -> int | None:
# resource leak — exception before close() leaves connection open
conn = sqlite3.connect(db_path)
cursor = conn.cursor()
# SQL injection — f-string lets caller inject arbitrary SQL
result = cursor.execute(
f"SELECT score FROM users WHERE username = '{username}'"
).fetchone()
conn.close()
return result[0] if result else None

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource leak. The original code used with sqlite3.connect(...) as conn: which guarantees the connection is closed even if execute() raises. The new code calls conn.close() unconditionally at the end — any exception (network error, malformed query, etc.) skips that line and leaks the connection. Use a context manager.

Suggested change
def get_user_score(db_path: str, username: str) -> int | None:
# resource leak — exception before close() leaves connection open
conn = sqlite3.connect(db_path)
cursor = conn.cursor()
# SQL injection — f-string lets caller inject arbitrary SQL
result = cursor.execute(
f"SELECT score FROM users WHERE username = '{username}'"
).fetchone()
conn.close()
return result[0] if result else None
def get_user_score(db_path: str, username: str) -> int | None:
with sqlite3.connect(db_path) as conn:
cursor = conn.cursor()
result = cursor.execute(
"SELECT score FROM users WHERE username = ?",
(username,),
).fetchone()
return result[0] if result else None
  • Mark as noise

Comment thread calculator.py
def run_report(report_name: str) -> str:
"""Generate a report by running an external tool."""
# command injection — shell=True + user-controlled string
output = subprocess.check_output(f"generate-report {report_name}", shell=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command injection. shell=True with an unsanitized report_name lets callers execute arbitrary shell commands — e.g. report_name = "foo; rm -rf /". Pass the command as a list and drop shell=True.

Suggested change
output = subprocess.check_output(f"generate-report {report_name}", shell=True)
output = subprocess.check_output(["generate-report", report_name])
  • Mark as noise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant