Repository navigation
Conversation
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.
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.
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.
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.