Harden DEPS parsing and sanitize revision metadata handling - #5438
Merged
Conversation
- Replace unrestricted exec() in deps_to_revisions_dict() with a safe AST evaluator (_SafeDepsEvaluator). - Validate revision string identifier against REVISION_PATTERN in get_component_revisions_dict() to reject URL manipulation / query delimiters. - Validate and sanitize numeric metadata fields (e.g. last_tested_crash_revision) in _update_issue_metadata(). - Add comprehensive unit tests covering safe AST parsing, malicious DEPS payloads, and revision format validation.
- Extract is_valid_revision() helper for clean identifier validation. - Split _SafeDepsEvaluator node processing into dedicated helpers (_eval_literal, _eval_binop, _eval_call, _eval_dict, _eval_sequence, _process_assignment). - Extract _sanitize_metadata_value() helper in progression_task.py. - Add test_is_valid_revision unit tests.
- Define explicit MetadataValue = Union[str, int, float, bool, None] in progression_task.py. - Add concrete type annotations for AST evaluator helper methods and revision validation in revisions.py.
- Move the numeric-metadata allowlist into data_handler.sanitize_issue_metadata() and apply it in corpus_pruning_task as well. It had the same unvalidated write of uworker issue_metadata into Datastore, so the poisoning primitive survived the progression_task fix. - Escape the revisions in show._get_revision_range_html()'s "No component revisions found!" fallback. That string is bound with inner-h-t-m-l, and a rejected revision now always routes to it. - Select the response parser in get_component_revisions_dict() from the REVISION_VARS_URL format string rather than the interpolated url. - Drop the dead ast.Str/ast.Num branches (Python 3.8+ only emits ast.Constant) and extract type aliases so annotations fit in 80 columns.
- Replace typing.Optional and typing.Union with | None and | syntax. - Replace typing.Dict and typing.Tuple with builtin dict and tuple generics.
Collaborator
Author
Verification
Compatibility & Risk Assessment
|
ViniciustCosta
approved these changes
Aug 31, 2026
ViniciustCosta
left a comment
ViniciustCosta
left a comment
Collaborator
There was a problem hiding this comment.
The sanitize DEPS logic is difficult for me to review, as I don't have context about this. Seems good to me given your tests on dev :)
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
2 participants
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.
b/534004999
TAG=agy
CONV=6fa9c8c8-6088-4c02-b27d-ab873dda019f