[Security/Bug]: SQL Injection vulnerability in dynamic UPDATE queries via unvalidated column names

[Muneerali199]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

Description

During a recent static security analysis of the pictopy repository, a genuine SQL Injection (CWE-89) risk was identified in the backend's database update logic.

While the values in the SQL queries are safely parameterized, the column names are being injected directly into the SQL string using Python f-strings and .join(). Because parameterization (?) only protects values and not identifiers (like table or column names), this creates an injection vector if the column names are derived from untrusted user input (e.g., parsed directly from a JSON request payload).

Vulnerability Details

File: backend/app/database/face_clusters.py (and potentially other files using dynamic updates)
Lines: ~223-225

cursor.execute(
    f"UPDATE face_clusters SET {', '.join(update_fields)} WHERE cluster_id = ?",
    update_values,
)

Attack Scenario:

If a malicious user sends a request payload where the JSON key (which populates update_fields) is manipulated—for example, {"cluster_name = 'hacked' --": "value"}—the resulting SQL query structure will be altered, bypassing the intended logic and allowing arbitrary database manipulation.
Expected Behavior
Column names in UPDATE queries must be strictly validated against a hardcoded allowlist before being dynamically formatted into the SQL string.
Proposed Solution
Implement an allowlist (e.g., ALLOWED_COLUMNS) and filter the requested update fields before constructing the query.
Here is an example of how this can be secured:

1. Define safe columns for this specific table

ALLOWED_COLUMNS = {"cluster_name", "face_image_base64", "is_hidden"}

safe_fields = []
safe_values =

Record

• I agree to follow this project's Code of Conduct

[github-actions]

👋 Thanks for opening this issue! The maintainers will review it shortly. Till then, do not open any PR. This is a strict rule we like to follow around here :)

[Riddhi8077]

@Muneerali199 Thanks for raising this security concern.

I reviewed the implementation of db_update_cluster in backend/app/database/face_clusters.py to understand how the UPDATE query is constructed.

From the current code, update_fields appears to be built entirely within the function and only includes the hardcoded expression "cluster_name = ?". The actual value for cluster_name is then passed separately through SQLite parameter binding:

cursor.execute(
f”UPDATE face_clusters SET {’, ’.join(update_fields)} WHERE cluster_id = ?”,
update_values,
)

Because the column identifier (cluster_name) is not derived from user input and the value itself is parameterized using ?, this specific query does not appear to expose a practical SQL injection vector.

Additionally, the request schema (RenameClusterRequest) only accepts a single cluster_name field, which further limits the request payload structure and prevents arbitrary column names from being supplied.

It is possible that the static analysis tool flagged the dynamic SQL pattern (f"... {', '.join(update_fields)}") as a precaution, even though the identifiers themselves are not user-controlled in the current implementation.

That said, if the update logic is expanded in the future to support multiple dynamically selected columns, introducing an explicit allowlist of valid column names could be a good defensive measure.

Curious to hear the @rahulharpal1603 maintainer's perspective on whether additional hardening would be desirable here.

[Muneerali199]

Hello @rahulharpal1603 , i think this issue needs to be fix and I create a pr also for this issue please assign me this issue so I can reopen the pr