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

Jump to bottom

Handle REQUEST_URI with base url #2328

Merged
merged 2 commits into from
Oct 26, 2022
Merged

Handle REQUEST_URI with base url #2328

merged 2 commits into from
Oct 26, 2022

Conversation

lloeki
Copy link
Contributor

@lloeki lloeki commented Oct 25, 2022

This has been reported to happen on WEBrick, it may happen in other ways depending on how the variable is set.

What does this PR do?

Handle #2310 differently, preventing an exception for invalid URLs as found in #2309

Motivation

#2310 causes #2309 again because of the introduction of URI parsing via URI.join at a stage to early.

Additional Notes

How to test the change?

Start an app like so (with tracing enabled of course):

bundle exec puma -w 0 -p 9292
bundle exec rackup -s webrick -p 9292

Then curl it:

curl -vvv 'http://user:pass@127.0.0.1:9292/foo/bar|'

On Puma this should return a HTTP/1.1 404 Not Found and in the rack.request span tags the path should be replaced by a ? placeholder by quantization:

// with quantization: base: :show
      "http.url": "http://127.0.0.1:9292/?",
// with quantization: base: :exclude (default)
      "http.url": "?",
      "http.base_url": "http://127.0.0.1:9292",

On WEBrick, which attempts to parse the URI by itself, this should return an early HTTP/1.1 400 Bad Request:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN">
<HTML>
  <HEAD><TITLE>Bad Request</TITLE></HEAD>
  <BODY>
    <H1>Bad Request</H1>
    bad URI `/foo/bar|'.
    <HR>
    <ADDRESS>
     WEBrick/1.7.0 (Ruby/3.0.4/2022-04-12) at
     Cassini.local:9292
    </ADDRESS>
  </BODY>
</HTML>

Unfortunately I could not introduce proper non-regression spec examples like this:

diff --git a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb
index 69416eef1..9f3049128 100644
--- a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb
+++ b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb
@@ -126,6 +126,28 @@ RSpec.describe 'Rack integration tests' do
           it { expect(trace.resource).to eq('GET 200') }
         end
 
+        context 'with Unicode characters' do
+          let(:route) { '/success/?繋がってて' }
+
+          it_behaves_like 'a rack GET 200 span'
+
+          it do
+            expect(span.get_tag('http.url')).to eq('?')
+            expect(span.get_tag('http.base_url')).to eq('http://example.org')
+          end
+        end
+
+        context 'with unencoded ASCII characters' do
+          let(:route) { '/success/|' }
+
+          it_behaves_like 'a rack GET 200 span'
+
+          it do
+            expect(span.get_tag('http.url')).to eq('?')
+            expect(span.get_tag('http.base_url')).to eq('http://example.org')
+          end
+        end
+
         context 'with query string parameters' do
           let(:route) { '/success?foo=bar' }

Indeed Rack::Test tries to parse the route very early when its get is called, therefore triggers the exception way before anything happens:

   URI::InvalidURIError:
       bad URI(is not URI?): "/success/|"
     Shared Example Group: "a rack GET 200 span" called from ./spec/datadog/tracing/contrib/rack/integration_test_spec.rb:143
     # /usr/local/bundle/gems/rack-test-1.1.0/lib/rack/test.rb:214:in `parse_uri'
     # /usr/local/bundle/gems/rack-test-1.1.0/lib/rack/test.rb:127:in `custom_request'
     # /usr/local/bundle/gems/rack-test-1.1.0/lib/rack/test.rb:58:in `get'
     # ./spec/datadog/tracing/contrib/rack/integration_test_spec.rb:99:in `block (5 levels) in <top (required)>'

So, short of hacking Rack::Test to death I skipped adding these.

@lloeki
Handle REQUEST_URI with base url
b6f2f09 
This has been reported to happen on WEBrick, it may happen in other ways
depending on how the variable is set.
@lloeki lloeki requested a review from a team October 25, 2022 19:28
@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Oct 25, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2022 •
edited
Loading

Codecov Report

Merging #2328 (e805fd4) into master (9e0e9ac) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2328   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files        1092     1092           
  Lines       57778    57788   +10     
=======================================
+ Hits        56798    56808   +10     
  Misses        980      980
Impacted Files Coverage Δ
lib/datadog/tracing/contrib/rack/middlewares.rb 99.21% <100.00%> (ø)
...c/datadog/tracing/contrib/rack/middlewares_spec.rb 100.00% <100.00%> (ø)
...adog/tracing/contrib/sidekiq/client_tracer_spec.rb 97.91% <0.00%> (-2.09%) ⬇️
...adog/tracing/contrib/sidekiq/server_tracer_spec.rb 100.00% <0.00%> (+0.83%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

marcotc
marcotc approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

I added unit tests based on your suggestion. I think it's the best we can do at this very moment.

@lloeki lloeki merged commit 0c66c3b into master Oct 26, 2022
@lloeki lloeki deleted the fix-uri-exception-again branch October 26, 2022 11:15
@github-actions github-actions bot added this to the 1.6.0 milestone Oct 26, 2022
@lloeki lloeki mentioned this pull request Oct 26, 2022
Merged
lloeki added a commit that referenced this pull request Oct 26, 2022
@lloeki
Merge pull request #2330 from DataDog/backport/2328
de0697d 
Backport #2328
@lloeki lloeki mentioned this pull request Oct 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcotc marcotc marcotc approved these changes

Assignees
No one assigned
Labels
integrations Involves tracing integrations tracing
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@lloeki @codecov-commenter @marcotc