xmidt-org / wrp-go

go implementation of the Web Routing Protocol
Apache License 2.0
4 stars 7 forks source link

Bug: 500 `Invalid WRP content type: */*` for an invalid accept header #74

Closed denopink closed 2 years ago

denopink commented 2 years ago

Overview

related to xmidt-org/talaria#227 where a 500 Invalid WRP content type: */* is produced when an invalid accept header is provided. Instead of a 500, 400 is now returned.

tl;rd

This patches the bug where replacing the header Accept: application/json with anything except application/json like Accept: /, returns a 500 Invalid WRP content type: /.

Explanation It starts off with an error from `DetermineFormat` not being handled at https://github.com/xmidt-org/wrp-go/blob/4b275411c7aa52e1a537cc130f3b0f37e526c978/wrphttp/requestResponse.go#L140-L143 Then `NewEntityResponseWriter`s `DetermineFormat error` gets handled as a `500` at https://github.com/xmidt-org/wrp-go/blob/4b275411c7aa52e1a537cc130f3b0f37e526c978/wrphttp/handler.go#L134-L138
Type of Change(s) - Bug fix (non-breaking change which fixes an issue) - All new and existing tests passed.
Module Unit Testing: [PASSING]
PR Affecting Unit Testing: handler_test.go [PASSING] ```console Running tool: /usr/local/bin/go test -timeout 30s -run ^(TestHandlerFunc|TestWithErrorEncoder|TestWithNewResponseWriter|TestWithDecoder|TestWithBefore|TestNewHTTPHandler|TestWRPHandler)$ github.com/xmidt-org/wrp-go/v3/wrphttp === RUN TestHandlerFunc --- PASS: TestHandlerFunc (0.00s) === RUN TestWithErrorEncoder === RUN TestWithErrorEncoder/Default === RUN TestWithErrorEncoder/Custom --- PASS: TestWithErrorEncoder (0.00s) --- PASS: TestWithErrorEncoder/Default (0.00s) --- PASS: TestWithErrorEncoder/Custom (0.00s) === RUN TestWithNewResponseWriter === RUN TestWithNewResponseWriter/Default === RUN TestWithNewResponseWriter/Custom --- PASS: TestWithNewResponseWriter (0.00s) --- PASS: TestWithNewResponseWriter/Default (0.00s) --- PASS: TestWithNewResponseWriter/Custom (0.00s) === RUN TestWithDecoder === RUN TestWithDecoder/Default === RUN TestWithDecoder/Custom --- PASS: TestWithDecoder (0.00s) --- PASS: TestWithDecoder/Default (0.00s) --- PASS: TestWithDecoder/Custom (0.00s) === RUN TestWithBefore === RUN TestWithBefore/0 === RUN TestWithBefore/1 === RUN TestWithBefore/2 === RUN TestWithBefore/3 --- PASS: TestWithBefore (0.00s) --- PASS: TestWithBefore/0 (0.00s) --- PASS: TestWithBefore/1 (0.00s) --- PASS: TestWithBefore/2 (0.00s) --- PASS: TestWithBefore/3 (0.00s) === RUN TestNewHTTPHandler --- PASS: TestNewHTTPHandler (0.00s) === RUN TestWRPHandler === RUN TestWRPHandler/ServeHTTP === RUN TestWRPHandler/ServeHTTP/DecodeError === RUN TestWRPHandler/ServeHTTP/ResponseWriterError === RUN TestWRPHandler/ServeHTTP/Success /Users/odc/Documents/GitHub/xmidt-org/wrp-go/wrphttp/handler_test.go:352: PASS: ServeWRP(mock.argumentMatcher,mock.argumentMatcher) --- PASS: TestWRPHandler (0.00s) --- PASS: TestWRPHandler/ServeHTTP (0.00s) --- PASS: TestWRPHandler/ServeHTTP/DecodeError (0.00s) --- PASS: TestWRPHandler/ServeHTTP/ResponseWriterError (0.00s) --- PASS: TestWRPHandler/ServeHTTP/Success (0.00s) PASS ok github.com/xmidt-org/wrp-go/v3/wrphttp 0.134s > Test run finished at 5/2/2022, 3:47:48 PM < ```

Local Test

Ran talaria locally

curl -v --location --request POST 'localhost:6200/api/v3/device/send' \
--header 'Content-Type: application/json' \
--header 'Authorization: Basic dXNlcjpwYXNz' \
--data-raw '{
"msg_type":3,
"content_type":"application/json",
"source":"dns:me",
"dest":"mac:112233445566",
"transaction_uuid":"1234567890",
"payload":"eyJjb21tYW5kIjoiR0VUIiwibmFtZXMiOlsiU29tZXRoaW5nIl19",
"partner_ids":["comcast"]
}'

# Output:

Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying 127.0.0.1:6200...
* Connected to localhost (127.0.0.1) port 6200 (#0)
> POST /api/v3/device/send HTTP/1.1
> Host: localhost:6200
> User-Agent: curl/7.79.1
> Accept: */*
> Content-Type: application/json
> Authorization: Basic dXNlcjpwYXNz
> Content-Length: 223
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 400 Bad Request
< Content-Length: 29
< Content-Type: text/plain; charset=utf-8
< X-Talaria-Build: 0.1.4
< X-Talaria-Flavor: mint
< X-Talaria-Region: east
< X-Talaria-Server: talaria
< X-Talaria-Start-Time: 26 Apr 22 15:51 UTC
< Date: Tue, 26 Apr 2022 16:03:41 GMT
<
* Connection #0 to host localhost left intact
Invalid WRP content type: */*%
#
denopink commented 2 years ago

This looks great! Can you add a test case in decoders_test.go for this scenario?

Will do and rebase with #73

codecov[bot] commented 2 years ago

Codecov Report

Merging #74 (9b0fca0) into main (7f41972) will increase coverage by 0.04%. The diff coverage is 100.00%.

:exclamation: Current head 9b0fca0 differs from pull request most recent head f5a6e28. Consider uploading reports for the commit f5a6e28 to get more accurate results

@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
+ Coverage   49.03%   49.08%   +0.04%     
==========================================
  Files          18       18              
  Lines        3214     3217       +3     
==========================================
+ Hits         1576     1579       +3     
  Misses       1468     1468              
  Partials      170      170              
Impacted Files Coverage Δ
format.go 70.90% <100.00%> (ø)
wrphttp/decoders.go 75.00% <100.00%> (+2.27%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7f41972...f5a6e28. Read the comment docs.