Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 31 additions & 4 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion internal/manifest/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
144 changes: 135 additions & 9 deletions internal/manifest/python.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package manifest
import (
"bufio"
"fmt"
"io"
"os"
"path/filepath"
"regexp"
Expand Down Expand Up @@ -522,18 +523,26 @@ 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
// though extra.txt did not exist. A URL is never copied into the result,
// 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)
Expand All @@ -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])
}
Expand Down Expand Up @@ -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".
Expand All @@ -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)
Expand Down Expand Up @@ -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]
Expand Down
Loading
Loading