fix(screenshot): disable base64 line wrap so STANDARD decoder accepts output#2
Merged
Conversation
todie
force-pushed
the
fix/screenshot-base64-wrap
branch
from
April 7, 2026 01:29
6388619 to
b1e07ef
Compare
todie
added a commit
that referenced
this pull request
Apr 7, 2026
…4+ schema Two pre-existing issues on main that were silently breaking CI on every open PR (#1 release-please, #2 screenshot fix, #3 editorconfig) via the Format and Deny jobs. Neither was introduced by the open PRs — they landed in the CI/CD overhaul commit 'ea74fa4 feat: CI/CD overhaul + process restart loop + zero clippy warnings' (the 'zero clippy warnings' part was true; the 'cargo fmt --check passes' part apparently wasn't). ## 1. rustfmt violations across 17 files `cargo fmt --check` on main produces 86 diff hunks across: crates/reach-cli/src/commands/{connect,create,destroy,exec,list,screenshot,serve,vnc}.rs crates/reach-cli/src/{config,docker,mcp}.rs crates/reach-cli/tests/{docker_types,e2e_container,mcp_types}.rs crates/reach-supervisor/src/{main,processes,signals}.rs Applied `cargo fmt --all` with the project's rustfmt.toml (max_width=100). No behavioral changes — pure whitespace/alignment/import-grouping edits. ## 2. deny.toml schema migration (cargo-deny 0.14+) cargo-deny 0.14 removed or changed several schema keys: - `[advisories] vulnerability = "deny"` → REMOVED (cargo-deny now always denies vulnerabilities in its pipeline). - `[advisories] unmaintained = "warn"` → REJECTED. The field's semantics changed from severity ("deny"/"warn") to scope ("all"/"workspace"/"transitive"/"none"). Migrated to "workspace" — we only care about crates we directly depend on being marked unmaintained, not transitives we don't control. - `[licenses] unlicensed = "deny"` → REMOVED (see EmbarkStudios/cargo-deny#611 for the migration note). cargo-deny now handles unlicensed crates as deny-by-default in the licenses pipeline without the explicit key. ## 3. RUSTSEC-2024-0437 ignore (protobuf 2.28 stack overflow) After the schema migration, `cargo deny check` surfaces a real vulnerability: protobuf 2.28.0 is transitively pulled in by `prometheus 0.13.4 → reach-supervisor`. RUSTSEC-2024-0437 is a stack overflow caused by unbounded recursion when PARSING untrusted protobuf input. reach-supervisor uses prometheus to EXPORT metrics (serialise to the Prometheus text format, served over an HTTP endpoint on port 8400). It never parses protobuf input from the network or from untrusted sources, so the vector does not apply. Added an explicit `[advisories] ignore` with a detailed justification comment so the rationale is preserved inline. Follow-up: upgrade to a prometheus version that uses protobuf 3.x, or drop the prometheus crate in favour of a pure-text-format exporter. ## Verification Local CI parity: `cargo fmt --check` clean `cargo deny check` advisories ok, bans ok, licenses ok, sources ok `cargo clippy --workspace -- -D warnings` zero warnings `cargo build --workspace --release` clean After this lands on main, the Format + Deny jobs will start passing on open PRs #1, #2, #3 without any per-branch rebasing (the fixes are on main, not downstream). Worthwhile to cascade-rebase the three so they pick up the fmt-normalised line-lengths in docker.rs (which PR #2 touches).
2 tasks
todie
added a commit
that referenced
this pull request
Apr 7, 2026
…4+ schema Two pre-existing issues on main that were silently breaking CI on every open PR (#1 release-please, #2 screenshot fix, #3 editorconfig) via the Format and Deny jobs. Neither was introduced by the open PRs — they landed in the CI/CD overhaul commit 'ea74fa4 feat: CI/CD overhaul + process restart loop + zero clippy warnings' (the 'zero clippy warnings' part was true; the 'cargo fmt --check passes' part apparently wasn't). ## 1. rustfmt violations across 17 files `cargo fmt --check` on main produces 86 diff hunks across: crates/reach-cli/src/commands/{connect,create,destroy,exec,list,screenshot,serve,vnc}.rs crates/reach-cli/src/{config,docker,mcp}.rs crates/reach-cli/tests/{docker_types,e2e_container,mcp_types}.rs crates/reach-supervisor/src/{main,processes,signals}.rs Applied `cargo fmt --all` with the project's rustfmt.toml (max_width=100). No behavioral changes — pure whitespace/alignment/import-grouping edits. ## 2. deny.toml schema migration (cargo-deny 0.14+) cargo-deny 0.14 removed or changed several schema keys: - `[advisories] vulnerability = "deny"` → REMOVED (cargo-deny now always denies vulnerabilities in its pipeline). - `[advisories] unmaintained = "warn"` → REJECTED. The field's semantics changed from severity ("deny"/"warn") to scope ("all"/"workspace"/"transitive"/"none"). Migrated to "workspace" — we only care about crates we directly depend on being marked unmaintained, not transitives we don't control. - `[licenses] unlicensed = "deny"` → REMOVED (see EmbarkStudios/cargo-deny#611 for the migration note). cargo-deny now handles unlicensed crates as deny-by-default in the licenses pipeline without the explicit key. ## 3. RUSTSEC-2024-0437 ignore (protobuf 2.28 stack overflow) After the schema migration, `cargo deny check` surfaces a real vulnerability: protobuf 2.28.0 is transitively pulled in by `prometheus 0.13.4 → reach-supervisor`. RUSTSEC-2024-0437 is a stack overflow caused by unbounded recursion when PARSING untrusted protobuf input. reach-supervisor uses prometheus to EXPORT metrics (serialise to the Prometheus text format, served over an HTTP endpoint on port 8400). It never parses protobuf input from the network or from untrusted sources, so the vector does not apply. Added an explicit `[advisories] ignore` with a detailed justification comment so the rationale is preserved inline. Follow-up: upgrade to a prometheus version that uses protobuf 3.x, or drop the prometheus crate in favour of a pure-text-format exporter. ## Verification Local CI parity: `cargo fmt --check` clean `cargo deny check` advisories ok, bans ok, licenses ok, sources ok `cargo clippy --workspace -- -D warnings` zero warnings `cargo build --workspace --release` clean After this lands on main, the Format + Deny jobs will start passing on open PRs #1, #2, #3 without any per-branch rebasing (the fixes are on main, not downstream). Worthwhile to cascade-rebase the three so they pick up the fmt-normalised line-lengths in docker.rs (which PR #2 touches).
…eject coreutils base64(1) wraps at 76 chars by default, which the STANDARD base64 decoder in the base64 crate rejects with "Invalid symbol 10, offset 76" because it does not tolerate whitespace between symbols. The one-line fix is passing -w 0 to the in-container base64 command. Added a defensive whitespace strip on the decoder side too, in case the CLI flag is ever unavailable on a non-coreutils runtime. Fixes: TOD-302 Tested: `reach screenshot cf-setup -o /tmp/shot.png` now produces a valid 1920x1080 PNG (verified with file(1) and pixel dimension check) against a running Ubuntu 24.04 sandbox.
todie
force-pushed
the
fix/screenshot-base64-wrap
branch
from
April 7, 2026 10:17
b1e07ef to
15a9e51
Compare
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
1 participant
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.
Bug
`reach screenshot ` and the MCP `screenshot` tool both failed on every invocation with:
```
Error: failed to decode screenshot base64
Caused by:
Invalid symbol 10, offset 76.
```
Symbol 10 = `\n`, offset 76 = end of the first MIME line. The fail site is:
```rust
// crates/reach-cli/src/docker.rs:437 (pre-fix)
"scrot -z /tmp/_reach_shot.png && base64 /tmp/_reach_shot.png && rm /tmp/_reach_shot.png"
```
GNU coreutils `base64` defaults to RFC 2045 line wrapping (76 chars per line). The decoder on the Rust side is `base64::engine::general_purpose::STANDARD`, which is strict and rejects any whitespace between symbols. `.trim()` only strips leading/trailing whitespace, not interior newlines.
This broke:
Fix
Two changes in `docker.rs::screenshot`:
Verification
On a running Ubuntu 24.04 sandbox (`reach create --name cf-setup --resolution 1920x1080`) before the fix:
```
$ reach screenshot cf-setup -o /tmp/shot.png
Error: failed to decode screenshot base64
Caused by: Invalid symbol 10, offset 76.
```
After:
```
$ reach screenshot cf-setup -o /tmp/shot.png
✓ Screenshot saved to /tmp/shot.png (6136 bytes)
$ file /tmp/shot.png
/tmp/shot.png: PNG image data, 1920 x 1080, 8-bit/color RGB, non-interlaced
```
Built via `cargo build --release -p reach-cli`, installed via `cargo install --path crates/reach-cli --force`, re-ran on the existing cf-setup sandbox, verified valid PNG bytes.
Related
Follow-up candidates (not in this PR)
🤖 Generated with Claude Code