From 07a46ea77084637d677b502bdc1946b472e0d54d Mon Sep 17 00:00:00 2001 From: Ryan Duguid Date: Fri, 7 Aug 2026 01:00:37 +1000 Subject: [PATCH 1/6] fix: sanitize downloaded response filenames --- tests/test_api_client/test_file_download.py | 58 +++++++++++++++++++++ xero_python/api_client/__init__.py | 13 +++-- 2 files changed, 67 insertions(+), 4 deletions(-) create mode 100644 tests/test_api_client/test_file_download.py diff --git a/tests/test_api_client/test_file_download.py b/tests/test_api_client/test_file_download.py new file mode 100644 index 00000000..9be78ee0 --- /dev/null +++ b/tests/test_api_client/test_file_download.py @@ -0,0 +1,58 @@ +# -*- coding: utf-8 -*- +from pathlib import Path + +import pytest + +from xero_python.api_client import ApiClient +from xero_python.api_client.configuration import Configuration + + +class FakeResponse: + def __init__(self, content_disposition, data=b"file contents"): + self.content_disposition = content_disposition + self.data = data + + def getheader(self, name): + if name == "Content-Disposition": + return self.content_disposition + return None + + +@pytest.fixture +def api_client(tmp_path): + configuration = Configuration() + configuration.temp_folder_path = str(tmp_path) + return ApiClient(configuration=configuration) + + +def deserialize_file(api_client, response): + return Path(api_client._ApiClient__deserialize_file(response)) + + +def test_deserialize_file_keeps_download_within_temp_directory(api_client, tmp_path): + path = deserialize_file( + api_client, FakeResponse('attachment; filename="../outside.txt"') + ) + + assert path.parent == tmp_path + assert path.name == "outside.txt" + assert path.read_bytes() == b"file contents" + assert not (tmp_path.parent / "outside.txt").exists() + + +def test_deserialize_file_uses_content_disposition_filename(api_client, tmp_path): + path = deserialize_file( + api_client, FakeResponse('attachment; filename="report.csv"') + ) + + assert path == tmp_path / "report.csv" + assert path.read_bytes() == b"file contents" + + +def test_deserialize_file_uses_generated_filename_without_filename_parameter( + api_client, tmp_path +): + path = deserialize_file(api_client, FakeResponse("inline")) + + assert path.parent == tmp_path + assert path.read_bytes() == b"file contents" diff --git a/xero_python/api_client/__init__.py b/xero_python/api_client/__init__.py index 645cf506..3417979e 100644 --- a/xero_python/api_client/__init__.py +++ b/xero_python/api_client/__init__.py @@ -603,10 +603,15 @@ def __deserialize_file(self, response): content_disposition = response.getheader("Content-Disposition") if content_disposition: - filename = re.search( - r'filename=[\'"]?([^\'"\s]+)[\'"]?', content_disposition - ).group(1) - path = os.path.join(os.path.dirname(path), filename) + match = re.search( + r'filename=[\'"]?([^\'"\s]+)[\'"]?', + content_disposition, + flags=re.IGNORECASE, + ) + if match: + filename = os.path.basename(match.group(1)) + if filename not in ("", ".", ".."): + path = os.path.join(os.path.dirname(path), filename) with open(path, "wb") as f: f.write(response.data) From a904f4665936295c6666025563564b933bc4920d Mon Sep 17 00:00:00 2001 From: Ryan Duguid <152749594+ryanduguid@users.noreply.github.com> Date: Thu, 13 Aug 2026 03:11:50 +1000 Subject: [PATCH 2/6] fix: harden downloaded filename handling --- tests/test_api_client/test_file_download.py | 82 ++++++++++++++++++++- xero_python/api_client/__init__.py | 78 ++++++++++++++++---- 2 files changed, 143 insertions(+), 17 deletions(-) diff --git a/tests/test_api_client/test_file_download.py b/tests/test_api_client/test_file_download.py index 9be78ee0..57e516df 100644 --- a/tests/test_api_client/test_file_download.py +++ b/tests/test_api_client/test_file_download.py @@ -29,10 +29,17 @@ def deserialize_file(api_client, response): return Path(api_client._ApiClient__deserialize_file(response)) -def test_deserialize_file_keeps_download_within_temp_directory(api_client, tmp_path): - path = deserialize_file( - api_client, FakeResponse('attachment; filename="../outside.txt"') - ) +@pytest.mark.parametrize( + "header", + [ + 'attachment; filename="../outside.txt"', + 'attachment; filename="..\\outside.txt"', + ], +) +def test_deserialize_file_keeps_traversal_within_temp_directory( + api_client, tmp_path, header +): + path = deserialize_file(api_client, FakeResponse(header)) assert path.parent == tmp_path assert path.name == "outside.txt" @@ -40,6 +47,34 @@ def test_deserialize_file_keeps_download_within_temp_directory(api_client, tmp_p assert not (tmp_path.parent / "outside.txt").exists() +@pytest.mark.parametrize( + "header", + [ + 'attachment; filename="/outside.txt"', + 'attachment; filename="C:\\outside.txt"', + 'attachment; filename="\\\\server\\share\\outside.txt"', + 'attachment; filename="NUL.txt"', + 'attachment; filename="bad\x00name.txt"', + 'attachment; filename="spoof\u202ename.txt"', + 'attachment; filename="report.csv."', + ], +) +def test_deserialize_file_rejects_unsafe_cross_platform_names( + api_client, tmp_path, header +): + path = deserialize_file(api_client, FakeResponse(header)) + + assert path.parent == tmp_path + assert path.name not in { + "outside.txt", + "NUL.txt", + "bad\x00name.txt", + "spoof\u202ename.txt", + "report.csv.", + } + assert path.read_bytes() == b"file contents" + + def test_deserialize_file_uses_content_disposition_filename(api_client, tmp_path): path = deserialize_file( api_client, FakeResponse('attachment; filename="report.csv"') @@ -49,6 +84,45 @@ def test_deserialize_file_uses_content_disposition_filename(api_client, tmp_path assert path.read_bytes() == b"file contents" +@pytest.mark.parametrize( + "header,expected", + [ + ( + "attachment; filename*=UTF-8''..%5Cencoded%20report.csv", + "encoded report.csv", + ), + ('attachment; filename="quarter; report.csv"', "quarter; report.csv"), + ], +) +def test_deserialize_file_parses_encoded_and_quoted_names( + api_client, tmp_path, header, expected +): + path = deserialize_file(api_client, FakeResponse(header)) + + assert path == tmp_path / expected + assert path.read_bytes() == b"file contents" + + +def test_deserialize_file_does_not_follow_existing_symlink(api_client, tmp_path): + target = tmp_path.parent / "symlink-target.txt" + target.write_bytes(b"do not overwrite") + link = tmp_path / "report.csv" + try: + link.symlink_to(target) + except OSError as error: + pytest.skip("symlinks are unavailable: {}".format(error)) + + path = deserialize_file( + api_client, FakeResponse('attachment; filename="report.csv"') + ) + + assert path != link + assert path.parent == tmp_path + assert path.read_bytes() == b"file contents" + assert link.is_symlink() + assert target.read_bytes() == b"do not overwrite" + + def test_deserialize_file_uses_generated_filename_without_filename_parameter( api_client, tmp_path ): diff --git a/xero_python/api_client/__init__.py b/xero_python/api_client/__init__.py index 3417979e..6c58ba3f 100644 --- a/xero_python/api_client/__init__.py +++ b/xero_python/api_client/__init__.py @@ -13,10 +13,14 @@ import datetime import json import mimetypes +import ntpath import os +import posixpath import re import tempfile +import unicodedata from decimal import Decimal +from email.message import Message from multiprocessing.pool import ThreadPool from urllib.parse import quote @@ -27,6 +31,48 @@ from xero_python.api_client.serializer import serialize from xero_python.exceptions import OAuth2TokenGetterError, OAuth2TokenSaverError +WINDOWS_RESERVED_FILENAMES = {"CON", "PRN", "AUX", "NUL"} +WINDOWS_RESERVED_FILENAMES.update("COM{}".format(number) for number in range(1, 10)) +WINDOWS_RESERVED_FILENAMES.update("LPT{}".format(number) for number in range(1, 10)) + + +def safe_download_filename(content_disposition): + message = Message() + message["Content-Disposition"] = content_disposition + filename = message.get_filename() + if not filename or any( + unicodedata.category(character).startswith("C") for character in filename + ): + return None + if ( + posixpath.isabs(filename) + or ntpath.isabs(filename) + or ntpath.splitdrive(filename)[0] + ): + return None + + filename = re.split(r"[\\/]", filename)[-1] + if filename in ("", ".", "..") or filename != filename.rstrip(" ."): + return None + if any(character in '<>:"|?*' for character in filename): + return None + if filename.split(".", 1)[0].upper() in WINDOWS_RESERVED_FILENAMES: + return None + return filename + + +def safe_download_path(directory, filename): + directory = os.path.realpath(directory) + path = os.path.join(directory, filename) + try: + if os.path.commonpath((directory, os.path.realpath(path))) != directory: + return None + except ValueError: + return None + if os.path.islink(path) or (os.path.lexists(path) and not os.path.isfile(path)): + return None + return path + class ModelFinder: """ @@ -598,23 +644,29 @@ def __deserialize_file(self, response): :return: file path. """ fd, path = tempfile.mkstemp(dir=self.configuration.temp_folder_path) - os.close(fd) - os.remove(path) + + try: + with os.fdopen(fd, "wb") as file_handle: + file_handle.write(response.data) + except Exception: + os.remove(path) + raise content_disposition = response.getheader("Content-Disposition") if content_disposition: - match = re.search( - r'filename=[\'"]?([^\'"\s]+)[\'"]?', - content_disposition, - flags=re.IGNORECASE, + filename = safe_download_filename(content_disposition) + destination = ( + safe_download_path(os.path.dirname(path), filename) + if filename + else None ) - if match: - filename = os.path.basename(match.group(1)) - if filename not in ("", ".", ".."): - path = os.path.join(os.path.dirname(path), filename) - - with open(path, "wb") as f: - f.write(response.data) + if destination: + try: + os.replace(path, destination) + except Exception: + os.remove(path) + raise + path = destination return path From 273b099f560e567c577625db32848218af1d19aa Mon Sep 17 00:00:00 2001 From: Ryan Duguid <152749594+ryanduguid@users.noreply.github.com> Date: Thu, 13 Aug 2026 03:16:45 +1000 Subject: [PATCH 3/6] test: preserve existing download destinations --- tests/test_api_client/test_file_download.py | 30 +++++++++++++++++++++ xero_python/api_client/__init__.py | 20 +++++++------- 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/tests/test_api_client/test_file_download.py b/tests/test_api_client/test_file_download.py index 57e516df..b7786a1e 100644 --- a/tests/test_api_client/test_file_download.py +++ b/tests/test_api_client/test_file_download.py @@ -123,6 +123,36 @@ def test_deserialize_file_does_not_follow_existing_symlink(api_client, tmp_path) assert target.read_bytes() == b"do not overwrite" +def test_deserialize_file_preserves_existing_regular_file(api_client, tmp_path): + destination = tmp_path / "report.csv" + destination.write_bytes(b"do not overwrite") + + path = deserialize_file( + api_client, FakeResponse('attachment; filename="report.csv"') + ) + + assert path != destination + assert path.parent == tmp_path + assert path.read_bytes() == b"file contents" + assert destination.read_bytes() == b"do not overwrite" + + +@pytest.mark.parametrize( + "header", + [ + 'attachment; filename="unterminated', + "attachment; filename*=UTF-8''bad%ZZname", + ], +) +def test_deserialize_file_handles_malformed_content_disposition_safely( + api_client, tmp_path, header +): + path = deserialize_file(api_client, FakeResponse(header)) + + assert path.parent == tmp_path + assert path.read_bytes() == b"file contents" + + def test_deserialize_file_uses_generated_filename_without_filename_parameter( api_client, tmp_path ): diff --git a/xero_python/api_client/__init__.py b/xero_python/api_client/__init__.py index 6c58ba3f..88a7730a 100644 --- a/xero_python/api_client/__init__.py +++ b/xero_python/api_client/__init__.py @@ -1,15 +1,14 @@ # coding: utf-8 """ - Xero oAuth 2 identity service +Xero oAuth 2 identity service - This specifing endpoints related to managing authentication tokens and identity for Xero API # noqa: E501 +This specifing endpoints related to managing authentication tokens and identity for Xero API # noqa: E501 - OpenAPI spec version: 2.0.4 - Contact: api@xero.com - Generated by: https://openapi-generator.tech +OpenAPI spec version: 2.0.4 +Contact: api@xero.com +Generated by: https://openapi-generator.tech """ - import datetime import json import mimetypes @@ -38,8 +37,11 @@ def safe_download_filename(content_disposition): message = Message() - message["Content-Disposition"] = content_disposition - filename = message.get_filename() + try: + message["Content-Disposition"] = content_disposition + filename = message.get_filename() + except (TypeError, ValueError): + return None if not filename or any( unicodedata.category(character).startswith("C") for character in filename ): @@ -69,7 +71,7 @@ def safe_download_path(directory, filename): return None except ValueError: return None - if os.path.islink(path) or (os.path.lexists(path) and not os.path.isfile(path)): + if os.path.lexists(path): return None return path From 8759375d58a56a306edf9bc9b629bba3506a7b5c Mon Sep 17 00:00:00 2001 From: Ryan Duguid <152749594+ryanduguid@users.noreply.github.com> Date: Thu, 13 Aug 2026 03:19:42 +1000 Subject: [PATCH 4/6] fix: avoid races when naming downloads --- tests/test_api_client/test_file_download.py | 14 +++++++++ xero_python/api_client/__init__.py | 32 +++++++++++++++------ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/tests/test_api_client/test_file_download.py b/tests/test_api_client/test_file_download.py index b7786a1e..23c134ff 100644 --- a/tests/test_api_client/test_file_download.py +++ b/tests/test_api_client/test_file_download.py @@ -4,6 +4,7 @@ import pytest from xero_python.api_client import ApiClient +from xero_python.api_client import copy_download_without_overwrite from xero_python.api_client.configuration import Configuration @@ -137,6 +138,19 @@ def test_deserialize_file_preserves_existing_regular_file(api_client, tmp_path): assert destination.read_bytes() == b"do not overwrite" +def test_copy_download_preserves_destination_created_after_validation(tmp_path): + source = tmp_path / "secure-random-file" + source.write_bytes(b"file contents") + destination = tmp_path / "report.csv" + + destination.write_bytes(b"created by racer") + copied = copy_download_without_overwrite(str(source), str(destination)) + + assert not copied + assert source.read_bytes() == b"file contents" + assert destination.read_bytes() == b"created by racer" + + @pytest.mark.parametrize( "header", [ diff --git a/xero_python/api_client/__init__.py b/xero_python/api_client/__init__.py index 88a7730a..9a13a847 100644 --- a/xero_python/api_client/__init__.py +++ b/xero_python/api_client/__init__.py @@ -71,11 +71,31 @@ def safe_download_path(directory, filename): return None except ValueError: return None - if os.path.lexists(path): - return None return path +def copy_download_without_overwrite(source, destination): + try: + descriptor = os.open(destination, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + except FileExistsError: + return False + + try: + with open(source, "rb") as source_handle, os.fdopen( + descriptor, "wb" + ) as destination_handle: + destination_handle.write(source_handle.read()) + except Exception: + try: + os.close(descriptor) + except OSError: + pass + os.remove(destination) + raise + os.remove(source) + return True + + class ModelFinder: """ Model finder to find correct model class for given model name @@ -663,12 +683,8 @@ def __deserialize_file(self, response): else None ) if destination: - try: - os.replace(path, destination) - except Exception: - os.remove(path) - raise - path = destination + if copy_download_without_overwrite(path, destination): + path = destination return path From 1a88e492b3c3d381371d458838e252f46320a5d5 Mon Sep 17 00:00:00 2001 From: Ryan Duguid <152749594+ryanduguid@users.noreply.github.com> Date: Thu, 13 Aug 2026 03:36:59 +1000 Subject: [PATCH 5/6] Avoid unsafe cleanup races for downloads --- tests/test_api_client/test_file_download.py | 20 ++++++++++++++++++++ xero_python/api_client/__init__.py | 19 +++++-------------- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/tests/test_api_client/test_file_download.py b/tests/test_api_client/test_file_download.py index 23c134ff..7e1fa28d 100644 --- a/tests/test_api_client/test_file_download.py +++ b/tests/test_api_client/test_file_download.py @@ -151,6 +151,26 @@ def test_copy_download_preserves_destination_created_after_validation(tmp_path): assert destination.read_bytes() == b"created by racer" +def test_copy_download_preserves_replacement_when_atomic_claim_fails( + tmp_path, monkeypatch +): + source = tmp_path / "secure-random-file" + source.write_bytes(b"file contents") + destination = tmp_path / "report.csv" + + def replace_name_and_fail(source_path, destination_path, follow_symlinks): + Path(destination_path).write_bytes(b"created by racer") + raise OSError("injected atomic-claim failure") + + monkeypatch.setattr("xero_python.api_client.os.link", replace_name_and_fail) + + copied = copy_download_without_overwrite(str(source), str(destination)) + + assert not copied + assert source.read_bytes() == b"file contents" + assert destination.read_bytes() == b"created by racer" + + @pytest.mark.parametrize( "header", [ diff --git a/xero_python/api_client/__init__.py b/xero_python/api_client/__init__.py index 9a13a847..f94da764 100644 --- a/xero_python/api_client/__init__.py +++ b/xero_python/api_client/__init__.py @@ -76,23 +76,14 @@ def safe_download_path(directory, filename): def copy_download_without_overwrite(source, destination): try: - descriptor = os.open(destination, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) - except FileExistsError: + os.link(source, destination, follow_symlinks=False) + except OSError: return False try: - with open(source, "rb") as source_handle, os.fdopen( - descriptor, "wb" - ) as destination_handle: - destination_handle.write(source_handle.read()) - except Exception: - try: - os.close(descriptor) - except OSError: - pass - os.remove(destination) - raise - os.remove(source) + os.remove(source) + except OSError: + pass return True From 412d3616aca4e5da57cc2621fdcf929ed7bffd4e Mon Sep 17 00:00:00 2001 From: Ryan Duguid <152749594+ryanduguid@users.noreply.github.com> Date: Wed, 19 Aug 2026 15:04:29 +1000 Subject: [PATCH 6/6] fix: drop dead follow_symlinks arg and cover download name guards 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. --- tests/test_api_client/test_file_download.py | 56 ++++++++++++++++++++- xero_python/api_client/__init__.py | 2 +- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/tests/test_api_client/test_file_download.py b/tests/test_api_client/test_file_download.py index 7e1fa28d..3cbe3916 100644 --- a/tests/test_api_client/test_file_download.py +++ b/tests/test_api_client/test_file_download.py @@ -5,6 +5,7 @@ from xero_python.api_client import ApiClient from xero_python.api_client import copy_download_without_overwrite +from xero_python.api_client import safe_download_filename from xero_python.api_client.configuration import Configuration @@ -76,6 +77,59 @@ def test_deserialize_file_rejects_unsafe_cross_platform_names( assert path.read_bytes() == b"file contents" +@pytest.mark.parametrize( + "reserved_name", + [ + "CON", + "PRN", + "AUX", + "NUL", + "COM1", + "COM2", + "COM3", + "COM4", + "COM5", + "COM6", + "COM7", + "COM8", + "COM9", + "LPT1", + "LPT2", + "LPT3", + "LPT4", + "LPT5", + "LPT6", + "LPT7", + "LPT8", + "LPT9", + ], +) +def test_safe_download_filename_rejects_every_windows_reserved_name(reserved_name): + assert ( + safe_download_filename('attachment; filename="{}.csv"'.format(reserved_name)) + is None + ) + assert ( + safe_download_filename( + 'attachment; filename="{}"'.format(reserved_name.lower()) + ) + is None + ) + + +@pytest.mark.parametrize( + "header", + [ + 'attachment; filename="."', + 'attachment; filename=".."', + 'attachment; filename="reports/"', + 'attachment; filename="reports\\\\"', + ], +) +def test_safe_download_filename_rejects_names_sanitising_to_no_filename(header): + assert safe_download_filename(header) is None + + def test_deserialize_file_uses_content_disposition_filename(api_client, tmp_path): path = deserialize_file( api_client, FakeResponse('attachment; filename="report.csv"') @@ -158,7 +212,7 @@ def test_copy_download_preserves_replacement_when_atomic_claim_fails( source.write_bytes(b"file contents") destination = tmp_path / "report.csv" - def replace_name_and_fail(source_path, destination_path, follow_symlinks): + def replace_name_and_fail(source_path, destination_path): Path(destination_path).write_bytes(b"created by racer") raise OSError("injected atomic-claim failure") diff --git a/xero_python/api_client/__init__.py b/xero_python/api_client/__init__.py index f94da764..693c3e0c 100644 --- a/xero_python/api_client/__init__.py +++ b/xero_python/api_client/__init__.py @@ -76,7 +76,7 @@ def safe_download_path(directory, filename): def copy_download_without_overwrite(source, destination): try: - os.link(source, destination, follow_symlinks=False) + os.link(source, destination) except OSError: return False