Allow deep mask json and custom JSON serializer. Fix non UTF-8 for js…#85
Allow deep mask json and custom JSON serializer. Fix non UTF-8 for js…#85trusche merged 5 commits intotrusche:masterfrom
Conversation
Meat-Chopper
force-pushed
the
mask_json
branch
from
23ac735 to
88b353d
Compare
| end | ||
| end | ||
|
|
||
| describe 'Non UTF-8 with JSON log' do |
There was a problem hiding this comment.
It fails in version 1.3.2
| # Capitalized for ::HTTP | ||
| %w[content-type Content-Type content-encoding Content-Encoding].include? k | ||
| end.to_h.each_with_object({}) { |(k, v), h| h[k.downcase] = v } | ||
|
|
There was a problem hiding this comment.
Maybe it is better to move this logic to the adapters?
There was a problem hiding this comment.
Firstly, this reads a little hard. I'd replace k with header on the first block - I tend to reserve single character variable names for single line blocks (as in 276).
There was a problem hiding this comment.
Secondly, it's not at all clear to me what this block does. Can you add a comment, or perhaps use clearer parameter names? The result of this block is not the request, it's just a bunch of headers.
But leaving the logic here is fine, to deal with differently capitalized headers in one place. It just needs to be clearer that that's what we're doing here. Perhaps move it to a dedicated class method.
trusche
left a comment
trusche
left a comment
There was a problem hiding this comment.
Thanks for the PR, looks good in principle. Just a few small tweaks please. Lots of tests is great - thanks!
| @prefix_data_lines = false | ||
| @prefix_response_lines = false | ||
| @prefix_line_numbers = false | ||
| @mask_json = false |
There was a problem hiding this comment.
This parameter seems redundant. One could just enable global JSON masking by setting the URL parameter to /.*/. The regex is checked every time anyway.
There was a problem hiding this comment.
If filter_parameters is set, we should just mask JSON as well by default.
|
|
||
| def masked_body_url?(url) | ||
| config.filter_parameters.any? && | ||
| (config.url_masked_body_pattern && url.to_s.match(config.url_masked_body_pattern) || config.mask_json) |
There was a problem hiding this comment.
See above - the final condition wouldn't be necessary if url_masked_body_pattern == /.*/
| begin | ||
| result = masked_data(config.json_parser.load(result)) | ||
| rescue => e | ||
| e.message + ': ' + result |
There was a problem hiding this comment.
This silently swallow a failure. Perhaps we should just log an error message and return an empty string so we're not accidentally leaking sensitive data into the logfile. Or looking further down, perhaps that's what's indented already and you forgot to re-raise the exception here?
| # Capitalized for ::HTTP | ||
| %w[content-type Content-Type content-encoding Content-Encoding].include? k | ||
| end.to_h.each_with_object({}) { |(k, v), h| h[k.downcase] = v } | ||
|
|
There was a problem hiding this comment.
Firstly, this reads a little hard. I'd replace k with header on the first block - I tend to reserve single character variable names for single line blocks (as in 276).
| # Capitalized for ::HTTP | ||
| %w[content-type Content-Type content-encoding Content-Encoding].include? k | ||
| end.to_h.each_with_object({}) { |(k, v), h| h[k.downcase] = v } | ||
|
|
There was a problem hiding this comment.
Secondly, it's not at all clear to me what this block does. Can you add a comment, or perhaps use clearer parameter names? The result of this block is not the request, it's just a bunch of headers.
But leaving the logic here is fine, to deal with differently capitalized headers in one place. It just needs to be clearer that that's what we're doing here. Perhaps move it to a dedicated class method.
| let(:mask_json) { HttpLog.configuration.mask_json } | ||
| let(:json_parser) { HttpLog.configuration.json_parser } | ||
| let(:filter_parameters) { HttpLog.configuration.filter_parameters } | ||
| let(:url_masked_body_pattern) { HttpLog.configuration.url_masked_body_pattern } |
There was a problem hiding this comment.
Align the block indentation above please.
|
Hello @trusche, |
Meat-Chopper
force-pushed
the
mask_json
branch
2 times, most recently
from
cff4bfc to
79a1678
Compare
…on and graylog formats.
Meat-Chopper
force-pushed
the
mask_json
branch
2 times, most recently
from
34eb49a to
3a9afe5
Compare
|
Released as v1.4.0. Thanks again! |
2 participants
Hi,
I see the implementation of JSON data masking in the "develop" branch. But since it has not yet been merged with the master, I have decided to try with my own implementation.
Also there is a bug when JSON is unable to dump non UTF-8 symbols.