12392a361e
Wave 2 of the remediation plan. Eight findings from the feature test run were the same defect twice: a write that reports success while storing something other than what was sent. The first half is a date parser that swallowed its own error. Two plugins carried an identical private _parse_date returning None on ValueError, so an unparseable date and a deliberately cleared one produced the same write. PUT /api/warranty/<id> with enddate '30-06-2027' answered 200 "Warranty updated" and stored NULL, dropping the record out of every expiry surface on the strength of a typo; the measuring-tool calibration date did the same and flipped the tool to 'unknown', out of the calibration report and the map overlay. shopdb/utils/datevalues.py now holds one strict parser on the contract surface: an explicitly empty value clears the field, anything that is not a date is refused with a 400 naming the field, and those are different intents with different answers. The vendor-lookup paths keep a lenient wrapper on purpose - a provider changing its date shape must not fail the refresh of an otherwise good record, and no person typed that value. The second half is why the truncation findings were possible at all. MySQL was running non-strict: sql_mode on the 5.6 server is NO_ENGINE_SUBSTITUTION with no STRICT_TRANS_TABLES, so it accepted writes it could not store and adjusted them. A 257-character username became its first 100 characters (and the next distinct one then collided with it); an over-long asset number was cut at 50; a malformed releasedate became '0000-00-00' and permanently broke every read of that application. All with a 201. The mode is now set on the CONNECTION rather than left to the server, so a site whose my.cnf disagrees still gets it. That trades silent corruption for a DataError, which uncaught is a 500 - and a 500 says the server is broken when the truth is that the value does not fit. An error handler maps it to a 400 naming the column where MySQL names one. Verified by hand against MySQL 5.6: POST /api/users with a 257-character username now answers 400 "That value is too long for username." and stores nothing, where it previously stored a truncated row and answered 201. Twelve tests. The date guard is verified by planting: restore the swallowing parser and four of them fail. The DataError tests raise the error directly because the suite runs on SQLite, which does not enforce VARCHAR length at all - there is no value that would trigger it there, which is precisely why this went unnoticed. Scope: product
83 lines
2.9 KiB
Python
83 lines
2.9 KiB
Python
"""Parsing a date out of a request body, strictly.
|
|
|
|
WHY THIS EXISTS
|
|
|
|
Two plugins carried an identical private `_parse_date` that swallowed the
|
|
error:
|
|
|
|
try:
|
|
return datetime.strptime(str(value)[:10], '%Y-%m-%d').date()
|
|
except (ValueError, TypeError):
|
|
return None
|
|
|
|
So an unparseable date and a deliberately cleared one produced the same
|
|
result, and the write went ahead. `PUT /api/warranty/<id>` with enddate
|
|
'not-a-date' answered 200 "Warranty updated" and stored NULL - the warranty
|
|
then vanished from every expiry surface, silently, on the strength of a typo.
|
|
The measuring-tool calibration date did the same thing and dropped the tool out
|
|
of the calibration report.
|
|
|
|
The rule here: an EXPLICIT empty value clears the field, and anything that is
|
|
not a date is refused. Those are different intents and they get different
|
|
answers. Callers turn `DateValueError` into a 400 naming the field, so the
|
|
person who mistyped it finds out at the point of the mistake rather than at the
|
|
next audit.
|
|
|
|
Accepted forms are deliberately narrow - 'YYYY-MM-DD', and the ISO-8601
|
|
datetime forms whose leading ten characters are a date, since that is what the
|
|
frontend's date inputs and every vendor API in the tree emit. Anything looser
|
|
is a guess about what somebody meant.
|
|
"""
|
|
|
|
from datetime import date, datetime
|
|
|
|
|
|
class DateValueError(ValueError):
|
|
"""A supplied value was neither empty nor a date this accepts.
|
|
|
|
Carries the field name so the error message can say which one, which is the
|
|
whole point of refusing rather than nulling.
|
|
"""
|
|
|
|
def __init__(self, field, value):
|
|
self.field = field
|
|
self.value = value
|
|
super().__init__(
|
|
'%s must be a date as YYYY-MM-DD (got %r)' % (field, value))
|
|
|
|
|
|
def parse_date(value, field='date'):
|
|
"""Return a date, or None for an explicitly empty value.
|
|
|
|
Raises DateValueError for anything else. None and '' mean "clear it";
|
|
they are the only values that produce None.
|
|
"""
|
|
if value is None:
|
|
return None
|
|
if isinstance(value, datetime):
|
|
return value.date()
|
|
if isinstance(value, date):
|
|
return value
|
|
if not isinstance(value, str):
|
|
# A number or a list is not a mistyped date, it is the wrong type.
|
|
raise DateValueError(field, value)
|
|
text = value.strip()
|
|
if not text:
|
|
return None
|
|
try:
|
|
return datetime.strptime(text[:10], '%Y-%m-%d').date()
|
|
except ValueError:
|
|
raise DateValueError(field, value)
|
|
|
|
|
|
def parse_date_fields(data, fields):
|
|
"""Parse several date fields out of a payload at once.
|
|
|
|
Only keys PRESENT in the payload are returned, so a caller can tell "not
|
|
mentioned" (leave it alone) from "sent as empty" (clear it) - a distinction
|
|
a plain `data.get(...)` throws away, and the reason the alias-clearing bug
|
|
existed on the PC form.
|
|
"""
|
|
return {field: parse_date(data[field], field)
|
|
for field in fields if field in data}
|