Skip to content

Commit 17edb5b

Browse files
committed
Security audit: fix critical vulnerabilities
- Fix parameter order bugs in repository.go (prevented data corruption) - Fix path traversal in local.go with securePath validation - Fix FFmpeg filter injection in afd_analyzer.go with proper escaping - Fix SQL injection in quality_repository.go using parameterized query - Fix CSRF timing attack in security.go with constant-time comparison - Fix error message copy-paste bug in repository.go - Update vulnerable dependencies (x/oauth2, x/crypto)
1 parent 18c102d commit 17edb5b

File tree

7 files changed

+120
-26
lines changed

7 files changed

+120
-26
lines changed

go.mod

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ require (
2323
github.com/redis/go-redis/v9 v9.12.1
2424
github.com/rs/zerolog v1.32.0
2525
github.com/spf13/cobra v1.10.2
26-
golang.org/x/crypto v0.44.0
26+
golang.org/x/crypto v0.45.0
2727
google.golang.org/api v0.177.0
2828
)
2929

@@ -96,7 +96,7 @@ require (
9696
go.uber.org/atomic v1.7.0 // indirect
9797
golang.org/x/arch v0.8.0 // indirect
9898
golang.org/x/net v0.47.0 // indirect
99-
golang.org/x/oauth2 v0.19.0 // indirect
99+
golang.org/x/oauth2 v0.33.0 // indirect
100100
golang.org/x/sync v0.18.0 // indirect
101101
golang.org/x/sys v0.38.0 // indirect
102102
golang.org/x/text v0.31.0 // indirect

go.sum

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -272,8 +272,8 @@ golang.org/x/arch v0.8.0 h1:3wRIsP3pM4yUptoR96otTUOXI367OS0+c9eeRi9doIc=
272272
golang.org/x/arch v0.8.0/go.mod h1:FEVrYAQjsQXMVJ1nsMoVVXPZg6p2JE2mx8psSWTDQys=
273273
golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
274274
golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
275-
golang.org/x/crypto v0.44.0 h1:A97SsFvM3AIwEEmTBiaxPPTYpDC47w720rdiiUvgoAU=
276-
golang.org/x/crypto v0.44.0/go.mod h1:013i+Nw79BMiQiMsOPcVCB5ZIJbYkerPrGnOa00tvmc=
275+
golang.org/x/crypto v0.45.0 h1:jMBrvKuj23MTlT0bQEOBcAE0mjg8mK9RXFhRH6nyF3Q=
276+
golang.org/x/crypto v0.45.0/go.mod h1:XTGrrkGJve7CYK7J8PEww4aY7gM3qMCElcJQ8n8JdX4=
277277
golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
278278
golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=
279279
golang.org/x/lint v0.0.0-20190227174305-5b3e6a55c961/go.mod h1:wehouNa3lNwaWXcvxsM5YxQ5yQlVC4a0KAMCusXpPoU=
@@ -287,8 +287,8 @@ golang.org/x/net v0.0.0-20201110031124-69a78807bb2b/go.mod h1:sp8m0HH+o8qH0wwXwY
287287
golang.org/x/net v0.47.0 h1:Mx+4dIFzqraBXUugkia1OOvlD6LemFo1ALMHjrXDOhY=
288288
golang.org/x/net v0.47.0/go.mod h1:/jNxtkgq5yWUGYkaZGqo27cfGZ1c5Nen03aYrrKpVRU=
289289
golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
290-
golang.org/x/oauth2 v0.19.0 h1:9+E/EZBCbTLNrbN35fHv/a/d/mOBatymz1zbtQrXpIg=
291-
golang.org/x/oauth2 v0.19.0/go.mod h1:vYi7skDa1x015PmRRYZ7+s1cWyPgrPiSYRe4rnsexc8=
290+
golang.org/x/oauth2 v0.33.0 h1:4Q+qn+E5z8gPRJfmRy7C2gGG3T4jIprK6aSYgTXGRpo=
291+
golang.org/x/oauth2 v0.33.0/go.mod h1:lzm5WQJQwKZ3nwavOZ3IS5Aulzxi68dUSgRHujetwEA=
292292
golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
293293
golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
294294
golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=

internal/database/quality_repository.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -794,21 +794,24 @@ func (r *qualityRepository) GetQualityStatistics(ctx context.Context, filters Qu
794794

795795
// GetQualityTrends retrieves quality trends over time
796796
func (r *qualityRepository) GetQualityTrends(ctx context.Context, metricType quality.QualityMetricType, days int) ([]*QualityTrend, error) {
797+
// Use parameterized query with SQLite-compatible date arithmetic
798+
// The '-X days' modifier is passed as a parameter to prevent SQL injection
797799
query := `
798-
SELECT
800+
SELECT
799801
DATE(created_at) as date,
800802
AVG(overall_score) as average_score,
801803
COUNT(*) as sample_count
802804
FROM quality_metrics
803-
WHERE metric_type = $1
804-
AND created_at >= CURRENT_DATE - INTERVAL '%d days'
805+
WHERE metric_type = $1
806+
AND created_at >= DATE('now', $2)
805807
GROUP BY DATE(created_at)
806808
ORDER BY date DESC`
807809

808-
query = fmt.Sprintf(query, days)
810+
// Format the days parameter as SQLite modifier string (e.g., "-30 days")
811+
daysModifier := fmt.Sprintf("-%d days", days)
809812

810813
var trends []*QualityTrend
811-
err := r.db.SelectContext(ctx, &trends, query, metricType)
814+
err := r.db.SelectContext(ctx, &trends, query, metricType, daysModifier)
812815
if err != nil {
813816
return nil, err
814817
}

internal/database/repository.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ func (r *SQLiteRepository) UpdateAnalysisStatus(ctx context.Context, id uuid.UUI
180180
SET status = ?, error_msg = ?, updated_at = ?
181181
WHERE id = ?`
182182

183-
_, err := r.db.DB.ExecContext(ctx, query, id, status, errorMsg, time.Now())
183+
_, err := r.db.DB.ExecContext(ctx, query, status, errorMsg, time.Now(), id)
184184
if err != nil {
185185
return fmt.Errorf("failed to update analysis status: %w", err)
186186
}
@@ -195,7 +195,7 @@ func (r *SQLiteRepository) UpdateAnalysisLLMReport(ctx context.Context, id uuid.
195195
SET llm_report = ?, updated_at = ?
196196
WHERE id = ?`
197197

198-
_, err := r.db.DB.ExecContext(ctx, query, id, report, time.Now())
198+
_, err := r.db.DB.ExecContext(ctx, query, report, time.Now(), id)
199199
if err != nil {
200200
return fmt.Errorf("failed to update analysis LLM report: %w", err)
201201
}
@@ -801,7 +801,7 @@ func (r *SQLiteRepository) DeleteQualityComparison(ctx context.Context, id uuid.
801801
}
802802

803803
if rowsAffected == 0 {
804-
return fmt.Errorf("processing job not found")
804+
return fmt.Errorf("quality comparison not found")
805805
}
806806

807807
return nil
@@ -845,7 +845,7 @@ func (r *SQLiteRepository) UpdateProcessingJob(ctx context.Context, job *models.
845845
WHERE id = ?`
846846

847847
_, err := r.db.DB.ExecContext(ctx, query,
848-
job.ID, job.Status, job.StartedAt, job.CompletedAt, job.ErrorMsg, job.RetryCount)
848+
job.Status, job.StartedAt, job.CompletedAt, job.ErrorMsg, job.RetryCount, job.ID)
849849
return err
850850
}
851851

internal/ffmpeg/afd_analyzer.go

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,16 +306,40 @@ func (aa *AFDAnalyzer) detectAFDFromVideoCharacteristics(ctx context.Context, fi
306306
return nil
307307
}
308308

309+
// escapeFFmpegFilterPath escapes a file path for use in FFmpeg filter expressions.
310+
// FFmpeg filter syntax requires special characters to be escaped.
311+
// See: https://ffmpeg.org/ffmpeg-filters.html#Notes-on-filtergraph-escaping
312+
func escapeFFmpegFilterPath(path string) string {
313+
// Escape backslashes first (must be done before other escapes)
314+
escaped := strings.ReplaceAll(path, `\`, `\\`)
315+
// Escape single quotes
316+
escaped = strings.ReplaceAll(escaped, `'`, `'\''`)
317+
// Escape colons (used as option separator)
318+
escaped = strings.ReplaceAll(escaped, `:`, `\:`)
319+
// Escape commas (used as filter separator)
320+
escaped = strings.ReplaceAll(escaped, `,`, `\,`)
321+
// Escape brackets (used in filter chains)
322+
escaped = strings.ReplaceAll(escaped, `[`, `\[`)
323+
escaped = strings.ReplaceAll(escaped, `]`, `\]`)
324+
// Escape semicolons (used as filterchain separator)
325+
escaped = strings.ReplaceAll(escaped, `;`, `\;`)
326+
// Wrap in single quotes for additional safety
327+
return `'` + escaped + `'`
328+
}
329+
309330
// detectLetterboxing analyzes frames to detect letterboxing/pillarboxing
310331
func (aa *AFDAnalyzer) detectLetterboxing(ctx context.Context, filePath string, analysis *AFDAnalysis) error {
332+
// Escape the file path for FFmpeg filter expression to prevent injection
333+
escapedPath := escapeFFmpegFilterPath(filePath)
334+
311335
// Use ffprobe with cropdetect filter to detect black bars
312336
cmd := []string{
313337
aa.ffprobePath,
314338
"-v", "quiet",
315339
"-print_format", "json",
316340
"-show_entries", "frame_tags=lavfi.cropdetect.w,lavfi.cropdetect.h,lavfi.cropdetect.x,lavfi.cropdetect.y",
317341
"-f", "lavfi",
318-
"-i", fmt.Sprintf("movie=%s,cropdetect=24:16:0", filePath),
342+
"-i", fmt.Sprintf("movie=%s,cropdetect=24:16:0", escapedPath),
319343
"-frames:v", "10",
320344
}
321345

internal/middleware/security.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package middleware
22

33
import (
44
"crypto/rand"
5+
"crypto/subtle"
56
"encoding/hex"
67
"fmt"
78
"net/http"
@@ -186,7 +187,9 @@ func (sm *SecurityMiddleware) CSRF() gin.HandlerFunc {
186187
// Get expected token from session/context
187188
expectedToken := c.GetString("csrf_token")
188189

189-
if token == "" || expectedToken == "" || token != expectedToken {
190+
// Use constant-time comparison to prevent timing attacks
191+
tokenMatch := subtle.ConstantTimeCompare([]byte(token), []byte(expectedToken)) == 1
192+
if token == "" || expectedToken == "" || !tokenMatch {
190193
sm.logger.Warn().
191194
Str("path", c.Request.URL.Path).
192195
Str("ip", c.ClientIP()).

internal/storage/local.go

Lines changed: 73 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"io"
77
"os"
88
"path/filepath"
9+
"strings"
910
)
1011

1112
type LocalProvider struct {
@@ -19,18 +20,62 @@ func NewLocalProvider(cfg Config) (*LocalProvider, error) {
1920
basePath = "./storage"
2021
}
2122

22-
if err := os.MkdirAll(basePath, 0755); err != nil {
23+
// Get absolute path for base directory
24+
absBasePath, err := filepath.Abs(basePath)
25+
if err != nil {
26+
return nil, fmt.Errorf("failed to resolve base path: %w", err)
27+
}
28+
29+
if err := os.MkdirAll(absBasePath, 0755); err != nil {
2330
return nil, fmt.Errorf("failed to create storage directory: %w", err)
2431
}
2532

2633
return &LocalProvider{
27-
basePath: basePath,
34+
basePath: absBasePath,
2835
baseURL: cfg.BaseURL,
2936
}, nil
3037
}
3138

39+
// securePath validates and returns a safe file path within the base directory.
40+
// Returns an error if the path would escape the base directory.
41+
func (l *LocalProvider) securePath(key string) (string, error) {
42+
// Clean the key to remove any .. or other traversal attempts
43+
cleanKey := filepath.Clean(key)
44+
45+
// Join with base path
46+
filePath := filepath.Join(l.basePath, cleanKey)
47+
48+
// Get absolute path to resolve any remaining traversal
49+
absPath, err := filepath.Abs(filePath)
50+
if err != nil {
51+
return "", fmt.Errorf("failed to resolve path: %w", err)
52+
}
53+
54+
// Verify the resolved path is within the base directory
55+
// Use filepath.Clean on basePath to ensure consistent comparison
56+
if !strings.HasPrefix(absPath, l.basePath+string(filepath.Separator)) && absPath != l.basePath {
57+
return "", fmt.Errorf("path traversal detected: access denied")
58+
}
59+
60+
// Check for symlinks that might escape the base directory
61+
// Only check if the path exists (for read operations)
62+
if _, err := os.Lstat(absPath); err == nil {
63+
realPath, err := filepath.EvalSymlinks(absPath)
64+
if err == nil {
65+
if !strings.HasPrefix(realPath, l.basePath+string(filepath.Separator)) && realPath != l.basePath {
66+
return "", fmt.Errorf("symlink escape detected: access denied")
67+
}
68+
}
69+
}
70+
71+
return absPath, nil
72+
}
73+
3274
func (l *LocalProvider) Upload(ctx context.Context, key string, reader io.Reader, size int64) error {
33-
filePath := filepath.Join(l.basePath, key)
75+
filePath, err := l.securePath(key)
76+
if err != nil {
77+
return err
78+
}
3479

3580
if err := os.MkdirAll(filepath.Dir(filePath), 0755); err != nil {
3681
return fmt.Errorf("failed to create directory: %w", err)
@@ -50,7 +95,11 @@ func (l *LocalProvider) Upload(ctx context.Context, key string, reader io.Reader
5095
}
5196

5297
func (l *LocalProvider) Download(ctx context.Context, key string) (io.ReadCloser, error) {
53-
filePath := filepath.Join(l.basePath, key)
98+
filePath, err := l.securePath(key)
99+
if err != nil {
100+
return nil, err
101+
}
102+
54103
file, err := os.Open(filePath)
55104
if err != nil {
56105
return nil, fmt.Errorf("failed to open file: %w", err)
@@ -59,16 +108,24 @@ func (l *LocalProvider) Download(ctx context.Context, key string) (io.ReadCloser
59108
}
60109

61110
func (l *LocalProvider) Delete(ctx context.Context, key string) error {
62-
filePath := filepath.Join(l.basePath, key)
111+
filePath, err := l.securePath(key)
112+
if err != nil {
113+
return err
114+
}
115+
63116
if err := os.Remove(filePath); err != nil {
64117
return fmt.Errorf("failed to delete file: %w", err)
65118
}
66119
return nil
67120
}
68121

69122
func (l *LocalProvider) Exists(ctx context.Context, key string) (bool, error) {
70-
filePath := filepath.Join(l.basePath, key)
71-
_, err := os.Stat(filePath)
123+
filePath, err := l.securePath(key)
124+
if err != nil {
125+
return false, err
126+
}
127+
128+
_, err = os.Stat(filePath)
72129
if err != nil {
73130
if os.IsNotExist(err) {
74131
return false, nil
@@ -80,9 +137,16 @@ func (l *LocalProvider) Exists(ctx context.Context, key string) (bool, error) {
80137

81138
func (l *LocalProvider) GetURL(ctx context.Context, key string) (string, error) {
82139
if l.baseURL != "" {
83-
return fmt.Sprintf("%s/%s", l.baseURL, key), nil
140+
// Sanitize key for URL to prevent injection
141+
cleanKey := filepath.Clean(key)
142+
return fmt.Sprintf("%s/%s", l.baseURL, cleanKey), nil
143+
}
144+
145+
filePath, err := l.securePath(key)
146+
if err != nil {
147+
return "", err
84148
}
85-
return fmt.Sprintf("file://%s", filepath.Join(l.basePath, key)), nil
149+
return fmt.Sprintf("file://%s", filePath), nil
86150
}
87151

88152
func (l *LocalProvider) GetSignedURL(ctx context.Context, key string, expiration int64) (string, error) {

0 commit comments

Comments
 (0)