fix(docker): nil-guard remaining convertFrom*/convertToMount helpers#658
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements defense-in-depth nil guards for several conversion functions in the Docker adapter, specifically convertToMount, convertFromAPIContainer, convertFromNetworkResource, and convertFromNetworkInspect, to prevent potential nil-pointer dereferences. Comprehensive regression tests have been added to verify these guards and ensure no regressions on valid inputs. I have no feedback to provide as there were no review comments to evaluate.
There was a problem hiding this comment.
Pull request overview
This PR completes a defense-in-depth audit in the Docker adapter by adding missing nil-guards to remaining convertFrom* helpers in convert.go and the asymmetric convertToMount helper in container.go, preventing potential nil-pointer panics from unsafe direct calls (while keeping current production behavior unchanged).
Changes:
- Add early-return nil-guards for
convertFromAPIContainer,convertFromNetworkResource,convertFromNetworkInspect, andconvertToMount. - Add regression tests covering nil-input behavior (panic-free) plus happy-path sanity checks.
- Document the fix in
CHANGELOG.mdunder “Fixed”.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| core/adapters/docker/convert.go | Adds nil-guards (zero-value / nil returns) and rationale godoc for remaining convertFrom* helpers. |
| core/adapters/docker/container.go | Adds a nil-guard and godoc rationale to convertToMount to match other convertTo* helpers. |
| core/adapters/docker/convert_nil_test.go | Adds nil-input regression tests + happy-path sanity checks for the newly guarded helpers. |
| CHANGELOG.md | Records the defense-in-depth fix and references the related issue/PR lineage. |
Sibling-hunt completion of the convert.go family of nil-guard gaps that PR #648 started. Four helpers in core/adapters/docker/ retained unguarded pointer derefs after #648: - convertFromAPIContainer(c *containertypes.Summary) -> derefs c.Names, c.ID, c.Image, c.Created, c.Labels, c.State - convertFromNetworkResource(n *networktypes.Summary) -> derefs n.Name, n.IPAM.Driver, n.Containers - convertFromNetworkInspect(n *networktypes.Inspect) -> derefs n.Name, n.IPAM, n.Containers - convertToMount(m *domain.Mount) -> derefs m.Type All four are reached via &loopVar from a range over a slice in production, so no live panic exists today. The signature contract is unsafe though, and convertToMount was especially asymmetric: every other convertTo* helper in container.go (convertToHostConfig, convertToNetworkingConfig, convertToEndpointSettings, convertToContainerConfig) already nil-guarded its argument. Each helper now early-returns the appropriate zero value (nil for the pointer-returning convertFromNetworkInspect, mirroring convertFromSwarmService from #648; zero struct value for the others) and gains a godoc note explaining the defense-in-depth rationale. Adds nil-input + happy-path regression tests in convert_nil_test.go using the existing failOnPanic helper. Same bug class as #619 / #626 / #632 / #648. Closes #654 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
✅ Mutation Testing ResultsMutation Score: 100.00% (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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #658 +/- ##
==========================================
- Coverage 87.88% 87.85% -0.03%
==========================================
Files 88 88
Lines 10978 11068 +90
==========================================
+ Hits 9648 9724 +76
- Misses 1086 1096 +10
- Partials 244 248 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
## Summary Cut the existing `[Unreleased]` CHANGELOG block at [fad5239](netresearch@fad5239) into a versioned `[0.25.0] - 2026-05-14` heading, and fill in two entries for PRs that landed after the previous `[Unreleased]` writeups: - **### Security** — Go toolchain `1.26.2` → `1.26.3` ([netresearch#662](netresearch#662)). Clears six stdlib advisories reachable from this codebase (`net/mail`, `html/template`, `net`, `net/http`); refreshes direct deps (`docker/cli`, `golang.org/x/{crypto,term,text}`) and the full indirect graph. Post-bump `govulncheck` is down to the two unfixable upstream moby advisories on `docker/docker` v28.5.2. - **### Fixed** — `MaxRuntime` cancellation now stops *and removes* the container/service ([netresearch#659](netresearch#659), fixes [netresearch#655](netresearch#655)). Completes [netresearch#651](netresearch#651 deadline wiring with a fresh `context.WithTimeout(context.Background(), jobCleanupTimeout)` cleanup context so stop/remove still runs after the parent deadline fires. Mirrored into `RunServiceJob`. ## Headline changes since v0.24.0 - **Security**: Go 1.26.3 toolchain, three silent-downgrade vectors closed (`https://` mTLS, SMTP STARTTLS default, webhook allow-list typo), fail-closed on `tcp+tls://` without cert material ([netresearch#660](netresearch#660), [netresearch#646](netresearch#646), [netresearch#662](netresearch#662)) - **New**: `tcp+tls://` `DOCKER_HOST` scheme re-enabled ([netresearch#625](netresearch#625)); `DOCKER_TLS_VERIFY` / `DOCKER_CERT_PATH` honored ([netresearch#613](netresearch#613)) - **Correctness**: bounded contexts in scheduler / health / Docker pings ([netresearch#636](netresearch#636), [netresearch#651](netresearch#651)); orphan-container cleanup on MaxRuntime ([netresearch#659](netresearch#659)); pervasive nil-guard pass across the Docker adapter ([netresearch#626](netresearch#626), [netresearch#639](netresearch#639), [netresearch#648](netresearch#648), [netresearch#658](netresearch#658)) - **Refactor / DX**: unified Docker host / scheme resolution ([netresearch#629](netresearch#629)); webhook global config dual-store collapsed ([netresearch#637](netresearch#637)); `[global]` label handling unified across all subsystems ([netresearch#661](netresearch#661)) ## Version bump rationale Pre-1.0 semver — minor bump because the range includes one `feat:` ([netresearch#625](netresearch#625) — `tcp+tls://` scheme re-enabled), several `fix(security):` PRs that surface previously-silent downgrades, and the `[global]` label-handling rework. The webhook key rename in [netresearch#620](netresearch#620) / [netresearch#637](netresearch#637) is shipped under `### Deprecated` (legacy `ofelia.webhooks` form keeps working with a one-shot warning), not as a breaking change. ## Notes - This PR touches **only `CHANGELOG.md`** — matches the v0.24.0 prep pattern ([netresearch#602](netresearch#602)). The Release workflow injects the version into `cli.Version` via ldflags from the tag, so no `cli/version.go` edit is needed. - After merge: signed annotated tag `v0.25.0` will be pushed to the merge commit, triggering [`release-go-app.yml`](https://github.com/netresearch/.github/blob/main/.github/workflows/release-go-app.yml) for binaries, container image, cosign `--bundle` signatures, and SLSA attestations. - Contributor thanks will be added directly to the GitHub release description (not the CHANGELOG, per project convention from v0.24.0). ## Test plan - [x] `go build ./...` clean - [ ] CI green on this PR - [ ] CHANGELOG renders correctly on GitHub Files Changed tab - [ ] After merge: signed annotated tag `v0.25.0` created on the merge commit (`git tag -s v0.25.0 -m "v0.25.0"`) and pushed - [ ] Release workflow run succeeds end-to-end



Summary
Closes #654. Sibling-hunt completion of the
convert.gofamily of nil-guard gaps that #619 / #626 / #632 / #648 started.PR #648 added nil-guards to the
convertFrom*swarm/event helpers andContainerServiceAdapter.Create. The follow-up audit found 4 more helpers incore/adapters/docker/with the same defense-in-depth gap:convertFromAPIContainer(c *containertypes.Summary)c.Names,c.ID,c.Image,c.Created,c.Labels,c.StateconvertFromNetworkResource(n *networktypes.Summary)n.Name,n.IPAM.Driver,n.ContainersconvertFromNetworkInspect(n *networktypes.Inspect)n.Name,n.IPAM,n.ContainersconvertToMount(m *domain.Mount)m.TypeAll four are reached via
&loopVarfrom arangeover a slice in production, so no live panic exists today. The signature contract is unsafe though — andconvertToMountwas especially asymmetric: every otherconvertTo*helper incontainer.go(convertToHostConfig,convertToNetworkingConfig,convertToEndpointSettings,convertToContainerConfig) already nil-guarded its argument.Changes
convertFromNetworkInspect(nil) -> nil(mirrorsconvertFromSwarmServicefrom fix(docker): nil-guard symmetric *From* converters and Container.Create (#632) #648)convertFromAPIContainer(nil) -> domain.Container{}convertFromNetworkResource(nil) -> domain.Network{}convertToMount(nil) -> mount.Mount{}convert_nil_test.gousing the existingfailOnPanichelper. (Comprehensive happy-path coverage for these helpers already exists inconvert_test.go/container_convert_test.go/convert_mutation_test.go; the colocated happy-path counterparts mirror the pattern established forconvertFromSDKEventinevent_nil_test.goso a future reader sees the nil + valid contract side-by-side.)CHANGELOG.mdentry under### Fixedadjacent to the existing Docker SDK: symmetric nil-guards on convertFromSwarmService / convertFromSwarmTask / convertFromSDKEvent #632 / fix(docker): nil-guard symmetric *From* converters and Container.Create (#632) #648 entry.Why "sibling-hunt completion"
This closes the convert.go family that #619 (Exec.Create / Run), #626 (
convertTo*swarm/mount), #632 / #648 (convertFrom*swarm/event +Container.Create) chipped away at. After this merges, everyconvertTo*andconvertFrom*helper incore/adapters/docker/nil-guards its pointer argument — the unsafe-signature class is closed.Test plan
go test ./core/adapters/docker/ -count=1 -short -race— all pass (existing happy-path tests inconvert_mutation_test.go/convert_test.go/container_convert_test.gocontinue to pass, confirming the new guards don't regress anything)golangci-lint run ./...— 0 issuesfailOnPanic(would catch a missing guard)