Support scrubbing gzip encoded bundles #36
Conversation
jurassix
force-pushed
the
clint/WEB-14177
branch
from
5364b93
to
d3f80d9
Compare
| @@ -82,7 +81,7 @@ func (service *Service) LastRequestBody() ([]byte, error) { | |||
| } | |||
|
|
|||
| defer request.Body.Close() | |||
| body, err := ioutil.ReadAll(request.Body) | |||
| body, err := io.ReadAll(request.Body) | |||
There was a problem hiding this comment.
Note ioutil is deprecated and internally just calls the io package, so I've updated all usages of ioutil.
|
cc @eugene-chang-fs @sethfowler this PR adds support for GZIP compressed bundle data being scrubbed and relayed correct. |
| @@ -94,7 +94,7 @@ func (f contentBlockerPluginFactory) New(configSection *config.Section) (traffic | |||
| case "header": | |||
| plugin.headerBlockers = append(plugin.headerBlockers, blockers...) | |||
| default: | |||
| return fmt.Errorf(`Unexpected content kind %s`, contentKind) | |||
| return fmt.Errorf(`unexpected content kind %s`, contentKind) | |||
There was a problem hiding this comment.
Note capitalization of error messages is a style lint warning for Golang so fixed these as well generally.
https://google.github.io/styleguide/go/decisions.html#error-strings
| contentLength := int64(len(processedBody)) | ||
| if contentLength != request.ContentLength { |
There was a problem hiding this comment.
Small change to directly verify the content length is valid instead of indirectly through the existing body content.
| runContentBlockerTest(t, testCase, Identity) | ||
| runContentBlockerTest(t, testCase, Gzip) |
There was a problem hiding this comment.
Modified this so all blocking tests are verified for compressed and identity encoded data.
relay/traffic/handler.go
Outdated
| encoding, err := GetContentEncoding(request) | ||
| if err != nil { | ||
| logger.Printf("URL %v error getting request content encoding: %v", request.URL, err) | ||
| request.Body = http.NoBody | ||
| return | ||
| } | ||
|
|
||
| if err := handler.prepareRequestBody(request, encoding); err != nil { | ||
| http.Error(response, fmt.Sprintf("Error setting up clientRequest body reader: %s", err), 500) | ||
| request.Body = http.NoBody | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Changes here preserve content decoding + encoding is working correctly and supports the assumption of our existing plugins. It's possible that the content-blocker-plugin could be modified to work with encoded data. However, our relay plugin model allows for customer created plugins that are expecting data to decoded. Wrapping the decode + encoding at the traffic handler layer preserves our existing ecosystem assumptions.
| // Create a new gzip.Reader to decompress the request body | ||
| return gzip.NewReader(request.Body) | ||
| default: | ||
| // If the content is not gzip-compressed, return the original request body |
There was a problem hiding this comment.
In all of these functions, should we be reporting an error if we don't recognize the encoding?
There was a problem hiding this comment.
SGTM - I'll handle "" and "gzip" as the 2 supported cases today
There was a problem hiding this comment.
@sethfowler feedback incorporated! PTAL
| @@ -99,6 +124,42 @@ func (handler *Handler) HandleRequest(clientResponse http.ResponseWriter, client | |||
| } | |||
| } | |||
|
|
|||
| func (handler *Handler) ensureBodyContentEncoding(clientRequest *http.Request, encoding Encoding) { | |||
There was a problem hiding this comment.
NIT: maybe add a comment that this function does not change the qs/header about the existing encoding; this function operates on the assumption that the downstream proxy target will be using the same encoding as what the relay had received (and likely this is always the case), but it wouldn't hurt to point out
jurassix
force-pushed
the
clint/WEB-14177
branch
from
f98f1b8
to
a52dacd
Compare
FullStory is rolling out gzip compression on the data sent from the client via
/rec/bundle. This PR ensures that the plugin behaviour of scrubbing PII within Relay is preserved.