Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses SQL injection vulnerabilities in datetime extraction and truncation operations by validating lookup types against an identifier regex and parameterizing timezone offsets rather than interpolating them directly into SQL queries. It also adds comprehensive unit and mock server tests to verify these changes. The feedback points out a bug in the timezone offset formatting logic within _prepare_tzname_delta where 4-digit offsets (e.g., "-0443") are not correctly handled due to an overly restrictive length check, and provides a robust suggestion to fix it.
b7010af to
ed9479d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses SQL injection vulnerabilities in date/time extract and truncation operations by validating lookup types against an identifier regex and parameterizing timezone names instead of interpolating them directly into SQL strings. Additionally, comprehensive unit and mock server tests have been added to verify the validation logic, parameterization, and defense against SQL injection. No review comments were provided, so there is no feedback to address.
ed9479d to
01126c6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request secures the Django Google Spanner backend against SQL injection by parameterizing timezone offsets in date/time extraction, truncation, and casting operations, and by validating lookup types using regular expressions. The feedback highlights a critical bug in _prepare_tzname_delta where named timezones containing + or - (such as Etc/GMT+5) are incorrectly parsed as pure offsets, reversing the timezone offset. It is recommended to only normalize timezone offsets if they are pure offset representations and to add corresponding test assertions to verify this behavior.
01126c6 to
f53e9c7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances security in the Django Google Spanner backend by validating lookup types and parameterizing timezone names in SQL statements for date/time extraction and truncation, preventing potential SQL injection vulnerabilities. It also updates and adds comprehensive tests. A review comment correctly identifies a regression in time_trunc_sql where the settings.USE_TZ check was omitted, which could cause incorrect timezone conversions when USE_TZ is disabled.
f53e9c7 to
1f75519
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces robust validation and parameterization for date/time extraction, truncation, and casting operations in the Django Google Spanner backend. This prevents SQL injection vulnerabilities by validating lookup types against an identifier regex and passing timezone names/offsets as query parameters rather than interpolating them directly into SQL strings. It also normalizes timezone offsets and handles Django's USE_TZ settings correctly. Feedback on the changes highlights an issue in _prepare_tzname_delta where timezone offsets with single-digit hours and minutes (e.g., +5:30) are not correctly matched by _well_formatted_offset_pattern, potentially bypassing normalization. A simplified regex pattern and normalization logic are suggested to resolve this.
1f75519 to
86b87eb
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves security and timezone handling in django-google-spanner by parameterizing timezone names in SQL queries (using %s instead of direct string interpolation) and validating lookup types against a regex pattern to prevent SQL injection. It also adds extensive unit, mock server, and system tests to verify these changes. I have no feedback to provide as there are no review comments.
Date and time operations (such as extracting date parts, truncating timestamps, and casting dates/times) previously formatted timezone names and lookup types directly into the SQL text as raw strings. When an application accepted timezones or date lookups from untrusted user input, this allowed an attacker to break out of quotes and execute arbitrary SQL. This change eliminates the vulnerability by: 1. Passing timezones as bound query parameters rather than embedding them in the SQL statement. 2. Validating date lookup types against an identifier allowlist before queries are built, following the standard approach used by other Django database engines. 3. Normalizing fixed-offset timezones (e.g. +02:00, UTC+02:00) to the format Cloud Spanner expects while preserving named timezones (such as Etc/GMT+5). Additionally, when no timezone is specified (or when USE_TZ = False), timezone arguments are now omitted from the generated SQL functions (EXTRACT, TIMESTAMP_TRUNC, DATE, FORMAT_TIMESTAMP) rather than falling back to a hardcoded "UTC". This allows Cloud Spanner to evaluate expressions using the database-level default timezone (ALTER DATABASE ... SET OPTIONS (default_time_zone = ...)). Unit, mock server, and emulator system tests have been added and verified, and previously skipped Django SQL injection regression tests have been re-enabled.
86b87eb to
4ee67f7
Compare
Date and time operations (such as extracting date parts or truncating timestamps) previously formatted timezone names and lookup types directly into the SQL text as raw strings. When an application accepted timezones or date lookups from untrusted user input, this allowed an attacker to break out of quotes and execute arbitrary SQL.
This change eliminates the vulnerability by passing timezones as bound query parameters rather than embedding them in the SQL statement. Date lookup types are also validated against an identifier allowlist before queries are built, following the standard approach used by other Django database engines.
Additionally, fixed-offset timezones are now normalized to the format Cloud Spanner expects, and previously skipped Django SQL injection regression tests have been re-enabled and verified.