fix: require web credentials conditionally, and make validate actually validate - #775
Merged
Merged
Conversation
Closes #774. A string field carrying no `default` tag was required unconditionally unless it appeared on an allow-list, so turning on enable-strict-validation demanded web-password-hash and web-secret-key from every config — including the ones with no web UI: [global] enable-strict-validation = true [job-local "hello"] schedule = @every 30s command = echo hi → 'web-password-hash': is required 'web-secret-key': is required The operator most likely to switch validation on is the one who wants their config checked, and they were met with errors about a feature they were not using. The practical answer was to leave validation off, which is how the other validation gaps stay 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; everything else keeps the old behaviour. A gate that cannot be resolved keeps the field required, because a mapping that has drifted from the config should surface rather than silently drop a check. The parent struct is threaded through validateField/validateStringField to make the sibling reachable; the internal tests that call those directly pass a zero Value, which is the unresolvable-gate case. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
…s so Part of #773. The checks sat behind enable-strict-validation, which defaults to false, so `ofelia validate` answered "looks fine" for a config it had not inspected. That flag is about whether the *daemon* refuses to start; running validate is itself the request to have the config checked. The error text in 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. This does not close #773. The validator walks structs and skips maps, and every job lives in one (map[string]*RunJobConfig and friends), so no job is reachable by it at all — an unparsable schedule passes with the flag on or off. Verified: with enable-strict-validation set, an invalid web-address is reported and `schedule = not-a-schedule` is not. 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. The e2e test added for exit codes in #771 has to change with this: it used a config whose only real failure was the web-credentials false positive fixed alongside this, so it was asserting that defect. It now fails on a global field that is genuinely checked. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
✅ Mutation Testing ResultsMutation Score: 71.43% (threshold: 60%)
What is mutation testing?Mutation testing measures test quality by introducing small changes (mutations) to the code and checking if tests detect them. A higher score means better test effectiveness.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
This PR improves configuration validation in Ofelia by (1) making “required” web-credential fields conditional on web-auth-enabled, and (2) ensuring ofelia validate performs semantic validation even when enable-strict-validation is off (while avoiding duplicate validation work when it’s already enabled).
Changes:
- Add conditional “required” gating for
web-password-hash/web-secret-keybased onweb-auth-enabled. - Ensure
validateruns semantic validation in the default (non-strict) case. - Update/add unit + mutation + e2e tests to pin the intended validation and exit-code behaviors.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cli/validate.go |
Runs config validation during validate even when strict validation is disabled. |
config/validator.go |
Passes parent struct into field validation and introduces conditional-required gating logic. |
config/validator_boundary_test.go |
Updates tests for the new validateStringField signature. |
config/validator_mutation_test.go |
Updates mutation tests for the new validateStringField signature. |
config/validator_conditional_test.go |
Adds focused unit tests for conditional-required behavior and fallback when the gate is missing. |
e2e/config_validation_test.go |
Adds e2e coverage for “validate actually validates” and for web credentials not being demanded when auth is off. |
e2e/cli_exit_codes_test.go |
Adjusts the semantic-failure exit-code test to fail on a truly validated global field. |
Suppressed comments (2)
config/validator.go:533
- Same issue as in
validateStruct:reflect.Typehas noFields()method, and this loop won’t compile. UseNumField/Field(i)to scan struct fields for the gate flag.
typ := parent.Type()
for fieldType := range typ.Fields() {
if !fieldType.IsExported() || fieldType.Type.Kind() != reflect.Bool {
continue
}
config/validator_conditional_test.go:57
- If
WebPasswordargis renamed (see struct definition above), this initializer also needs to use the corrected field name to keep the test compiling.
WebPasswordarg: "$2a$12$abcdefghijklmnopqrstuv",
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #775 +/- ##
==========================================
- Coverage 89.31% 89.29% -0.02%
==========================================
Files 88 88
Lines 12053 12071 +18
==========================================
+ Hits 10765 10779 +14
- Misses 994 997 +3
- Partials 294 295 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Review findings on PR #775. WebPasswordarg was a typo for WebPasswordHash and did not match the config key the test struct models. gosec's G101 fires on the requiredWhen map because an entry key contains "secret". Those are INI key names the validator matches on, not values, so the suppression follows the repo's existing inline form rather than being dismissed out of band — a reader of the map sees why it is there. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
codecov/patch reported 69.56% of the diff hit against a 80% target. The gaps were the branches a caller reaches when there is nothing to consult: gateIsOpen with no enclosing struct, and its skip over fields that carry the right key with the wrong type or the right type with the wrong key — either would silently turn a required field optional if it were mistaken for the gate. Also adds a unit test for validate rejecting a semantically invalid config without the strict flag, which until now was only asserted through the e2e suite. gateIsOpen goes to 100%, ValidateCommand.Execute from 81.8% to 90.9%. The two blocks still uncovered there predate this change: the log-level warning branch and the json.Marshal error path. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #774, partially addresses #773.
Strict validation demanded credentials for a feature you were not using
A string field with no
defaulttag was required unconditionally unless it appeared on an allow-list, so switching onenable-strict-validationproduced this for a config with no web UI at all: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-enabledis 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 validatedid not validateThe checks sat behind
enable-strict-validation(defaultfalse), 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; runningvalidateis itself the request to have the config checked.cli/config.goeven points at the command — "Use 'ofelia validate --config=…' for detailed validation" — while the command did not enable the detail.validatenow runs the validator regardless. When the flag is on,BuildFromFilehas 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]*RunJobConfigand friends), so no job is reachable by the validator at all. An unparsable schedule passes with the flag on or off:Fixing that means traversing the job maps, which under the current "no
defaulttag 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
go test ./...— green, coverage 90.32%go test -race -tags=e2e ./e2e/...— greengolangci-lint runincl.--build-tags="e2e unix"— 0 issueslefthook run pre-push— exit 0web-auth-enabled = truewithout a hash → still demanded