Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-bandit/S506] Dont report violation when using BaseLoader #13604

Open
mcmitch opened this issue Oct 2, 2024 · 1 comment
Open

[flake8-bandit/S506] Dont report violation when using BaseLoader #13604

mcmitch opened this issue Oct 2, 2024 · 1 comment
Labels
rule Implementing or modifying a lint rule

Comments

@mcmitch
Copy link

mcmitch commented Oct 2, 2024

From https://pyyaml.org/wiki/PyYAMLDocumentation

  • Loader supports all predefined tags and may construct an arbitrary Python object. Therefore it is not safe to use Loader to load a document received from an untrusted source. By default, the functions scan, parse, compose, construct, and others use Loader.
  • SafeLoader(stream) supports only standard YAML tags and thus it does not construct class instances and probably safe to use with documents received from an untrusted source. The functions safe_load and safe_load_all use SafeLoader to parse a stream.
  • BaseLoader(stream) does not resolve or support any tags and construct only basic Python objects: lists, dictionaries and Unicode strings.

For our project we are using the Baseloader, and do not want to use safeLoader, as this would not leave integer values as strings. The baseloader is not the unsafe FullLoader, and should not be flagged as an exception to S506.

Code to reproduce:

with open('testfile.yaml') as fhandle:
  loader_yaml = yaml.load(fhandle, Loader=yaml.Baseloader)

Ruff setting: [select = "S506"]
Ruff version: 0.6.8

@AlexWaygood
Copy link
Member

From my reading of the pyyaml docs, your rationale makes sense to me. I'm not a security expert, however, and it looks like we match bandit's original behaviour here (and it looks like they've had this behaviour for a long time).

Digging into the source code for pyyaml a bit:

  • BaseLoader is defined here, and SafeLoader here. They're the same, except that BaseLoader has BaseConstructor and BaseResolver in its bases list, whereas SafeLoader has SafeConstructor and Resolver in its bases list.
  • BaseConstructor is here, BaseResolver is here, SafeConstructor is here, Resolver is here. SafeConstructor is a subclass of BaseConstructor; Resolver is a subclass of BaseResolver. SafeConstructor overrides many methods from BaseConstructor, but Resolver seems to be something of a pointless subclass that doesn't override anything from BaseResolver.
  • From what I can tell, the methods overridden in SafeConstructor seem mainly to extend the capabilities provided in the BaseConstructor superclass, rather than overriding anything to make it safer -- i.e., exactly as you and the pyyaml docs state.

@ericwb, sorry for the ping -- I don't suppose you'd be able to shed light on this behaviour from bandit, would you? Is there a reason why SafeLoader would be safer than BaseLoader?

@AlexWaygood AlexWaygood added the rule Implementing or modifying a lint rule label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

2 participants