Fixed a bug that caused template updates to fail #6374
Conversation
WalkthroughThe update strengthens file path validation in the template installer by replacing a basic prefix check with a relative path computation to prevent directory traversal vulnerabilities. Additionally, the code now ensures that target directories exist before writing files, reducing the risk of file write errors due to missing directories. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🔭 Outside diff range comments (1)
pkg/installer/template.go (1)
221-226: Critical: LFI guard is bypassed for root-level/absolute entries (uri without '/').When strings.Index(uri, "/") == -1, the function returns early with filepath.Join(templateDir, uri) and never runs the filepath.Rel containment check. Join returns a clean path and will discard the base for absolute uris (e.g., "/etc/passwd" or "C:\Windows..."), allowing writes outside templateDir.
Fix by unifying path derivation and always applying the Rel-based containment check.
Proposed refactor (outside the changed hunk) to eliminate the early return and consistently run the guard:
// getAbsoluteFilePath returns an absolute path where a file should be written based on given uri(i.e., files in zip) // if a returned path is empty, it means that file should not be written and skipped func (t *TemplateManager) getAbsoluteFilePath(templateDir, uri string, f fs.FileInfo) string { // overwrite .nuclei-ignore every time nuclei-templates are downloaded if f.Name() == config.NucleiIgnoreFileName { return config.DefaultConfig.GetIgnoreFilePath() } // skip all meta files if !strings.EqualFold(f.Name(), config.NewTemplateAdditionsFileName) { if strings.TrimSpace(f.Name()) == "" || strings.HasPrefix(f.Name(), ".") || strings.EqualFold(f.Name(), "README.md") { return "" } } // Extract relative part from "projectdiscovery-nuclei-templates-<hash>[/...]" index := strings.Index(uri, "/") var relPath string if index == -1 { // No directory component in zip entry; still proceed with containment checks gologger.Warning().Msgf("failed to get directory name from uri: %s", uri) relPath = uri } else { rootDirectory := uri[:index+1] // includes separator relPath = strings.TrimPrefix(uri, rootDirectory) } // if it is a github meta directory skip it if stringsutil.HasPrefixAny(relPath, ".github", ".git") { return "" } newPath := filepath.Clean(filepath.Join(templateDir, relPath)) // Robust containment check relPathToTemplateDir, err := filepath.Rel(templateDir, newPath) if err != nil { return "" } if relPathToTemplateDir == ".." || strings.HasPrefix(relPathToTemplateDir, ".."+string(os.PathSeparator)) { // we don't allow LFI return "" } if newPath == templateDir || newPath == templateDir+string(os.PathSeparator) { // skip writing the folder itself since it already exists return "" } if relPath != "" && f.IsDir() { // if uri is a directory, create it if err := fileutil.CreateFolder(newPath); err != nil { gologger.Warning().Msgf("uri %v: got %s while installing templates", uri, err) } return "" } return newPath }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/installer/template.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-16T21:27:14.937Z
Learnt from: hdm
PR: projectdiscovery/nuclei#6322
File: pkg/templates/compile.go:79-81
Timestamp: 2025-07-16T21:27:14.937Z
Learning: To make the template caching mechanism in pkg/templates/compile.go production-ready, DSLs need to be updated to use runtime options instead of cached variables, rather than restoring the Compile() calls on each request.
Applied to files:
pkg/installer/template.go
🔇 Additional comments (1)
pkg/installer/template.go (1)
282-287: Solid fix: ensure parent directories exist before file writes.This addresses cases where archive entries are streamed before their directory entries (common on Windows), preventing empty-tree installs.
| // Use filepath.Rel for more robust path comparison | ||
| relPathToTemplateDir, err := filepath.Rel(templateDir, newPath) | ||
| if err != nil || strings.HasPrefix(relPathToTemplateDir, "..") { | ||
| // we don't allow LFI |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good hardening; make the '..' check segment-aware to avoid false positives.
Using filepath.Rel is the right fix. Replace the plain prefix check with a segment-aware one so names like "..." don’t get incorrectly rejected.
- relPathToTemplateDir, err := filepath.Rel(templateDir, newPath)
- if err != nil || strings.HasPrefix(relPathToTemplateDir, "..") {
+ relPathToTemplateDir, err := filepath.Rel(templateDir, newPath)
+ if err != nil {
+ return ""
+ }
+ // Disallow any path that escapes templateDir (first segment is "..")
+ if relPathToTemplateDir == ".." || strings.HasPrefix(relPathToTemplateDir, ".."+string(os.PathSeparator)) {
// we don't allow LFI
return ""
}🤖 Prompt for AI Agents
In pkg/installer/template.go around lines 238 to 241, the current check for
directory traversal uses strings.HasPrefix to detect ".." which can cause false
positives with names like "...". Replace this with a segment-aware check by
splitting relPathToTemplateDir using filepath separators and verifying if any
segment equals "..". This ensures only actual directory traversal attempts are
blocked without rejecting valid paths.
|
how has this been tested? and why is the current template version empty (seems like an edge case)? |
I always experience this in my daily use, but I don't know the exact cause, and I've seen it in many Issues. |
I think it's okay. |
3 participants
This error always occurs when a new version of nuclei is initialized or when updating a template.
In fact, the C:\Users\admin\nuclei-templates directory is empty.
My modification solves the following issues:
Summary by CodeRabbit