Skip to content

Handle messages containing only end boundary, fixes #38#142

Merged
Kludex merged 4 commits intoKludex:masterfrom
jhnstrk:empty_message
Dec 11, 2024
Merged

Handle messages containing only end boundary, fixes #38#142
Kludex merged 4 commits intoKludex:masterfrom
jhnstrk:empty_message

Conversation

@jhnstrk
Copy link
Contributor

@jhnstrk jhnstrk commented Apr 23, 2024 •
edited
Loading

Some browsers create empty messages when forms are posted - see #38 for examples.

The fix here adds a new state in the parser which is entered when the first hyphen after the first boundary is encountered. This is only the case for the very first boundary. Anything but a second hyphen after the first is considered an error.

@lovelydinosaur
Copy link
Collaborator

lovelydinosaur commented Apr 24, 2024 •
edited
Loading

Sounds feasible, let's get some clarity on it...

  • Is that spec compliant?
  • ~~Can you provide an example (which browser and how to test)?~

Edit:

Apologies for the noise, I've not confirmed this but can see the following linked...

Firefox and Chrome produce empty messages like this, e.g. when POSTing a HTML form with only a unchecked checkbox.

(And... probably not relevant if it's spec compliant or not if that's the actual behavior.)

@jhnstrk
Copy link
Contributor Author

jhnstrk commented Apr 24, 2024

To me the spec isn't 100% clear. rfc2046 appears to assume that there is always at least one body part, so this would not be valid. But it doesn't make it clear how a form with no parts should be communicated either.

Anyway, it's what Chrome (and probably Firefox, but I haven't tested myself) does now. Take the following text, adapted from the examples in #38

  • save as a test.html
  • open the file in Chrome
  • open Chrome dev tools
  • click the button on the form
  • inspect the payload sent to example.com (Network tab,--> Payload --> View Source)
<!DOCTYPE html>
<html>
  <head><meta charset="UTF-8"></head>
  <body>
        <form action="http://example.com/" enctype="multipart/form-data" method="post">
            <input type="checkbox" id="check" name="check">
            <label for="check">Breaks unless you check it</label>
			<input type="submit" value="Submit">
        </form>  
  </body>
</html>

I got the following payload: (Chrome Version 124.0.6367.61 (Official Build) (64-bit))

------WebKitFormBoundaryBBDliyOmePSMlFiJ--

Clicking (enabling) the check box in the form gives valid content with 1 part.

@Abdelhadi92
Copy link

Abdelhadi92 commented Sep 10, 2024

any updates on this?

@jhnstrk
Copy link
Contributor Author

jhnstrk commented Oct 28, 2024

Rebased on current master.

@Kludex
Copy link
Owner

Kludex commented Dec 6, 2024

@jhnstrk I think my previous PR to fix the security advisory put this PR in a non-perfect state:

If I run what you suggest:

import python_multipart

boundary = b"-"
parser = python_multipart.MultipartParser(boundary)
parser.write(b"------WebKitFormBoundaryBBDliyOmePSMlFiJ--\r\n")

I get Skipping data after last boundary, because - is computed on the END step.

Do you have suggestions on how we should fix it?

@jhnstrk
Copy link
Contributor Author

jhnstrk commented Dec 8, 2024

@jhnstrk I think my previous PR to fix the security advisory put this PR in a non-perfect state:

If I run what you suggest:

I get Skipping data after last boundary, because - is computed on the END step.

Do you have suggestions on how we should fix it?

This is expected though... setting the boundary to "-" makes the initial part of the message (-----) appear valid: you have an initial two hyphens, boundary, then trailing hyphens.. Everything after looks like a bad trailing boundary (by the spec only whitespace followed by CRLF is allowed). We could validate this more strictly, but it's a little out of scope here.

To test the empty message case you should use

boundary = b"----WebKitFormBoundaryBBDliyOmePSMlFiJ"

@Kludex Kludex enabled auto-merge (squash) December 11, 2024 16:41
@Kludex Kludex disabled auto-merge December 11, 2024 16:41
@Kludex Kludex enabled auto-merge (squash) December 11, 2024 16:41
@Kludex Kludex merged commit 04d3cf5 into Kludex:master Dec 11, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kludex Kludex Kludex approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jhnstrk @lovelydinosaur @Abdelhadi92 @Kludex