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).
This commit is contained in:
@@ -180,7 +180,13 @@ class BeerRepository:
|
|||||||
"date": _as_date(reading_date),
|
"date": _as_date(reading_date),
|
||||||
"final_reading": "True" if final_reading else "False",
|
"final_reading": "True" if final_reading else "False",
|
||||||
"comment": str(comment),
|
"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)
|
result = self.readings.update_one(query, update, upsert=True)
|
||||||
if result.upserted_id is not None:
|
if result.upserted_id is not None:
|
||||||
|
|||||||
@@ -225,6 +225,52 @@ def test_the_advice_is_stored_in_its_own_field(repo):
|
|||||||
assert updated["comment"] == "my own comment"
|
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):
|
def test_a_failed_generation_is_stored_as_a_failure(repo):
|
||||||
"""The note says it could not be generated, and is flagged as such."""
|
"""The note says it could not be generated, and is flagged as such."""
|
||||||
seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "")])
|
seed_readings(repo, [("1", "Ale", "1042", "2026-01-01", False, "")])
|
||||||
|
|||||||
Reference in New Issue
Block a user