containerbuildsystem / cachi2

Cachi2 is a CLI tool that pre-fetches your project's dependencies to aid in making your build process network-isolated.
GNU General Public License v3.0
8 stars 26 forks source link

Code QL fixes reported under the 'Security' tab #581

Closed eskultety closed 2 months ago

eskultety commented 3 months ago

This PR dabs at fixing the Code QL issues reported under the 'Security' tab. Most (if not all) are false positives anyway, but let's make Code QL happy for a while. The reported problem with tar.extractall will remain even after this PR because Code QL doesn't see past the immediate context at the end of its nose and so it doesn't see that we kinda validated the TAR members before extraction. It probably wants to see us applying the new filter= argument, but that's Python 3.12 only material [1] and so we'll have to dismiss that alert manually, besides it's reported against the unit tests so the naive solution proposed here should be good enough to catch test issues early.

[1] https://docs.python.org/3.12/library/tarfile.html#tarfile.TarFile.extractall

Maintainers will complete the following section

Note: if the contribution is external (not from an organization member), the CI pipeline will not run automatically. After verifying that the CI is safe to run:

eskultety commented 3 months ago

Whereas I don't mind adding the code for the tarball check, I wonder if we shouldn't just dismiss these specific alerts using the not used in production code CodeQL option.

In any case, CodeQL is dumb enough to still flag those lines even with the check added 🤦

Yes, as noted in

The reported problem with tar.extractall will remain even after this PR because Code QL doesn't see past the immediate context at the end of its nose and so it doesn't see that we kinda validated the TAR members before extraction.

Anyway, I spent time on adding the check just for the sake of argument, if we're okay with dismissing it, I can drop that patch and be done with it. :)

brunoapimentel commented 3 months ago

Anyway, I spent time on adding the check just for the sake of argument, if we're okay with dismissing it, I can drop that patch and be done with it. :)

I'm fine either way :)

eskultety commented 3 months ago

Anyway, I spent time on adding the check just for the sake of argument, if we're okay with dismissing it, I can drop that patch and be done with it. :)

I'm fine either way :)

I dropped the commit in question and I'll dismiss that particular alert.