Skip to content

feat: Add timezone to ALLOWED_FUNCTIONS - #209

Open
jack-r-warren wants to merge 1 commit into
crystaldba:mainfrom
jack-r-warren:add-timezone
Open

jack-r-warren wants to merge 1 commit into
crystaldba:mainfrom
jack-r-warren:add-timezone

Conversation

@jack-r-warren

Copy link
Copy Markdown

The timezone function is documented further down on https://www.postgresql.org/docs/current/functions-datetime.html than "Table 9.33. Date/Time Functions", it's here:

The function timezone(zone, timestamp) is equivalent to the SQL-conforming construct timestamp AT TIME ZONE zone.

The function timezone(zone, time) is equivalent to the SQL-conforming construct time AT TIME ZONE zone.

The function timezone(timestamp) is equivalent to the SQL-conforming construct timestamp AT LOCAL.

The function timezone(time) is equivalent to the SQL-conforming construct time AT LOCAL.

I'm copying the pattern from #118, happy to make any changes.

@jssmith jssmith added the openhands-review Pull requests that need OpenHands review label Aug 20, 2026

@jssmith jssmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review

Verdict: Approve. Clean, minimal change.

What it does

Adds "timezone" to ALLOWED_FUNCTIONS in SafeSqlDriver. This is a read-only date/time conversion function per the PostgreSQL docs.

Notable side effect — also fixes AT TIME ZONE

pglast parses the SQL AT TIME ZONE syntax into a FuncCall with funcname = pg_catalog.timezone. Before this PR, restricted mode would block SELECT ... AT TIME ZONE 'America/Denver' because the timezone function wasn't on the allowlist. Adding it unblocks both the explicit timezone(zone, timestamp) call and the AT TIME ZONE construct. Worth noting in the PR description, but it's a correct and welcome fix.

Verification

  • timezone() is a pure function — no file, network, or system access. No security concern.
  • Both new test queries pass. All 60 tests in test_safe_sql.py pass.
  • ruff check clean on both modified files.
  • The second test case (AT TIME ZONE) is valuable — it covers the SQL-syntax path, not just the explicit function call.

Minor note (non-blocking)

The two test queries are inserted in the middle of the existing test_datetime_functions list, which is fine. If more function additions follow this pattern, consider splitting datetime tests into their own dedicated test rather than growing the single list indefinitely — but not worth doing now.


This review was created by an AI agent (OpenHands) on behalf of @jssmith.

@jssmith jssmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review Summary

Approving. The change is correct, minimal, and well-tested.

What the PR does

Adds timezone to ALLOWED_FUNCTIONS in the safe SQL driver, permitting both the explicit timezone(zone, timestamp) function call and the SQL-standard AT TIME ZONE construct (which pglast internally rewrites to a timezone() function call).

Verification

  • All 60 tests in test_safe_sql.py pass.
  • Confirmed both test cases parse to a FuncCall with funcname='timezone', so both are directly exercising the allowlist change.
  • Confirmed both queries fail on main without the addition, proving the tests are meaningful.
  • timezone is a pure date/time conversion function — no side effects, no file/system access. Safe to allowlist.

One minor observation (non-blocking)

The AT TIME ZONE test case is a nice addition since it catches the non-obvious case where PostgreSQL's parser rewrites the SQL syntax into a function call. Good defensive testing.

No changes requested. Ready to merge.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

@jssmith jssmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: Add timezone to ALLOWED_FUNCTIONS

Verdict: Approve — clean, correct, minimal change.

What it does

Adds "timezone" to the ALLOWED_FUNCTIONS set in safe_sql.py, allowing the timezone() function through the safe SQL driver. Two test cases are added covering both the function-call form and the AT TIME ZONE syntactic form.

Correctness

  • pglast parses AT TIME ZONE as pg_catalog.timezone() internally, so both SQL forms resolve to the same function name. The existing PG_CATALOG_PATTERN strips the pg_catalog. prefix, so pg_catalog.timezonetimezone matches the new allowlist entry. Verified via AST inspection.
  • Function names are already lowercased (func_name = ... .lower()), so the lowercase "timezone" entry is correct regardless of how the user writes the SQL.
  • All 60 tests in test_safe_sql.py pass, including the new cases.

Security

timezone is a pure date/time conversion function with no side effects — consistent with the other datetime functions already in the allowlist (now, timeofday, date_part, etc.). No injection or data-modification risk.

Minor observations (non-blocking)

  • The PR description references PR #118 as a pattern to follow — consistent with how the allowlist is maintained.
  • The two test queries use realistic PostgreSQL docs examples. Good for readability.

No changes requested.

This review was created by an AI agent (OpenHands) on behalf of the user.

@jssmith jssmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: Add timezone to ALLOWED_FUNCTIONS

Verdict: Approve

Summary

One-line addition of timezone to the ALLOWED_FUNCTIONS allowlist, placed alphabetically in the Date/Time Functions section. Two test cases added covering both the function-call form and the AT TIME ZONE SQL syntax.

Analysis

  • Both SELECT timezone(...) and expr AT TIME ZONE zone parse to FuncCall with funcname=timezone in pglast, so both test queries are directly relevant to the change.
  • The timezone() function is a read-only datetime conversion with no side effects or sensitive system exposure. All four documented overloads (zone+timestamp, zone+time, timestamp, time) are safe.
  • The test in test_datetime_functions correctly asserts both queries pass through the safe driver and are forwarded with the expected /* crystaldba */ prefix and force_readonly=True.

Verification

  • Ran the full test_safe_sql.py suite locally: 60/60 passed.

No issues found

The change is minimal, correct, and well-tested. No blocking or advisory comments.

This review was created by an AI agent (OpenHands) on behalf of jssmith.

@jssmith jssmith removed the openhands-review Pull requests that need OpenHands review label Aug 20, 2026

@jssmith jssmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved. timezone() is a pure, side-effect-free datetime conversion function — no filesystem access, code execution, or system-info exposure. It fits naturally alongside the other allowed datetime functions (now, date_part, date_trunc, etc.).

Verification:

  • All 60 tests in test_safe_sql.py pass
  • The first test query (timezone('UTC', ...)) directly exercises the new allowlist entry
  • Alphabetical ordering is maintained in ALLOWED_FUNCTIONS

Minor note (non-blocking): The second test query (AT TIME ZONE syntax) exercises the existing SQL construct rather than the new allowlist entry — pglast parses AT TIME ZONE as an A_Expr, not a FuncCall, so it was already allowed. Both forms are worth covering though.

This review was created by an AI agent (OpenHands) on behalf of @jssmith.

@jssmith jssmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review

Verdict: Approve — clean, correct, minimal.

What it does

Adds "timezone" to ALLOWED_FUNCTIONS in SafeSqlDriver, alphabetically placed in the Date/Time Functions section. Two test cases added to test_datetime_functions.

Verification (independent)

  • AST inspection: Both timezone('UTC', ...) and expr AT TIME ZONE 'America/Denver' parse to a FuncCall with funcname = pg_catalog.timezone. The existing PG_CATALOG_PATTERN strips the pg_catalog. prefix, so both resolve to timezone and match the new allowlist entry.
  • Tests are meaningful: Confirmed both queries are blocked on main and allowed with this change.
  • All 60 tests in test_safe_sql.py pass. ruff check clean on both files.

Security

timezone() is a pure datetime conversion — no file, network, or system access. Safe alongside the other allowed datetime functions.

Correction to a prior review

Review #4979241001 claims AT TIME ZONE "parses as an A_Expr, not a FuncCall, so it was already allowed." This is incorrect — per AST inspection it produces a FuncCall (funcformat=COERCE_SQL_SYNTAX), and on main both forms are blocked. The AT TIME ZONE test case genuinely exercises the allowlist change.

No changes requested.

This review was created by an AI agent (OpenHands) on behalf of @jssmith.

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

Labels

openhands-review Pull requests that need OpenHands review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants