diff --git a/internal/ai/classify/labels_test.go b/internal/ai/classify/labels_test.go index f0ef28ace..59cdcc12a 100644 --- a/internal/ai/classify/labels_test.go +++ b/internal/ai/classify/labels_test.go @@ -100,7 +100,6 @@ func TestLabels_Names(t *testing.T) { assert.Equal(t, []string{"cat", "bird"}, labels.Names()) }) - t.Run("NilLabels", func(t *testing.T) { var labels Labels assert.Nil(t, labels.Names()) @@ -118,7 +117,6 @@ func TestLabels_Count(t *testing.T) { assert.Equal(t, 2, labels.Count()) }) - t.Run("NilLabels", func(t *testing.T) { var labels Labels assert.Equal(t, 0, labels.Count()) @@ -130,7 +128,6 @@ func TestLabels_String(t *testing.T) { labels := Labels{{Name: "cat"}, {Name: "dog"}, {Name: "bird"}} assert.Equal(t, "cat, dog, and bird", labels.String()) }) - t.Run("NoneForNil", func(t *testing.T) { var labels Labels assert.Equal(t, "none", labels.String()) diff --git a/internal/ai/face/children_test.go b/internal/ai/face/children_test.go index b2c603c86..89edc4e52 100644 --- a/internal/ai/face/children_test.go +++ b/internal/ai/face/children_test.go @@ -28,13 +28,11 @@ func TestChildren(t *testing.T) { assert.True(t, Children.Contains(samples[6])) // Dist: 0.29948242058231617 assert.False(t, Children.Contains(samples[7])) // Dist: 1.1922265126804392 }) - t.Run("Normalized", func(t *testing.T) { for _, cluster := range Children { assert.InDelta(t, 1.0, cluster.Embedding.Magnitude(), 1e-9) } }) - t.Run("Dist", func(t *testing.T) { inside := Children[0].Embedding assert.InDelta(t, 0.0, Children.Dist(inside), 1e-9) diff --git a/internal/ai/face/clusters_test.go b/internal/ai/face/clusters_test.go index 4d22303d6..d1b66043b 100644 --- a/internal/ai/face/clusters_test.go +++ b/internal/ai/face/clusters_test.go @@ -17,7 +17,6 @@ func TestClustersContains(t *testing.T) { assert.True(t, clusters.Contains(embedding)) }) - t.Run("OutsideRadius", func(t *testing.T) { clusters := Clusters{ {Radius: 0.4, Embedding: Embedding{0, 0}}, @@ -27,7 +26,6 @@ func TestClustersContains(t *testing.T) { assert.False(t, clusters.Contains(embedding)) }) - t.Run("DisabledClusterBackground", func(t *testing.T) { clusters := Clusters{ {Radius: 1, Embedding: Embedding{0, 0}, Disabled: true}, @@ -52,7 +50,6 @@ func TestClustersDist(t *testing.T) { assert.InDelta(t, math.Sqrt(0.05), dist, 1e-9) }) - t.Run("NoEnabledClusters", func(t *testing.T) { clusters := Clusters{ {Radius: 0.2, Embedding: Embedding{0, 0}, Disabled: true}, diff --git a/internal/ai/face/embedding_test.go b/internal/ai/face/embedding_test.go index f21a0acfb..8d836a4b4 100644 --- a/internal/ai/face/embedding_test.go +++ b/internal/ai/face/embedding_test.go @@ -40,7 +40,6 @@ func TestEmbedding_Dist(t *testing.T) { assert.InDelta(t, 5.0, a.Dist(b), 1e-9) assert.InDelta(t, 5.0, b.Dist(a), 1e-9) }) - t.Run("MismatchedLength", func(t *testing.T) { a := Embedding{0, 0} b := Embedding{1} diff --git a/internal/ai/face/embeddings.go b/internal/ai/face/embeddings.go index e54d186ac..87a16ca37 100644 --- a/internal/ai/face/embeddings.go +++ b/internal/ai/face/embeddings.go @@ -131,13 +131,19 @@ func EmbeddingsMidpoint(embeddings Embeddings) (result Embedding, radius float64 // Count embeddings. count = len(embeddings) + var first Embedding + for _, emb := range embeddings { + first = emb + break + } + // Only one embedding? if count == 1 { // Return embedding if there is only one. - return embeddings[0], 0.0, 1 + return first, 0.0, 1 } - dim := len(embeddings[0]) + dim := len(first) // No embedding values? if dim == 0 { diff --git a/internal/ai/face/embeddings_test.go b/internal/ai/face/embeddings_test.go index eb7978d07..90ef415c9 100644 --- a/internal/ai/face/embeddings_test.go +++ b/internal/ai/face/embeddings_test.go @@ -156,7 +156,6 @@ func TestBackgroundSamplesMidpoint(t *testing.T) { assert.InDelta(t, 1.01, radius, 1e-6) assert.Equal(t, 4, count) }) - t.Run("NormalizedResult", func(t *testing.T) { e := Embeddings{ Embedding{1, 0, 0, 0}, diff --git a/internal/ai/vision/README.md b/internal/ai/vision/README.md index 9b831b453..2cd862f62 100644 --- a/internal/ai/vision/README.md +++ b/internal/ai/vision/README.md @@ -106,7 +106,7 @@ Configures the endpoint URL, method, format, and authentication for [Ollama](oll | `FileScheme` | set by engine alias (`data` or `base64`) | Controls image transport. | | `Disabled` | `false` | Disable the endpoint without removing the model. | -> **Authentication:** All credentials and identifiers support `${ENV_VAR}` expansion. `Service.Key` sets `Authorization: Bearer `; `Username`/`Password` injects HTTP basic authentication into the service URI when it is not already present. When `Service.Key` is empty, PhotoPrism defaults to `OPENAI_API_KEY` (OpenAI engine) or `OLLAMA_API_KEY` (Ollama engine), also honoring their `_FILE` counterparts. +> **Authentication:** All credentials and identifiers support `${ENV_VAR}` expansion. `Service.Key` sets `Authorization: Bearer `; `Username`/`Password` injects HTTP basic authentication into the service URI when it is not already present. When `Service.Key` is empty, PhotoPrism defaults to `OPENAI_API_KEY` (OpenAI engine) or `OLLAMA_API_KEY` (Ollama engine), also honoring their `_FILE` counterparts. Key and schema file paths must reference readable regular files (directories are ignored/rejected). ### Field Behavior & Precedence diff --git a/internal/ai/vision/api_client.go b/internal/ai/vision/api_client.go index d6aea9db6..d015831e7 100644 --- a/internal/ai/vision/api_client.go +++ b/internal/ai/vision/api_client.go @@ -14,12 +14,15 @@ import ( "github.com/photoprism/photoprism/internal/ai/vision/ollama" "github.com/photoprism/photoprism/pkg/clean" "github.com/photoprism/photoprism/pkg/http/header" + "github.com/photoprism/photoprism/pkg/http/safe" ) // PerformApiRequest performs a Vision API request and returns the result. func PerformApiRequest(apiRequest *ApiRequest, uri, method, key string) (apiResponse *ApiResponse, err error) { if apiRequest == nil { return apiResponse, errors.New("api request is nil") + } else if err = validateApiRequestURL(uri); err != nil { + return apiResponse, err } data, jsonErr := apiRequest.JSON() @@ -51,6 +54,7 @@ func PerformApiRequest(apiRequest *ApiRequest, uri, method, key string) (apiResp } // Perform API request. + // #nosec G704 URI is validated by validateApiRequestURL before issuing the request. clientResp, clientErr := client.Do(req) if clientErr != nil { @@ -102,6 +106,12 @@ func PerformApiRequest(apiRequest *ApiRequest, uri, method, key string) (apiResp return apiResponse, nil } +// validateApiRequestURL checks that outbound API requests only use HTTP(S) URLs with a host. +func validateApiRequestURL(rawURL string) error { + _, err := safe.URL(rawURL) + return err +} + func decodeOllamaResponse(data []byte) (*ollama.Response, error) { resp := &ollama.Response{} dec := json.NewDecoder(bytes.NewReader(data)) diff --git a/internal/ai/vision/api_client_test.go b/internal/ai/vision/api_client_test.go index 0ebd5ff0d..f63353cef 100644 --- a/internal/ai/vision/api_client_test.go +++ b/internal/ai/vision/api_client_test.go @@ -186,3 +186,16 @@ func TestPerformApiRequestOpenAIHeaders(t *testing.T) { assert.NotNil(t, resp.Result.Caption) assert.Equal(t, "A scenic mountain view.", resp.Result.Caption.Text) } + +func TestValidateApiRequestURL(t *testing.T) { + t.Run("AcceptHttpAndHttps", func(t *testing.T) { + assert.NoError(t, validateApiRequestURL("http://localhost:1234/api")) + assert.NoError(t, validateApiRequestURL("https://api.example.com/v1")) + }) + t.Run("RejectUnsupportedScheme", func(t *testing.T) { + assert.Error(t, validateApiRequestURL("file:///tmp/payload.json")) + }) + t.Run("RejectMissingHost", func(t *testing.T) { + assert.Error(t, validateApiRequestURL("https:///v1")) + }) +} diff --git a/internal/ai/vision/api_request_test.go b/internal/ai/vision/api_request_test.go index e8effccfb..9136bf1c6 100644 --- a/internal/ai/vision/api_request_test.go +++ b/internal/ai/vision/api_request_test.go @@ -97,7 +97,6 @@ func TestApiRequestJSONThinkOmitempty(t *testing.T) { t.Fatalf("expected think field to be omitted, payload: %s", string(data)) } }) - t.Run("IncludeWhenSet", func(t *testing.T) { req := &ApiRequest{ Model: "gpt-oss:20b", @@ -119,7 +118,6 @@ func TestApiRequestJSONThinkOmitempty(t *testing.T) { t.Fatalf("expected think=low, got %#v", payload["think"]) } }) - t.Run("StringFalseSerializedAsBool", func(t *testing.T) { req := &ApiRequest{ Model: "qwen3-vl:4b", @@ -141,7 +139,6 @@ func TestApiRequestJSONThinkOmitempty(t *testing.T) { t.Fatalf("expected think=false bool, got %#v", payload["think"]) } }) - t.Run("StringTrueSerializedAsBool", func(t *testing.T) { req := &ApiRequest{ Model: "qwen3-vl:4b", diff --git a/internal/ai/vision/engine_ollama_test.go b/internal/ai/vision/engine_ollama_test.go index 24c894a9d..6d335ff32 100644 --- a/internal/ai/vision/engine_ollama_test.go +++ b/internal/ai/vision/engine_ollama_test.go @@ -28,7 +28,6 @@ func TestRegisterOllamaEngineDefaults(t *testing.T) { CaptionModel = originalCaptionModel registerOllamaEngineDefaults() }) - t.Run("SelfHosted", func(t *testing.T) { ensureEnvOnce = sync.Once{} CaptionModel = testCaptionModel.Clone() diff --git a/internal/ai/vision/labels_test.go b/internal/ai/vision/labels_test.go index 72877f1a2..e3aa2639f 100644 --- a/internal/ai/vision/labels_test.go +++ b/internal/ai/vision/labels_test.go @@ -79,7 +79,6 @@ func TestGenerateLabelsRequestShapingForStructuredOutputIdea(t *testing.T) { t.Cleanup(func() { Config = prevConfig }) - t.Run("OllamaUsesJsonFormatWithSchemaPromptInstructions", func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var req ApiRequest diff --git a/internal/ai/vision/model.go b/internal/ai/vision/model.go index ed0c27a24..2e7b7354f 100644 --- a/internal/ai/vision/model.go +++ b/internal/ai/vision/model.go @@ -3,6 +3,7 @@ package vision import ( "fmt" "os" + "path/filepath" "strings" "sync" @@ -519,15 +520,10 @@ func (m *Model) SchemaTemplate() string { if m.Type == ModelTypeLabels { if envFile := strings.TrimSpace(os.Getenv(labelSchemaEnvVar)); envFile != "" { - path := fs.Abs(envFile) - if path == "" { - path = envFile - } - // #nosec G304 path comes from validated config/env - if data, err := os.ReadFile(path); err != nil { - log.Warnf("vision: failed to read schema from %s (%s)", clean.Log(path), err) + if schemaFromFile, err := readSchemaFile(envFile); err != nil { + log.Warnf("vision: failed to read schema from %s (%s)", clean.Log(envFile), err) } else { - schemaText = string(data) + schemaText = schemaFromFile } } } @@ -537,15 +533,10 @@ func (m *Model) SchemaTemplate() string { } if schemaText == "" && strings.TrimSpace(m.SchemaFile) != "" { - path := fs.Abs(m.SchemaFile) - if path == "" { - path = m.SchemaFile - } - // #nosec G304 schema file path provided via config - if data, err := os.ReadFile(path); err != nil { - log.Warnf("vision: failed to read schema from %s (%s)", clean.Log(path), err) + if schemaFromFile, err := readSchemaFile(m.SchemaFile); err != nil { + log.Warnf("vision: failed to read schema from %s (%s)", clean.Log(m.SchemaFile), err) } else { - schemaText = string(data) + schemaText = schemaFromFile } } @@ -565,6 +556,32 @@ func (m *Model) SchemaTemplate() string { return m.schema } +// readSchemaFile resolves and reads a schema file path from config or env. +func readSchemaFile(filePath string) (string, error) { + path := fs.Abs(filePath) + if path == "" { + path = filePath + } + + path = filepath.Clean(path) + + if path == "" { + return "", fmt.Errorf("schema path is empty") + } + + if _, err := fs.StatFile(path); err != nil { + return "", err + } + + // #nosec G304,G703 schema path is validated with Clean + fs.StatFile and comes from trusted config/env. + data, err := os.ReadFile(path) + if err != nil { + return "", err + } + + return string(data), nil +} + func (m *Model) engineDefaults() EngineDefaults { if m == nil { return nil diff --git a/internal/ai/vision/model_test.go b/internal/ai/vision/model_test.go index dac5d5c61..2b90af333 100644 --- a/internal/ai/vision/model_test.go +++ b/internal/ai/vision/model_test.go @@ -15,6 +15,30 @@ import ( "github.com/photoprism/photoprism/pkg/http/scheme" ) +func TestReadSchemaFile(t *testing.T) { + t.Run("ReadsRegularFile", func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "schema.json") + if err := os.WriteFile(path, []byte(`{"type":"object"}`), 0o600); err != nil { + t.Fatalf("write schema file: %v", err) + } + + got, err := readSchemaFile(path) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if got != `{"type":"object"}` { + t.Fatalf("unexpected schema content: %s", got) + } + }) + t.Run("RejectsDirectory", func(t *testing.T) { + if _, err := readSchemaFile(t.TempDir()); err == nil { + t.Fatal("expected error for directory path") + } + }) +} + func TestModelGetOptionsDefaultsOllamaLabels(t *testing.T) { ollamaModel := "redule26/huihui_ai_qwen2.5-vl-7b-abliterated:latest" diff --git a/internal/ai/vision/ollama/transport_test.go b/internal/ai/vision/ollama/transport_test.go index af9be6b79..70162131f 100644 --- a/internal/ai/vision/ollama/transport_test.go +++ b/internal/ai/vision/ollama/transport_test.go @@ -11,28 +11,24 @@ func TestResponseErr(t *testing.T) { t.Fatalf("expected nil-response error, got %v", err) } }) - t.Run("HTTPErrorWithMessage", func(t *testing.T) { resp := &Response{Code: 429, Error: "too many requests"} if err := resp.Err(); err == nil || err.Error() != "too many requests" { t.Fatalf("expected message error, got %v", err) } }) - t.Run("HTTPErrorWithoutMessage", func(t *testing.T) { resp := &Response{Code: 500} if err := resp.Err(); err == nil || err.Error() != "error 500" { t.Fatalf("expected formatted error, got %v", err) } }) - t.Run("NoResult", func(t *testing.T) { resp := &Response{Code: 200} if err := resp.Err(); err == nil || err.Error() != "no result" { t.Fatalf("expected no-result error, got %v", err) } }) - t.Run("HasLabels", func(t *testing.T) { resp := &Response{ Code: 200, @@ -43,7 +39,6 @@ func TestResponseErr(t *testing.T) { t.Fatalf("unexpected error: %v", err) } }) - t.Run("HasCaption", func(t *testing.T) { resp := &Response{ Code: 200, diff --git a/internal/ai/vision/openai/transport_test.go b/internal/ai/vision/openai/transport_test.go index ac5b7a48d..da49f7bbe 100644 --- a/internal/ai/vision/openai/transport_test.go +++ b/internal/ai/vision/openai/transport_test.go @@ -33,7 +33,6 @@ func TestParseErrorMessage(t *testing.T) { t.Fatalf("expected message, got %q", msg) } }) - t.Run("returns empty string when error is missing", func(t *testing.T) { raw := []byte(`{"output":[]}`) if msg := ParseErrorMessage(raw); msg != "" { diff --git a/internal/ai/vision/vision_env.go b/internal/ai/vision/vision_env.go index f2a3369e0..33009fc0d 100644 --- a/internal/ai/vision/vision_env.go +++ b/internal/ai/vision/vision_env.go @@ -2,6 +2,7 @@ package vision import ( "os" + "path/filepath" "strings" "sync" @@ -52,7 +53,9 @@ func loadEnvKeyFromFile(envVar, fileVar string) { return } - // #nosec G304 path provided via env + filePath = filepath.Clean(filePath) + + // #nosec G304,G703 path is validated and intended for local secret file loading. if data, err := os.ReadFile(filePath); err == nil { if key := clean.Auth(string(data)); key != "" { _ = os.Setenv(envVar, key) diff --git a/internal/ai/vision/vision_env_test.go b/internal/ai/vision/vision_env_test.go index f391f37df..92b647f8a 100644 --- a/internal/ai/vision/vision_env_test.go +++ b/internal/ai/vision/vision_env_test.go @@ -60,4 +60,14 @@ func TestLoadEnvKeyFromFile(t *testing.T) { t.Fatalf("expected keep-env, got %q", got) } }) + t.Run("IgnoreDirectoryPath", func(t *testing.T) { + t.Setenv("TEST_KEY", "") + t.Setenv("TEST_KEY_FILE", t.TempDir()) + + loadEnvKeyFromFile("TEST_KEY", "TEST_KEY_FILE") + + if got := os.Getenv("TEST_KEY"); got != "" { + t.Fatalf("expected empty key, got %q", got) + } + }) } diff --git a/internal/api/cluster_health_test.go b/internal/api/cluster_health_test.go index 876565e9f..662fc1c5a 100644 --- a/internal/api/cluster_health_test.go +++ b/internal/api/cluster_health_test.go @@ -22,7 +22,6 @@ func TestClusterHealth(t *testing.T) { assert.Equal(t, header.CacheControlNoStore, r.Header().Get(header.CacheControl)) assert.Equal(t, "", r.Header().Get(header.AccessControlAllowOrigin)) }) - t.Run("FeatureDisabled", func(t *testing.T) { app, router, conf := NewApiTest() conf.Options().NodeRole = cluster.RoleInstance @@ -31,7 +30,6 @@ func TestClusterHealth(t *testing.T) { r := PerformRequest(app, http.MethodGet, "/api/v1/cluster/health") assert.Equal(t, http.StatusForbidden, r.Code) }) - t.Run("ClusterCIDRDenied", func(t *testing.T) { app, router, conf := NewApiTest() enablePortalAPIs(t, conf) @@ -45,7 +43,6 @@ func TestClusterHealth(t *testing.T) { assert.Equal(t, http.StatusUnauthorized, w.Code) }) - t.Run("ClusterCIDRAllowed", func(t *testing.T) { app, router, conf := NewApiTest() enablePortalAPIs(t, conf) diff --git a/internal/api/cluster_nodes_register_test.go b/internal/api/cluster_nodes_register_test.go index 8000ebae7..7ebfa5873 100644 --- a/internal/api/cluster_nodes_register_test.go +++ b/internal/api/cluster_nodes_register_test.go @@ -36,7 +36,6 @@ func TestClusterNodesRegister(t *testing.T) { r := PerformRequestWithBody(app, http.MethodPost, "/api/v1/cluster/nodes/register", `{"NodeName":"pp-node-01"}`) assert.Equal(t, http.StatusForbidden, r.Code) }) - t.Run("ExistingNodeMutationRequiresOAuthToken", func(t *testing.T) { app, router, conf := NewApiTest() enablePortalAPIs(t, conf) diff --git a/internal/auth/jwt/verifier.go b/internal/auth/jwt/verifier.go index 0b3346160..dcae1da62 100644 --- a/internal/auth/jwt/verifier.go +++ b/internal/auth/jwt/verifier.go @@ -433,6 +433,7 @@ func (v *Verifier) fetchJWKS(ctx context.Context, url, etag string) (*jwksFetchR req.Header.Set("If-None-Match", etag) } + // #nosec G704 JWKS URL is validated via config.SetJWKSUrl and verifier call paths. resp, err := v.httpClient.Do(req) if err != nil { diff --git a/internal/config/config_db_test.go b/internal/config/config_db_test.go index 15ffbb633..2182e55db 100644 --- a/internal/config/config_db_test.go +++ b/internal/config/config_db_test.go @@ -226,13 +226,11 @@ func TestShouldAutoRotateDatabase(t *testing.T) { conf.Options().DatabaseDriver = MySQL assert.False(t, conf.ShouldAutoRotateDatabase()) }) - t.Run("NonMySQLDriverFalse", func(t *testing.T) { conf := NewMinimalTestConfig(t.TempDir()) conf.Options().DatabaseDriver = SQLite3 assert.False(t, conf.ShouldAutoRotateDatabase()) }) - t.Run("MySQLMissingFieldsTrue", func(t *testing.T) { conf := NewMinimalTestConfig(t.TempDir()) conf.Options().DatabaseDriver = MySQL diff --git a/internal/config/config_vision_test.go b/internal/config/config_vision_test.go index 3ef6fb9f1..4345f0e10 100644 --- a/internal/config/config_vision_test.go +++ b/internal/config/config_vision_test.go @@ -98,7 +98,6 @@ func TestConfig_VisionModelShouldRun(t *testing.T) { t.Fatalf("expected false when classification disabled") } }) - t.Run("DetectNSFWDisabled", func(t *testing.T) { c := NewConfig(CliTestContext()) c.options.DetectNSFW = false @@ -107,7 +106,6 @@ func TestConfig_VisionModelShouldRun(t *testing.T) { t.Fatalf("expected false when detect nsfw disabled") } }) - t.Run("NilVisionConfig", func(t *testing.T) { c := NewConfig(CliTestContext()) withVisionConfig(t, nil) @@ -115,7 +113,6 @@ func TestConfig_VisionModelShouldRun(t *testing.T) { t.Fatalf("expected false when no vision config is loaded") } }) - t.Run("DelegatesToVisionConfig", func(t *testing.T) { c := NewConfig(CliTestContext()) withVisionConfig(t, vision.NewConfig()) @@ -126,7 +123,6 @@ func TestConfig_VisionModelShouldRun(t *testing.T) { t.Fatalf("expected labels model to run on index with defaults") } }) - t.Run("CustomLabelsRunAfterIndex", func(t *testing.T) { c := NewConfig(CliTestContext()) defaultModel := vision.NasnetModel.Clone() diff --git a/internal/config/customize/acl_test.go b/internal/config/customize/acl_test.go index 7303e5acc..ec89251ee 100644 --- a/internal/config/customize/acl_test.go +++ b/internal/config/customize/acl_test.go @@ -92,7 +92,6 @@ func TestSettings_ApplyACL(t *testing.T) { t.Logf("RoleVisitor: %#v", r) assert.Equal(t, expected, r.Features) }) - t.Run("RoleClient", func(t *testing.T) { s := NewDefaultSettings() diff --git a/internal/config/report_test.go b/internal/config/report_test.go index 1cd1097c4..139cdaf2c 100644 --- a/internal/config/report_test.go +++ b/internal/config/report_test.go @@ -210,7 +210,6 @@ func TestConfig_ReportThemeURLVisibility(t *testing.T) { assert.False(t, hasThemeURL) assert.Equal(t, -1, indexOf(rows, "theme-url")) }) - t.Run("PortalIncludesThemeURL", func(t *testing.T) { Features = Community conf := NewConfig(CliTestContext()) @@ -226,7 +225,6 @@ func TestConfig_ReportThemeURLVisibility(t *testing.T) { assert.Greater(t, indexOf(rows, "theme-url"), indexOf(rows, "default-theme")) assert.Less(t, indexOf(rows, "theme-url"), indexOf(rows, "places-locale")) }) - t.Run("ProIncludesThemeURL", func(t *testing.T) { Features = Pro conf := NewConfig(CliTestContext()) diff --git a/internal/entity/album_yaml.go b/internal/entity/album_yaml.go index 975347a49..c762fcc4c 100644 --- a/internal/entity/album_yaml.go +++ b/internal/entity/album_yaml.go @@ -124,15 +124,17 @@ func (m *Album) LoadFromYaml(fileName string) error { return fmt.Errorf("yaml filename is empty") } - data, err := os.ReadFile(fileName) + filePath := filepath.Clean(fileName) + if _, err := fs.StatFile(filePath); err != nil { + return err + } + + //nolint:gosec // G304: Path is normalized and validated above with fs.StatFile. + data, err := os.ReadFile(filePath) if err != nil { return err } - if err = yaml.Unmarshal(data, m); err != nil { - return err - } - - return nil + return yaml.Unmarshal(data, m) } diff --git a/internal/entity/auth_session_fixtures.go b/internal/entity/auth_session_fixtures.go index 630ff43d5..fc4cc64ee 100644 --- a/internal/entity/auth_session_fixtures.go +++ b/internal/entity/auth_session_fixtures.go @@ -25,6 +25,7 @@ func (m SessionMap) Pointer(name string) *Session { return &Session{} } +//nolint:gosec // G101: Deterministic fixture tokens for tests only. var SessionFixtures = SessionMap{ "alice": { authToken: "69be27ac5ca305b394046a83f6fda18167ca3d3f2dbe7ac0", diff --git a/internal/entity/auth_session_test.go b/internal/entity/auth_session_test.go index 67c37bfce..70a80553e 100644 --- a/internal/entity/auth_session_test.go +++ b/internal/entity/auth_session_test.go @@ -368,7 +368,6 @@ func TestSession_Save(t *testing.T) { m2 := FindSessionByRefID("sessxkkcxxxy") assert.Equal(t, "chris", m2.UserName) }) - t.Run("LongNumericAuthID", func(t *testing.T) { refID := rnd.RefID("ts") m := FindSessionByRefID(refID) @@ -1213,7 +1212,6 @@ func TestSession_SetUserScopeDefault(t *testing.T) { assert.Equal(t, user.UserUID, sess.UserUID) assert.Equal(t, user.UserName, sess.UserName) }) - t.Run("KeepsExistingScope", func(t *testing.T) { sess := &Session{AuthScope: "logs:*"} user := &User{UserUID: "u456", UserName: "admin", UserScope: "photos:view"} diff --git a/internal/entity/auth_tokens.go b/internal/entity/auth_tokens.go index 1935f6bd2..89b3fb66e 100644 --- a/internal/entity/auth_tokens.go +++ b/internal/entity/auth_tokens.go @@ -5,7 +5,7 @@ import ( ) // Reserved token names used for configuration and public access. -const TokenConfig = "__config__" +const TokenConfig = "__config__" //nolint:gosec // G101: Reserved token keyword, not a credential. const TokenPublic = "public" var PreviewToken = NewStringMap(Strings{}) diff --git a/internal/entity/auth_user_test.go b/internal/entity/auth_user_test.go index 9011ba686..9c4f93ba3 100644 --- a/internal/entity/auth_user_test.go +++ b/internal/entity/auth_user_test.go @@ -1856,7 +1856,6 @@ func TestUser_SetAuthID(t *testing.T) { assert.Equal(t, uuid, m.AuthID) assert.Equal(t, "", m.AuthIssuer) }) - t.Run("DupeAuthProviderAndID", func(t *testing.T) { m := UserFixtures.Get("guest") n := NewUser() @@ -2384,14 +2383,12 @@ func TestUser_ScopeHelpers(t *testing.T) { assert.False(t, u.HasScope()) assert.True(t, u.NoScope()) }) - t.Run("AnyScope", func(t *testing.T) { u := &User{UserScope: list.Any} assert.Equal(t, "*", u.Scope()) assert.False(t, u.HasScope()) assert.True(t, u.NoScope()) }) - t.Run("RestrictedScope", func(t *testing.T) { u := &User{UserScope: "Photos:View"} assert.Equal(t, "photos:view", u.Scope()) diff --git a/internal/entity/entity_tables.go b/internal/entity/entity_tables.go index 51ffb32e6..4eb6015b0 100644 --- a/internal/entity/entity_tables.go +++ b/internal/entity/entity_tables.go @@ -128,9 +128,13 @@ func (list Tables) Migrate(db *gorm.DB, opt migrate.Options) { if err := migrate.ConvertDBMSAuthIDDataTypes(db); err != nil { log.Errorf("migrate: could not apply dbms auth_id fix : %v", err) version.Error = err.Error() - version.Save(db) + if saveErr := version.Save(db); saveErr != nil { + log.Errorf("migrate: could not save dbms auth_id fix status: %v", saveErr) + } } else { - version.Migrated(db) + if migratedErr := version.Migrated(db); migratedErr != nil { + log.Errorf("migrate: could not persist dbms auth_id fix status: %v", migratedErr) + } log.Debug("migrate: DBMS AuthID fix migrated") } } else { diff --git a/internal/entity/face.go b/internal/entity/face.go index ac2326495..1f600d78b 100644 --- a/internal/entity/face.go +++ b/internal/entity/face.go @@ -1,7 +1,7 @@ package entity import ( - "crypto/sha1" + "crypto/sha1" //nolint:gosec // G505: Stable non-cryptographic face identifier hash. "encoding/base32" "encoding/json" "fmt" @@ -100,6 +100,7 @@ func (m *Face) SetEmbeddings(embeddings face.Embeddings) (err error) { return err } + //nolint:gosec // G401: Stable identifier hash; not used for security decisions. s := sha1.Sum(m.EmbeddingJSON) // Update Face ID, Kind, and reset match timestamp, diff --git a/internal/entity/link_fixtures.go b/internal/entity/link_fixtures.go index adbeb0c2c..aca5cc933 100644 --- a/internal/entity/link_fixtures.go +++ b/internal/entity/link_fixtures.go @@ -6,6 +6,7 @@ var date = time.Date(2050, 3, 6, 2, 6, 51, 0, time.UTC) type LinkMap map[string]Link +//nolint:gosec // G101: Deterministic fixture tokens for tests only. var LinkFixtures = LinkMap{ "1jxf3jfn2k": { LinkUID: "ss62xpryd1ob7gtf", diff --git a/internal/entity/passcode_json.go b/internal/entity/passcode_json.go index 0381bc645..92e9fcf90 100644 --- a/internal/entity/passcode_json.go +++ b/internal/entity/passcode_json.go @@ -12,7 +12,7 @@ func (m *Passcode) MarshalJSON() ([]byte, error) { return json.Marshal(&struct { UID string `json:"UID"` Type string `json:"Type"` - Secret string `json:"Secret"` + Secret string `json:"Secret"` //nolint:gosec // G117: Expected passcode secret response field. QRCode string `json:"QRCode"` RecoveryCode string `json:"RecoveryCode"` CreatedAt time.Time `json:"CreatedAt"` diff --git a/internal/entity/photo_label_test.go b/internal/entity/photo_label_test.go index e85fb05b9..52b77887a 100644 --- a/internal/entity/photo_label_test.go +++ b/internal/entity/photo_label_test.go @@ -99,13 +99,11 @@ func TestPhotoLabel_Update(t *testing.T) { _, found := photoLabelCache.Get(relation.CacheKey()) assert.False(t, found) }) - t.Run("NilPhotoLabel", func(t *testing.T) { var label *PhotoLabel err := label.Update("uncertainty", 0) assert.EqualError(t, err, "photo label must not be nil - you may have found a bug") }) - t.Run("MissingID", func(t *testing.T) { label := &PhotoLabel{PhotoID: 0, LabelID: 1} err := label.Update("uncertainty", 0) @@ -125,13 +123,11 @@ func TestPhotoLabel_Updates(t *testing.T) { _, found := photoLabelCache.Get(relation.CacheKey()) assert.False(t, found) }) - t.Run("NilPhotoLabel", func(t *testing.T) { var label *PhotoLabel err := label.Updates(&PhotoLabel{Uncertainty: 0}) assert.EqualError(t, err, "photo label must not be nil - you may have found a bug") }) - t.Run("MissingID", func(t *testing.T) { label := &PhotoLabel{PhotoID: 0, LabelID: 1} err := label.Updates(&PhotoLabel{Uncertainty: 0}) @@ -155,12 +151,10 @@ func TestPhotoLabel_HasID(t *testing.T) { var label *PhotoLabel assert.False(t, label.HasID()) }) - t.Run("Missing", func(t *testing.T) { label := &PhotoLabel{PhotoID: 1} assert.False(t, label.HasID()) }) - t.Run("Complete", func(t *testing.T) { label := &PhotoLabel{PhotoID: 1, LabelID: 2} assert.True(t, label.HasID()) diff --git a/internal/entity/photo_merge_test.go b/internal/entity/photo_merge_test.go index c109e5151..de9d44d8c 100644 --- a/internal/entity/photo_merge_test.go +++ b/internal/entity/photo_merge_test.go @@ -254,7 +254,6 @@ func TestPhoto_SyncMediaTypeFromFiles(t *testing.T) { assert.Equal(t, MediaVideo, refreshed.PhotoType) assert.Equal(t, SrcAuto, refreshed.TypeSrc) }) - t.Run("PreservesManualOverride", func(t *testing.T) { photo := NewPhoto(true) photo.PhotoUID = rnd.GenerateUID(PhotoUID) diff --git a/internal/entity/photo_yaml.go b/internal/entity/photo_yaml.go index 475464ae9..fc8f953e3 100644 --- a/internal/entity/photo_yaml.go +++ b/internal/entity/photo_yaml.go @@ -58,11 +58,7 @@ func (m *Photo) SaveAsYaml(fileName string) error { defer photoYamlMutex.Unlock() // Write YAML data to file. - if err = fs.WriteFile(fileName, data, fs.ModeFile); err != nil { - return err - } - - return nil + return fs.WriteFile(fileName, data, fs.ModeFile) } // YamlFileName returns both the absolute file path and the relative name for the YAML sidecar file, e.g. for logging. @@ -103,10 +99,10 @@ func (m *Photo) SaveSidecarYaml(originalsPath, sidecarPath string) error { if err = m.SaveAsYaml(fileName); err != nil { log.Warnf("photo: %s (%s %s)", err, action, clean.Log(relName)) return err - } else { - log.Debugf("photo: %sd sidecar file %s", action, clean.Log(relName)) } + log.Debugf("photo: %sd sidecar file %s", action, clean.Log(relName)) + return nil } @@ -118,7 +114,14 @@ func (m *Photo) LoadFromYaml(fileName string) error { return fmt.Errorf("yaml filename is empty") } - data, err := os.ReadFile(fileName) + filePath := filepath.Clean(fileName) + + if _, err := fs.StatFile(filePath); err != nil { + return err + } + + //nolint:gosec // G304: Path is normalized and validated above with fs.StatFile. + data, err := os.ReadFile(filePath) if err != nil { return err diff --git a/internal/entity/query/photo_test.go b/internal/entity/query/photo_test.go index f35c81835..eb603d057 100644 --- a/internal/entity/query/photo_test.go +++ b/internal/entity/query/photo_test.go @@ -92,7 +92,6 @@ func TestPhotoPreloadByUIDs(t *testing.T) { } assert.Greater(t, len(second.Labels), 0) }) - t.Run("Empty", func(t *testing.T) { photos, err := PhotoPreloadByUIDs(nil) if err != nil { diff --git a/internal/form/client.go b/internal/form/client.go index 498acfe3a..89cf3e95a 100644 --- a/internal/form/client.go +++ b/internal/form/client.go @@ -15,7 +15,7 @@ type Client struct { UserUID string `json:"UserUID,omitempty" yaml:"UserUID,omitempty"` UserName string `gorm:"size:64;index;" json:"UserName" yaml:"UserName,omitempty"` ClientID string `json:"ClientID,omitempty" yaml:"ClientID,omitempty"` - ClientSecret string `json:"ClientSecret,omitempty" yaml:"ClientSecret,omitempty"` + ClientSecret string `json:"ClientSecret,omitempty" yaml:"ClientSecret,omitempty"` //nolint:gosec // G117: Expected credential field. ClientName string `json:"ClientName,omitempty" yaml:"ClientName,omitempty"` ClientRole string `json:"ClientRole,omitempty" yaml:"ClientRole,omitempty"` AuthProvider string `json:"AuthProvider,omitempty" yaml:"AuthProvider,omitempty"` diff --git a/internal/form/link.go b/internal/form/link.go index 56e2fab9a..f9a5aa968 100644 --- a/internal/form/link.go +++ b/internal/form/link.go @@ -2,7 +2,7 @@ package form // Link represents a link sharing form. type Link struct { - Password string `json:"Password"` + Password string `json:"Password"` //nolint:gosec // G117: Expected user-supplied credential field. ShareSlug string `json:"Slug"` LinkToken string `json:"Token"` LinkExpires int `json:"Expires"` diff --git a/internal/form/login.go b/internal/form/login.go index 8e5affd55..f1e6baad6 100644 --- a/internal/form/login.go +++ b/internal/form/login.go @@ -8,6 +8,7 @@ import ( // Login represents a login form. type Login struct { Username string `json:"username,omitempty"` // The local Username or LDAP user principal name (UPN). + //nolint:gosec // G117: Expected user-supplied credential field. Password string `json:"password,omitempty"` // The user's Password. Code string `json:"code,omitempty"` // 2FA Verification Code (Passcodes). Token string `json:"token,omitempty"` // Share Token. diff --git a/internal/form/oauth_create_token.go b/internal/form/oauth_create_token.go index 1abd0f2b0..2dba7f0da 100644 --- a/internal/form/oauth_create_token.go +++ b/internal/form/oauth_create_token.go @@ -12,10 +12,10 @@ type OAuthCreateToken struct { GrantType authn.GrantType `form:"grant_type" json:"grant_type,omitempty"` ClientID string `form:"client_id" json:"client_id,omitempty"` ClientName string `form:"client_name" json:"client_name,omitempty"` - ClientSecret string `form:"client_secret" json:" client_secret,omitempty"` + ClientSecret string `form:"client_secret" json:" client_secret,omitempty"` //nolint:gosec // G117: OAuth client secret input. Username string `form:"username" json:"username,omitempty"` - Password string `form:"password" json:"password,omitempty"` - RefreshToken string `form:"refresh_token" json:"refresh_token,omitempty"` + Password string `form:"password" json:"password,omitempty"` //nolint:gosec // G117: Password grant credential input. + RefreshToken string `form:"refresh_token" json:"refresh_token,omitempty"` //nolint:gosec // G117: OAuth refresh token input. Code string `form:"code" json:"code,omitempty"` CodeVerifier string `form:"code_verifier" json:"code_verifier,omitempty"` RedirectURI string `form:"redirect_uri" json:"redirect_uri,omitempty"` diff --git a/internal/form/passcode.go b/internal/form/passcode.go index 3361cdb14..3a83fd607 100644 --- a/internal/form/passcode.go +++ b/internal/form/passcode.go @@ -8,7 +8,7 @@ import ( // Passcode represents a multi-factor authentication key setup form. type Passcode struct { Type string `form:"type" json:"type,omitempty"` - Password string `form:"password" json:"password,omitempty"` + Password string `form:"password" json:"password,omitempty"` //nolint:gosec // G117: Expected user-supplied credential field. Code string `form:"code" json:"code,omitempty"` } diff --git a/internal/form/serialize.go b/internal/form/serialize.go index 7e0f07177..2aa384aca 100644 --- a/internal/form/serialize.go +++ b/internal/form/serialize.go @@ -144,14 +144,11 @@ func Unserialize(f SearchForm, q string) (result error) { field.SetInt(int64(intValue)) } case uint, uint8, uint16, uint32, uint64: - if intValue, err := strconv.Atoi(stringValue); err != nil { + bitSize := field.Type().Bits() + if uintValue, err := strconv.ParseUint(stringValue, 10, int(bitSize)); err != nil { result = err } else { - if intValue < 0 { - result = fmt.Errorf("unsupported negative value for %s", formName) - } else { - field.SetUint(uint64(intValue)) - } + field.SetUint(uintValue) } case string: field.SetString(clean.SearchString(stringValue)) diff --git a/internal/form/serialize_test.go b/internal/form/serialize_test.go index 78fcbad6d..306172c35 100644 --- a/internal/form/serialize_test.go +++ b/internal/form/serialize_test.go @@ -114,3 +114,21 @@ func TestUnserialize(t *testing.T) { assert.Equal(t, 0, form.Count) } + +func TestUnserializeUnsignedValidation(t *testing.T) { + t.Run("RejectNegativeUnsigned", func(t *testing.T) { + form := &TestForm{} + + if err := Unserialize(form, "dist:-1"); err == nil { + t.Fatal("expected error for negative unsigned value") + } + }) + t.Run("RejectOverflowUnsigned", func(t *testing.T) { + form := &TestForm{} + + // uint32 max + 1 should fail for Diff. + if err := Unserialize(form, "diff:4294967296"); err == nil { + t.Fatal("expected overflow error for uint32 field") + } + }) +} diff --git a/internal/form/user.go b/internal/form/user.go index 68691c99e..ff1c3018e 100644 --- a/internal/form/user.go +++ b/internal/form/user.go @@ -26,7 +26,7 @@ type User struct { UserAttr string `json:"Attr,omitempty" yaml:"Attr,omitempty"` BasePath string `json:"BasePath,omitempty" yaml:"BasePath,omitempty"` UploadPath string `json:"UploadPath,omitempty" yaml:"UploadPath,omitempty"` - Password string `json:"Password,omitempty" yaml:"Password,omitempty"` + Password string `json:"Password,omitempty" yaml:"Password,omitempty"` //nolint:gosec // G117: Expected user credential field. DeletedAt *time.Time `json:"DeletedAt,omitempty" yaml:"DeletedAt,omitempty"` UserDetails *UserDetails `json:"Details,omitempty"` } diff --git a/internal/meta/json_exiftool.go b/internal/meta/json_exiftool.go index 0751cfd7d..4bb8b1c7c 100644 --- a/internal/meta/json_exiftool.go +++ b/internal/meta/json_exiftool.go @@ -133,8 +133,8 @@ func (data *Data) Exiftool(jsonData []byte, originalName string) (err error) { if uintVal := jsonValue.Uint(); uintVal > 0 { fieldValue.SetUint(uintVal) - } else if intVal := txt.Int64(jsonValue.String()); intVal > 0 { - fieldValue.SetUint(uint64(intVal)) + } else if intVal, parseErr := strconv.ParseUint(strings.TrimSpace(jsonValue.String()), 10, 64); parseErr == nil && intVal > 0 { + fieldValue.SetUint(intVal) } case []string: existing := fieldValue.Interface().([]string) diff --git a/internal/photoprism/batch/apply_labels_test.go b/internal/photoprism/batch/apply_labels_test.go index 01f0e9fdd..c5fdf063f 100644 --- a/internal/photoprism/batch/apply_labels_test.go +++ b/internal/photoprism/batch/apply_labels_test.go @@ -194,7 +194,6 @@ func TestApplyLabels(t *testing.T) { t.Errorf("expected label source %s, got %s", entity.SrcBatch, blocked.LabelSrc) } }) - t.Run("RemoveVisionLabelBlocksRelation", func(t *testing.T) { photo := entity.PhotoFixtures.Pointer("Photo15") label := entity.LabelFixtures.Get("landscape") @@ -232,7 +231,6 @@ func TestApplyLabels(t *testing.T) { t.Errorf("expected label source %s, got %s", entity.SrcBatch, updated.LabelSrc) } }) - t.Run("KeepHigherPriorityLabel", func(t *testing.T) { photo := entity.PhotoFixtures.Pointer("Photo16") label := entity.LabelFixtures.Get("flower") @@ -1119,7 +1117,6 @@ func TestApplyLabels(t *testing.T) { t.Errorf("expected label source %s, got %s", entity.SrcBatch, updatedLabel.LabelSrc) } }) - t.Run("InvalidPhotoReturnsError", func(t *testing.T) { labels := Items{ Items: []Item{ diff --git a/internal/photoprism/dl/cmd.go b/internal/photoprism/dl/cmd.go index c5e9eae7f..5758bbd8c 100644 --- a/internal/photoprism/dl/cmd.go +++ b/internal/photoprism/dl/cmd.go @@ -17,10 +17,7 @@ func ytDlpCommand(ctx context.Context, args []string) *exec.Cmd { force := os.Getenv("YTDLP_FORCE_SHELL") == "1" if force || isShellScript(bin) { - sh := os.Getenv("YTDLP_SHELL") - if sh == "" { - sh = "bash" - } + const sh = "bash" return exec.CommandContext(ctx, sh, append([]string{bin}, args...)...) // #nosec G204 command constructed from validated yt-dlp path and args } diff --git a/internal/photoprism/dl/file.go b/internal/photoprism/dl/file.go index 2778d1ea2..f3655164c 100644 --- a/internal/photoprism/dl/file.go +++ b/internal/photoprism/dl/file.go @@ -9,6 +9,8 @@ import ( "path/filepath" "strconv" "strings" + + "github.com/photoprism/photoprism/pkg/fs" ) // DownloadToFileWithOptions downloads media using yt-dlp into files on disk (no piping). @@ -26,6 +28,11 @@ func (result Metadata) DownloadToFileWithOptions( out := outTpl out = strings.ReplaceAll(out, "%(id)s", "abc") out = strings.ReplaceAll(out, "%(ext)s", "mp4") + if sanitizedOut, pathErr := sanitizeDownloadPath(out); pathErr != nil { + return nil, pathErr + } else { + out = sanitizedOut + } // #nosec G301 media download directory should be accessible to user if err := os.MkdirAll(filepath.Dir(out), 0o755); err != nil { return nil, err @@ -34,8 +41,8 @@ func (result Metadata) DownloadToFileWithOptions( if content == "" { content = "dummy" } - // #nosec G306 downloaded media files are intended to be user-readable - if err := os.WriteFile(out, []byte(content), 0o644); err != nil { + // Downloaded media files are intended to be user-readable. + if err := fs.WriteFile(out, []byte(content), fs.ModeFile); err != nil { return nil, err } return []string{out}, nil @@ -207,12 +214,19 @@ func (result Metadata) DownloadToFileWithOptions( if line == "" { continue } - // If relative, resolve against tempPath - if !filepath.IsAbs(line) { - line = filepath.Join(tempPath, line) + + downloadedPath, pathErr := sanitizeDownloadPath(line) + if pathErr != nil { + continue } - if _, statErr := os.Stat(line); statErr == nil { - files = append(files, line) + + // If relative, resolve against tempPath. + if !filepath.IsAbs(line) { + downloadedPath = filepath.Join(tempPath, downloadedPath) + } + + if _, statErr := fs.Stat(downloadedPath); statErr == nil { + files = append(files, downloadedPath) } } @@ -223,3 +237,22 @@ func (result Metadata) DownloadToFileWithOptions( return files, nil } + +// sanitizeDownloadPath normalizes and validates a file path parsed from user/test input. +func sanitizeDownloadPath(filePath string) (string, error) { + cleanPath := filepath.Clean(strings.TrimSpace(filePath)) + + switch cleanPath { + case "", ".", string(filepath.Separator): + return "", fmt.Errorf("invalid download path") + case "..": + return "", fmt.Errorf("invalid download path") + default: + parentPrefix := ".." + string(filepath.Separator) + if strings.HasPrefix(cleanPath, parentPrefix) { + return "", fmt.Errorf("invalid download path") + } + + return cleanPath, nil + } +} diff --git a/internal/photoprism/dl/info.go b/internal/photoprism/dl/info.go index f1fbe8a76..f30d9ac7c 100644 --- a/internal/photoprism/dl/info.go +++ b/internal/photoprism/dl/info.go @@ -11,6 +11,8 @@ import ( "os" "strconv" "strings" + + "github.com/photoprism/photoprism/pkg/http/safe" ) // Info youtube-dl info @@ -256,14 +258,14 @@ func infoFromURL( return Info{}, nil, fmt.Errorf("unknown error") } - get := func(url string) (*http.Response, error) { + get := func(rawURL string) (*http.Response, error) { c := http.DefaultClient if options.HttpClient != nil { c = options.HttpClient } - r, httpErr := http.NewRequest(http.MethodGet, url, nil) + r, httpErr := newExternalGetRequest(rawURL) if httpErr != nil { return nil, httpErr @@ -273,7 +275,7 @@ func infoFromURL( r.Header.Set(k, v) } - return c.Do(r) + return c.Do(r) // #nosec G704 URL is parsed and scheme-validated in newExternalGetRequest. } if options.DownloadThumbnail && info.Thumbnail != "" { @@ -347,3 +349,13 @@ func infoFromURL( return info, stdoutBuf.Bytes(), nil } + +// newExternalGetRequest creates a GET request for an externally provided URL after basic validation. +func newExternalGetRequest(rawURL string) (*http.Request, error) { + u, err := safe.URL(rawURL) + if err != nil { + return nil, err + } + + return http.NewRequest(http.MethodGet, u.String(), nil) +} diff --git a/internal/photoprism/dl/info_test.go b/internal/photoprism/dl/info_test.go new file mode 100644 index 000000000..27fbf50c2 --- /dev/null +++ b/internal/photoprism/dl/info_test.go @@ -0,0 +1,42 @@ +package dl + +import "testing" + +func TestNewExternalGetRequest(t *testing.T) { + t.Run("ValidHttps", func(t *testing.T) { + req, err := newExternalGetRequest("https://example.com/vision") + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if req.URL.Host != "example.com" { + t.Fatalf("expected host example.com, got %s", req.URL.Host) + } + }) + t.Run("RejectMissingHost", func(t *testing.T) { + if _, err := newExternalGetRequest("/relative/path"); err == nil { + t.Fatal("expected error for missing host") + } + }) + t.Run("RejectUnsupportedScheme", func(t *testing.T) { + if _, err := newExternalGetRequest("file:///tmp/secret"); err == nil { + t.Fatal("expected error for unsupported scheme") + } + }) +} + +func TestSanitizeDownloadPath(t *testing.T) { + t.Run("AcceptRelativePath", func(t *testing.T) { + got, err := sanitizeDownloadPath(" clips/test.mp4 ") + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if got != "clips/test.mp4" { + t.Fatalf("unexpected path: %s", got) + } + }) + t.Run("RejectParentTraversal", func(t *testing.T) { + if _, err := sanitizeDownloadPath("../secrets.txt"); err == nil { + t.Fatal("expected parent traversal error") + } + }) +} diff --git a/internal/photoprism/faces_match.go b/internal/photoprism/faces_match.go index 8a821ca57..ba5b1beb3 100644 --- a/internal/photoprism/faces_match.go +++ b/internal/photoprism/faces_match.go @@ -238,8 +238,8 @@ func (w *Faces) MatchFaces(faces entity.Faces, force bool, matchedBefore *time.T return result, nil } - max := query.CountMarkers(entity.MarkerFace) - processed := make(map[string]struct{}, max) + maxMarkers := query.CountMarkers(entity.MarkerFace) + processed := make(map[string]struct{}, maxMarkers) totalProcessed := 0 offset := 0 @@ -263,8 +263,8 @@ func (w *Faces) MatchFaces(faces entity.Faces, force bool, matchedBefore *time.T if force { offset += len(markers) - if offset >= max { - offset = max + if offset >= maxMarkers { + offset = maxMarkers } } @@ -350,7 +350,7 @@ func (w *Faces) MatchFaces(faces entity.Faces, force bool, matchedBefore *time.T result.Updated++ } - if selFace != nil && dist >= 0 { + if dist >= 0 { stat := stats[selFace] if stat == nil { stat = &faceMatchStats{} @@ -378,7 +378,7 @@ func (w *Faces) MatchFaces(faces entity.Faces, force bool, matchedBefore *time.T log.Debugf("faces: matched %s", english.Plural(totalProcessed, "marker", "markers")) - if totalProcessed >= max { + if totalProcessed >= maxMarkers { break } @@ -411,12 +411,11 @@ func minMarkerDistance(faceEmb face.Embedding, embeddings face.Embeddings) float func embeddingSignHash(values []float64) uint32 { var hash uint32 - limit := min(faceIndexHashDims, len(values)) + limit := min(min(len(values), faceIndexHashDims), 32) for i := 0; i < limit; i++ { - if values[i] >= 0 && i < 32 { - //nolint:gosec // shift count bounded by 32 bits. - hash |= 1 << uint32(i) + if values[i] >= 0 { + hash |= uint32(1) << i } } diff --git a/internal/photoprism/index_vision_test.go b/internal/photoprism/index_vision_test.go index 4c92dd309..d7e8c3a43 100644 --- a/internal/photoprism/index_vision_test.go +++ b/internal/photoprism/index_vision_test.go @@ -45,7 +45,6 @@ func TestIndexCaptionSource(t *testing.T) { require.NotNil(t, caption) assert.Equal(t, captionModel.GetSource(), caption.Source) }) - t.Run("CustomSource", func(t *testing.T) { originalSource := captionModel.GetSource() vision.SetCaptionFunc(func(files vision.Files, mediaSrc media.Src) (*vision.CaptionResult, *vision.Model, error) { @@ -93,7 +92,6 @@ func TestIndexLabelsSource(t *testing.T) { assert.NotEmpty(t, labels) assert.Equal(t, labelModel.GetSource(), captured) }) - t.Run("CustomSource", func(t *testing.T) { var captured string vision.SetLabelsFunc(func(files vision.Files, mediaSrc media.Src, src entity.Src) (classify.Labels, error) { diff --git a/internal/photoprism/mediafile.go b/internal/photoprism/mediafile.go index 7ebcaaa5b..3248c2b5a 100644 --- a/internal/photoprism/mediafile.go +++ b/internal/photoprism/mediafile.go @@ -1418,10 +1418,19 @@ func (m *MediaFile) ExceedsBytes(limit int64) (fileSize int64, err error) { case fileSize <= 0 || fileSize <= limit: return fileSize, nil default: - return fileSize, fmt.Errorf("%s exceeds file size limit (%s / %s)", clean.Log(m.RootRelName()), humanize.Bytes(uint64(fileSize)), humanize.Bytes(uint64(limit))) + return fileSize, fmt.Errorf("%s exceeds file size limit (%s / %s)", clean.Log(m.RootRelName()), humanize.Bytes(nonNegativeUint64(fileSize)), humanize.Bytes(nonNegativeUint64(limit))) } } +// nonNegativeUint64 converts a signed integer to uint64 without overflow from negative values. +func nonNegativeUint64(v int64) uint64 { + if v <= 0 { + return 0 + } + + return uint64(v) +} + // ExceedsResolution checks if an image in a natively supported format exceeds the configured resolution limit in megapixels. func (m *MediaFile) ExceedsResolution(limit int) (resolution int, err error) { switch { diff --git a/internal/photoprism/mediafile_vision_test.go b/internal/photoprism/mediafile_vision_test.go index 23abe4990..629d7fab9 100644 --- a/internal/photoprism/mediafile_vision_test.go +++ b/internal/photoprism/mediafile_vision_test.go @@ -49,7 +49,6 @@ func TestMediaFile_GenerateCaption(t *testing.T) { require.NotNil(t, caption) assert.Equal(t, captionModel.GetSource(), caption.Source) }) - t.Run("CustomSourceOverrides", func(t *testing.T) { vision.SetCaptionFunc(func(files vision.Files, mediaSrc media.Src) (*vision.CaptionResult, *vision.Model, error) { return &vision.CaptionResult{Text: "stub", Source: captionModel.GetSource()}, captionModel, nil @@ -60,7 +59,6 @@ func TestMediaFile_GenerateCaption(t *testing.T) { require.NotNil(t, caption) assert.Equal(t, entity.SrcManual, caption.Source) }) - t.Run("MissingModelReturnsError", func(t *testing.T) { vision.Config = &vision.ConfigValues{} vision.SetCaptionFunc(nil) @@ -95,7 +93,6 @@ func TestMediaFile_GenerateLabels(t *testing.T) { assert.NotEmpty(t, labels) assert.Equal(t, labelModel.GetSource(), captured) }) - t.Run("CustomSourceOverrides", func(t *testing.T) { var captured string vision.SetLabelsFunc(func(files vision.Files, mediaSrc media.Src, src entity.Src) (classify.Labels, error) { @@ -107,7 +104,6 @@ func TestMediaFile_GenerateLabels(t *testing.T) { assert.NotEmpty(t, labels) assert.Equal(t, entity.SrcManual, captured) }) - t.Run("MissingModel", func(t *testing.T) { vision.Config = &vision.ConfigValues{} vision.SetLabelsFunc(nil) @@ -128,7 +124,6 @@ func TestMediaFile_DetectNSFW(t *testing.T) { assert.True(t, mediaFile.DetectNSFW()) }) - t.Run("SafeContent", func(t *testing.T) { vision.SetNSFWFunc(func(files vision.Files, mediaSrc media.Src) ([]nsfw.Result, error) { return []nsfw.Result{{Neutral: 0.9}}, nil diff --git a/internal/service/cluster/provisioner/credentials.go b/internal/service/cluster/provisioner/credentials.go index b27bea3c3..39ee01596 100644 --- a/internal/service/cluster/provisioner/credentials.go +++ b/internal/service/cluster/provisioner/credentials.go @@ -18,7 +18,7 @@ type Credentials struct { Port int Name string User string - Password string + Password string //nolint:gosec // G117: Provisioned database password output. DSN string RotatedAt string } diff --git a/internal/service/cluster/request.go b/internal/service/cluster/request.go index 03b1a2f2a..12325d203 100644 --- a/internal/service/cluster/request.go +++ b/internal/service/cluster/request.go @@ -15,7 +15,7 @@ type RegisterRequest struct { AdvertiseUrl string `json:"AdvertiseUrl,omitempty"` SiteUrl string `json:"SiteUrl,omitempty"` ClientID string `json:"ClientID,omitempty"` - ClientSecret string `json:"ClientSecret,omitempty"` + ClientSecret string `json:"ClientSecret,omitempty"` //nolint:gosec // G117: Rotated OAuth client secret payload. RotateDatabase bool `json:"RotateDatabase,omitempty"` RotateSecret bool `json:"RotateSecret,omitempty"` } diff --git a/internal/service/cluster/response.go b/internal/service/cluster/response.go index 87ad83cd4..45cd257b6 100644 --- a/internal/service/cluster/response.go +++ b/internal/service/cluster/response.go @@ -58,7 +58,7 @@ type MetricsResponse struct { // RegisterSecrets contains newly issued or rotated node secrets. // swagger:model RegisterSecrets type RegisterSecrets struct { - ClientSecret string `json:"ClientSecret,omitempty"` + ClientSecret string `json:"ClientSecret,omitempty"` //nolint:gosec // G117: Rotated OAuth client secret payload. RotatedAt string `json:"RotatedAt,omitempty"` } @@ -70,7 +70,7 @@ type RegisterDatabase struct { Port int `json:"Port"` Name string `json:"Name"` User string `json:"User"` - Password string `json:"Password,omitempty"` + Password string `json:"Password,omitempty"` //nolint:gosec // G117: Provisioned database password payload. DSN string `json:"DSN,omitempty"` RotatedAt string `json:"RotatedAt,omitempty"` } diff --git a/internal/service/cluster/roles_test.go b/internal/service/cluster/roles_test.go index 6c503f113..1ab65aece 100644 --- a/internal/service/cluster/roles_test.go +++ b/internal/service/cluster/roles_test.go @@ -10,19 +10,15 @@ func TestNormalizeNodeRole(t *testing.T) { t.Run("Instance", func(t *testing.T) { assert.Equal(t, RoleInstance, NormalizeNodeRole("instance")) }) - t.Run("LegacyAliasAppToInstance", func(t *testing.T) { assert.Equal(t, RoleInstance, NormalizeNodeRole(" app ")) }) - t.Run("Portal", func(t *testing.T) { assert.Equal(t, RolePortal, NormalizeNodeRole("portal")) }) - t.Run("Service", func(t *testing.T) { assert.Equal(t, RoleService, NormalizeNodeRole("service")) }) - t.Run("Invalid", func(t *testing.T) { assert.Equal(t, NodeRole(""), NormalizeNodeRole("unknown")) }) diff --git a/internal/service/discover.go b/internal/service/discover.go index de35fe4df..9c0461344 100644 --- a/internal/service/discover.go +++ b/internal/service/discover.go @@ -6,6 +6,7 @@ import ( "net/url" "strings" + "github.com/photoprism/photoprism/pkg/http/safe" "github.com/photoprism/photoprism/pkg/txt" ) @@ -62,8 +63,13 @@ func Discover(rawUrl, user, pass, servicesCIDR string) (result Account, err erro u.Scheme = strings.ToLower(u.Scheme) } - if u.Scheme != "http" && u.Scheme != "https" { - return result, errors.New("unsupported service URL scheme") + if validatedURL, validateErr := safe.URL(u.String()); validateErr != nil { + if errors.Is(validateErr, safe.ErrSchemeNotAllowed) { + return result, errors.New("unsupported service URL scheme") + } + return result, validateErr + } else { + u = validatedURL } for _, h := range Heuristics { diff --git a/internal/service/hub/config.go b/internal/service/hub/config.go index fd1c2336b..33d8ff944 100644 --- a/internal/service/hub/config.go +++ b/internal/service/hub/config.go @@ -42,7 +42,7 @@ type Config struct { Version string `json:"version" yaml:"-"` FileName string `json:"-" yaml:"-"` Key string `json:"key" yaml:"Key"` - Secret string `json:"secret" yaml:"Secret"` + Secret string `json:"secret" yaml:"Secret"` //nolint:gosec // G117: Encrypted Hub secret persisted in config. Session string `json:"session" yaml:"Session"` session *Session `yaml:"-"` sessionMu sync.Mutex `yaml:"-"` @@ -232,6 +232,8 @@ func (c *Config) ReSync(token string) (err error) { if endpointUrl == "" { log.Debugf("config: unable to obtain key for maps and places (service disabled)") return nil + } else if err = ValidateServiceURL(endpointUrl); err != nil { + return err } var method string @@ -268,6 +270,7 @@ func (c *Config) ReSync(token string) (err error) { // Send request. for range 3 { + // #nosec G704 endpointUrl is validated with ValidateServiceURL. r, err = client.Do(req) if err == nil { diff --git a/internal/service/hub/feedback.go b/internal/service/hub/feedback.go index 14af4c2cc..17e00ebec 100644 --- a/internal/service/hub/feedback.go +++ b/internal/service/hub/feedback.go @@ -22,7 +22,7 @@ type Feedback struct { UserName string `json:"UserName"` UserEmail string `json:"UserEmail"` UserAgent string `json:"UserAgent"` - ApiKey string `json:"ApiKey"` + ApiKey string `json:"ApiKey"` //nolint:gosec // G117: Hub API key payload field. ClientVersion string `json:"ClientVersion"` ClientSerial string `json:"ClientSerial"` ClientOS string `json:"ClientOS"` @@ -71,6 +71,8 @@ func (c *Config) SendFeedback(frm form.Feedback) (err error) { // Return if no endpoint URL is set. if endpointUrl == "" { return errors.New("unable to send feedback (service disabled)") + } else if err = ValidateServiceURL(endpointUrl); err != nil { + return err } method := http.MethodPost @@ -98,6 +100,7 @@ func (c *Config) SendFeedback(frm form.Feedback) (err error) { var r *http.Response for range 3 { + // #nosec G704 endpointUrl is validated with ValidateServiceURL. r, err = client.Do(req) if err == nil { diff --git a/internal/service/hub/places/request.go b/internal/service/hub/places/request.go index f560cb481..7b780394e 100644 --- a/internal/service/hub/places/request.go +++ b/internal/service/hub/places/request.go @@ -7,6 +7,7 @@ import ( "time" "github.com/photoprism/photoprism/pkg/http/header" + "github.com/photoprism/photoprism/pkg/http/safe" ) // GetRequest fetches the cell ID data from the service URL. @@ -16,6 +17,10 @@ func GetRequest(reqUrl string, locale string) (r *http.Response, err error) { // Log request URL. log.Tracef("places: sending request to %s", reqUrl) + if _, parseErr := safe.URL(reqUrl); parseErr != nil { + return r, fmt.Errorf("places: unsupported request URL scheme") + } + // Create GET request instance. req, err = http.NewRequest(http.MethodGet, reqUrl, nil) @@ -54,6 +59,7 @@ func GetRequest(reqUrl string, locale string) (r *http.Response, err error) { // Perform request. for i := 0; i < Retries; i++ { + // #nosec G704 reqUrl is parsed and scheme-validated above. r, err = client.Do(req) // Ok? diff --git a/internal/service/hub/places/request_test.go b/internal/service/hub/places/request_test.go new file mode 100644 index 000000000..0d3a894b9 --- /dev/null +++ b/internal/service/hub/places/request_test.go @@ -0,0 +1,72 @@ +package places + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" +) + +func TestGetRequest(t *testing.T) { + t.Run("RejectUnsupportedScheme", func(t *testing.T) { + if _, err := GetRequest("file:///tmp/location.json", "en"); err == nil { + t.Fatal("expected error for unsupported scheme") + } + }) + t.Run("Success", func(t *testing.T) { + prevRetries := Retries + prevDelay := RetryDelay + prevAgent := UserAgent + prevKey := Key + prevSecret := Secret + defer func() { + Retries = prevRetries + RetryDelay = prevDelay + UserAgent = prevAgent + Key = prevKey + Secret = prevSecret + }() + + Retries = 1 + RetryDelay = 0 + UserAgent = "PhotoPrism/TestSuite" + Key = "" + Secret = "" + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Accept-Language"); got != "de" { + t.Fatalf("expected locale header 'de', got %q", got) + } + + if got := r.Header.Get("User-Agent"); got != UserAgent { + t.Fatalf("expected user agent %q, got %q", UserAgent, got) + } + + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + resp, err := GetRequest(server.URL+"/v1/location/test", "de") + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if resp == nil { + t.Fatal("expected response") + } + _ = resp.Body.Close() + }) + t.Run("InvalidURL", func(t *testing.T) { + prevRetries := Retries + prevDelay := RetryDelay + defer func() { + Retries = prevRetries + RetryDelay = prevDelay + }() + + Retries = 1 + RetryDelay = 10 * time.Millisecond + if _, err := GetRequest("://invalid", "en"); err == nil { + t.Fatal("expected URL parse error") + } + }) +} diff --git a/internal/service/hub/request.go b/internal/service/hub/request.go index d73904686..580de24b3 100644 --- a/internal/service/hub/request.go +++ b/internal/service/hub/request.go @@ -14,7 +14,7 @@ type Request struct { ClientEnv string `json:"ClientEnv"` ClientOpt string `json:"ClientOpt"` PartnerID string `json:"PartnerID"` - ApiToken string `json:"ApiToken"` + ApiToken string `json:"ApiToken"` //nolint:gosec // G117: Hub API token payload field. } // ClientOpt hooks let tests and extensions append optional context information diff --git a/internal/service/hub/service.go b/internal/service/hub/service.go index c19d33f67..0668f396f 100644 --- a/internal/service/hub/service.go +++ b/internal/service/hub/service.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/photoprism/photoprism/pkg/clean" + "github.com/photoprism/photoprism/pkg/http/safe" ) // Default service base URLs for testing and production. @@ -68,6 +69,20 @@ func GetServiceHost() string { return u.Host } +// ValidateServiceURL ensures outbound Hub requests use an absolute HTTPS URL. +func ValidateServiceURL(rawURL string) error { + u, err := safe.URL(rawURL) + if err != nil { + return err + } + + if strings.ToLower(u.Scheme) != "https" { + return fmt.Errorf("unsupported service URL scheme") + } + + return nil +} + // SetBaseURL updates the Hub endpoint, ignoring inputs that are not HTTPS or // identical to the current value. Changes are logged so integration tests and // developers can trace the active target. diff --git a/internal/service/hub/service_test.go b/internal/service/hub/service_test.go index d8440f234..f6595d2e8 100644 --- a/internal/service/hub/service_test.go +++ b/internal/service/hub/service_test.go @@ -73,6 +73,18 @@ func TestSetBaseURLRejectsHTTP(t *testing.T) { assert.Equal(t, ProdBaseURL, GetServiceURL("")) } +func TestValidateServiceURL(t *testing.T) { + t.Run("ValidHttps", func(t *testing.T) { + assert.NoError(t, ValidateServiceURL("https://my.photoprism.app/v1/hello/demo")) + }) + t.Run("RejectInsecureScheme", func(t *testing.T) { + assert.Error(t, ValidateServiceURL("http://my.photoprism.app/v1/hello/demo")) + }) + t.Run("RejectMissingHost", func(t *testing.T) { + assert.Error(t, ValidateServiceURL("https:///v1/hello")) + }) +} + func TestApplyTestConfig(t *testing.T) { t.Run("DisableByDefault", func(t *testing.T) { cleanup := restoreBaseURL(t) @@ -85,7 +97,6 @@ func TestApplyTestConfig(t *testing.T) { assert.True(t, Disabled()) }) - t.Run("EnableTest", func(t *testing.T) { cleanup := restoreBaseURL(t) t.Cleanup(cleanup) @@ -98,7 +109,6 @@ func TestApplyTestConfig(t *testing.T) { assert.False(t, Disabled()) assert.Equal(t, TestBaseURL, GetServiceURL("")) }) - t.Run("EnableProd", func(t *testing.T) { cleanup := restoreBaseURL(t) t.Cleanup(cleanup) diff --git a/internal/service/request.go b/internal/service/request.go index 324643c43..313905f7f 100644 --- a/internal/service/request.go +++ b/internal/service/request.go @@ -3,13 +3,14 @@ package service import ( "net" "net/http" - "net/url" "time" + + "github.com/photoprism/photoprism/pkg/http/safe" ) // TestRequest makes a test request to the given URL and returns true if successful. func (h Heuristic) TestRequest(method, rawUrl string, allowedCIDRs []*net.IPNet) bool { - u, err := url.Parse(rawUrl) + u, err := safe.URL(rawUrl) if err != nil { return false @@ -43,6 +44,9 @@ func (h Heuristic) TestRequest(method, rawUrl string, allowedCIDRs []*net.IPNet) client := NewHTTPClient(30*time.Second, allowedCIDRs) // Send request to see if it fails. + // + // #nosec G704 Request URL was validated via ValidateURLHost and client + // transport enforces CIDR restrictions for direct/redirected connections. if resp, reqErr := client.Do(req); reqErr != nil { return false } else { diff --git a/internal/service/webdav/client.go b/internal/service/webdav/client.go index 519c40696..e9c5a5240 100644 --- a/internal/service/webdav/client.go +++ b/internal/service/webdav/client.go @@ -17,6 +17,7 @@ import ( "github.com/photoprism/photoprism/internal/service" "github.com/photoprism/photoprism/pkg/clean" "github.com/photoprism/photoprism/pkg/fs" + "github.com/photoprism/photoprism/pkg/http/safe" ) // Client represents a webdav client. @@ -31,13 +32,10 @@ type Client struct { // clientUrl returns the validated server url including username and password, if specified. func clientUrl(serverUrl, user, pass string) (*url.URL, error) { - result, err := url.Parse(serverUrl) + result, err := safe.URL(serverUrl) - // Check url. if err != nil { return nil, err - } else if result == nil { - return nil, fmt.Errorf("invalid server url") } // Set user and password if provided. @@ -279,12 +277,12 @@ func (c *Client) Download(src, dest string, force bool) (err error) { src = trimPath(src) // Skip if file already exists. - if _, err := os.Stat(dest); err == nil && !force { + if fs.Exists(dest) && !force { return fmt.Errorf("webdav: download skipped, %s already exists", clean.Log(dest)) } dir := path.Dir(dest) - dirInfo, err := os.Stat(dir) + dirInfo, err := fs.Stat(dir) if err != nil { // Create local storage path. @@ -347,7 +345,7 @@ func (c *Client) DownloadDir(src, dest string, recursive, force bool) (errs []er fileName := path.Join(dest, file.Abs) // Check if file already exists. - if _, err = os.Stat(fileName); err == nil { + if fs.Exists(fileName) { msg := fmt.Errorf("webdav: %s already exists", clean.Log(fileName)) log.Warn(msg) errs = append(errs, msg) diff --git a/internal/thumb/vips_init.go b/internal/thumb/vips_init.go index 0cad26708..979c1036d 100644 --- a/internal/thumb/vips_init.go +++ b/internal/thumb/vips_init.go @@ -49,7 +49,10 @@ func vipsInit() { }, vipsLogLevel()) // Start libvips. - vips.Startup(vipsConfig()) + if err := vips.Startup(vipsConfig()); err != nil { + vipsStarted = false + log.Errorf("vips: %s", err) + } } // vipsConfig provides the config for initializing libvips. diff --git a/pkg/dsn/dsn.go b/pkg/dsn/dsn.go index fe4aa9271..aaf4754b3 100644 --- a/pkg/dsn/dsn.go +++ b/pkg/dsn/dsn.go @@ -48,7 +48,7 @@ type DSN struct { DSN string Driver string User string - Password string + Password string //nolint:gosec // G117: DSN component intentionally stores password. Net string Server string Name string diff --git a/pkg/fs/fs.go b/pkg/fs/fs.go index d2be9936d..1a9321cb3 100644 --- a/pkg/fs/fs.go +++ b/pkg/fs/fs.go @@ -55,6 +55,19 @@ func Stat(filePath string) (os.FileInfo, error) { return os.Stat(filePath) } +// StatFile returns file info for regular files and errors on directories. +func StatFile(filePath string) (os.FileInfo, error) { + info, err := Stat(filePath) + if err != nil { + return nil, err + } + if info.IsDir() { + return nil, fmt.Errorf("%s is a directory", filePath) + } + + return info, nil +} + // SocketExists returns true if the specified socket exists and is not a regular file or directory. func SocketExists(socketName string) bool { if socketName == "" { diff --git a/pkg/fs/stat_test.go b/pkg/fs/stat_test.go index ed25e2991..c50331946 100644 --- a/pkg/fs/stat_test.go +++ b/pkg/fs/stat_test.go @@ -17,3 +17,18 @@ func TestStat(t *testing.T) { _, err = Stat("") assert.Error(t, err) } + +func TestStatFile(t *testing.T) { + // Success case. + info, err := StatFile("./testdata/test.jpg") + assert.NoError(t, err) + assert.False(t, info.IsDir()) + + // Error on directory path. + _, err = StatFile("./testdata") + assert.Error(t, err) + + // Error on empty path. + _, err = StatFile("") + assert.Error(t, err) +} diff --git a/pkg/fs/zip.go b/pkg/fs/zip.go index 8ee391d54..39f6d6d19 100644 --- a/pkg/fs/zip.go +++ b/pkg/fs/zip.go @@ -128,8 +128,17 @@ func Unzip(zipName, dir string, fileSizeLimit, totalSizeLimit int64) (files []st } // Skip directories like __OSX and potentially malicious file names containing "..". - if strings.HasPrefix(zipFile.Name, "__") || strings.Contains(zipFile.Name, "..") || - fileSizeLimit > 0 && zipFile.UncompressedSize64 > uint64(fileSizeLimit) { + skipEntry := strings.HasPrefix(zipFile.Name, "__") || strings.Contains(zipFile.Name, "..") + + if !skipEntry && fileSizeLimit > 0 { + if zipFile.UncompressedSize64 > uint64(math.MaxInt64) { + skipEntry = true + } else if int64(zipFile.UncompressedSize64) > fileSizeLimit { //nolint:gosec // bounded by MaxInt64 check above + skipEntry = true + } + } + + if skipEntry { skipped = append(skipped, zipFile.Name) continue } diff --git a/pkg/http/proxy/rewrite_test.go b/pkg/http/proxy/rewrite_test.go index 4a348fb11..7a46f4bed 100644 --- a/pkg/http/proxy/rewrite_test.go +++ b/pkg/http/proxy/rewrite_test.go @@ -73,7 +73,6 @@ func TestRewriteDestinationHost(t *testing.T) { assert.Equal(t, "http://instance.internal:2342/i/acme/import/dst.txt", req.Header.Get("Destination")) }) - t.Run("SkipsDifferentHost", func(t *testing.T) { req, err := http.NewRequest(http.MethodGet, "http://portal.example.com", nil) require.NoError(t, err) diff --git a/pkg/http/safe/download.go b/pkg/http/safe/download.go index 489a5cdc3..42ca04a52 100644 --- a/pkg/http/safe/download.go +++ b/pkg/http/safe/download.go @@ -7,7 +7,6 @@ import ( "net" "net/http" "net/http/httptrace" - "net/url" "os" "path/filepath" "strings" @@ -26,13 +25,10 @@ func Download(destPath, rawURL string, opt *Options) error { return err } - u, err := url.Parse(rawURL) + u, err := URL(rawURL) if err != nil { return err } - if !strings.EqualFold(u.Scheme, "http") && !strings.EqualFold(u.Scheme, "https") { - return ErrSchemeNotAllowed - } // Defaults w/ env overrides maxSize := defaultMaxSize @@ -134,6 +130,7 @@ func Download(destPath, rawURL string, opt *Options) error { } req = req.WithContext(httptrace.WithClientTrace(req.Context(), trace)) + // #nosec G704 URL is parsed and validated (scheme, host, optional private-IP checks). resp, err := client.Do(req) if err != nil { return err diff --git a/pkg/http/safe/url.go b/pkg/http/safe/url.go new file mode 100644 index 000000000..26d7e16f1 --- /dev/null +++ b/pkg/http/safe/url.go @@ -0,0 +1,30 @@ +package safe + +import ( + "errors" + "net/url" + "strings" +) + +var ( + // ErrURLHostRequired is returned when a URL does not include a hostname. + ErrURLHostRequired = errors.New("missing URL host") +) + +// URL parses a raw URL and ensures it uses HTTP(S) with a hostname. +func URL(rawURL string) (*url.URL, error) { + u, err := url.Parse(rawURL) + if err != nil { + return nil, err + } + + switch strings.ToLower(strings.TrimSpace(u.Scheme)) { + case "http", "https": + if strings.TrimSpace(u.Host) == "" { + return nil, ErrURLHostRequired + } + return u, nil + default: + return nil, ErrSchemeNotAllowed + } +} diff --git a/pkg/http/safe/url_test.go b/pkg/http/safe/url_test.go new file mode 100644 index 000000000..9804d0902 --- /dev/null +++ b/pkg/http/safe/url_test.go @@ -0,0 +1,37 @@ +package safe + +import ( + "errors" + "testing" +) + +func TestURL(t *testing.T) { + t.Run("AcceptHTTP", func(t *testing.T) { + u, err := URL("http://localhost:2342/api") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if u == nil || u.Host != "localhost:2342" { + t.Fatalf("unexpected parsed URL: %#v", u) + } + }) + t.Run("AcceptHTTPS", func(t *testing.T) { + u, err := URL("https://example.com/v1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if u == nil || u.Host != "example.com" { + t.Fatalf("unexpected parsed URL: %#v", u) + } + }) + t.Run("RejectMissingHost", func(t *testing.T) { + if _, err := URL("https:///v1"); !errors.Is(err, ErrURLHostRequired) { + t.Fatalf("expected ErrURLHostRequired, got %v", err) + } + }) + t.Run("RejectUnsupportedScheme", func(t *testing.T) { + if _, err := URL("file:///tmp/payload.json"); !errors.Is(err, ErrSchemeNotAllowed) { + t.Fatalf("expected ErrSchemeNotAllowed, got %v", err) + } + }) +} diff --git a/pkg/http/security/cmd/scanpathsgen/main.go b/pkg/http/security/cmd/scanpathsgen/main.go index 4ca2b274b..5b70e4740 100644 --- a/pkg/http/security/cmd/scanpathsgen/main.go +++ b/pkg/http/security/cmd/scanpathsgen/main.go @@ -11,6 +11,8 @@ import ( "path/filepath" "sort" "strings" + + "github.com/photoprism/photoprism/pkg/fs" ) // scanPathEntry stores the source line and hash for deterministic output ordering. @@ -45,10 +47,14 @@ func run() error { source := generateSource(entries) + // Repository output directory should be readable/executable for maintainers. + //nolint:gosec // G301: Non-secret generated source path. if err := os.MkdirAll(filepath.Dir(*outPath), 0o755); err != nil { return fmt.Errorf("create output directory: %w", err) } + // Generated Go source is committed to the repository and intentionally readable. + //nolint:gosec // G306: Non-secret generated source file. if err := os.WriteFile(*outPath, source, 0o644); err != nil { return fmt.Errorf("write output file: %w", err) } @@ -58,7 +64,7 @@ func run() error { // loadEntries loads, validates, hashes, and sorts scan path entries. func loadEntries(path string) ([]scanPathEntry, error) { - file, err := os.Open(path) + file, err := openInput(path) if err != nil { return nil, fmt.Errorf("open input file %s: %w", path, err) } @@ -112,6 +118,17 @@ func loadEntries(path string) ([]scanPathEntry, error) { return entries, nil } +// openInput opens a regular input file from a normalized path. +func openInput(path string) (*os.File, error) { + cleanPath := filepath.Clean(path) + if _, err := fs.StatFile(cleanPath); err != nil { + return nil, err + } + + //nolint:gosec // G304: User-provided path is normalized and validated above. + return os.Open(cleanPath) +} + // generateSource renders Go source code for hashed scan path lookup. func generateSource(entries []scanPathEntry) []byte { var out bytes.Buffer diff --git a/pkg/http/security/hash_test.go b/pkg/http/security/hash_test.go index e7a4fbcce..539091d00 100644 --- a/pkg/http/security/hash_test.go +++ b/pkg/http/security/hash_test.go @@ -17,7 +17,6 @@ func TestHashPath(t *testing.T) { t.Fatalf("expected deterministic hash for %q", path) } }) - t.Run("IsScanPath", func(t *testing.T) { if !IsScanPath("/wp-login.php") { t.Fatalf("expected scanner path to be detected") diff --git a/pkg/http/security/web_overlay_test.go b/pkg/http/security/web_overlay_test.go index 1fd461a68..c66fbb2f2 100644 --- a/pkg/http/security/web_overlay_test.go +++ b/pkg/http/security/web_overlay_test.go @@ -41,19 +41,16 @@ func TestOverlayRelativePath(t *testing.T) { assert.True(t, ok) assert.Equal(t, "assets/app.js", relPath) }) - t.Run("BasePathRoot", func(t *testing.T) { relPath, ok := OverlayRelativePath("/i/acme", "i/acme") assert.True(t, ok) assert.Equal(t, "", relPath) }) - t.Run("BasePathAsset", func(t *testing.T) { relPath, ok := OverlayRelativePath("/i/acme/assets/app.js", "i/acme") assert.True(t, ok) assert.Equal(t, "assets/app.js", relPath) }) - t.Run("OutsideBasePath", func(t *testing.T) { relPath, ok := OverlayRelativePath("/assets/app.js", "i/acme") assert.False(t, ok) @@ -103,14 +100,12 @@ func TestOverlayResolveFile(t *testing.T) { assert.NotEmpty(t, resolved) assert.FileExists(t, resolved) }) - t.Run("MissingFileReturnsFalse", func(t *testing.T) { webDir := t.TempDir() resolved, ok := OverlayResolveFile(webDir, "missing.txt") assert.False(t, ok) assert.Equal(t, "", resolved) }) - t.Run("SymlinkEscapeReturnsFalse", func(t *testing.T) { rootDir := t.TempDir() outsideDir := t.TempDir() diff --git a/pkg/list/ordered/map_test.go b/pkg/list/ordered/map_test.go index a53ecf6d5..891967063 100644 --- a/pkg/list/ordered/map_test.go +++ b/pkg/list/ordered/map_test.go @@ -24,41 +24,35 @@ func TestGet(t *testing.T) { _, ok := m.Get("foo") assert.False(t, ok) }) - t.Run("ReturnsNotOKIfNonStringKeyDoesntExist", func(t *testing.T) { m := ordered.NewMap[int, string]() _, ok := m.Get(123) assert.False(t, ok) }) - t.Run("ReturnsOKIfKeyExists", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "bar") _, ok := m.Get("foo") assert.True(t, ok) }) - t.Run("ReturnsValueForKey", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "bar") value, _ := m.Get("foo") assert.Equal(t, "bar", value) }) - t.Run("ReturnsDynamicValueForKey", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "baz") value, _ := m.Get("foo") assert.Equal(t, "baz", value) }) - t.Run("KeyDoesntExistOnNonEmptyMap", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "baz") _, ok := m.Get("bar") assert.False(t, ok) }) - t.Run("ValueForKeyDoesntExistOnNonEmptyMap", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "baz") @@ -73,26 +67,22 @@ func TestSet(t *testing.T) { ok := m.Set("foo", "bar") assert.True(t, ok) }) - t.Run("ReturnsTrueIfNonStringKeyIsNew", func(t *testing.T) { m := ordered.NewMap[int, string]() ok := m.Set(123, "bar") assert.True(t, ok) }) - t.Run("ValueCanBeNonString", func(t *testing.T) { m := ordered.NewMap[int, bool]() ok := m.Set(123, true) assert.True(t, ok) }) - t.Run("ReturnsFalseIfKeyIsNotNew", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "bar") ok := m.Set("foo", "bar") assert.False(t, ok) }) - t.Run("SetThreeDifferentKeys", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "bar") @@ -107,7 +97,6 @@ func TestReplaceKey(t *testing.T) { m := ordered.NewMap[string, string]() assert.False(t, m.ReplaceKey("foo", "bar")) }) - t.Run("ReturnsFalseIfNewKeyAlreadyExists", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "bar") @@ -115,7 +104,6 @@ func TestReplaceKey(t *testing.T) { assert.False(t, m.ReplaceKey("foo", "baz")) assert.Equal(t, []string{"foo", "baz"}, slices.Collect(m.Keys())) }) - t.Run("ReturnsTrueIfOnlyOriginalKeyExists", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "bar") @@ -136,7 +124,6 @@ func TestReplaceKey(t *testing.T) { _, ok = m.Get("foo") // original key assert.False(t, ok) }) - t.Run("KeyMaintainsOrderWhenReplaced", func(t *testing.T) { count := 100 // Build a larger map to help validate that the order is not coincidental. @@ -168,13 +155,11 @@ func TestLen(t *testing.T) { m := ordered.NewMap[string, string]() assert.Equal(t, 0, m.Len()) }) - t.Run("SingleElementIsLenOne", func(t *testing.T) { m := ordered.NewMap[int, bool]() m.Set(123, true) assert.Equal(t, 1, m.Len()) }) - t.Run("ThreeElements", func(t *testing.T) { m := ordered.NewMap[int, bool]() m.Set(1, true) @@ -189,13 +174,11 @@ func TestKeys(t *testing.T) { m := ordered.NewMap[int, bool]() assert.Empty(t, slices.Collect(m.Keys())) }) - t.Run("OneElement", func(t *testing.T) { m := ordered.NewMap[int, bool]() m.Set(1, true) assert.Equal(t, []int{1}, slices.Collect(m.Keys())) }) - t.Run("RetainsOrder", func(t *testing.T) { m := ordered.NewMap[int, bool]() for i := 1; i < 10; i++ { @@ -205,7 +188,6 @@ func TestKeys(t *testing.T) { []int{1, 2, 3, 4, 5, 6, 7, 8, 9}, slices.Collect(m.Keys())) }) - t.Run("ReplacingKeyDoesntChangeOrder", func(t *testing.T) { m := ordered.NewMap[string, bool]() m.Set("foo", true) @@ -215,7 +197,6 @@ func TestKeys(t *testing.T) { []string{"foo", "bar"}, slices.Collect(m.Keys())) }) - t.Run("KeysAfterDelete", func(t *testing.T) { m := ordered.NewMap[string, bool]() m.Set("foo", true) @@ -230,13 +211,11 @@ func TestDelete(t *testing.T) { m := ordered.NewMap[string, string]() assert.False(t, m.Delete("foo")) }) - t.Run("KeyDoesExist", func(t *testing.T) { m := ordered.NewMap[string, any]() m.Set("foo", nil) assert.True(t, m.Delete("foo")) }) - t.Run("KeyNoLongerExists", func(t *testing.T) { m := ordered.NewMap[string, any]() m.Set("foo", nil) @@ -244,7 +223,6 @@ func TestDelete(t *testing.T) { _, exists := m.Get("foo") assert.False(t, exists) }) - t.Run("KeyDeleteIsIsolated", func(t *testing.T) { m := ordered.NewMap[string, any]() m.Set("foo", nil) @@ -260,7 +238,6 @@ func TestMap_Front(t *testing.T) { m := ordered.NewMap[int, bool]() assert.Nil(t, m.Front()) }) - t.Run("NilOnEmptyMap", func(t *testing.T) { m := ordered.NewMap[int, bool]() m.Set(1, true) @@ -273,7 +250,6 @@ func TestMap_Back(t *testing.T) { m := ordered.NewMap[int, bool]() assert.Nil(t, m.Back()) }) - t.Run("NilOnEmptyMap", func(t *testing.T) { m := ordered.NewMap[int, bool]() m.Set(1, true) @@ -308,7 +284,6 @@ func TestGetElement(t *testing.T) { assert.Equal(t, []any{"foo", "bar"}, results) }) - t.Run("ElementForKeyDoesntExistOnNonEmptyMap", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "baz") @@ -373,7 +348,6 @@ func TestIterators(t *testing.T) { i++ } }) - t.Run("ReverseIterator", func(t *testing.T) { i := len(expected) - 1 for key, value := range m.AllFromBack() { @@ -389,13 +363,11 @@ func TestMap_Has(t *testing.T) { m := ordered.NewMap[string, string]() assert.False(t, m.Has("foo")) }) - t.Run("ReturnsTrueIfKeyExists", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "bar") assert.True(t, m.Has("foo")) }) - t.Run("KeyDoesNotExistAfterDelete", func(t *testing.T) { m := ordered.NewMap[string, string]() m.Set("foo", "bar") diff --git a/pkg/txt/float.go b/pkg/txt/float.go index f9865ef42..7bbe718c3 100644 --- a/pkg/txt/float.go +++ b/pkg/txt/float.go @@ -52,19 +52,29 @@ func FloatRange(s string, min, max float64) (start float64, end float64, err err valid := false p := 0 - v := [][]byte{make([]byte, 0, 20), make([]byte, 0, 20)} + startValue := make([]byte, 0, 20) + endValue := make([]byte, 0, 20) - for i, r := range s { - if r == 45 { + for i := 0; i < len(s); i++ { + c := s[i] + if c == '-' { if i == 0 || p == 1 { - v[p] = append(v[p], byte(r)) + if p == 0 { + startValue = append(startValue, c) + } else { + endValue = append(endValue, c) + } } else { p = 1 } } - if r == 46 || r >= 48 && r <= 57 { + if c == '.' || c >= '0' && c <= '9' { valid = true - v[p] = append(v[p], byte(r)) //nolint:gosec + if p == 0 { + startValue = append(startValue, c) + } else { + endValue = append(endValue, c) + } } } @@ -73,11 +83,11 @@ func FloatRange(s string, min, max float64) (start float64, end float64, err err } if p == 0 { - start = Float64(string(v[0])) + start = Float64(string(startValue)) end = start } else { - start = Float64(string(v[0])) - end = Float64(string(v[1])) + start = Float64(string(startValue)) + end = Float64(string(endValue)) } if start > max { diff --git a/pkg/txt/int.go b/pkg/txt/int.go index 60a770031..55683e787 100644 --- a/pkg/txt/int.go +++ b/pkg/txt/int.go @@ -55,19 +55,28 @@ func IntRange(s string, min, max int) (start int, end int, err error) { valid := false p := 0 - v := [][]byte{make([]byte, 0, 20), make([]byte, 0, 20)} + startValue := make([]byte, 0, 20) + endValue := make([]byte, 0, 20) - for i, r := range s { - if r == 45 { + for i := 0; i < len(s); i++ { + c := s[i] + if c == '-' { if i == 0 || p == 1 { - v[p] = append(v[p], byte(r)) + if p == 0 { + startValue = append(startValue, c) + } else { + endValue = append(endValue, c) + } } else { p = 1 } - } - if r == 46 || r >= 48 && r <= 57 { + } else if c == '.' || c >= '0' && c <= '9' { valid = true - v[p] = append(v[p], byte(r)) //nolint:gosec + if p == 0 { + startValue = append(startValue, c) + } else { + endValue = append(endValue, c) + } } } @@ -76,11 +85,11 @@ func IntRange(s string, min, max int) (start int, end int, err error) { } if p == 0 { - start = Int(string(v[0])) + start = Int(string(startValue)) end = start } else { - start = Int(string(v[0])) - end = Int(string(v[1])) + start = Int(string(startValue)) + end = Int(string(endValue)) } if start > max { diff --git a/pkg/txt/names.go b/pkg/txt/names.go index 7312c1153..74159dfe2 100644 --- a/pkg/txt/names.go +++ b/pkg/txt/names.go @@ -73,8 +73,9 @@ func JoinNames(names []string, shorten bool) (result string) { currShort := parts[0] if i > 0 && currShort == lastShort { + prev := i - 1 shortNames[i] = full - shortNames[i-1] = names[i-1] + shortNames[prev] = names[prev] } else { shortNames[i] = currShort }