Skip to content

fix: pull files with non-ASCII characters in the path #2871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tonyandrewmeyer wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: pull files with non-ASCII characters in the path #2871

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
798f455
fix: pull files with non-ASCII characters in the path
tonyandrewmeyer Nov 9, 2026
f5c4623
test: check that pull keeps file bytes that are not valid UTF-8
tonyandrewmeyer Nov 10, 2026
92951d2
docs: say that surrogateescape matches os.fsdecode for pulled paths
tonyandrewmeyer Nov 11, 2026
bfa39ac
fix: keep leading and trailing whitespace in pulled file paths
tonyandrewmeyer Nov 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ops/pebble.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3632,8 +3632,10 @@ def __init__(self, boundary: bytes | str):
self._parser.feed(b'\r\n')

def _process_header(self, data: bytearray):
parser = email.parser.BytesFeedParser()
parser.feed(data)
# The bytes parser replaces anything outside ASCII in a header value,
# which loses a non-ASCII path. Pebble sends the headers as UTF-8.
parser = email.parser.FeedParser()
parser.feed(data.decode('utf-8', 'surrogateescape'))

@benhoyt benhoyt Nov 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree with the intent here, but I just want to make sure that this will still allow arbitrary binary bytes in the file payload part of the HTTP response.

@tonyandrewmeyer tonyandrewmeyer Nov 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, only the part headers go through the decode: _MultipartParser splits each part at the first \r\n\r\n, hands the header block to _process_header, and passes the body bytes to _process_body untouched, which writes them straight to the tempfile. I've extended test_pull_binary with some bytes that aren't valid UTF-8 (\xff\xfe\x00\x80) to ensure it stays that way.

self._headers = parser.close()

content_disposition = self._headers.get_content_disposition()
Expand Down
23 changes: 23 additions & 0 deletions test/test_pebble.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2542,6 +2542,29 @@ def test_pull_path_error(self, client: MockClient):
),
]

def test_pull_non_ascii_path(self, client: MockClient):
client.responses.append((
{'Content-Type': 'multipart/form-data; boundary=01234567890123456789012345678901'},
b"""\
--01234567890123456789012345678901\r
Content-Disposition: form-data; name="files"; filename="/tmp/cr\xc3\xa8me.txt"\r
\r
contents\r
--01234567890123456789012345678901\r
Content-Disposition: form-data; name="response"\r
\r
{
"result": [{"path": "/tmp/cr\xc3\xa8me.txt"}],
"status": "OK",
"status-code": 200,
"type": "sync"
}\r
--01234567890123456789012345678901--\r
""",
))
with client.pull('/tmp/crème.txt') as infile: # ruff: ignore[hardcoded-temp-file]
assert infile.read() == 'contents'

def test_pull_protocol_errors(self, client: MockClient):
client.responses.append(({'Content-Type': 'c/t'}, b''))
with pytest.raises(pebble.ProtocolError) as excinfo:
Expand Down
Loading