Security: SQL injection prevention and input validation across filtering layer - #19
Open
Oluwabori19 wants to merge 14 commits into
Open
Conversation
… them _sanitise_list now raises CriteriaLimitExceeded when a value exceeds max_value_length. Previously the value was silently dropped, which produced a query that looked valid but reflected only part of the user's filter — a correctness bug harder to detect than a visible error. Callers catch CriteriaLimitExceeded and return a 400.
… binding Without this, named placeholders (:p0, :p1) from the criteria SQL builder could never be bound. they would reach the database as literal text. params defaults to None so existing callers are unaffected.
… models CriteriaGroup and Group support the new groups format in the filter endpoint. Setting Optional fields to = None fixes a crash when geometry or criteria are absent. previously len(None) would raise TypeError before any security checks could run.
- Validate table_name against safe identifier regex before interpolation - Bind geometry UUIDs as named params instead of raw string interpolation - Fix criteria handling: call generate_criteria_sql with criteria list directly, unpack (sql, params) tuple, accumulate and forward all params - Validate metric and GRZ column names and operators against allowlists - Bind metric and GRZ values as named params - Remove broken %(p0)s substitution loop that was a no-op - Remove debug print statements leaking request data to stdout
- Fix runtime crash: generate_criteria_sql returns (sql, params) tuple but was assigned without unpacking, breaking every criteria request - Validate table_name against safe identifier regex - Build two parallel WHERE clauses: named params for the outer CTE, safely-escaped literals for the pgr_connectedComponents dollar-quoted block where bound params cannot reach - Bind geometry UUIDs, criteria params, metric/GRZ values, and threshold as named params in the outer query - Validate metric and GRZ column names and operators against allowlists - Remove debug print statement
…injection geometry_operations.py: - Validate tableName against safe identifier regex before interpolation - Bind GeoJSON geometry string as named param :geom instead of raw %s interpolation, user-derived geometry data no longer touches SQL - Remove unused Session import isochronesFuncs.py: - Validate mode against explicit allowlist (walk_network, bike_network, drive_network) — mode is used as a table name in four positions including inside a pgr_drivingDistance string literal where named params cannot bind; the allowlist makes interpolation safe - Bind lng, lat, and time as named params in the outer query
- Validate target_column.name against safe identifier regex — this field is user-supplied and was interpolated raw as a column name in the WHERE clause - Bind target_value_list integers as named params :v0, :v1 ... instead of raw tuple interpolation (also fixes trailing-comma bug on single element tuples) - Catch ValueError from validation and return 400 instead of 500
46 core behaviour tests covering empty inputs, single clauses, combinations, key precedence, SQL structure, limits, and real-world scenarios. 16 security tests covering SQL injection (8), Denial of Service (3), and semantic gap handling (5). 32 edge case tests covering malformed data, invalid structure, and boundary values.
_sanitise_list now raises instead of silently dropping oversized values. The test was written before that change and checked for sql == ""; update it to use pytest.raises(CriteriaLimitExceeded) to match the new behaviour.
parcel_maximizer.py: backslash escapes inside nested f-string expressions are not allowed before Python 3.12. Extract the UUID quoting logic to a plain variable to eliminate the nested f-string. isochronesFuncs.py: the broad except Exception block was catching the ValueError raised by mode allowlist validation and re-wrapping it as a 500, so the router ValueError handler never saw it. Add an explicit except (ValueError, HTTPException): raise so validation errors reach the router and return 400.
After rebasing on the remote conflict-resolution commits, the file ended up with two execute_sql_query calls: our parameterised version (correct) followed by the original no-params version (which overwrote the result and bypassed parameter binding). Remove the duplicate.
- Changed bare `from ligfinderFunc import` to relative package imports so tests run correctly under pytest with the package structure - Fixed all param-value assertions from `x in params` (key lookup) to `x in params.values()` (value lookup) to match the named-param dict - Updated placeholder assertions from `"%s" in sql` to `":p" in sql` to reflect the named-parameter binding style (:p0, :p1, ...) - Corrected undefined `result` variable references to `sql` in test_edge_cases_ligfinder error messages
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
What was changed
Test plan