Skip to content

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

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

Jump to bottom
Merged
Kludex merged 4 commits into from
Dec 11, 2024
Merged

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

Kludex merged 4 commits into from
Dec 11, 2024

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