Potential fix for code scanning alert no. 18: Use of a broken or weak cryptographic hashing algorithm on sensitive data

[ewlarson]

Potential fix for https://github.com/geobtaa/geospatial-api/security/code-scanning/18

In general terms, the fix is to avoid using a fast, general-purpose hash (SHA-256) for hashing secrets used as authenticators and instead use a password hashing/key derivation function that is intentionally slow and parameterizable (e.g., Argon2, bcrypt, scrypt, or PBKDF2). This makes offline brute-force attacks impractical if the stored hashes are leaked.

For this specific code, the best minimal-impact fix is to change hash_api_key to use Python’s standard-library PBKDF2 implementation (hashlib.pbkdf2_hmac) with sufficient iterations and a per-key random salt. Because we only see the hashing function and the write path in this snippet (and not the verify/lookup path), we must preserve the method signature and return type while improving security. We can do this by:

• Introducing a module-level secret/salt for PBKDF2 (or a per-key salt if the schema supports it; since we can’t change the schema here, we’ll use a module-level application-wide salt).
• Changing hash_api_key to derive a key using hashlib.pbkdf2_hmac with a high iteration count (e.g., 100,000+), and return the hex-encoded result.
• Keeping the hash_api_key(key: str) -> str interface so all existing callers (like the insert logic on lines 176–189) remain unchanged.

Concretely in backend/app/services/api_key_service.py:

• Add an import of os (for generating a module-level random salt) and define a constant API_KEY_HASH_SALT and iteration count near the top of the file, close to the logger definition.
• Replace the body of hash_api_key so it uses hashlib.pbkdf2_hmac("sha256", key.encode("utf-8"), API_KEY_HASH_SALT, API_KEY_HASH_ITERATIONS).hex() instead of hashlib.sha256(...).hexdigest().
• Leave all other logic, including API key generation, tier lookups, and DB writes, untouched.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

In general, the fix is to ensure that sensitive data (the API keys returned from create_api_key) are never written directly to logs or standard output in clear text. Non-sensitive metadata such as counts, IDs, and tier names can remain logged.

In this code, the sensitive flow is: APIKeyService.create_api_key() returns a dict that includes "api_key", regenerate_api_keys() stores that in new_api_key and in the "new_api_key" field of each migration record, and print_report() prints that field ("→ New API Key: {new_key}"). results['keys_to_regenerate'] itself is safe to print; the real problem and the reason for the taint is that results also contains the sensitive new_api_key fields. The minimal fix that preserves existing functionality while removing the clear-text exposure is:

• Stop printing the actual API key value in print_report. Instead, print only non-sensitive metadata (e.g., key IDs and tier, which are already there).
• Optionally, keep "new_api_key" available in the results structure so that calling code (if any) can decide how to handle it, but do not print it or log it. Since this script is stand-alone and only uses results for printing and exit codes, leaving the field in place but not printing it is functionally equivalent for the caller.
• No changes are needed in APIKeyService.create_api_key itself; it is reasonable for that service to return the key exactly once to trusted callers.

Concretely, in backend/scripts/regenerate_api_keys.py:

• In print_report, remove or comment out the line that prints new_key (" → New API Key: {new_key}"). We will simply delete that line. This removes the only direct clear-text logging of the actual API key in the script.
• Leave the lines that print counts (results['keys_to_regenerate'], etc.) as they are; they are safe and useful.

No new methods or imports are needed; this is just removing a single print of sensitive data.

In general, the problem should be fixed by ensuring that sensitive values (API keys) are never written to logs or stdout/stderr in clear text. Instead, only non-sensitive metadata (IDs, counts, tiers, truncated hashes, etc.) should be logged. Any structures that carry secrets (like migrations and results) must not be serialized, printed, or logged in a way that might emit those secrets, unless they are explicitly stripped or redacted first.

For this specific code, the best minimal fix is to change how the migration report prints details so that it no longer outputs the full new_api_key. We can still display IDs and counts for operational visibility, but omit or redact the actual API key. This addresses all variants because the sensitive taint originates from result["api_key"] / new_api_key in migrations, which is what makes results tainted; by ensuring that we never print the key itself in print_report, we remove the clear-text exposure. We do not need to change how results is constructed, only how it is rendered.

Concretely, in backend/scripts/regenerate_api_keys.py:

• In print_report, inside the status == "regenerated" branch, remove or replace the line:

  print(f"    → New API Key: {new_key}")

• Option A (most conservative): drop the line entirely so the API key is never printed.

• Option B (if you want to give a hint without exposing the secret): print only a redacted form, e.g., last 4 characters, clearly labeled as redacted.

Given the instructions to avoid logging sensitive data, Option A is the safest, but Option B still avoids logging the full key and is often acceptable in practice. I’ll implement Option B so operators can match a key they just copied, while making sure the full key never appears.

No additional imports or helpers are necessary; we can compute a simple redacted string inline.

---

General approach:

• Do not print the raw API key (new_api_key) directly in print_report.
• Avoid passing the raw API key through the same results/migrations structure used for general reporting and potentially logging.
• If we still need to expose new API keys for operators, provide a safer, more explicit mechanism (e.g., a separate “secrets output” structure the caller can decide how to handle) or make printing keys opt‑in via a CLI flag.
• Ensure we do not treat key IDs as secrets; only the actual API key value should be removed from the logging/reporting path.

Best minimal fix while preserving functionality:

1. Stop including new_api_key in the migrations entries used for print_report.

   • In regenerate_api_keys, change the "new_api_key": new_api_key field to something non-sensitive (e.g., None) in the migrations.append(...) call for successful regenerations.
   • Similarly, for the dry‑run and error cases, leave "new_api_key": None as it is (already not exposing data).

2. Stop printing the new API key in print_report.

   • In print_report, under the status == "regenerated" branch, remove or replace the line print(f" → New API Key: {new_key}").
   • We can still print the non-secret metadata (old ID, tier, new key ID). Key IDs are not secrets, so print(f" → New Key ID: {new_id}") is safe and should remain.

3. (Optional but consistent): If we want admins to still obtain the new API keys in a controlled way, we can:

   • Keep new_api_key only in the in-memory results structure returned from regenerate_api_keys, but not in migrations that print_report iterates. Instead, add a separate top‑level list, e.g., "new_keys_secrets": [...], that contains dicts with old_key_id, new_key_id, api_key. Then, the main function (or the caller) can decide what to do with it (write to a secure file, print only when a --show-secrets flag is passed, etc.).
   • However, since we’re constrained to the shown snippets and want minimal change, the simplest and safest fix is just not to surface new_api_key at all via print_report/stdout and to keep the rest as-is.

Given the instructions, I’ll implement the minimal change:

• In backend/scripts/regenerate_api_keys.py:
  • In the migrations.append dict for the status == "regenerated" case, set "new_api_key": None instead of new_api_key.
  • In print_report, remove the retrieval of new_key = migration["new_api_key"] and the subsequent line that prints "→ New API Key: {new_key}".

This removes the clear-text logging path for the API key while keeping the rest of the reporting (including non-secret key IDs) intact.

No changes are needed in backend/app/services/api_key_service.py for this issue.

---

In general, the fix is to avoid printing the raw API key value anywhere. The script can still report that a new key was created and show its ID and metadata, but not the actual secret. If operators genuinely need to see the key once, that should be handled via a more controlled/explicit output mechanism (e.g., a dedicated flag), but given the current requirements we should remove the clear-text API key from the report.

The minimal, non‑functional‑breaking change is to adjust print_report in backend/scripts/regenerate_api_keys.py so that it no longer prints migration["new_api_key"]. Instead, we can print a generic message that a new key was generated and where to retrieve it, or note that the key is intentionally not shown. This keeps the structure of results and migrations untouched, so all other code paths remain the same. No changes are needed in APIKeyService itself.

Concretely:

• In print_report, in the elif status == "regenerated": block, remove or replace the line:
  • print(f" → New API Key: {new_key}")
• Optionally, replace it with something like:
  • print(" → New API Key: [REDACTED - not logged]")
    or a similar wording so the report is still informative.
• We do not need any new imports or helper methods; this is a simple change to the printed string.

This single change will address all CodeQL variants because they all point to the same sink: the printing of new_key (the API key) as clear text.

---

In general, the fix is to ensure that sensitive data (the raw API key) is never written to logs or stdout/stderr. Instead, logs should use non-sensitive identifiers (such as key IDs) or redacted/masked representations and keep the API key only in memory for the minimal time necessary to display it securely to an authorized operator, if at all.

For this specific code, we should:

• Stop including new_api_key in the migrations report structure, so it never reaches print_report.
• Adjust print_report so it no longer prints new_api_key.
• Optionally, clearly document in the report that the new key is intentionally omitted from logs, and rely on whatever the caller already does (or could do in a follow-up) to present the key in a safer, ephemeral way if needed.

Concretely, in backend/scripts/regenerate_api_keys.py:

1. In the regeneration loop, immediately after calling create_api_key, we currently do:
   • new_api_key = result["api_key"]
   • Store "new_api_key": new_api_key into each migration entry.
2. We should:
   • Still extract new_api_key so we can use it locally if needed, but not put it into the migrations dict that is returned and printed.
   • Replace the "new_api_key": new_api_key field with something non-sensitive, such as None or a placeholder string, or simply omit the field if the structure is not strictly relied upon elsewhere. To avoid changing external behavior too much, we can keep the key but set the value explicitly to None.
3. In print_report, remove the line print(f" → New API Key: {new_key}"), and optionally replace it with a message like print(" → New API Key: [REDACTED - see secure output]") or, more neutral, print(" → New API Key: (not logged)"). This preserves the shape of the report while avoiding secret leakage.
4. No changes are needed in backend/app/services/api_key_service.py for this particular issue, since the service already avoids logging the API key and only returns it once.

No new imports, methods, or dependencies are required; all changes are confined to backend/scripts/regenerate_api_keys.py within the shown snippets.

---

In general, the fix is to ensure that sensitive values such as raw API keys never appear in logs or user‑visible output in clear text. For this script, that means (1) removing or masking the direct printing of new_api_key in the migration report, and (2) ensuring that error messages and report structures do not inadvertently include those secrets.

The minimal, functionality‑preserving change is to stop printing the full API key and instead print only a safe, non‑sensitive representation, such as a fixed‑length prefix with the rest masked. We can also keep the API key in results["migrations"] if the calling code (or operator) needs to handle it programmatically, but we must not display it in the report. To additionally satisfy the analyzer, we can explicitly mask the key when constructing the migrations entries that are meant for human consumption, or at least when printing. Concretely:

• In regenerate_api_keys, when constructing the migrations entry for a successful regeneration, replace "new_api_key": new_api_key with "new_api_key": None or a masked/truncated representation that is not usable as a credential.
• In print_report, remove or change the line that prints new_key so it never prints the full API key. If a hint is useful for operators, print a masked form (e.g., first 4 characters and last 4 characters, with the middle replaced by *), computed locally in print_report.

These changes only affect backend/scripts/regenerate_api_keys.py; no changes are needed in backend/app/services/api_key_service.py. No additional imports are required; masking can be done with basic string operations.

General approach: Ensure that raw API keys are never stored in or exposed via structures that are later logged or printed, and keep any sensitive values as ephemeral as possible (e.g., only used to show one-time output to the operator, or written to a dedicated secure channel). Specifically, we should avoid including new_api_key inside the migrations entries that are returned from regenerate_api_keys, because results (containing migrations) is passed to print_report(results) and may be logged or displayed. Instead, we can handle the API key in a separate, clearly-scoped way (e.g., print it directly when created, with a clear warning, and only once) and track only non-sensitive metadata in migrations.

Best minimal fix without changing existing external behavior too much:

1. In backend/scripts/regenerate_api_keys.py, inside regenerate_api_keys:

   • Stop storing new_api_key in each migration dict. Replace the "new_api_key": new_api_key field with something non-sensitive, such as "new_api_key_redacted": True (or simply omit that field altogether). This removes the secret from the aggregate results structure and breaks the taint chain into results.
   • Optionally (and safely) log or print the new API key immediately after creation, but keep that outside of any aggregated structure. The comment already says # Note: API key is not logged for security - see report output instead; to stay conservative and keep functionality, we should not log the key and should keep any existing behavior of print_report (which we are not shown). Given the constraint to only change shown snippets, the safest approach is simply to avoid putting new_api_key into migrations and let print_report work with the remaining fields (key IDs, status, etc.). If print_report expects new_api_key, it will just see None, which is safer than exposing secrets and is an acceptable hardening for an admin-only migration script.
   • This change ensures that results contains no raw key material, so logging results["failed"] or any other counter is guaranteed not to leak the key.

2. No change is required to backend/app/services/api_key_service.py for logging purposes, since that file already avoids logging the api_key and only returns it once in a dict. The main issue arises from how that return value is propagated into a long-lived report.

Concrete edits:

• In backend/scripts/regenerate_api_keys.py, in the migrations.append({ ... }) block for the successful regeneration, change:

  "new_api_key": new_api_key,

  to either:

  "new_api_key": None,

  or (better, to reflect semantics explicitly):

  "new_api_key": None,  # API key intentionally omitted for security

  This ensures that no secret is reachable via results.

No other lines need to change to satisfy the clear-text logging concern, and we do not need to modify the flagged log line itself, since after this fix results no longer contains the sensitive field.

---