From 1828dcb81de6b13091f29decbef5938f2db4ef93 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 10:28:15 +1000 Subject: [PATCH] fix(security): address the independent review's blocking findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- RUNBOOK.md | 71 +++++++++++++++++-- flask.Dockerfile | 8 ++- requirements-dev.txt | 4 +- requirements.txt | 13 ++-- src/flask/app_factory.py | 50 +++++++++++++- src/flask/brewing.py | 11 +++ src/flask/mongo/repository.py | 100 +++++++++++++++++++++------ src/flask/templates/brews.html | 2 +- src/flask/templates/run_detail.html | 4 +- tests/conftest.py | 6 ++ tests/test_app.py | 101 ++++++++++++++++++++++++++++ 11 files changed, 328 insertions(+), 42 deletions(-) diff --git a/RUNBOOK.md b/RUNBOOK.md index d556037..668b8de 100644 --- a/RUNBOOK.md +++ b/RUNBOOK.md @@ -24,7 +24,7 @@ The pods are pinned to `archlinux-k3s-3` (`.161`) via `nodeSelector`. | `MONGO_HOST` | `192.168.178.229` | Deployment manifest | | `MONGO_USER` | `root` | Deployment manifest | | `MONGO_PASS` | *(see below)* | Deployment manifest | -| `SECRET_KEY` | *(see below)* | Deployment manifest — Flask session signing key | +| `SECRET_KEY` | *(see below)* | Deployment manifest — Flask session signing key. **Required** | | `FLASK_ENV` | `production` | Dockerfile | | `FLASK_DEBUG` | `0` | Dockerfile | @@ -35,9 +35,37 @@ The pods are pinned to `archlinux-k3s-3` (`.161`) via `nodeSelector`. kubectl -n beer get deploy beer-deployment -o jsonpath='{.spec.template.spec.containers[0].env}' ``` -> The app refuses to start a *usable* session without a `SECRET_KEY`; the -> Dockerfile ships a placeholder so the container still boots if the variable -> is missing. Treat the placeholder as a misconfiguration. +> **The app refuses to start without a real `SECRET_KEY`.** There is no default +> and no placeholder: a missing or placeholder value raises +> `ImproperlyConfigured` and the container exits. A known session-signing key +> would let anyone forge a login cookie, so a misconfigured deployment must +> fail loudly rather than run insecurely. +> +> Generate one with: +> ```bash +> python -c 'import secrets; print(secrets.token_hex(32))' +> ``` + +### Moving the credentials into a Kubernetes Secret + +The live Deployment currently carries `MONGO_PASS` as a plain environment +value. Moving it to a Secret is the better shape and is **not** done here +because it would mean replacing the live hand-provisioned values — a change +that needs the operator's sign-off. When it is done: + +```yaml +env: + - name: MONGO_PASS + valueFrom: + secretKeyRef: + name: beer-db + key: mongo_pass + - name: SECRET_KEY + valueFrom: + secretKeyRef: + name: beer-app + key: secret_key +``` ## Deploying @@ -76,10 +104,31 @@ re-applying the manifest. kubectl -n beer get pods kubectl -n beer logs deploy/beer-deployment --tail=50 curl -s http://:30015/healthz # -> {"status":"ok"} +curl -s http://:30015/readyz # -> {"status":"ready"} ``` -`/healthz` never touches Mongo, so it proves the process is up but not that the -database is reachable. +`/healthz` is the **liveness** probe and never touches Mongo — it answers "is +the process up". A database outage must not make the kubelet restart a healthy +process. + +`/readyz` is the **readiness** probe and pings Mongo, returning 503 when the +database is unreachable. Wire it to the Service so a pod with no database is +pulled out of the endpoints rather than serving errors: + +```yaml +readinessProbe: + httpGet: + path: /readyz + port: 5000 + initialDelaySeconds: 5 + periodSeconds: 10 +livenessProbe: + httpGet: + path: /healthz + port: 5000 + initialDelaySeconds: 10 + periodSeconds: 20 +``` ## Data safety @@ -112,6 +161,16 @@ current gravity is above the assumed original gravity of 1012. Fixed in the current version: the standard `(OG - FG) × 131.25` formula is used, and a run with no known OG shows a dash instead of a figure. +### The pod will not start after a deploy + +The most likely cause is a missing `SECRET_KEY` — the app exits rather than +run with a default. Check the logs: + +```bash +kubectl -n beer logs deploy/beer-deployment --tail=20 +# ImproperlyConfigured: SECRET_KEY is not set. ... +``` + ### The chart on the run page is blank The chart library is served from `/static/js/chart.umd.js` — it is vendored in diff --git a/flask.Dockerfile b/flask.Dockerfile index e6cfa00..2cbd69c 100644 --- a/flask.Dockerfile +++ b/flask.Dockerfile @@ -15,9 +15,11 @@ ENV FLASK_ENV=production ENV FLASK_DEBUG=0 ENV PORT=5000 -# SECRET_KEY is supplied by the deployment (see RUNBOOK.md). The default here -# is only a placeholder so the container starts if it is missing. -ENV SECRET_KEY=change-me-in-the-deployment +# SECRET_KEY MUST be supplied by the deployment (see RUNBOOK.md). There is +# deliberately no default: the app refuses to start without a real one, so a +# misconfigured deployment fails loudly instead of running with a known +# session-signing key. +# ENV SECRET_KEY=... EXPOSE 5000 diff --git a/requirements-dev.txt b/requirements-dev.txt index 0d4ba38..777f65a 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,3 +1,3 @@ -r requirements.txt -pytest -mongomock +pytest==9.2.0 +mongomock==4.3.0 diff --git a/requirements.txt b/requirements.txt index a4f6bce..ba57dab 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1,8 @@ -flask -Flask-WTF -waitress -pymongo -requests +# Pinned to the versions the test suite runs against. The image is rebuilt +# on every merge, so an unpinned dependency can change behaviour underneath a +# deploy with no commit to show for it. +Flask==3.1.3 +Flask-WTF==1.3.0 +waitress==3.0.2 +pymongo==4.19.0 +requests==2.32.5 diff --git a/src/flask/app_factory.py b/src/flask/app_factory.py index ff54e9a..8cbfda3 100644 --- a/src/flask/app_factory.py +++ b/src/flask/app_factory.py @@ -10,12 +10,42 @@ from __future__ import annotations import os from pathlib import Path -from flask import Flask, g, redirect, request, session, url_for +from flask import (Flask, g, redirect, request, session, url_for) from flask_wtf import CSRFProtect __all__ = ["create_app"] +class ImproperlyConfigured(RuntimeError): + """Raised when the app is missing configuration it cannot safely default. + + Werkzeug 3 dropped its own ``ImproperlyConfigured``, and Flask has never + exported one, so the app defines its own. It is a ``RuntimeError`` so it + is not mistaken for an HTTP error the app should try to render a page for. + """ + + +def _require_secret_key() -> str: + """Return the Flask secret key, refusing to start without a real one. + + A known default secret key means anyone can forge a session cookie and + log in as the operator, so a missing or placeholder value must stop the + app rather than silently fall back. The one exception is the test + suite, which supplies its own key through the config. + """ + secret = os.getenv("SECRET_KEY", "").strip() + if not secret: + raise ImproperlyConfigured( + "SECRET_KEY is not set. Export a random value, e.g. " + "`export SECRET_KEY=$(python -c 'import secrets; print(secrets.token_hex(32))')`." + ) + if secret.lower() in {"change-me-in-the-deployment", "dev-secret-change-me"}: + raise ImproperlyConfigured( + "SECRET_KEY is still the placeholder value. Set a real secret key." + ) + return secret + + def create_app(config: dict | None = None) -> Flask: """Build and configure the Flask application.""" app = Flask( @@ -25,7 +55,7 @@ def create_app(config: dict | None = None) -> Flask: ) app.config.update( - SECRET_KEY=os.getenv("SECRET_KEY", "dev-secret-change-me"), + SECRET_KEY=_require_secret_key(), MONGO_HOST=os.getenv("MONGO_HOST", "127.0.0.1"), MONGO_USER=os.getenv("MONGO_USER", "root"), MONGO_PASS=os.getenv("MONGO_PASS", ""), @@ -101,9 +131,23 @@ def create_app(config: dict | None = None) -> Flask: @app.route("/healthz") def healthz(): - """Liveness probe used by Kubernetes. Never touches Mongo.""" + """Liveness probe. Never touches Mongo — it answers "is the process + up", not "is the app working". A database outage must not cause the + kubelet to restart a perfectly healthy process.""" return {"status": "ok"}, 200 + @app.route("/readyz") + def readyz(): + """Readiness probe. Checks that Mongo is actually reachable, so a + pod with no database is taken out of the Service's endpoints instead + of serving errors.""" + try: + repo = get_repo() + repo.client.admin.command("ping") + except Exception as exc: # noqa: BLE001 - any failure means not ready + return {"status": "not ready", "error": str(exc)}, 503 + return {"status": "ready"}, 200 + @app.route("/") def index(): if logged_in(): diff --git a/src/flask/brewing.py b/src/flask/brewing.py index 58e8ad5..e1a2459 100644 --- a/src/flask/brewing.py +++ b/src/flask/brewing.py @@ -390,6 +390,13 @@ def days_between(start: date, end: date) -> int: return (end - start).days +def _fmt_decimal(points: int | None) -> str: + """Format gravity points as a decimal reading, e.g. ``1039`` -> ``1.039``.""" + if points is None: + return "" + return f"{points / 1000:.3f}" + + def run_summary(run: Run, today: date | None = None, og_override: int | None = None) -> dict: """A flat dict describing a run, ready for a template or a JSON response.""" @@ -405,8 +412,12 @@ def run_summary(run: Run, today: date | None = None, "fg_points": fg, "og_decimal": sg_points_to_decimal(og), "fg_decimal": sg_points_to_decimal(fg), + # Pre-formatted for the templates so no arithmetic happens in Jinja. + "og_decimal_str": _fmt_decimal(og), + "fg_decimal_str": _fmt_decimal(fg), "max_sg": run.max_sg(), "min_sg": run.min_sg(), + "min_sg_decimal": _fmt_decimal(run.min_sg()), "drop_points": run.gravity_drop_points(), "abv": run.abv_percent(), "projected_abv": run.projected_abv_percent(og_override=og_override), diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index 165d55e..baa7bbd 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -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: diff --git a/src/flask/templates/brews.html b/src/flask/templates/brews.html index 57ad51d..9487482 100644 --- a/src/flask/templates/brews.html +++ b/src/flask/templates/brews.html @@ -94,7 +94,7 @@ {% if run.min_sg is not none %} {{ run.min_sg }} -
{{ '%.3f' % (run.min_sg / 1000) }}
+
{{ run.min_sg_decimal }}
{% else %}—{% endif %} diff --git a/src/flask/templates/run_detail.html b/src/flask/templates/run_detail.html index 7cd6f15..9d970b6 100644 --- a/src/flask/templates/run_detail.html +++ b/src/flask/templates/run_detail.html @@ -29,7 +29,7 @@ {% if summary.og_points is not none %}{{ summary.og_points }}{% else %}—{% endif %} {% if summary.og_decimal %} -
{{ '%.3f' % summary.og_decimal }}
+
{{ summary.og_decimal_str }}
{% endif %}
@@ -40,7 +40,7 @@ {% if summary.fg_points is not none %}{{ summary.fg_points }}{% else %}—{% endif %}
{% if summary.fg_decimal %} -
{{ '%.3f' % summary.fg_decimal }}
+
{{ summary.fg_decimal_str }}
{% endif %}
diff --git a/tests/conftest.py b/tests/conftest.py index d15fd27..f196fd5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -15,6 +15,12 @@ if APP_DIR not in sys.path: sys.path.insert(0, APP_DIR) +@pytest.fixture(autouse=True) +def secret_key_env(monkeypatch): + """The app refuses to start without SECRET_KEY — give the tests one.""" + monkeypatch.setenv("SECRET_KEY", "test-secret-key-not-a-real-secret") + + @pytest.fixture() def mongo_client(): """An in-memory Mongo, shared by the repository the app builds.""" diff --git a/tests/test_app.py b/tests/test_app.py index 5ef5824..dd73020 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -26,6 +26,25 @@ def test_healthz_is_public(client): assert response.json == {"status": "ok"} +def test_readyz_checks_the_database(client): + response = client.get("/readyz") + assert response.status_code == 200 + assert response.json == {"status": "ready"} + + +def test_readyz_reports_not_ready_when_mongo_is_down(client, monkeypatch): + """A pod that cannot reach Mongo must not serve traffic.""" + from pymongo.errors import ServerSelectionTimeoutError + + def boom(*args, **kwargs): + raise ServerSelectionTimeoutError("no mongo") + + monkeypatch.setattr("mongo.repository.BeerRepository.__init__", + lambda self, **kw: (_ for _ in ()).throw(ServerSelectionTimeoutError("no mongo"))) + response = client.get("/readyz") + assert response.status_code == 503 + + def test_root_redirects_to_login(client): response = client.get("/") assert response.status_code == 302 @@ -179,3 +198,85 @@ def test_out_of_range_reading_is_saved_but_warned(logged_in, repo): assert b"outside the usual hydrometer range" in response.data # It is still stored — the warning is advisory, not a rejection. assert repo.readings.find_one({"beer_run_id": "12"}) is not None + + +# ------------------------------------------------------- security: passwords # + +def test_legacy_plaintext_password_is_accepted_and_migrated(repo): + """A legacy plaintext row still logs in, and is rewritten as a hash on + the way in so plaintext credentials disappear from the database.""" + repo.users.insert_one({"username": "beer", "password": "beer_pass"}) + + assert repo.user_check("beer", "beer_pass") is True + + doc = repo.users.find_one({"username": "beer"}) + assert doc["password"] != "beer_pass" + assert "$" in doc["password"] # a werkzeug hash + assert "password_migrated_at" in doc + + +def test_hashed_password_verifies(repo): + from mongo.repository import hash_password + + repo.users.insert_one({"username": "beer", "password": hash_password("beer_pass")}) + + assert repo.user_check("beer", "beer_pass") is True + assert repo.user_check("beer", "wrong") is False + + +def test_user_check_rejects_empty_credentials(repo): + repo.users.insert_one({"username": "beer", "password": "beer_pass"}) + + assert repo.user_check("", "") is False + assert repo.user_check("beer", "") is False + assert repo.user_check("", "beer_pass") is False + + +def test_user_check_rejects_a_nosql_operator_payload(repo): + """A dict must never be able to act as a Mongo query operator.""" + repo.users.insert_one({"username": "beer", "password": "beer_pass"}) + + assert repo.user_check({"$ne": ""}, "anything") is False + assert repo.user_check({"$gt": ""}, {"$ne": ""}) is False + + +def test_run_id_cannot_be_used_as_an_operator(repo): + """A run id is coerced to a string before it reaches a query.""" + from mongo.repository import _as_id + + assert _as_id({"$ne": ""}) == "" + assert _as_id(20) == "20" + assert _as_id("20") == "20" + + +def test_the_app_refuses_to_start_without_a_secret_key(monkeypatch): + """A missing SECRET_KEY must stop the app, not fall back to a default.""" + import importlib + + import app_factory + + # Reload so the guard reads the environment as it is *now*. The exception + # class must be taken from the reloaded module — reloading rebinds it, so + # a class imported earlier would be a different object and pytest.raises + # would not match it. + monkeypatch.delenv("SECRET_KEY", raising=False) + importlib.reload(app_factory) + with pytest.raises(app_factory.ImproperlyConfigured): + app_factory.create_app() + + monkeypatch.setenv("SECRET_KEY", "test-secret-key-not-a-real-secret") + importlib.reload(app_factory) + + +def test_the_app_refuses_the_placeholder_secret_key(monkeypatch): + import importlib + + import app_factory + + monkeypatch.setenv("SECRET_KEY", "change-me-in-the-deployment") + importlib.reload(app_factory) + with pytest.raises(app_factory.ImproperlyConfigured): + app_factory.create_app() + + monkeypatch.setenv("SECRET_KEY", "test-secret-key-not-a-real-secret") + importlib.reload(app_factory)