From 785b6c2199a032f3cdecbd5e3bec436d969b8b2f Mon Sep 17 00:00:00 2001 From: Michael Kedar Date: Thu, 28 Mar 2024 12:49:24 +1100 Subject: [PATCH] GR: Add simple test for package-lock.json writing (#891) Added a basic-ish lockfile write test, which needs the mock npm registry. Also, fixed a bug on an edge-case where if the new version has new dependency types (e.g. newly added `optionalDependencies`) it wouldn't get added to the newly-written lockfile. I had to change how I attempt to preserve formatting to do that. --- cmd/osv-scanner/__snapshots__/fix_test.snap | 4 +- go.mod | 2 +- .../lockfile/__snapshots__/npm_test.snap | 192 ++++++++++++++++++ .../npm_registry/@fake-registry-a-1.2.4.json | 26 +++ .../npm_registry/@fake-registry-a-2.3.5.json | 28 +++ internal/resolution/lockfile/npm_test.go | 65 ++++++ internal/resolution/lockfile/npm_v1.go | 2 +- internal/resolution/lockfile/npm_v2.go | 63 ++++-- 8 files changed, 358 insertions(+), 24 deletions(-) create mode 100755 internal/resolution/lockfile/__snapshots__/npm_test.snap create mode 100644 internal/resolution/lockfile/fixtures/npm_registry/@fake-registry-a-1.2.4.json create mode 100644 internal/resolution/lockfile/fixtures/npm_registry/@fake-registry-a-2.3.5.json diff --git a/cmd/osv-scanner/__snapshots__/fix_test.snap b/cmd/osv-scanner/__snapshots__/fix_test.snap index ae75c589e93..96f2fb1a732 100755 --- a/cmd/osv-scanner/__snapshots__/fix_test.snap +++ b/cmd/osv-scanner/__snapshots__/fix_test.snap @@ -165,7 +165,9 @@ Warning: `fix` exists as both a subcommand of OSV-Scanner and as a file on the f "version": "1.6.1", "resolved": "https://registry.npmjs.org/concat-stream/-/concat-stream-1.6.1.tgz", "integrity": "sha512-gslSSJx03QKa59cIKqeJO9HQ/WZMotvYJCuaUULrLpjj8oG40kV2Z+gz82pVxlTkOADi4PJxQPPfhl1ELYrrXw==", - "engines": ["node >= 0.8"], + "engines": [ + "node >= 0.8" + ], "dependencies": { "inherits": "^2.0.3", "typedarray": "^0.0.6", diff --git a/go.mod b/go.mod index b7d0f3cea5c..411cde85434 100644 --- a/go.mod +++ b/go.mod @@ -27,6 +27,7 @@ require ( github.com/pandatix/go-cvss v0.6.2 github.com/spdx/tools-golang v0.5.3 github.com/tidwall/gjson v1.17.1 + github.com/tidwall/pretty v1.2.1 github.com/tidwall/sjson v1.2.5 github.com/urfave/cli/v2 v2.27.1 golang.org/x/exp v0.0.0-20240325151524-a685a6edb6d8 @@ -93,7 +94,6 @@ require ( github.com/skeema/knownhosts v1.2.1 // indirect github.com/spdx/gordf v0.0.0-20221230105357-b735bd5aac89 // indirect github.com/tidwall/match v1.1.1 // indirect - github.com/tidwall/pretty v1.2.1 // indirect github.com/vbatts/tar-split v0.11.5 // indirect github.com/xanzy/ssh-agent v0.3.3 // indirect github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect diff --git a/internal/resolution/lockfile/__snapshots__/npm_test.snap b/internal/resolution/lockfile/__snapshots__/npm_test.snap new file mode 100755 index 00000000000..b186ad80745 --- /dev/null +++ b/internal/resolution/lockfile/__snapshots__/npm_test.snap @@ -0,0 +1,192 @@ + +[TestNpmWrite - 1] +{ + "name": "r", + "version": "1.0.0", + "lockfileVersion": 2, + "requires": true, + "packages": { + "": { + "name": "r", + "version": "1.0.0", + "license": "ISC", + "workspaces": [ + "W" + ], + "dependencies": { + "@fake-registry/a": "^1.2.3", + "@fake-registry/b": "^1.0.1" + }, + "devDependencies": { + "a-dev": "npm:@fake-registry/a@^2.3.4" + } + }, + "node_modules/@fake-registry/a": { + "version": "1.2.4", + "resolved": "http://localhost:4873/@fake-registry%2fa/-/a-1.2.4.tgz", + "integrity": "sha512-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa==", + "license": "OriginalLicenseDoNotSteal", + "dependencies": { + "@fake-registry/b": "^1.0.0", + "@fake-registry/e": "^1.0.0" + } + }, + "node_modules/@fake-registry/b": { + "version": "1.0.1", + "resolved": "http://localhost:4873/@fake-registry%2fb/-/b-1.0.1.tgz", + "integrity": "sha512-uocjkNUbEPBa/oFFYNq+CkWkkr4My+gJQHnB1fFqIrIQtvjbQ/4dtp/6Kmfb5SFJ/bVfAGZ8mmC+c3Bz4JISYQ==", + "license": "ISC" + }, + "node_modules/@fake-registry/c": { + "version": "1.1.1", + "resolved": "http://localhost:4873/@fake-registry%2fc/-/c-1.1.1.tgz", + "integrity": "sha512-bihWUzvU62dcwPN4TguhntQpC1zpj7H0fHAhffya6tl3BCrEdjqO4oEpNHF/jtU5PPvY0L60mJNUS6MbizEVrA==", + "dev": true, + "license": "BSD-2-Clause", + "peerDependencies": { + "@fake-registry/d": "^2.0.0" + } + }, + "node_modules/@fake-registry/d": { + "version": "2.2.2", + "resolved": "http://localhost:4873/@fake-registry%2fd/-/d-2.2.2.tgz", + "integrity": "sha512-YLeJVbfOCJZcUizGgpvPesIVSY9TYmWz2HcF+7mWxSuXuvu13FABprnRSDGEhljBRM+QAaUD+nbUHWenq2vL4w==", + "dev": true, + "license": "ISC" + }, + "node_modules/a-dev": { + "name": "@fake-registry/a", + "version": "2.3.5", + "resolved": "http://localhost:4873/@fake-registry%2fa/-/a-2.3.5.tgz", + "integrity": "none", + "dev": true, + "license": "Stolen", + "optionalDependencies": { + "@fake-registry/b": "*" + } + }, + "node_modules/a-dev/node_modules/@fake-registry/b": { + "version": "2.0.0", + "resolved": "http://localhost:4873/@fake-registry%2fb/-/b-2.0.0.tgz", + "integrity": "sha512-ZYMUG0g+wowBRAVWuRMI9mV8/3IJ5tYw1i+Xedy5LjVo7RAQaOqJbhEWvdubBlkmaXSoI666cdnJIX/SI6FPpw==", + "dev": true, + "license": "ISC", + "dependencies": { + "@fake-registry/c": "^1.0.0", + "@fake-registry/d": "^2.0.0" + } + }, + "node_modules/w": { + "resolved": "W", + "link": true + }, + "W": { + "name": "w", + "version": "1.0.0", + "license": "ISC", + "devDependencies": { + "@fake-registry/a": "^2.3.4" + } + }, + "W/node_modules/@fake-registry/a": { + "version": "2.3.5", + "resolved": "http://localhost:4873/@fake-registry%2fa/-/a-2.3.5.tgz", + "integrity": "none", + "dev": true, + "license": "Stolen", + "optionalDependencies": { + "@fake-registry/b": "*" + } + }, + "W/node_modules/@fake-registry/b": { + "version": "2.0.0", + "resolved": "http://localhost:4873/@fake-registry%2fb/-/b-2.0.0.tgz", + "integrity": "sha512-ZYMUG0g+wowBRAVWuRMI9mV8/3IJ5tYw1i+Xedy5LjVo7RAQaOqJbhEWvdubBlkmaXSoI666cdnJIX/SI6FPpw==", + "dev": true, + "license": "ISC", + "dependencies": { + "@fake-registry/c": "^1.0.0", + "@fake-registry/d": "^2.0.0" + } + } + }, + "dependencies": { + "@fake-registry/a": { + "version": "1.2.4", + "resolved": "http://localhost:4873/@fake-registry%2fa/-/a-1.2.4.tgz", + "integrity": "sha512-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa==", + "requires": { + "@fake-registry/b": "^1.0.0", + "@fake-registry/e": "^1.0.0" + } + }, + "@fake-registry/b": { + "version": "1.0.1", + "resolved": "http://localhost:4873/@fake-registry%2fb/-/b-1.0.1.tgz", + "integrity": "sha512-uocjkNUbEPBa/oFFYNq+CkWkkr4My+gJQHnB1fFqIrIQtvjbQ/4dtp/6Kmfb5SFJ/bVfAGZ8mmC+c3Bz4JISYQ==" + }, + "@fake-registry/c": { + "version": "1.1.1", + "resolved": "http://localhost:4873/@fake-registry%2fc/-/c-1.1.1.tgz", + "integrity": "sha512-bihWUzvU62dcwPN4TguhntQpC1zpj7H0fHAhffya6tl3BCrEdjqO4oEpNHF/jtU5PPvY0L60mJNUS6MbizEVrA==", + "dev": true, + "requires": {} + }, + "@fake-registry/d": { + "version": "2.2.2", + "resolved": "http://localhost:4873/@fake-registry%2fd/-/d-2.2.2.tgz", + "integrity": "sha512-YLeJVbfOCJZcUizGgpvPesIVSY9TYmWz2HcF+7mWxSuXuvu13FABprnRSDGEhljBRM+QAaUD+nbUHWenq2vL4w==", + "dev": true + }, + "a-dev": { + "version": "npm:@fake-registry/a@2.3.5", + "resolved": "http://localhost:4873/@fake-registry%2fa/-/a-2.3.5.tgz", + "integrity": "none", + "dev": true, + "requires": { + "@fake-registry/b": "*" + }, + "dependencies": { + "@fake-registry/b": { + "version": "2.0.0", + "resolved": "http://localhost:4873/@fake-registry%2fb/-/b-2.0.0.tgz", + "integrity": "sha512-ZYMUG0g+wowBRAVWuRMI9mV8/3IJ5tYw1i+Xedy5LjVo7RAQaOqJbhEWvdubBlkmaXSoI666cdnJIX/SI6FPpw==", + "dev": true, + "requires": { + "@fake-registry/c": "^1.0.0", + "@fake-registry/d": "^2.0.0" + } + } + } + }, + "w": { + "version": "file:W", + "requires": { + "@fake-registry/a": "^2.3.4" + }, + "dependencies": { + "@fake-registry/a": { + "version": "2.3.5", + "resolved": "http://localhost:4873/@fake-registry%2fa/-/a-2.3.5.tgz", + "integrity": "none", + "dev": true, + "requires": { + "@fake-registry/b": "*" + } + }, + "@fake-registry/b": { + "version": "2.0.0", + "resolved": "http://localhost:4873/@fake-registry%2fb/-/b-2.0.0.tgz", + "integrity": "sha512-ZYMUG0g+wowBRAVWuRMI9mV8/3IJ5tYw1i+Xedy5LjVo7RAQaOqJbhEWvdubBlkmaXSoI666cdnJIX/SI6FPpw==", + "dev": true, + "requires": { + "@fake-registry/c": "^1.0.0", + "@fake-registry/d": "^2.0.0" + } + } + } + } + } +} + +--- diff --git a/internal/resolution/lockfile/fixtures/npm_registry/@fake-registry-a-1.2.4.json b/internal/resolution/lockfile/fixtures/npm_registry/@fake-registry-a-1.2.4.json new file mode 100644 index 00000000000..35eff95c3e3 --- /dev/null +++ b/internal/resolution/lockfile/fixtures/npm_registry/@fake-registry-a-1.2.4.json @@ -0,0 +1,26 @@ +{ + "name": "@fake-registry/a", + "version": "1.2.4", + "description": "package a", + "main": "index.js", + "scripts": { + "test": "echo \"Error: no test specified\" && exit 1" + }, + "author": { + "name": "a author" + }, + "license": "OriginalLicenseDoNotSteal", + "dependencies": { + "@fake-registry/b": "^1.0.0", + "@fake-registry/e": "^1.0.0" + }, + "_id": "@fake-registry/a@1.2.4", + "_nodeVersion": "10.24.1", + "_npmVersion": "7.24.2", + "dist": { + "integrity": "sha512-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa==", + "shasum": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "tarball": "http://localhost:4873/@fake-registry%2fa/-/a-1.2.4.tgz" + }, + "contributors": [] +} \ No newline at end of file diff --git a/internal/resolution/lockfile/fixtures/npm_registry/@fake-registry-a-2.3.5.json b/internal/resolution/lockfile/fixtures/npm_registry/@fake-registry-a-2.3.5.json new file mode 100644 index 00000000000..f516938220b --- /dev/null +++ b/internal/resolution/lockfile/fixtures/npm_registry/@fake-registry-a-2.3.5.json @@ -0,0 +1,28 @@ +{ + "name": "@fake-registry/a", + "version": "2.3.5", + "description": "package a@2", + "main": "index.js", + "scripts": { + "test": "echo \"Error: no test specified\" && exit 1" + }, + "author": { + "name": "a@2 author" + }, + "license": "Stolen", + "optionalDependencies": { + "@fake-registry/b": "*" + }, + "dependencies": { + "@fake-registry/b": "*" + }, + "_id": "@fake-registry/a@2.3.5", + "_nodeVersion": "10.24.1", + "_npmVersion": "7.24.2", + "dist": { + "integrity": "none", + "shasum": "ffffffff", + "tarball": "http://localhost:4873/@fake-registry%2fa/-/a-2.3.5.tgz" + }, + "contributors": [] +} \ No newline at end of file diff --git a/internal/resolution/lockfile/npm_test.go b/internal/resolution/lockfile/npm_test.go index 35d3130d9eb..c026300bb60 100644 --- a/internal/resolution/lockfile/npm_test.go +++ b/internal/resolution/lockfile/npm_test.go @@ -1,12 +1,16 @@ package lockfile_test import ( + "bytes" + "os" + "path/filepath" "testing" "deps.dev/util/resolve" "deps.dev/util/resolve/dep" "github.com/google/go-cmp/cmp" "github.com/google/osv-scanner/internal/resolution/lockfile" + "github.com/google/osv-scanner/internal/testutility" lf "github.com/google/osv-scanner/pkg/lockfile" ) @@ -142,3 +146,64 @@ func TestNpmReadV1(t *testing.T) { t.Errorf("npm lockfile mismatch (-want/+got):\n%s", diff) } } + +func TestNpmWrite(t *testing.T) { + t.Parallel() + + // Set up mock npm registry + srv := testutility.NewMockHTTPServer(t) + srv.SetResponseFromFile(t, "/@fake-registry%2Fa/1.2.4", "./fixtures/npm_registry/@fake-registry-a-1.2.4.json") + srv.SetResponseFromFile(t, "/@fake-registry%2Fa/2.3.5", "./fixtures/npm_registry/@fake-registry-a-2.3.5.json") + + // Copy package-lock.json to temporary directory + dir := testutility.CreateTestDir(t) + b, err := os.ReadFile("./fixtures/npm_v2/package-lock.json") + if err != nil { + t.Fatalf("could not read test file: %v", err) + } + file := filepath.Join(dir, "package-lock.json") + if err := os.WriteFile(file, b, 0600); err != nil { + t.Fatalf("could not copy test file: %v", err) + } + + // create an npmrc file in temp directory pointing to mock registry + npmrcFile, err := os.Create(filepath.Join(dir, ".npmrc")) + if err != nil { + t.Fatalf("could not create .npmrc file: %v", err) + } + if _, err := npmrcFile.WriteString("registry=" + srv.URL); err != nil { + t.Fatalf("failed writing npmrc file: %v", err) + } + + patches := []lockfile.DependencyPatch{ + { + Pkg: resolve.PackageKey{ + System: resolve.NPM, + Name: "@fake-registry/a", + }, + OrigVersion: "1.2.3", + NewVersion: "1.2.4", + }, + { + Pkg: resolve.PackageKey{ + System: resolve.NPM, + Name: "@fake-registry/a", + }, + OrigVersion: "2.3.4", + NewVersion: "2.3.5", + }, + } + + df, err := lf.OpenLocalDepFile(file) + if err != nil { + t.Fatalf("failed to open file: %v", err) + } + defer df.Close() + + buf := new(bytes.Buffer) + npmIO := lockfile.NpmLockfileIO{} + if err := npmIO.Write(df, buf, patches); err != nil { + t.Fatalf("unable to update npm package-lock.json: %v", err) + } + testutility.NewSnapshot().WithCRLFReplacement().MatchText(t, buf.String()) +} diff --git a/internal/resolution/lockfile/npm_v1.go b/internal/resolution/lockfile/npm_v1.go index be09852354c..e64397d37e1 100644 --- a/internal/resolution/lockfile/npm_v1.go +++ b/internal/resolution/lockfile/npm_v1.go @@ -105,7 +105,7 @@ func (rw NpmLockfileIO) modifyPackageLockDependencies(lockJSON string, patches m func (rw NpmLockfileIO) modifyPackageLockDependenciesRecurse(lockJSON, path string, depth int, patches map[string]map[string]string, api *datasource.NpmRegistryAPIClient) (string, error) { for pkg, data := range gjson.Get(lockJSON, path).Map() { - pkgPath := fmt.Sprintf("%s.%s", path, strings.ReplaceAll(pkg, ".", "\\.")) + pkgPath := fmt.Sprintf("%s.%s", path, gjson.Escape(pkg)) if data.Get("dependencies").Exists() { var err error lockJSON, err = rw.modifyPackageLockDependenciesRecurse(lockJSON, pkgPath+".dependencies", depth+1, patches, api) diff --git a/internal/resolution/lockfile/npm_v2.go b/internal/resolution/lockfile/npm_v2.go index eef52b2c4b2..43642089d52 100644 --- a/internal/resolution/lockfile/npm_v2.go +++ b/internal/resolution/lockfile/npm_v2.go @@ -12,6 +12,7 @@ import ( "github.com/google/osv-scanner/internal/resolution/datasource" "github.com/google/osv-scanner/pkg/lockfile" "github.com/tidwall/gjson" + "github.com/tidwall/pretty" "github.com/tidwall/sjson" "golang.org/x/exp/maps" ) @@ -186,7 +187,7 @@ func (rw NpmLockfileIO) modifyPackageLockPackages(lockJSON string, patches map[s } if upgrades, ok := patches[pkg]; ok { if newVer, ok := upgrades[value.Get("version").String()]; ok { - fullPath := "packages." + strings.ReplaceAll(key, ".", "\\.") + fullPath := "packages." + gjson.Escape(key) var err error if lockJSON, err = rw.updatePackage(lockJSON, fullPath, pkg, newVer, api); err != nil { return lockJSON, err @@ -207,37 +208,57 @@ func (rw NpmLockfileIO) updatePackage(jsonText, jsonPath, packageName, newVersio // The "dependencies" returned from the registry includes (can include?) both optional and regular dependencies // But the "optionalDependencies" are (always?) removed from "dependencies" package-lock.json. for _, opt := range npmData.Get("optionalDependencies|@keys").Array() { - s, _ := sjson.Delete(npmData.Raw, "dependencies."+opt.String()) + depName := gjson.Escape(opt.String()) + s, _ := sjson.Delete(npmData.Raw, "dependencies."+depName) npmData = gjson.Parse(s) } + if len(npmData.Get("dependencies").Map()) == 0 { + s, _ := sjson.Delete(npmData.Raw, "dependencies") + npmData = gjson.Parse(s) + } + + pkgData := gjson.Get(jsonText, jsonPath) + pkgText := pkgData.Raw + // I can't find a consistent list of what fields should be included in package-lock.json packages // https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json#packages seems list some // but I've seen fields not listed there get included, and fields that it says to include (e.g. license) missing; // Might fill in as much of package.json? https://docs.npmjs.com/cli/v9/configuring-npm/package-json // It also seems to depend on npm version? // Instead, just modify the fields that are present - for _, key := range gjson.Get(jsonText, jsonPath+"|@keys").Array() { - switch key.String() { + keyArray := pkgData.Get("@keys").Array() + // If dependency types were not previously present, we want to add them. + necessaryKeys := []string{"dependencies", "optionalDependencies", "peerDependencies"} + + keys := make([]string, len(keyArray), len(keyArray)+len(necessaryKeys)) + for i, key := range keyArray { + keys[i] = gjson.Escape(key.String()) + } + for _, key := range necessaryKeys { + if npmData.Get(key).Exists() && !pkgData.Get(key).Exists() { + keys = append(keys, key) + } + } + for _, key := range keys { + switch key { case "resolved": - jsonText, _ = sjson.Set(jsonText, jsonPath+".resolved", npmData.Get("dist.tarball").String()) + pkgText, _ = sjson.Set(pkgText, "resolved", npmData.Get("dist.tarball").String()) case "integrity": - jsonText, _ = sjson.Set(jsonText, jsonPath+".integrity", npmData.Get("dist.integrity").String()) + pkgText, _ = sjson.Set(pkgText, "integrity", npmData.Get("dist.integrity").String()) case "bin": // the api formats the paths as "./path/to", while package-lock.json seem to use "path/to" // TODO: smarter way for indentation - newVal := npmData.Get(key.String() + "|@pretty:{\"prefix\": \" \"}") + newVal := npmData.Get(key) if newVal.Exists() { text := newVal.Raw - // remove trailing newlines that @pretty creates for objects - text = strings.TrimSuffix(text, "\n") for k, v := range newVal.Map() { text, _ = sjson.Set(text, k, filepath.Clean(v.String())) } - jsonText, _ = sjson.SetRaw(jsonText, jsonPath+".bin", text) + pkgText, _ = sjson.SetRaw(pkgText, "bin", text) } else { // explicitly remove it if it's no longer present - jsonText, _ = sjson.Delete(jsonText, jsonPath+".bin") + pkgText, _ = sjson.Delete(pkgText, "bin") } // if all dependencies have been removed, explicitly remove the field case "dependencies": @@ -247,27 +268,27 @@ func (rw NpmLockfileIO) updatePackage(jsonText, jsonPath, packageName, newVersio case "peerDependencies": fallthrough case "optionalDependencies": - if !npmData.Get(key.String()).Exists() { + if !npmData.Get(key).Exists() { // TODO: Think of the orphaned children - jsonText, _ = sjson.Delete(jsonText, jsonPath+"."+key.String()) + pkgText, _ = sjson.Delete(pkgText, key) continue } fallthrough default: - // use @pretty to format objects correctly & with correct indentation - // TODO: smarter way for indentation - newVal := npmData.Get(key.String() + "|@pretty:{\"prefix\": \" \"}") + newVal := npmData.Get(key) if newVal.Exists() { - text := newVal.Raw - // remove trailing newlines that @pretty creates for objects - text = strings.TrimSuffix(text, "\n") - jsonText, _ = sjson.SetRaw(jsonText, jsonPath+"."+key.String(), text) + pkgText, _ = sjson.SetRaw(pkgText, key, newVal.Raw) } // if it doesn't exist, assume it's one of the package-lock flags e.g. "dev" // TODO: It could be a removed field } } - return jsonText, nil + // pretty the json because setting nested objects breaks the formatting. + // Setting Prefix & Indent to account for the fact that this is not the top-level object. + pkgText = string(pretty.PrettyOptions([]byte(pkgText), &pretty.Options{Prefix: " ", Indent: " "})) + pkgText = strings.TrimSpace(pkgText) // remove leading spaces & newline pretty creates + + return sjson.SetRaw(jsonText, jsonPath, pkgText) }