Skip to content

fix: safely handle Content-Disposition download filenames - #237

Closed
ryanduguid wants to merge 7 commits into
XeroAPI:masterfrom
ryanduguid:audit/safe-content-disposition-filenames
Closed

fix: safely handle Content-Disposition download filenames#237
ryanduguid wants to merge 7 commits into
XeroAPI:masterfrom
ryanduguid:audit/safe-content-disposition-filenames

Conversation

@ryanduguid

@ryanduguid ryanduguid commented Aug 6, 2026

Copy link
Copy Markdown

Summary

  • parse quoted and RFC 2231 encoded Content-Disposition filenames with the standard-library MIME parser
  • extract relative leaves using both POSIX and Windows separators
  • reject absolute, drive, UNC, control, invalid and Windows-reserved names
  • resolve and contain the final path under the configured temporary directory
  • write through an already-open private temporary file and claim the requested leaf with an atomic same-directory hard link, preserving the random path if the name exists or the filesystem cannot make the link
  • avoid name-based cleanup after a failed claim, so a concurrent replacement is never removed
  • add adversarial traversal, encoded/quoted, malformed-header, reserved-name, control-character, regular-file preservation, injected race/replacement and symlink regressions

This SDK contains generated client code. The same hardening should also be carried into Xero's customised Python API-client template before the next regeneration.

Validation

@ryanduguid
ryanduguid marked this pull request as draft August 12, 2026 17:49
@ryanduguid
ryanduguid marked this pull request as ready for review August 18, 2026 15:06
os.link is not in os.supports_follow_symlinks on Windows and the source is
always a fresh mkstemp regular file, so follow_symlinks=False never had an
effect. On a build that honours the documented contract it raises
NotImplementedError, a RuntimeError subclass that escapes the except OSError
and breaks every file download. Drop the argument; destination symlinks are
still refused because os.link fails when the destination exists.

Cover the whole Windows reserved-name set with literal names rather than
deriving them from the implementation constant, so narrowing the COM/LPT
range fails the suite instead of passing silently. Add cases for names that
sanitise down to an empty string, which only the ("", ".", "..") guard
rejects and which would otherwise resolve to the download directory itself.
@ryanduguid

Copy link
Copy Markdown
Author

Closing this as part of cleaning up an unsolicited batch I opened across several Xero repositories. I will not reopen unless a maintainer asks for a single focused change.

@ryanduguid ryanduguid closed this Aug 20, 2026
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.

1 participant