feat(reporting): add PDF exporter for scan results#7074
Open
yuliuyi717-ux wants to merge 4 commits intoprojectdiscovery:devfrom
Open
feat(reporting): add PDF exporter for scan results#7074yuliuyi717-ux wants to merge 4 commits intoprojectdiscovery:devfrom
yuliuyi717-ux wants to merge 4 commits intoprojectdiscovery:devfrom
Conversation
Neo - PR Security ReviewMedium: 2 Highlights
Medium (2)
Attack ExamplesAbsolute path writes still allowed in PDF exporter ( Absolute path writes still allowed in PDF exporter ( Suggested FixesAbsolute path writes still allowed in PDF exporter ( Absolute path writes still allowed in PDF exporter ( 🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In Verify each finding against the current code and only fix it if needed. In Comment |
Contributor
WalkthroughAdds a new PDF exporter that buffers ResultEvent entries and generates a styled PDF on Close (configurable output path and omission of raw blocks). Tests and reporting integration were added; go.mod gains an indirect gofpdf dependency. Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant Reporting as Reporting System
participant Exporter as PDF Exporter
participant Buffer as Results Buffer
participant PDF as gofpdf
participant FS as File System
Config->>Reporting: Initialize with PDFExporter options
Reporting->>Exporter: New(options)
Exporter->>FS: Ensure output directory / validate path
Reporting->>Exporter: Export(result event)
Exporter->>Buffer: Lock & append deep-copied result
Reporting->>Exporter: Export(result event)
Exporter->>Buffer: Lock & append deep-copied result
Reporting->>Exporter: Close()
Exporter->>Buffer: Lock, snapshot buffered results
Exporter->>PDF: Build report (header, summary, findings, details)
PDF->>PDF: Render pages (colors, truncation, code blocks)
PDF->>FS: Write PDF file to disk
FS-->>Exporter: File write success
Exporter-->>Reporting: Close returns
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/reporting/exporters/pdf/pdf_test.go (1)
114-119: Don’t ignoreExporterrors in the concurrency test.This can hide regressions and let the test pass when concurrent exports fail.
Proposed test hardening
var wg sync.WaitGroup + errCh := make(chan error, 30) for i := 0; i < 30; i++ { wg.Add(1) go func(index int) { defer wg.Done() event := buildEvent("concurrent.example.com", severity.Low) event.TemplateID = "tmpl-concurrent-" + time.Unix(int64(index), 0).UTC().Format("150405") - _ = exporter.Export(event) + errCh <- exporter.Export(event) }(i) } wg.Wait() + close(errCh) + for err := range errCh { + require.NoError(t, err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reporting/exporters/pdf/pdf_test.go` around lines 114 - 119, The goroutine in the concurrency test is ignoring the result of exporter.Export which can hide failures; modify the test to capture and assert the Export error instead of discarding it: create a buffered errs channel (or slice protected by mutex), have the goroutine send the error returned by exporter.Export(event) to that channel (reference the anonymous goroutine, exporter.Export, buildEvent and event.TemplateID), wait for wg, close the channel and then iterate over collected errors and fail the test on any non-nil error (e.g. t.Fatalf or t.Errorf/require.NoError for each) so concurrent export failures surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/reporting/exporters/pdf/pdf.go`:
- Line 96: Remove the explicit disabling of PDF compression by deleting the call
to pdf.SetCompression(false) (the pdf.SetCompression call in pdf.go) so
compression remains enabled by default; locate where pdf.SetCompression(false)
is invoked (in the PDF exporter initialization) and remove that line to avoid
inflated report sizes in production.
---
Nitpick comments:
In `@pkg/reporting/exporters/pdf/pdf_test.go`:
- Around line 114-119: The goroutine in the concurrency test is ignoring the
result of exporter.Export which can hide failures; modify the test to capture
and assert the Export error instead of discarding it: create a buffered errs
channel (or slice protected by mutex), have the goroutine send the error
returned by exporter.Export(event) to that channel (reference the anonymous
goroutine, exporter.Export, buildEvent and event.TemplateID), wait for wg, close
the channel and then iterate over collected errors and fail the test on any
non-nil error (e.g. t.Fatalf or t.Errorf/require.NoError for each) so concurrent
export failures surface.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.modpkg/reporting/exporters/pdf/pdf.gopkg/reporting/exporters/pdf/pdf_test.gopkg/reporting/options.gopkg/reporting/reporting.go
![neo-by-projectdiscovery-dev[bot]](https://avatars.githubusercontent.com/in/2878468?s=60&v=4){kind=small}
Author
|
Formatting-safe follow-up: Addressed the open review findings in commit c2544f5:
Validation run:
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/reporting/exporters/pdf/pdf.go (1)
54-58:⚠️ Potential issue | 🟠 MajorAbsolute output paths still bypass the path-hardening guard.
Line 54 blocks
..segments, but Line 57/Line 114 still allow absolute paths (for example/tmp/report.pdf), which can write outside the project working tree.Suggested fix
func New(options *Options) (*Exporter, error) { if options == nil { options = &Options{} } if options.File == "" { options.File = defaultFileName } - if hasParentPathSegment(options.File) { + cleaned := filepath.Clean(options.File) + if filepath.IsAbs(cleaned) { + return nil, fmt.Errorf("invalid PDF report path: absolute paths are not allowed") + } + if hasParentPathSegment(cleaned) { return nil, fmt.Errorf("invalid PDF report path: parent directory traversal is not allowed") } - options.File = filepath.Clean(options.File) + options.File = cleaned if dir := filepath.Dir(options.File); dir != "" && dir != "." { if err := os.MkdirAll(dir, 0755); err != nil { return nil, fmt.Errorf("could not create directory for PDF report: %w", err) } }Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reporting/exporters/pdf/pdf.go` around lines 54 - 58, The path-hardening misses absolute paths: update the validation around options.File (the branch that currently calls hasParentPathSegment(options.File) and then filepath.Clean/Dir) to also reject absolute paths by checking filepath.IsAbs(options.File) (and return an error like "absolute paths not allowed") before cleaning; apply the same absolute-path check to the other occurrence that validates options.File so both code paths (the initial validator and the later validator near the second use) consistently deny absolute output paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/reporting/exporters/pdf/pdf.go`:
- Around line 54-58: The path-hardening misses absolute paths: update the
validation around options.File (the branch that currently calls
hasParentPathSegment(options.File) and then filepath.Clean/Dir) to also reject
absolute paths by checking filepath.IsAbs(options.File) (and return an error
like "absolute paths not allowed") before cleaning; apply the same absolute-path
check to the other occurrence that validates options.File so both code paths
(the initial validator and the later validator near the second use) consistently
deny absolute output paths.
Author
|
Follow-up update (absolute-path hardening): Addressed in commit 4745b84:
Validation:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/reporting/exporters/pdf/pdf.go (1)
78-81: Consider implications of shallow copy for event storage.The struct assignment
eventCopy := *eventcreates a shallow copy. IfResultEventcontains pointer fields, slices, or maps (e.g.,ExtractedResults,Info.Reference), those remain shared with the original. If the caller modifies nested fields after callingExport(), the stored data could be affected.This is generally acceptable if callers follow the convention of not mutating events post-emission, but worth documenting or verifying against the project's usage patterns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reporting/exporters/pdf/pdf.go` around lines 78 - 81, The code uses a shallow copy (eventCopy := *event) before appending to e.results which leaves nested pointers/slices/maps shared; change the Export path that appends to e.results to perform a deep copy of ResultEvent (clone all nested mutable fields such as ExtractedResults, Info.Reference, any slices/maps or pointer sub-structs) before appending under e.mu.Lock, so stored events are independent of future mutations; locate the append site where e.results is modified and implement a deep-copy/clone helper for ResultEvent and use it when appending.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/reporting/exporters/pdf/pdf.go`:
- Around line 155-165: The loop that tallies severities iterates over results
but only checks result for nil before dereferencing result.Info; update the nil
checks to guard against a nil Info (and nested SeverityHolder) before accessing
result.Info.SeverityHolder.Severity in the counting loop (the same pattern
should be applied in writeFindingsTable and writeDetailedFindings): ensure each
loop first checks result != nil && result.Info != nil &&
result.Info.SeverityHolder != nil (or otherwise defensively handle missing
fields) before computing severity and updating the counts map (fall back to
"unknown" when any of those are nil).
---
Nitpick comments:
In `@pkg/reporting/exporters/pdf/pdf.go`:
- Around line 78-81: The code uses a shallow copy (eventCopy := *event) before
appending to e.results which leaves nested pointers/slices/maps shared; change
the Export path that appends to e.results to perform a deep copy of ResultEvent
(clone all nested mutable fields such as ExtractedResults, Info.Reference, any
slices/maps or pointer sub-structs) before appending under e.mu.Lock, so stored
events are independent of future mutations; locate the append site where
e.results is modified and implement a deep-copy/clone helper for ResultEvent and
use it when appending.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/reporting/exporters/pdf/pdf.gopkg/reporting/exporters/pdf/pdf_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/reporting/exporters/pdf/pdf_test.go
Author
|
Follow-up update: pushed one more hardening commit ( What changed:
Validation:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/reporting/exporters/pdf/pdf_test.go`:
- Around line 188-194: Test constructs a path that isn't guaranteed to be
OS-absolute on Windows; change the absolutePath setup in
pkg/reporting/exporters/pdf/pdf_test.go to produce a true absolute path by
wrapping the joined path with filepath.Abs (e.g. set absolutePath =
filepath.Abs(filepath.Join(string(os.PathSeparator), "tmp", "outside",
"report.pdf")) and handle the returned error before calling New). Update the
variable used in the New(&Options{File: absolutePath}) call so the test uses the
OS-absolute path.
In `@pkg/reporting/exporters/pdf/pdf.go`:
- Around line 48-70: The New function currently validates the caller-supplied
*Options and then stores the same pointer into Exporter, allowing TOCTOU
mutation; instead, make a defensive copy of Options at the start of New (e.g.,
allocate a new Options value and copy the fields from the incoming *options or
use a shallow copy and then normalize/clean fields on the copy), run all path
validation and normalization against that copy (including filepath.Clean, IsAbs,
hasParentPathSegment, and directory creation), and then store the copied Options
on the returned Exporter (Exporter.options = copiedOptions) so the original
caller pointer cannot modify validated fields later (ensure behavior of results
initialization remains unchanged).
Comment on lines
+188
to
+194
| absolutePath := filepath.Join(string(os.PathSeparator), "tmp", "outside", "report.pdf") | ||
|
|
||
| _, err := New(&Options{File: absolutePath}) | ||
|
|
||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "absolute path") | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual test file to see the exact code
cat -n pkg/reporting/exporters/pdf/pdf_test.go | sed -n '180,210p'Repository: projectdiscovery/nuclei
Length of output: 1018
🏁 Script executed:
# Also check the pdf.go file to understand what validation it does
cat -n pkg/reporting/exporters/pdf/pdf.go | head -100Repository: projectdiscovery/nuclei
Length of output: 3287
🌐 Web query:
filepath.IsAbs behavior Windows "\tmp\report.pdf" vs Unix cross-platform Go path validation
💡 Result:
What filepath.IsAbs does with "\tmp\report.pdf" on Windows vs Unix
- On Unix-like systems:
filepath.IsAbs("/tmp/report.pdf") == truebecause paths starting with/are absolute. (pkg.go.dev) - On Windows:
filepath.IsAbs("\\tmp\\report.pdf") == false(and likewise"/tmp/report.pdf" == false) because an absolute Windows path normally includes a volume (drive letter likeC:\...or a UNC share like\\host\share\...). A leading slash/backslash without a volume is treated as not absolute by Go’s Windows rules. (stackoverflow.com)
Windows examples that are absolute to filepath.IsAbs
C:\tmp\report.pdf→true(stackoverflow.com)\\server\share\report.pdf(UNC) →true(stackoverflow.com)
Cross-platform path validation guidance in Go
- Use
path/filepathfor OS-native filesystem paths; thepathpackage is for forward-slash paths like URLs and will mis-handle Windows paths. (stackoverflow.com) - If you need to validate “absolute on either Windows or Unix” regardless of the current OS, you generally must implement an OS-agnostic check (e.g., accept either
^[A-Za-z]:[\\/]|^\\\\\\\\for Windows, or^/for Unix), becausefilepath.IsAbsis explicitly OS-dependent. (pkg.go.dev)
Citations:
- 1: https://pkg.go.dev/path/filepath
- 2: https://stackoverflow.com/questions/56913799/how-do-you-get-filepath-isabs-to-correctly-indentify-an-absolute-path?utm_source=openai
- 3: https://stackoverflow.com/questions/56913799/how-do-you-get-filepath-isabs-to-correctly-indentify-an-absolute-path?utm_source=openai
- 4: https://stackoverflow.com/questions/60483303/path-isabs-returning-incorrect-result-for-windows-paths?utm_source=openai
- 5: https://stackoverflow.com/questions/60483303/path-isabs-returning-incorrect-result-for-windows-paths?utm_source=openai
- 6: https://pkg.go.dev/path/filepath
Use filepath.Abs() to construct a truly absolute path in the absolute-path rejection test.
The test constructs \tmp\... style paths, which filepath.IsAbs does not recognize as absolute on Windows (only paths with a drive letter like C:\... are absolute on Windows). This causes the test to fail on Windows, defeating its purpose.
Suggested fix
func TestNewRejectsAbsolutePath(t *testing.T) {
- absolutePath := filepath.Join(string(os.PathSeparator), "tmp", "outside", "report.pdf")
+ absolutePath, err := filepath.Abs(filepath.Join("tmp", "outside", "report.pdf"))
+ require.NoError(t, err)
- _, err := New(&Options{File: absolutePath})
+ _, err = New(&Options{File: absolutePath})
require.Error(t, err)
require.Contains(t, err.Error(), "absolute path")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/reporting/exporters/pdf/pdf_test.go` around lines 188 - 194, Test
constructs a path that isn't guaranteed to be OS-absolute on Windows; change the
absolutePath setup in pkg/reporting/exporters/pdf/pdf_test.go to produce a true
absolute path by wrapping the joined path with filepath.Abs (e.g. set
absolutePath = filepath.Abs(filepath.Join(string(os.PathSeparator), "tmp",
"outside", "report.pdf")) and handle the returned error before calling New).
Update the variable used in the New(&Options{File: absolutePath}) call so the
test uses the OS-absolute path.
Comment on lines
+48
to
+70
| func New(options *Options) (*Exporter, error) { | ||
| if options == nil { | ||
| options = &Options{} | ||
| } | ||
| if options.File == "" { | ||
| options.File = defaultFileName | ||
| } | ||
| if filepath.IsAbs(options.File) { | ||
| return nil, fmt.Errorf("invalid PDF report path: absolute path is not allowed") | ||
| } | ||
| if hasParentPathSegment(options.File) { | ||
| return nil, fmt.Errorf("invalid PDF report path: parent directory traversal is not allowed") | ||
| } | ||
| options.File = filepath.Clean(options.File) | ||
| if dir := filepath.Dir(options.File); dir != "" && dir != "." { | ||
| if err := os.MkdirAll(dir, 0755); err != nil { | ||
| return nil, fmt.Errorf("could not create directory for PDF report: %w", err) | ||
| } | ||
| } | ||
| return &Exporter{ | ||
| options: options, | ||
| results: make([]*output.ResultEvent, 0), | ||
| }, nil |
Contributor
There was a problem hiding this comment.
Copy Options in New to prevent post-validation mutation (TOCTOU).
Line 68 stores the caller’s *Options directly. That allows options.File to be modified after validation and before Close, bypassing the hardened path checks and creating concurrency risk.
Suggested fix
func New(options *Options) (*Exporter, error) {
- if options == nil {
- options = &Options{}
- }
- if options.File == "" {
- options.File = defaultFileName
+ resolved := Options{}
+ if options != nil {
+ resolved = *options
+ }
+ if resolved.File == "" {
+ resolved.File = defaultFileName
}
- if filepath.IsAbs(options.File) {
+ if filepath.IsAbs(resolved.File) {
return nil, fmt.Errorf("invalid PDF report path: absolute path is not allowed")
}
- if hasParentPathSegment(options.File) {
+ if hasParentPathSegment(resolved.File) {
return nil, fmt.Errorf("invalid PDF report path: parent directory traversal is not allowed")
}
- options.File = filepath.Clean(options.File)
- if dir := filepath.Dir(options.File); dir != "" && dir != "." {
+ resolved.File = filepath.Clean(resolved.File)
+ if dir := filepath.Dir(resolved.File); dir != "" && dir != "." {
if err := os.MkdirAll(dir, 0755); err != nil {
return nil, fmt.Errorf("could not create directory for PDF report: %w", err)
}
}
return &Exporter{
- options: options,
+ options: &resolved,
results: make([]*output.ResultEvent, 0),
}, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/reporting/exporters/pdf/pdf.go` around lines 48 - 70, The New function
currently validates the caller-supplied *Options and then stores the same
pointer into Exporter, allowing TOCTOU mutation; instead, make a defensive copy
of Options at the start of New (e.g., allocate a new Options value and copy the
fields from the incoming *options or use a shallow copy and then normalize/clean
fields on the copy), run all path validation and normalization against that copy
(including filepath.Clean, IsAbs, hasParentPathSegment, and directory creation),
and then store the copied Options on the returned Exporter (Exporter.options =
copiedOptions) so the original caller pointer cannot modify validated fields
later (ensure behavior of results initialization remains unchanged).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
pdfexporter underpkg/reporting/exporters/pdfthat implements the existing reportingExporterinterfaceomit-rawsupportExportcalls by guarding shared state and snapshotting before renderyaml: pdf) and exporter registration inpkg/reporting/reporting.goProof
Executed locally:
Checklist
/claim #2063
Summary by CodeRabbit
New Features
Tests
Chores