Fix 9 bugs from audit (security + correctness)#3454
Merged
Conversation
JSON payload keys were interpolated directly into the UPDATE statement, so any attacker-controlled key could inject SQL (or silently tamper with identity columns like user_id). Restrict writable columns to a static whitelist and reject unknown keys with HTTP 400. Fixes #3445
@validate_country was stacked above @bp.route, so Flask registered the unwrapped view and validate_country never ran. Requests with an unknown country_id reached the handler and produced 200/500 instead of a 400. Swap the order to match household_routes. Fixes #3446
The UPDATE statement keyed only on household id, so two different countries with the same numeric id could clobber each other. Add country_id to the WHERE clause and surface missing rows with a LookupError rather than silently returning stale data. Fixes #3447
The simulation and report_output create/update handlers wrapped every Exception as BadRequest, which silently downgraded DB failures and bugs to 400s with no traceback logged. Only ValueError / pydantic / jsonschema validation errors now become 400; everything else propagates as 500 via the Flask error handler, after being logged with logger.exception(). Fixes #3448
update_fields unconditionally appended api_version, so the "no fields to update" guard never fired and an empty PATCH still rewrote the row. Append api_version only after we know at least one user field was supplied; the route converts the resulting False return value into a 400. Fixes #3449
Python's builtin `hash()` is salted per process by default (PYTHONHASHSEED), so two gunicorn workers computed different cache keys for identical requests and the cache rarely hit. Switch to sha256(full_path + data) so the digest is stable across workers and restarts. Fixes #3450
The f-string LIMIT clause allowed the caller-controlled max_results value into the SQL statement and could be omitted entirely. Always apply LIMIT, clamp max_results to [1, 1000], and bind the value as a parameter. Fixes #3451
A bare except suppressed BaseException-derived exits (SystemExit, KeyboardInterrupt) in addition to regular errors and dropped the traceback entirely. Use except Exception plus logger.exception so the failure is visible in logs while keeping the existing fallback to empty wealth-decile data. Fixes #3452
uk_constituency_breakdown used `"E" not in code` (and friends)
to skip rows outside a selected UK nation, so a Welsh code
containing any 'E' in its tail was treated as English (and
double-counted in the England bucket). Switch to
`code.startswith("E"/"S"/"W"/"N")`, matching the local-authority
code already at line 721+.
Fixes #3453
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3454 +/- ##
==========================================
+ Coverage 77.10% 77.97% +0.87%
==========================================
Files 62 62
Lines 3258 3301 +43
Branches 584 596 +12
==========================================
+ Hits 2512 2574 +62
+ Misses 582 562 -20
- Partials 164 165 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The original test sent json={}, which fails validate_sim_analysis_payload
with 400 before reaching the country check. That meant the test passed
with or without the decorator-order fix. Use a payload that satisfies
validate_sim_analysis_payload so the only remaining reason to 400 is the
unknown country_id. With the pre-#3446 decorator order the view runs
first, calls ai_prompt_service.get_prompt which returns None for an
unknown prompt name, and yields 404 - so the test now genuinely detects
the regression.
BadRequest and NotFound raised explicitly from within the create/update try blocks were being caught by the generic `except Exception:` arm and logged as "Unexpected error" before being re-raised. Add an `except HTTPException: raise` ahead of the generic handler so expected client-error responses pass through without polluting the logs. This also subsumes the existing `except NotFound: raise` in the PATCH handlers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundles nine bug fixes identified in a recent audit. Each is a small, focused change with a regression test. Commits are kept separate per issue so they can be cherry-picked or reverted individually.
UPDATEkeys inupdate_user_policy(whitelist writable columns, reject unknown keys with 400). Fixes SQL injection in update_user_policy via unparameterized dict keys #3445economy_routesandai_prompt_routes(move@bp.routeoutside@validate_countryso unknown countries return 400). Fixes @validate_country decorator order bypasses country allow-list on economy and ai-prompt routes #3446update_householdWHERE missingcountry_id(scope the UPDATE by country; surface missing rows withLookupError). Fixes update_household UPDATE misses country_id predicate, cross-country overwrite possible #3447ValueError/pydantic.ValidationError/jsonschema.ValidationErrorbecome 400; everything else propagates as 500 withlogger.exception). Fixes Simulation and report_output routes turn every exception into HTTP 400 #3448update_simulationalways rewroteapi_versioneven for empty PATCH bodies (appendapi_versiononly after a user field is supplied; empty PATCH now returns 400). Fixes update_simulation always adds api_version, making empty-PATCH guard unreachable #3449str(hash(...))withhashlib.sha256(...).hexdigest()so workers share a cache. Fixes Cache keys use randomized Python hash(), breaking cache sharing between workers #3450get_simulationsLIMIT hardening: always apply LIMIT, clampmax_resultsto[1, 1000], bind as a parameter. Fixes get_simulations drops LIMIT when max_results is None and interpolates into SQL #3451except:inendpoints/economy/compare.pyreplaced withexcept Exception:+logger.exception(...). Fixes Bare except in wealth_decile_impact swallows signals and hides real errors #3452"E" not in code) instead ofcode.startswith("E"), so a Welsh code containing anyEcould leak into the England bucket. Align with the pattern already used for local authorities. Fixes UK constituency country filter uses substring match instead of startswith #3453Also updates
tests/to_refactor/python/test_household_routes.py::test_update_household_successso its mock assertion matches the country-scoped UPDATE statement from #3447. Each fix adds a changelog fragment inchangelog.d/.Test plan
ruff format --check .passes (CIlintjob)make test(pytest tests/to_refactor tests/unit) passes on CItests/unit/endpoints/test_update_user_policy.py(SQL injection payload → 400)tests/unit/routes/test_decorator_order.py(bogus country → 400 on economy and ai-prompt routes)tests/unit/services/test_household_service.py::TestUpdateHousehold::test_update_household_rejects_cross_country_idtests/unit/routes/test_route_exception_handling.py(non-validation exceptions → 500)tests/unit/services/test_simulation_service.py::TestUpdateSimulation::test_update_simulation_with_no_user_fields_returns_falsetests/unit/utils/test_cache_utils.py(SHA-256 stability, including across processes with different PYTHONHASHSEED)tests/unit/endpoints/test_get_simulations.py(clamping, default, non-integer input)tests/unit/endpoints/economy/test_compare.py::TestUKConstituencyBreakdownFunction::test__country_filter_uses_prefix_not_substring