Skip to content

fix(docker): guard nil Config, and raise coverage to 90% for Best Practices Gold - #768

Merged
CybotTM merged 4 commits into
mainfrom
chore/security-and-coverage-followups
Aug 2, 2026
Merged

fix(docker): guard nil Config, and raise coverage to 90% for Best Practices Gold#768
CybotTM merged 4 commits into
mainfrom
chore/security-and-coverage-followups

Conversation

@CybotTM

@CybotTM CybotTM commented Aug 2, 2026

Copy link
Copy Markdown
Member

Follow-ups to the security and quality reports on the repository's dashboards. Each item below was verified before acting; the ones that turned out to be false positives are dismissed at the source with a justification rather than worked around in code.

A crash, found by writing the coverage tests

convert.go and image.go both read Labels straight off the SDK's Config field. Config is a pointer in container.InspectResponse and image.InspectResponse, so a daemon that omits it — a socket proxy, or a Docker-compatible API that fills in less than Docker does — is a nil dereference on a path that does not recover: the scheduler goes down.

The surrounding code already treats this as a real shape (State and State.Health are guarded a few lines below the container dereference). Config was the field that was missed, and it is the same defect ErrNilContainerConfig was added for in #632/#626, in the two places that fix did not reach.

Both regression tests were confirmed to fail against the unpatched code.

Coverage: 87.64% → 90.08%

OpenSSF Best Practices Gold asks for ≥90% statement coverage. The gap sat where a daemon or a process boundary made code awkward to reach:

before after
core/adapters/docker 66.7% 78.3%
root (ofelia.go) 22.0% 95.1%
repository 87.64% 90.08%

A stub daemon answers as Docker would, so each adapter's translation is asserted directly — container lifecycle, the stdcopy demultiplexing CopyLogs performs, system/image/network calls, and the swarm polling loops including both exits (terminal task, and the timeout that stops a wedged job hanging the scheduler). main() is driven over --version, --help, an unknown command and a bare invocation, so a command dropped from the parser fails a test instead of only surfacing when a user runs the binary.

Branch coverage (the second Gold criterion) is not addressed here and cannot be from this repository: Go's toolchain measures statements, not branches, and there is no established FLOSS branch-coverage tool for Go. That criterion needs a justified N/A on bestpractices.dev, which is a change to the badge questionnaire rather than to this repo.

Codecov components

The Components page was empty — codecov.yml declared no component_management, so a coverage drop said nothing about where it happened. Components now follow the boundaries in ARCHITECTURE.md; core negates the adapter and persistence subtrees so those files are not counted twice. Validated against https://codecov.io/validate.

Dismissed at the source (no code change — the code is correct)

  • 13× CodeQL go/log-injection in web/server.go. Every one passes req.Name as a structured slog attribute, and ofelia.go:39 installs the stdlib slog.NewTextHandler, which escapes newlines in attribute values and in the message — verified empirically with a payload containing a full forged log line. The action concatenated into two of those messages is a compile-time literal from server.go:618,622. Sanitizing the values would add code that demonstrably prevents nothing.
  • 2× Scorecard PinnedDependencies on zizmor.yml:18 and verify-release.yml:90. Both point at netresearch/.github reusables — first-party, misclassified as third-party. Org policy keeps them on @main so upstream security fixes propagate; third-party actions stay hash-pinned.
  • 1× zizmor dangerous-triggers on the workflow_run in verify-release.yml. Nothing is checked out, permissions: {} at workflow level with minimal per-job scopes, and the upstream Release workflow fires only on maintainer tag pushes — not fork PRs. The one external value (head_branch) is env-passed and validated against a character allowlist before use.

Open code-scanning alerts on main: 16 → 0.

Reported, not actioned

  • Trivy "results may be out of date" is a stale configuration, not a broken scan. The filesystem scan lived in ci.yml and was removed on 2026-04-19 by the template sync (50803c4); the shared go-check.yml uses govulncheck + gosec instead. Trivy still runs as container-scan on every release (last: v0.28.1, 2026-07-28). Clearing the banner means deleting 1016 analyses across 437 refs — GitHub releases only the newest per ref, so the API path is ~1016 chained irreversible deletes. The one-click alternative is Security → Code scanning → Tool status.
  • GO-2026-5932 (x/crypto/openpgp unmaintained) is not reachable: 0 occurrences in the build graph, and govulncheck reports 0 vulnerabilities in code. x/crypto is required for bcrypt (cli, web auth). There is no fixed version — introduced: 0 with no fix event — so no upgrade can clear it.
  • Missing release provenance is a detection gap, not missing provenance. actions/attest-build-provenance runs in the release; ofelia-linux-amd64 of v0.28.1 has an in-toto attestation retrievable by digest. Scorecard reads release assets and looks for .intoto.jsonl. Fixing it belongs in netresearch/.github, as does the JUnit upload that would populate Codecov Test Analytics.

Test plan

  • go test ./... — full suite green, 0 failures
  • Coverage measured at 90.0784% (7699/8547 statements)
  • golangci-lint run over the whole repo — 0 issues
  • lefthook run pre-push — exit 0
  • Both nil-Config regression tests confirmed red against the unpatched code
  • codecov.yml accepted by Codecov's validator

CybotTM added 3 commits August 2, 2026 09:53
…esponse

Both inspect converters read Labels straight off the SDK's Config field:

    core/adapters/docker/convert.go:106   c.Config.Labels
    core/adapters/docker/image.go:146     img.Config.Labels

Config is a pointer in both `container.InspectResponse` and
`image.InspectResponse`, so a response without it is a nil dereference,
and neither call site is on a path that recovers - the panic takes the
scheduler down. A daemon that sends less than Docker does is enough to
trigger it: a socket proxy, or a Docker-compatible API.

The surrounding code already treats this as a real shape. State and
State.Health are both guarded a few lines below the container
dereference; Config was the one field that was not. It is the same
defect ErrNilContainerConfig was added for in #632/#626, in the two
places that fix did not reach.

Both now populate Labels only when Config is present, leaving it nil
otherwise, which is what a caller already has to handle for a container
whose daemon reported no labels.

Found while adding the stub-daemon coverage in the following commit: the
image-inspect success-path test panicked instead of failing. Regression
tests for both call sites are included there and were confirmed to fail
without this change.

Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
OpenSSF Best Practices Gold asks for at least 90% statement coverage.
The repository sat at 87.64%, and the gap was concentrated where a
daemon or a process boundary made the code awkward to reach:

    core/adapters/docker   66.7%   thin SDK wrappers, success paths
    (root) ofelia.go       22.0%   main() entirely uncovered

Both are now reachable without installing anything.

A stub daemon (stubSDK) answers as Docker would, so each adapter's
translation - request shape out, domain type back - is asserted
directly: container create/start/list/wait/logs/kill/pause/unpause/
rename, the stdcopy demultiplexing CopyLogs performs for non-TTY
containers, system info/ping/version/disk-usage, image list/inspect/
remove/tag/exists, network list/inspect/create/connect/disconnect, and
the swarm polling loops including both of their exits (a task reaching a
terminal state, and the timeout that stops a wedged job hanging the
scheduler). Route fragments are matched longest-first because several
Docker endpoints are prefixes of others and ranging a map would pick
between them in Go's randomized order.

main() is reachable from a test because each of its exits is a plain
return - the flag-error path was deliberately changed away from
os.Exit(1). Driving it over --version, --help, an unknown command and a
bare invocation covers the command wiring, so a command dropped from the
parser fails a test rather than only showing up when a user runs the
binary. Those tests cannot run in parallel: they replace os.Args and
os.Stdout, which is why they carry an explicit paralleltest exemption.

Also included are the two regression tests for the nil-Config panic
fixed in the previous commit. Both were confirmed to fail against the
unpatched code.

    core/adapters/docker   66.7% -> 78.3%
    (root)                 22.0% -> 95.1%
    repository             87.64% -> 90.08%

No production code changed here; the whole suite passes with -race and
golangci-lint reports no issues.

Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
The Codecov Components page was empty because codecov.yml declared no
component_management section, so every coverage change was reported
against the repository as a whole and a drop said nothing about where it
happened.

Components now follow the architecture boundaries in ARCHITECTURE.md:
core, adapters, persistence, web, cli, config, middlewares, metrics. The
core component negates the adapter and persistence subtrees, which are
components in their own right - without that, those files would be
counted twice.

Statuses are informational, matching the existing project status: the
gate that can fail a pull request stays the 80% patch target, not
per-component drift.

Validated against https://codecov.io/validate, which accepts the file
and resolves the negated paths.

Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
Copilot AI review requested due to automatic review settings August 2, 2026 07:55
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@github-actions github-actions Bot added the tests label Aug 2, 2026
github-actions[bot]
github-actions Bot previously approved these changes Aug 2, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated approval for maintainer PR

All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown

✅ Mutation Testing Results

Mutation Score: 100.00% (threshold: 60%)

✨ Good job! Mutation score meets the threshold.

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.

  • Killed mutants: Tests caught the mutation (good!)
  • Survived mutants: Tests missed the mutation (needs improvement)

@codecov

codecov Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.04%. Comparing base (659bb36) to head (56b07b3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #768      +/-   ##
==========================================
+ Coverage   87.70%   89.04%   +1.33%     
==========================================
  Files          90       90              
  Lines       12057    12059       +2     
==========================================
+ Hits        10575    10738     +163     
+ Misses       1193     1024     -169     
- Partials      289      297       +8     
Flag Coverage Δ
integration 89.04% <100.00%> (+1.33%) ⬆️
unittests 88.47% <66.66%> (+3.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Docker SDK adapter layer against nil-pointer dereferences when Docker (or a proxy/compatible API) omits Config in inspect responses, and adds extensive stub-daemon driven tests to raise statement coverage to ≥90% and improve Codecov attribution via components.

Changes:

  • Guard Config pointer access in Docker container/image inspect conversions to prevent scheduler crashes.
  • Add stub-daemon based adapter tests and main() invocation tests to cover previously hard-to-reach success/termination paths.
  • Configure Codecov component management to attribute coverage changes to architectural layers.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
ofelia_main_test.go Exercises main() parse/return-only paths (--version, --help, invalid/bare invocations) to lock in command wiring and boost root coverage.
core/adapters/docker/service_stub_test.go Adds stub-daemon tests for Swarm polling loops and terminal/timeout exits, plus allTasksTerminal coverage.
core/adapters/docker/image.go Prevents nil dereference by reading labels only when img.Config != nil.
core/adapters/docker/convert.go Prevents nil dereference by reading container labels only when c.Config != nil.
core/adapters/docker/convert_nil_test.go Adds regression coverage for convertFromContainerJSON when Config is omitted.
core/adapters/docker/container_wrappers_test.go Introduces stub-daemon tests for container wrapper “success” paths and log demux behavior.
core/adapters/docker/adapters_stub_test.go Adds stub-daemon tests for system/image/network adapters’ success paths and field mapping.
codecov.yml Adds Codecov component definitions aligned to architecture boundaries to improve attribution.
Suppressed comments (5)

core/adapters/docker/container_wrappers_test.go:366

  • This table test writes to gotPath from the httptest server handler goroutine and reads it in the test goroutine without synchronization; go test -race will flag this. Use a channel (or other synchronization) to pass the observed path back to the test.
			var gotPath string
			adapter := stubDaemon(t, map[string]http.HandlerFunc{
				tc.route: func(w http.ResponseWriter, r *http.Request) {
					gotPath = r.URL.Path
					w.WriteHeader(http.StatusNoContent)

core/adapters/docker/container_wrappers_test.go:390

  • This test writes to gotSignal from the httptest server handler goroutine and reads it in the test goroutine without synchronization; go test -race will report a data race. Pass the signal back via a channel (or other synchronization).
	var gotSignal string
	adapter := stubDaemon(t, map[string]http.HandlerFunc{
		"/kill": func(w http.ResponseWriter, r *http.Request) {
			gotSignal = r.URL.Query().Get("signal")
			w.WriteHeader(http.StatusNoContent)

core/adapters/docker/container_wrappers_test.go:411

  • This test writes to gotName from the httptest server handler goroutine and reads it in the test goroutine without synchronization; go test -race will report a data race. Use a channel (or other synchronization) to capture the query param value.
	var gotName string
	adapter := stubDaemon(t, map[string]http.HandlerFunc{
		"/rename": func(w http.ResponseWriter, r *http.Request) {
			gotName = r.URL.Query().Get("name")
			w.WriteHeader(http.StatusNoContent)

core/adapters/docker/adapters_stub_test.go:318

  • This subtest toggles reached from the httptest server handler goroutine and reads it in the test goroutine without synchronization; go test -race will report a data race. Use a channel (or other synchronization) to signal that the handler was reached.
			reached := false
			adapter := &NetworkServiceAdapter{client: stubSDK(t, map[string]http.HandlerFunc{
				tc.route: func(w http.ResponseWriter, _ *http.Request) {
					reached = true
					w.WriteHeader(http.StatusOK)

core/adapters/docker/service_stub_test.go:189

  • poll is incremented in the httptest server handler goroutine and read in the test goroutine without synchronization; go test -race will flag this. Use a channel (or atomic) to drive the handler’s state machine and assert the number of polls in a race-free way.
	poll := 0
	adapter := &SwarmServiceAdapter{client: stubSDK(t, map[string]http.HandlerFunc{
		"/tasks": func(w http.ResponseWriter, _ *http.Request) {
			poll++
			second := string(domain.TaskStateRunning)

Comment thread core/adapters/docker/container_wrappers_test.go Outdated
Comment thread core/adapters/docker/container_wrappers_test.go Outdated
Comment thread core/adapters/docker/adapters_stub_test.go Outdated
Comment thread core/adapters/docker/service_stub_test.go Outdated
Review findings from Copilot on PR #768.

The stub tests assigned to plain variables inside the httptest handler
and read them from the test goroutine. That works today — every handler
assigns before writing the response, which gives the client's return a
happens-before edge — and 20 runs under -race reported nothing. But the
memory model does not guarantee that ordering, and a later edit moving
an assignment after the response write would turn it into a real race
that the detector may or may not catch on any given run.

A requestRecorder now carries the observations under a mutex, and the
two poll counters became atomic.Int32. The assertions read through
accessors, so the synchronization cannot be dropped by accident. It also
removes the repeated `var gotX string` capture blocks.

Also corrects the stubSDK comment: an unrouted request both fails the
test and answers 404, where the comment claimed it did the former
"instead of" the latter.

Full suite green, coverage unchanged at 90.08%, golangci-lint reports no
issues, and the docker package passes -count=10 under -race.

Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
@sonarqubecloud

sonarqubecloud Bot commented Aug 2, 2026

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated approval for maintainer PR

All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.

@CybotTM
CybotTM added this pull request to the merge queue Aug 2, 2026
Merged via the queue into main with commit cca6d9c Aug 2, 2026
37 checks passed
@CybotTM
CybotTM deleted the chore/security-and-coverage-followups branch August 2, 2026 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants