Skip to content

file: validate output path for OS-specific invalid characters#4053

Open
twmb wants to merge 1 commit intomainfrom
1005
Open

file: validate output path for OS-specific invalid characters#4053
twmb wants to merge 1 commit intomainfrom
1005

Conversation

@twmb
Copy link
Contributor

@twmb twmb commented Mar 2, 2026

Override the benthos file output with a connect-local implementation that rejects characters causing silent data loss: colons on macOS (HFS+/APFS maps ':' to '/'), and <, >, :, ", |, ?, * plus control characters on Windows. NUL bytes are rejected on all platforms.

Fixes #1005.

@claude
Copy link

claude bot commented Mar 2, 2026

Claude finished @twmb's task in 4m 40s —— View job


  • Setup: fetch PR data and read agent guidelines
  • Step 1: Commit policy validation
  • Step 2: Code review
  • Step 3: Post summary

Commits

LGTM

Review

Clean implementation that overrides the benthos file output to add OS-specific path validation. Registration pattern, field name constants, config parsing, mutex usage, and test structure all follow project conventions. One minor pattern violation found.

  1. Error wrap prefix at output_file.go#L187 uses "path interpolation error: %w" instead of gerund form ("interpolating path: %w") per godev.md.

@claude
Copy link

claude bot commented Mar 2, 2026

Claude finished @twmb's task in 1m 40s —— View job


  • Setup: fetch PR data and read agent guidelines
  • Step 1: Commit policy validation
  • Step 2: Code review
  • Step 3: Post summary

Commits

LGTM

Review

Reviewed 4 files: new file output override with OS-specific path validation (output_file.go, 336 lines), table-driven tests (output_file_test.go, 159 lines), doc reorder (file.adoc, +2/-2), and public wrapper import (package.go, +3 lines). Component registration, ConfigSpec construction, field name constants, ParsedConfig extraction, error handling, mutex encapsulation, idempotent Close, license headers (2026), and test patterns all follow project conventions.

  1. Error wrap prefix at output_file.go#L187 still uses "path interpolation error: %w" instead of gerund form ("interpolating path: %w") per godev.md. (Same issue as previous review.)

@mmatczuk
Copy link
Contributor

mmatczuk commented Mar 3, 2026

Override the benthos file output with a connect-local implementation
that rejects characters causing silent data loss: colons on macOS
(HFS+/APFS maps ':' to '/'), and <, >, :, ", |, ?, * plus control
characters on Windows. NUL bytes are rejected on all platforms.

Fixes #1005.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines +224 to +229
handle, ok := file.(io.WriteCloser)
if !ok {
_ = file.Close()
return errors.New("opening file for writing")
}

Copy link

Choose a reason for hiding this comment

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

Bug: If w.handle.Close() returns an error here, w.handle is not set to nil. Every subsequent Write call with a different path will re-attempt closing the same broken handle, fail with the same error, and never be able to open a new file — the output gets permanently stuck.

Note that Close() at line 272 already handles this correctly by always setting w.handle = nil regardless of the close error. Apply the same pattern here: set w.handle = nil before (or unconditionally after) the close error check.

@claude
Copy link

claude bot commented Mar 16, 2026

Commits
LGTM

Review
New connect-local file output overriding the benthos implementation, adding OS-specific path validation. Clean implementation with proper field name constants, config parsing, error wrapping, mutex encapsulation, and idempotent Close. Tests cover the new validation logic well.

  1. Bug — handle not cleared on close error in Write: When switching paths, if closing the old file handle fails, w.handle is never nilled out, permanently blocking all subsequent writes to different paths. The Close() method already handles this correctly. See inline comment.

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.

Invalid output file name succeeds ambiguously

2 participants