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.
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.
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.
| def _prepare_tzname_delta(self, tzname): | ||
| if not tzname or tzname == "UTC": | ||
| return "UTC" | ||
| if "+" not in tzname and "-" not in tzname: | ||
| return tzname | ||
| if self._well_formatted_offset_pattern.fullmatch(tzname): | ||
| return tzname | ||
| if tzname.startswith("UTC") and self._well_formatted_offset_pattern.fullmatch( | ||
| tzname[3:] | ||
| ): | ||
| return tzname[3:] | ||
|
|
||
| tzname = self._four_digit_offset_pattern.sub(r"\1\2:\3", tzname) | ||
| tzname = self._two_digit_offset_pattern.sub(r"\1\2:00", tzname) | ||
| tzname, sign, offset = split_tzname_delta(tzname) | ||
| if offset: | ||
| if len(offset) in (7, 8) and offset.endswith(":00"): | ||
| offset = offset[:-3] | ||
| if len(offset) == 4 and offset.isdigit(): | ||
| offset = f"{offset[:2]}:{offset[2:]}" | ||
| return f"{sign}{offset}" | ||
| return tzname |
There was a problem hiding this comment.
Standard TZ database names can contain + or - characters (for example, Etc/GMT+5 or Etc/GMT-10). In the POSIX/TZ database standard, Etc/GMT+5 represents GMT-5 (5 hours behind UTC). Under the current implementation, since it contains +, it gets parsed by split_tzname_delta and normalized to +05:00 (which represents GMT+5), effectively reversing the timezone offset and causing incorrect query results. To prevent this, we should only normalize the timezone name if it is a pure offset representation (i.e., it starts with +, -, UTC+, or UTC-). If it is a named timezone, we should return it as-is so that Cloud Spanner can resolve it correctly.
def _prepare_tzname_delta(self, tzname):
if not tzname or tzname == "UTC":
return "UTC"
is_offset = tzname.startswith(("+", "-")) or (
tzname.startswith("UTC") and len(tzname) > 3 and tzname[3] in ("+", "-")
)
if not is_offset:
return tzname
if self._well_formatted_offset_pattern.fullmatch(tzname):
return tzname
if tzname.startswith("UTC") and self._well_formatted_offset_pattern.fullmatch(
tzname[3:]
):
return tzname[3:]
tzname = self._four_digit_offset_pattern.sub(r"\1\2:\3", tzname)
tzname = self._two_digit_offset_pattern.sub(r"\1\2:00", tzname)
tzname, sign, offset = split_tzname_delta(tzname)
if offset:
if len(offset) in (7, 8) and offset.endswith(":00"):
offset = offset[:-3]
if len(offset) == 4 and offset.isdigit():
offset = f"{offset[:2]}:{offset[2:]}"
return f"{sign}{offset}"
return tzname| self.assertEqual( | ||
| self.db_operations._prepare_tzname_delta("America/New_York"), | ||
| "America/New_York", | ||
| ) | ||
| self.assertEqual(self.db_operations._prepare_tzname_delta("UTC"), "UTC") | ||
| self.assertEqual(self.db_operations._prepare_tzname_delta(""), "UTC") | ||
| self.assertEqual(self.db_operations._prepare_tzname_delta(None), "UTC") |
There was a problem hiding this comment.
Add test assertions to verify that named timezones containing + or - (such as Etc/GMT+5 and Etc/GMT-5) are preserved as-is and not incorrectly parsed as offsets.
self.assertEqual(
self.db_operations._prepare_tzname_delta("America/New_York"),
"America/New_York",
)
self.assertEqual(
self.db_operations._prepare_tzname_delta("Etc/GMT+5"),
"Etc/GMT+5",
)
self.assertEqual(
self.db_operations._prepare_tzname_delta("Etc/GMT-5"),
"Etc/GMT-5",
)
self.assertEqual(self.db_operations._prepare_tzname_delta("UTC"), "UTC")
self.assertEqual(self.db_operations._prepare_tzname_delta(""), "UTC")
self.assertEqual(self.db_operations._prepare_tzname_delta(None), "UTC")
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.