🔒 Fix command injection, path traversal, and file disclosure in deposits

[kirkkwang]

Attacker-controlled Content-Disposition filenames and deposited archives flowed unsanitized into shell commands and filesystem paths.

• Remove both backtick shell sinks: md5sum -> Digest::MD5.file, file --mime-type -> Marcel::MimeType.for (CWE-78)
• Parse Content-Disposition properly (filename* precedence, quoted values, case-insensitive) and sanitize the result: File.basename + strip control/bidi chars, blocking path traversal
• Reject deposits whose extracted archive contains a symlink, which the bagit copy would dereference to ingest arbitrary server files

[Copilot]

Pull request overview

Hardens deposit processing against command injection, path traversal, and symlink-based file disclosure.

Changes:

• Replaces shell-based checksum and MIME detection.
• Sanitizes Content-Disposition and multipart filenames.
• Rejects extracted archives containing symbolic links.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
app/controllers/concerns/willow_sword/fetch_headers.rb	Parses and sanitizes filenames.
app/controllers/concerns/willow_sword/process_request.rb	Uses Ruby for MD5 checksums.
app/controllers/concerns/willow_sword/save_data.rb	Sanitizes uploads, detects MIME types, and rejects symlinks.
spec/controllers/concerns/willow_sword/fetch_headers_spec.rb	Tests filename handling.
spec/controllers/concerns/willow_sword/save_data_spec.rb	Tests symlink rejection.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

app/controllers/concerns/willow_sword/fetch_headers.rb:61

• scrub does not transcode an ASCII-8BIT header/upload filename, because every byte is valid in that encoding. A non-ASCII byte then reaches the UTF-8 control/bidi regexp on the next line and raises Encoding::CompatibilityError, turning a valid raw UTF-8 filename from Rack into a 500. Normalize binary input to UTF-8 before applying the Unicode regexp.

      name = name.to_s.scrub('').strip

app/controllers/concerns/willow_sword/fetch_headers.rb:36

• These unanchored expressions also match parameter names or values that merely contain filename (for example, xfilename=wrong.txt; filename=right.txt), so the wrong filename is selected. Parse semicolon-delimited parameters while respecting quoted strings, and only recognize a parameter whose complete name is filename or filename*.

      if (m = cd.match(/filename\*\s*=\s*([^;]+)/i))

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

app/controllers/concerns/willow_sword/save_data.rb:131

• Passing a Pathname lets Marcel fall back to the attacker-controlled filename extension when the bytes have no recognized magic. For example, arbitrary binary data named payload.zip is classified as application/zip, unlike the previous content-only file --mime-type check. organize_data then invokes unzip; because unzip_file's false result is ignored, a multipart deposit can continue with its payload silently omitted. Pass an IO without a name so archive handling is selected from file content only.

      Marcel::MimeType.for(Pathname.new(file_path))

app/controllers/concerns/willow_sword/fetch_headers.rb:36

• These searches are not scoped to actual parameter boundaries, so text inside another parameter name or quoted value is treated as filename*. For example, attachment; filename="notes filename*=draft.pdf" selects draft.pdf" instead of the quoted filename, and xfilename*=... also incorrectly wins precedence. Parse semicolon-delimited parameters while respecting quoted strings, then compare each parameter name exactly.

      if (m = cd.match(/filename\*\s*=\s*([^;]+)/i))

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

app/controllers/concerns/willow_sword/save_data.rb:163

• This drops the @error assignment that the work controller's local rescue StandardError relies on. Invalid BagIt deposits are therefore rewrapped as the default 400 instead of 422, regressing the existing expectation in spec/request/v2/works_spec.rb:428-432. Preserve @error while raising the valid exception wrapper.

        raise WillowSword::SwordError.new(WillowSword::Error.new(message, :unprocessable_entity))

app/controllers/concerns/willow_sword/save_data.rb:140

• WorksController catches this exception inside create/update and preserves the status only when @error is already set; otherwise it wraps the exception message in the default 400 error. Consequently, a symlink archive sent to a work endpoint will return 400 rather than the intended 422. Assign the embedded error before raising so both work and file-set controllers retain the status.

This issue also appears on line 163 of the same file.

      raise WillowSword::SwordError.new(WillowSword::Error.new(message, :unprocessable_entity))

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.