Skip to content

Allow user to save patch file to disk#424

Merged
wesm merged 4 commits intoroborev-dev:mainfrom
tebeka:save-patch
Mar 6, 2026
Merged

Allow user to save patch file to disk#424
wesm merged 4 commits intoroborev-dev:mainfrom
tebeka:save-patch

Conversation

@tebeka
Copy link
Contributor

@tebeka tebeka commented Mar 4, 2026

When user hits s in patch view, it'll be prompted for file name (default to /tmp/roborev-<id>.patch).

Somewhat related to #407

@roborev-ci
Copy link

roborev-ci bot commented Mar 4, 2026

roborev: Combined Review (e70f1f5)

Summary Verdict: Adds an interactive "save patch to file" flow in the TUI patch viewer, but
introduces medium-severity path construction bugs and file-creation security risks.

Medium Severity Findings

1. Non-portable / Incorrect Default Save Path Construction

  • File: cmd/roborev/tui/handlers_modal.go (lines 512, 514)

  • Description: The default path is constructed with hardcoded slashes using fmt.Sprintf("/%s/roborev-%d.patch", os.TempDir(), m.patchJobID). This results in invalid absolute paths on Windows (e.g., /C:\Temp\...) and redundant double
    -slashes on POSIX systems (e.g., //tmp//...).

  • Suggested Fix: Import path/filepath and use filepath.Join(os.TempDir(), fmt.Sprintf("roborev-%d.patch", m.patchJobID)).

2.
Predictable /tmp Filename / Insecure File Creation (Symlink Clobbering)

  • File: cmd/roborev/tui/handlers_modal.go (lines 514, 548)
  • Description: The default save path is deterministic (
    /tmp/roborev-<jobID>.patch), and os.WriteFile will follow symlinks and truncate existing targets. On multi-user systems, another local user could pre-create a symlink at that path and cause overwrite of arbitrary files writable by the victim user.
  • Suggested Fix: Use
    a secure creation flow: os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) (or os.CreateTemp for default path generation), and verify targets are not symlinks.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@tebeka
Copy link
Contributor Author

tebeka commented Mar 4, 2026

Fixed 1, ino 2 is a non issue

@roborev-ci
Copy link

roborev-ci bot commented Mar 4, 2026

roborev: Combined Review (c50df69)

Verdict: The new patch saving feature is functional but contains a few medium-severity issues regarding predictable temporary files, byte-based string truncation, and a lack of targeted test coverage.

Medium

  • Predictable filename in shared temp space / Symlink overwrite

  • Location: cmd/roborev/tui/handlers_modal.go:515, cmd/roborev/t ui/handlers_modal.go:549

    • Issue: Default save path uses a predictable filename in shared temp space (/tmp/ roborev-<jobID>.patch) and writes via os.WriteFile, which follows symlinks. On multi-user systems, another local user can pre-create that path as a symlink, causing overwrite/truncation of an arbitrary file the victim can write.
    • Remediation: Use
      os.CreateTemp (randomized filename) in a private directory (0700) and write through the returned file descriptor. If supporting user-provided paths, open with safer flags (O_CREATE|O_EXCL) and reject symlinks (Lstat/EvalSymlinks +
      policy, or platform-specific no-follow behavior).
  • Filename input truncation is byte-based

    • Location: [cmd/roborev/tui/render_tasks.go:501-503](/home/roborev/repos/roborev
      /cmd/roborev/tui/render_tasks.go:501)
    • Issue: Slicing the display string using its byte length (len(display) > inputWidth) can split multi-byte characters in half, resulting in invalid UTF-8 sequences and
      rendering artifacts if the file path contains non-ASCII characters.
    • Remediation: Convert display to a slice of runes before checking its length and slicing (e.g., runes := []rune(display) then string(runes[len(runes)-inputWidth:]) ), or use a display-width aware function consistent with other TUI rendering code.
  • New save flow lacks targeted tests

    • Location: [cmd/roborev/tui/handlers_modal.go:470](/home/roborev/repos/robore
      v/cmd/roborev/tui/handlers_modal.go:470), [cmd/roborev/tui/handlers_msg.go:657](/home/roborev/repos/roborev/cmd/roborev/tui/handlers
      _msg.go:657), cmd/roborev/tui/render_tasks.go:498, cmd/ roborev/tui/types.go:233
    • Issue: No new _test.go coverage appears for: entering/ex
      iting save-input mode, default path population, save success/error handling, and patch-view rendering while input is active.
    • Remediation: Add unit tests for handlePatchKey, handleSavePatchResultMsg, and renderPatchView save-mode UI branches.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@tebeka
Copy link
Contributor Author

tebeka commented Mar 5, 2026

Fixed reivew #2 in df6a151
IMO 1 is not an issue and don't see any tests for 3

@roborev-ci
Copy link

roborev-ci bot commented Mar 5, 2026

roborev: Combined Review (df6a151)

Verdict: The patch export functionality introduces a predictable temporary file vulnerability and lacks test coverage; these issues should be addressed before merging.

Medium

1. Predictable Temp Filename / Symlink Clob
bering Vulnerability

  • Locations: cmd/roborev/tui/handlers_modal.go (Lines 510-548)

  • Issue: The default save path is deterministic (e.g., /tmp/roborev-<jobID>.patch), and os.WriteFile follows symlinks while truncating existing files. On multi-user systems, a local attacker could pre-create a symlink at the predictable path, causing the
    application to overwrite an unintended target file when the operator saves the patch.

  • Remediation: Generate default paths using os.CreateTemp for unpredictability, or use os.OpenFile with O_CREATE|O_EXCL to ensure the write fails safely if a file or
    symlink already exists.

2. Missing TUI Unit Tests

  • Locations: [cmd/roborev/tui/handlers_modal.go](/home/roborev/repos/roborev/cmd/roborev/tui/handlers_modal.go#L
    468), cmd/roborev/tui/handlers_msg.go, cmd/roborev/tui/render_
    tasks.go

  • Issue: New behaviors (save input mode, key handling, async save result handling, and filename rendering) were added without corresponding tests.

  • Remediation: Add unit tests for handlePatchKey (covering s, enter, esc, backspace), handleSavePatchResultMsg success and error paths, and render behavior with non-ASCII paths.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

tebeka and others added 4 commits March 5, 2026 19:38
Both truncation and padding were using inconsistent width measures
(rune count vs byte length). Switch to runewidth.StringWidth for
correct terminal alignment with wide/combining characters.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link

roborev-ci bot commented Mar 6, 2026

roborev: Combined Review (563f5a7)

Verdict: The patch-export feature adds useful functionality to the TUI but introduces a medium-severity security vulnerability related to predictable file creation.

Medium

  • Predictable temporary file creation and insecure write
    • Where
      :
      cmd/roborev/tui/handlers_modal.go:513, cmd/roborev/tui/handlers_
      modal.go:548
    • Issue: The default save path is a predictable filename in a shared temp directory (/tmp/robore v-<jobID>.patch), and writes use os.WriteFile (truncate/create, follows symlinks, no exclusivity). A local attacker on multi-user systems could pre-create or link that path and cause unintended overwrite/redirection when the user accepts the default.
    • Remediation:
      Create temp files with os.CreateTemp for unique names, and for manual paths use os.OpenFile with secure flags (O_CREATE|O_EXCL|O_WRONLY) plus explicit overwrite confirmation if the file exists.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm wesm merged commit 4abe3e2 into roborev-dev:main Mar 6, 2026
8 checks passed
@wesm
Copy link
Collaborator

wesm commented Mar 6, 2026

thanks!

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.

2 participants