GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix(django-spanner): prevent SQL injection in date and time functions by olavloite · Pull Request #19040 · googleapis/google-cloud-python

Conversation

@olavloite

@olavloite olavloite commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gemini-code-assist[bot] reacted with eyes emoji
All reactions
  • 👀 1 reaction
@olavloite
olavloite requested a review from a team as a code owner November 10, 2026 16:08
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Nov 10, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread packages/django-google-spanner/django_spanner/operations.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@olavloite
olavloite force-pushed the spanner-django-use-query-params-for-time-funcs branch from b7010af to ed9479d Compare November 10, 2026 16:18
@olavloite

olavloite commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review
gemini-code-assist[bot] reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Nov 10, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@olavloite
olavloite force-pushed the spanner-django-use-query-params-for-time-funcs branch from ed9479d to 01126c6 Compare November 10, 2026 19:21
@olavloite

olavloite commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review
gemini-code-assist[bot] reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Nov 10, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread packages/django-google-spanner/django_spanner/operations.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread packages/django-google-spanner/tests/unit/django_spanner/test_operations.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@olavloite
olavloite force-pushed the spanner-django-use-query-params-for-time-funcs branch from 01126c6 to f53e9c7 Compare November 11, 2026 08:20
@olavloite

olavloite commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review
gemini-code-assist[bot] reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Nov 11, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread packages/django-google-spanner/django_spanner/operations.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@olavloite
olavloite force-pushed the spanner-django-use-query-params-for-time-funcs branch from f53e9c7 to 1f75519 Compare November 11, 2026 09:19
@olavloite

olavloite commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review
gemini-code-assist[bot] reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Nov 11, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread packages/django-google-spanner/django_spanner/operations.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@olavloite
olavloite force-pushed the spanner-django-use-query-params-for-time-funcs branch from 1f75519 to 86b87eb Compare November 11, 2026 10:05
@olavloite

olavloite commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review
gemini-code-assist[bot] reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Nov 11, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@olavloite
fix(django-spanner): prevent SQL injection in date and time functions
4ee67f7 
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.
@olavloite
olavloite force-pushed the spanner-django-use-query-params-for-time-funcs branch from 86b87eb to 4ee67f7 Compare November 11, 2026 12:54
@olavloite
olavloite requested a review from sakthivelmanii November 11, 2026 13:01

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sakthivelmanii sakthivelmanii Awaiting requested review from sakthivelmanii
1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

1 participant

@olavloite
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.