Skip to content

fix(python): decode the Pipfile as TOML - #346

Open
arpitjain099 wants to merge 1 commit into
XmirrorSecurity:masterfrom
arpitjain099:fix/pipfile-toml
Open

arpitjain099 wants to merge 1 commit into
XmirrorSecurity:masterfrom
arpitjain099:fix/pipfile-toml

Conversation

@arpitjain099

Copy link
Copy Markdown

ParsePipfile declares TOML struct tags and then decodes with a JSON decoder:

pip := struct {
    DevPackages map[string]string `toml:"dev-packages"`
    Packages    map[string]string `toml:"packages"`
}{}

file.OpenReader(func(reader io.Reader) {
    if err := json.NewDecoder(reader).Decode(&pip); err != nil {
        logs.Warnf("unmarshal file %s err: %s", file.Relpath(), err)
    }
})

A Pipfile is TOML, so the decode fails on the first line of every real file. The error is logged at WARN, which is invisible on a default run, and the function returns a graph with no children. The packages are absent from the graph rather than present-but-unresolved, so no lookup happens for any of them.

The repo's own test/python case 2 shows it. The fixture is a valid Pipfile declaring elasticsearch, and on master the case produces an empty tree:

[WARN] pip.go:25: unmarshal file 2/Pipfile err: invalid character 's' looking for beginning of value
res:
[:]<>()
  [:]<>()

[[source]] is the first line of the file, and s is the character the JSON decoder chokes on.

The change

BurntSushi/toml is already a direct dependency, and golang/gopkg.go and rust/cargo.go already read TOML with toml.NewDecoder(reader).Decode(&x), so the first half is that same call.

Swapping the decoder then exposes the second half. A Pipfile entry is either a bare specifier or an inline table carrying extras, markers or a VCS source:

requests = "==2.25.1"
django = {version = "==3.2.4", extras = ["bcrypt"]}
somelib = {git = "https://example.com/somelib.git"}

Against map[string]string the table form errors out and takes the rest of the file with it, which I hit while testing: a four package Pipfile came back with one entry. So the maps are read as any and the version pulled out of either shape, with the leading == stripped the way ParsePipfileLock already does at line 65. A table with no version, such as a git or path dependency, contributes the package with an empty version rather than being dropped.

Verification

With the change, test/python case 2 goes from an empty tree to elasticsearch:*. It does not fully pass in my checkout: the expected tree is elasticsearch:8.9.0 with its transitives, and resolving * to a concrete version needs the remote registry, which my sandbox has no network for. So the parse half is fixed and the resolve half I could not exercise here.

opensca/sca/python/pip_test.go covers the parse half without a network: a Pipfile with a bare specifier, a *, an inline table with version and extras, a git table with no version, and a [dev-packages] entry. It asserts all five are present, the versions are stripped, and only the dev one is marked Develop. On master it fails with got 0 dependencies [], want 5.

go test ./... gives the same package-level results before and after this change: test/java, test/php, test/python and test/ruby fail either way in my environment, all of them wanting network or a local toolchain.

ParsePipfile declares toml struct tags and then decodes the file with
json.NewDecoder. A Pipfile is TOML, so the decode fails on the first line of
every real file, the error is logged at WARN (invisible without -v), and the
function returns an empty graph. Every package declared in a Pipfile is absent
from the dependency graph rather than merely unresolved, so nothing is looked
up and the scan reports no components for it.

The repo already depends on BurntSushi/toml and already reads TOML this way in
golang/gopkg.go and rust/cargo.go, so this is the same call.

Decoding as TOML then exposes the second half: a Pipfile entry is either a bare
specifier, requests = "==2.25.1", or an inline table carrying extras, markers or
a VCS source, django = {version = "==3.2.4", extras = ["bcrypt"]}. Against
map[string]string the table form errors and abandons the rest of the file, so
the entries are read as any and the version pulled out of either shape, with the
leading == stripped the way ParsePipfileLock already does.

The repo's own test/python case 2 shows the effect: its fixture is a valid
Pipfile declaring elasticsearch, and the case produced an empty tree.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant