Fix MLAllowlist shadowing (GHSA-cffv-grgg-g429)#278
Open
thomas-chauchefoin-tob wants to merge 2 commits into
Open
Fix MLAllowlist shadowing (GHSA-cffv-grgg-g429)#278thomas-chauchefoin-tob wants to merge 2 commits into
thomas-chauchefoin-tob wants to merge 2 commits into
Conversation
…429) AnalysisContext.shorten_code() mutated a shared reported_shortened_code set as a side effect of formatting, even though most callers only wanted the formatted string. It could then shadow findings from future analysis runs. For instance, UnsafeImportsML processed every import and would leave MLAllowlist with an already-reported state for all of them. Any import outside ML_ALLOWLIST would not be flagged by the second run. Split shorten_code() into a pure formatter and an explicit mark_reported() helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Analysis.__init_subclass__ put every subclass into Analysis.ALL, and fickling/__init__.py transitively imported fickling.ml via hook.py, wich registered MLAllowlist. The shorten_code() shadowing bug (GHSA-cffv-grgg-g429) made it dead code until the previous commit, and now it conflicts with other default analysis passes. This commit makes MLAllowlist opt-in; use `check_safety(analyze=...)` if you need it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
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
1 participant
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.
@reapermunky reported in GHSA-cffv-grgg-g429 that
AnalysisContext.shorten_code()would mutate the shared dedup set of the context analysis, which then prevents future analysis passes from yielding anAnalysisResult. SinceUnsafeImportsMLalready marks every import withAnalysisContext.shorten_code(),MLAllowlistwould never analyze them. The first commit moves dedup tracking fromAnalysisContext.shorten_code()to a separate functionAnalysisContext.mark_reported().Now that
MLAllowlistis no longer dead code, it conflicts with other analysis passes (for instance, combining results fromMLAllowlistandNonStandardImportswould always flag pickles that import at least one module). I assume this was the initial goal since it was put in a distinct file, but 3faad11 made it a transitive import and the self-registration ofAnalysissub-classes enabled it. The second commit makesMLAllowlistopt-in; library users can still invoke it with: