fix: require web credentials conditionally, and make validate actually validate (#775)

Closes #774, partially addresses #773.

## Strict validation demanded credentials for a feature you were not
using

A string field with no `default` tag was required unconditionally unless
it appeared on an allow-list, so switching on `enable-strict-validation`
produced this for a config with no web UI at all:

```
'web-password-hash': is required
'web-secret-key': is required
```

The operator most likely to enable validation is the one who wants their
config checked, and they were met with errors about a feature they do
not use. The practical answer was to leave validation off — which is how
the *other* validation gaps stayed invisible.

A field can now name the sibling flag that governs it. The two web
fields are required exactly when `web-auth-enabled` is set. A gate that
cannot be resolved keeps the field required: a mapping that has drifted
from the config should surface, not silently drop a check.

## `ofelia validate` did not validate

The checks sat behind `enable-strict-validation` (default `false`), so
the command whose only purpose is checking a config answered "looks
fine" without inspecting it. That flag is about whether the **daemon**
refuses to start; running `validate` is itself the request to have the
config checked. `cli/config.go` even points at the command — *"Use
'ofelia validate --config=…' for detailed validation"* — while the
command did not enable the detail.

`validate` now runs the validator regardless. When the flag is on,
`BuildFromFile` has already run it, so this only adds work in the
default case.

## What this does **not** fix — #773 stays open

The validator walks structs and skips maps. Every job lives in one
(`map[string]*RunJobConfig` and friends), so **no job is reachable by
the validator at all**. An unparsable schedule passes with the flag on
or off:

```
enable-strict-validation = true + web-address = definitely-not-an-address  →  reported
enable-strict-validation = true + schedule = not-a-schedule                →  not reported
```

Fixing that means traversing the job maps, which under the current *"no
`default` tag means required"* rule would demand nearly every job field
and reject configs that work today. It needs its own change and its own
decision about the required-heuristic, so it is not bundled here.

## A test that was asserting the defect

`TestE2E_ExitCode_StrictValidationFails`, added for exit codes in #771,
used a config whose only genuine failure was the web-credentials false
positive being fixed here — so it was pinning the bug rather than the
behaviour. It now fails on a global field that is actually checked, and
is renamed to say what it tests.

## Test plan

- [x] `go test ./...` — green, coverage 90.32%
- [x] `go test -race -tags=e2e ./e2e/...` — green
- [x] `golangci-lint run` incl. `--build-tags="e2e unix"` — 0 issues
- [x] `lefthook run pre-push` — exit 0
- [x] Both directions verified against the built binary: no web UI → no
demand; `web-auth-enabled = true` without a hash → still demanded

Author: CybotTM

cli/validate.go

@@ -10,6 +10,8 @@ import (
 	"os"
 
 	defaults "github.com/creasty/defaults"
+
+	"github.com/netresearch/ofelia/config"
 )
 
 // ValidateCommand validates the config file
@@ -39,6 +41,21 @@ func (c *ValidateCommand) Execute(_ []string) error {
 		}
 	}
 
+	// Validate regardless of enable-strict-validation. That flag decides
+	// whether the *daemon* refuses to start on a questionable config; running
+	// this command is itself the request to have the config checked, so
+	// answering "looks fine" because a runtime toggle is off would make the
+	// one command whose purpose is validation the one that does not validate.
+	//
+	// BuildFromFile has already run the same validator when the flag is on, so
+	// this only adds work in the default case.
+	if !conf.Global.EnableStrictValidation {
+		if err := config.NewConfigValidator(conf).Validate(); err != nil {
+			c.Logger.Error("ERROR")
+			return fmt.Errorf("configuration validation failed: %w", err)
+		}
+	}
+
 	applyConfigDefaults(conf)
 	out, err := json.MarshalIndent(conf, "", "  ")
 	if err != nil {

cli/validate_test.go

@@ -67,3 +67,40 @@ func TestValidateExecuteMissingFile(t *testing.T) {
 	err := cmd.Execute(nil)
 	assert.Error(t, err)
 }
+
+// TestValidateExecuteRunsValidatorWithoutStrictFlag pins that running validate
+// is itself the request to have the config checked. The checks used to sit
+// behind enable-strict-validation, which defaults to false, so the one command
+// whose purpose is validation reported success on a config it had not
+// inspected.
+//
+// The config below parses cleanly as INI and is only wrong semantically, so it
+// exercises the validator rather than the loader.
+func TestValidateExecuteRunsValidatorWithoutStrictFlag(t *testing.T) {
+	// Not parallel: modifies global os.Stdout which races with other tests.
+
+	configFile := filepath.Join(t.TempDir(), "config.ini")
+	content := `
+[global]
+web-address = definitely-not-an-address
+
+[job-exec "foo"]
+schedule = @every 10s
+command = echo "foo"
+`
+	require.NoError(t, os.WriteFile(configFile, []byte(content), 0o644))
+
+	r, w, _ := os.Pipe()
+	oldStdout := os.Stdout
+	os.Stdout = w
+	defer func() { os.Stdout = oldStdout }()
+
+	cmd := ValidateCommand{ConfigFile: configFile, Logger: test.NewTestLogger()}
+	err := cmd.Execute(nil)
+
+	w.Close()
+	_, _ = io.ReadAll(r)
+
+	require.Error(t, err, "an invalid web-address was accepted without the strict flag")
+	assert.Contains(t, err.Error(), "web-address")
+}

config/validator.go

@@ -231,8 +231,9 @@ func (cv *Validator2) validateStruct(v *Validator, obj any, path string) {
 			continue
 		}
 
-		// Validate based on field type and value
-		cv.validateField(v, field, fieldPath, defaultTag)
+		// Validate based on field type and value. The enclosing struct travels
+		// with it so a field can be required conditionally on a sibling.
+		cv.validateField(v, val, field, fieldPath, defaultTag)
 	}
 }
 
@@ -274,10 +275,12 @@ func resolveFieldPath(parentPath, fieldName, gcfgTag, mapstructureTag string) st
 }
 
 // validateField validates individual fields based on their type and tags
-func (cv *Validator2) validateField(v *Validator, field reflect.Value, path string, defaultTag string) {
+func (cv *Validator2) validateField(
+	v *Validator, parent, field reflect.Value, path string, defaultTag string,
+) {
 	switch field.Kind() {
 	case reflect.String:
-		cv.validateStringField(v, field, path, defaultTag)
+		cv.validateStringField(v, parent, field, path, defaultTag)
 	case reflect.Int, reflect.Int64:
 		cv.validateIntField(v, field, path)
 	case reflect.Slice:
@@ -296,7 +299,9 @@ func (cv *Validator2) validateField(v *Validator, field reflect.Value, path stri
 }
 
 // validateStringField validates string type fields
-func (cv *Validator2) validateStringField(v *Validator, field reflect.Value, path string, defaultTag string) {
+func (cv *Validator2) validateStringField(
+	v *Validator, parent, field reflect.Value, path string, defaultTag string,
+) {
 	str := field.String()
 
 	// Skip validation for fields with defaults when they're empty
@@ -305,7 +310,7 @@ func (cv *Validator2) validateStringField(v *Validator, field reflect.Value, pat
 	}
 
 	// Check for required fields
-	if defaultTag == "" && str == "" && !cv.isOptionalField(path) {
+	if defaultTag == "" && str == "" && !cv.isOptionalField(path) && cv.gateIsOpen(parent, path) {
 		v.ValidateRequired(path, str)
 	}
 
@@ -491,6 +496,50 @@ func (cv *Validator2) isOptionalField(path string) bool {
 	return false
 }
 
+// requiredWhen names, for a field that only means anything alongside a
+// feature, the sibling boolean that switches that feature on.
+//
+// Without this, a field carrying no `default` tag is required unconditionally,
+// which demanded web-UI credentials from every config that enabled strict
+// validation — including the ones with no web UI at all. That made strict
+// validation impractical to adopt, and an operator who cannot adopt it does
+// not get the checks it exists to provide.
+// #nosec G101 -- these are INI key names the validator matches on, not values
+var requiredWhen = map[string]string{
+	"web-password-hash": "web-auth-enabled",
+	"web-secret-key":    "web-auth-enabled",
+}
+
+// gateIsOpen reports whether a conditionally-required field is currently
+// required, i.e. whether the sibling flag that governs it is set. Fields with
+// no entry in requiredWhen are always required and answer true.
+//
+// A gate that cannot be found answers true as well: an unresolvable gate means
+// the mapping and the config have drifted apart, and demanding the field is
+// the safe direction — it surfaces, where silently dropping the check would
+// not.
+func (cv *Validator2) gateIsOpen(parent reflect.Value, path string) bool {
+	gate, conditional := requiredWhen[path]
+	if !conditional {
+		return true
+	}
+	if !parent.IsValid() || parent.Kind() != reflect.Struct {
+		return true
+	}
+
+	typ := parent.Type()
+	for fieldType := range typ.Fields() {
+		if !fieldType.IsExported() || fieldType.Type.Kind() != reflect.Bool {
+			continue
+		}
+		if fieldType.Tag.Get("gcfg") != gate && fieldType.Tag.Get("mapstructure") != gate {
+			continue
+		}
+		return parent.FieldByIndex(fieldType.Index).Bool()
+	}
+	return true
+}
+
 // isValidAddress checks if an address string is valid
 func (cv *Validator2) isValidAddress(addr string) bool {
 	// Allow formats like ":8080", "localhost:8080", "127.0.0.1:8080"

config/validator_boundary_test.go

@@ -432,7 +432,7 @@ func TestValidator2ValidateStringFieldDefaults(t *testing.T) {
 			v := NewValidator()
 
 			field := reflect.ValueOf(tt.value)
-			cv.validateStringField(v, field, tt.path, tt.defaultTag)
+			cv.validateStringField(v, reflect.Value{}, field, tt.path, tt.defaultTag)
 
 			if v.HasErrors() != tt.wantError {
 				t.Errorf("validateStringField(%q, %q, %q) hasError = %v, want %v",

config/validator_conditional_test.go

@@ -0,0 +1,128 @@
+// Copyright (c) 2025-2026 Netresearch DTT GmbH
+// SPDX-License-Identifier: MIT
+
+package config
+
+import (
+	"reflect"
+	"strings"
+	"testing"
+)
+
+// A string field carrying no `default` tag is required unless it is on the
+// optional list. That heuristic demanded web-UI credentials from every config
+// that switched strict validation on, including the ones with no web UI, so
+// the only way to run ofelia was to leave the checking off. These pin the
+// conditional rule that replaced it: the credentials are required exactly when
+// the flag that uses them is set.
+
+// gatedConfig mirrors the shape of the real global section: a boolean that
+// turns a feature on, and the fields that feature needs.
+type gatedConfig struct {
+	WebAuthEnabled  bool   `gcfg:"web-auth-enabled"   mapstructure:"web-auth-enabled"   default:"false"`
+	WebPasswordHash string `gcfg:"web-password-hash"  mapstructure:"web-password-hash"`
+	WebSecretKey    string `gcfg:"web-secret-key"     mapstructure:"web-secret-key"`
+}
+
+func TestConditionalRequired_NotDemandedWhenFeatureIsOff(t *testing.T) {
+	t.Parallel()
+
+	err := NewConfigValidator(&gatedConfig{WebAuthEnabled: false}).Validate()
+	if err != nil {
+		t.Errorf("web credentials were demanded with web auth off: %v", err)
+	}
+}
+
+func TestConditionalRequired_DemandedWhenFeatureIsOn(t *testing.T) {
+	t.Parallel()
+
+	err := NewConfigValidator(&gatedConfig{WebAuthEnabled: true}).Validate()
+	if err == nil {
+		t.Fatal("web auth is on with no password hash or secret key, expected an error")
+	}
+
+	for _, want := range []string{"web-password-hash", "web-secret-key"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("error %q does not mention the missing %s", err, want)
+		}
+	}
+}
+
+// TestConditionalRequired_SatisfiedWhenProvided closes the loop: with the
+// feature on and the fields filled in, nothing is reported.
+func TestConditionalRequired_SatisfiedWhenProvided(t *testing.T) {
+	t.Parallel()
+
+	err := NewConfigValidator(&gatedConfig{
+		WebAuthEnabled:  true,
+		WebPasswordHash: "$2a$12$abcdefghijklmnopqrstuv",
+		WebSecretKey:    "a-secret",
+	}).Validate()
+	if err != nil {
+		t.Errorf("a complete web-auth config was rejected: %v", err)
+	}
+}
+
+// TestConditionalRequired_UnknownGateStaysRequired pins the fallback. If the
+// mapping and the config drift apart so the gate cannot be found, the field
+// stays required — surfacing beats silently dropping a check.
+func TestConditionalRequired_UnknownGateStaysRequired(t *testing.T) {
+	t.Parallel()
+
+	// No web-auth-enabled field at all, so the gate is unresolvable.
+	type noGate struct {
+		WebSecretKey string `gcfg:"web-secret-key" mapstructure:"web-secret-key"`
+	}
+
+	if err := NewConfigValidator(&noGate{}).Validate(); err == nil {
+		t.Error("with no gate to consult the field should stay required, got no error")
+	}
+}
+
+// TestGateIsOpen_InvalidParentKeepsFieldRequired covers the guard for a caller
+// that has no enclosing struct to offer — the internal helpers are called that
+// way in tests. With nothing to consult, the field stays required, which is
+// the same safe direction as an unresolvable gate.
+func TestGateIsOpen_InvalidParentKeepsFieldRequired(t *testing.T) {
+	t.Parallel()
+
+	cv := NewConfigValidator(nil)
+	if !cv.gateIsOpen(reflect.Value{}, "web-secret-key") {
+		t.Error("with no parent to inspect the field should stay required")
+	}
+}
+
+// TestGateIsOpen_IgnoresMismatchedFields covers the skip inside the search: a
+// field is only the gate if it is a bool AND carries the expected key. A
+// string field named like the gate, or a bool with a different key, must not
+// be mistaken for it.
+func TestGateIsOpen_IgnoresMismatchedFields(t *testing.T) {
+	t.Parallel()
+
+	type decoys struct {
+		// Right key, wrong type.
+		WebAuthEnabled string `gcfg:"web-auth-enabled"`
+		// Right type, wrong key.
+		SomethingElse bool `gcfg:"some-other-flag"`
+	}
+
+	cv := NewConfigValidator(nil)
+	parent := reflect.ValueOf(decoys{WebAuthEnabled: "true", SomethingElse: true})
+
+	// Neither decoy qualifies, so the search finds no gate and the field stays
+	// required rather than being switched on by the wrong field.
+	if !cv.gateIsOpen(parent, "web-secret-key") {
+		t.Error("a string field and an unrelated bool were treated as the gate")
+	}
+}
+
+// TestGateIsOpen_UnconditionalFieldsAlwaysRequired pins the common case: a
+// field with no entry in requiredWhen is not gated at all.
+func TestGateIsOpen_UnconditionalFieldsAlwaysRequired(t *testing.T) {
+	t.Parallel()
+
+	cv := NewConfigValidator(nil)
+	if !cv.gateIsOpen(reflect.ValueOf(gatedConfig{}), "web-address") {
+		t.Error("an ungated field reported as not required")
+	}
+}

config/validator_mutation_test.go

@@ -454,7 +454,7 @@ func TestValidateStringField_DefaultTagConditions(t *testing.T) {
 			t.Parallel()
 			v := NewValidator()
 			field := reflect.ValueOf(tt.value)
-			cv.validateStringField(v, field, tt.path, tt.defaultTag)
+			cv.validateStringField(v, reflect.Value{}, field, tt.path, tt.defaultTag)
 			if v.HasErrors() != tt.wantError {
 				t.Errorf("validateStringField(%q, %q, %q) hasErrors=%v, want %v",
 					tt.value, tt.path, tt.defaultTag, v.HasErrors(), tt.wantError)
@@ -826,7 +826,7 @@ func TestMut_Line280_DefaultTagEmptyString(t *testing.T) {
 		cv := &Validator2{sanitizer: NewSanitizer()}
 		v := NewValidator()
 		field := reflect.ValueOf("")
-		cv.validateStringField(v, field, "schedule", "some-default")
+		cv.validateStringField(v, reflect.Value{}, field, "schedule", "some-default")
 
 		if v.HasErrors() {
 			t.Error("empty value with default tag must skip validation (no error expected)")
@@ -838,7 +838,7 @@ func TestMut_Line280_DefaultTagEmptyString(t *testing.T) {
 		cv := &Validator2{sanitizer: NewSanitizer()}
 		v := NewValidator()
 		field := reflect.ValueOf("GARBAGE_CRON!!!")
-		cv.validateStringField(v, field, "schedule", "some-default")
+		cv.validateStringField(v, reflect.Value{}, field, "schedule", "some-default")
 
 		if !v.HasErrors() {
 			t.Error("non-empty value with default tag must still be validated (invalid cron → error)")
@@ -850,7 +850,7 @@ func TestMut_Line280_DefaultTagEmptyString(t *testing.T) {
 		cv := &Validator2{sanitizer: NewSanitizer()}
 		v := NewValidator()
 		field := reflect.ValueOf("")
-		cv.validateStringField(v, field, "schedule", "") // no default, required
+		cv.validateStringField(v, reflect.Value{}, field, "schedule", "") // no default, required
 
 		if !v.HasErrors() {
 			t.Error("empty value without default on required field must produce error")
@@ -862,7 +862,7 @@ func TestMut_Line280_DefaultTagEmptyString(t *testing.T) {
 		cv := &Validator2{sanitizer: NewSanitizer()}
 		v := NewValidator()
 		field := reflect.ValueOf("")
-		cv.validateStringField(v, field, "image", "") // no default, optional
+		cv.validateStringField(v, reflect.Value{}, field, "image", "") // no default, optional
 
 		if v.HasErrors() {
 			t.Error("empty value without default on optional field should not error")

e2e/cli_exit_codes_test.go

@@ -66,24 +66,24 @@ func TestE2E_ExitCode_SuccessPaths(t *testing.T) {
 	}
 }
 
-// TestE2E_ExitCode_StrictValidationFails complements the INI-syntax case in
-// config_validation_test.go with a file that parses but does not satisfy
-// strict validation. Both have to fail, and for a deploy gate the distinction
-// does not matter — what matters is that neither is silently accepted.
+// TestE2E_ExitCode_SemanticFailureExits1 complements the INI-syntax case in
+// config_validation_test.go with a file that parses but is semantically wrong.
+// Both have to fail, and for a deploy gate the distinction does not matter —
+// what matters is that neither is silently accepted.
 //
-// Strict validation is opt-in (`enable-strict-validation`, default false), so
-// the config turns it on. Without it ofelia accepts semantically broken jobs
-// here — including an unparsable schedule, which the daemon then logs as a
-// warning while starting anyway, leaving a job that never fires. That is a
-// separate question from exit codes and is not pinned here.
-func TestE2E_ExitCode_StrictValidationFails(t *testing.T) {
+// The first version of this test used an unparsable schedule and only passed
+// because validation demanded web-UI credentials from a config that had no web
+// UI. That false positive is gone, and jobs are not reachable by the validator
+// at all, so the config here fails on a global field that genuinely is checked.
+// The schedule gap is tracked separately in #773.
+func TestE2E_ExitCode_SemanticFailureExits1(t *testing.T) {
 	t.Parallel()
 
 	configPath := writeConfig(t, `[global]
-  enable-strict-validation = true
+  web-address = definitely-not-an-address
 
-[job-local "broken"]
-  schedule = not-a-schedule
+[job-local "hello"]
+  schedule = @every 30s
   command = echo hi
 `)