[CO-370] Make Content-Type parsing more lenient #3596

KtorZ · 2018-09-14T05:04:08Z

Description

We have had a lot of trouble where people will send no content-type or
simply 'application/json' and request would fail with 415 Unsupported
Media Type. Without knowing where to look, the error looks quite
unexpected and could give a lot of headache. This commit does two
changes:

It slightly modifies the 'Accept' instance to use 'contentTypes'
instead of 'contentType'. The latter is defined from the former by
simply taking the head of the list returned by 'contentTypes', which in
our case is 'application/json;charset=utf-8'
It provides a default content-type 'application/json' when no
content-type is provided

TODO

Add entry in the CHANGELOG once changing base against develop

Linked issue

[CO-370]

Type of change

🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
🔨 New or improved tests for existing code
⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

With cURL:

Content-Type is "application/json;charset=utf-8"

POST /api/v1/wallets HTTP/1.1
Host: localhost:8090
User-Agent: curl/7.47.0
Accept: */*
Content-Type: application/json;charset=utf-8
Content-Length: 209

< HTTP/1.1 201 Created
< Transfer-Encoding: chunked
< Date: Fri, 14 Sep 2018 04:53:31 GMT
< Server: Warp/3.2.22
< Content-Type: application/json;charset=utf-8

Content-Type is "application/json"

POST /api/v1/wallets HTTP/1.1
Host: localhost:8090
User-Agent: curl/7.47.0
Accept: */*
Content-Type: application/json
Content-Length: 209

< HTTP/1.1 201 Created
< Transfer-Encoding: chunked
< Date: Fri, 14 Sep 2018 04:51:55 GMT
< Server: Warp/3.2.22
< Content-Type: application/json;charset=utf-8

Content-Type is not defined

POST /api/v1/wallets HTTP/1.1
Host: localhost:8090
User-Agent: curl/7.47.0
Accept: /
Content-Length: 209

< HTTP/1.1 201 Created
< Transfer-Encoding: chunked
< Date: Fri, 14 Sep 2018 04:56:23 GMT
< Server: Warp/3.2.22
< Content-Type: application/json;charset=utf-8

Content-Type is "application/x-www-form-urlencoded"

POST /api/v1/wallets HTTP/1.1
Host: localhost:8090
User-Agent: curl/7.47.0
Accept: */*
Content-Length: 209
Content-Type: application/x-www-form-urlencoded

< HTTP/1.1 415 Unsupported Media Type
< Transfer-Encoding: chunked
< Date: Fri, 14 Sep 2018 04:56:08 GMT
< Server: Warp/3.2.22

Screenshots (if available)

KtorZ · 2018-09-14T05:05:07Z

@akegalj This is currently targetting the throttling branch as I added the middleware on top of your refactor.
Once it got merged into develop, I'll change the base branch of this PR to develop as well, but for now, it makes it easier for the review 👍

KtorZ · 2018-09-14T05:06:17Z

wallet-new/src/Cardano/Wallet/API/Response.hs

@@ -226,7 +226,7 @@ instance FromJSON a => MimeUnrender ValidJSON a where
        Right v  -> return v

 instance Accept ValidJSON where
-    contentType _ = contentType (Proxy @ JSON)
+    contentTypes _ = contentTypes (Proxy @ JSON)


This simple change already makes the parser accept both application/json and application/json;charset=utf-8.

Actually, contentType is defined using the head of what's returned by contentTypes, which in the case of JSON is application/json;charset=utf-8 !

@KtorZ Interesting, good catch! I am wondering if this change, alone, suffice to make the content type lenient or if we do need the extra middleware machinery at all? 🤔

This alone covers the requirements 1, 2 and 4 from CO-369

The middleware machinery covers the requirement 3.

akegalj

LGTM

akegalj · 2018-09-14T05:44:18Z

wallet-new/server/Main.hs

@@ -109,6 +111,7 @@ actionWithLegacyWallet coreConfig walletConfig txpConfig sscParams nodeParams nt
        mconcat [ LegacyPlugins.conversation wArgs
                , LegacyPlugins.legacyWalletBackend coreConfig txpConfig wArgs ntpStatus
                    [ throttleMiddleware (ccThrottle walletConfig)
+                    , withDefaultHeader (Headers.applicationJson)


you really like these parens :D

Hahaha, yeah, that's called refactoring 😂 And now I don't have hlint to warn me about those since we disabled the extra-parens rule 😶

Add request throttling feature for new wallet. It is possible now to extend list of Middleware's with other middlewares such as gzip Middleware or others defined in wai-extra. Legacy middlewares are also cleaned up and untangled in the same way and it is possible to list all Middlewares top level in a similar way as we list all Plugins and other workers.

We have had a lot of trouble where people will send no content-type or simply 'application/json' and request would fail with 415 Unsupported Media Type. Without knowing where to look, the error looks quite unexpected and could give a lot of headache. This commit does two changes: - It slightly modifies the 'Accept' instance to use 'contentTypes' instead of 'contentType'. The latter is defined from the former by simply taking the head of the list returned by 'contentTypes', which in our case is 'application/json;charset=utf-8' - It provides a default content-type 'application/json' when no content-type is provided

KtorZ requested a review from akegalj September 14, 2018 05:04

KtorZ requested a review from adinapoli-iohk September 14, 2018 05:05

KtorZ commented Sep 14, 2018

View reviewed changes

akegalj approved these changes Sep 14, 2018

View reviewed changes

adinapoli-iohk approved these changes Sep 14, 2018

View reviewed changes

KtorZ force-pushed the Squad1/CBR-179/bouncing-and-throttling branch 2 times, most recently from dcce5c1 to 992ee9c Compare September 14, 2018 15:59

KtorZ force-pushed the KtorZ/CO-370/lenient-content-type-parser branch from 430a0ed to 74cc084 Compare September 15, 2018 03:22

KtorZ changed the base branch from Squad1/CBR-179/bouncing-and-throttling to develop September 15, 2018 03:23

KtorZ force-pushed the KtorZ/CO-370/lenient-content-type-parser branch 3 times, most recently from 4de9471 to bdf68fc Compare September 15, 2018 04:55

akegalj and others added 5 commits September 15, 2018 15:43

[CBR-179] Move middlewares to dedicated module

05a16a3

[CBR-179] Remove wai-cors dependency

52b7e06

[CO-370] Update CHANGELOG entry

49b06f0

KtorZ force-pushed the KtorZ/CO-370/lenient-content-type-parser branch from bdf68fc to 49b06f0 Compare September 15, 2018 13:52

KtorZ merged commit 8fdc1a0 into develop Sep 15, 2018

KtorZ deleted the KtorZ/CO-370/lenient-content-type-parser branch September 15, 2018 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CO-370] Make Content-Type parsing more lenient #3596

[CO-370] Make Content-Type parsing more lenient #3596

KtorZ commented Sep 14, 2018

KtorZ commented Sep 14, 2018

KtorZ Sep 14, 2018

adinapoli-iohk Sep 14, 2018

KtorZ Sep 14, 2018 •

edited

Loading

akegalj left a comment

akegalj Sep 14, 2018

KtorZ Sep 14, 2018

[CO-370] Make Content-Type parsing more lenient #3596

[CO-370] Make Content-Type parsing more lenient #3596

Conversation

KtorZ commented Sep 14, 2018

Description

TODO

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Content-Type is "application/json;charset=utf-8"

Content-Type is "application/json"

Content-Type is not defined

Content-Type is "application/x-www-form-urlencoded"

Screenshots (if available)

KtorZ commented Sep 14, 2018

KtorZ Sep 14, 2018

Choose a reason for hiding this comment

adinapoli-iohk Sep 14, 2018

Choose a reason for hiding this comment

KtorZ Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

akegalj left a comment

Choose a reason for hiding this comment

akegalj Sep 14, 2018

Choose a reason for hiding this comment

KtorZ Sep 14, 2018

Choose a reason for hiding this comment

KtorZ Sep 14, 2018 •

edited

Loading