[APPSEC-9936] Update AppSec response content_type resolution #2900
Conversation
GustavoCaso
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2900 +/- ##
=======================================
Coverage 98.09% 98.09%
=======================================
Files 1268 1268
Lines 70031 70047 +16
Branches 3195 3195
=======================================
+ Hits 68697 68714 +17
+ Misses 1334 1333 -1
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
|
||
| accepted.each_with_object(DEFAULT_CONTENT_TYPE) do |range, _default| | ||
| match = FORMAT_MAP.keys.find { |type| range === type } | ||
| accepted.each do |range| |
There was a problem hiding this comment.
accepted.each_with_object(DEFAULT_CONTENT_TYPE) was making it so it returns DEFAULT_CONTENT_TYPE when no match has occured.
accepted.each now instead returns accepted, which is an array of MediaRange, so:
- the return value of
content_typeis now inconsistent: when a match is found it returns a content type but when none is found it returns an array ofMediaRange - when there is no match it does not return the default value anymore, which will break
FORMAT_MAP[content_type]
There was a problem hiding this comment.
Excellent point and catch @lloeki
I decided to know use each_with_object because I believe it doesn't express as clearly the intent of returning DEFAULT_CONTENT_TYPE as having an explicit return value at the end of the loop.
I added the necessary code and specs to make sure we do not introduce any regression on this code here:
fe93a72
lib/datadog/appsec/response.rb
Outdated
| accepted.each_with_object(DEFAULT_CONTENT_TYPE) do |range, _default| | ||
| match = FORMAT_MAP.keys.find { |type| range === type } | ||
| accepted.each do |range| | ||
| match = FORMAT_MAP.keys.find { |key| range === key } |
There was a problem hiding this comment.
FORMAT_MAP keys are content types, so I'm questioning the name change here.
I would even change it that way to make it clearer:
type_match = FORMAT_MAP.keys.find { |type| range === type }
return type_match if type_match
83dcc33
to
c4df9db
Compare
GustavoCaso
force-pushed
the
appsec-make-sure-blocked-reponse-page-is-correct
branch
from
c4df9db
to
fe93a72
Compare
|
|
||
| return match if match | ||
| accepted.each do |range| | ||
| type_match = CONTENT_TYPE_TO_FORMAT.keys.find { |type| range === type } |
There was a problem hiding this comment.
I noticed MediaRange#=== with a String as input will parse that input into a MediaRange before doing anything else. Aka, we keep parsing the items in CONTENT_TYPE_TO_FORMAT all the type on this check. Maybe something to improve?
What does this PR do?
Based on the [internal] specs. The default content type and the response of the blocked page should be
JSON.I update the code that handles that, I also added a bunch os new specs. I fixed an issue in which spaces between the
,for separating multiple HTTP accept values.Motivation
Additional Notes
How to test the change?
CI