Repository navigation
fix(agent-learning): normalise privilege values before the excessive-privilege check - #4411
Open
MohammadHaroonAbuomar wants to merge 4 commits into
Open
MohammadHaroonAbuomar wants to merge 4 commits into
MohammadHaroonAbuomar wants to merge 4 commits into
Conversation
ExcessivePrivilegeEvaluation compared the privilege, role, scopes and permissions values as given, so an action could slip past the check with whitespace around a value, a list-valued privilege, a nested list in scopes or permissions, a partial wildcard such as admin:*, or a capitalised key such as Privilege or ROLE. The four keys are now looked up case-insensitively. Values are stripped of surrounding whitespace, including Unicode whitespace, and case-folded before the comparison. Nested lists, tuples and sets are flattened to their leaves with an iterative walk that tolerates self-referencing collections, and max_scopes counts the flattened leaves. Any string that carries a "*" is treated as a wildcard and so as elevated. A leaf that is not a string (None inside a list, bytes, a mapping, a number) fails closed and is reported with its own message, as the cost check does for a non-numeric cost. The public API and the result shape are unchanged. Add regression tests for every form in the issue and for mixed-case keys and values, Unicode whitespace, deeply nested and empty lists, unreadable leaves, and low privileges that must still pass. Fixes #4400 Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
MohammadHaroonAbuomar
requested review from
liamcrumm and
Prayag (prayagupa)
as code owners
October 22, 2026 06:22
github-actions
Bot
added
documentation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
Contributor
|
Looks good overall, thanks for covering the nested privilege values. Could you please review these potential edge cases in
Could we track only active traversal ancestors instead, keeping cycle protection without skipping noncyclic aliases, and add regressions for both cases? |
…alues _flatten_leaves kept a set of every container id it had seen for the whole walk. Two values slipped past it: a container that is freed before a later one is created can hand the later one the same id, so a fresh list carrying "admin" was treated as already visited; and the same list object reached by two paths was read once. The walker now keeps an explicit stack of (container, iterator) frames and tracks only the ids of containers on the current path, adding on enter and removing on exit. Each frame holds its container, so an id on the path cannot be reused mid-walk. A self-referencing collection still terminates; a container reached twice by different paths is read each time. Tests cover a list yielded twice from a generator with the elevated value added between the yields, a chain of generator-built lists where the last one carries "admin", and an elevated value inside a cycle. Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
…wice The same six-entry list under both keys must count as twelve scopes and exceed max_scopes=10. The earlier walker read a shared object once and let it pass. Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
Collaborator
Author
|
Thanks Ricky Gummadi (@Ricky-G), both cases were real.
One note: with per-reference reading, a heavily aliased acyclic structure costs time proportional to the number of paths. I left any leaf cap for a follow-up rather than fold it in here. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Fixes #4400
Problem & Solution
Problem:
ExcessivePrivilegeEvaluationin agent-learning compared theprivilege,role,scopesandpermissionsvalues exactly as given. Five gaps let an elevated action pass:" admin ") or a different case ("Admin","ADMIN").privilegeorrole(["user", "admin"]), which was never compared element by element.scopesorpermissions([["*"]]), which_as_sequenceleft as one opaque element.admin:*,*:writeorrepo.*; only the exact value*counted.Privilege,ROLE,"privilege "), which the lookup did not match.Solution: normalise keys and values before the comparison. Behaviour changes:
privilege,role,scopes,permissionsare matched case-insensitively and with surrounding whitespace stripped.privilegeorroleis checked element by element.*is elevated (previously only the exact value*). A legitimate scope such asadmin-reports:readstill passes.scope_countand themax_scopescomparison now count flattened leaves rather than top-level elements.Noneinside a list,bytes, a dict, a number) now produces a HIGH finding with the message "Action privilege, role, scope and permission values must be strings". Before, such values were silently ignored. A top-levelNone({"privilege": None}) still means absent and passes, as in fix(agent-learning): check role and permissions in ExcessivePrivilegeEvaluation #4364.scope_countin that edge case but never hides a leaf. The walk is iterative, so deep nesting and self-referencing lists cannot raiseRecursionError._as_sequenceis removed; it had no other callers.The result shape and public API are unchanged:
GovernanceEvaluationResult(check_id, passed, findings, metrics={"actions_checked"}), finding fieldscheck_id, message, severity, artifact_type, artifact_id, action_id, metadata={"scope_count"}.__all__inevaluation.pyand__init__.pyis untouched.Impact on Your Work
The excessive-privilege check is a security gate. Any caller that trusted it to catch
adminor*in an action could be bypassed by trivial changes to case, whitespace, nesting or key spelling. After this change the gate reads the same values a human reader would, and it refuses values it cannot read instead of ignoring them.Known gaps left out of scope, for a follow-up issue rather than this PR: Unicode confusables (
adminwith a zero-width space, fullwidthａｄｍｉｎ, TurkishADMİN) still pass; NFKC normalisation plus strippingCfcharacters would close them. A glob-style scope such asfiles:*.mdis now flagged because it contains*; this is what the issue asked for but is broader than a*suffix. Top-level non-string scalars ({"privilege": 5}) now fail closed where they used to pass silently; this is deliberate and noted in the CHANGELOG.Timeline
None
Alternatives Considered
Two options for a value the check cannot read (a dict, bytes, a number,
Noneinside a list):Both options fail closed; the difference is only in the message. When one value tree holds both an unreadable leaf and an elevated string, the excessive-privilege message wins, so a bypass attempt cannot downgrade its own finding by adding junk.
Rejected: keeping exact comparison and only adding
strip(). That closes gap 1 alone and leaves nesting, partial wildcards and key casing open.Type of Change
Package(s) Affected
Core & runtime:
Governance & security:
Platform & tooling:
CLI plugins:
Shared / other:
Other:
agent-governance-python/agent-learning(not in the list above).Testing
Unit Testing
New tests in
agent-governance-python/agent-learning/tests/test_evaluation.py:test_excessive_privilege_normalizes_values_and_keys(23 cases): case, whitespace and Unicode whitespace around values; list-valuedprivilegeandrole; nested lists, tuples and sets inscopesandpermissions; partial wildcards (admin:*,*:write,repo.*); mixed-case and whitespace-padded keys.test_excessive_privilege_fails_closed_on_unexpected_leaf_types(5 cases):Noneinside a list,bytes, a dict, a number, and a nested unreadable leaf, each yielding the "must be strings" finding.{"permissions": [None, "*"]}reports excessive privilege, not the unreadable-leaf message.test_excessive_privilege_counts_flattened_scopes:scope_countandmax_scopesuse flattened leaves.admin-reports:read, plus a self-referencing-list guard.Manual Testing
Environment:
.venvwith pytest, pyyaml, pydantic, cryptography;PYTHONPATH=src:../agent-os/src; run fromagent-governance-python/agent-learning.mhabuomar/issue-4400:121 passed, 1 skipped, 1 warning in 0.27s(the skip is the agent-learning 0.8.x integration test).origin/mainunmodified (9c85646):84 passed, 1 skipped, 1 warning in 0.25s.origin/mainsource with this branch'stests/test_evaluation.pycopied in (temporary copy, removed afterwards):29 failed, 92 passed, 1 skipped, 1 warning in 0.56s. The 29 failures are all 23 cases oftest_excessive_privilege_normalizes_values_and_keys, all 5 cases oftest_excessive_privilege_fails_closed_on_unexpected_leaf_types, andtest_excessive_privilege_counts_flattened_scopes. The 8 new tests that pass on main are the must-still-pass guards, the self-referencing-list guard, and the message-precedence test (main already flagged[None, "admin"]through"admin").Lint and repo checks:
All checks passed. (tests/has pre-existing E712 findings on untouched lines that CI does not lint; the new tests follow the file's existing== Falsestyle.)0 issues.OK (grand total 4).Bypass probes, all run against the branch. Each of these now yields a HIGH finding with
action_idset:"Admin","ADMIN"," admin\t"," admin","admin:*","ADMIN:*","*:write","a.*",["user", ["admin"]],{"scopes": [[["*"]]]},{"Privilege": "admin"},{"PRIVILEGE": "admin"},{"privilege ": "admin"}(excessive-privilege message);{"privilege": {"level": "admin"}},{"privilege": b"admin"}(must-be-strings message);{"permissions": [None, "*"]}(excessive-privilege message wins).{"privilege": None}still passes as absent.Scale and robustness: a 10,000-element
scopeslist is checked in 3 ms and flagged throughmax_scopes; the same list with"Admin "at position 10,000 andmax_scopes=100000is flagged. A 200,000-deep nested["*"]is flagged in 198 ms with noRecursionError; a 200,000-deep["read"]passes. A self-referencing list containing"Admin"is flagged. A 1 MB privilege string passes in 1.6 ms; 1 MB of spaces aroundadminis flagged. A generator-valuedscopesis consumed and checked. Non-string keys are skipped. Peak RSS 78 MB.Legitimate inputs still pass:
{"privilege": "user", "role": "viewer", "scopes": ["read", "write"]},["repo.read", "admin-reports:read"],{},{"scopes": []}, and{"query": "*", "Privileges": "admin", "roles": ["admin"]}(unrelated keys are not read).git merge-tree --write-tree origin/main HEADis clean. Commit subject 65 characters, body wrapped to 72 columns, oneSigned-off-byline.Checklist
Attribution & Prior Art
Prior art / related projects (if any):
None.
AI Assistance
If AI tools materially shaped this change, briefly note what was used:
Claude Code drafted the fix and tests under my direction; an independent verification pass re-ran the tests on main and on the branch and probed bypasses, and I reviewed the result.
IP, Patents, and Licensing