feat: Add timezone to ALLOWED_FUNCTIONS - #209
jack-r-warren wants to merge 1 commit into
Conversation
jssmith
left a comment
There was a problem hiding this comment.
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.pypass. ruff checkclean 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
left a comment
There was a problem hiding this comment.
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.pypass. - Confirmed both test cases parse to a
FuncCallwithfuncname='timezone', so both are directly exercising the allowlist change. - Confirmed both queries fail on
mainwithout the addition, proving the tests are meaningful. timezoneis 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
left a comment
There was a problem hiding this comment.
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 ZONEaspg_catalog.timezone()internally, so both SQL forms resolve to the same function name. The existingPG_CATALOG_PATTERNstrips thepg_catalog.prefix, sopg_catalog.timezone→timezonematches 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.pypass, 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
left a comment
There was a problem hiding this comment.
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(...)andexpr AT TIME ZONE zoneparse toFuncCallwithfuncname=timezonein 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_functionscorrectly asserts both queries pass through the safe driver and are forwarded with the expected/* crystaldba */prefix andforce_readonly=True.
Verification
- Ran the full
test_safe_sql.pysuite 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
left a comment
There was a problem hiding this comment.
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.pypass - 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
left a comment
There was a problem hiding this comment.
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', ...)andexpr AT TIME ZONE 'America/Denver'parse to aFuncCallwithfuncname = pg_catalog.timezone. The existingPG_CATALOG_PATTERNstrips thepg_catalog.prefix, so both resolve totimezoneand match the new allowlist entry. - Tests are meaningful: Confirmed both queries are blocked on
mainand allowed with this change. - All 60 tests in
test_safe_sql.pypass.ruff checkclean 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.
The
timezonefunction is documented further down on https://www.postgresql.org/docs/current/functions-datetime.html than "Table 9.33. Date/Time Functions", it's here:I'm copying the pattern from #118, happy to make any changes.