Repository: pallets/werkzeug (python)

Issue:
Extra Newline Added to Binary File Upload
## Problem:
After uploading a specific binary file, an unexpected newline character (\n) is being added to the beginning of the file. This behavior is traced back to Werkzeug's multipart request body parsing during POST requests.

## Steps to Reproduce:

1. Create a binary file named one_newline_bigger_than_64k with the following content:
```py
with open("one_newline_bigger_than_64k", "wb") as file:
    bytes_to_write = b'X' + b'\n' + 64 * 1024 * b'c'
    file.write(bytes_to_write)
```
2. Upload the one_newline_bigger_than_64k file.
3. Compare the binary content of the original and uploaded files.
## Root Cause:
This issue arises due to Werkzeug's [_parse_data](https://github.com/pallets/werkzeug/blob/main/src/werkzeug/sansio/multipart.py#L246) method. Specifically, when a newline (\n or \r) is present at the second position (index 1) of the file's content and no additional newlines are within the first 64KB of the request body, an extra newline is inadvertently introduced during parsing.

## Code Flow:

1. Werkzeug's next_event method invokes _parse_data.
2. In _parse_data, a data_start value is set based on the file's content type.
3. If the boundary indicator is absent in the buffer, the [last_newline](https://github.com/pallets/werkzeug/blob/main/src/werkzeug/sansio/multipart.py#L124) method is employed to locate the latest newline (\n or \r) character in the file's content. However, this method operates on a modified data slice that starts at data_start.
4. Since data is modified, any index from last_newline needs data_start adjustment.
5. Incorrectly adjusted indices lead to an empty byte segment when slicing data[data_start:data_end], resulting in an empty data segment.
6. This leads to an unintended addition of a newline character in the file's content.
## Solution:
To resolve this, a correction in the _parse_data method is needed. The index returned by last_newline should be adjusted by adding the data_start before slicing data.

### Proposed Fix:
Update _parse_data as follows:

```py
# Inside _parse_data method
if self.buffer.find(b"--" + self.boundary) == -1:
    # No complete boundary in buffer, but a partial boundary might exist. 
    # Find the earliest newline (nl or cr) and return data up to that.
    last_newline_idx = self.last_newline(data[data_start:])
    data_end = del_index = last_newline_idx + data_start  # Adjusted index
    more_data = True
```

## Environment:

Python: 3.10.12
Werkzeug: 2.3.6

## Resolution:
A pull request will be created to implement this fix, preventing unintended newline addition in binary file uploads. This adjustment ensures proper index handling, preventing unintended manipulation of file content during parsing.

Pull request description:
Fix multipart parsing when additional newlines are present
Replaces #2762 and fixes #2761.

Checklist:

- [x] Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
- [x] Add or update relevant docs, in the docs folder and in code.
- [x] Add an entry in `CHANGES.rst` summarizing the change and linking to the issue.
- [x] Add `.. versionchanged::` entries in any relevant code docs.
- [x] Run `pre-commit` hooks and fix any issues.
- [x] Run `pytest` and `tox`, no tests failed.


Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index f3784c23..28a2b192 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -6,6 +6,8 @@ Version 2.3.7
 Unreleased
 
 -   Use ``flit_core`` instead of ``setuptools`` as build backend.
+-   Fix parsing of multipart bodies.
+    Adjust index of last newline in data start. :issue:`2761`
 
 
 Version 2.3.6
diff --git a/src/werkzeug/sansio/multipart.py b/src/werkzeug/sansio/multipart.py
index 11e65ed0..380993af 100644
--- a/src/werkzeug/sansio/multipart.py
+++ b/src/werkzeug/sansio/multipart.py
@@ -256,7 +256,7 @@ class MultipartDecoder:
             # a partial boundary at the end. As the boundary
             # starts with either a nl or cr find the earliest and
             # return up to that as data.
-            data_end = del_index = self.last_newline(data[data_start:])
+            data_end = del_index = self.last_newline(data[data_start:]) + data_start
             more_data = True
         else:
             match = self.boundary_re.search(data)
@@ -268,7 +268,7 @@ class MultipartDecoder:
                 data_end = match.start()
                 del_index = match.end()
             else:
-                data_end = del_index = self.last_newline(data[data_start:])
+                data_end = del_index = self.last_newline(data[data_start:]) + data_start
             more_data = match is None
 
         return bytes(data[data_start:data_end]), del_index, more_data


Test patch:
diff --git a/tests/sansio/test_multipart.py b/tests/sansio/test_multipart.py
index 1d836d64..35109d4b 100644
--- a/tests/sansio/test_multipart.py
+++ b/tests/sansio/test_multipart.py
@@ -1,3 +1,5 @@
+import pytest
+
 from werkzeug.datastructures import Headers
 from werkzeug.sansio.multipart import Data
 from werkzeug.sansio.multipart import Epilogue
@@ -56,6 +58,57 @@ asdasd
     assert data == result
 
 
+@pytest.mark.parametrize(
+    "data_start",
+    [
+        b"A",
+        b"\n",
+        b"\r",
+        b"\r\n",
+        b"\n\r",
+        b"A\n",
+        b"A\r",
+        b"A\r\n",
+        b"A\n\r",
+    ],
+)
+@pytest.mark.parametrize("data_end", [b"", b"\r\n--foo"])
+def test_decoder_data_start_with_different_newline_positions(
+    data_start: bytes, data_end: bytes
+) -> None:
+    boundary = b"foo"
+    data = (
+        b"\r\n--foo\r\n"
+        b'Content-Disposition: form-data; name="test"; filename="testfile"\r\n'
+        b"Content-Type: application/octet-stream\r\n\r\n"
+        b"" + data_start + b"\r\nBCDE" + data_end
+    )
+    decoder = MultipartDecoder(boundary)
+    decoder.receive_data(data)
+    events = [decoder.next_event()]
+    # We want to check up to data start event
+    while not isinstance(events[-1], Data):
+        events.append(decoder.next_event())
+    expected = data_start if data_end == b"" else data_start + b"\r\nBCDE"
+    assert events == [
+        Preamble(data=b""),
+        File(
+            name="test",
+            filename="testfile",
+            headers=Headers(
+                [
+                    (
+                        "Content-Disposition",
+                        'form-data; name="test"; filename="testfile"',
+                    ),
+                    ("Content-Type", "application/octet-stream"),
+                ]
+            ),
+        ),
+        Data(data=expected, more_data=True),
+    ]
+
+
 def test_chunked_boundaries() -> None:
     boundary = b"--boundary"
     decoder = MultipartDecoder(boundary)
diff --git a/tests/test_datastructures.py b/tests/test_datastructures.py
index 6d2f9fe5..5206aa6a 100644
--- a/tests/test_datastructures.py
+++ b/tests/test_datastructures.py
@@ -63,7 +63,7 @@ class _MutableMultiDictTests:
             d = create_instance()
             s = pickle.dumps(d, protocol)
             ud = pickle.loads(s)
-            assert type(ud) == type(d)
+            assert type(ud) == type(d)  # noqa: E721
             assert ud == d
             alternative = pickle.dumps(create_instance("werkzeug"), protocol)
             assert pickle.loads(alternative) == d
diff --git a/tests/test_routing.py b/tests/test_routing.py
index 15bcc0af..65d2a5f9 100644
--- a/tests/test_routing.py
+++ b/tests/test_routing.py
@@ -748,7 +748,7 @@ def test_uuid_converter():
     m = r.Map([r.Rule("/a/<uuid:a_uuid>", endpoint="a")])
     a = m.bind("example.org", "/")
     route, kwargs = a.match("/a/a8098c1a-f86e-11da-bd1a-00112444be1e")
-    assert type(kwargs["a_uuid"]) == uuid.UUID
+    assert type(kwargs["a_uuid"]) == uuid.UUID  # noqa: E721
 
 
 def test_converter_with_tuples():