Skip to content

fix(cdp): throw if select in filters#30250

Merged
meikelmosby merged 1 commit intomasterfrom
select-throws-in-filters
Mar 21, 2025
Merged

fix(cdp): throw if select in filters#30250
meikelmosby merged 1 commit intomasterfrom
select-throws-in-filters

Conversation

@mariusandra
Copy link
Collaborator

Problem

Turns out you can write subqueries in HogQL filters in data pipelines.

Changes

Throws an error when saving the filter, not later during runtime.

2025-03-20 21 00 22

How did you test this code?

In the browser and added a test

@mariusandra mariusandra requested review from a team and meikelmosby March 20, 2025 20:00
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR adds a security check to prevent SQL injection via subqueries in HogQL filters for data pipelines.

  • Added SelectFinder visitor class in posthog/cdp/filters.py to detect SELECT queries in filter ASTs
  • Modified compile_filters_bytecode to throw early error during filter saving rather than at runtime
  • Added test case test_filters_raises_on_select in posthog/cdp/test/test_filters.py to verify SELECT detection
  • Error message could be more specific about why SELECT queries are not allowed in filters

2 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +110 to +111
if SelectFinder.has_select(expr):
raise Exception("Select queries are not allowed in filters")
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Generic Exception is too broad. Consider creating a specific exception type for filter validation errors.

Comment on lines +94 to +96
def visit_select_query(self, node):
self.found = True
return
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: visit_select_query() should call super().visit_select_query(node) to ensure proper traversal of child nodes

Comment on lines +91 to +92
class SelectFinder(TraversingVisitor):
found = False
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: found should be instance-specific, not class-level, to avoid state sharing between instances

Suggested change
class SelectFinder(TraversingVisitor):
found = False
class SelectFinder(TraversingVisitor):
def __init__(self):
super().__init__()
self.found = False

@meikelmosby meikelmosby merged commit ae9cd5c into master Mar 21, 2025
91 checks passed
@meikelmosby meikelmosby deleted the select-throws-in-filters branch March 21, 2025 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants