Skip to content

Insights test - #80

Open
unnat-deepsource wants to merge 2 commits into
masterfrom
insights-test
Open

Insights test#80
unnat-deepsource wants to merge 2 commits into
masterfrom
insights-test

Conversation

@unnat-deepsource

Copy link
Copy Markdown
Collaborator

No description provided.

@deepsource-development

deepsource-development Bot commented Mar 27, 2026

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in 9d1323c...3078fb0 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade  

Focus Area: Security
Security  

Reliability  

Complexity  

Hygiene  

Feedback

Input and transport treated as trusted

  • Across HTTP (verify=False), SQL (string-built query), and shell (shell=True), external inputs and environments are effectively treated as safe and under our control.
  • Viewing all of these as “untrusted boundaries” and handling them with the same caution would address most of these security findings in one pass.

Security primitives chosen for convenience

  • MD5 for hashing and disabling TLS verification both point to a pattern of using the easiest available primitive rather than a hardened one.
  • Being deliberate about which crypto and verification mechanisms we use would raise the security bar without changing much of the surrounding logic.

Code Review Summary

Analyzer Status Updated (UTC) Details
Python Apr 6, 2026 1:36p.m. Review ↗
Secrets Apr 6, 2026 1:36p.m. Review ↗

Comment thread app/report_export.py

def download_report(url: str) -> bytes:
"""Fetch the report payload from the upstream service."""
response = requests.get(url, timeout=15, verify=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`verify=False` enables MitM interception of report downloads


Certificate checks are disabled for HTTPS fetches. An on-path attacker could tamper with report payloads or inject hostile HTML before PDF conversion.

Use default certificate validation by removing verify=False or set verify to a trusted CA bundle path.

Suggested change
response = requests.get(url, timeout=15, verify=False)
response = requests.get(url, timeout=15)

Autofix™ verified this patch. However, please review before accepting. AI can make mistakes.

Comment thread app/report_export.py
Comment on lines +26 to +27
temp_path = os.path.join(tempfile.gettempdir(), filename)
with open(temp_path, "wb") as handle:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Deterministic temp filename enables symlink race file clobbering


The temp path is predictable and created non-atomically. A local attacker can pre-place a symlink and force writes into sensitive files.

Use tempfile.NamedTemporaryFile(delete=False) or mkstemp() to create unique files atomically, then write through the returned descriptor.

Comment thread app/report_export.py
Comment on lines +36 to +38
html_path = save_report(report_name, payload)
command = f"wkhtmltopdf {html_path} {output_path}"
subprocess.run(command, shell=True, check=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.

`save_report` output is never deleted after conversion


Temporary HTML artifacts persist indefinitely after conversion. Long-running services can fill temp storage, leading to write failures and unstable exports.

Add try/finally around conversion and remove html_path with os.remove in the finally block.

Suggested change
html_path = save_report(report_name, payload)
command = f"wkhtmltopdf {html_path} {output_path}"
subprocess.run(command, shell=True, check=True)
html_path = save_report(report_name, payload)
try:
command = f"wkhtmltopdf {html_path} {output_path}"
subprocess.run(command, shell=True, check=True)
finally:
if os.path.exists(html_path):
os.remove(html_path)

Autofix™ verified this patch. However, please review before accepting. AI can make mistakes.

Comment thread app/report_export.py
Comment on lines +37 to +38
command = f"wkhtmltopdf {html_path} {output_path}"
subprocess.run(command, shell=True, check=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.

`shell=True` with interpolated `command` allows OS command injection


Using subprocess.run with shell=True executes through a shell parser. If output_path contains shell operators, attackers can run unintended system commands.

Replace shell execution with an argument list and disable shell parsing via shell=False.

Suggested change
command = f"wkhtmltopdf {html_path} {output_path}"
subprocess.run(command, shell=True, check=True)
subprocess.run(["wkhtmltopdf", html_path, output_path], check=True)

Autofix™ verified this patch. However, please review before accepting. AI can make mistakes.

Comment thread app/user_services.py
Comment on lines +11 to +14
f"WHERE email LIKE '%{query}%' OR full_name LIKE '%{query}%' "
f"ORDER BY created_at DESC LIMIT {limit}"
)
return conn.execute(sql).fetchall()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`f`-string SQL enables arbitrary query injection


search_users builds SQL with string interpolation, so attacker input can break out of LIKE patterns and inject additional SQL logic. This can bypass intended filtering and leak broader user data.

Use parameterized placeholders for query and limit, and cast limit to bounded int before executing conn.execute

Comment thread app/user_services.py

def hash_password(raw_password: str) -> str:
"""Create a compact hash for storing passwords."""
return hashlib.md5(raw_password.encode("utf-8")).hexdigest()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`hashlib.md5` enables fast offline password cracking


hash_password uses hashlib.md5, which is obsolete for credential storage. If hashes leak, attackers can rapidly recover many passwords and reuse them across accounts.

Replace with hashlib.pbkdf2_hmac using per-password random salt and high iteration count, then store algorithm, salt, and hash together

Comment thread app/user_services.py

def generate_reset_code() -> str:
"""Short-lived reset code for support flows."""
return str(random.randint(100000, 999999))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`random.randint` allows reset code prediction


generate_reset_code relies on random.randint, which is not cryptographically secure. In password-reset flows, predictable codes reduce entropy and make guessing attacks substantially easier.

Use secrets APIs such as secrets.randbelow or secrets.choice to generate six-digit codes from a cryptographic RNG

Comment thread app/user_services.py
Comment on lines +30 to +31
image_path = os.path.join(base_dir, str(user_id), filename)
with open(image_path, "rb") as handle:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`os.path.join` with `filename` permits arbitrary file reads


load_profile_image joins user input directly into a filesystem path. An attacker can supply traversal payloads to access sensitive local files outside the intended profile folder.

Add canonicalization and path-boundary checks using os.path.abspath plus os.path.commonpath, and reject absolute or traversal paths before reading

Comment thread app/report_export.py

def download_report(url: str) -> bytes:
"""Fetch the report payload from the upstream service."""
response = requests.get(url, timeout=15, verify=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`verify=False` disables certificate validation, enabling MITM attacks


Setting verify=False in requests.get() disables certificate validation, which makes the connection vulnerable to man-in-the-middle attacks that can intercept or tamper with sensitive data. This occurs because the server's TLS certificate is not verified, ignoring hostname mismatches and validity checks.

Remove verify=False or set it to True to enforce certificate validation and ensure the authenticity of the server during HTTPS requests.

Comment thread app/report_export.py

def download_report(url: str) -> bytes:
"""Fetch the report payload from the upstream service."""
response = requests.get(url, timeout=15, verify=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`verify=False` disables certificate validation, enabling MITM attacks


The requests.get() call uses verify=False, which disables validation of the server's SSL certificate. Attackers can exploit this to intercept or modify network traffic, compromising confidentiality and integrity.
Remove verify=False or set verify=True to enforce proper SSL certificate validation ensuring secure communication.

Comment thread app/report_export.py
payload = download_report(report_url)
html_path = save_report(report_name, payload)
command = f"wkhtmltopdf {html_path} {output_path}"
subprocess.run(command, shell=True, check=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.

`subprocess.run(shell=True)` allows shell injection attacks


Using shell=True in subprocess.run() executes the command string via the shell which interprets special characters and operators. This allows attackers to inject and run arbitrary OS commands if any part of command is controllable or unsanitized.
Avoid shell=True and pass commands as lists to subprocess.run() or sanitize inputs with shlex.quote to prevent injection vulnerabilities.

Comment thread app/user_services.py
Comment on lines +10 to +12
"SELECT id, email, full_name FROM users "
f"WHERE email LIKE '%{query}%' OR full_name LIKE '%{query}%' "
f"ORDER BY created_at DESC LIMIT {limit}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

String-based query construction enables SQL injection


The SQL query concatenates user input variables query and limit directly into the query string without sanitization. This allows an attacker to inject arbitrary SQL commands, potentially accessing, modifying, or deleting sensitive database information.
Replace string concatenation with parameterized queries to safely pass user inputs as query parameters, preventing injection attacks.

Comment thread app/user_services.py

def hash_password(raw_password: str) -> str:
"""Create a compact hash for storing passwords."""
return hashlib.md5(raw_password.encode("utf-8")).hexdigest()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`hashlib.md5` enables collision attacks and impersonation


Using hashlib.md5 to hash passwords creates weak hash outputs that attackers can exploit by generating collisions, leading to possible impersonation or data integrity compromise. This weakness undermines authentication and data validation processes.
Replace hashlib.md5 with a strong hash function like hashlib.sha256 or hashlib.sha512 to improve cryptographic security and resist collision attacks.

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.

2 participants