Mscasso/feat/sp 3838 update go.api to support the new scanning parameters #64
Conversation
📝 WalkthroughWalkthroughIntroduce a ScanningServiceConfig and header-based scan settings; route config through scanning flows and SBOM handling; add server scanning parameters and defaults; enforce minimum engine version; add related tests, Makefile coverage targets, dependency bumps, script tweaks, and config updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as "HTTP Handler"
participant Config as "ScanningServiceConfig"
participant KB as "KB Loader"
participant Engine as "Scan Engine"
Client->>HTTP: POST /scan/direct (form + Scanoss-Settings header)
HTTP->>Config: getConfigFromRequest(params, decoded header)
Config->>Config: UpdateScanningServiceConfigDTO (merge JSON/header/params)
HTTP->>KB: loadKBDetails()
KB->>KB: validateEngineVersion(current, min)
KB-->>HTTP: KB info
HTTP->>Engine: run engine with args (sbom, sbomType, ranking, snippets, honour-file-exts)
Engine-->>HTTP: scan results
HTTP-->>Client: HTTP response with results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting 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 `@pkg/service/kb_details.go`:
- Around line 47-68: Fix the typo in validateEngineVersion: update the zs.Errorf
call that logs when currentVersion.LessThan(minVersion) to include a space after
the period in the message (change "'%s'.Some" to "'%s'. Some" or better "'%s'.
Some features..." ), ensuring the log still references currentEngineVersion and
minEngineVersion in the same zs.Errorf invocation.
In `@pkg/service/scanning_service_config.go`:
- Around line 145-153: The doc comment lists a "snippet_range_tolerance" JSON
field that is not implemented; update the scanSettings struct or remove the doc
entry. Either add a field named snippetRangeTolerance (or SnippetRangeTolerance)
of type int to the scanSettings struct and wire any deserialization/validation
logic where scanSettings is parsed, or remove the "snippet_range_tolerance" line
from the comment so documentation matches the implemented scanSettings fields.
In `@pkg/service/scanning_service.go`:
- Around line 436-439: The pull request adds an unsupported "--ignore-file-ext"
flag when config.honourFileExts is false which causes the SCANOSS engine to
error; remove the args append that injects "--ignore-file-ext" in
scanning_service.go (the block that checks config.honourFileExts and calls args
= append(args, "--ignore-file-ext")), or alternatively add a version check for
the engine binary and return a clear error from the same function if the
requested flag is not supported; in short, stop passing "--ignore-file-ext" to
the scanoss engine (or validate engine support and fail fast with a descriptive
message) referencing the config.honourFileExts branch and the args slice
construction.
🧹 Nitpick comments (7)
Makefile (2)
36-39: Minor inconsistency: Missing-vflag compared tounit_test.The
unit_testtask uses-vfor verbose output, butunit_test_coveromits it. Consider adding-vfor consistency:♻️ Suggested fix
unit_test_cover: ## Run all unit tests in the pkg folder `@echo` "Running unit test framework with coverage..." - go test -cover ./pkg/... + go test -cover -v ./pkg/...
44-47: Minor: Echo message doesn't mention coverage.The echo message should indicate coverage is enabled for clarity.
♻️ Suggested fix
int_test_cover: clean_testcache ## Run all integration tests in the tests folder - `@echo` "Running integration test framework..." + `@echo` "Running integration test framework with coverage..." go test -cover -v ./testspkg/service/scanning_service_config.go (1)
41-54: Consider adding nil check forserverDefaultConfig.If
serverDefaultConfigis nil, this function will panic on the first field access. While callers may ensure this never happens, a defensive check would improve robustness.♻️ Proposed fix
func DefaultScanningServiceConfig(serverDefaultConfig *cfg.ServerConfig) ScanningServiceConfig { + if serverDefaultConfig == nil { + return ScanningServiceConfig{} + } return ScanningServiceConfig{ flags: serverDefaultConfig.Scanning.ScanFlags,pkg/service/kb_details_test.go (1)
87-114: Test lacks proper assertions and modifies global state.This test has several quality concerns:
- Global state modification: Directly setting
engineVersion = "5.4.0"(line 102) modifies package-level state, which could cause test flakiness in parallel execution.- No actual assertion: The test acknowledges it "can't easily assert on log output" (lines 110-112) and relies on manual log inspection.
- Fragile timing:
time.Sleep(3 * time.Second)is unreliable and slows down the test suite.Consider refactoring to either:
- Use a log capture mechanism to assert on logged messages
- Refactor
validateEngineVersionto return validation status for testability- At minimum, add
t.Parallel()avoidance comment to prevent parallel test issues♻️ Suggested improvement - extract testable validation
One approach would be to have
validateEngineVersionreturn a boolean or error that can be tested directly:// In kb_details.go - make validation testable func validateEngineVersion(zs *zap.SugaredLogger, currentEngineVersion, minEngineVersion string) bool { // ... existing logic ... if currentVersion.LessThan(minVersion) { zs.Errorf("...") return false } zs.Infof("...") return true } // In test file - direct unit test func TestValidateEngineVersion(t *testing.T) { logger, _ := zap.NewDevelopment() sugar := logger.Sugar() if validateEngineVersion(sugar, "5.4.0", "6.0.0") { t.Error("Expected validation to fail for version below minimum") } if !validateEngineVersion(sugar, "6.1.0", "6.0.0") { t.Error("Expected validation to pass for version meeting minimum") } }pkg/service/scanning_service.go (1)
412-423: Consider removing unreachable default case.The
defaultcase on line 419-420 is unreachable becausesbomTypeis already validated at lines 104-109 inscanDirect, ensuring onlysbomIdentifyorsbomBlackListreach this point. While defensive coding is good practice, unreachable code can cause confusion.♻️ Optional: Remove unreachable default case
// SBOM configuration if len(sbomFile) > 0 && len(config.sbomType) > 0 { switch config.sbomType { case sbomIdentify: args = append(args, "-s") case sbomBlackList: args = append(args, "-b") - default: - args = append(args, "-s") // Default to identify } args = append(args, sbomFile) }tests/scanning_test.go (2)
136-187: Consider generating base64 strings at test time for maintainability.The hardcoded base64 strings with comments work, but generating them at test time would make the tests more maintainable and self-documenting:
♻️ Optional: Generate base64 from JSON for clarity
import "encoding/base64" import "encoding/json" // Helper to encode settings for tests func encodeSettings(settings map[string]interface{}) string { data, _ := json.Marshal(settings) return base64.StdEncoding.EncodeToString(data) } // Then in test: scanSettingsB64: encodeSettings(map[string]interface{}{ "ranking_enabled": true, "ranking_threshold": 85, "min_snippet_hits": 3, "min_snippet_lines": 8, "honour_file_exts": false, }),
206-219: Close response body to prevent resource leaks.The
resp.Bodyshould be closed after reading. While Go's GC will eventually handle it, explicit closure is best practice for E2E tests that may run many iterations.♻️ Proposed fix
resp, err := c.Do(req) if err != nil { s.Failf("an error was not expected when sending request.", "error: %v", err) } + defer resp.Body.Close() s.Equal(test.want, resp.StatusCode, test.description)Note: The same pattern exists in
TestScanningat line 119 - consider applying there as well for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
CHANGELOG.mdMakefilego.modpkg/config/server_config.gopkg/service/kb_details.gopkg/service/kb_details_test.gopkg/service/scanning_service.gopkg/service/scanning_service_config.gopkg/service/scanning_service_config_test.gopkg/service/scanning_service_test.gotest-support/scanoss.shtests/scanning_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/service/scanning_service_config_test.go (2)
pkg/config/server_config.go (1)
ServerConfig(37-93)pkg/service/scanning_service_config.go (3)
DefaultScanningServiceConfig(41-54)ScanningServiceConfig(28-39)UpdateScanningServiceConfigDTO(162-183)
pkg/service/kb_details_test.go (1)
pkg/service/utils_service.go (1)
NewAPIService(76-79)
pkg/service/kb_details.go (1)
pkg/service/scanning_service_config.go (1)
DefaultScanningServiceConfig(41-54)
🔇 Additional comments (29)
pkg/service/scanning_service_test.go (1)
3-3: LGTM!Copyright year update is appropriate maintenance.
CHANGELOG.md (2)
11-21: LGTM!The changelog entries accurately document the new
scanoss.jsonconfiguration support and the server-side scanning parameters introduced in this PR. The format follows the existing Keep a Changelog conventions.
175-175: LGTM!Version link correctly references the comparison from v1.5.2 to v1.6.0.
Makefile (1)
78-84: LGTM!The
e2e_test_covertask correctly mirrorse2e_testwhile adding the-coverflag for coverage reporting.test-support/scanoss.sh (3)
30-34: LGTM!The MD5 validation correctly ensures the hash is exactly 32 hexadecimal characters using a proper regex pattern. Clear error message and appropriate exit code.
59-68: LGTM!The validation correctly rejects
-nas a standalone argument while allowing-n<name>(attached format). The error message clearly indicates the expected usage.
71-79: Last argument assumption is valid given actual usage pattern.The script captures the last argument as the WFP file, which works correctly because
scanning_service.goalways appends-wand the file path at the end of the argument list with no subsequent arguments. The concern about additional flags following the WFP file is theoretically sound but does not apply to the current invocation pattern.go.mod (1)
37-37: LGTM! Dependency addition supports version validation.The
hashicorp/go-versionlibrary is a well-established choice for semantic version parsing and comparison. Version 1.8.0 is current (released November 2025) and correctly supports the engine version validation implemented inpkg/service/kb_details.go.pkg/config/server_config.go (2)
73-80: LGTM! Well-structured configuration additions.The new scanning parameters are properly documented with inline comments, follow the existing naming conventions, and have appropriate environment variable tags for external configuration.
131-138: LGTM! Sensible defaults.The defaults follow a good principle: ranking is allowed but disabled by default (opt-in), and zero values for thresholds delegate decision-making to the engine. The
HonourFileExts = truedefault is appropriately conservative.pkg/service/scanning_service_config.go (6)
28-39: LGTM! Good encapsulation with unexported fields.The struct properly encapsulates scanning configuration with unexported fields, enforcing access control at the package level.
56-63: LGTM! Proper use of pointer types for optional JSON fields.Using pointers allows distinguishing between "field not provided" (nil) and "field explicitly set to zero/false", which is essential for partial updates.
65-80: LGTM! Proper guard for ranking settings.The function correctly checks
rankingAllowedbefore applying ranking changes and logs a warning when settings are ignored, providing good debugging visibility.
82-106: LGTM! Appropriate validation for snippet settings.The different validation thresholds make sense:
MinSnippetHits >= 0allows disabling the check (0 = engine decides), whileMinSnippetLines > 0correctly requires a positive value since 0 lines would be meaningless.
108-131: LGTM! Proper handling of legacy string parameters.The function correctly treats empty strings as "no change" and gracefully handles invalid flag values by logging an error without failing the entire update.
162-182: LGTM! Clean copy-on-write implementation.The implementation correctly creates a shallow copy (safe since all fields are value types), applies updates sequentially, and maintains immutability of the original config.
pkg/service/scanning_service_config_test.go (7)
27-64: LGTM! Comprehensive validation of default config creation.The test thoroughly validates that all fields are correctly mapped from
ServerConfig.ScanningtoScanningServiceConfig.
66-126: LGTM! Thorough JSON settings test.The test validates all JSON-configurable fields and properly uses pointer types to ensure the JSON payload is correctly structured.
128-168: LGTM! Good negative test for ranking guard.The test correctly validates that ranking settings are ignored when
rankingAllowedis false, ensuring the security guard works as intended.
170-204: LGTM! Validates legacy parameter handling.The test ensures backward compatibility by validating that legacy string parameters (flags, scanType, sbom, dbName) are correctly applied.
206-234: LGTM! Proper error handling validation.The test correctly validates two error scenarios: graceful degradation for invalid flags (keeps original) and proper error return for malformed JSON.
236-301: LGTM! Good integration test for combined updates.The test validates that both JSON settings and legacy string parameters can be applied in a single call, which is the expected real-world usage pattern.
303-313: LGTM! Validates nil config error handling.The test ensures that passing a nil config returns an appropriate error rather than panicking.
pkg/service/kb_details.go (1)
113-114: LGTM! Clean integration with new config model.Using
DefaultScanningServiceConfig(s.config)properly initializes the scanning config from server defaults for the KB details probe. TheminEngineVersionconstant is correctly defined in the package and properly used for engine version validation.pkg/service/scanning_service.go (4)
40-44: LGTM! Constants defined for SBOM types and engine version.The constants are well-named and provide clear documentation of their purpose. Note that
minEngineVersionis declared here but used inkb_details.gofor validation.
169-208: LGTM! Well-structured configuration extraction with proper fallback handling.The function properly:
- Extracts from form values first, falling back to headers
- Handles base64 decoding with appropriate error propagation
- Creates defaults from server config before applying request-specific overrides
227-229: LGTM! Consistent refactoring to use ScanningServiceConfig.The function signatures have been cleanly updated to accept the configuration struct, maintaining a consistent pattern across
singleScan,scanThreaded,workerScan, andscanWfp.
102-120: LGTM! Proper SBOM type validation and file handling.The validation correctly checks against the defined constants and provides clear error messages. The temporary file cleanup via
defer removeFileensures resources are properly released.tests/scanning_test.go (1)
93-101: LGTM! Updated asset format to JSON structure.The assets field now uses the proper JSON format with
componentsarray, which aligns with the SBOM structure expected by the scanning service.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Honour file extensions (not yet implemented in scanoss engine) | ||
| if !config.honourFileExts { | ||
| args = append(args, "--ignore-file-ext") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual code in scanning_service.go to understand the context
wc -l pkg/service/scanning_service.goRepository: scanoss/api.go
Length of output: 93
🏁 Script executed:
# Read the section around lines 436-439 with more context
sed -n '399,441p' pkg/service/scanning_service.goRepository: scanoss/api.go
Length of output: 1429
🏁 Script executed:
# Search for any version checks or feature flags related to this
rg -n "ignore-file-ext|honourFileExts|HonourFileExts" --type go -A 3 -B 3Repository: scanoss/api.go
Length of output: 8037
🏁 Script executed:
# Look for any comments or documentation about scanoss engine compatibility
rg -n "scanoss.*engine|engine.*version|feature.*flag" --type go -iRepository: scanoss/api.go
Length of output: 3024
🏁 Script executed:
# Search for tests related to the --ignore-file-ext flag or honourFileExts
rg -n "ignore-file-ext|honourFileExts" --type go -A 5 -B 5Repository: scanoss/api.go
Length of output: 7804
🏁 Script executed:
# Check if there are any tests in scanning_service_test.go
fd "scanning_service_test.go" --type fRepository: scanoss/api.go
Length of output: 94
🏁 Script executed:
# Look for any README, documentation, or comments about engine flags
fd "README|CHANGELOG|MANIFEST" -t f | head -20Repository: scanoss/api.go
Length of output: 156
🏁 Script executed:
# Check scanning_service_test.go for any tests related to flag construction
grep -n "ignore-file-ext\|honourFileExts\|args\|append" pkg/service/scanning_service_test.go | head -50Repository: scanoss/api.go
Length of output: 40
🏁 Script executed:
# Look at the CHANGELOG to see if there's any mention of --ignore-file-ext feature
rg -n "ignore-file-ext|honour.*file.*ext|HonourFileExts" CHANGELOG.md -iRepository: scanoss/api.go
Length of output: 79
🏁 Script executed:
# Look at the scanning_service.go more carefully around minEngineVersion usage
sed -n '40,50p' pkg/service/scanning_service.goRepository: scanoss/api.go
Length of output: 561
🏁 Script executed:
# Search for more context in CHANGELOG about honourFileExts
rg -n "honourFileExts|honour.*file" CHANGELOG.md -B 5 -A 5Repository: scanoss/api.go
Length of output: 309
🏁 Script executed:
# Look for the specific version requirements or notes
rg -n "5\.4\.|engine.*version|flag.*support" CHANGELOG.md -i -B 2 -A 2Repository: scanoss/api.go
Length of output: 40
🏁 Script executed:
# Check if there are any comments or TODOs in scanning_service.go related to this feature
rg -n "TODO|FIXME|not yet|not implemented|honour" pkg/service/scanning_service.go -iRepository: scanoss/api.go
Length of output: 161
🏁 Script executed:
# Look at the entire scanning_service_test.go to understand test coverage
wc -l pkg/service/scanning_service_test.goRepository: scanoss/api.go
Length of output: 98
🏁 Script executed:
# Read the full CHANGELOG to understand the context better
head -50 CHANGELOG.mdRepository: scanoss/api.go
Length of output: 1549
🏁 Script executed:
# Check if there's any mention of engine version or feature support
rg -n "1\.6\.|Added|implement" CHANGELOG.md | head -30Repository: scanoss/api.go
Length of output: 1093
🏁 Script executed:
# Look at scanning_service_test.go to see if there are tests for flag construction
head -100 pkg/service/scanning_service_test.goRepository: scanoss/api.go
Length of output: 2724
🏁 Script executed:
# Look for any test that exercises the flag construction or engine execution
sed -n '1,200p' pkg/service/scanning_service_test.go | tail -100Repository: scanoss/api.go
Length of output: 2694
🏁 Script executed:
# Search for any reference to "not yet implemented" in code or comments
rg -n "not yet|not implemented" --type goRepository: scanoss/api.go
Length of output: 160
🏁 Script executed:
# Check if there are integration tests or test support files
fd "test.*support|scanoss.sh" --type fRepository: scanoss/api.go
Length of output: 106
🏁 Script executed:
# Look for any release notes or documentation about engine feature support
rg -n "ignore.*file.*ext|honour.*file" --type md -iRepository: scanoss/api.go
Length of output: 92
🏁 Script executed:
# Let's check the test-support file to see what the mock engine does
cat test-support/scanoss.shRepository: scanoss/api.go
Length of output: 2856
🏁 Script executed:
# Check if there are any version-specific release notes for the scanoss engine
rg -n "5\.4\.20|engine.*version|--ignore-file-ext" --type md -iRepository: scanoss/api.go
Length of output: 40
🏁 Script executed:
# Look for any integration with the actual scanoss engine or its documentation
fd "engine|scanoss" --type md | xargs grep -l "ignore\|honour\|file.*ext" 2>/dev/null || echo "No matches found"Repository: scanoss/api.go
Length of output: 261
🏁 Script executed:
# Search for any recent changes or history related to this feature
git log --oneline --all | grep -i "honour\|ignore.*file\|file.*ext" | head -20Repository: scanoss/api.go
Length of output: 40
🏁 Script executed:
# Look for any engine documentation or release notes mentioning this
find . -name "*.md" -o -name "*.txt" | xargs grep -l "scanoss.*engine\|engine.*feature" 2>/dev/null | head -10Repository: scanoss/api.go
Length of output: 40
🏁 Script executed:
# Check if there are any error handling patterns around flag construction
rg -n "args.*append\|execute.*args" pkg/service/scanning_service.go -B 2 -A 2 | head -50Repository: scanoss/api.go
Length of output: 40
🏁 Script executed:
# Search web documentation or any README that might reference engine capabilities
rg -n "scanoss.*5\.4\.|engine.*capability|--ignore-file-ext" . --type txt --type md 2>/dev/null || trueRepository: scanoss/api.go
Length of output: 40
🌐 Web query:
scanoss engine 5.4.20 --ignore-file-ext flag support
💡 Result:
Short answer: No — SCANOSS Engine (including v5.4.x per the official docs) does not expose a --ignore-file-ext flag. The engine CLI options in the SCANOSS Engine docs and the scanoss-py CLI docs list other options (e.g., -s, -b, various flags) but do not document any --ignore-file-ext switch. [1][2]
If you need to skip files by extension, workarounds:
- Pre-filter files (find/rsync/git ls-files) and pass only desired paths to scanoss.
- Use the scanoss-py client to control inputs (scan selected folders/files). See scanoss-py CLI options. [2][3]
Sources:
- SCANOSS Engine CLI documentation (options list, no --ignore-file-ext). [1]
- scanoss-py CLI/options documentation. [2]
- scanoss PyPI / project page (client usage). [3]
References:
[1] https://scanoss.readthedocs.io/projects/scanoss-engine/en/latest/
[2] https://scanoss.readthedocs.io/projects/scanoss-py/en/latest/
[3] https://pypi.org/project/scanoss/
Remove --ignore-file-ext flag — it is not supported by the SCANOSS engine.
The official SCANOSS Engine documentation (v5.4.x and later) does not expose a --ignore-file-ext flag. Passing this unsupported flag will cause the engine to fail with an "unknown option" error when honourFileExts is set to false. The flag should be removed entirely until the engine implements this capability, or the code should validate engine version and fail fast with a clear error message if the flag is not available.
🤖 Prompt for AI Agents
In `@pkg/service/scanning_service.go` around lines 436 - 439, The pull request
adds an unsupported "--ignore-file-ext" flag when config.honourFileExts is false
which causes the SCANOSS engine to error; remove the args append that injects
"--ignore-file-ext" in scanning_service.go (the block that checks
config.honourFileExts and calls args = append(args, "--ignore-file-ext")), or
alternatively add a version check for the engine binary and return a clear error
from the same function if the requested flag is not supported; in short, stop
passing "--ignore-file-ext" to the scanoss engine (or validate engine support
and fail fast with a descriptive message) referencing the config.honourFileExts
branch and the args slice construction.
There was a problem hiding this comment.
this is why we are validating the engine version
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the engine version validation flow in kb_details.go
sed -n '1,100p' pkg/service/kb_details.go | grep -A 20 "validateEngineVersion\|minEngineVersion"
Length of output: 1709
🏁 Script executed:
#!/bin/bash
# Check where engine version validation is actually called
rg -n "validateEngineVersion" pkg/service/kb_details.go -B 5 -A 10
Length of output: 1154
🏁 Script executed:
#!/bin/bash
# Look at when the engine version is checked vs when scanWfp is called
rg -n "loadKBDetails\|validateEngineVersion\|scanWfp" pkg/service/ -B 2 -A 2 --type go
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's any logic that prevents flag usage based on engine version
rg -n "ignore-file-ext|honourFileExts" pkg/service/scanning_service.go -B 5 -A 5
Length of output: 642
@mscasso-scanoss, I understand you've added engine version validation, but there's a critical gap: the validateEngineVersion function in kb_details.go only logs a warning asynchronously via a cron job—it doesn't prevent the --ignore-file-ext flag from being passed during actual scan operations.
Current flow:
loadKBDetailsruns every 30 minutes via cronvalidateEngineVersionlogs an error if version < 5.4.20- Meanwhile,
scanWfp(line 438) immediately adds--ignore-file-extwhenhonourFileExtsis false - If the engine doesn't support this flag, the scan fails with an "unknown option" error
The issue: The version check doesn't gate flag usage. An incompatible engine will still receive the unsupported flag.
Recommended fix:
// In scanWfp, before adding the flag:
if !config.honourFileExts {
if engineVersion == "" || engineVersion == "unknown" {
zs.Warnf("Cannot validate --ignore-file-ext support: engine version unknown")
} else {
currentVer, err := version.NewVersion(engineVersion)
minVer, _ := version.NewVersion("5.4.20") // Version that supports the flag
if err == nil && currentVer.LessThan(minVer) {
return "", fmt.Errorf("--ignore-file-ext requires engine >= 5.4.20, current: %s", engineVersion)
}
}
args = append(args, "--ignore-file-ext")
}This ensures the flag is only added when the engine version is confirmed to support it, preventing runtime failures.
🧬 Code Graph Analysis Results
-
File: pkg/service/scanning_service.go, lines 23-28
const (
sbomIdentify = "identify" // SBOM type to identify components
sbomBlackList = "blacklist" // SBOM type to blacklist components
minEngineVersion = "5.4.20" // Minimum required engine version
)Rationale: Defines the minimum engine version requirement mentioned in the user comment. This is central to understanding why engine version validation exists in the scanning service.
-
File: pkg/service/scanning_service.go, lines 76-79
span.SetAttributes(attribute.Int64("scan.file_count", wfpCount), attribute.String("scan.engine_version", engineVersion))Rationale: Shows where the engine version is exposed as metadata in tracing for a scan, illustrating how engine version information is used during scans. This supports understanding of how engine version is validated or tracked in the system.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@pkg/service/scanning_service.go`:
- Around line 169-191: In getConfigFromRequest replace the debug log that prints
r.Header and r.Form with a redacted/whitelist-only log: update the zs.Debugf
call in function getConfigFromRequest to omit r.Header and r.Form entirely and
only log the safe, trimmed variables (flags, scanType, sbom, dbName,
scanSettings) and optionally a small whitelist of specific headers (e.g.,
"Authorization" excluded; only include header names you explicitly permit) or
indicate "<redacted>" for headers; ensure you modify the zs.Debugf invocation so
it no longer interpolates r.Header or r.Form to avoid leaking credentials.
In `@test-support/scanoss.sh`:
- Around line 30-34: The MD5 validation block that checks the variable md5
currently prints the error with echo to stdout; update the if block that tests
[[ ! "$md5" =~ ^[a-fA-F0-9]{32}$ ]] so the error message for "Error: Invalid MD5
hash format: $md5" is written to stderr (use the same redirection pattern as
other errors in this script, e.g., append >&2) and then exit 1; ensure only the
echo destination changes, leaving the condition and exit intact.
In `@tests/charset_detection_test.go`:
- Around line 81-83: Add the same 403 check and skip logic to the
TestFileContentsWithMissingMD5 test: detect when the HTTP response (resp) from
the file_contents endpoint has StatusCode == http.StatusForbidden and call
s.T().Skip("skipping test: file_contents endpoint returned 403 Forbidden") so
the test is skipped in restricted environments; modify the
TestFileContentsWithMissingMD5 function to mirror the existing check used
elsewhere.
♻️ Duplicate comments (1)
pkg/service/scanning_service.go (1)
399-439: Do not pass--ignore-file-extunless the engine explicitly supports it.
The inline comment says it’s not implemented; passing it risks breaking scans. Gate by engine version or remove the flag.Option A: remove the unsupported flag
- // Honour file extensions (not yet implemented in scanoss engine) - if !config.honourFileExts { - args = append(args, "--ignore-file-ext") - }Please verify current engine support in the official docs:
SCANOSS engine "--ignore-file-ext" flag support (v5.4.20+)?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/service/scanning_service.gotest-support/scanoss.shtests/charset_detection_test.gotests/file_contents_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/service/scanning_service.go (2)
pkg/service/utils_service.go (1)
APIService(71-73)pkg/service/scanning_service_config.go (3)
ScanningServiceConfig(28-39)DefaultScanningServiceConfig(41-54)UpdateScanningServiceConfigDTO(161-182)
🔇 Additional comments (8)
tests/file_contents_test.go (1)
41-43: LGTM - Graceful handling of disabled/restricted endpoints.Skipping on 403 is reasonable for E2E tests that may run against environments with different access configurations. The skip message is descriptive.
tests/charset_detection_test.go (1)
42-44: LGTM - Consistent with other file_contents tests.test-support/scanoss.sh (2)
59-71: LGTM - KB validation logic is clear and well-documented.The distinction between
-n(bare, invalid) and-n<name>(attached, valid but checked for test_kb) is correctly implemented with helpful comments.
74-82: LGTM - Position-independent-wdetection is more robust.The refactor makes the script handle
-wregardless of its position in the argument list, which is more flexible for varying invocation patterns.pkg/service/scanning_service.go (4)
3-3: Copyright year update looks good.
22-44: SBOM constants/min engine version addition is clear.
96-139: Scan config extraction + SBOM validation integration looks solid.
227-265: Config propagation through scan workers is consistent.Also applies to: 338-353
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // getConfigFromRequest extracts the form values from a request and returns the scanning configuration. | ||
| func (s APIService) getConfigFromRequest(r *http.Request, zs *zap.SugaredLogger) (ScanningServiceConfig, error) { | ||
| flags := strings.TrimSpace(r.FormValue("flags")) // Check form for scanning flags | ||
| scanType := strings.TrimSpace(r.FormValue("type")) // Check form for SBOM type | ||
| sbom := strings.TrimSpace(r.FormValue("assets")) // Check form for SBOM contents | ||
| dbName := strings.TrimSpace(r.FormValue("db_name")) // Check form for db name | ||
| // TODO is it necessary to check the header also for these values? | ||
| // Fall back to headers if form values are empty | ||
| if len(flags) == 0 { | ||
| flags = strings.TrimSpace(r.Header.Get("flags")) // Check header for Scanning flags | ||
| flags = strings.TrimSpace(r.Header.Get("flags")) | ||
| } | ||
| if len(scanType) == 0 { | ||
| scanType = strings.TrimSpace(r.Header.Get("type")) // Check header for SBOM type | ||
| scanType = strings.TrimSpace(r.Header.Get("type")) | ||
| } | ||
| if len(sbom) == 0 { | ||
| sbom = strings.TrimSpace(r.Header.Get("assets")) // Check header for SBOM contents | ||
| sbom = strings.TrimSpace(r.Header.Get("assets")) | ||
| } | ||
| if len(dbName) == 0 { | ||
| dbName = strings.TrimSpace(r.Header.Get("db_name")) // Check header for SBOM contents | ||
| dbName = strings.TrimSpace(r.Header.Get("db_name")) | ||
| } | ||
| scanSettings := strings.TrimSpace(r.Header.Get("scanoss-settings")) // Check the header for scan settings | ||
| if s.config.App.Trace { | ||
| zs.Debugf("Header: %v, Form: %v, flags: %v, type: %v, assets: %v, db_name %v", r.Header, r.Form, flags, scanType, sbom, dbName) | ||
| zs.Debugf("Header: %v, Form: %v, flags: %v, type: %v, assets: %v, db_name: %v, scanSettings: %v", | ||
| r.Header, r.Form, flags, scanType, sbom, dbName, scanSettings) |
There was a problem hiding this comment.
Avoid logging full headers/forms to prevent credential leakage.
r.Header and r.Form can contain auth tokens or cookies; tracing should log only whitelisted fields or redacted values.
Suggested redacted logging
- zs.Debugf("Header: %v, Form: %v, flags: %v, type: %v, assets: %v, db_name: %v, scanSettings: %v",
- r.Header, r.Form, flags, scanType, sbom, dbName, scanSettings)
+ zs.Debugw("Scan request params",
+ "flags", flags,
+ "type", scanType,
+ "assets_len", len(sbom),
+ "db_name", dbName,
+ "scanSettings_present", len(scanSettings) > 0,
+ )🤖 Prompt for AI Agents
In `@pkg/service/scanning_service.go` around lines 169 - 191, In
getConfigFromRequest replace the debug log that prints r.Header and r.Form with
a redacted/whitelist-only log: update the zs.Debugf call in function
getConfigFromRequest to omit r.Header and r.Form entirely and only log the safe,
trimmed variables (flags, scanType, sbom, dbName, scanSettings) and optionally a
small whitelist of specific headers (e.g., "Authorization" excluded; only
include header names you explicitly permit) or indicate "<redacted>" for
headers; ensure you modify the zs.Debugf invocation so it no longer interpolates
r.Header or r.Form to avoid leaking credentials.
| # Validate MD5 format (32 hexadecimal characters) | ||
| if [[ ! "$md5" =~ ^[a-fA-F0-9]{32}$ ]]; then | ||
| echo "Error: Invalid MD5 hash format: $md5" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Error message should go to stderr for consistency.
Other error messages in this script (lines 63, 68) redirect to stderr with >&2. This error message should follow the same pattern.
Proposed fix
# Validate MD5 format (32 hexadecimal characters)
if [[ ! "$md5" =~ ^[a-fA-F0-9]{32}$ ]]; then
- echo "Error: Invalid MD5 hash format: $md5"
+ echo "Error: Invalid MD5 hash format: $md5" >&2
exit 1
fi🤖 Prompt for AI Agents
In `@test-support/scanoss.sh` around lines 30 - 34, The MD5 validation block that
checks the variable md5 currently prints the error with echo to stdout; update
the if block that tests [[ ! "$md5" =~ ^[a-fA-F0-9]{32}$ ]] so the error message
for "Error: Invalid MD5 hash format: $md5" is written to stderr (use the same
redirection pattern as other errors in this script, e.g., append >&2) and then
exit 1; ensure only the echo destination changes, leaving the condition and exit
intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@go.mod`:
- Around line 14-22: The go.mod pins OTLP exporter modules
(go.opentelemetry.io/otel/exporters/otlp/otlptrace,
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc,
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc) to v1.38.0
while core otel packages are v1.39.0; either bump these three exporter module
versions to v1.39.0 in go.mod for consistency, or add a short comment in the
file/PR confirming that keeping v1.38.0 is intentional for compatibility (and
document which incompatibility requires the pin). Ensure you update the module
lines for otlptrace, otlptracegrpc, and otlpmetricgrpc or add the confirmation
comment accordingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod
🔇 Additional comments (2)
go.mod (2)
23-23: LGTM!The zap logger patch update from v1.27.0 to v1.27.1 is a safe change.
37-37: No action required. Security verification confirms all critical indirect dependencies are in patched versions:
- google.golang.org/grpc v1.78.0: No CVEs; CVE-2023-44487 fixed in v1.56.3+
- google.golang.org/protobuf v1.36.11: No CVEs; prior vulnerabilities fixed in earlier versions
- golang.org/x/net v0.47.0: Exceeds minimum patched versions for CVE-2025-22870 (v0.36.0) and CVE-2025-22872 (v0.38.0)
- golang.org/x/sys v0.39.0: No active security advisories
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.