Skip to content
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.

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

Fix browsable API not supporting multipart/form-data correctly #5176

Merged
merged 4 commits into from Jun 16, 2017

Conversation

levic
Copy link

@levic levic commented May 25, 2017

  • Autodetect missing boundary parameter for Content-Type header
  • textarea value normalises EOL chars to \n when multipart/form-data requires \r\n

Fixes #1579

- Autodetect missing boundary parameter for Content-Type header
- textarea value normalises EOL chars to \n when multipart/form-data requires \r\n
Copy link
Member

@tomchristie tomchristie left a comment

Couple of questions here - 1. could you make the reason for that particular regex more clear. 2. What's the intent of re.exec? 3. Could you include before/after screenshots in the PR description?

Looking good!

@levic
Copy link
Author

levic commented May 30, 2017

  • Have explained a bit more with comments what the code is doing
  • Have replaced re.exec(String) with String.match(Regex) (same output but may be a bit clearer)

@levic
Copy link
Author

levic commented May 30, 2017

Not sure what I can show in a screenshot -- the UI hasn't changed and what happens is often dependent on your application code.

Old behaviour with multipart/form-data

  • If you fill in Content & submit, DRF's API Browser will print the following exception:
HTTP 400 Bad Request
Allow: POST, OPTIONS
Content-Type: application/json
Vary: Accept

{
    "detail": "Multipart form parse error - Invalid boundary in multipart: None"
}
  • If you Content empty then the request will make it through to your view, but request.data will be empty. Output will depend on how your view handles this.

New behaviour with multipart/form-data

  • When JS can detect a boundary:

    • Your view will process the POST request
    • The API browser output will make a new GET request; output will be the response from this GET (the same behaviour as other media types)
  • When JS is unable to detect a boundary (including where content is empty):

    • Request is left unmodified by JS
    • For empty content your view will be called with empty request.data. Output will depend on how your view handles this.
    • For non-empty content, no boundary will be added to the header and django will raise the same exception as before (Multipart form parse error - Invalid boundary in multipart: None). Your view will not be called and the Browser API will show the multipart form parse exception

// We need to add a boundary parameter to the header
// We assume the first valid-looking boundary line in the body is correct
// regex is from RFC 2046 appendix A
var re = /^--([0-9A-Z'()+_,-./:=?]{1,70})[ \f\t\v\u00a0\u1680\u180e\u2000-\u200a\u2028\u2029\u202f\u205f\u3000\ufeff]*$/im;
Copy link
Member

@tomchristie tomchristie May 30, 2017

Choose a reason for hiding this comment

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

Can we reduce the whitespace part of this regex to \s* or something similar?

Copy link
Member

@tomchristie tomchristie May 30, 2017

Choose a reason for hiding this comment

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

The boundary grouping isn't quite correct against the spec, as it can include " ", it just can't do so at the end.

The only mandatory global parameter for the "multipart" media type is
the boundary parameter, which consists of 1 to 70 characters from a
set of characters known to be very robust through mail gateways, and
NOT ending with white space.

boundary := 0*69<bchars> bcharsnospace

Copy link
Author

@levic levic May 31, 2017

Choose a reason for hiding this comment

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

  • Now handles space in the boundary delimiter correctly (with a more obvious translation from the spec)
  • Simplified whitespace to \s*? (the non-greedy qualifier because we want the $ to match the current line only and \s includes \n)

Re-tested locally with and without whitespace in the boundary; django handles this correctly too.

@tomchristie
Copy link
Member

tomchristie commented May 30, 2017

Great work on this! A couple of comments worth addressing there.

@tomchristie tomchristie added this to the 3.6.4 Release milestone May 30, 2017
@levic levic force-pushed the browsable-api-multipart-form-data branch from 191e418 to 6b8d601 Compare May 31, 2017
@levic
Copy link
Author

levic commented Jun 15, 2017

(Comments should be addressed)

// We need to add a boundary parameter to the header
// We assume the first valid-looking boundary line in the body is correct
// regex is from RFC 2046 appendix A
var boundaryCharNoSpace = "[0-9A-Z'()+_,-./:=?";
Copy link
Member

@tomchristie tomchristie Jun 16, 2017

Choose a reason for hiding this comment

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

The leading [ here looks wrong.

@tomchristie tomchristie merged commit b069b0d into encode:master Jun 16, 2017
@tomchristie
Copy link
Member

tomchristie commented Jun 16, 2017

Lovely bit of enhancement - thanks! 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants