feat: deny every ast node explicitly with a documented reason (PEP 20)

[loechel]

Summary

Explicit is better than implicit. — PEP 20

This PR makes a rule out of it: every ast node has an explicit visit_<node> method in RestrictingNodeTransformer, the denied ones included, and the docstring of a denying method explains why the node is denied and which guard or check it would bypass.

generic_visit stays what it is meant to be: the safety net for nodes of a new Python version which nobody has reviewed yet. It is not the place where the decision about a known language feature is recorded. A node denied only by generic_visit looks exactly like a node nobody has ever looked at; an explicit method records the review and its result at the place where the next reviewer looks for it. In a security package that knowledge is worth more than the lines it takes.

This settles the open todo "Option 1 / Option 2" in docs/contributing/index.rst in favour of being explicit; the section is replaced by the rule (new section Every AST node has an explicit visit_<AST Node> method).

Changes

• New explicit denials for all nodes which were only denied implicitly (none of them had a test before):

  • AnnAssign
  • Match, match_case, MatchValue, MatchSingleton, MatchSequence, MatchMapping, MatchClass, MatchStar, MatchAs, MatchOr
  • TypeAlias, TypeVar, ParamSpec, TypeVarTuple (Python 3.12+)
  • FunctionType, TypeIgnore (only reachable via a pre-parsed ast)

• Documented reasons in the docstrings of these and of the existing denying methods (Nonlocal, TryStar, AsyncFunctionDef, Await, AsyncFor, AsyncWith). Where no concrete bypass is known (Nonlocal), the docstring says so and that a security review is missing.

• The rule is enforced by a test: test_explicit_deny__1 fails for every ast node of the running Python version without a visit_<node> method. Adding a new Python version to the matrix therefore fails until each new node has been decided on.

• The reasons are backed by tests where they describe interpreter behaviour. With the pattern nodes allowed, the tests show:

  • class patterns read attributes (keywords and __match_args__) without _getattr_,
  • mapping patterns read items without _getitem_,
  • sequence patterns iterate without _getiter_,
  • capture names (case _secret, *_secret, **_secret, as _secret) escape the check for a leading underscore — the same class of bug as GHSA-ffg3-p8fm-mjx2,
  • a guarded attribute lookup is not accepted by compile() in a value pattern.

  If a future Python changes that behaviour, the test fails and the docstring gets reviewed.

Behaviour change

None for the security boundary: every affected node was denied before and is denied now, with the same error message "<Node> statements are not allowed.". The only visible difference is that the warning "<Node> statement is not known to RestrictedPython" from generic_visit is no longer emitted for these nodes.

Tests

Run locally:

• tox -e py310,py311,py312,py313,py314,py315,py311-datetime — all green
• tox -e lint (incl. flake8, isort, mypy, sphinx-lint) — green
• tox -e docs — build succeeded (the 4 remaining warnings are pre-existing todo entries)
• tox -e coverage — 100 %

Risks

• Maintenance: each new Python version needs a decision per new ast node. That is intended; the new test makes it impossible to forget.
• Applications which evaluate the warnings of compile_restricted_* see the "not known" warning disappear for the nodes listed above.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[icemac]

Sorry, but I do not like this PR at all. (You'd probably expected this.)

We already have an explicit deny of unknown AST nodes in

RestrictedPython/src/RestrictedPython/transformer.py

Lines 500 to 514 in c5066f7

	def generic_visit(self, # type: ignore[override]
	node: ast.AST) -> _T_visit_return:
	"""Reject ast nodes which do not have a corresponding `visit_` method.
	This is needed to prevent new ast nodes from new Python versions to be
	trusted before any security review.
	To access `generic_visit` on the super class use `node_contents_visit`.
	"""
	self.warn(
	node,
	'{0.__class__.__name__}'
	' statement is not known to RestrictedPython'.format(node)
	)
	self.not_allowed(node)

– this is enough, for all new upcoming AST nodes, they are already denied. (It would be way more helpful to invest into changing AST node structures (added attributes, changed constructor signature) – this is where energy and time can flow into because there we have way lesser defenses.

The idea behind this PR makes maintaining this package even harder and adds nothing (I see no security gain at all!). I have been fighting against it for years and it seems that my message has not been come across clearly enough: This PR leads into a direction we should not go.

There is also Although practicality beats purity. in the Zen of Python and this PR feels like violating this principle.