Skip to content

Commit

Permalink
Only set the body as binary when the Content-Type Header is a binary …
Browse files Browse the repository at this point in the history 
…type.
  • Loading branch information
@tateexon
tateexon committed Jun 25, 2024
1 parent b7bafe8 commit ac2569a
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 3 deletions.
29 changes: 26 additions & 3 deletions internal/server/http/dump.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,25 @@ func writeLog(writer io.Writer, params handlers.LogFormatterParams, body string)
writer.Write(buf)
}

// isBinaryContent checks to see if the body is a common binary content type
func isBinaryContent(r *http.Request) bool {
contentType := r.Header.Get("Content-Type")
binaryContentTypes := []string{

This comment has been minimized.

Sign in to view

Copy link
@asad-urrahman

asad-urrahman Jun 25, 2024

may be global?

This comment has been minimized.

Sign in to view

Copy link
@tateexon

tateexon Jun 26, 2024

Author

Changed to global: 8c2e34c

This comment has been minimized.

Sign in to view

Copy link
@joanlopez

joanlopez Jul 1, 2024 •
edited
Loading

Member

Could you bring more context about this specific use case and which are the implications, please?

This comment has been minimized.

Sign in to view

Copy link
@tateexon

tateexon Jul 1, 2024

Author

If the request body contains binary data it can cause problems when marshalled into json. So we try to detect binary data based on the headers to prevent issues in the output. I am assuming dumping out raw binary in the log would be real ugly as well. That is what I understood at least.

This comment has been minimized.

Sign in to view

Copy link
@joanlopez

joanlopez Jul 1, 2024

Member

Yeah, right! Some other, non-excluding, ideas could be:

  • Default to UTF-8/Unicode (something representable as a string)
    • Otherwise, encode it to Base64 (for decent text visualization)
  • Set a maximum length (to be dumped)

This comment has been minimized.

Sign in to view

Copy link
@asad-urrahman

asad-urrahman Jul 1, 2024

Addition to @tateexon ; Beyond aesthetic considerations, it also poses safety risks due to potential control sequences.

Solely depending on UTF-8 or character encoding is insufficient, as headers often specify only the MIME type. However, prioritizing a check for UTF-8 when present allows for optimization (direct logging), while in its absence, individual MIME types should be checked.

One important caveat is that browsers will use the last Content-Type header if multiple are included. For instance:

HTTP/1.1 200 OK
// ...
Content-Type: text/plain
Content-Type: text/html
...

In this example, the browser would interpret the response as text/html.

This comment has been minimized.

Sign in to view

Copy link
@tateexon

tateexon Jul 16, 2024

Author

Is there something we want to be changed here? I am not clear from the comments above.

This comment has been minimized.

Sign in to view

Copy link
@tateexon

tateexon Jul 17, 2024

Author

It looks like go's http uses the first Content type header by default so I can update that to grab the last if that is what we want here. It also shouldn't be too hard to add a maximum length to dump setting if that is wanted. Just want to be clear on what is wanted before I jump back in.
"application/octet-stream",
"image/",
"audio/",
"video/",
"application/pdf",
}

for _, binaryType := range binaryContentTypes {
if strings.HasPrefix(contentType, binaryType) {
return true
}
}
return false
}

// CustomLoggingHandler provides a way to supply a custom log formatter
// while taking advantage of the mechanisms in this package
func CustomLoggingHandler(out io.Writer, h http.Handler, s *Server) http.Handler {
Expand All @@ -176,18 +195,22 @@ func CustomLoggingHandler(out io.Writer, h http.Handler, s *Server) http.Handler
return
}
params.Request.Body = io.NopCloser(bytes.NewReader(bodyBytes)) // Reset the body
base64Body := base64.StdEncoding.EncodeToString(bodyBytes)
body := string(bodyBytes)
// if content is binary, encode it to base64
if isBinaryContent(params.Request) {
body = base64.StdEncoding.EncodeToString(bodyBytes)
}
verbose := s.verbose

// record request, if error set verbose to true to log current request since
// it didn't make it into a full channel
err = recordRequest(params.Request, s, base64Body)
err = recordRequest(params.Request, s, body)
if err != nil {
verbose = true
}

if verbose {
writeLog(writer, params, base64Body)
writeLog(writer, params, body)
}
})
}
Expand Down
22 changes: 22 additions & 0 deletions internal/server/http/server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,34 +219,55 @@ func TestBuildLogRequests(t *testing.T) {
testCases := map[string]struct {
method string
path string
contentType string
body string
expectedLog string
expectedStatus int
}{
"GET valid imposter request": {
method: "GET",
path: "/yamlTestDumpRequest",
contentType: "text/plain",
body: "Dumped",
expectedLog: "GET /yamlTestDumpRequest HTTP/1.1\" 200 17 Dumped\n",
expectedStatus: http.StatusOK,
},
"GET valid imposter binary request": {
method: "GET",
path: "/yamlTestDumpRequest",
contentType: "application/octet-stream",
body: "Dumped",
expectedLog: "GET /yamlTestDumpRequest HTTP/1.1\" 200 17 RHVtcGVk\n",
expectedStatus: http.StatusOK,
},
"GET valid imposter request no body": {
method: "GET",
path: "/yamlTestDumpRequest",
contentType: "text/plain",
body: "",
expectedLog: "GET /yamlTestDumpRequest HTTP/1.1\" 200 17\n",
expectedStatus: http.StatusOK,
},
"GET invalid imposter request": {
method: "GET",
path: "/doesnotexist",
contentType: "text/plain",
body: "Dumped",
expectedLog: "GET /doesnotexist HTTP/1.1\" 404 19 Dumped\n",
expectedStatus: http.StatusNotFound,
},
"GET invalid imposter binary request": {
method: "GET",
path: "/doesnotexist",
contentType: "video/mp4",
body: "Dumped",
expectedLog: "GET /doesnotexist HTTP/1.1\" 404 19 RHVtcGVk\n",
expectedStatus: http.StatusNotFound,
},
"GET invalid imposter request no body": {
method: "GET",
path: "/doesnotexist",
contentType: "text/plain",
body: "",
expectedLog: "GET /doesnotexist HTTP/1.1\" 404 19\n",
expectedStatus: http.StatusNotFound,
Expand All @@ -270,6 +291,7 @@ func TestBuildLogRequests(t *testing.T) {

w := httptest.NewRecorder()
req := httptest.NewRequest(tc.method, tc.path, strings.NewReader(tc.body))
req.Header.Set("Content-Type", tc.contentType)

server.httpServer.Handler.ServeHTTP(w, req)

Expand Down

0 comments on commit ac2569a

Please sign in to comment.