From d8c315cf4c6ccced9ce855477b2bea830bd79d49 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Wed, 9 Sep 2026 10:08:44 -0700 Subject: [PATCH 1/4] feat(apidocs): Lint request parameters read without a serializer A parameter read straight off request.GET or request.data is invisible twice. The schema has nothing to generate from, so no client can send it, and the value is an unchecked string, so a misspelled key fails at runtime rather than in review. Reading through a serializer named in @extend_schema fixes both at once, because drf-spectacular derives the documented parameters from the same fields the handler validates with. Adds three diagnostics under one rule: S026 a literal key read straight off the query string or request body S027 a key computed at runtime, which no schema can document S028 the whole dict handed to a callable, so what is read is unknowable Counting or iterating the dict is not a hand-off, and building a serializer from it is the target rather than a violation, so neither is reported. Reads through validated_data are accepted. The rule is absent from ENFORCED, so nothing gates. The backlog is 208 raw reads across 67 files, 2 computed keys and 21 hand-offs. Co-Authored-By: Claude Opus 5 (1M context) --- setup.cfg | 4 +- tests/tools/test_flake8_plugin.py | 148 +++++++++++++++++++++++++++++- tools/flake8_plugin.py | 88 ++++++++++++++++++ 3 files changed, 237 insertions(+), 3 deletions(-) diff --git a/setup.cfg b/setup.cfg index 56c4db0e942b..6d846bd497a1 100644 --- a/setup.cfg +++ b/setup.cfg @@ -70,9 +70,9 @@ per-file-ignores = tools/*: S # testing the options manager itself src/sentry/testutils/helpers/options.py, tests/sentry/options/test_manager.py: S011 - # S021-S025 lint the shipped API surface and its lint infrastructure; test + # S021-S028 lint the shipped API surface and its lint infrastructure; test # modules deliberately contain the shapes they exercise - tests/*: S021, S022, S023, S024, S025 + tests/*: S021, S022, S023, S024, S025, S026, S027, S028 [flake8:local-plugins] paths = . diff --git a/tests/tools/test_flake8_plugin.py b/tests/tools/test_flake8_plugin.py index 56dbb4d4578b..1fa688ba79a0 100644 --- a/tests/tools/test_flake8_plugin.py +++ b/tests/tools/test_flake8_plugin.py @@ -1224,7 +1224,7 @@ def _run_input(src: str, enforced: frozenset[str] = frozenset({"declared"})) -> return [ e for e in _run(src, filename="src/sentry/api/endpoints/t.py") - if "S025" in e or "S026" in e or "S027" in e + if any(code in e for code in ("S025", "S026", "S027", "S028")) ] finally: plugin.ENFORCED = original @@ -1388,3 +1388,149 @@ def post(self, request) -> Response[X]: UploadSerializer(data=request.data) """ assert _run_input(src) == [] + + +SHAPED = frozenset({"shaped"}) + + +def test_S026_raw_query_read_is_reported() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + def get(self, request) -> Response[X]: + return request.GET.get("truncate") +""" + errors = _run_input(src, SHAPED) + assert errors == [ + "t.py:5:15: S026 'truncate' is read straight off the query string, so the schema has " + "nothing to document and the value is an unchecked string. Read it through a " + "serializer declared in @extend_schema." + ] + + +def test_S026_raw_body_read_is_reported() -> None: + src = """\ +class E(Endpoint): + publish_status = {"POST": ApiPublishStatus.PUBLIC} + + def post(self, request) -> Response[X]: + return request.data["origin"] +""" + errors = _run_input(src, SHAPED) + assert len(errors) == 1 + assert "'origin' is read straight off the request body" in errors[0] + + +def test_S026_subscript_and_getlist_are_both_reads() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + def get(self, request) -> Response[X]: + a = request.GET["one"] + b = request.GET.getlist("two") + return a, b +""" + assert len(_run_input(src, SHAPED)) == 2 + + +def test_S026_read_through_validated_data_is_accepted() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + @extend_schema(parameters=[QuerySerializer]) + def get(self, request) -> Response[X]: + serializer = QuerySerializer(data=request.GET) + return serializer.validated_data["truncate"] +""" + assert _run_input(src, SHAPED) == [] + + +def test_S026_private_method_is_skipped() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PRIVATE} + + def get(self, request) -> Response[X]: + return request.GET.get("truncate") +""" + assert _run_input(src, SHAPED) == [] + + +def test_S027_computed_key_is_reported() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + def get(self, request) -> Response[X]: + return request.GET.get(some_name) +""" + errors = _run_input(src, SHAPED) + assert len(errors) == 1 + assert errors[0].startswith( + "t.py:5:15: S027 the query string is read with the computed key some_name" + ) + + +def test_S028_hand_off_is_reported() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + def get(self, request) -> Response[X]: + return installation.get_link_issue_config(params=request.GET) +""" + errors = _run_input(src, SHAPED) + assert len(errors) == 1 + assert "handed to get_link_issue_config" in errors[0] + + +def test_S028_container_operations_are_not_hand_offs() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + def get(self, request) -> Response[X]: + return len(request.GET), sorted(request.GET) +""" + assert _run_input(src, SHAPED) == [] + + +def test_S028_building_a_serializer_is_not_a_hand_off() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + @extend_schema(parameters=[QuerySerializer]) + def get(self, request) -> Response[X]: + return QuerySerializer(data=request.GET) +""" + assert _run_input(src, SHAPED) == [] + + +def test_shaped_rule_records_instead_of_gating_when_unenforced() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + def get(self, request) -> Response[X]: + return request.GET.get("truncate") +""" + assert _run_input(src, frozenset()) == [] + + +def test_rules_are_enabled_independently() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + def get(self, request) -> Response[X]: + QuerySerializer(data=request.GET) + return request.GET.get("truncate") +""" + declared_only = _run_input(src, frozenset({"declared"})) + assert len(declared_only) == 1 and "S025" in declared_only[0] + shaped_only = _run_input(src, SHAPED) + assert len(shaped_only) == 1 and "S026" in shaped_only[0] diff --git a/tools/flake8_plugin.py b/tools/flake8_plugin.py index c65c2d663380..5e1e8430d6f5 100644 --- a/tools/flake8_plugin.py +++ b/tools/flake8_plugin.py @@ -168,6 +168,19 @@ "@extend_schema(parameters=...), so the schema does not document what this " "endpoint accepts. Add it to parameters=." ) +S026_msg = ( + "S026 {} is read straight off the {}, so the schema has nothing to document and " + "the value is an unchecked string. Read it through a serializer declared in " + "@extend_schema." +) +S027_msg = ( + "S027 the {} is read with the computed key {}, so no schema can document it. " + "Read a literal key, or declare this endpoint's input as an exception." +) +S028_msg = ( + "S028 the whole {} is handed to {}, so what this endpoint accepts cannot be " + "determined. Read the values here, or declare this endpoint's input as an exception." +) S025_body_msg = ( "S025 {} validates the request body but is not declared in " "@extend_schema(request=...), so the schema does not document what this " @@ -309,6 +322,27 @@ def extend_schema_kwarg(decorators: list[ast.expr], name: str) -> Generator[ast. _QUERY_ATTRS = frozenset(("GET", "query_params")) +_READ_METHODS = frozenset(("get", "getlist", "pop")) +# Counting or iterating the dict does not read a parameter out of it, so these +# are not hand-offs. Anything else receiving the whole dict might read anything. +_CONTAINER_OPS = frozenset( + ( + "len", + "list", + "set", + "tuple", + "sorted", + "dict", + "bool", + "any", + "all", + "iter", + "append", + "extend", + "update", + "dumps", + ) +) _COPY_METHODS = frozenset(("copy", "dict")) @@ -582,6 +616,12 @@ def __init__(self, declared_params: set[str], declared_body: set[str]) -> None: # (line, col, serializer) for each serializer built from that source self.query_validators: list[tuple[int, int, str]] = [] self.body_validators: list[tuple[int, int, str]] = [] + # (line, col, key, source) reads with a literal key + self.literal_reads: list[tuple[int, int, str, str]] = [] + # (line, col, rendered key, source) reads whose key is computed + self.computed_reads: list[tuple[int, int, str, str]] = [] + # (line, col, callee, source) the whole dict passed somewhere + self.hand_offs: list[tuple[int, int, str, str]] = [] def _is(self, node: ast.expr, attrs: frozenset[str], locals_: set[str]) -> bool: node = _unwrap_copy(node) @@ -595,6 +635,20 @@ def is_query(self, node: ast.expr) -> bool: def is_body(self, node: ast.expr) -> bool: return self._is(node, frozenset(("data",)), self.body_locals) + def source_of(self, node: ast.expr) -> str | None: + """ "query string" / "request body" for an input source, else None.""" + if self.is_query(node): + return "query string" + if self.is_body(node): + return "request body" + return None + + def record_read(self, key: ast.expr, source: str, line: int, col: int) -> None: + if isinstance(key, ast.Constant) and isinstance(key.value, str): + self.literal_reads.append((line, col, key.value, source)) + else: + self.computed_reads.append((line, col, ast.unparse(key), source)) + class SentryVisitor(ast.NodeVisitor): def __init__( @@ -923,6 +977,14 @@ def _s024_visit_call(self, node: ast.Call) -> None: if self._s024_parses is None: self._s024_parses = (node.lineno, node.col_offset) + def visit_Subscript(self, node: ast.Subscript) -> None: + if self._input_stack: + ctx = self._input_stack[-1] + source = ctx.source_of(node.value) + if source is not None: + ctx.record_read(node.slice, source, node.lineno, node.col_offset) + self.generic_visit(node) + def _enter_input(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> bool: """Push an accumulator for a PUBLIC HTTP method on an endpoint class.""" if len(self._class_stack) != 1 or self._function_depth != 0: @@ -940,6 +1002,25 @@ def _enter_input(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> bool: ) return True + def _record_input_call(self, node: ast.Call) -> None: + """A `.get()` read, or the whole dict handed to something else.""" + ctx = self._input_stack[-1] + func = node.func + if isinstance(func, ast.Attribute) and func.attr in _READ_METHODS: + source = ctx.source_of(func.value) + if source is not None and node.args: + ctx.record_read(node.args[0], source, node.lineno, node.col_offset) + return + if any(keyword.arg == "data" for keyword in node.keywords): + return + for argument in [*node.args, *(keyword.value for keyword in node.keywords)]: + source = ctx.source_of(argument) + if source is not None: + name = _name_of(func).rsplit(".", 1)[-1] + if name not in _CONTAINER_OPS: + ctx.hand_offs.append((node.lineno, node.col_offset, name, source)) + return + def _record_validator(self, node: ast.Call) -> None: """A serializer built from the query string or the request body.""" ctx = self._input_stack[-1] @@ -969,11 +1050,18 @@ def _exit_input(self) -> None: for line, col, name in ctx.body_validators: if name not in ctx.declared_body: self._report_input(line, col, S025_body_msg.format(name), "declared") + for line, col, key, source in ctx.literal_reads: + self._report_input(line, col, S026_msg.format(repr(key), source), "shaped") + for line, col, key, source in ctx.computed_reads: + self._report_input(line, col, S027_msg.format(source, key), "shaped") + for line, col, callee, source in ctx.hand_offs: + self._report_input(line, col, S028_msg.format(source, callee), "shaped") def visit_Call(self, node: ast.Call) -> None: self._s024_visit_call(node) if self._input_stack: self._record_validator(node) + self._record_input_call(node) if _is_tests_path(self.filename): if ( isinstance(node.func, ast.Name) From 4dc8b45663f62c451d9683b17bf096e37897c57e Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Wed, 9 Sep 2026 11:52:07 -0700 Subject: [PATCH 2/4] fix(apidocs): Narrow input reads to the request object Both review bots caught the same false positive. `_is` matched any attribute named `data`, so `serializer.data["title"]` and `response.data["title"]` were reported as raw request-body reads. Those are outputs a handler builds, not parameters a client sent, and reporting them inflates the backlog and would fail CI once the rule is enforced. The attribute now has to hang off `request` or `self.request`. `visit_Subscript` also recorded every subscript regardless of context, so `request.data["title"] = default` counted as a read of a parameter that no client supplies. It now records loads only. Together these drop the reported backlog from 208 raw reads to 197 and from 21 hand-offs to 12. Co-Authored-By: Claude Opus 5 (1M context) --- tests/tools/test_flake8_plugin.py | 39 +++++++++++++++++++++++++++++++ tools/flake8_plugin.py | 15 ++++++++++-- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_flake8_plugin.py b/tests/tools/test_flake8_plugin.py index 1fa688ba79a0..03ae7223091a 100644 --- a/tests/tools/test_flake8_plugin.py +++ b/tests/tools/test_flake8_plugin.py @@ -1534,3 +1534,42 @@ def get(self, request) -> Response[X]: assert len(declared_only) == 1 and "S025" in declared_only[0] shaped_only = _run_input(src, SHAPED) assert len(shaped_only) == 1 and "S026" in shaped_only[0] + + +def test_S026_serializer_and_response_data_are_not_request_body() -> None: + src = """\ +class E(Endpoint): + publish_status = {"POST": ApiPublishStatus.PUBLIC} + + def post(self, request) -> Response[X]: + serializer.data["title"] + response.data["title"] + return serializer.data.get("slug") +""" + assert _run_input(src, SHAPED) == [] + + +def test_S026_request_via_self_is_still_a_read() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + def get(self, request) -> Response[X]: + return self.request.GET.get("truncate") +""" + assert len(_run_input(src, SHAPED)) == 1 + + +def test_S026_subscript_write_is_not_a_read() -> None: + src = """\ +class E(Endpoint): + publish_status = {"POST": ApiPublishStatus.PUBLIC} + + def post(self, request) -> Response[X]: + request.data["title"] = "default" + del request.data["scratch"] + return request.data["title"] +""" + errors = _run_input(src, SHAPED) + assert len(errors) == 1 + assert "t.py:7:" in errors[0] diff --git a/tools/flake8_plugin.py b/tools/flake8_plugin.py index 5e1e8430d6f5..ef314a86e23e 100644 --- a/tools/flake8_plugin.py +++ b/tools/flake8_plugin.py @@ -346,6 +346,13 @@ def extend_schema_kwarg(decorators: list[ast.expr], name: str) -> Generator[ast. _COPY_METHODS = frozenset(("copy", "dict")) +def _is_request(node: ast.expr) -> bool: + """The handler's request argument, as `request` or `self.request`.""" + if isinstance(node, ast.Name): + return node.id == "request" + return isinstance(node, ast.Attribute) and node.attr == "request" + + def _unwrap_copy(node: ast.expr) -> ast.expr: """Strip `.copy()` / `.dict()` so `request.GET.copy()` still reads as the source.""" while ( @@ -627,7 +634,9 @@ def _is(self, node: ast.expr, attrs: frozenset[str], locals_: set[str]) -> bool: node = _unwrap_copy(node) if isinstance(node, ast.Name): return node.id in locals_ - return isinstance(node, ast.Attribute) and node.attr in attrs + # The attribute has to hang off the request. `serializer.data` and + # `response.data` are outputs, not parameters a client sent. + return isinstance(node, ast.Attribute) and node.attr in attrs and _is_request(node.value) def is_query(self, node: ast.expr) -> bool: return self._is(node, _QUERY_ATTRS, self.query_locals) @@ -978,7 +987,9 @@ def _s024_visit_call(self, node: ast.Call) -> None: self._s024_parses = (node.lineno, node.col_offset) def visit_Subscript(self, node: ast.Subscript) -> None: - if self._input_stack: + # Load only: `request.data["title"] = ...` writes a value, it does not + # read a parameter the client sent. + if self._input_stack and isinstance(node.ctx, ast.Load): ctx = self._input_stack[-1] source = ctx.source_of(node.value) if source is not None: From 2a882465c9154ba56a9235dd50961149853133bb Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Wed, 9 Sep 2026 11:58:53 -0700 Subject: [PATCH 3/4] fix(apidocs): Only treat a serializer's data= as the target shape The guard that keeps a serializer construction from counting as a hand-off matched any call with a data= keyword, so `my_func(data=request.GET)` was silently dropped while the same call written positionally reported S028. The keyword name is not what makes a call safe; the callee being a serializer is. Both paths now share one predicate for what looks like a class, so they cannot disagree again. S025 already used that test to skip plain calls and runtime-chosen classes, and the hand-off guard skipped nothing at all. Surfaces 5 hand-offs that were dropped by both rules at once: three `serializer_cls(data=...)` sites where the class is chosen at runtime, and the integration issue-config calls that take the whole body. Those are exactly the unanalyzable cases S028 exists to report. Co-Authored-By: Claude Opus 5 (1M context) --- tests/tools/test_flake8_plugin.py | 26 ++++++++++++++++++++++++++ tools/flake8_plugin.py | 13 ++++++++++--- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/tests/tools/test_flake8_plugin.py b/tests/tools/test_flake8_plugin.py index 03ae7223091a..9ab69532bcba 100644 --- a/tests/tools/test_flake8_plugin.py +++ b/tests/tools/test_flake8_plugin.py @@ -1573,3 +1573,29 @@ def post(self, request) -> Response[X]: errors = _run_input(src, SHAPED) assert len(errors) == 1 assert "t.py:7:" in errors[0] + + +def test_S028_plain_function_taking_data_is_still_a_hand_off() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + def get(self, request) -> Response[X]: + return my_func(data=request.GET) +""" + errors = _run_input(src, SHAPED) + assert len(errors) == 1 + assert "handed to my_func" in errors[0] + + +def test_S028_method_taking_data_is_still_a_hand_off() -> None: + src = """\ +class E(Endpoint): + publish_status = {"POST": ApiPublishStatus.PUBLIC} + + def post(self, request) -> Response[X]: + return installation.build(data=request.data) +""" + errors = _run_input(src, SHAPED) + assert len(errors) == 1 + assert "handed to build" in errors[0] diff --git a/tools/flake8_plugin.py b/tools/flake8_plugin.py index ef314a86e23e..9ffc9abc91aa 100644 --- a/tools/flake8_plugin.py +++ b/tools/flake8_plugin.py @@ -346,6 +346,11 @@ def extend_schema_kwarg(decorators: list[ast.expr], name: str) -> Generator[ast. _COPY_METHODS = frozenset(("copy", "dict")) +def _looks_like_a_class(func: ast.expr) -> bool: + """Callee named like a class, which is how a serializer is spelled.""" + return _name_of(func).rsplit(".", 1)[-1][:1].isupper() + + def _is_request(node: ast.expr) -> bool: """The handler's request argument, as `request` or `self.request`.""" if isinstance(node, ast.Name): @@ -1022,7 +1027,9 @@ def _record_input_call(self, node: ast.Call) -> None: if source is not None and node.args: ctx.record_read(node.args[0], source, node.lineno, node.col_offset) return - if any(keyword.arg == "data" for keyword in node.keywords): + # Only a serializer's data= is the target shape. `my_func(data=...)` + # hands the dict over exactly as a positional argument would. + if _looks_like_a_class(func) and any(kw.arg == "data" for kw in node.keywords): return for argument in [*node.args, *(keyword.value for keyword in node.keywords)]: source = ctx.source_of(argument) @@ -1038,11 +1045,11 @@ def _record_validator(self, node: ast.Call) -> None: for keyword in node.keywords: if keyword.arg != "data": continue - name = _name_of(node.func).rsplit(".", 1)[-1] # A class, by convention. Skips plain calls taking data=, and # runtime-chosen classes the schema could not name either. - if not name[:1].isupper(): + if not _looks_like_a_class(node.func): continue + name = _name_of(node.func).rsplit(".", 1)[-1] if ctx.is_query(keyword.value): ctx.query_validators.append((node.lineno, node.col_offset, name)) elif ctx.is_body(keyword.value): From 0b5bfa48eb027976ac0b25ddc0081b7631983b33 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Wed, 9 Sep 2026 12:16:04 -0700 Subject: [PATCH 4/4] fix(apidocs): Stop a lookup default counting as a hand-off A read accessor on something that is not the request fell through to hand-off detection, so `options.get("key", request.GET)` reported that the query string was handed to `get`. The dict is a default value there, not something a callee reads parameters out of, and naming `get` as the callee made the diagnostic read as nonsense. A read method now returns once handled: on the request it is the read itself, and on anything else its arguments are ordinary lookup arguments. No occurrences on the current tree, so the counts are unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- tests/tools/test_flake8_plugin.py | 11 +++++++++++ tools/flake8_plugin.py | 4 +++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_flake8_plugin.py b/tests/tools/test_flake8_plugin.py index 9ab69532bcba..e65069c56d42 100644 --- a/tests/tools/test_flake8_plugin.py +++ b/tests/tools/test_flake8_plugin.py @@ -1599,3 +1599,14 @@ def post(self, request) -> Response[X]: errors = _run_input(src, SHAPED) assert len(errors) == 1 assert "handed to build" in errors[0] + + +def test_S028_request_data_as_a_lookup_default_is_not_a_hand_off() -> None: + src = """\ +class E(Endpoint): + publish_status = {"GET": ApiPublishStatus.PUBLIC} + + def get(self, request) -> Response[X]: + return options.get("key", request.GET) +""" + assert _run_input(src, SHAPED) == [] diff --git a/tools/flake8_plugin.py b/tools/flake8_plugin.py index 9ffc9abc91aa..db108521a1ff 100644 --- a/tools/flake8_plugin.py +++ b/tools/flake8_plugin.py @@ -1026,7 +1026,9 @@ def _record_input_call(self, node: ast.Call) -> None: source = ctx.source_of(func.value) if source is not None and node.args: ctx.record_read(node.args[0], source, node.lineno, node.col_offset) - return + # On anything but the request this is an ordinary lookup, and + # `options.get("k", request.GET)` passes a default, not the dict. + return # Only a serializer's data= is the target shape. `my_func(data=...)` # hands the dict over exactly as a positional argument would. if _looks_like_a_class(func) and any(kw.arg == "data" for kw in node.keywords):