Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 57 additions & 5 deletions snuba/web/rpc/common/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,14 @@
not_cond,
or_cond,
)
from snuba.query.expressions import Expression, FunctionCall, SubscriptableReference
from snuba.query.expressions import (
Argument,
DangerousRawSQL,
Expression,
FunctionCall,
Lambda,
SubscriptableReference,
)
from snuba.web.rpc.common.exceptions import BadSnubaRPCRequestException


Expand All @@ -53,6 +60,24 @@ def attribute_key_to_expression(attr_key: AttributeKey) -> Expression:
BUCKET_COUNT = 40


def _build_array_attr_expression(attr_name: str) -> DangerousRawSQL:
"""Build a DangerousRawSQL expression that extracts string values from
the attributes_array JSON column for a given attribute name."""
safe_name = attr_name.replace("`", "``")
Copy link

Choose a reason for hiding this comment

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

Incorrect identifier escaping in raw SQL construction

High Severity

The _build_array_attr_expression escapes backticks by doubling (replace("", "")`), but the codebase's own `escape_identifier` in `snuba/clickhouse/escaping.py` uses backslash-prefix escaping for both backticks and backslashes ( `` → `` \ , `\` → `\\`). This inconsistency means backslashes in `attr_name` are not escaped at all. An attribute name containing a backslash followed by content could cause ClickHouse to misparse the backtick-quoted identifier boundary, since `\ would be interpreted as an escaped backtick rather than the closing delimiter. Since this flows through DangerousRawSQL which bypasses all safety checks, this could lead to SQL parsing errors or injection.

Fix in Cursor Fix in Web

sql = (
f"arrayMap("
f"x -> coalesce("
f"x.`String`, "
f"toString(x.`Int`), "
f"toString(x.`Double`), "
f"toString(x.`Bool`)"
f"), "
f"attributes_array.`{safe_name}`.:`Array(JSON)`"
f")"
)
return DangerousRawSQL(alias=None, sql=sql)


def transform_array_value(value: dict[str, str]) -> Any:
for t, v in value.items():
if t == "Int":
Expand Down Expand Up @@ -216,16 +241,43 @@ def trace_item_filters_to_expression(

if item_filter.HasField("comparison_filter"):
k = item_filter.comparison_filter.key
k_expression = attribute_key_to_expression(k)
op = item_filter.comparison_filter.op
v = item_filter.comparison_filter.value

# TYPE_ARRAY only supports LIKE/NOT_LIKE — handle it early
# before calling attribute_key_to_expression (which doesn't support TYPE_ARRAY).
if k.type == AttributeKey.Type.TYPE_ARRAY:
if op not in (ComparisonFilter.OP_LIKE, ComparisonFilter.OP_NOT_LIKE):
raise BadSnubaRPCRequestException(
"only LIKE and NOT_LIKE comparisons are supported on array keys"
)
if v.WhichOneof("value") != "val_str":
raise BadSnubaRPCRequestException(
"LIKE/NOT_LIKE on array keys requires a string pattern"
)
v_expression: Expression = literal(v.val_str)
array_expression = _build_array_attr_expression(k.name)
like_fn = f.ilike if item_filter.comparison_filter.ignore_case else f.like
exists_expr = f.arrayExists(
Lambda(
None,
("x",),
like_fn(Argument(None, "x"), v_expression),
),
array_expression,
)
if op == ComparisonFilter.OP_NOT_LIKE:
return not_cond(exists_expr)
return exists_expr

k_expression = attribute_key_to_expression(k)

value_type = v.WhichOneof("value")
if value_type is None:
raise BadSnubaRPCRequestException("comparison does not have a right hand side")

if v.is_null:
v_expression: Expression = literal(None)
v_expression = literal(None)
else:
match value_type:
case "val_bool":
Expand Down Expand Up @@ -284,14 +336,14 @@ def trace_item_filters_to_expression(
if op == ComparisonFilter.OP_LIKE:
if k.type != AttributeKey.Type.TYPE_STRING:
raise BadSnubaRPCRequestException(
"the LIKE comparison is only supported on string keys"
"the LIKE comparison is only supported on string and array keys"
)
comparison_function = f.ilike if item_filter.comparison_filter.ignore_case else f.like
return comparison_function(k_expression, v_expression)
if op == ComparisonFilter.OP_NOT_LIKE:
if k.type != AttributeKey.Type.TYPE_STRING:
raise BadSnubaRPCRequestException(
"the NOT LIKE comparison is only supported on string keys"
"the NOT LIKE comparison is only supported on string and array keys"
)
comparison_function = (
f.notILike if item_filter.comparison_filter.ignore_case else f.notLike
Expand Down
144 changes: 143 additions & 1 deletion tests/web/rpc/test_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,20 @@
from google.protobuf.timestamp_pb2 import Timestamp
from sentry_protos.snuba.v1.error_pb2 import Error as ErrorProto
from sentry_protos.snuba.v1.request_common_pb2 import RequestMeta
from sentry_protos.snuba.v1.trace_item_attribute_pb2 import AttributeKey, AttributeValue
from sentry_protos.snuba.v1.trace_item_filter_pb2 import ComparisonFilter, TraceItemFilter

from snuba import settings
from snuba.web.rpc.common.common import next_monday, prev_monday, use_sampling_factor
from snuba.query.expressions import DangerousRawSQL, FunctionCall, Lambda
from snuba.web.rpc.common.common import (
attribute_key_to_expression,
next_monday,
prev_monday,
trace_item_filters_to_expression,
use_sampling_factor,
)
from snuba.web.rpc.common.exceptions import (
BadSnubaRPCRequestException,
RPCAllocationPolicyException,
convert_rpc_exception_to_proto,
)
Expand Down Expand Up @@ -45,6 +55,138 @@ def test_use_sampling_factor(self, snuba_set_config: SnubaSetConfig) -> None:
assert not use_sampling_factor(RequestMeta(start_timestamp=Timestamp(seconds=9)))


class TestTraceItemFiltersArrayLike:
def _make_like_filter(
self,
attr_name: str,
attr_type: AttributeKey.Type.ValueType,
pattern: str,
op: ComparisonFilter.Op.ValueType = ComparisonFilter.OP_LIKE,
ignore_case: bool = False,
) -> TraceItemFilter:
return TraceItemFilter(
comparison_filter=ComparisonFilter(
key=AttributeKey(type=attr_type, name=attr_name),
op=op,
value=AttributeValue(val_str=pattern),
ignore_case=ignore_case,
)
)

def test_like_on_array_key(self) -> None:
item_filter = self._make_like_filter("my_tags", AttributeKey.Type.TYPE_ARRAY, "%error%")
result = trace_item_filters_to_expression(item_filter, attribute_key_to_expression)
assert isinstance(result, FunctionCall)
assert result.function_name == "arrayExists"
# First param is a Lambda with like
lam = result.parameters[0]
assert isinstance(lam, Lambda)
assert lam.parameters == ("x",)
assert isinstance(lam.transformation, FunctionCall)
assert lam.transformation.function_name == "like"
# Second param is the array expression (DangerousRawSQL)
assert isinstance(result.parameters[1], DangerousRawSQL)

def test_like_on_array_key_ignore_case(self) -> None:
item_filter = self._make_like_filter(
"my_tags", AttributeKey.Type.TYPE_ARRAY, "%error%", ignore_case=True
)
result = trace_item_filters_to_expression(item_filter, attribute_key_to_expression)
assert isinstance(result, FunctionCall)
assert result.function_name == "arrayExists"
lam = result.parameters[0]
assert isinstance(lam, Lambda)
assert isinstance(lam.transformation, FunctionCall)
assert lam.transformation.function_name == "ilike"

def test_not_like_on_array_key(self) -> None:
item_filter = self._make_like_filter(
"my_tags",
AttributeKey.Type.TYPE_ARRAY,
"%error%",
op=ComparisonFilter.OP_NOT_LIKE,
)
result = trace_item_filters_to_expression(item_filter, attribute_key_to_expression)
# Result should be NOT(arrayExists(...))
assert isinstance(result, FunctionCall)
assert result.function_name == "not"
inner = result.parameters[0]
assert isinstance(inner, FunctionCall)
assert inner.function_name == "arrayExists"
lam = inner.parameters[0]
assert isinstance(lam, Lambda)
assert isinstance(lam.transformation, FunctionCall)
assert lam.transformation.function_name == "like"

def test_not_like_on_array_key_ignore_case(self) -> None:
item_filter = self._make_like_filter(
"my_tags",
AttributeKey.Type.TYPE_ARRAY,
"%error%",
op=ComparisonFilter.OP_NOT_LIKE,
ignore_case=True,
)
result = trace_item_filters_to_expression(item_filter, attribute_key_to_expression)
assert isinstance(result, FunctionCall)
assert result.function_name == "not"
inner = result.parameters[0]
assert isinstance(inner, FunctionCall)
assert inner.function_name == "arrayExists"
lam = inner.parameters[0]
assert isinstance(lam, Lambda)
assert isinstance(lam.transformation, FunctionCall)
assert lam.transformation.function_name == "ilike"

def test_like_on_array_key_non_string_value_raises(self) -> None:
item_filter = TraceItemFilter(
comparison_filter=ComparisonFilter(
key=AttributeKey(type=AttributeKey.Type.TYPE_ARRAY, name="my_tags"),
op=ComparisonFilter.OP_LIKE,
value=AttributeValue(val_int=42),
)
)
with pytest.raises(
BadSnubaRPCRequestException,
match="LIKE/NOT_LIKE on array keys requires a string pattern",
):
trace_item_filters_to_expression(item_filter, attribute_key_to_expression)

def test_equals_on_array_key_raises(self) -> None:
item_filter = TraceItemFilter(
comparison_filter=ComparisonFilter(
key=AttributeKey(type=AttributeKey.Type.TYPE_ARRAY, name="my_tags"),
op=ComparisonFilter.OP_EQUALS,
value=AttributeValue(val_str="something"),
)
)
with pytest.raises(
BadSnubaRPCRequestException,
match="only LIKE and NOT_LIKE comparisons are supported on array keys",
):
trace_item_filters_to_expression(item_filter, attribute_key_to_expression)

def test_like_on_int_key_raises(self) -> None:
item_filter = self._make_like_filter("my_int", AttributeKey.Type.TYPE_INT, "%something%")
with pytest.raises(
BadSnubaRPCRequestException,
match="LIKE comparison is only supported on string and array keys",
):
trace_item_filters_to_expression(item_filter, attribute_key_to_expression)

def test_not_like_on_int_key_raises(self) -> None:
item_filter = self._make_like_filter(
"my_int",
AttributeKey.Type.TYPE_INT,
"%something%",
op=ComparisonFilter.OP_NOT_LIKE,
)
with pytest.raises(
BadSnubaRPCRequestException,
match="NOT LIKE comparison is only supported on string and array keys",
):
trace_item_filters_to_expression(item_filter, attribute_key_to_expression)


@pytest.mark.redis_db
@pytest.mark.clickhouse_db
def test_convert_rpc_exception_to_proto_packs_details() -> None:
Expand Down
Loading