It's nice that it fixes the `InsecureProtocol` test-case (which maybe
should have been a test-case for the import resolution library in the
first place?)
But it's not quite right:
1. it adds spurious flow for `clashing_attr`
2. it runs into huge problems for typetracking_imports/tracked.expected
3. it runs into the problem for
https://github.com/github/codeql/pull/10176 with an `from <pkg>
import *` blocking flow from previously defined variable, that is NOT
overridden. (simplistic_reexport.bar_attr)
However, we can see that `from <pkg> import *` and `import pkg` are
handled differently. Would have liked `has_defined_all_indirection` to
behave in the same way no matter how the import was made.
For `from <pkg> import <attr>` we would use to treat the `<pkg>`
(ImportExpr) as a definition of the name `<attr>`.
Since this removes bad import-flow, and nothing broke, I'm guessing this
was never intentional.
However, as illustrated by the `CWE-327-InsecureProtocol` test, this fix
is NOT good enough, since now even the `secure_context` is considered to
be insecure (for both versions). Ouch.
Will fix this in a later commit, since it was only discoverd late on.
We talked about how it's annoying that we in 4 places have the same fix
`isPreferredModuleForName(<module>.getFile(), <name> + ["", ".__init__"])`
, and that it would be nice to have a simple wrapper predicate that
ensures we never forget to do the `+ ["", ".__init__"]` dance...
I had trouble coming up with a name for this (ironically), but
I think `getModuleFromName` is good enough.
I have tried to add a few links to support the claim that these
algorithms are strong/safe. It wasn't always super easy, so in some
cases I have ended up just linking to the documentation of the
`cryptography` Python package.
Co-authored-by: REDMOND\brodes <brodes@microsoft.com>
Similar to the Ruby changes from
https://github.com/github/codeql/pull/11461
I feel the change to `DataFlowFunciton.getParameter` where we use
`not exists(func.getArgByName(_))` is not very great, but I was not allowed
to use `not exists(this.getParameter(any(ParameterPosition _).isKeyword(_)))`
because of negative recursion.
