Use parameterized SQL in query_tracks() to prevent injection#78
Use parameterized SQL in query_tracks() to prevent injection#78arpitjain099 wants to merge 1 commit into
Conversation
Replace f-string interpolation of date values in the SQL query with SQLAlchemy text() bound parameters. This prevents potential SQL injection through the start_date and end_date arguments. The spatial filter (ST_Intersects with WKT) is left as a literal since the geometry is derived from an internally-constructed GeoDataFrame, not from external user input. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
elliott-ruebush
left a comment
elliott-ruebush
left a comment
There was a problem hiding this comment.
I don't know that we need protection from SQL injection atm, but this is a cheap change, so seems reasonable to move ahead with it for now.
| wheres = [f"fp.ak_datetime::date BETWEEN '{start_date}' AND '{end_date}'"] # start and end date are inclusive | ||
| # Use bound parameters for date values to prevent SQL injection | ||
| wheres = ["fp.ak_datetime::date BETWEEN :start_date AND :end_date"] | ||
| params = {"start_date": start_date, "end_date": end_date} |
There was a problem hiding this comment.
May be out of scope for this PR, but should we change the type of these to datetime while we're editing the code here? Doing so may help with ensuring they're converted properly for use in this SQL query.
There was a problem hiding this comment.
Good idea. The start/end values come in as strings from the config and get passed straight through to the query. Converting them to datetime objects at the boundary (right before the query) would catch format issues early and make the parameterized query more robust. I can add that in this PR if you'd like, or keep it scoped and handle the conversion separately.
| ak_albers_mask = mask.to_crs(epsg=3338) | ||
| mask.geometry = ak_albers_mask.buffer( | ||
| mask_buffer_distance).to_crs(epsg=4326) | ||
| # Spatial filter uses internally-generated WKT from the mask geometry, |
There was a problem hiding this comment.
We don't have external users inputting things into this software from anywhere at the moment, so theoretically you could argue we don't need the protection from SQL injection for any queries at the moment. However, I think it's pretty reasonable to use passed params just for best practice and in case we use this behind a user-facing application in the future.
There was a problem hiding this comment.
Agreed, no external user input today but parameterized queries are just good hygiene. Thanks for the approval.
2 participants
Summary
The
query_tracks()function innps_active_space/utils/helpers.pybuilds its SQL query by interpolatingstart_dateandend_datedirectly into the query string via f-strings. If those values ever come from user-controlled input, this could allow SQL injection.This change switches the date clause to use SQLAlchemy's
text()with bound parameters (:start_date,:end_date), so the database driver handles escaping safely. The spatial filter (ST_Intersectswith WKT geometry) is left as a string literal since it is derived from an internally-constructed GeoDataFrame, not from external input.What changed
from sqlalchemy import textimporttext(...).bindparams(...)before passing it tofrom_postgis()The query logic and output are unchanged.
gpd.GeoDataFrame.from_postgis()acceptstext()objects the same way it accepts raw strings.Testing