Commit c0425241 authored by Stan Hu's avatar Stan Hu

Fix obsoleted URI.{encode,decode} calls for Ruby 2.7

Attempting to use these calls results in these obsolete warnings:

```
warning: URI.escape is obsolete
```

As mentioned by the Knapsack Pro blog:


> The trouble with a concept of “escaping the URI” is that URI consists of
> many components (like path or query), and we don’t want to escape them
> in the same way. For example, it’s fine for a # character to appear at
> the end of the URI (when it’s used as what’s usually called an anchor,
> or in URI parlance - a fragment component) - but when the same # is part
> of user’s input (like in a search query), we want to encode it to ensure
> correct interpretation. Similarly, if the query string value contains
> other reserved characters, like = or &, we do want to escape them so
> that they are not incorrectly interpreted as delimiters, as if they were
> used as reserved characters.
>
> URI.escape relies on a simple gsub operation for the whole string and
> doesn’t differentiate between distinct components, which doesn’t take
> into account intricacies like those mentioned above.


CGI.escape and CGI.encode_www_form_component are often mentioned as
replacements, but both substitute spaces for `+` instead of `%20`. This
doesn't work for Grafana. Using `%20` for spaces everywhere is valid,
which is what `Addressable::URI` does.
parent d487b9e2
...@@ -105,14 +105,6 @@ Lint/StructNewOverride: ...@@ -105,14 +105,6 @@ Lint/StructNewOverride:
- 'app/serializers/environment_serializer.rb' - 'app/serializers/environment_serializer.rb'
- 'lib/gitlab/ci/pipeline/duration.rb' - 'lib/gitlab/ci/pipeline/duration.rb'
# Offense count: 7
Lint/UriEscapeUnescape:
Exclude:
- 'app/controllers/application_controller.rb'
- 'app/models/project_services/drone_ci_service.rb'
- 'spec/lib/google_api/auth_spec.rb'
- 'spec/requests/api/files_spec.rb'
# Offense count: 65 # Offense count: 65
# Cop supports --auto-correct. # Cop supports --auto-correct.
Migration/DepartmentName: Migration/DepartmentName:
......
...@@ -484,7 +484,7 @@ class ApplicationController < ActionController::Base ...@@ -484,7 +484,7 @@ class ApplicationController < ActionController::Base
def set_page_title_header def set_page_title_header
# Per https://tools.ietf.org/html/rfc5987, headers need to be ISO-8859-1, not UTF-8 # Per https://tools.ietf.org/html/rfc5987, headers need to be ISO-8859-1, not UTF-8
response.headers['Page-Title'] = URI.escape(page_title('GitLab')) response.headers['Page-Title'] = Addressable::URI.encode_component(page_title('GitLab'))
end end
def set_current_admin(&block) def set_current_admin(&block)
......
...@@ -42,7 +42,7 @@ class DroneCiService < CiService ...@@ -42,7 +42,7 @@ class DroneCiService < CiService
def commit_status_path(sha, ref) def commit_status_path(sha, ref)
Gitlab::Utils.append_path( Gitlab::Utils.append_path(
drone_url, drone_url,
"gitlab/#{project.full_path}/commits/#{sha}?branch=#{URI.encode(ref.to_s)}&access_token=#{token}") "gitlab/#{project.full_path}/commits/#{sha}?branch=#{Addressable::URI.encode_component(ref.to_s)}&access_token=#{token}")
end end
def commit_status(sha, ref) def commit_status(sha, ref)
...@@ -75,7 +75,7 @@ class DroneCiService < CiService ...@@ -75,7 +75,7 @@ class DroneCiService < CiService
def build_page(sha, ref) def build_page(sha, ref)
Gitlab::Utils.append_path( Gitlab::Utils.append_path(
drone_url, drone_url,
"gitlab/#{project.full_path}/redirect/commits/#{sha}?branch=#{URI.encode(ref.to_s)}") "gitlab/#{project.full_path}/redirect/commits/#{sha}?branch=#{Addressable::URI.encode_component(ref.to_s)}")
end end
def title def title
......
...@@ -19,8 +19,8 @@ module Grafana ...@@ -19,8 +19,8 @@ module Grafana
# @param name [String] Unique identifier for a Grafana datasource # @param name [String] Unique identifier for a Grafana datasource
def get_datasource(name:) def get_datasource(name:)
# CGI#escape formats strings such that the Grafana endpoint # CGI#escape formats strings such that the Grafana endpoint
# will not recognize the dashboard name. Preferring URI#escape. # will not recognize the dashboard name. Prefer Addressable::URI#encode_component.
http_get("#{@api_url}/api/datasources/name/#{URI.escape(name)}") # rubocop:disable Lint/UriEscapeUnescape http_get("#{@api_url}/api/datasources/name/#{Addressable::URI.encode_component(name)}")
end end
# @param datasource_id [String] Grafana ID for the datasource # @param datasource_id [String] Grafana ID for the datasource
......
...@@ -12,12 +12,12 @@ RSpec.describe GoogleApi::Auth do ...@@ -12,12 +12,12 @@ RSpec.describe GoogleApi::Auth do
end end
describe '#authorize_url' do describe '#authorize_url' do
subject { client.authorize_url } subject { Addressable::URI.parse(client.authorize_url) }
it 'returns authorize_url' do it 'returns authorize_url' do
is_expected.to start_with('https://accounts.google.com/o/oauth2') expect(subject.to_s).to start_with('https://accounts.google.com/o/oauth2')
is_expected.to include(URI.encode(redirect_uri, URI::PATTERN::RESERVED)) expect(subject.query_values['state']).to eq(redirect_to)
is_expected.to include(URI.encode(redirect_to, URI::PATTERN::RESERVED)) expect(subject.query_values['redirect_uri']).to eq(redirect_uri)
end end
end end
......
...@@ -750,7 +750,7 @@ RSpec.describe API::Files do ...@@ -750,7 +750,7 @@ RSpec.describe API::Files do
it "updates existing file in project repo with accepts correct last commit id" do it "updates existing file in project repo with accepts correct last commit id" do
last_commit = Gitlab::Git::Commit last_commit = Gitlab::Git::Commit
.last_for_path(project.repository, 'master', URI.unescape(file_path)) .last_for_path(project.repository, 'master', Addressable::URI.unencode_component(file_path))
params_with_correct_id = params.merge(last_commit_id: last_commit.id) params_with_correct_id = params.merge(last_commit_id: last_commit.id)
put api(route(file_path), user), params: params_with_correct_id put api(route(file_path), user), params: params_with_correct_id
...@@ -760,7 +760,7 @@ RSpec.describe API::Files do ...@@ -760,7 +760,7 @@ RSpec.describe API::Files do
it "returns 400 when file path is invalid" do it "returns 400 when file path is invalid" do
last_commit = Gitlab::Git::Commit last_commit = Gitlab::Git::Commit
.last_for_path(project.repository, 'master', URI.unescape(file_path)) .last_for_path(project.repository, 'master', Addressable::URI.unencode_component(file_path))
params_with_correct_id = params.merge(last_commit_id: last_commit.id) params_with_correct_id = params.merge(last_commit_id: last_commit.id)
put api(route(rouge_file_path), user), params: params_with_correct_id put api(route(rouge_file_path), user), params: params_with_correct_id
...@@ -772,7 +772,7 @@ RSpec.describe API::Files do ...@@ -772,7 +772,7 @@ RSpec.describe API::Files do
it_behaves_like 'when path is absolute' do it_behaves_like 'when path is absolute' do
let(:last_commit) do let(:last_commit) do
Gitlab::Git::Commit Gitlab::Git::Commit
.last_for_path(project.repository, 'master', URI.unescape(file_path)) .last_for_path(project.repository, 'master', Addressable::URI.unencode_component(file_path))
end end
let(:params_with_correct_id) { params.merge(last_commit_id: last_commit.id) } let(:params_with_correct_id) { params.merge(last_commit_id: last_commit.id) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment