refactor(disk): templates catalog #5914
Conversation
|
""" WalkthroughThe pull request introduces modifications to the template path handling and error management in the Nuclei project. Changes primarily focus on the Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
pkg/catalog/disk/path.go (1)
75-77: LGTM: Well-documented deprecation noticeThe deprecation notice follows Go's standard format and clearly explains the reason for deprecation.
Consider adding a target version for removal in the deprecation notice to help users plan their migration.
pkg/catalog/disk/find.go (1)
79-90: Consider adding error variable for glob pattern errorsWhile the glob pattern handling is improved, consider defining a custom error variable for glob-specific errors, similar to
ErrNoTemplatesFound.+var ErrGlobPatternFailed = errors.New("failed to process glob pattern") if strings.Contains(target, "*") { globMatches, err := c.findGlobPathMatches(target, processed) if err != nil { - return nil, errors.Wrap(err, "could not globbing path") + return nil, errors.Wrap(err, ErrGlobPatternFailed.Error()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/runner/runner.go(0 hunks)pkg/catalog/disk/catalog.go(1 hunks)pkg/catalog/disk/errors.go(1 hunks)pkg/catalog/disk/find.go(6 hunks)pkg/catalog/disk/path.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/runner/runner.go
✅ Files skipped from review due to trivial changes (1)
- pkg/catalog/disk/errors.go
🔇 Additional comments (3)
pkg/catalog/disk/find.go (2)
73-76: LGTM: Improved error handling
The error handling is now more specific and informative with proper error wrapping.
249-253: LGTM: Consistent deprecation notice
The deprecation notice aligns well with the BackwardsCompatiblePaths deprecation.
pkg/catalog/disk/catalog.go (1)
Line range hint 75-77: Verify deprecated function usage
Let's verify if there are any remaining usages of the deprecated functions in the codebase.
Also applies to: 249-253
✅ Verification successful
Let me check if these functions are actually being called anywhere in the codebase.
Functions are safe to be removed
The deprecated functions BackwardsCompatiblePaths and PrintDeprecatedPathsMsgIfApplicable are only defined in their respective files (pkg/catalog/disk/path.go and pkg/catalog/disk/find.go) but are not being called anywhere in the codebase. They can be safely removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of deprecated functions
echo "Checking for BackwardsCompatiblePaths usage..."
rg "BackwardsCompatiblePaths" --type go
echo "Checking for PrintDeprecatedPathsMsgIfApplicable usage..."
rg "PrintDeprecatedPathsMsgIfApplicable" --type go
Length of output: 702
Script:
#!/bin/bash
# Search for actual usage/calls of these functions
echo "Checking for BackwardsCompatiblePaths function calls..."
ast-grep --pattern 'BackwardsCompatiblePaths($$$)'
echo -e "\nChecking for PrintDeprecatedPathsMsgIfApplicable function calls..."
ast-grep --pattern 'PrintDeprecatedPathsMsgIfApplicable($$$)'
Length of output: 386
There was a problem hiding this comment.
failing functional tests related to this change:
[✓] Test "{{binary}} -tags cve -exclude-templates cves/2020/CVE-2020-9757.yaml -exclude-templates cves/2021/" passed!
[✘] Test "{{binary}} -t cves/" failed: could not run nuclei main test: exit status 1
[✘] Test "{{binary}} -t cves/ -t exposures/" failed: could not run nuclei main test: exit status 1
[✘] Test "{{binary}} -t cves/ -t exposures/ -tags config" failed: could not run nuclei main test: exit status 1
[✘] Test "{{binary}} -t cves/ -t exposures/ -tags config,ssrf" failed: could not run nuclei main test: exit status 1
[✘] Test "{{binary}} -t cves/ -t exposures/ -tags config -severity high,critical" failed: could not run nuclei main test: exit status 1
[✘] Test "{{binary}} -t cves/ -t exposures/ -tags config -severity high,critical -author geeknik,pdteam" failed: could not run nuclei main test: exit status 1
[✘] Test "{{binary}} -t cves/ -t exposures/ -tags config -severity high,critical -author geeknik,pdteam -etags sqli" failed: could not run nuclei main test: exit status 1
[✘] Test "{{binary}} -t cves/ -t exposures/ -tags config -severity high,critical -author geeknik,pdteam -etags sqli -exclude-templates cves/2021/" failed: could not run nuclei main test: exit status 1
[✘] Test "{{binary}} -t cves/ -t exposures/ -tags config -severity high,critical -author geeknik,pdteam -etags sqli -exclude-templates cves/2017/CVE-2017-72[69](https://github.com/projectdiscovery/nuclei/actions/runs/12373483567/job/34535007796?pr=5914#step:4:74).yaml" failed: could not run nuclei main test: exit status 1
[✘] Test "{{binary}} -t cves/ -t exposures/ -tags config -severity high,critical -author geeknik,pdteam -etags sqli -include-templates cves/2017/CVE-2017-7269.yaml" failed: could not run nuclei main test: exit status 1
[✓] Test "{{binary}} -tags cve -author geeknik,pdteam -tc severity=='high'" passed!
[✓] Test "{{binary}} -tc contains(authors,'pdteam')" passed!
[✘] Test "{{binary}} -t cves/ -t exposures/ -tc contains(tags,'cve') -exclude-templates cves/2020/CVE-2020-9757.yaml" failed: could not run nuclei main test: exit status 1
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/functional-test/testcases.txt (1)
39-56: Consider updating related documentationThe template path changes are consistent and correct. However, since this represents a structural change in how templates are referenced, consider:
- Updating the project's documentation to reflect the new path structure
- Adding a migration guide for users updating from older versions
- Mentioning this change in the changelog
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/functional-test/testcases.txt(2 hunks)
🔇 Additional comments (3)
cmd/functional-test/testcases.txt (3)
42-51: LGTM! Template selection commands are consistently updated
The changes to template paths are applied consistently across all template selection commands, maintaining proper syntax and flag combinations.
56-56: LGTM! Advanced filtering command is correctly updated
The template path update in the advanced filtering command maintains consistency with the overall refactoring pattern.
39-41: Verify backward compatibility for template exclusion paths
The addition of the http/ prefix to template paths is consistent with the refactoring goals. However, we should ensure this change doesn't break existing user configurations.
✅ Verification successful
Path changes are consistent and non-breaking
The addition of the http/ prefix to template paths in the test cases is consistent with the codebase organization. Based on the verification:
- No references to the old path format (
cves/20XX/) were found in configuration files or documentation - The changes are limited to test cases and appear to be part of a coordinated template reorganization
- The test file itself shows consistent usage of the new
http/prefix across all commands
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any references to the old path format in documentation or example files
# Search for old path format in markdown files and examples
rg -g '*.{md,yaml,txt}' 'cves/20[0-9]{2}/' --no-filename
# Search for any migration guides or documentation updates
fd -g '*.md' --exec grep -l -i 'migration\|breaking change\|path change'
Length of output: 1011
Script:
#!/bin/bash
# Let's check the content of the README files for any migration notes
cat README.md lib/README.md
# Also check for any previous versions of these test cases
git log -p -- cmd/functional-test/testcases.txt | head -n 100
Length of output: 60003
Script:
#!/bin/bash
# Let's check if there are any other test files or configuration files that might reference these paths
fd -e yaml -e txt -e json --exec grep -l 'cves/20[0-9]{2}'
# Also check for any template files in the old path structure
fd -g 'cves/20[0-9]{2}/*'
Length of output: 88
@ehsandeep - since we're deprecating
This should increment minor version, as we no longer support backward-compatible paths from the old Nuclei Templates repo structure ( |
dwisiswant0
force-pushed
the
dwisiswant0/refactor/disk/templates-catalog
branch
from
64bfd3a to
8635b4a
Compare
There was a problem hiding this comment.
Looks good. I also think we can remove the backward compatibility logic since it's no longer relevant. The main consideration is that -t cves and -t misconfiguration are very common filters. While these can be achieved with -tags, we could also support this feature by updating the file glob matcher to match wildcard prefixes (it currently only matches suffixes of given keywords).
Can you give an example for that case? |
|
Hi Team, do we have an updated timeline for the integration? Appreciate your insights - thanks! |
dwisiswant0
force-pushed
the
dwisiswant0/refactor/disk/templates-catalog
branch
from
8635b4a to
9d6b9af
Compare
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
since `BackwardsCompatiblePaths` func is already deprecated. Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
dwisiswant0
force-pushed
the
dwisiswant0/refactor/disk/templates-catalog
branch
from
9d6b9af to
e438108
Compare
Proposed changes
BackwardsCompatiblePaths&PrintDeprecatedPathsMsgIfApplicableChecklist
Summary by CodeRabbit
New Features
ErrNoTemplatesFoundfor better error handling related to template retrieval.Improvements
Deprecations
BackwardsCompatiblePathsfunction as deprecated due to repository restructuring.Bug Fixes