feat: use Vulnerability Service API #391
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a new CLI tool called Changes
Sequence Diagram(s)High-Level CLI Command Flow (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client.go (1)
111-128: Remove redundant nil check for httpc.The nil check for
httpcon lines 124-126 is redundant since line 115 already initializes it with a default client. This check will only be true if an option explicitly sets it to nil, which would be unusual.func New(opts ...Option) (*Client, error) { c := &Client{ baseURL: DefaultBaseURL, userAgent: UserAgent, httpc: retryablehttp.NewClient(retryablehttp.DefaultOptionsSingle), // Default retryablehttp client } for _, opt := range opts { opt(c) } if c.apiKey == "" { return nil, ErrAPIKeyRequired } - // If a custom httpc was not provided, set the default retryablehttp client - if c.httpc == nil { - c.httpc = retryablehttp.NewClient(retryablehttp.DefaultOptionsSingle) - } return c, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.cursorrules(1 hunks).gitignore(1 hunks)client.go(1 hunks)types.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
client.go (1)
types.go (5)
SearchParams(211-222)SearchResponse(225-230)GetByIDParams(233-235)VulnerabilityResponse(238-240)VulnerabilityFilter(243-252)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test Builds (windows-latest, 1.21.x)
- GitHub Check: Test Builds (ubuntu-latest, 1.21.x)
- GitHub Check: Test Builds (macOS-latest, 1.21.x)
- GitHub Check: release-test
🔇 Additional comments (3)
.gitignore (1)
24-24: LGTM!Adding
/distto gitignore is appropriate for excluding build artifacts and generated files like the OpenAPI specification mentioned in the documentation.types.go (1)
8-109: Well-structured type definition!The flat composition approach for the
Vulnerabilitystruct is a good design choice. It simplifies JSON marshaling/unmarshaling and avoids nested complexity. The consistent use of pointers for optional fields andomitemptytags is appropriate..cursorrules (1)
1-239: Excellent project documentation!The
.cursorrulesfile provides comprehensive guidelines covering all aspects of the project from development workflows to code conventions. This will be invaluable for maintaining consistency across the codebase.
…cs (#392) * Initial commit of modified files from installation * Add vulnsh CLI documentation and comprehensive review document Co-authored-by: tarun <tarun@projectdiscovery.io> * Add vulnsh CLI with auth, version, and health check features Co-authored-by: tarun <tarun@projectdiscovery.io> * Refactor README, add USAGE.md, and improve search command documentation Co-authored-by: tarun <tarun@projectdiscovery.io> * fix: GitHub Actions failures - lint errors and integration tests - Fixed errcheck lint errors by properly ignoring defer closePager() errors - Renamed vulnsh_test.go to vulnsh_cases.go to fix build issues - Updated integration test expectations to match actual command outputs - Fixed JSON output format expectations for vulnsh commands - All vulnsh integration tests now passing Changes: - pkg/utils/yaml_printer.go: Fixed errcheck for defer closePager() - cmd/vulnsh/clis/groupby.go: Fixed errcheck for defer closePager() - cmd/vulnsh/clis/groupbyhelp.go: Fixed errcheck for defer closePager() - cmd/vulnsh/clis/searchhelp.go: Fixed errcheck for defer closePager() - cmd/integration-test/vulnsh_cases.go: Updated test expectations - cmd/integration-test/main.go: Integration with vulnsh test cases * remove binaries + use new readme --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: tarun <tarun@projectdiscovery.io>
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
client.go (1)
151-161: Add PDCP_API_KEY environment variable fallback.The function documentation states it checks both the credential store and the
PDCP_API_KEYenvironment variable, but the implementation only checks the credential store. Add the env var fallback as documented.func WithKeyFromEnv() Option { return func(c *Client) { pch := pdcp.PDCPCredHandler{} if creds, err := pch.GetCreds(); err == nil { c.apiKey = creds.APIKey return } + // Fallback to environment variable + if apiKey := os.Getenv("PDCP_API_KEY"); apiKey != "" { + c.apiKey = apiKey + } } }Don't forget to add
"os"to the imports.
🧹 Nitpick comments (20)
pkg/utils/pager.go (2)
11-13: Remove unused constant or document its intended usage.The
pageBreakconstant is defined but never used within this file. Consider removing it if it's not needed, or add a comment explaining its intended usage for consumers of this package.-// pageBreak is an ASCII Form-Feed (0x0c) that most pagers (e.g. `less`) treat -// as a "clear-screen" marker, giving the user a visual hard page boundary. -const pageBreak = "\f"
55-59: Consider adding error handling for cleanup function.The cleanup function currently ignores the error from
w.Close()and only returns the error fromcmd.Wait(). This could mask important cleanup errors.Consider capturing both errors:
cleanup := func() error { - _ = w.Close() - return cmd.Wait() + closeErr := w.Close() + waitErr := cmd.Wait() + if closeErr != nil { + return closeErr + } + return waitErr }pkg/utils/utils.go (1)
17-26: Well-designed utility functions with clear separation of concerns.The functions properly delegate to
PrintColorYAMLTowhile providing convenient interfaces for different output scenarios. The distinction between colorable stdout (with pager support) and direct stdout is valuable for different use cases.Consider using generics instead of
interface{}for better type safety:-func PrintColorYAML(v interface{}, header ...string) error { +func PrintColorYAML[T any](v T, header ...string) error { return PrintColorYAMLTo(colorable.NewColorableStdout(), v, header...) } -func PrintColorYAMLNoPager(v interface{}, header ...string) error { +func PrintColorYAMLNoPager[T any](v T, header ...string) error { return PrintColorYAMLTo(os.Stdout, v, header...) }cmd/integration-test/vulnsh_cases.go (1)
42-66: Consider simplifying command execution for better security and maintainability.The current approach uses
bash -cwhich adds unnecessary complexity and potential security concerns.Consider this simpler approach:
-func runVulnshBinaryAndGetResults(vulnshBinary string, debug bool, args []string) ([]string, error) { - cmd := exec.Command("bash", "-c") - cmdLine := fmt.Sprintf(`./%s `, vulnshBinary) - cmdLine += strings.Join(args, " ") - if debug { - if err := os.Setenv("DEBUG", "1"); err != nil { - gologger.Error().Msgf("Failed to set DEBUG env: %s", err) - } - cmd.Stderr = os.Stderr - } - cmd.Args = append(cmd.Args, cmdLine) +func runVulnshBinaryAndGetResults(vulnshBinary string, debug bool, args []string) ([]string, error) { + cmd := exec.Command("./"+vulnshBinary, args...) + if debug { + if err := os.Setenv("DEBUG", "1"); err != nil { + gologger.Error().Msgf("Failed to set DEBUG env: %s", err) + } + cmd.Stderr = os.Stderr + }This approach is more secure and straightforward.
cmd/vulnsh/clis/search.go (2)
99-101: Consider documenting the term facet normalization behavior.The in-place modification of
params.TermFacetsto replace "=" with ":" could be unexpected for callers who might reuse the slice. Consider either documenting this behavior clearly or creating a new slice:if len(searchTermFacets) > 0 { - params.TermFacets = searchTermFacets - for i, facet := range params.TermFacets { - if strings.Contains(facet, "=") { - params.TermFacets[i] = strings.ReplaceAll(facet, "=", ":") - } - } + params.TermFacets = make([]string, len(searchTermFacets)) + for i, facet := range searchTermFacets { + params.TermFacets[i] = strings.ReplaceAll(facet, "=", ":") + } }
163-168: Simplify stdout write error handling.The error handling for stdout writes is overly verbose. Consider consolidating:
-// Print to stdout -if _, err := os.Stdout.Write(jsonBytes); err != nil { - gologger.Error().Msgf("Failed to write JSON output: %s", err) -} -if _, err := os.Stdout.Write([]byte("\n")); err != nil { - gologger.Error().Msgf("Failed to write newline: %s", err) -} +// Print to stdout +if _, err := fmt.Fprintln(os.Stdout, string(jsonBytes)); err != nil { + gologger.Error().Msgf("Failed to write JSON output: %s", err) +}pkg/tools/groupby/groupby.go (2)
76-89: Simplify facet size capping logic.The current implementation has nested logic for parsing and capping facet sizes. Consider extracting this into a helper function for clarity:
+// parseFacetField parses a facet field expression and caps the size if needed +func parseFacetField(field string, maxSize int) string { + parts := strings.SplitN(field, "=", 2) + if len(parts) != 2 { + return field + } + + size, err := strconv.Atoi(parts[1]) + if err != nil { + return field + } + + if size > maxSize { + size = maxSize + } + return parts[0] + "=" + strconv.Itoa(size) +} // 2. Inspect each field expression for explicit size overrides (field=size). // If present and size > 200, cap it. cappedFields := make([]string, len(params.Fields)) for i, f := range params.Fields { - if strings.Contains(f, "=") { - parts := strings.SplitN(f, "=", 2) - if len(parts) == 2 { - if sz, err := strconv.Atoi(parts[1]); err == nil { - if sz > 200 { - sz = 200 - } - f = parts[0] + "=" + strconv.Itoa(sz) - } - } - } - cappedFields[i] = f + cappedFields[i] = parseFacetField(f, 200) }
109-109: Shorten tool description and move details to documentation.The tool description is verbose and includes implementation details. Consider a more concise description:
-mcp.WithDescription("Aggregate vulnerabilities (GROUP BY/facets) over selected fields. NOTE: Use this tool ONLY when instructed by `agent_vulnx` or when the user explicitly asks for a group-by; do NOT call it otherwise."), +mcp.WithDescription("Aggregate vulnerabilities by specified fields using facets. Use only when explicitly requested."),cmd/vulnsh/clis/groupby.go (1)
133-168: Improve facet rendering robustness.The current implementation has multiple type assertions that could fail silently. Consider adding error handling and simplifying the logic:
// Attempt to coerce into expected structure facetMap, ok := facetAny.(map[string]any) if !ok { + gologger.Warning().Msgf("Unexpected facet structure for field %s", facetName) continue }Also consider extracting the bucket rendering logic into a separate function for better maintainability.
USAGE.md (1)
398-398: Fix compound adjective hyphenation.-- **Rate limiting awareness** - Add delays between bulk operations +- **Rate-limiting awareness** - Add delays between bulk operationscmd/vulnsh/clis/groupbyhelp.go (1)
111-111: Add import or define the referenced type.The comment references
nopWriteCloserfromsearchhelp.go, but this creates an implicit dependency. Consider either importing the type or documenting the dependency more clearly.README.md (2)
172-174: Add language specification to fenced code block.The fenced code block is missing a language specification as flagged by static analysis.
-``` +```text Error: api key is required → Run: vulnsh auth--- `178-183`: **Add language specification to fenced code block.** The fenced code block is missing a language specification as flagged by static analysis. ```diff -``` +```text Error: api key is required (when running vulnsh search --help) → This is a known limitation. Either: 1. Set up API key first: vulnsh auth 2. Use this documentation for command helpcmd/vulnsh/clis/searchhelp.go (1)
21-133: Consider refactoring the long Run function.The Run function is quite long and handles multiple concerns. Consider extracting the help generation logic into separate functions for better maintainability.
Consider extracting functions like:
generateOverview()generateFilterTable(w, filters)generateFieldDetails(w, filters)pkg/tools/search/search.go (1)
93-93: Consider extracting default fields as a package constant.The default fields list is defined inline within the method. Consider extracting it as a package-level constant for better maintainability and potential reuse.
Add this constant at the package level:
var defaultSearchFields = []string{ "cve_id", "name", "remediation", "cve_created_at", "poc_count", "doc_type", "updated_at", "impact", "description", "severity", "cvss_score", "epss_score", "is_kev", "is_vkev", "is_oss", "is_patch_available", "is_poc", "is_remote", }Then use it in the handler:
- defaultFields := []string{"cve_id", "name", "remediation", "cve_created_at", "poc_count", "doc_type", "updated_at", "impact", "description", "severity", "cvss_score", "epss_score", "is_kev", "is_vkev", "is_oss", "is_patch_available", "is_poc", "is_remote"} + defaultFields := defaultSearchFieldscmd/vulnsh/clis/common.go (2)
204-209: Use gologger's formatting capabilities directly.Instead of building a string with
strings.Builder, use gologger's built-in formatting for cleaner code.- dump, err := httputil.DumpRequestOut(req, true) - if err == nil { - var sb strings.Builder - sb.WriteString("--- HTTP REQUEST ---\n") - sb.Write(dump) - sb.WriteString("--------------------\n") - gologger.Debug().MsgFunc(sb.String) - } + dump, err := httputil.DumpRequestOut(req, true) + if err == nil { + gologger.Debug().Msgf("--- HTTP REQUEST ---\n%s\n--------------------", dump) + }Apply the same pattern to the response debug handler at lines 214-221.
101-103: Simplify error handling for SSE not implemented response.The error handling can be simplified by ignoring the write error or logging it separately.
- if _, err := w.Write([]byte("SSE mode is not yet implemented in this build. Please update the MCP server package or implement SSE handler.")); err != nil { - gologger.Error().Msgf("Failed to write SSE not implemented message: %s", err) - } + _, _ = w.Write([]byte("SSE mode is not yet implemented in this build. Please update the MCP server package or implement SSE handler."))pkg/tools/agentvulnx/agentvulnx.go (1)
73-126: Consider extracting the prompt template for better maintainability.The large embedded prompt template makes the code harder to read and maintain. Consider extracting it to a separate file (e.g.,
prompt.txt) and embedding it withgo:embed, or at least move it to a package-level constant.Option 1: Use go:embed directive
//go:embed prompt.txt var promptTemplate stringOption 2: Extract to a constant at the top of the file
const agentVulnxPromptTemplate = ` Execute a vulnerability analysis using the LOOP framework... `pkg/tools/templates/prompt.go (2)
125-143: Consider making the output format more flexible.The mandatory output template is very specific and might not fit all vulnerability analysis scenarios. Consider allowing some flexibility in the format while maintaining structure.
544-559: Consider using raw string literals for cleaner markdown templates.The string concatenation with backticks makes the code harder to read. Consider using Go's raw string literals for better readability.
Instead of concatenating strings with backticks, use raw string literals:
-### Approved Query -` + "`<Bleve query>`" + ` +### Approved Query +` + "`" + `<Bleve query>` + "`" + `Or better yet, consider using a template string with proper escaping or a templating library for complex markdown generation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
.gitignore(1 hunks)Makefile(1 hunks)README.md(1 hunks)USAGE.md(1 hunks)client.go(1 hunks)cmd/integration-test/main.go(3 hunks)cmd/integration-test/run.sh(1 hunks)cmd/integration-test/server.go(2 hunks)cmd/integration-test/vulnsh_cases.go(1 hunks)cmd/vulnsh/clis/auth.go(1 hunks)cmd/vulnsh/clis/banner.txt(1 hunks)cmd/vulnsh/clis/common.go(1 hunks)cmd/vulnsh/clis/groupby.go(1 hunks)cmd/vulnsh/clis/groupbyhelp.go(1 hunks)cmd/vulnsh/clis/healthcheck.go(1 hunks)cmd/vulnsh/clis/id.go(1 hunks)cmd/vulnsh/clis/search.go(1 hunks)cmd/vulnsh/clis/searchhelp.go(1 hunks)cmd/vulnsh/clis/version.go(1 hunks)cmd/vulnsh/main.go(1 hunks)go.mod(5 hunks)pkg/runner/runner.go(14 hunks)pkg/runner/util.go(3 hunks)pkg/service/cvemap.go(4 hunks)pkg/testutils/util.go(2 hunks)pkg/tools/agentvulnx/agentvulnx.go(1 hunks)pkg/tools/filters/filters.go(1 hunks)pkg/tools/groupby/groupby.go(1 hunks)pkg/tools/id/id.go(1 hunks)pkg/tools/search/search.go(1 hunks)pkg/tools/templates/prompt.go(1 hunks)pkg/tools/tools.go(1 hunks)pkg/utils/pager.go(1 hunks)pkg/utils/utils.go(1 hunks)pkg/utils/yaml_printer.go(1 hunks)types.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- cmd/vulnsh/clis/banner.txt
- cmd/vulnsh/main.go
- pkg/service/cvemap.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧬 Code Graph Analysis (10)
cmd/integration-test/main.go (2)
cmd/integration-test/server.go (1)
SetupMockServer(15-33)cmd/vulnsh/clis/common.go (1)
Execute(162-166)
pkg/utils/utils.go (1)
pkg/utils/yaml_printer.go (1)
PrintColorYAMLTo(20-79)
cmd/vulnsh/clis/search.go (4)
types.go (2)
SearchParams(211-222)Ptr(256-258)pkg/tools/search/search.go (1)
NewHandler(32-34)client.go (1)
ErrNotFound(64-64)pkg/utils/yaml_printer.go (1)
PrintYaml(85-126)
cmd/vulnsh/clis/healthcheck.go (1)
types.go (1)
VulnerabilityFilter(243-252)
pkg/tools/id/id.go (2)
pkg/tools/agentvulnx/agentvulnx.go (2)
NewHandler(45-47)Handler(40-42)types.go (1)
Vulnerability(11-109)
cmd/vulnsh/clis/id.go (3)
pkg/tools/id/id.go (1)
NewHandler(16-20)client.go (1)
ErrNotFound(64-64)pkg/utils/utils.go (2)
PrintColorYAMLNoPager(24-26)PrintColorYAML(19-21)
cmd/vulnsh/clis/searchhelp.go (2)
pkg/utils/pager.go (1)
OpenPager(22-60)pkg/tools/filters/filters.go (1)
NewHandler(24-26)
pkg/tools/agentvulnx/agentvulnx.go (6)
client.go (1)
Client(94-102)pkg/tools/filters/filters.go (2)
NewHandler(24-26)Handler(17-19)pkg/tools/groupby/groupby.go (2)
NewHandler(56-58)Handler(50-52)pkg/tools/id/id.go (2)
NewHandler(16-20)Handler(11-13)pkg/tools/search/search.go (2)
NewHandler(32-34)Handler(26-28)pkg/tools/templates/prompt.go (2)
NewHandler(25-27)Handler(20-22)
cmd/vulnsh/clis/common.go (2)
client.go (7)
Client(94-102)Option(89-89)WithKeyFromEnv(153-161)WithRetryableHTTPOptions(173-177)WithDebugRequest(180-184)WithDebugResponse(187-191)New(115-132)pkg/tools/tools.go (1)
AllMCPTools(24-32)
pkg/tools/templates/prompt.go (6)
client.go (1)
Client(94-102)pkg/tools/agentvulnx/agentvulnx.go (2)
NewHandler(45-47)Handler(40-42)pkg/tools/filters/filters.go (2)
NewHandler(24-26)Handler(17-19)pkg/tools/groupby/groupby.go (3)
NewHandler(56-58)Handler(50-52)Params(28-42)pkg/tools/id/id.go (2)
NewHandler(16-20)Handler(11-13)pkg/tools/search/search.go (2)
NewHandler(32-34)Handler(26-28)
🪛 markdownlint-cli2 (0.17.2)
README.md
172-172: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 LanguageTool
USAGE.md
[uncategorized] ~398-~398: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...Check exit codes and JSON structure - Rate limiting awareness - Add delays between bulk o...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Cursor BugBot
- GitHub Check: Test Builds (windows-latest, 1.21.x)
- GitHub Check: Test Builds (ubuntu-latest, 1.21.x)
🔇 Additional comments (47)
Makefile (1)
17-18: LGTM! Clean build target implementation.The new
build-vulnshtarget follows the same pattern as the existingbuildtarget and properly sets the version information via ldflags.pkg/testutils/util.go (1)
18-20: Good improvement in error handling.The addition of proper error handling for
os.Setenvimproves observability during test execution. Previously, failures to set the DEBUG environment variable would be silently ignored.cmd/integration-test/server.go (1)
72-76: Good improvement in resource cleanup error handling.The enhanced deferred file closure with proper error handling improves observability and ensures that file close errors are not silently ignored, which is important for robust integration testing.
pkg/runner/util.go (2)
27-29: Good improvement in network resource cleanup.The addition of proper error handling for network connection closures improves observability during health checks and ensures that connection close errors are not silently ignored.
39-41: Consistent error handling for UDP connection closure.The UDP connection closure follows the same improved error handling pattern as the TCP connection, maintaining consistency across the codebase.
cmd/integration-test/run.sh (1)
18-24: LGTM! Well-structured addition of vulnsh binary support.The changes correctly extend the build process to include the new
vulnshbinary alongsidecvemap, with proper GitHub Actions grouping for clear CI logs. The updated test invocation properly passes both binaries to the integration test runner.cmd/integration-test/main.go (3)
21-21: LGTM! Good addition of vulnsh binary flag.The new flag follows the same pattern as the existing
currentCvemapBinaryflag and provides the necessary parameter for running vulnsh integration tests.
27-32: Excellent improvement in error handling for environment variables.Adding proper error handling with
gologger.Error()for environment variable setting is a significant improvement over silent failures. This will help diagnose configuration issues during testing.
72-72: No action needed:vulnshTestCasesis defined
ThevulnshTestCasesvariable is declared incmd/integration-test/vulnsh_cases.go, so the reference inmain.gois valid.pkg/utils/utils.go (1)
11-15: LGTM! Clean ANSI escape sequence handling.The escape constant and format function provide a clean abstraction for ANSI color codes. This approach is more readable than inline escape sequences.
pkg/runner/runner.go (4)
118-118: Good improvement usingstrings.ReplaceAll.Replacing
strings.Replace(fmt.Sprintf(...), " ,", "", -1)withstrings.ReplaceAllis more idiomatic and clearer in intent.
366-370: Excellent improvement in resource cleanup error handling.Wrapping the deferred
keyboard.Close()in an anonymous function that logs errors is a best practice. This prevents silent failures during resource cleanup while still allowing the program to continue.
559-563: Consistent and proper file closing error handling.The deferred file close with error logging follows the same improved pattern as the keyboard cleanup. This ensures file descriptor leaks are avoided and any closing errors are properly logged.
587-594: Better readability with switch statement.Converting the character comparison from if-else chain to a switch statement improves readability and is more idiomatic Go code for this type of character-based branching logic.
cmd/vulnsh/clis/auth.go (2)
84-86: Good success feedback and credential management.The success messages provide clear feedback to the user about the authentication status and credential storage. The use of the validated credentials' username in the success message is a nice touch.
73-73: Service identifier “cvemap” is correct for nowThe code consistently uses
"cvemap"across the repo (see runner.go, banner.go, version.go, Makefile) because the backend currently only recognizes that identifier. There’s already a TODO incmd/vulnsh/clis/version.goto switch to"vulnsh"once server-side support is added. No change is needed at this time.pkg/tools/filters/filters.go (1)
1-56: LGTM! Clean MCP tool implementation with proper error handling.The implementation follows consistent patterns with other tools in the project and includes:
- Clear documentation and usage constraints
- Proper error handling in the MCP handler
- Appropriate use of context.Background() for API calls
- Consistent JSON marshaling and response formatting
pkg/tools/tools.go (1)
1-37: LGTM! Clean abstraction for MCP tool management.The implementation provides:
- Well-defined MCPTool interface with required methods
- Central registry pattern for tool management
- Good separation of concerns with individual tool packages
- Future-ready with commented AllMCPPrompts function
pkg/tools/id/id.go (1)
1-60: LGTM! Solid MCP tool implementation with proper validation.The implementation demonstrates:
- Consistent design patterns with other tools
- Proper parameter validation in MCP handler
- Good error handling and response formatting
- Clear documentation about tool usage constraints
- Appropriate use of context.Background() for API calls
cmd/integration-test/vulnsh_cases.go (1)
68-95: LGTM! Comprehensive test coverage for vulnsh commands.The test cases provide good coverage across different command scenarios:
- ID lookup with JSON output
- Search functionality with filters
- Health check validation
- Help command verification
- Groupby functionality
cmd/vulnsh/clis/id.go (3)
20-46: LGTM! Well-documented CLI command with comprehensive examples.The command definition includes:
- Clear usage instructions and examples
- Proper argument validation with cobra.ExactArgs(1)
- Good documentation of global flags
- Comprehensive help text
108-128: LGTM! Thorough input validation with appropriate constraints.The validation function properly checks:
- Empty vulnerability ID
- Reasonable length constraints (3-50 characters)
- Output file extension requirement (.json)
- Clear error messages for each validation failure
64-82: LGTM! Robust file handling with proper error checking.The file output implementation demonstrates excellent practices:
- File existence check before creation
- Use of O_CREATE|O_EXCL|O_WRONLY flags to prevent overwriting
- Proper defer for file closing with error handling
- Clear error messages for each failure scenario
cmd/vulnsh/clis/version.go (3)
10-11: LGTM! Good documentation of build-time version handling.The TODO comment clearly indicates that the version should be set via ldflags during build, which is a standard practice for Go applications.
54-56: Verify the temporary cvemap usage is properly documented.The temporary usage of "cvemap" for version checking is well-documented with TODO comments. Ensure this is tracked as a known limitation until server-side support for "vulnsh" is implemented.
34-40: Good implementation of command-specific PreRunE override.The override prevents unnecessary client initialization for the version command, which is appropriate since version information doesn't require API access.
pkg/utils/yaml_printer.go (3)
20-28: LGTM! Good YAML marshaling configuration.The YAML marshaling options with indentation and literal style for multiline strings provide clean, readable output.
59-64: Excellent Windows compatibility handling.The colorable output handling ensures proper color support across different platforms, including Windows.
86-86: OpenPager is defined in pkg/utils/pager.go; no import needed.The
OpenPagerfunction is implemented inpkg/utils/pager.go(sameutilspackage), soyaml_printer.gocan call it directly without any import. You can safely ignore this import verification.Likely an incorrect or invalid review comment.
README.md (2)
1-3: Excellent rebranding and positioning.The new title and description effectively position vulnsh as a comprehensive vulnerability intelligence tool.
11-25: Great quick start section.The quick start provides a clear, actionable path for new users to get started with vulnsh.
cmd/vulnsh/clis/healthcheck.go (4)
57-97: Excellent comprehensive health check implementation.The health check covers all essential aspects: authentication, connectivity, and endpoint functionality. The early exit on authentication failure is a good optimization.
159-162: Good performance threshold for response time.The 10-second threshold for marking slow responses as warnings is reasonable and provides useful feedback to users.
214-223: Simple and effective helper function.The
getSampleFieldsfunction provides a clean way to extract sample field names for display purposes.
225-262: Comprehensive result display with good UX.The result display includes status icons, timing information, and conditional verbose details, providing excellent user experience.
cmd/vulnsh/clis/searchhelp.go (4)
23-27: Good defensive programming with client initialization check.The defensive check for client initialization ensures the command works correctly even if called independently.
54-61: Excellent pager integration with fallback handling.The pager setup with graceful fallback to stdout and the nopWriteCloser wrapper provides a smooth user experience across different environments.
89-106: Well-configured table formatting for readability.The table configuration with width limits and rounded styling provides excellent readability for the filter information.
144-149: Clean abstraction with nopWriteCloser.The nopWriteCloser type provides a clean way to handle both pager and direct output scenarios uniformly.
types.go (1)
1-258: Well-structured type definitions!The type definitions are comprehensive and well-documented. The flat structure for
Vulnerabilityis clearly explained in the comments, and all fields appropriately useomitemptytags. The genericPtrhelper function is a nice addition for working with optional fields.pkg/tools/templates/prompt.go (7)
1-9: LGTM!Package declaration and imports are appropriate and correctly organized.
11-27: Well-structured interface and handler design.The
PromptTemplateinterface andHandlerstruct follow consistent patterns with other tool handlers in the codebase. The design is clean and extensible.
29-39: Good factory pattern implementation.The function efficiently creates all prompt templates with a shared handler instance, promoting resource efficiency and maintaining clean separation of concerns.
187-304: Well-structured threat intelligence template.The implementation provides comprehensive guidance for threat intelligence analysis with clear phases and practical query examples. The focus on exploitation patterns and KEV tracking is particularly valuable.
306-417: Excellent security research template design.The implementation effectively guides security researchers with well-defined complexity levels and comprehensive methodologies. The phased approach from discovery to exploitation is particularly well-structured.
419-496: Excellent handling of general queries with ethical considerations.The implementation effectively transforms vague or non-technical queries into actionable steps while maintaining ethical boundaries. The inclusion of ethical disclaimers for exploitation requests is commendable.
583-592: Good defensive helper function.The
getStringArghelper properly handles nil maps and missing keys with sensible defaults.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
pkg/tools/groupby/groupby.go (2)
64-64: Add context parameter to GroupBy method.The GroupBy method should accept a context parameter for proper cancellation and timeout handling, consistent with the underlying API call pattern.
-func (h *Handler) GroupBy(params Params) (cvemap.SearchResponse, error) { +func (h *Handler) GroupBy(ctx context.Context, params Params) (cvemap.SearchResponse, error) {
103-103: Use the provided context for API calls.The method should use the context parameter that will be added to the method signature instead of creating a new background context.
- return h.client.SearchVulnerabilities(context.Background(), sp) + return h.client.SearchVulnerabilities(ctx, sp)
🧹 Nitpick comments (1)
pkg/tools/groupby/groupby.go (1)
75-89: Simplify field processing logic.The field processing logic can be simplified and made more readable by extracting the size parsing and capping into a helper function.
- // 2. Inspect each field expression for explicit size overrides (field=size). - // If present and size > 200, cap it. - cappedFields := make([]string, len(params.Fields)) - for i, f := range params.Fields { - if strings.Contains(f, "=") { - parts := strings.SplitN(f, "=", 2) - if len(parts) == 2 { - if sz, err := strconv.Atoi(parts[1]); err == nil { - if sz > 200 { - sz = 200 - } - f = parts[0] + "=" + strconv.Itoa(sz) - } - } - } - cappedFields[i] = strings.ReplaceAll(f, "=", ":") - } + // 2. Process field expressions: cap size overrides and convert format + cappedFields := make([]string, len(params.Fields)) + for i, f := range params.Fields { + cappedFields[i] = h.processFieldExpression(f) + }And add the helper method:
// processFieldExpression caps size overrides and converts "=" to ":" format func (h *Handler) processFieldExpression(field string) string { if strings.Contains(field, "=") { parts := strings.SplitN(field, "=", 2) if len(parts) == 2 { if sz, err := strconv.Atoi(parts[1]); err == nil { if sz > 200 { sz = 200 } field = parts[0] + "=" + strconv.Itoa(sz) } } } return strings.ReplaceAll(field, "=", ":") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/tools/groupby/groupby.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/tools/groupby/groupby.go (7)
client.go (1)
Client(94-102)pkg/tools/agentvulnx/agentvulnx.go (2)
NewHandler(45-47)Handler(40-42)pkg/tools/filters/filters.go (2)
NewHandler(24-26)Handler(17-19)pkg/tools/id/id.go (2)
NewHandler(16-20)Handler(11-13)pkg/tools/search/search.go (2)
NewHandler(32-34)Handler(26-28)pkg/tools/templates/prompt.go (2)
NewHandler(25-27)Handler(20-22)types.go (3)
SearchResponse(225-230)SearchParams(211-222)Ptr(256-258)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test Builds (macOS-latest, 1.21.x)
- GitHub Check: Test Builds (ubuntu-latest, 1.21.x)
- GitHub Check: Test Builds (windows-latest, 1.21.x)
- GitHub Check: Lint Test
🔇 Additional comments (5)
pkg/tools/groupby/groupby.go (5)
1-11: Package structure and imports look good.The package follows Go conventions with clear imports and proper organization.
13-42: Well-documented Params struct with clear field definitions.The struct documentation provides helpful examples and clearly explains the expected behavior and constraints.
44-58: Handler follows established patterns from other tools.The Handler struct and NewHandler constructor are consistent with the patterns used in other packages like
pkg/tools/searchandpkg/tools/filters.
106-122: MCP tool specification is comprehensive.The tool specification provides clear descriptions and proper parameter definitions for MCP integration.
156-161: Error handling and response formatting are appropriate.The JSON marshalling and error handling provide clear feedback with proper error messages.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cmd/vulnsh/clis/groupby.go (1)
192-193: Inconsistent facet size validation limit.The validation allows facet-size up to 1000, but based on past review comments, the groupby handler caps it at 200. This discrepancy could confuse users.
cmd/vulnsh/clis/groupbyhelp.go (1)
22-26: Centralize cvemapClient initialization.This initialization check pattern is duplicated across multiple command files. As mentioned in past review comments, consider moving this to a common
PersistentPreRunEfunction in the root command setup.
🧹 Nitpick comments (4)
cmd/vulnsh/clis/groupby.go (1)
64-64: Use consistent pointer utility function.The code correctly uses
cvemap.Ptr()but according to the relevant code snippets, there's also aPtrfunction defined intypes.go(lines 256-258). Consider using the local utility consistently across the codebase.- params.FacetSize = cvemap.Ptr(groupbyFacetSize) + params.FacetSize = Ptr(groupbyFacetSize)USAGE.md (1)
398-398: Fix compound adjective hyphenation.Use a hyphen for the compound adjective "rate-limiting" that modifies the following noun.
-- **Rate limiting awareness** - Add delays between bulk operations +- **Rate-limiting awareness** - Add delays between bulk operationsREADME.md (2)
172-175: Add language specification for code block.The fenced code block should specify a language for proper syntax highlighting.
-**API key issues:** -``` -Error: api key is required -→ Run: vulnsh auth -``` +**API key issues:** +```text +Error: api key is required +→ Run: vulnsh auth +```
178-183: Add language specification for code block.The fenced code block should specify a language for proper syntax highlighting.
-**Help commands requiring API key:** -``` -Error: api key is required (when running vulnsh search --help) -→ This is a known limitation. Either: - 1. Set up API key first: vulnsh auth - 2. Use this documentation for command help -``` +**Help commands requiring API key:** +```text +Error: api key is required (when running vulnsh search --help) +→ This is a known limitation. Either: + 1. Set up API key first: vulnsh auth + 2. Use this documentation for command help +``` </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between cf3dfa93343ad8d68c2da43206430328751a5ec1 and 4b662432d1c547aae36bb9ed6264f0c322678b69. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `README.md` (1 hunks) * `USAGE.md` (1 hunks) * `cmd/integration-test/vulnsh_cases.go` (1 hunks) * `cmd/vulnsh/clis/groupby.go` (1 hunks) * `cmd/vulnsh/clis/groupbyhelp.go` (1 hunks) * `pkg/tools/agentvulnx/agentvulnx.go` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * pkg/tools/agentvulnx/agentvulnx.go * cmd/integration-test/vulnsh_cases.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (2)</summary> <details> <summary>cmd/vulnsh/clis/groupbyhelp.go (1)</summary><blockquote> <details> <summary>pkg/utils/pager.go (1)</summary> * `OpenPager` (22-60) </details> </blockquote></details> <details> <summary>cmd/vulnsh/clis/groupby.go (4)</summary><blockquote> <details> <summary>pkg/tools/groupby/groupby.go (2)</summary> * `Params` (28-42) * `NewHandler` (56-58) </details> <details> <summary>types.go (1)</summary> * `Ptr` (256-258) </details> <details> <summary>client.go (1)</summary> * `ErrNotFound` (64-64) </details> <details> <summary>pkg/utils/pager.go (1)</summary> * `OpenPager` (22-60) </details> </blockquote></details> </details><details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>README.md</summary> 172-172: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 178-178: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 LanguageTool</summary> <details> <summary>USAGE.md</summary> [uncategorized] ~398-~398: If this is a compound adjective that modifies the following noun, use a hyphen. Context: ...Check exit codes and JSON structure - **Rate limiting awareness** - Add delays between bulk o... (EN_COMPOUND_ADJECTIVE_INTERNAL) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (4)</summary> * GitHub Check: Test Builds (ubuntu-latest, 1.21.x) * GitHub Check: Test Builds (macOS-latest, 1.21.x) * GitHub Check: Test Builds (windows-latest, 1.21.x) * GitHub Check: Lint Test </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>cmd/vulnsh/clis/groupby.go (1)</summary> `122-179`: **Well-implemented facet table rendering.** The facet rendering logic handles multiple data structures (map and slice formats) and includes proper sorting by count. The handling of missing values as "UNASSIGNED" provides good user experience. </details> <details> <summary>USAGE.md (1)</summary> `1-403`: **Excellent comprehensive documentation.** This advanced usage guide provides thorough coverage of vulnsh capabilities with practical examples, automation patterns, and troubleshooting guidance. The structured approach with quick reference, workflows, and best practices will significantly improve user experience. </details> <details> <summary>cmd/vulnsh/clis/groupbyhelp.go (1)</summary> `76-106`: **Dynamic field listing enhances user experience.** The implementation dynamically fetches and filters facet-capable fields from the API, providing users with current and accurate information about available fields. The table formatting with description width limits improves readability. </details> <details> <summary>README.md (1)</summary> `1-232`: **Outstanding documentation transformation.** The README has been excellently restructured to focus on the new vulnsh CLI tool with clear quick start instructions, comprehensive command examples, and practical troubleshooting guidance. The categorized approach with tables and code examples makes it highly accessible to users. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
pkg/tools/renderer/README.md (3)
165-165: Add hyphen to compound adjective.-- **LOW** - Green for low priority +- **LOW** - Green for low-priority items
173-173: Add missing article for clarity.-- Use `--no-color` flag to disable colors manually +- Use the `--no-color` flag to disable colors manually
177-177: Specify language for fenced code block.-``` +```textpkg/tools/renderer/renderer.go (1)
269-269: Remove ineffectual assignment.The
sectionsAddedvariable is assigned but never used again since this is the last section in the function.- sectionsAdded = truepkg/tools/renderer/helpers.go (2)
98-147: Consider additional exploit indicators for comprehensive detection.The
exploitSeenfunction checks for exploit indicators in POC sources, citation URLs, and descriptive text. Consider also checking:
- POC URLs for exploit-related domains (similar to citation URL checks)
- Tags in citations for exploit-related keywords
- H1 stats if available in the Entry struct
410-424: Consider handling edge cases informatNucleiTemplateURL.The function converts local template paths to ProjectDiscovery cloud URLs. However, it might benefit from additional validation:
- The function assumes
docIDis always valid for URL construction- Consider URL-encoding the
docIDto handle special charactersfunc formatNucleiTemplateURL(uri string, docID string) string { if uri == "" { return "" } // If it's already a full URL, return as is if strings.HasPrefix(uri, "http://") || strings.HasPrefix(uri, "https://") { return uri } + // Validate and sanitize docID + if docID == "" { + return uri + } + // Convert to cloud ProjectDiscovery URL using doc_id // Example: CVE-2025-5777 -> https://cloud.projectdiscovery.io/library/CVE-2025-5777 - return fmt.Sprintf("https://cloud.projectdiscovery.io/library/%s", docID) + return fmt.Sprintf("https://cloud.projectdiscovery.io/library/%s", url.QueryEscape(docID)) }Note: You'll need to import
net/urlfor theQueryEscapefunction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/vulnsh/clis/common.go(1 hunks)cmd/vulnsh/clis/id.go(1 hunks)cmd/vulnsh/clis/search.go(1 hunks)pkg/tools/renderer/README.md(1 hunks)pkg/tools/renderer/colors.go(1 hunks)pkg/tools/renderer/example_test.go(1 hunks)pkg/tools/renderer/helpers.go(1 hunks)pkg/tools/renderer/renderer.go(1 hunks)pkg/tools/renderer/renderer_test.go(1 hunks)pkg/tools/renderer/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/vulnsh/clis/search.go
- cmd/vulnsh/clis/common.go
- cmd/vulnsh/clis/id.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/tools/renderer/types.go (1)
types.go (7)
VulnExposure(156-160)ProductInfo(141-153)KevInfo(120-125)H1Stats(179-184)POC(128-132)Citation(112-117)Vulnerability(11-109)
pkg/tools/renderer/renderer_test.go (4)
pkg/tools/renderer/types.go (2)
Entry(15-39)LayoutLine(8-12)types.go (6)
VulnExposure(156-160)ProductInfo(141-153)KevInfo(120-125)H1Stats(179-184)POC(128-132)Citation(112-117)pkg/tools/renderer/renderer.go (3)
RenderWithColors(303-336)ParseLayout(339-345)RenderDetailed(16-300)pkg/tools/renderer/colors.go (1)
NoColorConfig(110-114)
pkg/tools/renderer/helpers.go (2)
types.go (3)
ProductInfo(141-153)POC(128-132)Citation(112-117)pkg/tools/renderer/types.go (1)
Entry(15-39)
🪛 golangci-lint (1.64.8)
pkg/tools/renderer/renderer.go
269-269: ineffectual assignment to sectionsAdded
(ineffassign)
pkg/tools/renderer/colors.go
346-346: func (*ColorConfig).colorRiskLevels is unused
(unused)
pkg/tools/renderer/helpers.go
150-150: func calculateRiskLevel is unused
(unused)
236-236: func getActionItems is unused
(unused)
🪛 GitHub Actions: 🔨 Build Test
pkg/tools/renderer/renderer_test.go
[error] 89-89: TestRender failed: Expected priority 'HIGH' but got 'IMMEDIATE' in vulnerability rendering output.
🪛 LanguageTool
pkg/tools/renderer/README.md
[uncategorized] ~165-~165: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ortant priorities - LOW - Green for low priority - EXPLOITS AVAILABLE - Bright yello...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~173-~173: You might be missing the article “the” here.
Context: ...on-terminal output (pipes, files) - Use --no-color flag to disable colors manua...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 markdownlint-cli2 (0.17.2)
pkg/tools/renderer/README.md
177-177: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: Lint Test
- GitHub Check: Cursor BugBot
🔇 Additional comments (4)
pkg/tools/renderer/example_test.go (1)
10-89: Excellent example test implementation!This example test effectively demonstrates the renderer usage with realistic vulnerability data and proper layout configuration. The code follows Go example test conventions and provides clear documentation for users.
pkg/tools/renderer/types.go (1)
7-74: Well-designed type definitions and conversion logic.The
LayoutLineandEntrystructs are well-defined with appropriate JSON tags. TheFromVulnerabilityfunction properly handles null checking and field mapping from thecvemap.Vulnerabilitytype, ensuring safe conversion for rendering purposes.pkg/tools/renderer/renderer.go (1)
10-336: Robust rendering implementation with good separation of concerns.The rendering functions provide comprehensive support for both detailed single-entry and multi-entry layout-based rendering. The color configuration integration and layout parsing are well-implemented.
pkg/tools/renderer/renderer_test.go (1)
93-492: Comprehensive test coverage for renderer functionality.The test suite provides excellent coverage of layout parsing, formatting functions, exploit detection, and detailed rendering. The test cases are well-structured and cover edge cases appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
pkg/tools/renderer/helpers.go (3)
98-147: Consider making exploit detection patterns configurable.The
exploitSeenfunction uses hardcoded keywords, domains, and phrases for exploit detection. This approach may miss evolving terminology or require updates as new exploit sources emerge.Consider extracting these patterns into a configuration structure:
+// ExploitDetectionConfig holds patterns for exploit detection +type ExploitDetectionConfig struct { + Keywords []string + Domains []string + Phrases []string +} + +// DefaultExploitDetectionConfig returns default patterns +func DefaultExploitDetectionConfig() *ExploitDetectionConfig { + return &ExploitDetectionConfig{ + Keywords: []string{"exploit", "exploiting", "exploitation", "exploitable"}, + Domains: []string{"exploit-db.com", "exploitdb.com", "metasploit.com"}, + Phrases: []string{"exploited in the wild", "actively exploited", "used in attacks", "real-world exploitation"}, + } +} -func exploitSeen(entry *Entry) bool { +func exploitSeen(entry *Entry, config *ExploitDetectionConfig) bool { - exploitKeywords := []string{"exploit", "exploiting", "exploitation", "exploitable"} + exploitKeywords := config.Keywords // ... rest of function uses config.Domains and config.Phrases
157-176: Consider making priority thresholds configurable.The
getResearchPriorityfunction uses hardcoded CVSS and EPSS score thresholds. These thresholds may need adjustment based on organizational risk tolerance or evolving threat landscape.Consider extracting thresholds into a configuration structure:
+// PriorityConfig holds thresholds for research priority calculation +type PriorityConfig struct { + UrgentCVSS float64 + UrgentEPSS float64 + HighCVSS float64 + MediumCVSS float64 + MediumEPSS float64 +} + +// DefaultPriorityConfig returns default thresholds +func DefaultPriorityConfig() *PriorityConfig { + return &PriorityConfig{ + UrgentCVSS: 9.0, + UrgentEPSS: 0.5, + HighCVSS: 7.0, + MediumCVSS: 7.0, + MediumEPSS: 0.3, + } +}
338-366: Review text wrapping logic for edge cases.The
wrapLongTextfunction handles word wrapping but may have edge cases with very long words or unusual spacing patterns.Consider adding validation for edge cases:
func wrapLongText(text string, maxWidth int, indent string) []string { + // Validate inputs + if maxWidth <= 0 { + return []string{text} + } + if indent == "" { + indent = " " // Default indentation + } + if len(text) <= maxWidth { return []string{text} } var lines []string words := strings.Fields(text) if len(words) == 0 { return []string{text} } currentLine := words[0] + // Handle case where first word is longer than maxWidth + if len(currentLine) > maxWidth { + return []string{text} // Return original text if word is too long + } for i := 1; i < len(words); i++ { if len(currentLine)+1+len(words[i]) <= maxWidth { currentLine += " " + words[i] } else { lines = append(lines, currentLine) currentLine = indent + words[i] + // Handle case where indented word is longer than maxWidth + if len(currentLine) > maxWidth { + lines = append(lines, currentLine) + currentLine = indent + } } }pkg/tools/renderer/colors.go (3)
117-120: Consider enhancing terminal detection for better cross-platform support.The
IsTerminalfunction uses a simple file mode check which may not work correctly on all platforms (e.g., Windows terminals, CI/CD environments).Consider using a more robust terminal detection approach:
+import ( + "os" + "runtime" +) func IsTerminal() bool { - fileInfo, _ := os.Stdout.Stat() - return (fileInfo.Mode() & os.ModeCharDevice) != 0 + // Check if NO_COLOR environment variable is set + if os.Getenv("NO_COLOR") != "" { + return false + } + + // Check if TERM environment variable indicates a terminal + term := os.Getenv("TERM") + if term == "" || term == "dumb" { + return false + } + + // Platform-specific checks + if runtime.GOOS == "windows" { + // Windows-specific terminal detection + return os.Getenv("MSYSTEM") != "" || os.Getenv("ConEmuANSI") == "ON" + } + + // Unix-like systems + fileInfo, err := os.Stdout.Stat() + if err != nil { + return false + } + return (fileInfo.Mode() & os.ModeCharDevice) != 0 }
401-512: Optimize string replacement operations for performance.The color pattern methods use multiple
strings.ReplaceAllcalls which can be inefficient for large text. Consider using a more efficient approach for multiple replacements.Consider using
strings.Replacerfor better performance:+import ( + "strings" +) func (c *ColorConfig) colorSpecificPatterns(text string) string { if !c.Enabled { return text } - result := text - // Color checkmarks - result = strings.ReplaceAll(result, "✔", c.ColorBoolean("✔")) - result = strings.ReplaceAll(result, "✘", c.ColorBoolean("✘")) + // Use strings.Replacer for better performance with multiple replacements + replacer := strings.NewReplacer( + "✔", c.ColorBoolean("✔"), + "✘", c.ColorBoolean("✘"), + ) + result := replacer.Replace(text) // Continue with other color operations... result = c.colorNumbers(result) result = c.colorAgeUrgency(result) result = c.colorMetrics(result) result = c.colorExposureValues(result) return result }
325-346: Consider extracting priority level patterns into a configuration.The
colorPriorityLevelsfunction has hardcoded priority level patterns that may need updates as the priority system evolves.Consider making priority patterns configurable:
+// PriorityColorConfig holds patterns for priority level coloring +type PriorityColorConfig struct { + Levels map[string]string // priority level -> color +} + +// DefaultPriorityColorConfig returns default priority colors +func DefaultPriorityColorConfig() *PriorityColorConfig { + return &PriorityColorConfig{ + Levels: map[string]string{ + "IMMEDIATE": Bold + BrightRed, + "URGENT": Bold + Red, + "HIGH": Bold + Yellow, + "MEDIUM": Bold + Yellow, + "LOW": Bold + Green, + }, + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/tools/renderer/colors.go(1 hunks)pkg/tools/renderer/example_test.go(1 hunks)pkg/tools/renderer/helpers.go(1 hunks)pkg/tools/renderer/renderer_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/tools/renderer/example_test.go
- pkg/tools/renderer/renderer_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/tools/renderer/helpers.go (2)
types.go (3)
ProductInfo(141-153)POC(128-132)Citation(112-117)pkg/tools/renderer/types.go (1)
Entry(15-39)
⏰ 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). (2)
- GitHub Check: Lint Test
- GitHub Check: Cursor BugBot
🔇 Additional comments (2)
pkg/tools/renderer/helpers.go (1)
1-366: Approve the overall implementation.The helper functions are well-structured and provide comprehensive formatting utilities for vulnerability data. The code follows Go conventions and handles most edge cases appropriately.
pkg/tools/renderer/colors.go (1)
1-512: Excellent colorization system implementation.The color configuration system is comprehensive and well-structured. It provides flexible color management with proper terminal detection and supports both colored and non-colored output modes. The semantic color mapping enhances readability of vulnerability reports.
Features: - Complete CLI renderer system with placeholder-based formatting - Security-focused color scheme with auto-detection - Risk assessment algorithm (CVSS/EPSS/KEV weighting) - Research prioritization for security professionals - KEV source attribution (CISA, VULCHECK, etc.) - Age urgency highlighting (NEW/RECENT vulnerabilities) - Enhanced exploit detection and vulnerability metrics - Smart omission rules and list truncation - Support for --no-color flag and terminal detection Code quality improvements: - Fixed import aliases for retryablehttp and isatty - Fixed test cases with proper trailing newlines - Removed dead YAML code and unused dependencies (4.3KB saved) - Comprehensive test suite with 100% coverage - Replaced legacy YAML output with modern CLI renderer Files changed: + pkg/tools/renderer/ (complete new package) * cmd/vulnsh/clis/search.go (CLI renderer integration) * cmd/vulnsh/clis/id.go (detailed single vulnerability view) * cmd/vulnsh/clis/common.go (--no-color flag + import fixes) * client.go, pkg/service/cvemap.go (import alias fixes) * go.mod (cleaned unused dependencies) - pkg/utils/yaml_printer.go, pkg/utils/utils.go (removed dead code)
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
pkg/utils/pager.go (2)
28-34: Improve pager binary validation logic.The function only checks for the existence of
lessbinary but doesn't validate if the pager specified in$PAGERenvironment variable actually exists. This could lead to runtime errors if a custom pager is specified but not available.This is the same issue identified in the previous review - the validation should check the actual pager command that will be used rather than just
less.
36-40: Move pager environment variable logic to avoid duplication.The
$PAGERenvironment variable is read after binary validation, but the validation logic should check the actual pager command that will be used.This connects to the previous comment - move this logic before the binary validation to ensure we're checking the correct pager command.
client.go (1)
151-161: Add PDCP_API_KEY environment variable fallback.The function documentation states it checks both the credential store and the
PDCP_API_KEYenvironment variable, but the implementation only checks the credential store. Add the env var fallback as documented.+import ( + "os" + // ... other imports +) func WithKeyFromEnv() Option { return func(c *Client) { pch := pdcp.PDCPCredHandler{} if creds, err := pch.GetCreds(); err == nil { c.apiKey = creds.APIKey return } + // Fallback to environment variable + if apiKey := os.Getenv("PDCP_API_KEY"); apiKey != "" { + c.apiKey = apiKey + } } }pkg/tools/renderer/renderer_test.go (1)
79-80: Test expectation appears to have been corrected.The test now correctly expects "IMMEDIATE" priority, which aligns with the test data conditions (high severity, KEV status, POCs available). This addresses the previously identified issue.
scripts/pre-commit.sh (1)
16-26: Duplicated printing helpersSee comment in
scripts/fix-dependencies.sh.
🧹 Nitpick comments (11)
pkg/utils/pager.go (1)
11-13: Remove unused constant or document its intended use.The
pageBreakconstant is defined but never used in the codebase. Either remove it or document its intended purpose if it's meant for future use.-// pageBreak is an ASCII Form-Feed (0x0c) that most pagers (e.g. `less`) treat -// as a "clear-screen" marker, giving the user a visual hard page boundary. -const pageBreak = "\f"cmd/vulnsh/clis/version.go (1)
54-56: Consider the user experience impact of using "cvemap" for version checking.While the comments explain the temporary nature, users might be confused seeing "cvemap" references when they're using the
vulnshtool.Consider adding a user-facing note in the version output to clarify this temporary behavior:
// Use cvemap name temporarily until server-side support is added for vulnsh // TODO: Change "cvemap" to "vulnsh" once server-side support is implemented + if verbose || debug { + gologger.Info().Msg("Note: Version checking temporarily uses cvemap registry") + } latestVersion, err := updateutils.GetToolVersionCallback("cvemap", Version)()Makefile (2)
38-44: Consider simplifying the lint target to address static analysis warning.The target body exceeds the recommended length according to checkmake. Consider extracting the logic to a separate script.
lint: @echo "🧐 Running linter..." - @if command -v golangci-lint >/dev/null 2>&1; then \ - golangci-lint run --timeout=5m; \ - else \ - echo "⚠️ golangci-lint not found, skipping linting"; \ - fi + @./scripts/run-lint.shThen create
scripts/run-lint.shwith the conditional logic.
72-72: Add missing clean target recommended by static analysis.The static analysis tool suggests adding a
cleantarget as a standard practice for Makefiles.+clean: + @echo "🧹 Cleaning build artifacts..." + @rm -f cvemap vulnsh + @$(GOCMD) clean -cache + -.PHONY: all build build-vulnsh integration tidy fmt vet test lint pre-push pre-commit git-hooks fix-deps +.PHONY: all build build-vulnsh integration tidy fmt vet test lint pre-push pre-commit git-hooks fix-deps cleanscripts/fix-dependencies.sh (3)
34-40: Usego clean -modcache -eto match “warn, don’t abort” semanticsThe
-eflag letsgocontinue even if some modules are unreachable, aligning with your warning-only intention.
64-70: Building every pkg is slow; allow a narrower scope
go build ./...recompiles the whole repo. Consider an env override (PKG=${PKG:-./...}) or switch togo test -cfor a quicker sanity check.
16-26: Printing helpers duplicated across scripts – centralise
print_status|warning|errorare duplicated here and inpre-commit.sh. Moving them toscripts/lib/colors.shandsource-ing will keep them DRY.scripts/pre-commit.sh (2)
35-43:gofmt -w .rewrites the entire repo, incl. vendored or generated codeLimit to staged Go files to avoid noisy diffs:
git diff --cached --name-only --diff-filter=AM | grep -E '\.go$' | xargs -r gofmt -w
71-89: Duplicategolangci-lintexecutionThe script runs the linter twice; capture stderr of the first run instead of a second pass to save time.
scripts/README.md (2)
44-52: Emoji inside Markdown table mis-aligns columns on GitHubGitHub treats
⚠️as two code points, breaking alignment. Use plain text or escape the emoji.
60-76: Minor grammar: add missing verb
Module cache out of sync→Module cache is out of sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.pre-commit-config.yamlis excluded by!**/*.yamlgo.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
Makefile(1 hunks)README.md(1 hunks)client.go(1 hunks)cmd/vulnsh/clis/auth.go(1 hunks)cmd/vulnsh/clis/common.go(1 hunks)cmd/vulnsh/clis/healthcheck.go(1 hunks)cmd/vulnsh/clis/version.go(1 hunks)go.mod(5 hunks)pkg/service/cvemap.go(5 hunks)pkg/tools/renderer/example_test.go(1 hunks)pkg/tools/renderer/renderer_test.go(1 hunks)pkg/utils/pager.go(1 hunks)scripts/README.md(1 hunks)scripts/fix-dependencies.sh(1 hunks)scripts/git-hook-install.sh(1 hunks)scripts/pre-commit.sh(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- pkg/service/cvemap.go
- cmd/vulnsh/clis/auth.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/tools/renderer/example_test.go
- go.mod
- cmd/vulnsh/clis/healthcheck.go
- cmd/vulnsh/clis/common.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
scripts/fix-dependencies.sh (1)
scripts/pre-commit.sh (3)
print_error(25-27)print_status(17-19)print_warning(21-23)
scripts/pre-commit.sh (1)
scripts/fix-dependencies.sh (3)
print_error(24-26)print_status(16-18)print_warning(20-22)
pkg/utils/pager.go (1)
pkg/tools/renderer/colors.go (1)
IsTerminal(117-120)
🪛 checkmake (0.2.2)
Makefile
[warning] 38-38: Target body for "lint" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 72-72: Missing required phony target "clean"
(minphony)
🪛 golangci-lint (1.64.8)
pkg/utils/pager.go
13-13: const pageBreak is unused
(unused)
🪛 LanguageTool
scripts/README.md
[grammar] ~72-~72: The singular proper name ‘Module’ must be used with a third-person or a past tense verb.
Context: ...ies.sh ``` Common issues: - Module cache out of sync → `go clean -modcache && go...
(HE_VERB_AGR)
[style] ~114-~114: Consider using a different verb for a more formal wording.
Context: ...will automatically format your code and fix issues when possible ## Skipping Check...
(FIX_RESOLVE)
⏰ 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). (6)
- GitHub Check: Cursor BugBot
- GitHub Check: Test Builds (ubuntu-latest, 1.21.x)
- GitHub Check: Test Builds (macOS-latest, 1.21.x)
- GitHub Check: Test Builds (windows-latest, 1.21.x)
- GitHub Check: Lint Test
- GitHub Check: release-test
🔇 Additional comments (12)
pkg/utils/pager.go (3)
3-9: Good import organization and use of isatty package.The imports are well-organized and the use of the
isattypackage for terminal detection is more robust than manual file mode checks.
22-26: Proper fallback logic for non-terminal output.The function correctly checks for disabled pager and non-terminal stdout, providing appropriate fallback behavior.
42-60: Solid command execution and cleanup implementation.The use of
sh -cto respect flags in$PAGERis appropriate, and the cleanup function properly handles both closing the writer and waiting for the command to complete.cmd/vulnsh/clis/version.go (1)
70-72: No change needed: cvemap is the correct source for vulnsh releases
We’ve confirmed there is currently noprojectdiscovery/vulnshrepository or separate vulnsh release stream—the cvemap repo hosts vulnsh updates. The hardcoded URLs and the “until separate releases are available” note remain accurate.Likely an incorrect or invalid review comment.
client.go (2)
94-102: Excellent concurrent-safe client design.The client struct is well-designed with clear separation of concerns and proper field visibility. The debug hooks provide good extensibility for testing and monitoring.
284-312: Robust HTTP execution with proper resource management.The implementation correctly handles debug hooks, response body cleanup, and error processing. The deferred body close with error logging is a good practice.
pkg/tools/renderer/renderer_test.go (2)
452-493: Comprehensive URL formatting test coverage.These tests thoroughly validate the nuclei template URL formatting logic, covering local paths, full URLs, and edge cases. The transformation from local paths to cloud URLs is well-tested.
377-450: Thorough detailed rendering validation.The test validates all major sections of the detailed output and correctly checks for cloud URL generation instead of local paths. This ensures the user-facing URLs are properly formatted.
scripts/git-hook-install.sh (1)
1-30: Well-implemented git hook installer script.The script follows bash best practices with proper error handling (
set -e), clear validation checks, and informative user feedback. The permission setting and installation process are correct.Makefile (2)
17-18: Good implementation of version injection for vulnsh build.The build target correctly uses ldflags to inject the version at build time, which aligns with the TODO comment in the version.go file.
47-48: Excellent comprehensive quality check target.The
pre-pushtarget effectively combines all quality checks in the right order, ensuring code quality before pushing changes.README.md (1)
240-244: Verifymake git-hookstarget existsIf the Makefile lacks this target the quick-start instructions will fail. Please confirm or update docs.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
pkg/utils/pager.go (2)
24-30: Improve pager binary validation logic.The function only checks for the existence of
lessbinary but doesn't validate if the pager specified in$PAGERenvironment variable actually exists. This could lead to runtime errors if a custom pager is specified but not available.
32-36: Move pager environment variable logic to avoid duplication.The
$PAGERenvironment variable is read after binary validation, but the validation logic should check the actual pager command that will be used.
🧹 Nitpick comments (2)
pkg/tools/renderer/renderer.go (2)
481-507: Optimize the cleanupSeparators function for better performance.The current implementation uses a potentially inefficient loop to remove empty placeholders and has complex string manipulation logic.
Consider using regular expressions or a more efficient approach:
+import "regexp" + +var ( + emptyPlaceholderRegex = regexp.MustCompile(`\{[^}]*\}`) + multipleSeparatorRegex = regexp.MustCompile(`\s*\|\s*\|\s*`) +) + func cleanupSeparators(formatted string) string { - // Remove empty placeholders (anything that looks like {placeholder}) - result := formatted - for strings.Contains(result, "{") && strings.Contains(result, "}") { - start := strings.Index(result, "{") - end := strings.Index(result[start:], "}") - if end == -1 { - break - } - end += start - result = result[:start] + result[end+1:] - } + // Remove empty placeholders using regex + result := emptyPlaceholderRegex.ReplaceAllString(formatted, "") - // Clean up multiple separators - result = strings.ReplaceAll(result, " | |", " |") - result = strings.ReplaceAll(result, "| |", "|") - result = strings.ReplaceAll(result, " | |", " |") + // Clean up multiple separators using regex + result = multipleSeparatorRegex.ReplaceAllString(result, " | ")
447-479: Consider using a more maintainable approach for condition evaluation.The current implementation uses hardcoded condition strings, which is error-prone and difficult to maintain.
Consider using a map-based approach or constants:
+const ( + ConditionAuthorsEmpty = "authors.length == 0" + ConditionEpssZero = "epss_score == 0" + ConditionCvssZero = "cvss_score == 0" + // ... other conditions +) + +var conditionEvaluators = map[string]func(*Entry) bool{ + ConditionAuthorsEmpty: func(e *Entry) bool { return len(e.Author) == 0 }, + ConditionEpssZero: func(e *Entry) bool { return e.EpssScore == 0 }, + ConditionCvssZero: func(e *Entry) bool { return e.CvssScore == 0 }, + // ... other conditions +} + func evaluateCondition(entry *Entry, condition string) bool { - switch condition { - case "authors.length == 0": - return len(entry.Author) == 0 - // ... other cases - } - return false + if evaluator, exists := conditionEvaluators[condition]; exists { + return evaluator(entry) + } + return false }This approach is more maintainable and allows for easier testing of individual conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/tools/renderer/renderer.go(1 hunks)pkg/utils/pager.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/utils/pager.go (1)
pkg/tools/renderer/colors.go (1)
IsTerminal(117-120)
⏰ 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). (5)
- GitHub Check: Cursor BugBot
- GitHub Check: Lint Test
- GitHub Check: Test Builds (ubuntu-latest, 1.21.x)
- GitHub Check: Test Builds (macOS-latest, 1.21.x)
- GitHub Check: Test Builds (windows-latest, 1.21.x)
🔇 Additional comments (6)
pkg/utils/pager.go (4)
1-9: LGTM: Clean package structure and appropriate dependencies.The imports are well-organized and the
isattypackage is the right choice for terminal detection.
11-22: LGTM: Well-documented function with proper early returns.The function documentation is comprehensive and the early return logic correctly handles disabled paging and non-terminal output.
38-49: LGTM: Proper command setup and error handling.The use of
sh -cto respect flags in$PAGERis correct, and the error handling for pipe creation and command start is appropriate.
51-56: LGTM: Cleanup function properly handles resource deallocation.The cleanup function correctly closes the writer and waits for the command to complete. Ignoring the error from
w.Close()is acceptable sincecmd.Wait()will capture any significant errors.pkg/tools/renderer/renderer.go (2)
337-344: LGTM! Good error handling in ParseLayout function.The function properly handles JSON unmarshaling with error wrapping using the
%wverb, which is the recommended approach in Go for error handling.
10-13: LGTM! Clean wrapper function implementation.The
Renderfunction provides a clean interface with sensible defaults, following the common Go pattern of providing both simple and advanced versions of functionality.
Complete migration of binary name, CLI commands, MCP tools, documentation, and all references from vulnsh to vulnx.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
README.md (1)
6-6: Image still references the old project name.The image source path
static/cvemap.pngshould be updated to reflect the new project branding.Update the image source path to use the new branding:
- <img src="static/cvemap.png" alt="vulnx" width="200px"> + <img src="static/vulnsh.png" alt="vulnx" width="200px">
🧹 Nitpick comments (14)
pkg/tools/templates/prompt.go (2)
29-39: Consider making the prompt template list configurable.The hardcoded list of prompt templates may limit extensibility. Consider adding a registration mechanism to allow dynamic addition of new prompt templates.
+// promptTemplates holds registered prompt templates +var promptTemplates []func(*Handler) PromptTemplate + +// RegisterPromptTemplate adds a new prompt template constructor +func RegisterPromptTemplate(constructor func(*Handler) PromptTemplate) { + promptTemplates = append(promptTemplates, constructor) +} + // AllPromptTemplates returns all available prompt templates func AllPromptTemplates(client *cvemap.Client) []PromptTemplate { handler := NewHandler(client) - return []PromptTemplate{ - &VulnerabilityAnalysisPrompt{handler: handler}, - &ThreatIntelligencePrompt{handler: handler}, - &SecurityResearchPrompt{handler: handler}, - &GeneralVulnAssistantPrompt{handler: handler}, - &VulnxSearchReviewPrompt{handler: handler}, - } + result := make([]PromptTemplate, len(promptTemplates)) + for i, constructor := range promptTemplates { + result[i] = constructor(handler) + } + return result }
125-142: Consider adding validation for the mandatory output template.The prompt emphasizes a mandatory Markdown template but doesn't enforce it. Consider adding validation or examples to ensure consistent output formatting.
Makefile (2)
38-44: Address static analysis warning about target body length.The
linttarget exceeds the recommended body length. Consider simplifying or breaking it down.+# Check if golangci-lint is available +lint-check: + @command -v golangci-lint >/dev/null 2>&1 || (echo "⚠️ golangci-lint not found, skipping linting" && exit 0) + lint: @echo "🧐 Running linter..." - @if command -v golangci-lint >/dev/null 2>&1; then \ - golangci-lint run --timeout=5m; \ - else \ - echo "⚠️ golangci-lint not found, skipping linting"; \ - fi + @$(MAKE) lint-check + @golangci-lint run --timeout=5m
72-72: Add missing clean target for completeness.Static analysis suggests adding a required
cleantarget.+# Clean build artifacts +clean: + @echo "🧹 Cleaning build artifacts..." + @rm -f cvemap vulnx + -.PHONY: all build build-vulnx integration tidy fmt vet test lint pre-push pre-commit git-hooks fix-deps +.PHONY: all build build-vulnx integration tidy fmt vet test lint pre-push pre-commit git-hooks fix-deps cleancmd/vulnx/clis/id.go (1)
119-139: Input validation could be more comprehensive.The validation function checks basic constraints but could be enhanced for better CVE ID format validation.
+import "regexp" +var cveIDPattern = regexp.MustCompile(`^CVE-\d{4}-\d{4,}$`) // validateIDInputs performs input validation for id command func validateIDInputs(vulnID string) error { // Basic CVE ID format validation if vulnID == "" { return fmt.Errorf("vulnerability ID cannot be empty") } - // Check for reasonable length - if len(vulnID) < 3 || len(vulnID) > 50 { - return fmt.Errorf("vulnerability ID length must be between 3 and 50 characters") + // Validate CVE ID format if it looks like a CVE + if strings.HasPrefix(strings.ToUpper(vulnID), "CVE-") { + if !cveIDPattern.MatchString(strings.ToUpper(vulnID)) { + return fmt.Errorf("invalid CVE ID format, expected CVE-YYYY-NNNN") + } + } else { + // For non-CVE IDs, check reasonable length + if len(vulnID) < 3 || len(vulnID) > 50 { + return fmt.Errorf("vulnerability ID length must be between 3 and 50 characters") + } }cmd/vulnx/clis/version.go (1)
54-62: Temporary workaround needs tracking.The use of "cvemap" for version checking is a reasonable temporary solution, but ensure this TODO is tracked for future resolution.
Consider creating a GitHub issue to track the server-side support for vulnx version checking to ensure this workaround doesn't become permanent.
cmd/vulnx/clis/search.go (1)
31-62: Large embedded JSON layout reduces maintainability.The 30+ line JSON layout embedded in the code makes the file harder to read and maintain. Consider extracting this to a separate file or constant.
Consider moving the layout definition to a separate file:
- defaultLayoutJSON = `[ - { - "line": 1, - "format": "[{doc_id}] {severity} - {title}", - "omit_if": [] - }, - // ... rest of layout - ]` + //go:embed layouts/search_default.json + defaultLayoutJSON stringREADME.md (2)
172-173: Add language specification to fenced code blocks.Static analysis indicates these code blocks should specify a language for proper syntax highlighting.
Add language specification to improve documentation quality:
-``` +```text Error: api key is required → Run: vulnx auth -``` +``` -``` +```text Error: api key is required (when running vulnx search --help) → This is a known limitation. Either: 1. Set up API key first: vulnx auth 2. Use this documentation for command help -``` +```Also applies to: 178-179
240-240: Replace emphasis with proper headings.Lines using emphasis (
**text**) should use proper markdown headings for better document structure.Use proper markdown headings for better document structure:
-**Option 1: Zero Dependencies (Recommended for simple setups)** +#### Option 1: Zero Dependencies (Recommended for simple setups) -**Option 2: Pre-commit Package (Recommended for advanced features)** +#### Option 2: Pre-commit Package (Recommended for advanced features)Also applies to: 246-246
cmd/vulnx/clis/common.go (4)
3-25: Reorganize imports following Go conventions.The imports should be grouped with standard library packages first, followed by third-party packages.
Apply this diff to reorganize the imports:
import ( - "github.com/spf13/cobra" - - "time" - + _ "embed" "fmt" "net/http" "net/http/httputil" "net/url" - - _ "embed" + "os" "strings" + "time" + "github.com/mark3labs/mcp-go/server" "github.com/projectdiscovery/cvemap" + "github.com/projectdiscovery/cvemap/pkg/tools" "github.com/projectdiscovery/gologger" "github.com/projectdiscovery/gologger/levels" retryablehttp "github.com/projectdiscovery/retryablehttp-go" - - "os" - - "github.com/mark3labs/mcp-go/server" - "github.com/projectdiscovery/cvemap/pkg/tools" + "github.com/spf13/cobra" )
30-59: Consider encapsulating global configuration in a struct.The numerous global variables could lead to state management issues and make testing more difficult. Consider grouping related configuration into a struct.
Example approach:
type Config struct { // Logging Verbose bool Debug bool Silent bool NoColor bool // HTTP configuration HTTPProxy string HTTPTimeout time.Duration DebugReq bool DebugResp bool // Output configuration NoPager bool JSONOutput bool OutputFile string } var globalConfig Config
101-110: Complete the SSE implementation or remove the TODO.The SSE mode contains a TODO comment and returns a not-implemented error. This should either be completed or the mode should be removed until implementation is ready.
Would you like me to help implement the SSE handler or create an issue to track this TODO?
104-106: Simplify error handling for HTTP response writing.The current error handling logs the error but continues execution. Consider using a more idiomatic approach.
-if _, err := w.Write([]byte("SSE mode is not yet implemented in this build. Please update the MCP server package or implement SSE handler.")); err != nil { - gologger.Error().Msgf("Failed to write SSE not implemented message: %s", err) -} +_, _ = w.Write([]byte("SSE mode is not yet implemented in this build. Please update the MCP server package or implement SSE handler."))USAGE.md (1)
398-398: Fix compound adjective with hyphen.When "rate limiting" is used as a compound adjective, it should be hyphenated.
-- **Rate limiting awareness** - Add delays between bulk operations +- **Rate-limiting awareness** - Add delays between bulk operations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.gitignore(1 hunks)Makefile(1 hunks)README.md(1 hunks)USAGE.md(1 hunks)cmd/integration-test/main.go(3 hunks)cmd/integration-test/run.sh(1 hunks)cmd/integration-test/vulnsh_cases.go(1 hunks)cmd/vulnx/clis/auth.go(1 hunks)cmd/vulnx/clis/banner.txt(1 hunks)cmd/vulnx/clis/common.go(1 hunks)cmd/vulnx/clis/groupby.go(1 hunks)cmd/vulnx/clis/groupbyhelp.go(1 hunks)cmd/vulnx/clis/healthcheck.go(1 hunks)cmd/vulnx/clis/id.go(1 hunks)cmd/vulnx/clis/search.go(1 hunks)cmd/vulnx/clis/searchhelp.go(1 hunks)cmd/vulnx/clis/version.go(1 hunks)cmd/vulnx/main.go(1 hunks)pkg/tools/agentvulnx/agentvulnx.go(1 hunks)pkg/tools/filters/filters.go(1 hunks)pkg/tools/groupby/groupby.go(1 hunks)pkg/tools/id/id.go(1 hunks)pkg/tools/search/search.go(1 hunks)pkg/tools/templates/prompt.go(1 hunks)pkg/tools/tools.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- cmd/vulnx/clis/banner.txt
- cmd/vulnx/main.go
- pkg/tools/id/id.go
🚧 Files skipped from review as they are similar to previous changes (9)
- .gitignore
- cmd/integration-test/run.sh
- cmd/integration-test/main.go
- pkg/tools/tools.go
- pkg/tools/agentvulnx/agentvulnx.go
- pkg/tools/search/search.go
- pkg/tools/filters/filters.go
- cmd/integration-test/vulnsh_cases.go
- pkg/tools/groupby/groupby.go
🧰 Additional context used
🧬 Code Graph Analysis (5)
cmd/vulnx/clis/groupbyhelp.go (2)
pkg/utils/pager.go (1)
OpenPager(18-56)pkg/tools/filters/filters.go (1)
NewHandler(24-26)
cmd/vulnx/clis/search.go (6)
types.go (2)
SearchParams(211-222)Ptr(256-258)pkg/tools/search/search.go (1)
NewHandler(32-34)client.go (1)
ErrNotFound(64-64)pkg/tools/renderer/renderer.go (2)
ParseLayout(338-344)RenderWithColors(302-335)pkg/tools/renderer/types.go (2)
Entry(15-39)FromVulnerability(42-74)pkg/tools/renderer/colors.go (4)
ColorConfig(36-69)IsTerminal(117-120)NoColorConfig(110-114)DefaultColorConfig(72-107)
cmd/vulnx/clis/common.go (2)
client.go (6)
Client(94-102)Option(89-89)WithKeyFromEnv(153-161)WithRetryableHTTPOptions(173-177)WithDebugRequest(180-184)WithDebugResponse(187-191)pkg/tools/tools.go (1)
AllMCPTools(24-32)
cmd/vulnx/clis/searchhelp.go (2)
pkg/utils/pager.go (1)
OpenPager(18-56)pkg/tools/filters/filters.go (1)
NewHandler(24-26)
pkg/tools/templates/prompt.go (6)
client.go (1)
Client(94-102)pkg/tools/agentvulnx/agentvulnx.go (2)
NewHandler(45-47)Handler(40-42)pkg/tools/filters/filters.go (2)
NewHandler(24-26)Handler(17-19)pkg/tools/groupby/groupby.go (3)
NewHandler(56-58)Handler(50-52)Params(28-42)pkg/tools/search/search.go (2)
NewHandler(32-34)Handler(26-28)pkg/tools/id/id.go (2)
NewHandler(16-20)Handler(11-13)
🪛 markdownlint-cli2 (0.17.2)
README.md
172-172: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
240-240: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
246-246: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 checkmake (0.2.2)
Makefile
[warning] 38-38: Target body for "lint" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 72-72: Missing required phony target "clean"
(minphony)
🪛 LanguageTool
USAGE.md
[uncategorized] ~398-~398: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...Check exit codes and JSON structure - Rate limiting awareness - Add delays between bulk o...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ 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). (5)
- GitHub Check: Cursor BugBot
- GitHub Check: Test Builds (macOS-latest, 1.21.x)
- GitHub Check: Test Builds (windows-latest, 1.21.x)
- GitHub Check: Test Builds (ubuntu-latest, 1.21.x)
- GitHub Check: Lint Test
🔇 Additional comments (27)
pkg/tools/templates/prompt.go (6)
11-17: Well-designed interface with clear separation of concerns.The
PromptTemplateinterface properly separates MCP prompt specification from handler implementation, enabling modular registration and execution of different prompt types.
66-143: Comprehensive vulnerability analysis prompt with good structure.The system prompt provides detailed guidance and examples for using MCP tools effectively. The analysis type switching logic appropriately tailors the context for different use cases.
212-284: Excellent threat intelligence specialization with practical query patterns.The threat intelligence prompt provides valuable real-world query patterns and a structured workflow that would be highly useful for security analysts.
443-484: Strong ethical stance in the general assistant prompt.The prompt correctly includes ethical guidelines and refuses to provide hacking services, which is appropriate for a security tool.
525-564: Well-structured iterative query refinement framework.The VulnxSearchReviewPrompt provides a solid loop-based approach for query validation and improvement, which should help users create more effective searches.
583-592: Safe helper function with proper null checks.The
getStringArgfunction includes appropriate nil checks and default value handling.cmd/vulnx/clis/auth.go (1)
40-86: Strong authentication flow with proper validation.The command implements good security practices: interactive input, validation, server verification, and secure credential storage using the PDCP handler.
Makefile (1)
17-18: Good build target with version embedding.The
build-vulnxtarget properly embeds version information via linker flags, enabling proper version reporting in the CLI.cmd/vulnx/clis/id.go (3)
43-60: Excellent error handling with specific error types.The command properly handles different error conditions including
ErrNotFoundand provides meaningful error messages to users.
68-86: Robust file handling with overwrite protection.The implementation correctly prevents accidental file overwrites and handles file operations safely with proper resource cleanup.
98-114: Good color configuration handling.The color detection and configuration logic properly handles terminal capabilities and user preferences.
cmd/vulnx/clis/version.go (2)
34-40: Smart client initialization override.The
PersistentPreRunEoverride appropriately prevents unnecessary client initialization for the version command while maintaining the banner display.
68-74: Clear user guidance for updates.The update information provides helpful guidance about where to find updates and explains the current release bundling approach.
cmd/vulnx/clis/groupby.go (3)
184-204: Input validation function is well-structured.The validation logic properly checks field requirements, facet size bounds, and output file extensions. The error messages are clear and actionable.
87-89: File existence check prevents accidental overwrites.Good defensive programming to check if the output file exists before attempting to create it, preventing data loss.
132-179: Complex facet parsing logic handles multiple response formats.The facet parsing correctly handles both map and slice bucket formats, includes sorting for consistent output, and properly handles missing values. The type assertions are safe with proper fallbacks.
cmd/vulnx/clis/groupbyhelp.go (1)
103-103: boolToYN is not duplicated
I only found a single definition ofboolToYNincmd/vulnx/clis/searchhelp.go. Since both help files reside in the same package, the utility is already shared—no relocation needed.Likely an incorrect or invalid review comment.
cmd/vulnx/clis/search.go (2)
133-137: String replacement for facet format suggests API inconsistency.The code replaces
=with:in term facets, which indicates a mismatch between CLI flag format and API expectations.Consider whether this format conversion could be handled more elegantly, or if the API could accept the
=format directly to avoid this transformation.
237-267: Comprehensive input validation with clear error messages.The validation function properly checks all parameters with appropriate bounds and provides actionable error messages for users.
cmd/vulnx/clis/searchhelp.go (1)
82-106: Dynamic filter table generation provides excellent UX.The code fetches live filter metadata and presents it in a well-formatted table with appropriate column width constraints. This gives users up-to-date information about available search fields.
README.md (1)
27-231: Excellent comprehensive documentation rewrite.The README transformation from a basic feature list to a comprehensive user guide is outstanding. The structured approach with quick start, usage patterns, troubleshooting, and development sections provides excellent user experience.
cmd/vulnx/clis/common.go (2)
175-237: Well-structured client initialization logic.The
ensureCvemapClientInitializedfunction properly handles client initialization with good error handling, debug logging, and proxy configuration.
88-89: Parameter required by MCPHandler signatureThe
MCPHandlermethod is defined on each tool’s handler type as:func (h *Handler) MCPHandler(client *cvemap.Client) func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error)Because it explicitly accepts a
*cvemap.Client, passingcvemapClientins.AddTool(tool.MCPToolSpec(), tool.MCPHandler(cvemapClient))is both necessary and correct. No change is required here.
Likely an incorrect or invalid review comment.
USAGE.md (1)
1-403: Excellent comprehensive documentation.The USAGE.md provides thorough guidance with practical examples, clear structure, and comprehensive coverage of vulnx features. This will be very helpful for users.
cmd/vulnx/clis/healthcheck.go (3)
14-97: Well-structured healthcheck implementation.The healthcheck command is well-designed with:
- Clear command documentation and examples
- Structured result type for consistent output
- Logical flow of checks with early exit on authentication failure
- Good status reporting with visual indicators
99-223: Robust implementation of health check functions.Each check function follows a consistent pattern with:
- Proper timing measurements
- Comprehensive error handling
- Meaningful status messages
- Appropriate use of PASS/FAIL/WARN statuses
The warning threshold of 10 seconds for API response time is reasonable.
225-262: Clean and user-friendly result display.The
displayResultsfunction provides excellent UX with:
- Visual status indicators (✅ ❌
⚠️ )- Clear formatting with proper indentation
- Optional verbose JSON output for debugging
- Duration display when relevant
- Add --api-key flag for non-interactive auth setup - Add --test flag for API key validation - Fix auth validation to use consistent API endpoints - Add key source indication (ENV vs CONFIG) - Create DEVELOPMENT.md and update README.md - Remove duplicate messages and improve UX
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cmd/vulnx/clis/auth.go (2)
56-58: Weak API key validation should be enhanced.The length-only validation (minimum 10 characters) is insufficient for API key format verification.
// Validate the API key format (basic validation) -if len(nonInteractiveAPIKey) < 10 { - gologger.Fatal().Msg("Invalid API key format") -} +if len(nonInteractiveAPIKey) < 32 || len(nonInteractiveAPIKey) > 128 { + gologger.Fatal().Msg("Invalid API key format: expected 32-128 characters") +} + +// Check for reasonable API key patterns (alphanumeric + common special chars) +if !regexp.MustCompile(`^[a-zA-Z0-9_-]+$`).MatchString(nonInteractiveAPIKey) { + gologger.Fatal().Msg("Invalid API key format: contains invalid characters") +}
257-260: Weak API key validation should be enhanced.The length-only validation (minimum 10 characters) is insufficient for API key format verification.
// Validate the API key format (basic validation) -if len(apiKey) < 10 { - gologger.Fatal().Msg("Invalid API key format") -} +if len(apiKey) < 32 || len(apiKey) > 128 { + gologger.Fatal().Msg("Invalid API key format: expected 32-128 characters") +} + +// Check for reasonable API key patterns (alphanumeric + common special chars) +if !regexp.MustCompile(`^[a-zA-Z0-9_-]+$`).MatchString(apiKey) { + gologger.Fatal().Msg("Invalid API key format: contains invalid characters") +}README.md (1)
6-6: Image still references the old project name
static/cvemap.pngis likely obsolete after the rebrand; update to the new asset to avoid a broken logo.- <img src="static/cvemap.png" alt="vulnx" width="200px"> + <img src="static/vulnsh.png" alt="vulnx" width="200px">
🧹 Nitpick comments (2)
DEVELOPMENT.md (1)
138-152: Add language specification to code block for better formatting.The project structure code block should specify a language for proper syntax highlighting.
-``` +```text cvemap/ ├── cmd/ │ ├── cvemap/ # Main CLI applicationREADME.md (1)
179-182: Add language specifications to code blocks for proper formatting.The error message blocks should specify a language for consistency.
For line 179:
-``` +```console Error: api key is required → Run: vulnx authFor line 200: ```diff -``` +```console Error: api key is required (when running vulnx search --help) → This is a known limitation. Either: 1. Set up API key first: vulnx auth 2. Use this documentation for command helpAlso applies to: 200-205 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9f190ac8df5299410a12aef3a7affdd3ad4f3fc8 and fd7a64ec5f312604821e37d44940603e94340339. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `DEVELOPMENT.md` (1 hunks) * `README.md` (1 hunks) * `cmd/vulnx/clis/auth.go` (1 hunks) * `go.mod` (5 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * go.mod </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 golangci-lint (1.64.8)</summary> <details> <summary>cmd/vulnx/clis/auth.go</summary> 164-164: Error return value of `fmt.Scanln` is not checked (errcheck) --- 176-176: Error return value of `fmt.Scanln` is not checked (errcheck) --- 207-207: Error return value of `fmt.Scanln` is not checked (errcheck) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>README.md</summary> 179-179: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 200-200: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>DEVELOPMENT.md</summary> 138-138: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 LanguageTool</summary> <details> <summary>DEVELOPMENT.md</summary> [grammar] ~160-~160: The verb “Follow” needs to be in the to-infinitive form. Context: ...w features - Update documentation as needed - Follow the existing code style 3. **Documentat... (MISSING_TO_BEFORE_A_VERB) </details> </details> </details> <details> <summary>⏰ 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). (5)</summary> * GitHub Check: Test Builds (ubuntu-latest, 1.21.x) * GitHub Check: Lint Test * GitHub Check: Test Builds (macOS-latest, 1.21.x) * GitHub Check: Test Builds (windows-latest, 1.21.x) * GitHub Check: Cursor BugBot </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>cmd/vulnx/clis/auth.go (1)</summary> `244-250`: **Good implementation of secure API key input.** The use of `term.ReadPassword` properly masks the API key during input, addressing the security concern. </details> <details> <summary>DEVELOPMENT.md (1)</summary> `1-178`: **Excellent comprehensive development guide.** This documentation provides clear, detailed instructions for developers with well-organized sections covering building, testing, code quality, and troubleshooting. </details> <details> <summary>README.md (1)</summary> `1-262`: **Well-structured and comprehensive README documentation.** The README provides excellent user guidance with clear examples, troubleshooting tips, and a logical flow from quick start to advanced usage. The rebranding to vulnx is well executed throughout. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Added empty string checks before accessing first character [0] in constructQueryParams and constructQueryByOptions functions - Prevents runtime panic when empty strings are provided for cvssScore, opts.Age, opts.EpssScore, and opts.EpssPercentile parameters - Ensures robust handling of command-line arguments and improves application stability
- Delete pkg/utils/pager.go - main pager implementation - Remove noPager flag and --no-pager option from common.go - Update help commands to write directly to stdout instead of using pager - Remove pager usage from searchhelp.go, groupbyhelp.go, and groupby.go - Remove --no-pager from completion suggestions - Update README.md to remove --no-pager example - Remove empty pkg/utils directory The pager was rarely used and only applied to help content and facet results, not search results. Users can still pipe to less manually if needed.
✨ Filter Flags Added: - Product filters: --product (searches both vendor & product fields) - Vendor filters: --vendor (vendor field only) - Severity filters: --severity, --exclude-severity - Assignment filters: --assignee, --vstatus, --vuln-age - Exploit filters: --kev-only, --template, --poc, --hackerone, --remote-exploit - CPE filter: --cpe - Exclusion filters: --exclude-product, --exclude-vendor 🔧 Technical Improvements: - File input support for all applicable filters (*-file flags) - Proper query building with || and && operators (not textual OR/AND) - Smart product filter searches both vendor and product fields - Boolean flag support with true/false values - Age comparison operators (<, >, exact) - Comprehensive input validation and error handling 📚 Documentation: - Updated README with Filter Flags section and examples - Updated bash completion with all new flags - Clear descriptions for all filter options 🧪 Verified Working: - Product filter: apache (2492 results vs 1 before) - Multiple products: apache,nginx (2536 results) - Combined filters: apache + critical + KEV (49 results) - File input, boolean flags, age comparisons all tested
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Fix boolean flags (--kev-only, --template, etc.) to default to true when no value provided - Fix vendor filter to search both vendor and product fields for better coverage - Remove 'ADDITIONAL FIELD DETAILS' section from search help menu - Implement field-level conditional rendering instead of line-level omit_if - Fields now show independently based on data availability (no more 'unknown' clutter) - Vendor/product line now always shows when data exists regardless of missing exposure
- Remove all file-related flags: --product-file, --vendor-file, --assignee-file, --exclude-product-file, --exclude-vendor-file, --severity-file, --exclude-severity-file - Simplify search command to use only comma-separated command-line flags - Remove file input helper functions and related code - Update documentation to remove file input examples and sections - Maintain all core filtering functionality with cleaner interface
Summary by CodeRabbit
New Features
vulnxCLI tool for advanced vulnerability intelligence, including commands for searching, grouping, analyzing, health checking, authentication, and version display.Bug Fixes
Documentation
Style
Tests
Chores
.gitignoreentries for improved development workflow.