Treat posted data as multipart #285

mvrhov · 2018-05-30T08:16:07Z

Use Multipart form parse function as some client implementations use it to send form data.
e.g All of our native clients which use CURL

aeneasr · 2018-05-30T08:17:05Z

I don't think multipart is allowed per spec, or am I mistaken?

mvrhov · 2018-05-30T08:27:05Z

The rfc6749 doesn't forbid it it just gives all examples in application/x-www-form-urlencoded, because it's more terse and readable.

The 2^24 aka 16M limit is a bit high.
Are we afraid of the DDOS? then maybe we should change it to 2^10 aka 1M?

Hm tests. Why the hell are they failing now.

aeneasr · 2018-05-30T08:42:33Z

My guess is that tests are failing because the valid application/x-www-form-urlencoded content type has been replaced with multipart which causes the encoding of e.g. the client-id to fail. Multipart should be a fallback, not the default (if at all, I have to check with the specs if that content type makes sense).

aeneasr · 2018-05-30T08:57:02Z

By the way, just do go test ./... locally to check if tests pass locally!

mvrhov · 2018-05-30T08:58:06Z

I changed this now to ignore the specific error (NotMultiart, just like request.FormValue does) will do.

mvrhov · 2018-05-30T10:27:28Z

All tests except one now pass. And I have no Idea why the last one reports different error

aeneasr · 2018-05-30T10:44:06Z

access_request_handler.go

@@ -62,7 +62,7 @@ func (f *Fosite) NewAccessRequest(ctx context.Context, r *http.Request, session

 	if r.Method != "POST" {
 		return accessRequest, errors.WithStack(ErrInvalidRequest.WithDebug("HTTP method is not POST"))
-	} else if err := r.ParseForm(); err != nil {
+	} else if err := r.ParseMultipartForm(2 ^ 24); err != nil && err != http.ErrNotMultipart {


ParseMultipartForm does not parse anything except the content type multipart, so the proper encoding has been replaced with multipart as ParseForm has been replaced and if the form is not multipart, then the flow fails (so every oauth2 library out there will not be able to make any requests to this library any more :D ).

Parsing multipart should be a fallback, not the default!

Also, do we really need 16MB of data here? Smells like a possible DoS vulnerability!

I've just seen this review. I've changed this to be 2^20 aka 1M. The other thing being
I don't know which version of go you are running but in 1.10 ParseMultipartForm first calls ParseForm and then it continues to do a multipart parse

Also in case you missed it. If you use the FormValue or PostFormValue of the request both of them call ParseMultipartForm with the default value of 32M!

I've just seen this review. I've changed this to be 2^20 aka 1M.

Nice!

I don't know which version of go you are running but in 1.10 ParseMultipartForm first calls ParseForm and then it continues to do a multipart parse

I think the tests run Go 1.8. We should definitely make this bc compatible with older versions of Go!

Also in case you missed it. If you use the FormValue or PostFormValue of the request both of them call ParseMultipartForm with the default value of 32M!

I didn't know that. We are however only calling PostForm which requires a prior ParseForm (or it's nil). This led me to investigate though and it seems that ParseForm reads up to 10MB, so also quite a lot (not 32M but still). So 1MB is definitely fine here, and 16MB was probably too! Parsing explicitly only 1MB is even better though!

I've checked the source of request.go in 1.8 and it seems that the ParseMultipartForm code is the same and it also calls ParseForm

Ohh, I see, I didn't know that. I was looking at another line where multipart is ignored but apparently it's handled in the outer functions of that call. Thank you for clarifying that! Learned a ton about form parsing in this PR :)

aeneasr · 2018-05-30T10:49:17Z

Hm, I don't think we should allow this, see:

https://tools.ietf.org/html/rfc6750#section-2.2

That explicitly enforces application/x-www-form-urlencoded. Will close if you agree.

mvrhov · 2018-05-30T11:41:52Z

I don't agree as the last thing I want to do is maintain my own fork of the library. We have numerous copies of our software out and removing support for old versions is out of the question.
The other thing is that the implementations in other languages are a lot more forgiven about this and parse such requests without any issues.

aeneasr · 2018-05-30T11:43:56Z

Sorry, I linked to a spec that doesn't concern this PR, I'll check again to see if this may be allowed

aeneasr · 2018-05-30T11:52:21Z

I'm still looking for hints in the specs as the are very vague. However, the big ones (like Google OAuth 2.0) are not forgiving when it comes to this parameter, see the docs.

Usually, the issue you describe comes from using either outdated OAuth2 clients or self-written code that handles OAuth2. Both should (must) be changed in order to (a) have an OAuth2 compliant flow and (b) remove any potential security issues. Keep in mind that this is probably at the core of your authorization flow, so why rely on potentially insecure code?

Second, you won't have to maintain your own fork. It's very easy to either write a middleware that encodes from multipart and decodes to application/x-www-form-urlencoded or you can use an API gateway that does that (I think most support this type of operation). It will give you more flexibility (e.g. allow this only for certain clients, define the maximum memory size yourself, ...) while removing the need of having non-standard things in this library.

I know that we're strict and that's the point this library is making. We don't want to compromise integrity, reliability, or security for 1% use cases.

Having said that, I haven't explicitly decided if this should make it or not in the library. I'll see what other big OAuth2 Providers are doing and will try to find information on this in the specs.

aeneasr · 2018-05-31T18:37:51Z

I checked the code and the failing test case is because ParseForm returns an error if no the body is empty. Apparently, that's not the case with ParseMultiForm which isn't bothered by this. Not sending a body should IMO return ErrInvalidRequest, not an unauthorized error (which is caused by the request lacking a bearer token).

aeneasr · 2018-06-04T17:00:56Z

So, I don't think there's actually anything speaking against adding this feature and I was not able to find a resource that would say otherwise. There's however still a failing test :|

mvrhov · 2018-06-05T10:03:21Z

Yeah. But for that we would need to re-read the body.
Example for access_request_handler...

	if body, _ := ioutil.ReadAll(r.Body); len(body) == 0 {
		return accessRequest, errors.WithStack(ErrInvalidRequest.WithDebug("missing form body"))
	}

// or
	if body, _ :=ioutil.ReadAll(io.LimitReader(r.Body, 1)); len(body) == 0 {
		return accessRequest, errors.WithStack(ErrInvalidRequest.WithDebug("missing form body"))
	}

aeneasr · 2018-06-05T10:06:09Z

Ugh, that sucks, especially because we actually can't re-read the body afaik, as the body is treated as a stream, unless ParseForm does some magic there. Another option could be to check if PostForm has any data? As in if len(r.PostForm) == 0?

…it to send form data

mvrhov · 2018-06-05T11:00:25Z

I've added this check only into the revoke and introspect handlers.
If I also add it to others then a lot of tests fail. Because the ErrInvalidRequest is returned instead of for example ErrInvaildClient.

coveralls · 2018-06-05T11:00:54Z

Coverage decreased (-0.1%) to 84.844% when pulling e2a9122 on mvrhov:multipart into 93618d6 on ory:master.

aeneasr · 2018-06-07T09:05:38Z

Perfect, I'll take another look at this today and will merge if nothing else emerges. Thank you for your persistence and work! :)

…ory#285)

mvrhov force-pushed the multipart branch from d5cd52f to 8e58f41 Compare May 30, 2018 08:53

mvrhov force-pushed the multipart branch from 8e58f41 to dca70d6 Compare May 30, 2018 08:59

aeneasr reviewed May 30, 2018

View reviewed changes

mvrhov force-pushed the multipart branch from dca70d6 to 8beb591 Compare May 31, 2018 08:23

mvrhov force-pushed the multipart branch from 8beb591 to e78122c Compare June 1, 2018 09:21

mvrhov force-pushed the multipart branch from e78122c to 2ef5008 Compare June 5, 2018 10:37

Use Multipart form parse function as some client implementations use …

e2a9122

…it to send form data

mvrhov force-pushed the multipart branch from 2ef5008 to e2a9122 Compare June 5, 2018 10:54

aeneasr merged commit 2edf8f8 into ory:master Jun 7, 2018

mvrhov deleted the multipart branch June 7, 2018 11:28

budougumi0617 added a commit to budougumi0617/fosite that referenced this pull request May 10, 2019

Allows multipart content type as alternative to x-www-form-urlencoded (…

b661a03

…ory#285)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat posted data as multipart #285

Treat posted data as multipart #285

mvrhov commented May 30, 2018

aeneasr commented May 30, 2018

mvrhov commented May 30, 2018 •

edited

Loading

aeneasr commented May 30, 2018

aeneasr commented May 30, 2018

mvrhov commented May 30, 2018

mvrhov commented May 30, 2018

aeneasr May 30, 2018 •

edited

Loading

mvrhov May 31, 2018 •

edited

Loading

aeneasr May 31, 2018

mvrhov May 31, 2018

aeneasr May 31, 2018

aeneasr commented May 30, 2018

mvrhov commented May 30, 2018

aeneasr commented May 30, 2018 •

edited

Loading

aeneasr commented May 30, 2018

aeneasr commented May 31, 2018

aeneasr commented Jun 4, 2018

mvrhov commented Jun 5, 2018

aeneasr commented Jun 5, 2018

mvrhov commented Jun 5, 2018

coveralls commented Jun 5, 2018

aeneasr commented Jun 7, 2018

Treat posted data as multipart #285

Treat posted data as multipart #285

Conversation

mvrhov commented May 30, 2018

aeneasr commented May 30, 2018

mvrhov commented May 30, 2018 • edited Loading

aeneasr commented May 30, 2018

aeneasr commented May 30, 2018

mvrhov commented May 30, 2018

mvrhov commented May 30, 2018

aeneasr May 30, 2018 • edited Loading

Choose a reason for hiding this comment

mvrhov May 31, 2018 • edited Loading

Choose a reason for hiding this comment

aeneasr May 31, 2018

Choose a reason for hiding this comment

mvrhov May 31, 2018

Choose a reason for hiding this comment

aeneasr May 31, 2018

Choose a reason for hiding this comment

aeneasr commented May 30, 2018

mvrhov commented May 30, 2018

aeneasr commented May 30, 2018 • edited Loading

aeneasr commented May 30, 2018

aeneasr commented May 31, 2018

aeneasr commented Jun 4, 2018

mvrhov commented Jun 5, 2018

aeneasr commented Jun 5, 2018

mvrhov commented Jun 5, 2018

coveralls commented Jun 5, 2018

aeneasr commented Jun 7, 2018

mvrhov commented May 30, 2018 •

edited

Loading

aeneasr May 30, 2018 •

edited

Loading

mvrhov May 31, 2018 •

edited

Loading

aeneasr commented May 30, 2018 •

edited

Loading