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.

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

Remove dynamic input used in regular expression #2867

Merged
merged 7 commits into from
Jun 21, 2023
Merged

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented May 19, 2023

This PR removes the usage of base_url, which is retrieve from Rack::Request object, to compose the dynamic regular expression /^#{base_url}/.

Dynamically created regular expression from untrusted sources are an attack vector.

This PR replaces this regular expression with a plain string search for the base_url prefix.

@marcotc marcotc requested a review from a team May 19, 2023 18:15
@marcotc marcotc self-assigned this May 19, 2023
@github-actions github-actions bot added integrations Involves tracing integrations tracing labels May 19, 2023
@@ -304,7 +304,15 @@ def parse_url(env, original_env)
else
# normally REQUEST_URI starts at the path, but it
# might contain the full URL in some cases (e.g WEBrick)
request_uri.sub(/^#{base_url}/, '')
#
# DEV: Use `request_uri.delete_prefix(base_url)`, supported in Ruby 2.5+
Copy link
Member Author

Choose a reason for hiding this comment

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

I've received push back in the past with moving some refinements to a shared location.
A String#delete_prefix refinement already exists in ddtrace, but was kept close to its usage at the time:

unless String.method_defined?(:delete_prefix)
refine ::String do
def delete_prefix(prefix)
prefix = prefix.to_s
if rindex(prefix, 0)
self[prefix.length..-1]
else
dup
end
end
end
end

This PR could be simplified to use the refinement, but that refinement declaration would have to be moved to a more shareable location than it is today.

This PR becomes

- request_uri.sub(/^#{base_url}/, '')
+ request_uri.delete_prefix(base_url)

with the refinement.

Copy link
Member

Choose a reason for hiding this comment

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

I'll leave this up to you, although I would offer a suggestion of using a regular module to contain these "future apis", rather than refinements, e.g. something like:

module FromFuture
  if # Ruby 2.5+?
    def self.string_delete_prefix(string, prefix)
      string.delete_prefix(prefix)
    end

    #... other similar fixes...
  else
    def self.string_delete_prefix(string, prefix)
      ...
    end
  end
end

This would allow us to get the benefit of the newer implementation but avoid the refinements. Furthermore, when we drop support for older Rubies, we could look at this file and see which "if ..." branches could be dropped and we could then inline then where they're called.

Copy link
Member Author

Choose a reason for hiding this comment

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

I extracted it to its own method.

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

👍 LGTM

lib/datadog/tracing/contrib/rack/middlewares.rb Outdated Show resolved Hide resolved
@@ -304,7 +304,15 @@ def parse_url(env, original_env)
else
# normally REQUEST_URI starts at the path, but it
# might contain the full URL in some cases (e.g WEBrick)
request_uri.sub(/^#{base_url}/, '')
#
# DEV: Use `request_uri.delete_prefix(base_url)`, supported in Ruby 2.5+
Copy link
Member

Choose a reason for hiding this comment

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

I'll leave this up to you, although I would offer a suggestion of using a regular module to contain these "future apis", rather than refinements, e.g. something like:

module FromFuture
  if # Ruby 2.5+?
    def self.string_delete_prefix(string, prefix)
      string.delete_prefix(prefix)
    end

    #... other similar fixes...
  else
    def self.string_delete_prefix(string, prefix)
      ...
    end
  end
end

This would allow us to get the benefit of the newer implementation but avoid the refinements. Furthermore, when we drop support for older Rubies, we could look at this file and see which "if ..." branches could be dropped and we could then inline then where they're called.

@github-actions github-actions bot added the core Involves Datadog core libraries label Jun 19, 2023
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Comment on lines 3 to 12
module Datadog
module Core
module Utils
# Methods from future versions of Ruby implemented in for older rubies.
#
# This helps keep the project using newer APIs for never rubies and
# facilitates cleaning up when support for an older versions of Ruby is removed.
module Backport
# `String` class backports.
module String
Copy link
Member

Choose a reason for hiding this comment

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

Minor: As more of a "futurology" comment (e.g. thinking of how this may perhaps evolve in the future), maybe it would make sense to have a more top-level module to contain a bag of backports (e.g. Datadog::Core::Backport) or even per-minimum-Ruby-version-to-avoid-the-backport (Datadog::Core::BackportFrom24) versus having it scoped into Utils > String per-class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand the suggestion, could you expand the examples a bit more?

Copy link
Member

Choose a reason for hiding this comment

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

Yes!

My thinking is -- in this PR we create a Datadog::Core::Utils::Backport::String. This seems to be highly specific to, er, strings. So every time we need to do this, we'd need to create backport modules for every class.

Furthermore, when eventually we drop support for a few Ruby versions, we'll need to revisit these to figure out which are still needed.

So my suggestion is, why not have a backport module per Ruby version, that would have all backports for all classes there.

E.g. something like:

module Datadog
  module Core
    # This module is used to provide features from Ruby 2.5+ to older Rubies
    module BackportFrom25
      if ::String.method_defined?(:delete_prefix)
        def string_delete_prefix(string, prefix)
          string.delete_prefix(prefix)
        end
      else
        def string_delete_prefix(string, prefix)
          prefix = prefix.to_s
          if string.start_with?(prefix)
            string[prefix.length..-1] || raise('rbs-guard: String#[] is non-nil as `prefix` is guaranteed present')
          else
            string.dup
          end
        end
      end
    end
  end
end

# in another file
module Datadog
  module Core
    # This module is used to provide features from Ruby 2.3+ to older Rubies
    module BackportFrom23
      if ::Thread.method_defined?(:name=)
        def thread_name=(thread, name)
          thread.name = name
        end
      else
        def thread_name=(thread, name)
          # not supported, no-op
        end
      end
    end
  end
end 

This way:

  1. We'd have centralized places to find backports (we can always break them down later if needed)
  2. We know that when we drop support for Ruby 2.1 and 2.2, we can nuke BackportFrom23 and clean up everything that uses it

@codecov-commenter
Copy link

Codecov Report

Merging #2867 (ba6caf0) into master (bdd22a4) will decrease coverage by 0.01%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master    #2867      +/-   ##
==========================================
- Coverage   97.99%   97.99%   -0.01%     
==========================================
  Files        1280     1281       +1     
  Lines       70555    70569      +14     
  Branches     3237     3241       +4     
==========================================
+ Hits        69142    69154      +12     
- Misses       1413     1415       +2     
Impacted Files Coverage Δ
lib/datadog/core/backport.rb 75.00% <75.00%> (ø)
lib/datadog/tracing/contrib/rack/middlewares.rb 96.59% <100.00%> (+0.02%) ⬆️

... and 3 files with indirect coverage changes

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

@marcotc marcotc merged commit 09033a7 into master Jun 21, 2023
@marcotc marcotc deleted the remove-dynamic-regex branch June 21, 2023 19:36
@github-actions github-actions bot added this to the 1.13.0 milestone Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants