Answer a nonsense filter with a 400, and sweep every list route for the rest
Wave 3. Six findings, one cause: `int(request.args.get('typeid'))` with nothing
catching the ValueError. The caller sent a bad argument and got
500 INTERNAL_ERROR plus a traceback in the log, which says the server is broken
when the truth is that 'abc' is not a number.
shopdb/utils/queryargs.intarg reads it and raises QueryArgError, which an
app-level handler answers as a 400 naming the argument. App-level on purpose: a
route should not have to remember, and forgetting was the entire defect. The
policy differs from get_pagination_params, which falls back to a default for a
bad page number - a nonsense page is harmless and the caller still wants their
list, but silently ignoring a nonsense FILTER returns rows they believe they
excluded.
The gate matters more than the six fixes. tests/test_core/test_bad_query_args.py
sweeps all 131 registered no-parameter GET routes under /api/ with twelve
argument shapes and fails on any 5xx. It found sites the test run had not
reported - statusid and locationid on /api/assets, the second asset-list route,
limit on the employee lookup and the printer supply history - because the
pattern is how a list filter gets written rather than a property of any one
route. Verified non-vacuous by planting the old `int(...)` back into
/api/machines: four tests fail.
Four usb routes are skipped by exact path, with the reason: they reach the
EXTERNAL cmmc_usb database, so their 500 in a test environment is a missing
database rather than an argument fault. That is the complete set - every usb
GET route calling cmmc_usb_connection - not a convenient subset.
One correction worth recording: the first pass rewrote every
`if x := request.args.get(...)` in the touched files, including text arguments,
so a search for 'ZZNOTHINGMATCHES' briefly became a 400. Caught by
test_list_search_identifiers. Only id-shaped arguments are integers.
Scope: product
This commit is contained in:
120
tests/test_core/test_bad_query_args.py
Normal file
120
tests/test_core/test_bad_query_args.py
Normal file
@@ -0,0 +1,120 @@
|
||||
"""No list route may answer a nonsense filter with a 500.
|
||||
|
||||
THE GATE, not just the six fixes
|
||||
|
||||
The feature test run found `int(request.args.get(...))` with nothing catching
|
||||
the ValueError in six routes across five plugins - machines, network, printers,
|
||||
knowledgebase, computers and the asset core. Fixing six sites does not stop a
|
||||
seventh: the pattern is how a filter gets written, and it is invisible in review
|
||||
because the happy path works.
|
||||
|
||||
So this sweeps every registered GET route that takes no path parameters and
|
||||
sends each of them a set of arguments that are not what any filter wants. The
|
||||
contract is narrow on purpose:
|
||||
|
||||
- a 5xx is ALWAYS a failure. The caller sent a bad argument; that is a 400.
|
||||
- a 400 is the intended answer.
|
||||
- a 200 is acceptable: a route is entitled to ignore an argument it does not
|
||||
know about, and most of these routes will not have the argument at all.
|
||||
- a 401/403/404 is acceptable: this test is about argument handling, not
|
||||
about authorization.
|
||||
|
||||
Run as an admin, because a route that refuses everyone tests nothing.
|
||||
"""
|
||||
import pytest
|
||||
|
||||
# Names taken from the filters the failing routes actually declared, plus the
|
||||
# generic shapes a client sends. Values chosen to break a bare int(), a bare
|
||||
# float(), and a naive split.
|
||||
BAD_ARGS = [
|
||||
('typeid', 'abc'),
|
||||
('type_id', 'abc'),
|
||||
('assettypeid', '1; DROP TABLE assets'),
|
||||
('vendorid', 'null'),
|
||||
('locationid', '1.5'),
|
||||
('businessunitid', ''),
|
||||
('appid', 'abc'),
|
||||
('statusid', 'NaN'),
|
||||
('modelid', '-'),
|
||||
('limit', 'lots'),
|
||||
('page', 'abc'),
|
||||
('perpage', 'abc'),
|
||||
]
|
||||
|
||||
# Routes that are not list endpoints and whose GET does real work with side
|
||||
# effects or external calls. Excluded by PREFIX, and each one says why.
|
||||
SKIP_PREFIXES = (
|
||||
'/api/collector/', # needs the collector key; fail-closed by design
|
||||
'/api/endpoints/', # agent wire protocol, same key
|
||||
'/api/backups/run', # kicks off work rather than reading
|
||||
)
|
||||
|
||||
# Skipped by EXACT path, not prefix, so the rest of the plugin stays covered.
|
||||
SKIP_EXACT = {
|
||||
# These four reach the EXTERNAL cmmc_usb database. Their 500 here is "no
|
||||
# such database in the test environment", a different fault from the one
|
||||
# this gate is about, and the list is the complete set - every usb GET
|
||||
# route whose handler calls cmmc_usb_connection. The self-hosted USB
|
||||
# surface is covered by test_usb_kiosk.py.
|
||||
'/api/usb',
|
||||
'/api/usb/checkouts',
|
||||
'/api/usb/checkouts/active',
|
||||
'/api/usb/reports/audit',
|
||||
}
|
||||
|
||||
|
||||
def _listish_routes(app):
|
||||
routes = []
|
||||
for rule in app.url_map.iter_rules():
|
||||
path = str(rule.rule)
|
||||
if not path.startswith('/api/') or '<' in path:
|
||||
continue
|
||||
if 'GET' not in (rule.methods or ()):
|
||||
continue
|
||||
if path in SKIP_EXACT:
|
||||
continue
|
||||
if any(path.startswith(prefix) for prefix in SKIP_PREFIXES):
|
||||
continue
|
||||
routes.append(path)
|
||||
return sorted(set(routes))
|
||||
|
||||
|
||||
def test_the_sweep_actually_covers_something(app):
|
||||
"""A gate that silently matches nothing passes forever. This one has been
|
||||
known to cover 90+ routes; if it drops to a handful, the filter above is
|
||||
wrong rather than the app being small."""
|
||||
assert len(_listish_routes(app)) > 40
|
||||
|
||||
|
||||
def test_no_route_answers_a_nonsense_filter_with_a_server_error(
|
||||
client, auth_headers, app):
|
||||
failures = []
|
||||
for path in _listish_routes(app):
|
||||
for name, value in BAD_ARGS:
|
||||
response = client.get('%s?%s=%s' % (path, name, value),
|
||||
headers=auth_headers)
|
||||
if response.status_code >= 500:
|
||||
failures.append('%s?%s=%s -> %d' % (path, name, value,
|
||||
response.status_code))
|
||||
assert not failures, (
|
||||
'a bad query argument is a 400, never a 500:\n '
|
||||
+ '\n '.join(failures[:40]))
|
||||
|
||||
|
||||
@pytest.mark.parametrize('value', ['abc', '', '1.5', 'null'])
|
||||
def test_a_bad_type_filter_is_a_400_that_names_the_argument(
|
||||
client, auth_headers, value):
|
||||
"""The routes the run actually caught. An empty value is the exception:
|
||||
absent and empty both mean 'no filter', which is what a cleared dropdown
|
||||
sends."""
|
||||
for path in ('/api/machines', '/api/computers', '/api/network',
|
||||
'/api/printers', '/api/assets'):
|
||||
response = client.get('%s?typeid=%s' % (path, value),
|
||||
headers=auth_headers)
|
||||
if value == '':
|
||||
assert response.status_code == 200, (path, response.get_json())
|
||||
continue
|
||||
assert response.status_code == 400, (path, response.status_code)
|
||||
body = response.get_json()
|
||||
assert body['data']['error']['code'] == 'VALIDATION_ERROR'
|
||||
assert 'typeid' in body['data']['error']['message']
|
||||
Reference in New Issue
Block a user