fix: patch 4 critical security vulnerabilities

[gn00295120]

Summary

This PR fixes 4 security vulnerabilities identified in the codebase:

• PLUGIN-01 — SQL Injection in postgres_datastore.py delete_by_filters
• PLUGIN-06 — Zip Slip path traversal in process_zip.py
• PLUGIN-03 — Hardcoded temp file path race condition in services/file.py
• PLUGIN-07 — Timing side-channel in bearer token comparison in server/main.py

Changes

1. SQL Injection in delete_by_filters (datastore/providers/postgres_datastore.py)

Before (vulnerable): Filter values interpolated directly into SQL via f-strings.

filters += f" document_id = '{filter.document_id}' AND"
# ...
cur.execute(f"DELETE FROM {table} {filters}")

After (fixed): All filter values use parameterized %s placeholders passed via a params list.

filters += " document_id = %s AND"
params.append(filter.document_id)
# ...
cur.execute(f"DELETE FROM {table} {filters}", params)

The table name remains from config (not user input). All six user-controllable filter fields (document_id, source, source_id, author, start_date, end_date) are now parameterized.

2. Zip Slip path traversal (scripts/process_zip/process_zip.py)

Before (vulnerable): zip_file.extractall("dump") with no member path validation.

After (fixed): Added _safe_extract() that resolves each member's real path and rejects any that escape the target directory:

def _safe_extract(zip_file, target_dir):
    target_dir = os.path.realpath(target_dir)
    for member in zip_file.namelist():
        member_path = os.path.realpath(os.path.join(target_dir, member))
        if not member_path.startswith(target_dir + os.sep) and member_path != target_dir:
            raise ValueError(f"Attempted path traversal in zip: {member}")
    zip_file.extractall(target_dir)

3. Hardcoded temp file path (services/file.py)

Before (vulnerable): All uploads wrote to the same /tmp/temp_file, creating a race condition (TOCTOU) and predictable path.

temp_file_path = "/tmp/temp_file"
with open(temp_file_path, "wb") as f:
    f.write(file_stream)

After (fixed): Uses tempfile.NamedTemporaryFile for unique, unpredictable temp files with correct extension:

with tempfile.NamedTemporaryFile(
    delete=False, suffix=os.path.splitext(file.filename or "")[1]
) as tmp:
    tmp.write(file_stream)
    temp_file_path = tmp.name

4. Timing side-channel in token comparison (server/main.py)

Before (vulnerable): Direct != comparison leaks token length/content via timing.

credentials.credentials != BEARER_TOKEN

After (fixed): Constant-time comparison via secrets.compare_digest:

not secrets.compare_digest(credentials.credentials, BEARER_TOKEN)

Test plan

• Verify delete_by_filters works with various filter combinations against a Postgres instance
• Verify zip extraction rejects archives containing ../../etc/passwd style paths
• Verify concurrent file uploads don't overwrite each other's temp files
• Verify valid bearer tokens still authenticate successfully
• Verify invalid bearer tokens are still rejected

[Copilot]

Pull request overview

This PR addresses multiple security vulnerabilities across the Postgres datastore, zip processing script, upload temp-file handling, and bearer token validation.

Changes:

• Parameterizes delete_by_filters SQL predicates to prevent SQL injection.
• Adds a safe zip extraction helper to block Zip Slip path traversal.
• Replaces a fixed /tmp upload path with NamedTemporaryFile for safer temp storage.
• Uses secrets.compare_digest to reduce timing side-channel leakage in token checks.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
datastore/providers/postgres_datastore.py	Switches delete_by_filters to parameterized predicates via %s + params list.
scripts/process_zip/process_zip.py	Introduces _safe_extract() to validate zip member paths before extraction.
services/file.py	Uses tempfile.NamedTemporaryFile instead of a hardcoded temp filepath for uploads.
server/main.py	Replaces direct token inequality with constant-time comparison (compare_digest).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

filters = filters[:-4] assumes at least one predicate was appended (ending in " AND"). If the caller passes a DocumentMetadataFilter with all fields unset, filters becomes "W" and the resulting SQL (DELETE FROM <table> W) will fail. Consider explicitly handling the empty-filter case (e.g., raise a clear exception / return early) rather than issuing an invalid query (and definitely avoid turning it into an unbounded DELETE).

[Copilot]

This change materially alters delete behavior and is security-sensitive, but there’s no test exercising delete_by_filters (especially for combinations of filters and the empty-filter case). Adding a targeted unit/integration test in the existing Postgres datastore test suite would help prevent regressions and confirm correct parameter binding.

[Copilot]

secrets.compare_digest will raise a TypeError if BEARER_TOKEN is None. Today that’s prevented via assert BEARER_TOKEN is not None, but asserts can be disabled (e.g., running Python with -O), which would turn a missing env var into a 500 instead of a deterministic startup failure or a 401. Prefer an explicit runtime check (raise a clear exception during startup / module import, or guard in validate_token) rather than relying on assert for required configuration.