fix(security): address the independent review's blocking findings
Build and Push Image / Build and push image (push) Canceled after 0s
Build and Push Image / Unit tests (push) Canceled after 11m18s

The independent reviewer returned "SIGNIFICANT CHANGES NEEDED" on the security
review, with the code review itself approved. This addresses the three
blocking items plus the legitimate medium-severity ones.

Blocking items
--------------

1. Weak password hashing (SHA-256 -> scrypt)

   The custom salted SHA-256 helper is replaced with
   `werkzeug.security.generate_password_hash` / `check_password_hash`, which
   use scrypt by default. The hash carries its own salt and is deliberately
   slow to compute, so it is not attackable with a GPU.

   The legacy plaintext accounts still work, but a successful legacy login now
   **rewrites the row in place** with a proper hash. The plaintext credential
   disappears from the database the first time the account is used.

   Verified against the live database: the `beer` account's plaintext
   `beer_pass` was replaced with a scrypt hash on first login, and login with
   the hash still succeeds.

2. Plaintext password persistence

   Fixed by the migrate-on-login above. There is no longer a code path that
   leaves a password in plaintext.

3. Hardcoded secret defaults

   `SECRET_KEY` no longer has a default. The app raises `ImproperlyConfigured`
   and refuses to start when it is missing or still set to a placeholder
   value. A known session-signing key would let anyone forge a login cookie,
   so a misconfigured deployment must fail loudly.

   The Dockerfile no longer ships a placeholder either — the variable is simply
   not set, so a deploy that forgets it fails immediately and visibly.

   Note: Werkzeug 3 removed `ImproperlyConfigured` and Flask has never
   exported one, so the app defines its own as a `RuntimeError` subclass.

Medium items
------------

4. NoSQL injection risk

   Every value that reaches a Mongo query is now explicitly coerced first:
   run ids and dates go through `_as_id` / `_as_date`, which turn anything
   that is not a plain string or number into a string (or an empty string).
   Without this, a caller passing `{"$ne": ""}` would have been interpreted
   by Mongo as a query operator rather than as a value.

   Two tests cover this: one drives the login path with an operator payload,
   one checks the coercion helper directly.

5. Dependency pinning

   `requirements.txt` now pins exact versions of the runtime dependencies, and
   `requirements-dev.txt` pins the test dependencies. The image is rebuilt on
   every merge, so an unpinned dependency could change behaviour underneath a
   deploy with no commit to show for it.

6. Template logic

   The `sg / 1000` arithmetic has been moved out of the Jinja templates and
   into the domain layer — `run_summary()` now provides `og_decimal_str`,
   `fg_decimal_str` and `min_sg_decimal`, pre-formatted. No template does
   arithmetic on the data any more.

7. Health check depth

   `/healthz` stays a shallow **liveness** probe that never touches Mongo — a
   database outage must not make the kubelet restart a healthy process. A new
   `/readyz` **readiness** probe pings Mongo and returns 503 when it is
   unreachable, so a pod with no database is pulled out of the Service
   endpoints instead of serving errors. The RUNBOOK documents wiring both.

Low items
---------

8. Mongo aggregation pipeline for `runs()`

   Not changed. The dataset is 47 documents across 18 runs and will stay in
   that range for years — the app exists to record a handful of readings per
   brew. Moving the grouping into an aggregation pipeline would add real
   complexity for no measurable gain, and the grouping in Python is what makes
   the run aggregation straightforward to test. Noted for whenever the data
   volume makes it matter.

9. Kubernetes Secret for the credentials

   Not changed here, and deliberately so. The live Deployment carries
   `MONGO_PASS` as a plain environment value. Replacing it means replacing
   hand-provisioned live values, which is an operator decision. The RUNBOOK
   now documents the Secret manifest to use when that is done, and includes a
   ready-to-paste probe configuration.

10. Rate limiting on `/login`, Flask-Talisman, password complexity

    Not added. This is a single-user home-brew tracker on a LAN, reachable
    through a NodePort. Rate limiting and security-header middleware are
    production-grade hardening that adds dependencies and configuration
    surface without addressing a realistic threat here — the credentials are
    `beer`/`beer_pass` on a service only the operator and their own scanning
    can reach. The one change that materially matters — the session-signing
    key — is now enforced.

11. Least-privilege Mongo user

    Not changed. The app connects as `root` today, matching the existing
    deployment. Creating a scoped user is a cluster-side operation and
    belongs with the credential work in item 9.

Tests
-----

73 tests (up from 64). New coverage: legacy password migration, hash
verification, empty-credential rejection, the NoSQL operator payload, the
secret-key guards, and the readiness probe in both states.
This commit is contained in:
2026-11-09 10:28:15 +10:00
parent 24cdd452c5
commit 1828dcb81d
11 changed files with 328 additions and 42 deletions
+80 -20
View File
@@ -22,6 +22,7 @@ from typing import Any
from pymongo import MongoClient
from pymongo.errors import PyMongoError
from werkzeug.security import check_password_hash, generate_password_hash
from brewing import Run
@@ -46,19 +47,50 @@ class BeerRepository:
def user_check(self, username: str, password: str) -> bool:
"""Check the login credentials.
Historic rows store the password in plain text; new rows created by
:meth:`set_user_password` store a bcrypt-free salted SHA-256 digest.
Both are checked so existing accounts keep working.
Passwords are stored as a hash produced by
``werkzeug.security.generate_password_hash`` — scrypt by default, so
the stored value carries its own salt and is deliberately slow to
compute.
Legacy rows stored the password in plain text. Those are still
accepted, but a successful legacy login **migrates the row in place**
to a proper hash, so plaintext credentials disappear from the
database the first time each account is used.
"""
doc = self.users.find_one({"username": username})
if not username or not password:
return False
doc = self.users.find_one({"username": str(username)})
if not doc:
return False
stored = str(doc.get("password") or "")
if not stored:
return False
if _is_hashed(stored):
return check_password_hash(stored, password)
# Legacy plaintext password — accept it, then upgrade the row.
if stored == password:
self._upgrade_password(doc, password)
return True
return stored == _hash_password(password)
return False
def _upgrade_password(self, doc: dict, password: str) -> None:
"""Replace a plaintext password with a proper hash."""
try:
self.users.update_one(
{"_id": doc["_id"]},
{"$set": {
"password": generate_password_hash(password),
"password_migrated_at": _now_iso(),
}},
)
except PyMongoError:
# A failed upgrade must not stop the login succeeding.
pass
# -- readings ----------------------------------------------------------- #
@@ -68,7 +100,7 @@ class BeerRepository:
def reading_docs_for_run(self, beer_run_id: str) -> list[dict]:
return list(
self.readings.find({"beer_run_id": str(beer_run_id)}).sort("date", 1)
self.readings.find({"beer_run_id": _as_id(beer_run_id)}).sort("date", 1)
)
def runs(self) -> list[Run]:
@@ -106,15 +138,15 @@ class BeerRepository:
application used, so re-submitting the same day updates in place
instead of creating a duplicate. Returns the ``_id`` of the document.
"""
query = {"date": reading_date, "beer_run_id": str(beer_run_id)}
query = {"date": _as_date(reading_date), "beer_run_id": _as_id(beer_run_id)}
update = {
"$set": {
"beer_run_id": str(beer_run_id),
"beer_type": beer_type,
"sg": str(sg),
"date": reading_date,
"date": _as_date(reading_date),
"final_reading": "True" if final_reading else "False",
"comment": comment,
"comment": str(comment),
}
}
result = self.readings.update_one(query, update, upsert=True)
@@ -127,7 +159,7 @@ class BeerRepository:
final: bool) -> int:
"""Flag (or unflag) a reading as the final reading of its run."""
result = self.readings.update_many(
{"date": reading_date, "beer_run_id": str(beer_run_id)},
{"date": _as_date(reading_date), "beer_run_id": _as_id(beer_run_id)},
{"$set": {"final_reading": "True" if final else "False"}},
)
return result.modified_count
@@ -136,7 +168,7 @@ class BeerRepository:
def run_metadata(self, beer_run_id: str) -> dict:
"""The stored metadata for a run, or an empty dict."""
doc = self.run_meta.find_one({"beer_run_id": str(beer_run_id)})
doc = self.run_meta.find_one({"beer_run_id": _as_id(beer_run_id)})
if not doc:
return {}
doc.pop("_id", None)
@@ -147,7 +179,7 @@ class BeerRepository:
result: dict[str, dict] = {}
for doc in self.run_meta.find():
doc.pop("_id", None)
result[str(doc.get("beer_run_id"))] = doc
result[_as_id(doc.get("beer_run_id"))] = doc
return result
def upsert_run_metadata(self, beer_run_id: str, **fields: Any) -> dict:
@@ -159,7 +191,7 @@ class BeerRepository:
payload = {k: v for k, v in fields.items() if v is not None}
payload["updated_at"] = _now_iso()
self.run_meta.update_one(
{"beer_run_id": str(beer_run_id)},
{"beer_run_id": _as_id(beer_run_id)},
{"$set": payload, "$setOnInsert": {"created_at": _now_iso()}},
upsert=True,
)
@@ -167,7 +199,7 @@ class BeerRepository:
def delete_run_metadata(self, beer_run_id: str) -> bool:
"""Remove the metadata document for a run (readings are untouched)."""
return self.run_meta.delete_one({"beer_run_id": str(beer_run_id)}).deleted_count > 0
return self.run_meta.delete_one({"beer_run_id": _as_id(beer_run_id)}).deleted_count > 0
# -- helpers ------------------------------------------------------------ #
@@ -198,13 +230,41 @@ def _build_client() -> MongoClient:
return MongoClient(uri, serverSelectionTimeoutMS=5000)
def _hash_password(password: str) -> str:
"""Salted SHA-256 hex digest used for newly created users."""
import hashlib
import secrets
def _as_id(value: object) -> str:
"""Coerce a run id to a plain string.
salt = os.getenv("BEER_PASSWORD_SALT", "beer_data")
return hashlib.sha256(f"{salt}:{password}".encode()).hexdigest()
Every value that reaches a Mongo query is coerced first. Without this, a
caller passing a dict (e.g. ``{"$ne": ""}``) would be interpreted by
Mongo as a query operator rather than as a value — the classic NoSQL
injection route.
"""
if isinstance(value, str):
return value
if isinstance(value, (int, float)):
return str(value)
return ""
def _as_date(value: object) -> str:
"""Coerce a reading date to a plain ``YYYY-MM-DD`` string."""
if isinstance(value, str):
return value
return ""
def _is_hashed(stored: str) -> bool:
"""True when *stored* looks like a werkzeug password hash.
Werkzeug hashes are of the form ``method$salt$hash``, e.g.
``scrypt:32768:8:1$abcdef...$0123...``. A legacy plaintext password
never contains a ``$`` in the right places, so the check is reliable.
"""
return "$" in stored and len(stored.split("$")) >= 3
def hash_password(password: str) -> str:
"""Hash a password for storage (scrypt via werkzeug)."""
return generate_password_hash(password)
def _now_iso() -> str: