From d0aef7a787da822fe4727c2fae073877a9ab8d23 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 16:05:59 +1000 Subject: [PATCH] fix(advice): clear stale advice on edit, and pin the injection guard Two things I wanted to be sure of before this merges. Stale advice ------------ The advice is written for the gravity that was there when it was generated. If the brewer then edits the reading - to fix a typo, or to mark it final - the old advice no longer describes it, and the page was showing advice that contradicted the number next to it. `upsert_reading` now clears the advice fields, so the stale advice goes and a fresh one is generated (the form already regenerates on every save). Only the advice fields are cleared - the brewer's own comment is never touched. The injection guard ------------------- The reading's `_id` is an ObjectId, so the original `_as_id` coercion (which turns run ids into plain strings, to stop a caller passing a dict that Mongo would interpret as a query operator) did not apply - it turned the ObjectId into an empty string and the update silently matched nothing. `_as_doc_id` accepts only an ObjectId or a string that parses as one. Verified against a live injection attempt: `{"$ne": None}`, `{"$gt": ""}`, a list, an int, None and a non-ObjectId string all return None and match nothing. There is now a test that pins it, because it is a security property that would otherwise only be covered by reading the code. 160 tests (up from 158). --- src/flask/mongo/repository.py | 8 +++++- tests/test_brew_advice.py | 46 +++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index 0ca5649..1de2779 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -180,7 +180,13 @@ class BeerRepository: "date": _as_date(reading_date), "final_reading": "True" if final_reading else "False", "comment": str(comment), - } + }, + # The advice was written for the gravity that was there when + # it was generated. If the reading has been edited, that + # advice no longer describes it, so it is cleared and a fresh + # one is generated. (Advice is only ever cleared here - never + # the brewer's comment.) + "$unset": {"advice": "", "advice_ok": "", "advice_at": ""}, } result = self.readings.update_one(query, update, upsert=True) if result.upserted_id is not None: diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 7d807c3..3485594 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -225,6 +225,52 @@ def test_the_advice_is_stored_in_its_own_field(repo): assert updated["comment"] == "my own comment" +def test_editing_a_reading_clears_its_stale_advice(repo): + """The advice was written for the gravity that was there before. + + If the reading is edited, that advice no longer describes it, so it + is cleared and a fresh one is generated. The brewer's own comment is + never cleared. + """ + seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "my comment")]) + + doc = repo.readings.find_one({"beer_run_id": "1"}) + repo.set_reading_advice(str(doc["_id"]), "THE ADVICE", True) + + # Edit the reading. + repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1030, + reading_date="2026-01-01", final_reading=False, + comment="my comment") + + after = repo.readings.find_one({"beer_run_id": "1"}) + assert after.get("advice") is None, "the stale advice was kept" + assert after["comment"] == "my comment", "the brewer's comment was touched" + assert after["sg"] == "1030" + + +def test_the_reading_id_cannot_be_used_to_inject_a_query(repo): + """The reading id is coerced before it reaches Mongo. + + Every value that reaches a Mongo query is coerced first, because a + caller passing a dict would have it interpreted as a query operator + rather than a value - the classic NoSQL injection route. A reading + id that is not a plain ObjectId or string must match nothing. + """ + seed_readings(repo, [ + ("1", "Ale", "1042", "2026-01-01", False, ""), + ("2", "Ale", "1030", "2026-01-01", False, ""), + ]) + + for hostile in [{"$ne": None}, {"$gt": ""}, ["a"], 123, None, + "not-an-objectid", ""]: + modified = repo.set_reading_advice(hostile, "INJECTED", True) + assert modified == 0, f"{hostile!r} matched something" + + # And no document was touched. + for doc in repo.readings.find({}): + assert doc.get("advice") is None + + def test_a_failed_generation_is_stored_as_a_failure(repo): """The note says it could not be generated, and is flagged as such.""" seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "")])