Skip to content

Commit e2732da

Browse files
committed
fix: address PR review comments
- Fix gosec G304 by adding path containment validation and #nosec directive - Fix log message accuracy by tracking actual count of restored jobs - Add documentation comments to SaveConfig fields
1 parent 3c2437b commit e2732da

File tree

2 files changed

+32
-7
lines changed

2 files changed

+32
-7
lines changed

middlewares/restore.go

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ func RestoreHistory(saveFolder string, maxAge time.Duration, jobs []core.Job, lo
8888
}
8989

9090
restoredCount := 0
91+
restoredJobCount := 0
9192
for jobName, jobEntries := range entriesByJob {
9293
job, exists := jobsByName[jobName]
9394
if !exists {
@@ -112,10 +113,11 @@ func RestoreHistory(saveFolder string, maxAge time.Duration, jobs []core.Job, lo
112113
setter.SetLastRun(entry.Execution)
113114
restoredCount++
114115
}
116+
restoredJobCount++
115117
}
116118

117119
if restoredCount > 0 {
118-
logger.Noticef("Restored %d history entries for %d job(s) from saved files", restoredCount, len(entriesByJob))
120+
logger.Noticef("Restored %d history entries for %d job(s) from saved files", restoredCount, restoredJobCount)
119121
}
120122

121123
return nil
@@ -125,7 +127,13 @@ func RestoreHistory(saveFolder string, maxAge time.Duration, jobs []core.Job, lo
125127
func parseHistoryFiles(saveFolder string, cutoff time.Time, logger core.Logger) ([]*restoredEntry, error) {
126128
var entries []*restoredEntry
127129

128-
err := filepath.Walk(saveFolder, func(path string, info os.FileInfo, walkErr error) error {
130+
// Resolve save folder to absolute path for containment check
131+
absSaveFolder, err := filepath.Abs(saveFolder)
132+
if err != nil {
133+
return nil, fmt.Errorf("resolve save folder: %w", err)
134+
}
135+
136+
err = filepath.Walk(saveFolder, func(path string, info os.FileInfo, walkErr error) error {
129137
if walkErr != nil {
130138
return nil //nolint:nilerr // Intentionally skip inaccessible files
131139
}
@@ -140,8 +148,14 @@ func parseHistoryFiles(saveFolder string, cutoff time.Time, logger core.Logger)
140148
return nil
141149
}
142150

151+
// Verify path is within save folder (defense in depth for G304)
152+
absPath, absErr := filepath.Abs(path)
153+
if absErr != nil || !strings.HasPrefix(absPath, absSaveFolder) {
154+
return nil //nolint:nilerr // Intentionally skip invalid paths
155+
}
156+
143157
// Parse the JSON file
144-
entry, err := parseHistoryFile(path)
158+
entry, err := parseHistoryFile(absPath)
145159
if err != nil {
146160
logger.Debugf("Skipping invalid history file %q: %v", path, err)
147161
return nil
@@ -162,8 +176,9 @@ func parseHistoryFiles(saveFolder string, cutoff time.Time, logger core.Logger)
162176
}
163177

164178
// parseHistoryFile reads and parses a single JSON history file.
179+
// The path is validated by parseHistoryFiles to be within the save folder.
165180
func parseHistoryFile(path string) (*restoredEntry, error) {
166-
data, err := os.ReadFile(path)
181+
data, err := os.ReadFile(path) //#nosec G304 -- path is validated to be within save folder
167182
if err != nil {
168183
return nil, fmt.Errorf("read file: %w", err)
169184
}

middlewares/save.go

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,19 @@ import (
1212

1313
// SaveConfig configuration for the Save middleware
1414
type SaveConfig struct {
15-
SaveFolder string `gcfg:"save-folder" mapstructure:"save-folder"`
16-
SaveOnlyOnError bool `gcfg:"save-only-on-error" mapstructure:"save-only-on-error"`
17-
RestoreHistory *bool `gcfg:"restore-history" mapstructure:"restore-history"`
15+
// SaveFolder is the directory path where job execution logs and metadata are saved.
16+
// When configured, execution output (stdout, stderr) and context (JSON) are saved
17+
// after each job run. Leave empty to disable saving.
18+
SaveFolder string `gcfg:"save-folder" mapstructure:"save-folder"`
19+
// SaveOnlyOnError when true, only saves execution logs when a job fails.
20+
// Defaults to false (saves all executions).
21+
SaveOnlyOnError bool `gcfg:"save-only-on-error" mapstructure:"save-only-on-error"`
22+
// RestoreHistory controls whether previously saved execution history is restored on startup.
23+
// When nil (default), history restoration is enabled if SaveFolder is configured.
24+
// Set explicitly to false to disable restoration even when SaveFolder is set.
25+
RestoreHistory *bool `gcfg:"restore-history" mapstructure:"restore-history"`
26+
// RestoreHistoryMaxAge defines the maximum age of execution history to restore on startup.
27+
// Only executions newer than this duration are restored. Defaults to 24 hours.
1828
RestoreHistoryMaxAge time.Duration `gcfg:"restore-history-max-age" mapstructure:"restore-history-max-age"`
1929
}
2030

0 commit comments

Comments
 (0)