Skip to content

[FIX] FromRequest() JSON content type validation#63

Merged
inhere merged 2 commits intogookit:masterfrom
quetzyg:master
Oct 26, 2020
Merged

[FIX] FromRequest() JSON content type validation#63
inhere merged 2 commits intogookit:masterfrom
quetzyg:master

Conversation

@quetzyg
Copy link
Copy Markdown
Contributor

@quetzyg quetzyg commented Oct 24, 2020

Problem

The logic to check if a request contains JSON is too restrictive. Since only the exact application/json string is considered valid, when the Content-Type header contains values such as application/vnd.api+json or application/json; charset=UTF-8, the request body is dismissed.

Another problem is that there are other JSON MIME types besides application/json.

If we do a quick search here, we can find 200+ MIME types related to JSON.

Solution

The mime type check logic has been improved by using a regular expression. Here's a brief explanation:

(?i)  --> case insensitive matches
application/ --> the only type we care about and the type/subtype separator
((\w|\.|-)+\+)? --> this group may exist with one or more (a-z, 0-9, _, . and -) characters and always with a + at the end
json --> the final part of the subtype
(-seq)? --> some JSON MIME types have this suffix

Caveats

The current regular expression isn't strict about the combination of allowed characters in a subtype, which means that non-existent/unofficial MIME types, like application/foobar+json or application/vnd.123+json are considered valid.

For the sake of simplicity, I think that's a good trade-off. It also means that when new JSON MIME types are added in the future, this logic should require low maintenance, if any.

Task items:

  • Improve JSON Content-Type check logic in FromRequest();
  • Update existing tests;

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 24, 2020

Pull Request Test Coverage Report for Build 325982699

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.874%

Totals Coverage Status
Change from base Build 297365813: 0.0%
Covered Lines: 2498
Relevant Lines: 2661

💛 - Coveralls

@inhere inhere self-assigned this Oct 25, 2020
@inhere inhere added the enhancement New feature or request label Oct 25, 2020
@inhere inhere merged commit 2566bfe into gookit:master Oct 26, 2020
@quetzyg
Copy link
Copy Markdown
Contributor Author

quetzyg commented Oct 26, 2020

Cheers @inhere 👍

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants