Use SafeRoundtripper to fetch templates #4
Conversation
vbrown608
changed the title
mailme.go
Outdated
| @@ -111,20 +112,20 @@ func (t *TemplateCache) Set(key, value string, expirationTime time.Duration) (*t | |||
| } | |||
|
|
|||
| func (t *TemplateCache) fetchTemplate(url string, triesLeft int) (string, error) { | |||
| client := http.Client{ | |||
| client := &http.Client{ | |||
| Transport: &http.Transport{ | |||
There was a problem hiding this comment.
no need to set the transport. it's going to be replaced in the safehttp call anyways
There was a problem hiding this comment.
The original transport gets wrapped by the new one. It's stored as noLocalTransport.inner.
However, Go's DefaultTransport is identical to the one defined here, except that the default has ForceAttemptHTTP2: true. Any idea if that's deliberate?
If it's not, we can just use the DefaultTransport. If it is, I think we should still start with the default and just update that one setting to make it explicit.
There was a problem hiding this comment.
agree on using DefaultTransport 👍
|
Yep if we're going with the DefaultTransport I think we can definitely do that :) |
|
Actually, we may want to specify a timeout on the default http client, like Otherwise a malicious template URL could make us try to read the body forever. |
this is actual a very limited attack vector because people could only provoke that for their own site and we don't care if people deliberately break it. i think we can ignore for now. a timeout might even be weird for customers because they might have apis that take a long time to provide the template and since mailing is never in the hot path it would not influence latency of a service. |
| }, | ||
| } | ||
|
|
||
| client := nfhttp.SafeHTTPClient(http.DefaultClient, t.logger) |
There was a problem hiding this comment.
The problem with the default client is that it doesn't have any concepts of timeouts. I don't think it is a huge issue but something to be aware of.
There was a problem hiding this comment.
See above comments ☝️
Updates template fetch to make use of netlify/netlify-commons#82