-
Notifications
You must be signed in to change notification settings - Fork 884
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
Open
quartzmo
wants to merge
8
commits into
googleapis:main
Choose a base branch
from
quartzmo:discovery-core-validation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+337
−144
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
73681e6
feat(core): implement path validation and traversal checks in HttpCom…
quartzmo c84404d
fix(core): simplify path validation to fail always on dots in double-…
quartzmo 38e5c18
docs: improve validate_path_parameters! documentation in HttpCommand
quartzmo ec1d995
docs: add @raise tags to execute and prepare! methods in HttpCommand
quartzmo 08bbbea
refactor(core): extract template scan regex to TEMPLATE_VAR_PATTERN c…
quartzmo f201209
refactor(core): unify dot check construct in HttpCommand
quartzmo e51a367
refactor(core): make dot validation error messages consistent in Http…
quartzmo 649445e
docs(core): refine TEMPLATE_VAR_PATTERN comments in HttpCommand
quartzmo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 == '' | ||
| 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 | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?