feat: sbom generation ubuntu and nix packages #1973
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![snyk-io[bot]](https://avatars.githubusercontent.com/in/372950?s=60&v=4){kind=small}
WalkthroughAdds a Go-based SBOM CLI, Ubuntu and Nix SBOM generators, an SPDX merger, SPDX types and UUID generator, Nix packaging and dev tooling, tests, integration into AMI provisioning to produce an Ubuntu SBOM, and workflow steps to upload SBOM artifacts to S3. Changes
sequenceDiagram
participant CLI as SBOM CLI
participant UbuntuGen as Ubuntu Generator
participant NixWrap as Nix Wrapper (sbomnix)
participant Merger as SBOM Merger
participant FS as File System
CLI->>UbuntuGen: request Ubuntu SBOM (ubuntu)
UbuntuGen->>FS: run dpkg-query, read /usr/share/doc
FS-->>UbuntuGen: package metadata, license excerpts
UbuntuGen-->>CLI: write Ubuntu SPDX JSON
CLI->>NixWrap: request Nix SBOM (nix)
NixWrap->>FS: exec sbomnix --spdx=path
FS-->>NixWrap: Nix SPDX JSON
NixWrap-->>CLI: write Nix SPDX JSON
CLI->>Merger: merge Ubuntu & Nix SPDX (combined)
Merger->>FS: load Ubuntu and Nix JSON
FS-->>Merger: parsed documents
Merger->>Merger: renumber IDs, merge creators, sanitize CPEs, create System root
Merger-->>CLI: write merged SPDX JSON
🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @nix/packages/sbom/internal/merge/merger.go:
- Around line 311-319: generateUUID currently fills bytes using time.Now which
yields nearly identical bytes; replace the loop with crypto/rand to securely
populate the 16-byte slice in generateUUID, handle the read error, then set
UUIDv4 version/variant bits (b[6] = (b[6]&0x0f)|0x40 and b[8] =
(b[8]&0x3f)|0x80) before formatting the string; this will produce proper unique
UUIDs used by DocumentNamespace.
In @nix/packages/sbom/internal/nix/wrapper.go:
- Around line 19-35: Wrapper.Generate currently passes the user-controlled
outputPath directly to sbomnix; sanitize and validate outputPath before use by
applying filepath.Clean and rejecting paths that perform traversal (e.g.,
contain ".." segments or resolve outside an allowed base directory), ensure it
is a valid relative/absolute path as per your policy, and return an error if
validation fails; perform this validation prior to constructing exec.Command
(use the same validation pattern as the Save() methods in ubuntu/generator.go
and merger.go) and only then invoke sbomnix with the validated outputPath via
w.SbomnixPath.
In @nix/packages/sbom/internal/ubuntu/generator.go:
- Around line 393-402: The duplicated, insecure generateUUID() in generator.go
(and the same in merger.go) uses time.Now for randomness; replace both with a
single secure implementation using crypto/rand: create a new internal package
(e.g., package spdx) with a GenerateUUID() that reads 16 bytes via rand.Read,
sets RFC4122 v4 bits (b[6] and b[8]) and formats the UUID, then import and call
spdx.GenerateUUID() from generator.go and merger.go to remove duplication and
ensure proper randomness.
🧹 Nitpick comments (3)
nix/packages/sbom/internal/merge/merger.go (1)
69-90: Consider preserving original relationships from Ubuntu SBOM.The merge logic discards existing relationships from both source SBOMs and only creates new
CONTAINSrelationships to the system root. If the source SBOMs contain inter-package dependency relationships (e.g.,DEPENDS_ON), these are lost.If preserving dependency graphs is needed, consider also copying relevant relationships from the source documents with renumbered SPDX IDs.
nix/packages/sbom/internal/spdx/types.go (1)
21-36: Consider addingprimaryPackagePurposefor better SBOM classification.The
Packagestruct is comprehensive for current needs. If you later need to distinguish between application, library, container, etc., the SPDX 2.3primaryPackagePurposefield would be useful.nix/packages/sbom/internal/ubuntu/generator.go (1)
297-356: License normalization is comprehensive but uses iteration order-dependent prefix matching.The prefix matching at lines 352-356 iterates over a map, which has undefined order in Go. This could cause inconsistent results if multiple prefixes match (e.g.,
gpl-2vsgpl-2+). However, since exact matches are checked first and the current map doesn't have overlapping prefixes that would conflict, this is acceptable.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.github/workflows/ami-release-nix.ymlflake.nixnix/checks.nixnix/devShells.nixnix/packages/default.nixnix/packages/sbom/cmd/sbom/main.gonix/packages/sbom/default.nixnix/packages/sbom/go.modnix/packages/sbom/internal/merge/merger.gonix/packages/sbom/internal/nix/wrapper.gonix/packages/sbom/internal/spdx/types.gonix/packages/sbom/internal/ubuntu/generator.goscripts/nix-provision.shstage2-nix-psql.pkr.hcl
🧰 Additional context used
🧬 Code graph analysis (3)
nix/packages/sbom/internal/ubuntu/generator.go (2)
nix/packages/sbom/internal/spdx/types.go (6)
Document(4-13)CreationInfo(15-19)Package(21-36)Relationship(47-51)ExternalRef(53-57)Checksum(42-45)nix/packages/supascan/internal/validator/validator.go (1)
New(78-80)
nix/packages/sbom/cmd/sbom/main.go (3)
nix/packages/sbom/internal/ubuntu/generator.go (1)
NewGenerator(36-41)nix/packages/sbom/internal/nix/wrapper.go (1)
NewWrapper(13-17)nix/packages/sbom/internal/merge/merger.go (1)
NewMerger(16-18)
nix/packages/sbom/internal/merge/merger.go (1)
nix/packages/sbom/internal/spdx/types.go (5)
Document(4-13)CreationInfo(15-19)Package(21-36)Relationship(47-51)ExternalRef(53-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: nix-eval / eval
🔇 Additional comments (24)
nix/packages/sbom/go.mod (1)
1-3: LGTM!The Go module definition is straightforward with no external dependencies. The module path correctly reflects the package location within the repository.
scripts/nix-provision.sh (2)
62-75: LGTM with minor observation on error handling.The SBOM generation function is well-structured:
- Properly sources the Nix environment before execution
- Uses the pinned
GIT_SHAfor reproducibility- The
--no-progressflag is appropriate for CI environmentsSince
set -o errexitis enabled at the script level (line 4), the build will fail if SBOM generation fails, which is appropriate behavior for this provisioning context.
84-86: Correct placement in the provisioning flow.Generating the SBOM after the stage2 playbook and before cleanup ensures it captures the complete package state.
nix/packages/sbom/default.nix (2)
3-16: LGTM!The Go module build configuration is correct:
vendorHash = nullaligns with the dependency-freego.modsubPackagescorrectly targets the CLI entry point- Metadata is properly defined
18-33: Well-designed wrapper architecture.The wrappers correctly:
- Keep
sbom-ubuntuminimal (no external tool dependencies)- Inject
sbomnixinto PATH only for Nix-related operations- Provide clear separation between Ubuntu-only, Nix-only, and combined workflows
stage2-nix-psql.pkr.hcl (1)
131-136: LGTM!The file provisioner correctly:
- Downloads the SBOM after generation (proper ordering after the shell provisioner)
- Includes the postgres version in the output filename for traceability
- Uses the same path (
/tmp/ubuntu-sbom.spdx.json) thatgenerate_sbomwrites toIf SBOM generation fails, the build will appropriately fail when this provisioner cannot find the source file.
flake.nix (1)
32-32: LGTM!The
sbomnixinput fromtiiuae/sbomnixis a well-established tool for generating SBOMs from Nix derivations. The flake.lock file will pin the specific revision for reproducibility.nix/devShells.nix (1)
64-73: LGTM!The additions are well-organized with clear comments grouping the tools by purpose. The Go tooling and SBOM packages are appropriately added to the development shell.
.github/workflows/ami-release-nix.yml (2)
146-155: LGTM, mirrors staging upload correctly.The prod upload step follows the same pattern as staging, which is appropriate for CI workflow clarity and maintainability.
114-123: SBOM file location is correctly configured. The file is generated at/tmp/ubuntu-sbom.spdx.jsonon the EC2 instance, downloaded by the packer provisioner instage2-nix-psql.pkr.hclasubuntu-sbom-${var.postgres-version}.spdx.jsoninto the working directory, and then uploaded by the workflow using the same versioned filename. The version variable is consistently sourced fromansible/vars.ymlthrough the build-ami action, ensuring the naming convention aligns between packer and the workflow upload step.nix/checks.nix (2)
400-407: LGTM!The
sbom-buildscheck is a straightforward smoke test that verifies the binary builds and runs. Redirecting to/dev/nullis appropriate for this validation.
409-426: LGTM!The
sbomnix-availablecheck properly:
- Sets up PATH with the sbomnix package
- Verifies the command is discoverable
- Validates
--helpfunctionality- Uses appropriate exit codes for failure cases
nix/packages/default.nix (2)
10-15: LGTM!Clean integration of the SBOM tooling. The
systemparameter is correctly used to access the platform-specific sbomnix package, and the sbomPkgs binding follows the standardcallPackagepattern.
39-46: LGTM!The SBOM tools are properly exported using
inherit, which cleanly exposes all the SBOM-related packages from the sbomPkgs set.nix/packages/sbom/internal/nix/wrapper.go (2)
9-17: LGTM!The
Wrapperstruct andNewWrapperconstructor are straightforward and follow Go conventions.
21-23: Good defensive check on derivation path.Validating that
derivationPathexists before invoking sbomnix is appropriate defensive programming.nix/packages/sbom/cmd/sbom/main.go (3)
14-36: LGTM!Clean CLI structure with proper subcommand routing and helpful error messages for unknown commands.
53-87: LGTM!The
ubuntuCommandcorrectly parses flags with sensible defaults and properly handles theprogress/no-progressflag combination.
128-163: LGTM!Good use of
os.MkdirTempwithdefer os.RemoveAllfor proper cleanup of temporary files. The required flag validation for--nix-targetis appropriate.nix/packages/sbom/internal/merge/merger.go (1)
194-218: CPE validation approach looks reasonable.The complex regex pattern for CPE 2.3 validation and the fallback to
fixCPEFormatfor invalid CPEs from sbomnix output is a pragmatic approach. The code correctly preserves non-CPE external references.nix/packages/sbom/internal/spdx/types.go (1)
4-13: LGTM!The
Documentstruct correctly models the core SPDX 2.3 document structure with appropriate JSON tags.nix/packages/sbom/internal/ubuntu/generator.go (3)
139-156: Path traversal protections are correctly implemented.The multi-layer defense against CWE-23 is well done:
filepath.Cleannormalizes the path- Explicit check for
.., leading/, and path separators- Verification that resolved path stays within
/usr/share/doc/This addresses the security fix mentioned in the commit history.
255-282: LGTM on file hashing security.Using
os.Lstatto check for regular files before reading prevents symlink-following attacks that could read arbitrary files outside package directories.
225-236: Shell command is safe; package name sanitization is adequate.Go's
exec.Commanddoesn't invoke a shell, so shell metacharacters are not a concern. Additionally, dpkg package names are constrained by policy to lowercase letters, digits, plus (+), hyphen (-), and dot (.) only—shell-sensitive characters are not permitted by dpkg specification. The path traversal checks (blocking "..", "/", and path separators) are appropriate for this use case.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @nix/packages/sbom/internal/nix/wrapper_test.go:
- Around line 69-89: Add a clarifying comment in wrapper.go next to the
outputPath validation (the code that calls filepath.Clean and checks for ".."
via strings.Contains) stating that outputPath is expected to be
operator-controlled/untrusted-input must not be accepted and that Generate() is
invoked from command-line flags (e.g., via main.go) so this assumption holds;
mention that allowing absolute paths that clean to system locations is
intentional given that input is trusted, and warn future maintainers to
revalidate if Generate() becomes reachable from untrusted sources.
🧹 Nitpick comments (4)
nix/packages/sbom/internal/nix/wrapper_test.go (1)
96-107: Consider adding explicit success assertion for non-error cases.When
tc.wantErrisfalse, the test implicitly passes without checking thaterr == nil. Adding an explicit assertion would make the test behavior clearer and catch unexpected errors.♻️ Suggested improvement
if tc.wantErr { if err == nil { t.Errorf("Generate() with outputPath=%q should return error", tc.outputPath) return } if tc.errContain != "" && !strings.Contains(err.Error(), tc.errContain) { t.Errorf("Generate() error = %q, want error containing %q", err.Error(), tc.errContain) } + } else if err != nil { + // For cases using "echo" as fake sbomnix, we expect success + t.Errorf("Generate() with outputPath=%q returned unexpected error: %v", tc.outputPath, err) } // Note: For non-error cases, we can't fully test without a real sbomnix // The test verifies that path validation passesnix/packages/sbom/internal/ubuntu/generator.go (1)
356-361: Non-deterministic prefix matching due to map iteration order.Go's map iteration order is random, so when checking prefix matches, input like
"Apache-2.0"could match either"apache-2"→"Apache-2.0"or"apache"→"NOASSERTION"depending on which key is checked first. The testTestNormalizeLicenseApache20acknowledges this, but the behavior is unpredictable.Consider using an ordered slice of key-value pairs sorted by key length (longest first) to ensure deterministic matching:
Suggested fix using ordered matching
+type licenseMapping struct { + pattern string + replacement string +} + +// Ordered by length (longest first) for deterministic prefix matching +var licenseMappings = []licenseMapping{ + {"eclipse-public-license-v1.0", "EPL-1.0"}, + {"ubuntu-font-licence-1.0", "Ubuntu-Font-1.0"}, + {"nrl-2-clause", "NOASSERTION"}, + {"public-domain", "NOASSERTION"}, + {"openldap-2.8", "NOASSERTION"}, + // ... remaining entries ordered by length +} func normalizeLicense(license string) string { // ...existing checks... - // Check for prefix match (case-insensitive) - for old, new := range replacements { - if strings.HasPrefix(licenseLower, old) { - return new + // Check for prefix match (longest first for deterministic behavior) + for _, m := range licenseMappings { + if strings.HasPrefix(licenseLower, m.pattern) { + return m.replacement } }nix/packages/sbom/internal/ubuntu/generator_test.go (1)
114-122: Acknowledges non-deterministic behavior, but consider fixing the root cause.This test documents that
"Apache-2.0"can yield either"Apache-2.0"or"NOASSERTION"due to Go's random map iteration order. While documenting this behavior is appropriate, it may be worth fixing the underlying issue innormalizeLicense(as suggested earlier) rather than accepting non-deterministic behavior in production.nix/packages/sbom/internal/merge/merger_test.go (1)
565-593: Minor: Benchmark package generation produces duplicate IDs.The package generation uses
string(rune('0'+i%10))which only produces characters '0'-'9', resulting in duplicate SPDXID values for the 100 packages. While acceptable for performance benchmarking, this doesn't test merge behavior with realistic unique IDs. Also, errors fromos.WriteFileon lines 591-592 are silently ignored.Consider using unique IDs for more realistic benchmarking
for i := 0; i < 100; i++ { ubuntuDoc.Packages[i] = spdx.Package{ - SPDXID: "SPDXRef-Ubuntu-Package-" + string(rune('0'+i%10)), - Name: "package-" + string(rune('0'+i%10)), + SPDXID: fmt.Sprintf("SPDXRef-Ubuntu-Package-%d", i), + Name: fmt.Sprintf("package-%d", i), } }And add error handling:
-os.WriteFile(ubuntuPath, ubuntuData, 0644) -os.WriteFile(nixPath, nixData, 0644) +if err := os.WriteFile(ubuntuPath, ubuntuData, 0644); err != nil { + b.Fatalf("Failed to write ubuntu file: %v", err) +} +if err := os.WriteFile(nixPath, nixData, 0644); err != nil { + b.Fatalf("Failed to write nix file: %v", err) +}
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
nix/checks.nixnix/packages/sbom/internal/merge/merger.gonix/packages/sbom/internal/merge/merger_test.gonix/packages/sbom/internal/nix/wrapper.gonix/packages/sbom/internal/nix/wrapper_test.gonix/packages/sbom/internal/spdx/uuid.gonix/packages/sbom/internal/spdx/uuid_test.gonix/packages/sbom/internal/ubuntu/generator.gonix/packages/sbom/internal/ubuntu/generator_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- nix/packages/sbom/internal/nix/wrapper.go
- nix/checks.nix
- nix/packages/sbom/internal/merge/merger.go
🧰 Additional context used
🧬 Code graph analysis (5)
nix/packages/sbom/internal/ubuntu/generator_test.go (3)
nix/packages/sbom/internal/ubuntu/generator.go (2)
NewGenerator(36-41)DpkgPackage(19-29)nix/packages/sbom/internal/spdx/types.go (4)
Document(4-13)CreationInfo(15-19)Package(21-36)Relationship(47-51)nix/packages/sbom/internal/merge/merger_test.go (1)
TestSaveInvalidPath(457-465)
nix/packages/sbom/internal/spdx/uuid_test.go (1)
nix/packages/sbom/internal/spdx/uuid.go (1)
GenerateUUID(9-22)
nix/packages/sbom/internal/nix/wrapper_test.go (1)
nix/packages/sbom/internal/nix/wrapper.go (1)
NewWrapper(15-19)
nix/packages/sbom/internal/ubuntu/generator.go (2)
nix/packages/sbom/internal/spdx/types.go (6)
Document(4-13)CreationInfo(15-19)Package(21-36)Relationship(47-51)ExternalRef(53-57)Checksum(42-45)nix/packages/sbom/internal/spdx/uuid.go (1)
GenerateUUID(9-22)
nix/packages/sbom/internal/merge/merger_test.go (3)
nix/packages/sbom/internal/merge/merger.go (1)
NewMerger(16-18)nix/packages/sbom/internal/spdx/types.go (5)
ExternalRef(53-57)Document(4-13)CreationInfo(15-19)Package(21-36)Relationship(47-51)nix/packages/sbom/internal/ubuntu/generator_test.go (1)
TestSaveInvalidPath(351-364)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: nix-eval / eval
🔇 Additional comments (25)
nix/packages/sbom/internal/nix/wrapper_test.go (4)
10-21: LGTM!Constructor test is straightforward and validates the expected field assignment.
23-35: LGTM!Good validation test ensuring non-existent derivation paths are properly rejected with an appropriate error message.
111-129: LGTM!While this tests standard library behavior rather than wrapper logic directly, it serves as useful documentation of the path normalization assumptions the wrapper relies on.
131-161: LGTM!Good documentation test that clearly explains the traversal detection mechanism and its limitations via comments on lines 132-135. The test cases effectively demonstrate when
..is retained vs. resolved byfilepath.Clean.nix/packages/sbom/internal/spdx/uuid.go (1)
9-22: LGTM! Clean RFC 4122 UUID v4 implementation.The implementation correctly uses
crypto/randfor secure randomness, properly sets the version 4 bits (0100xxxxat byte 6) and RFC 4122 variant bits (10xxxxxxat byte 8), and formats the output in the standard 8-4-4-4-12 pattern.nix/packages/sbom/internal/ubuntu/generator.go (8)
19-41: LGTM!Well-structured types with appropriate fields for capturing dpkg package metadata and generator configuration.
43-106: LGTM!The
Generate()method follows a clear workflow: enumerate packages, create document structure with a root system package, convert each package to SPDX format with proper containment relationships, and establish the document-describes-system relationship. Error handling is appropriate.
108-142: LGTM with a minor suggestion.The implementation correctly parses dpkg-query output with proper bounds checking. The stdout print at line 140 works for CLI usage but could be conditioned on
ShowProgressfor consistency, or use a proper logger for library reuse.
144-157: Good defensive path validation.The layered approach—sanitizing the name, checking for traversal patterns, and verifying the final path prefix—provides solid protection against path traversal attacks.
185-228: LGTM!The conversion properly maps dpkg fields to SPDX Package structure, handles placeholder values like
(none), and constructs valid purl external references. The SPDXID format ensures uniqueness across packages.
230-258: LGTM!The checksum calculation properly sanitizes input to prevent command injection, skips directories, and computes a combined hash from individual file hashes. Silent failures are acceptable since this is an optional feature enabled by
IncludeFiles.
260-287: LGTM! Secure file hashing.Good security practices: uses
Lstatto avoid following symlinks, rejects relative paths and traversal attempts, only processes regular files, and properly closes the file handle.
289-300: LGTM!Standard JSON file writing pattern with proper indentation for readability.
nix/packages/sbom/internal/spdx/uuid_test.go (1)
8-125: LGTM! Comprehensive UUID test coverage.Excellent test suite covering all critical aspects: basic functionality, format validation, RFC 4122 version 4 and variant bit verification, uniqueness across 1000 iterations, length validation, and a benchmark. The test assertions correctly validate the UUID structure at positions 14 (version) and 19 (variant).
nix/packages/sbom/internal/ubuntu/generator_test.go (5)
12-38: LGTM!Clean table-driven test covering all flag combinations for the generator constructor.
152-213: LGTM! Good security test coverage.The test cases correctly verify path validation behavior, including that
filepath.Cleanresolves absolute paths with traversal patterns (like/tmp/../../../etc/passwd→/etc/passwd), while relative traversal paths are rejected. The tests document the actual security boundaries of thehashFilefunction.
215-270: LGTM!Good tests verifying hash function properties: consistency across multiple calls, correct length (64 hex chars for SHA256), and differentiation for distinct content.
272-364: LGTM!Comprehensive save/load roundtrip tests verify JSON serialization integrity and proper error handling for invalid paths.
366-454: LGTM!Thorough tests for package-to-SPDX conversion covering field mappings, SPDXID format, external reference structure, and edge cases for placeholder homepage values.
nix/packages/sbom/internal/merge/merger_test.go (6)
20-51: LGTM!Comprehensive table-driven tests for CPE component sanitization covering special characters, underscores, empty strings, and edge cases with leading/trailing dashes.
53-121: LGTM!Good coverage of SPDX ID renumbering with various prefixes and CPE format fixing, including handling of sbomnix duplicate patterns and non-CPE inputs.
123-217: LGTM!Good tests verifying external reference cleaning preserves non-CPE refs, and creator merging properly combines sources while deduplicating shared creators.
232-353: LGTM!Solid end-to-end merge test that verifies document creation, SPDX version, naming convention, package count, root system package presence, and proper Nix package prefixing. Diagnostic logging on failure aids debugging.
355-415: LGTM!Good error handling tests verifying that merge failures for missing or invalid files produce appropriate error messages identifying the problematic source.
508-548: LGTM!Good test verifying that repeated merges produce unique
DocumentNamespacevalues, confirming proper UUID integration.
| { | ||
| name: "path with embedded traversal resolves clean", | ||
| outputPath: "/tmp/foo/../../../etc/passwd", | ||
| wantErr: false, // filepath.Clean resolves this to /etc/passwd (no .. remains) | ||
| }, | ||
| { | ||
| name: "path with double dot in middle", | ||
| outputPath: "/tmp/../tmp/output.json", | ||
| wantErr: false, // filepath.Clean resolves this to /tmp/output.json | ||
| }, | ||
| { | ||
| name: "path traversal at start", | ||
| outputPath: "../../output.json", | ||
| wantErr: true, | ||
| errContain: "path traversal", | ||
| }, | ||
| { | ||
| name: "deeply nested traversal resolves clean", | ||
| outputPath: "/a/b/c/../../../../etc/passwd", | ||
| wantErr: false, // filepath.Clean resolves this to /etc/passwd (no .. remains) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the wrapper.go file and examine the Generate function
fd -t f 'wrapper\.go' --exclude='*_test.go'Repository: supabase/postgres
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Check how Generate() is called in the codebase
rg -n 'Generate\(' --type=go | head -20Repository: supabase/postgres
Length of output: 1424
🏁 Script executed:
#!/bin/bash
# Look at the wrapper.go implementation to understand the validation logic
cat -n nix/packages/sbom/internal/nix/wrapper.go | head -100Repository: supabase/postgres
Length of output: 1307
🏁 Script executed:
#!/bin/bash
# Check for security comments or assumptions in wrapper.go
cat -n nix/packages/sbom/internal/nix/wrapper.goRepository: supabase/postgres
Length of output: 1307
🏁 Script executed:
#!/bin/bash
# Check if outputFile/outputPath could come from untrusted sources by reviewing main.go context
rg -n -B5 -A5 'Generate.*outputFile\|outputPath' nix/packages/sbom/cmd/sbom/main.goRepository: supabase/postgres
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for any security or threat model documentation
fd -t f '(README|SECURITY|THREAT|\.md)' nix/Repository: supabase/postgres
Length of output: 806
Add documentation clarifying that outputPath is expected to be operator-controlled.
The validation at lines 27-31 in wrapper.go intentionally allows absolute paths with embedded traversal (e.g., /tmp/foo/../../../etc/passwd → /etc/passwd) because filepath.Clean() resolves the .. sequences away before the strings.Contains() check. While relative traversal (../foo) is correctly rejected, the implementation relies on the implicit assumption that outputPath is always operator-controlled.
Since Generate() is called with command-line flags (see main.go lines 121 and 182), this assumption is currently valid. Add a comment in wrapper.go documenting that outputPath must not accept untrusted input, to prevent future security gaps if the API surface changes.
🤖 Prompt for AI Agents
In @nix/packages/sbom/internal/nix/wrapper_test.go around lines 69 - 89, Add a
clarifying comment in wrapper.go next to the outputPath validation (the code
that calls filepath.Clean and checks for ".." via strings.Contains) stating that
outputPath is expected to be operator-controlled/untrusted-input must not be
accepted and that Generate() is invoked from command-line flags (e.g., via
main.go) so this assumption holds; mention that allowing absolute paths that
clean to system locations is intentional given that input is trusted, and warn
future maintainers to revalidate if Generate() becomes reachable from untrusted
sources.
Files Created/Modified:
New Files:
Modified Files:
New Packages:
New Checks:
CI Integration:
At release time, the SBOM will be:
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.