Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function _escape_filter_value to escape backslashes and double quotes in Google Cloud Monitoring filter string literals, protecting against filter injection vulnerabilities. It applies this escaping to metric types, group IDs, project lists, prefix/suffix matches, and default equality operators, and adds corresponding unit tests. However, the review comments highlight that the inequality operator (!=) and comparison operators (such as greater and greaterequal) still lack proper quoting and escaping, which leaves them vulnerable to filter injection and syntax errors.
| @@ -635,6 +653,7 @@ def _build_label_filter(category, *args, **kwargs): | |||
| term = "{key} != {value}" | |||
There was a problem hiding this comment.
The != operator does not enclose {value} in double quotes. This leads to both a security vulnerability (filter injection) and a correctness issue, as the Cloud Monitoring API requires string literals to be double-quoted. Since the default = operator in the else block quotes and escapes the value, the != operator should do the same.
| term = "{key} != {value}" | |
| term = '{key} != "{value}"' | |
| value = _escape_filter_value(value) |
There was a problem hiding this comment.
Good catch. != is the string counterpart to =, so it now quotes and routes the value through _escape_filter_value the same way. Added a regression test.
| elif suffix == "greater": | ||
| term = "{key} > {value}" | ||
| elif suffix == "greaterequal": |
There was a problem hiding this comment.
The comparison operators greater and greaterequal (as well as less and lessequal which are outside this diff hunk) do not enclose {value} in double quotes.
If these operators are intended only for numeric comparisons, we should validate that value is a safe numeric representation (e.g., by verifying it can be parsed as a float/int) to prevent filter injection. If they are also intended for lexicographical string comparisons, they must be double-quoted and escaped using _escape_filter_value(value) to comply with the Cloud Monitoring API syntax and prevent injection.
There was a problem hiding this comment.
Those operators are documented as INT64 numeric comparisons, so they stay unquoted. I added _numeric_filter_value to reject any value that does not parse as a number (covers greater/greaterequal/less/lessequal), which closes the injection path without breaking the numeric semantics. Test added.
This branch has not been deployed
1 participant
_build_label_filter and _Filter.str in monitoring_v3/query.py interpolate label values (and the metric type, group id, and project) into double-quoted Cloud Monitoring filter literals without escaping, so a value passed to Query.select_metrics/select_resources that contains a double quote closes the literal and injects filter syntax, for example select_metrics(instance_name='x" OR metric.label.y = "z') broadens the returned time series. Route every value that lands in a quoted filter literal through a helper that backslash-escapes backslashes and double quotes per the Monitoring filter syntax. Legitimate values are unchanged and the numeric comparison operators are untouched; regression tests are added in test_query.py.