Skip to content

fix(bridge): confirm before wiping non-empty convert output dir - #14174

Merged
ndeloof merged 1 commit into
docker:mainfrom
glours:fix/bridge-convert-unsafe-output-removal
Sep 3, 2026
Merged

fix(bridge): confirm before wiping non-empty convert output dir#14174
ndeloof merged 1 commit into
docker:mainfrom
glours:fix/bridge-convert-unsafe-output-removal

Conversation

@glours

@glours glours commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

What I did
bridge convert -o <dir> unconditionally ran os.RemoveAll on the output directory and discarded any error, so a typo (-o ., -o $PWD, -o ~) silently destroyed unrelated user data.

An empty or missing output directory is still created silently, but a non-empty one now requires explicit confirmation (prompted interactively, or via the new --yes flag for scripts/CI) before being wiped.

Related issue
https://docker.atlassian.net/browse/DDB-683

(not mandatory) A picture of a cute animal, if possible in relation to what you did
image

`bridge convert -o <dir>` unconditionally ran `os.RemoveAll` on the
output directory and discarded any error, so a typo (`-o .`, `-o
$PWD`, `-o ~`) silently destroyed unrelated user data.

An empty or missing output directory is still created silently, but a
non-empty one now requires explicit confirmation (prompted
interactively, or via the new `--yes` flag for scripts/CI) before
being wiped.

Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
@glours
glours requested review from a team as code owners September 3, 2026 09:47
@glours
glours requested a review from ndeloof September 3, 2026 09:47
@glours glours self-assigned this Sep 3, 2026

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Assessment: 🟢 APPROVE

The safety guard is correctly implemented. prepareOutputDir checks for a non-empty directory, prompts for confirmation (or auto-confirms via --yes), and only then calls os.RemoveAll + os.MkdirAll — all with proper error propagation. The --yes flag is wired correctly through the CLI layer. The new FileAbsent / FileContains check helpers and the two Scenario-based e2e tests cover both the confirmed and declined paths cleanly.

Two hypotheses were raised and investigated during review:

  • Non-TTY stdin behaviour without --yes: NewPrompt already branches on stdin.IsTerminal() and returns a Pipe implementation for non-TTY contexts. Pipe.Confirm reads via fmt.Fscanln; on EOF it silently gets an empty string, StringToBool("")false, and the convert is declined with an explicit error — no hang, no silent data destruction. ✅ Dismissed.
  • E2E declined-path test hanging: The test intentionally exercises the non-TTY decline path. Because Pipe.Confirm returns false on EOF stdin, the command exits with code 1 and the guard file is untouched — exactly what ExitCode(1) + FileContains assert. ✅ Dismissed.

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.45455% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/bridge/convert.go 63.88% 9 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

@ndeloof
ndeloof merged commit a896529 into docker:main Sep 3, 2026
52 of 53 checks passed
@ndeloof
ndeloof deleted the fix/bridge-convert-unsafe-output-removal branch September 3, 2026 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants