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: When property value contains forwardslash #12

Closed
wants to merge 2 commits into from
Closed

FIX: When property value contains forwardslash #12

wants to merge 2 commits into from

Conversation

srolija
Copy link

@srolija srolija commented Apr 3, 2017

We encountered edge case when using FormData on iOS inside React Native would generate boundary value that would include forward slashes.

I checked RFC7231, section 3.1.1.1. (Media Type) and there is nothing specified regarding property value format. Since there is obviously case when such property is generated I believe that it should be supported (especially since there are other libraries that depend on this module used for server side parsing).

I added test covering case we encountered. I didn't touch versioning but if you want I can create minor version bump.

We encountered edge case when using FormData on iOS inside React Native
would generate boundry value that would include forward slashes.

I checked RFC7231, section 3.1.1.1. (Media Type) and there is nothing
specified regarding property value format. Since there is obviously case
when such property is generated I believe that it should be supported
(especially since there are other libraries that depend on this module
used for server side parsing).

I added test covering case we encountered.
@dougwilson dougwilson self-assigned this Apr 3, 2017
@dougwilson dougwilson added the pr label Apr 3, 2017
@dougwilson
Copy link
Contributor

Hi @srolija , the / character is not a valid character that can exist in a parameter value outside of being inside quotes.

You can read more about the syntax for a Content-Type header in the specification: https://tools.ietf.org/html/rfc7231#section-3.1.1.1

Here is how you read the ABNF specs in the RFC:

The following is the ABF for the contents of the Content-Type header:

     media-type = type "/" subtype *( OWS ";" OWS parameter )
     type       = token
     subtype    = token

So you are talking about what is in the parameter token, which you can follow right in the next section:

     parameter      = token "=" ( token / quoted-string )

You can see that a parameter's value is specified to be either a token or a quoted-string. So you're saying that / should be allowed in an unquoted value, so let's see what token is defined as. You can find it in Appendix D. Collected ABNF:

   token = <token, see [RFC7230], Section 3.2.6>

So this says that the token ABF is defined in RFC 7230, section 3.2.6. Here it is:

     token          = 1*tchar

So, one or more tchar:

     tchar          = "!" / "#" / "$" / "%" / "&" / "'" / "*"
                    / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
                    / DIGIT / ALPHA
                    ; any VCHAR, except delimiters

And as you can see, / is not a valid tchar.

@dougwilson
Copy link
Contributor

P.S. I see you mentioned Reactive Native on iOS. I remember a previous issue about this somewhere, and it was deemed a bug in that library, and I feel like they even fixed it. Let me see if I can dig that up.

@dougwilson
Copy link
Contributor

So this is the commit I'm thinking of facebook/react-native@8ec7743 . If I'm reading their history right, looks like it first made it into Reactive Native v0.41.0

@srolija
Copy link
Author

srolija commented Apr 3, 2017

Thank you very much!
It was the root cause of the problem!

I'm sorry if I am going off topic now, but I just have question regarding https://github.com/jshttp/type-is , I saw that you were also maintaining it. Reason why I was creating pull request here is because I saw it depended on media-typer which in latest version removed parameter handling rendering multipart queries like this invalid (regardless of forwardslash). So I suposed its implementation would sooner or later replace it with this library. Would you like me to create pull request for required changes?

@dougwilson
Copy link
Contributor

Would you like me to create pull request for required changes?

There is still churn around those modules, so it's hard to say what exactly the change will actually be in type-is at this time.

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