-
Notifications
You must be signed in to change notification settings - Fork 886
feat(core): implement path validation and traversal checks in HttpCommand #27476
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
base: main
Are you sure you want to change the base?
Changes from all commits
73681e6
c84404d
38e5c18
ec1d995
08bbbea
f201209
e51a367
649445e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,7 @@ class HttpCommand | |
| RETRIABLE_ERRORS = [Google::Apis::ServerError, | ||
| Google::Apis::RateLimitError, | ||
| Google::Apis::TransmissionError, | ||
| Google::Apis::RequestTimeOutError] | ||
| Google::Apis::RequestTimeOutError].freeze | ||
|
|
||
| begin | ||
| require 'opencensus' | ||
|
|
@@ -83,15 +83,15 @@ def initialize(method, url, body: nil) | |
| self.url = url | ||
| self.url = Addressable::Template.new(url) if url.is_a?(String) | ||
| self.method = method | ||
| self.header = Hash.new | ||
| self.header =({}) | ||
| self.body = body | ||
| self.query = {} | ||
| self.params = {} | ||
| @opencensus_span = nil | ||
| if OPENCENSUS_AVAILABLE | ||
| logger.warn 'OpenCensus support is now deprecated. ' + | ||
| 'Please refer https://github.com/googleapis/google-api-ruby-client#tracing for migrating to use OpenTelemetry.' | ||
| 'Please refer https://github.com/googleapis/google-api-ruby-client#tracing for migrating to use OpenTelemetry.' | ||
|
|
||
| end | ||
| end | ||
|
|
||
|
|
@@ -104,6 +104,7 @@ def initialize(method, url, body: nil) | |
| # @raise [Google::Apis::ServerError] An error occurred on the server and the request can be retried | ||
| # @raise [Google::Apis::ClientError] The request is invalid and should not be retried without modification | ||
| # @raise [Google::Apis::AuthorizationError] Authorization is required | ||
| # @raise [Google::Apis::Error] If path parameter validation fails. | ||
| def execute(client, &block) | ||
| prepare! | ||
| opencensus_begin_span | ||
|
|
@@ -114,8 +115,8 @@ def execute(client, &block) | |
| release! | ||
| end | ||
|
|
||
| def do_retry func, client | ||
| begin | ||
| def do_retry(func, client) | ||
|
|
||
| Retriable.retriable tries: options.retries + 1, | ||
| max_elapsed_time: options.max_elapsed_time, | ||
| base_interval: options.base_interval, | ||
|
|
@@ -130,19 +131,17 @@ def do_retry func, client | |
| on: [Google::Apis::AuthorizationError, Signet::AuthorizationError, Signet::RemoteServerError, Signet::UnexpectedStatusError], | ||
| on_retry: proc { |*| refresh_authorization } do | ||
| send(func, client).tap do |result| | ||
| if block_given? | ||
| yield result, nil | ||
| end | ||
| yield result, nil if block_given? | ||
| end | ||
| end | ||
| end | ||
| rescue => e | ||
| rescue StandardError => e | ||
| if block_given? | ||
| yield nil, e | ||
| else | ||
| raise e | ||
| end | ||
| end | ||
|
|
||
| end | ||
|
|
||
| # Refresh the authorization authorization after a 401 error | ||
|
|
@@ -164,13 +163,15 @@ def authorization_refreshable? | |
| # | ||
| # @private | ||
| # @return [void] | ||
| # @raise [Google::Apis::Error] If path parameter validation fails. | ||
| def prepare! | ||
| normalize_unicode = true | ||
| if options | ||
| header.update(options.header) if options.header | ||
| query.update(options.query) if options.query | ||
| normalize_unicode = options.normalize_unicode | ||
| end | ||
| validate_path_parameters! if url.is_a?(Addressable::Template) | ||
| self.url = url.expand(params, nil, normalize_unicode) if url.is_a?(Addressable::Template) | ||
| url.query_values = normalize_query_values(query).merge(url.query_values || {}) | ||
|
|
||
|
|
@@ -182,17 +183,16 @@ def prepare! | |
| @form_encoded = false | ||
| end | ||
|
|
||
| self.body = '' if self.body.nil? && [:post, :put, :patch].include?(method) | ||
| self.body = '' if self.body.nil? && %i[post put patch].include?(method) | ||
| if defined?(::Google::Apis::Core::CompositeIO) && body.is_a?(::Google::Apis::Core::CompositeIO) | ||
| header["Content-Length"] ||= body.size.to_s | ||
| header['Content-Length'] ||= body.size.to_s | ||
| end | ||
| end | ||
|
|
||
| # Release any resources used by this command | ||
| # @private | ||
| # @return [void] | ||
| def release! | ||
| end | ||
| def release!; end | ||
|
|
||
| # Check the response and either decode body or raise error | ||
| # | ||
|
|
@@ -298,7 +298,7 @@ def error(err, rethrow: false, &block) | |
| err = Google::Apis::TransmissionError.new(err) | ||
| end | ||
| block.call(nil, err) if block_given? | ||
| fail err if rethrow || block.nil? | ||
| raise err if rethrow || block.nil? | ||
| end | ||
|
|
||
| # Execute the command once. | ||
|
|
@@ -323,7 +323,7 @@ def execute_once(client) | |
| logger.debug { safe_single_line_representation @http_res } | ||
| response = process_response(@http_res.status.to_i, @http_res.headers, @http_res.body) | ||
| success(response) | ||
| rescue => e | ||
| rescue StandardError => e | ||
| logger.debug { sprintf('Caught error %s', e) } | ||
| error(e, rethrow: true) | ||
| end | ||
|
|
@@ -343,28 +343,43 @@ def apply_request_options(req_header) | |
| end | ||
|
|
||
| def allow_form_encoding? | ||
| [:post, :put].include?(method) && body.nil? | ||
| %i[post put].include?(method) && body.nil? | ||
| end | ||
|
|
||
| # Set the API version header for the service if not empty. | ||
| # @return [void] | ||
| def set_api_version_header api_version | ||
| def set_api_version_header(api_version) | ||
| self.header['X-Goog-Api-Version'] = api_version unless api_version.empty? | ||
| end | ||
|
|
||
| private | ||
|
|
||
| UNSAFE_CLASS_NAMES = [ | ||
| "Google::Apis::CloudkmsV1::DecryptResponse", | ||
| "Google::Apis::SecretmanagerV1::SecretPayload", | ||
| "Google::Apis::SecretmanagerV1beta1::SecretPayload" | ||
| ] | ||
| 'Google::Apis::CloudkmsV1::DecryptResponse', | ||
| 'Google::Apis::SecretmanagerV1::SecretPayload', | ||
| 'Google::Apis::SecretmanagerV1beta1::SecretPayload' | ||
| ].freeze | ||
|
|
||
| # Pattern to scan for RFC 6570 URI template expressions enclosed in curly braces {...}. | ||
| # | ||
| # Regex Mechanics: | ||
| # - Matches literal '{' and '}' characters. | ||
| # - Capture Group 1: Captures a single prefix operator character if present (one of: +, #, ., /, ;, ?, &). | ||
| # - Capture Group 2: Captures all characters up to the closing brace ('[^}]+'). | ||
| # | ||
| # Expected Capture Format: | ||
| # Capture Group 2 is expected to contain a raw string block representing one or more variable | ||
| # definitions (e.g. "var1" or "var1,var2"). This string block may contain commas, variable names, | ||
| # prefix length constraints (e.g. "var1:5"), or explode modifiers (e.g. "var1*"). The commas are | ||
| # split at the code level during traversal. | ||
| TEMPLATE_VAR_PATTERN = /\{([\+#\.\/;\?&])?([^}]+)\}/.freeze | ||
|
|
||
| module RedactingPPMethods | ||
| def pp_object obj | ||
| def pp_object(obj) | ||
| return super unless UNSAFE_CLASS_NAMES.include? obj.class.name | ||
|
|
||
| object_address_group obj do | ||
| text "(fields redacted)" | ||
| text '(fields redacted)' | ||
| end | ||
| end | ||
| end | ||
|
|
@@ -377,16 +392,16 @@ class RedactingSingleLine < PP::SingleLine | |
| include RedactingPPMethods | ||
| end | ||
|
|
||
| def safe_pretty_representation obj | ||
| out = +"" | ||
| def safe_pretty_representation(obj) | ||
| out = +'' | ||
| printer = RedactingPP.new out, 79 | ||
| printer.guard_inspect_key { printer.pp obj } | ||
| printer.flush | ||
| out << "\n" | ||
| end | ||
|
|
||
| def safe_single_line_representation obj | ||
| out = +"" | ||
| def safe_single_line_representation(obj) | ||
| out = +'' | ||
| printer = RedactingSingleLine.new out | ||
| printer.guard_inspect_key { printer.pp obj } | ||
| printer.flush | ||
|
|
@@ -400,18 +415,16 @@ def opencensus_begin_span | |
|
|
||
| @opencensus_span = OpenCensus::Trace.start_span url.path.to_s | ||
| @opencensus_span.kind = OpenCensus::Trace::SpanBuilder::CLIENT | ||
| @opencensus_span.put_attribute "http.host", url.host.to_s | ||
| @opencensus_span.put_attribute "http.method", method.to_s.upcase | ||
| @opencensus_span.put_attribute "http.path", url.path.to_s | ||
| @opencensus_span.put_attribute 'http.host', url.host.to_s | ||
| @opencensus_span.put_attribute 'http.method', method.to_s.upcase | ||
| @opencensus_span.put_attribute 'http.path', url.path.to_s | ||
| sent_size = | ||
| if body.respond_to? :bytesize | ||
| body.bytesize | ||
| elsif body.nil? | ||
| 0 | ||
| end | ||
| if sent_size | ||
| @opencensus_span.put_message_event OpenCensus::Trace::SpanBuilder::SENT, 1, sent_size | ||
| end | ||
| @opencensus_span.put_message_event OpenCensus::Trace::SpanBuilder::SENT, 1, sent_size if sent_size | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. super nit: I actually like the original pattern (more explicit/readable) FWIW |
||
|
|
||
| formatter = OpenCensus::Trace.config.http_formatter | ||
| if formatter.respond_to? :header_name | ||
|
|
@@ -436,7 +449,7 @@ def opencensus_end_span | |
| status = @http_res.status.to_i | ||
| if status > 0 | ||
| @opencensus_span.set_status map_http_status status | ||
| @opencensus_span.put_attribute "http.status_code", status | ||
| @opencensus_span.put_attribute 'http.status_code', status | ||
| end | ||
| end | ||
|
|
||
|
|
@@ -452,7 +465,7 @@ def form_encoded? | |
| @form_encoded | ||
| end | ||
|
|
||
| def map_http_status http_status | ||
| def map_http_status(http_status) | ||
| case http_status | ||
| when 200..399 then 0 # OK | ||
| when 400 then 3 # INVALID_ARGUMENT | ||
|
|
@@ -468,9 +481,8 @@ def map_http_status http_status | |
| end | ||
|
|
||
| def normalize_query_values(input) | ||
| input.inject({}) do |h, (k, v)| | ||
| input.each_with_object({}) do |(k, v), h| | ||
| h[k] = normalize_query_value(v) | ||
| h | ||
| end | ||
| end | ||
|
|
||
|
|
@@ -484,6 +496,63 @@ def normalize_query_value(v) | |
| v.to_s | ||
| end | ||
| end | ||
|
|
||
| # Validates user-supplied path parameter values against the URL template specification | ||
| # to prevent directory traversal and parameter injection exploits. | ||
| # | ||
| # Validation Mechanism: | ||
| # 1. Identifies query/fragment injections: Rejects values containing '?' or '#' characters. | ||
| # 2. For simple variables (standard single-wildcard behavior): | ||
| # - Rejects if the value contains '/' (cannot span multiple path segments). | ||
| # - Rejects if the value is exactly '.' or '..'. | ||
| # 3. For reserved variables (reserved expansion like '+' or '#', double-wildcard behavior): | ||
| # - Splits the value by slash ('/') using a `-1` limit to preserve empty trailing segments. | ||
| # - Rejects if any segment is a directory traversal segment ('.' or '..'). | ||
| # - Rejects empty segments ('', meaning duplicate slashes '//' or trailing slashes). | ||
| # | ||
| # @raise [Google::Apis::Error] If any validation check fails. | ||
| def validate_path_parameters! | ||
| template_pattern = url.pattern | ||
|
|
||
| # Parse variables and operators | ||
| variables = [] | ||
| template_pattern.scan(TEMPLATE_VAR_PATTERN) do |operator, var_list| | ||
| var_list.split(',').each do |var| | ||
| var_name = var.split(':').first.split('*').first | ||
| variables << { name: var_name, operator: operator, reserved: (operator == '+' || operator == '#') } | ||
| end | ||
| end | ||
|
|
||
| variables.each do |v| | ||
| var_name = v[:name] | ||
| var_key = params.key?(var_name) ? var_name : var_name.to_sym | ||
| next unless params.key?(var_key) | ||
| value = params[var_key].to_s | ||
|
|
||
| if value.include?('?') || value.include?('#') | ||
| raise Google::Apis::Error, "Parameter #{var_name} contains invalid characters (? or #)" | ||
| end | ||
|
|
||
| if v[:reserved] | ||
| value_segments = value.split('/', -1) | ||
| value_segments.each do |seg| | ||
| if seg == '.' || seg == '..' | ||
| raise Google::Apis::Error, | ||
| "Path traversal segment #{seg.inspect} is not allowed in parameter #{var_name}: #{value}" | ||
| end | ||
| raise Google::Apis::Error, "Invalid path segment '' in parameter #{var_name}" if seg == '' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
raise Google::Apis::Error, "Empty path segment (e.g. '//' or trailing '/') is not allowed in parameter #{var_name}: #{value}" if seg == '' |
||
| end | ||
| else | ||
| if value.include?('/') | ||
| raise Google::Apis::Error, "Simple parameter #{var_name} cannot contain slashes: #{value}" | ||
| end | ||
| if value == '.' || value == '..' | ||
| raise Google::Apis::Error, | ||
| "Path traversal segment #{value.inspect} is not allowed in parameter #{var_name}: #{value}" | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious whether this was a Rubocop fix or was there a technical detail about instantiating a Hash one way vs the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubocop!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we fix the missing space? FWIW, I think
self.header = {}is sufficient.