Add support for injecting a banner in HTML responses #35
Conversation
ojarjur
requested review from
bsidhom,
ZainRizvi,
JerryLeiDing,
malinajiang and
DanSedov
| @@ -70,10 +72,18 @@ func hostProxy(ctx context.Context, host, shimPath string, injectShimCode bool) | |||
| Host: host, | |||
There was a problem hiding this comment.
I realize this wasn't changed here, but is the backend necessarily http?
There was a problem hiding this comment.
That's a good point.
Currently, every user of this I know of is just connecting to a server on localhost, so they are always http, but there is no reason to require that.
File a tracking issue for that here
agent/banner/banner.go
Outdated
| return false | ||
| } | ||
| contentType := resp.Header.Get(contentTypeHeader) | ||
| return strings.Contains(contentType, "html") |
There was a problem hiding this comment.
Should we be more strict in the content-type headers we accept?
| var frameWrapperTmpl = template.Must(template.New("frame-wrapper").Parse(frameWrapperTemplate)) | ||
|
|
||
| func isHTMLRequest(r *http.Request) bool { | ||
| if r.Method != http.MethodGet { |
There was a problem hiding this comment.
Do we ever want to proxy and banner-ize other request types (e.g., POSTs)?
There was a problem hiding this comment.
I thought through this a bit and came to the conclusion that we should not inject the banner into responses for POSTS.
My concern is that an app might send a POST request and not be resilient to the response being modified.
As such, I think we should err on the side of not injecting the banner.
I've added comments to the code explaining this.
agent/banner/banner.go
Outdated
| } | ||
| responseRecorder := httptest.NewRecorder() | ||
| wrapped.ServeHTTP(responseRecorder, r) | ||
| result := responseRecorder.Result() |
There was a problem hiding this comment.
It seems a bit weird to use httptest for this purpose. Could we somehow inspect/transform the results in-line? How do the other proxy layers do this (or is the answer that they effectively return all responses unchanged)?
There was a problem hiding this comment.
Fair enough. I've changed it to modify the response in-line.
…tead of reading them in entirely and then replaying them
…ion more principled
This can be useful for adding functionality to a web-app without having to change the backend server.