fix(backend): move Docker client to the maintained moby client module#1935
Conversation
github.com/docker/docker is frozen at v28.5.2 — moby publishes v29+ under github.com/moby/moby/v2, with the API client split out into the standalone github.com/moby/moby/client and github.com/moby/moby/api modules. That leaves five Dependabot alerts on the old module path with no reachable fix version: CVE-2026-42306 high docker cp bind mount redirection <= 28.5.2 CVE-2026-41567 high PUT /containers/{id}/archive <= 28.5.2 CVE-2026-41568 medium docker cp symlink swap <= 28.5.2 CVE-2026-34040 high AuthZ plugin bypass < 29.3.1 CVE-2026-33997 medium plugin privilege off-by-one < 29.3.1 All five are daemon-side defects, and we only ever imported the client. Moving to the split client module drops the daemon source out of the module graph entirely, so these alerts resolve and future daemon advisories will not flag us at all. It also trims six transitive dependencies. The new client takes an options struct per call and returns a result struct, so this is a mechanical rewrite of the wrapper. Behavioural notes: - Ports moved from docker/go-connections/nat to moby/api/types/network, where Port is opaque and PortBinding.HostIP is a netip.Addr. buildDockerPortBindings now returns an error for out-of-range ports and unparseable host IPs instead of handing the daemon a malformed request. An empty HostIP still maps to the zero address, preserving "publish on all interfaces". - DiskUsage needs Verbose to get per-image and per-container records; the aggregate LayersSize is now Images.TotalSize. - BuildCachePrune no longer takes KeepStorage — the client derives the legacy keep-storage parameter from ReservedSpace for daemons on API <= 1.47. - ImagesPrune is now ImagePrune. - Container state and health are typed string enums, and InspectResponse.Config and .State are pointers, so inspect mapping guards for nil. Also corrects the RUSTSEC-2024-0429 note in the cargo-audit ignore list. It claimed the advisory resolves once Tauri moves to webkit2gtk-4.1, but Tauri v2 is already on those bindings and still resolves glib to 0.18.5; the real blocker is that gtk3-rs is archived. Backport submitted upstream.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe backend Docker integration migrates container, networking, inspection, lifecycle, and storage operations to typed Moby client APIs. Tests update fakes and coverage for typed responses and port validation. Go dependencies and a desktop cargo-audit note are also updated. ChangesDocker client migration
Cargo audit tracking note
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/backend/internal/agent/docker/client.go (2)
274-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate container/host config construction across
CreateContainerandCreateContainerInteractive.Both functions now repeat the same
buildDockerPortBindingscall + error wrap, mount building,containerCfg/hostCfgconstruction, andContainerCreateinvocation. Since this PR already touches both call sites to add error handling, consider extracting a shared helper (e.g.buildContainerCreateInputs(cfg ContainerConfig) (*container.Config, *container.HostConfig, error)) that both callers customize (interactive fields on top).As per path instructions, apps/backend/**/*.go must "Keep functions within the configured quality limits... and no large duplicated blocks; extract helpers when limits are reached."
Also applies to: 743-790
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/internal/agent/docker/client.go` around lines 274 - 315, Extract the duplicated container and host configuration construction from CreateContainer and CreateContainerInteractive into a shared helper such as buildContainerCreateInputs, including mount and port-binding setup with contextual error wrapping. Have both callers reuse the helper, then apply only their interactive-specific configuration before invoking ContainerCreate, preserving existing logging and error behavior.Source: Path instructions
444-495: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify test coverage for the refactored inspection/state/IP/host-port helpers.
inspectContainer,applyContainerState(with its nil-State/nil-Confighandling), the netip-validity IP selection inGetContainerIP, and the new parse/normalize path inGetContainerHostPortare new or materially changed logic. client_test.go's diff only adds coverage forbuildDockerPortBindings,normalizeDockerHostIP, andparseHostPort— no visible tests exerciseGetContainerInfo/GetContainerIP/GetContainerHostPortagainst the new typed inspect response (e.g. nilState/Config, invalidIPAddress).As per coding guidelines, "Every code change must include tests for new or changed logic, except configuration files, generated code, and React component markup" for
**/*.gofiles.Also applies to: 507-532, 535-583
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/internal/agent/docker/client.go` around lines 444 - 495, Add client tests covering GetContainerInfo with nil State and Config, including inspectContainer’s typed response handling; cover GetContainerIP selecting only netip-valid addresses; and cover GetContainerHostPort’s parse/normalize behavior with representative valid and invalid host-port inputs. Reuse existing test helpers or fixtures where possible and preserve current error and zero-value behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/backend/internal/agent/docker/client.go`:
- Around line 274-315: Extract the duplicated container and host configuration
construction from CreateContainer and CreateContainerInteractive into a shared
helper such as buildContainerCreateInputs, including mount and port-binding
setup with contextual error wrapping. Have both callers reuse the helper, then
apply only their interactive-specific configuration before invoking
ContainerCreate, preserving existing logging and error behavior.
- Around line 444-495: Add client tests covering GetContainerInfo with nil State
and Config, including inspectContainer’s typed response handling; cover
GetContainerIP selecting only netip-valid addresses; and cover
GetContainerHostPort’s parse/normalize behavior with representative valid and
invalid host-port inputs. Reuse existing test helpers or fixtures where possible
and preserve current error and zero-value behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27730807-a651-4d92-864b-f68fc5661867
⛔ Files ignored due to path filters (1)
apps/backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
apps/backend/go.modapps/backend/internal/agent/docker/activity_test.goapps/backend/internal/agent/docker/client.goapps/backend/internal/agent/docker/client_test.goapps/backend/internal/agent/docker/storage.goapps/backend/internal/agent/docker/storage_test.goapps/desktop/src-tauri/.cargo/audit.toml
1 participant
Summary
Clears the five open Docker alerts on the Dependabot page by moving the backend off
github.com/docker/docker, and corrects a stale rationale in the cargo-audit ignore list.What the alerts actually are
docker/docker?<= 28.5.2<= 28.5.2<= 28.5.2< 29.3.1< 29.3.1There was no version bump available for any of them, and the reason is structural rather than a missing release.
github.com/docker/dockerstops at v28.5.2 — the last tag on that module path. Moby v29+ publishes undergithub.com/moby/moby/v2, and the API client was split into two standalone modules,github.com/moby/moby/clientandgithub.com/moby/moby/api. So "upgrade to 29.3.1" is not something the old import path can express; the three<= 28.5.2advisories list their fix undergithub.com/moby/moby/v2instead.All five are daemon-side defects —
docker cpraces, the archive endpoint, plugin authorization. We only ever importedclientandapi/types/*, so none of them were reachable from our code. But the module carries the whole daemon source, which is why Dependabot flags us, and it will keep doing so for every future daemon CVE.Moving to the split client module fixes the class rather than the instances: the daemon source leaves the module graph, so these five resolve and future daemon advisories never reach us. It also drops six transitive dependencies (
containerd/log,moby/sys/atomicwriter,moby/term,morikuni/aec,pkg/errors,gotest.tools/v3).Migration notes
The new client takes an options struct per call and returns a result struct, so the wrapper rewrite is mostly mechanical. Genuine behaviour changes worth review:
docker/go-connections/nattomoby/api/types/network.network.Portis opaque andPortBinding.HostIPis anetip.Addrrather than a string.buildDockerPortBindingsnow returns an error for out-of-range container ports and unparseable host IPs, instead of handing the daemon a malformed request and getting an opaque failure back. An emptyHostIPstill maps to the zero address, which preserves Docker's publish-on-all-interfaces behaviour — there's a test pinning that.DiskUsageneedsVerbose: truefor the daemon to return per-image and per-container records; without it only aggregates come back and the storage page would show empty lists. The flatLayersSizeis nowImages.TotalSize.BuildCachePruneno longer has aKeepStoragefield. We were setting both it andReservedSpaceto the same value; the client now derives the legacykeep-storagequery parameter fromReservedSpacewhen talking to daemons on API <= 1.47, so the duplication goes away.ImagesPruneis nowImagePrune.InspectResponse.Config/.Stateare pointers. Inspect mapping is extracted intoapplyContainerStateand guards for nil rather than dereferencing blind.github.com/creack/pty(1.1.21 → 1.1.24) andMicrosoft/go-winio(0.4.21 → 0.6.2) moved as a consequence of minimal version selection.API version negotiation is still on, so older daemons remain supported.
Second change: the glib note in
audit.tomlWhile going through the remaining alerts I found the ignore entry for RUSTSEC-2024-0429 (glib 0.18.5) was justified with something that isn't true:
Tauri v2 is already on the webkit2gtk-4.1 bindings and still resolves glib to 0.18.5. The actual blocker is that
gtk-rs/gtk3-rs— which publishesgtk,gdk,webkit2gtkand friends — was archived in March 2024, so no GTK3 release depending on glib >= 0.20 will ever exist. Tauri closed its own tracking issue asnot_plannedand targets GTK4 for v3, so waiting on that is a multi-year no-op.The comment now says what's really going on. There is a way out, though: the unsound code is in
glib, whose repo is active and still has a live0.18branch, andgtk0.18.2's dependency is a caret requirement. Aglib0.18.6 would fix this for us on the nextcargo updatewith no changes to anything archived. I've submitted that upstream — gtk-rs-core#2009 backports the two-character fix to0.18, #2010 asks for the release. The advisory reproduces on rustc 1.97.1 and the0.18branch's own test suite SIGSEGVs in release mode, so it's a real crash rather than a theoretical unsoundness. We drop the ignore entry if and when 0.18.6 ships.Testing
go build ./...,go vet ./...,go mod verifyclean.golangci-lint run ./internal/agent/docker/... --new-from-rev=<base>— 0 issues.go test -race ./...— the only failures are the pre-existingTestServiceInitializeLocalRepository*set, which fail identically on a pristinemaincheckout in this environment (symlink resolution under a sandboxedTMPDIR). Untouched by this change.client_test.gofor the port-binding validation paths, the all-interfaces case, andparseHostPort.storage_test.gonow asserts thatDiskUsagerequestsVerbose, since forgetting it silently empties the storage page.Still open after this
One alert remains: #178, glib, medium, and it stays open until the upstream release lands. Worth noting separately that Dependabot security updates are currently disabled on the repo and there is no
.github/dependabot.yml, so nothing opens version-bump PRs on a schedule. That looked like it might be deliberate, so I've left it alone rather than turning it on in this PR.