diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e349d1..072fdc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **requirements.txt lines that name a local path, such as + `./vendor/foo-1.0.tar.gz` or `../lib`, were dropped with no signal.** Such + a line was neither analyzed nor listed with the other lines the scan did not + read, so that block read as complete while the line was missing from it. A + line that begins with a dot or a path separator is now listed by line number + with its path under `unreadLines` in JSON, and in the block before the + results in the table and markdown reports, with the reason `local path + requirements are not read, so this dependency is not analyzed`. The + dependency it declares is still not analyzed. + - **A `GITHUB_TOKEN` that GitHub refused turned the database download into an unexplained `HTTP 401`.** The release lookup behind `cryptodeps update` and the automatic update, and the repository lookup of a GitHub URL scan, sent @@ -49,10 +59,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **requirements.txt `-r` includes and `name @ url` lines were dropped with no signal.** Such lines, and `-e` and plain URL lines, are now listed by line number with a reason, before the results in the table and markdown reports, - and under `unreadLines` in JSON. The included file or the package name is - shown; a URL from the manifest is not, so a `-r` line that names a URL is - listed by its line number and kind only. The dependencies they declare are - still not analyzed. + and under `unreadLines` in JSON. They are listed the same way in the other + requirements files the scan reads, the `requirements-*.txt` family (such as + `requirements-dev.txt` or `requirements_test.txt`) and `requirements/*.txt`. + The included file or the package name is shown; a URL from the manifest is + not, so a `-r` line that names a URL is listed by its line number and kind + only. The dependencies they declare are still not analyzed. + +- **A requirements.txt `-r` line continued with a backslash was listed as an + include named `\`.** pip reads a line that ends in a backslash together with + the next line. For a requirements.txt holding `cryptography==42.0.0`, `-r \` + and `https://example.invalid/continued.txt` on three lines, which pip reads + as one `-r` URL include, the table listed `line 2: \: included requirements + files are not followed, so the dependencies it declares are not analyzed` + and `line 3: URL requirements are not read, so this dependency is not + analyzed`, and the JSON `unreadLines` entry for line 2 had `"name": "\\"`. + The unread lines are now read with a continued line joined to the next one + as pip joins it, and numbered by its first line, so that file is listed as + `line 2: included requirements URLs are not followed, so the dependencies it + declares are not analyzed`, with no name. A comment line that ends in a + backslash is not joined to the next line. The dependencies, findings and + exit code for that file are unchanged. - **A scan of a GitHub repository reported reachability for source code it never fetched.** A GitHub URL or `owner/repo` argument downloads the diff --git a/internal/manifest/parser.go b/internal/manifest/parser.go index 7d25773..c148055 100644 --- a/internal/manifest/parser.go +++ b/internal/manifest/parser.go @@ -30,7 +30,8 @@ type Manifest struct { Ecosystem types.Ecosystem Dependencies []types.Dependency // Unread lists the lines that declare dependencies the parser did not - // read. Only requirements.txt fills it today. + // read. Only requirements files fill it today: requirements.txt, the + // requirements-*.txt family and the requirements/*.txt layout. Unread []types.UnreadLine } diff --git a/internal/manifest/python.go b/internal/manifest/python.go index f9b2986..684eef0 100644 --- a/internal/manifest/python.go +++ b/internal/manifest/python.go @@ -6,6 +6,7 @@ package manifest import ( "bufio" "fmt" + "io" "os" "path/filepath" "regexp" @@ -522,9 +523,10 @@ var pypiSeparatorPattern = regexp.MustCompile(`[-_.]+`) // as "paramiko @ https://example.org/paramiko.tar.gz". var directReferencePattern = regexp.MustCompile(`^([A-Za-z0-9][A-Za-z0-9._-]*)\s*(?:\[[^\]]*\])?\s*@`) -// unreadLines lists the lines of a requirements.txt that declare dependencies -// parseRequirementsTxt does not read: "-r" includes, editable installs and URL -// requirements. It returns nil for every other manifest. +// unreadLines lists the lines of a requirements file that declare dependencies +// parseRequirementsTxt does not read: "-r" includes, editable installs, URL +// requirements and local path requirements. It returns nil for every manifest +// the scan does not read with parseRequirementsTxt. // // parseRequirementsTxt skips these lines, and the scan used to say nothing // about them, so a requirements.txt holding "-r extra.txt" was reported as @@ -532,8 +534,15 @@ var directReferencePattern = regexp.MustCompile(`^([A-Za-z0-9][A-Za-z0-9._-]*)\s // because one in a manifest can carry credentials. That holds for an "-r" // include as well: pip fetches "-r https://host/reqs.txt", so such a line is // listed by its kind and line number only, while "-r extra.txt" names the file. +// A local path such as "./vendor/foo-1.0.tar.gz" is shown, as the file of an +// "-r" include is. +// +// Lines are read as pip reads them, with a line that ends in a backslash joined +// to the next one. Read one physical line at a time, "-r \" followed by a URL +// on the next line was listed as an include named "\" and then as a URL +// requirement, where pip reads one URL include. func unreadLines(path string) []types.UnreadLine { - if strings.ToLower(filepath.Base(path)) != "requirements.txt" { + if !readAsRequirements(path) { return nil } file, err := os.Open(path) @@ -543,11 +552,11 @@ func unreadLines(path string) []types.UnreadLine { defer file.Close() var unread []types.UnreadLine - scanner := bufio.NewScanner(file) - lineNo := 0 - for scanner.Scan() { - lineNo++ - line := strings.TrimSpace(scanner.Text()) + // On a read error the lines read before it are still listed. + lines, _ := joinContinuedLines(file) + for _, l := range lines { + lineNo := l.number + line := strings.TrimSpace(l.text) if idx := strings.Index(line, " #"); idx != -1 { line = strings.TrimSpace(line[:idx]) } @@ -582,11 +591,70 @@ func unreadLines(path string) []types.UnreadLine { unread = append(unread, types.UnreadLine{Line: lineNo, Reason: "URL requirements are not read, so this dependency is not analyzed"}) } + continue + } + if isLocalPathRequirement(line) { + unread = append(unread, types.UnreadLine{Line: lineNo, Name: localPathRequirementName(line), + Reason: "local path requirements are not read, so this dependency is not analyzed"}) } } return unread } +// isLocalPathRequirement reports whether line names a local project directory +// or archive rather than a package: it begins with a dot or a path separator, +// as in "./vendor/foo-1.0.tar.gz", "../lib" or "/opt/wheels/foo.whl". The +// requirement pattern in parseRequirementsTxt never matched such a line, so it +// was dropped, and unreadLines did not list it either. +func isLocalPathRequirement(line string) bool { + return strings.HasPrefix(line, ".") || strings.HasPrefix(line, "/") || strings.HasPrefix(line, `\`) +} + +// localPathRequirementName returns the path a local path requirement line +// names: the line up to its first per-requirement option (a token beginning +// with "-", such as --hash) and before any environment marker, which is how +// pip splits such a line. +func localPathRequirementName(line string) string { + if idx := strings.Index(line, ";"); idx != -1 { + line = line[:idx] + } + var path []string + for _, tok := range strings.Fields(line) { + if strings.HasPrefix(tok, "-") { + break + } + path = append(path, tok) + } + return strings.Join(path, " ") +} + +// readAsRequirements reports whether the scan reads path with +// parseRequirementsTxt, as it does requirements.txt, the requirements-*.txt +// family and the requirements/*.txt layout. It asks getParserForPath, the +// lookup DetectAndParseAll picks parsers with, rather than repeating its rule, +// and then leaves out pyproject.toml and Pipfile, which Parse reads with +// readers of their own. For a file DetectAndParse accepts, getParserForPath +// returns the parser getParser chose. +// +// unreadLines used to test for the name "requirements.txt" alone, so an "-r" +// include, an "-e" line or a URL requirement in requirements-dev.txt or +// requirements/dev.txt was dropped with no signal, though the scan reads both +// files. +func readAsRequirements(path string) bool { + parser, err := getParserForPath(path) + if err != nil { + return false + } + if _, ok := parser.(*PythonParser); !ok { + return false + } + switch strings.ToLower(filepath.Base(path)) { + case "pyproject.toml", "pipfile": + return false + } + return true +} + // requirementOption reports whether line is the option short or long, and // returns its value. It accepts "-r file", "-rfile", "--requirement file" and // "--requirement=file". @@ -602,6 +670,58 @@ func requirementOption(line, short, long string) (string, bool) { return "", false } +// requirementsLine is one line of a requirements.txt as pip reads it, which +// can span several lines of the file. number is the first of them. +type requirementsLine struct { + number int + text string +} + +// joinContinuedLines splits a requirements.txt into lines as pip does before it +// reads them. A line that ends in a backslash is joined to the next one without +// the backslash, and the joined line is numbered by its first line, so +// +// -r \ +// https://host/reqs.txt +// +// is the one line "-r https://host/reqs.txt", numbered 1. A comment line is not +// continued, even when it ends in a backslash, and a comment line after a +// continued line ends it and adds nothing to it. Comments are otherwise left +// in place for the caller to remove. +func joinContinuedLines(r io.Reader) ([]requirementsLine, error) { + var lines []requirementsLine + var joined strings.Builder + first := 0 // the number of the line being joined, 0 when there is none + scanner := bufio.NewScanner(r) + number := 0 + for scanner.Scan() { + number++ + text := scanner.Text() + comment := strings.HasPrefix(strings.TrimSpace(text), "#") + switch { + case strings.HasSuffix(text, `\`) && !comment: + if first == 0 { + first = number + } + joined.WriteString(strings.Trim(text, `\`)) + case first == 0: + lines = append(lines, requirementsLine{number: number, text: text}) + default: + if !comment { + joined.WriteString(text) + } + lines = append(lines, requirementsLine{number: first, text: joined.String()}) + joined.Reset() + first = 0 + } + } + if first != 0 { + // The file ended on a backslash. + lines = append(lines, requirementsLine{number: first, text: joined.String()}) + } + return lines, scanner.Err() +} + // parseRequirementsTxt parses a requirements.txt file. func (p *PythonParser) parseRequirementsTxt(path string) ([]types.Dependency, error) { file, err := os.Open(path) @@ -640,6 +760,12 @@ func (p *PythonParser) parseRequirementsTxt(path string) ([]types.Dependency, er continue } + // Skip local path requirements such as ./vendor/foo-1.0.tar.gz, which + // unreadLines lists. + if isLocalPathRequirement(line) { + continue + } + // Parse requirement line if matches := requirementPattern.FindStringSubmatch(line); matches != nil { name := matches[1] diff --git a/internal/manifest/requirements_unread_test.go b/internal/manifest/requirements_unread_test.go index d8fe5f8..29ddf4f 100644 --- a/internal/manifest/requirements_unread_test.go +++ b/internal/manifest/requirements_unread_test.go @@ -6,6 +6,7 @@ package manifest import ( "os" "path/filepath" + "reflect" "strings" "testing" ) @@ -64,6 +65,222 @@ func TestRequirementsUnreadLines(t *testing.T) { } } +// A requirements.txt line that names a local path, such as +// "./vendor/foo-1.0.tar.gz" or "../lib", declares a dependency the parser does +// not read. It was dropped without an entry, so the unread block read as +// complete while such lines were missing from it. Each is now listed by line +// with the path it names, without any per-requirement option, environment +// marker or inline comment, and the parser still reads none of them. +func TestRequirementsUnreadLocalPathLines(t *testing.T) { + dir := t.TempDir() + content := strings.Join([]string{ + "cryptography==42.0.0", // 1 read + "./vendor/foo-1.0.tar.gz", // 2 + "../lib", // 3 + "/opt/wheels/bar-2.0-py3-none-any.whl", // 4 + `\\build\wheels\baz-3.0.tar.gz`, // 5 + "./vendor/qux[ssh] --hash=sha256:FAKE", // 6 per-requirement option + `../lib2 ; python_version < "3.12"`, // 7 environment marker + "./vendor/quux-1.0.tar.gz # a vendored copy", // 8 inline comment + "-e ./editable", // 9 editable, listed by kind + "", + }, "\n") + path := filepath.Join(dir, "requirements.txt") + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + m, err := DetectAndParse(dir) + if err != nil { + t.Fatal(err) + } + if len(m.Dependencies) != 1 || m.Dependencies[0].Name != "cryptography" { + t.Errorf("dependencies = %+v, want cryptography only", m.Dependencies) + } + + want := []struct { + line int + name string + reason string + }{ + {2, "./vendor/foo-1.0.tar.gz", "local path"}, + {3, "../lib", "local path"}, + {4, "/opt/wheels/bar-2.0-py3-none-any.whl", "local path"}, + {5, `\\build\wheels\baz-3.0.tar.gz`, "local path"}, + {6, "./vendor/qux[ssh]", "local path"}, + {7, "../lib2", "local path"}, + {8, "./vendor/quux-1.0.tar.gz", "local path"}, + {9, "", "editable"}, + } + if len(m.Unread) != len(want) { + t.Fatalf("Unread = %+v, want %d entries", m.Unread, len(want)) + } + for i, w := range want { + got := m.Unread[i] + if got.Line != w.line || got.Name != w.name || !strings.Contains(got.Reason, w.reason) { + t.Errorf("Unread[%d] = %+v, want line %d name %q and a reason naming %q", i, got, w.line, w.name, w.reason) + } + } +} + +// The scan reads the requirements-*.txt family and requirements/*.txt as well +// as requirements.txt, but listed unread lines only for a file named +// requirements.txt, so the same lines in the others were dropped with no +// signal. Each now lists what requirements.txt lists for the same content, in +// a scan of its directory and, for a file the scan accepts as its argument, in +// a scan of the file itself. +func TestUnreadLinesForEveryRequirementsFile(t *testing.T) { + content := strings.Join([]string{ + "cryptography==42.0.0", // 1 read + "-r base.txt", // 2 + "-r https://example.invalid/private-reqs.txt", // 3 a URL include + "-e git+https://example.invalid/repo.git#egg=x", // 4 + "paramiko @ https://example.invalid/p", // 5 + "https://example.invalid/pkg.whl", // 6 + "--index-url https://example.invalid/simple", // 7 not a dependency + "", + }, "\n") + + reference, err := DetectAndParse(writeFiles(t, map[string]string{"requirements.txt": content})) + if err != nil { + t.Fatal(err) + } + if len(reference.Unread) != 5 { + t.Fatalf("requirements.txt: Unread = %+v, want lines 2 to 6", reference.Unread) + } + + for _, tc := range []struct { + name string + // argument is whether the scan accepts the file as its argument. A + // requirements/*.txt file is read in a scan of a directory only. + argument bool + }{ + {"requirements-dev.txt", true}, + {"requirements_test.txt", true}, + {"requirements.prod.txt", true}, + {filepath.Join("requirements", "dev.txt"), false}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := writeFiles(t, map[string]string{tc.name: content}) + manifests, skipped, err := DetectAndParseAll(dir) + if err != nil || len(skipped) != 0 || len(manifests) != 1 { + t.Fatalf("DetectAndParseAll = %d manifests, skipped %+v, error %v; want 1 manifest", len(manifests), skipped, err) + } + if !reflect.DeepEqual(manifests[0].Unread, reference.Unread) { + t.Errorf("scan of the directory: Unread = %+v, want %+v", manifests[0].Unread, reference.Unread) + } + + if !tc.argument { + return + } + m, err := DetectAndParse(filepath.Join(dir, tc.name)) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(m.Unread, reference.Unread) { + t.Errorf("scan of the file: Unread = %+v, want %+v", m.Unread, reference.Unread) + } + }) + } +} + +// pip reads a line that ends in a backslash together with the next one, so +// "-r \" followed by a URL on the next line is one URL include. It was listed +// as an include named "\" and then as a URL requirement. Each continued line +// is now listed once, by the number of its first line, and a comment line is +// not continued. +func TestRequirementsUnreadLinesJoinContinuedLines(t *testing.T) { + const ( + urlInclude = "included requirements URLs are not followed" + fileInclude = "included requirements files are not followed" + editable = "editable installs (-e) are not read" + directRef = "name @ URL references are not read" + urlReq = "URL requirements are not read" + ) + type entry struct { + line int + name string + reason string + } + cases := []struct { + name string + lines []string + want []entry + }{ + {"URL include", []string{"cryptography==42.0.0", `-r \`, "https://example.invalid/continued.txt"}, + []entry{{2, "", urlInclude}}}, + {"file include", []string{`-r \`, "extra.txt"}, + []entry{{1, "extra.txt", fileInclude}}}, + {"over three lines", []string{`--requirement \`, `\`, "more.txt"}, + []entry{{1, "more.txt", fileInclude}}}, + {"CRLF line endings", []string{"-r \\\r", "extra.txt\r"}, + []entry{{1, "extra.txt", fileInclude}}}, + {"editable install", []string{`-e \`, "git+https://example.invalid/repo.git#egg=x"}, + []entry{{1, "", editable}}}, + {"name @ URL", []string{`paramiko @ \`, "https://example.invalid/p"}, + []entry{{1, "paramiko", directRef}}}, + {"comment line ending in a backslash", []string{`# see below \`, "https://example.invalid/pkg.whl"}, + []entry{{2, "", urlReq}}}, + {"comment line after a continued line", []string{`-r \`, "# a comment", "https://example.invalid/pkg.whl"}, + []entry{{1, "", fileInclude}, {3, "", urlReq}}}, + {"backslash on the last line", []string{"cryptography==42.0.0", `-r \`}, + []entry{{2, "", fileInclude}}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "requirements.txt") + if err := os.WriteFile(path, []byte(strings.Join(c.lines, "\n")+"\n"), 0o644); err != nil { + t.Fatal(err) + } + got := unreadLines(path) + if len(got) != len(c.want) { + t.Fatalf("unreadLines = %+v, want %d entries", got, len(c.want)) + } + for i, w := range c.want { + g := got[i] + if g.Line != w.line || g.Name != w.name || !strings.HasPrefix(g.Reason, w.reason) { + t.Errorf("unreadLines[%d] = %+v, want line %d name %q reason %q...", i, g, w.line, w.name, w.reason) + } + if strings.Contains(g.Name+g.Reason, "example.invalid") { + t.Errorf("unreadLines[%d] copies a URL: %+v", i, g) + } + } + }) + } +} + +// pyproject.toml and Pipfile also go to the Python parser, which reads them +// with readers of their own, so a URL in either is not listed as an unread +// requirements line. +func TestUnreadLinesNotForPyprojectOrPipfile(t *testing.T) { + dir := writeFiles(t, map[string]string{ + "pyproject.toml": "[project]\nname = \"x\"\ndependencies = [\"paramiko @ https://example.invalid/p.tar.gz\"]\n", + "Pipfile": "[packages]\nparamiko = {file = \"https://example.invalid/p.tar.gz\"}\n", + }) + for _, name := range []string{"pyproject.toml", "Pipfile"} { + if got := unreadLines(filepath.Join(dir, name)); got != nil { + t.Errorf("unreadLines(%s) = %+v, want nil", name, got) + } + } +} + +// writeFiles writes files, named relative to a new directory, and returns the +// directory. +func writeFiles(t *testing.T, files map[string]string) string { + t.Helper() + dir := t.TempDir() + for name, content := range files { + path := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + return dir +} + // Other manifests report no unread lines. func TestUnreadLinesOnlyForRequirementsTxt(t *testing.T) { dir := t.TempDir() diff --git a/pkg/types/types.go b/pkg/types/types.go index 30a6968..1ee9f07 100644 --- a/pkg/types/types.go +++ b/pkg/types/types.go @@ -327,9 +327,10 @@ type ScanResult struct { type UnreadLine struct { // Line is the 1-based line number in the manifest. Line int `json:"line" yaml:"line"` - // Name is what the line names: the included file of a "-r" line, or the - // package of a "name @ url" line. Empty when the line is reported by kind - // only, which keeps a URL that may carry credentials out of the report. + // Name is what the line names: the included file of a "-r" line, the + // package of a "name @ url" line, or the path of a local path line. Empty + // when the line is reported by kind only, which keeps a URL that may carry + // credentials out of the report. Name string `json:"name,omitempty" yaml:"name,omitempty"` // Reason says why the line was not read. Reason string `json:"reason" yaml:"reason"`