Repository navigation
Add privilege-change review records and impact/confidence fields - #262
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d44e385ce3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _is_directory_role_assignment(call: ast.Call) -> bool: | ||
| """Recognize a literal Graph directory role assignment request, not its outcome.""" |
There was a problem hiding this comment.
This new security-sensitive parser recognizes an additional privilege-change boundary in untrusted repository source and emits it as review evidence, but docs/THREAT_MODEL.md remains at its earlier baseline and does not assess this rule's false-positive/false-negative, tampering, or evidence-overclaim risks. Update the threat model with the new parser/control and its limitations before release; the repository contract explicitly requires the model to be maintained when code or trust-boundary behavior changes.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
| ) | ||
| self.generic_visit(call) | ||
|
|
||
| DirectoryRoleVisitor().visit(tree) |
There was a problem hiding this comment.
For a changed Python file containing a sufficiently deep but valid expression, this recursive NodeVisitor raises an uncaught RecursionError and aborts the entire review. For example, a flat expression with only 500 chained additions is well below MAX_BLOB, parsed successfully, and was handled by the previous iterative traversal, but now fails here even when it contains no role-assignment call. Use a bounded iterative traversal or convert recursion exhaustion into a controlled ReviewError so an untrusted patch cannot deny review service.
AGENTS.md reference: AGENTS.md:L23-L24
Useful? React with 👍 / 👎.
|
|
||
| def visit_Call(self, call: ast.Call) -> None: | ||
| if _is_directory_role_assignment(call): | ||
| affected = sorted(line for line in changed if call.lineno <= line <= (call.end_lineno or call.lineno)) |
There was a problem hiding this comment.
When a patch only deletes a line inside an otherwise intact role-assignment expression, changed contains no line in the resulting call, so this rule emits no candidate. This can hide a privilege-relevant change—for example, deleting a final **approved_fields override from a literal payload leaves attacker-controlled literal identifiers active but produces an empty candidate list. Account for deleted hunk positions or compare the base and head calls rather than requiring an added head line.
Useful? React with 👍 / 👎.
1 participant
Closes #196.
Change
requests.postorhttpx.postcall to the literal Microsoft Graph directory role assignment endpoint with the required JSON identifiers. It detects positional and keyword URL arguments, module-level calls, and calls affected by a changed payload line. The report records a reproducible file and changed line, enclosing entrypoint, candidate boundary, conditional impact, and limited confidence.Validation
python3 plugins/vulnerability-management/patch-security-review/scripts/validate_package.py— 12 tests passed..venv/bin/python -m pytest -q tests/step_defs/test_patch_security_review.py— 5 tests passed.PATH=<project-venv>/bin:$PATH make check PYTHON=<project-venv>/bin/python— passed in the issue worktree with the project virtual environment.git diff --check— passed.Security and limits
The fixture is read as source and never imported or executed. The new rule makes no network request. It recognizes a bounded source pattern and does not establish runtime reachability, authorization, effective role grant, or exploitability. Computed URLs and payloads and other Graph clients remain uncovered. Analyst assertions remain unverified self-recorded evidence.
The first
make checkattempt used the system Python 3.9 for capability probes and could not bind a Unix test socket in the sandbox. Rerunning with the project virtual environment first onPATHand the required socket permission passed the full gate.