Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[APPSEC-9936] Update AppSec response content_type resolution #2900

Merged
merged 2 commits into from
Jun 12, 2023
Merged

[APPSEC-9936] Update AppSec response content_type resolution #2900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15a0801
Update AppSec response content_type resolution
GustavoCaso Jun 7, 2023
fe93a72
Apply feedabck and add test to check unsupported Accept header value
GustavoCaso Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions lib/datadog/appsec/response.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,30 +36,34 @@ def negotiate(env)
Response.new(
status: 403,
headers: { 'Content-Type' => content_type },
body: [Datadog::AppSec::Assets.blocked(format: FORMAT_MAP[content_type])]
body: [Datadog::AppSec::Assets.blocked(format: CONTENT_TYPE_TO_FORMAT[content_type])]
)
end

private

FORMAT_MAP = {
'text/plain' => :text,
'text/html' => :html,
CONTENT_TYPE_TO_FORMAT = {
'application/json' => :json,
'text/html' => :html,
'text/plain' => :text,
}.freeze

DEFAULT_CONTENT_TYPE = 'text/plain'
DEFAULT_CONTENT_TYPE = 'application/json'

def content_type(env)
return DEFAULT_CONTENT_TYPE unless env.key?('HTTP_ACCEPT')

accepted = env['HTTP_ACCEPT'].split(',').map { |m| Utils::HTTP::MediaRange.new(m) }.sort!.reverse!
accept_types = env['HTTP_ACCEPT'].split(',').map(&:strip)

accepted.each_with_object(DEFAULT_CONTENT_TYPE) do |range, _default|
match = FORMAT_MAP.keys.find { |type| range === type }
accepted = accept_types.map { |m| Utils::HTTP::MediaRange.new(m) }.sort!.reverse!

return match if match
accepted.each do |range|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_type is now inconsistent: when a match is found it returns a content type but when none is found it returns an array of MediaRange
  • when there is no match it does not return the default value anymore, which will break FORMAT_MAP[content_type]

Copy link
Member Author

@GustavoCaso GustavoCaso Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

type_match = CONTENT_TYPE_TO_FORMAT.keys.find { |type| range === type }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good 😄


return type_match if type_match
end

DEFAULT_CONTENT_TYPE
rescue Datadog::AppSec::Utils::HTTP::MediaRange::ParseError
DEFAULT_CONTENT_TYPE
end
Expand Down
2 changes: 1 addition & 1 deletion sig/datadog/appsec/response.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module Datadog

private

FORMAT_MAP: ::Hash[::String, ::Symbol]
CONTENT_TYPE_TO_FORMAT: ::Hash[::String, ::Symbol]
DEFAULT_CONTENT_TYPE: ::String

def self.format: (::Hash[untyped, untyped] env) -> ::Symbol
Expand Down
30 changes: 27 additions & 3 deletions spec/datadog/appsec/response_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,19 +74,43 @@
end

context('without Accept header') do
it { is_expected.to eq 'text/plain' }
it { is_expected.to eq 'application/json' }
end

context('with Accept: */*') do
let(:accept) { '*/*' }

it { is_expected.to eq 'text/plain' }
it { is_expected.to eq 'application/json' }
end

context('with Accept: text/*') do
let(:accept) { 'text/*' }

it { is_expected.to eq 'text/html' }
end

context('with Accept: application/*') do
let(:accept) { 'application/*' }

it { is_expected.to eq 'application/json' }
end

context('with unparseable Accept header') do
let(:accept) { 'invalid' }

it { is_expected.to eq 'text/plain' }
it { is_expected.to eq 'application/json' }
end

context('with Accept: text/*;q=0.7, application/*;q=0.8, */*;q=0.9') do
let(:accept) { 'text/*;q=0.7, application/*;q=0.8, */*;q=0.9' }

it { is_expected.to eq 'application/json' }
end

context('with unsupported Accept header') do
let(:accept) { 'image/webp' }

it { is_expected.to eq 'application/json' }
end

context('with Mozilla Firefox Accept') do
Expand Down