mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
fix(mapping): record every callback of a register_rest_route call as a route
The mapper took the FIRST 'callback' in a 12-line window. One registration
commonly carries an array of endpoints; PMPro's /discount_code has a GET
and a POST handler, so the writer, pmpro_rest_api_set_discount_code, was an
ordinary `function` entry. Since da0ee57 such an entry's parameters are
not attacker input, so its $request stopped being a source and the REST
route that injects SQL through the discount-code save() had no source at
all.
The argument list of the call is now read to its balancing parenthesis,
every callback in it is a route, and each keeps the permission callback of
its own endpoint (the first after it before the next callback, else the
last before it). PMPro: 15 -> 18 REST routes.
This restores the source, not the finding: traced unfiltered, the engine
does not carry $discount_code->code = $code (written in the handler) into
$this->code inside save() once other methods have been called on the
object; the one path it reports detours through a prepared query's result.
Recorded as the next recall gap: a cross-file field join keyed by class.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
ece4473c2b
commit
53d20691c3
@@ -288,6 +288,36 @@ _PHP_FUNCTION = re.compile(
|
||||
)
|
||||
|
||||
|
||||
def _call_arguments(lines: list[str], number: int, opener: re.Pattern[str], *, limit: int = 120) -> str:
|
||||
"""The text of one call's argument list, from the line that opens it to its balancing parenthesis."""
|
||||
text = "\n".join(lines[number - 1 : number - 1 + limit])
|
||||
start = opener.search(text)
|
||||
if start is None:
|
||||
return text
|
||||
depth = 0
|
||||
for position in range(start.end() - 1, len(text)):
|
||||
character = text[position]
|
||||
if character == "(":
|
||||
depth += 1
|
||||
elif character == ")":
|
||||
depth -= 1
|
||||
if depth == 0:
|
||||
return text[start.start() : position + 1]
|
||||
return text
|
||||
|
||||
|
||||
def _own_permission(
|
||||
callback: re.Match[str], previous: re.Match[str] | None, following: re.Match[str] | None, permissions: list[re.Match[str]]
|
||||
) -> re.Match[str] | None:
|
||||
"""The permission callback of the same endpoint: the first after this callback and before the next one,
|
||||
else the last after the previous callback and before this one -- endpoints write them in either order."""
|
||||
after = [p for p in permissions if p.start() > callback.end() and (following is None or p.start() < following.start())]
|
||||
if after:
|
||||
return after[0]
|
||||
before = [p for p in permissions if p.start() < callback.start() and (previous is None or p.start() > previous.end())]
|
||||
return before[-1] if before else None
|
||||
|
||||
|
||||
def _php_entry_points(target: FileTarget, text: str) -> list[EntryPointRecord]:
|
||||
"""WordPress's registration model, plus every top-level function as a region of its own.
|
||||
|
||||
@@ -323,13 +353,23 @@ def _php_entry_points(target: FileTarget, text: str) -> list[EntryPointRecord]:
|
||||
)
|
||||
)
|
||||
if _PHP_REST_ROUTE.search(line):
|
||||
window = "\n".join(lines[number - 1 : number + 12])
|
||||
callback = _PHP_CALLBACK.search(window)
|
||||
if callback is not None:
|
||||
# EVERY callback of the registration, not the first. One `register_rest_route` commonly carries
|
||||
# an array of endpoints -- PMPro's `/discount_code` has a GET and a POST handler -- and taking the
|
||||
# first recorded the reader as a route and left the writer an ordinary function, whose `$request`
|
||||
# then was no source: the REST path that injects SQL through the discount-code `save()` vanished.
|
||||
window = _call_arguments(lines, number, _PHP_REST_ROUTE)
|
||||
callbacks = list(_PHP_CALLBACK.finditer(window))
|
||||
permissions = list(_PHP_PERMISSION.finditer(window))
|
||||
for index, callback in enumerate(callbacks):
|
||||
handler = callback.group("handler") or callback.group("method")
|
||||
if not handler:
|
||||
continue
|
||||
permission = _PHP_PERMISSION.search(window)
|
||||
permission = _own_permission(
|
||||
callback,
|
||||
callbacks[index - 1] if index else None,
|
||||
callbacks[index + 1] if index + 1 < len(callbacks) else None,
|
||||
permissions,
|
||||
)
|
||||
access, evidence = _rest_route_access(permission.group("value").strip() if permission else None)
|
||||
start, end = bounds.get(handler, (number, number))
|
||||
records.append(
|
||||
|
||||
@@ -73,3 +73,26 @@ def test_a_hook_callback_gets_the_registrys_arguments_not_the_requests(tmp_path)
|
||||
# A route that did not come from a registry keeps its parameters as input.
|
||||
route = dataclasses.replace(regions["show_history"], entry_registry="", entry_kind="route")
|
||||
assert scan._parameters_are_input(route)
|
||||
|
||||
|
||||
def test_every_callback_of_one_rest_registration_is_a_route(tmp_path) -> None:
|
||||
"""PMPro registers a GET and a POST handler in one `register_rest_route`; taking the first callback left
|
||||
the writer an ordinary function, so its `$request` was no source."""
|
||||
from openultrasast.mapping import analyze_entry_points
|
||||
from openultrasast.preprocess import build_file_target, enumerate_source_files
|
||||
|
||||
(tmp_path / "rest.php").write_text(
|
||||
"<?php\nclass Routes {\n function register() {\n"
|
||||
" register_rest_route( $ns, '/code',\n array(\n"
|
||||
" array( 'methods' => 'GET', 'callback' => array( $this, 'read_code' ),\n"
|
||||
" 'permission_callback' => array( $this, 'can_read' ) ),\n"
|
||||
" array( 'methods' => 'POST', 'callback' => array( $this, 'write_code' ), 'permission_callback' => '__return_true' ),\n"
|
||||
" ));\n }\n"
|
||||
" function read_code($request) { return 1; }\n function write_code($request) { return 2; }\n}\n"
|
||||
)
|
||||
files = enumerate_source_files(tmp_path)
|
||||
entries = analyze_entry_points(tmp_path, [build_file_target(tmp_path, p) for p in files])
|
||||
routes = {e.function_name: e for e in entries if e.name == "wp:rest_route"}
|
||||
assert set(routes) == {"read_code", "write_code"}
|
||||
# Each endpoint keeps its OWN permission callback.
|
||||
assert routes["write_code"].access_level == "public" and routes["read_code"].access_level != "public"
|
||||
|
||||
Reference in New Issue
Block a user