Skip to content

Commit

Permalink
Handle incorrectly UTF-8 encoded query and cookie url
Browse files Browse the repository at this point in the history
Use a different exception when this situation is detected:
* Sanitiser::Strategy::SanitisingError
This error is not sent to Sentry.
See: alphagov/govuk_app_config#402
  • Loading branch information
unoduetre committed Oct 25, 2024
1 parent b179f9d commit 37135b7
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 2 deletions.
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ gem "govuk_app_config"
gem "govuk_document_types"
gem "govuk_publishing_components"
gem "plek"
gem "rack-utf8_sanitizer"
gem "rails-i18n"
gem "rails_translation_manager"
gem "rinku", require: "rails_rinku"
Expand Down
3 changes: 3 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,8 @@ GEM
rack (>= 3.0.0)
rack-test (2.1.0)
rack (>= 1.3)
rack-utf8_sanitizer (1.9.1)
rack (>= 1.0, < 4.0)
rackup (2.1.0)
rack (>= 3)
webrick (~> 1.8)
Expand Down Expand Up @@ -795,6 +797,7 @@ DEPENDENCIES
pact_broker-client
plek
pry-byebug
rack-utf8_sanitizer
rails (= 7.2.1.1)
rails-controller-testing
rails-i18n
Expand Down
13 changes: 11 additions & 2 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# require "action_view/railtie"
# require "action_cable/engine"
require "rails/test_unit/railtie"
require_relative "../lib/sanitiser/strategy"

# Require the gems listed in Gemfile, including any gems
# you've limited to :test, :development, or :production.
Expand Down Expand Up @@ -117,12 +118,20 @@ class Application < Rails::Application
# Please, add to the `ignore` list any other `lib` subdirectories that do
# not contain `.rb` files, or that should not be reloaded or eager loaded.
# Common ones are `templates`, `generators`, or `middleware`, for example.
config.autoload_lib(ignore: %w[assets tasks])
config.autoload_lib(ignore: %w[assets tasks sanitiser])

# Configuration for the application, engines, and railties goes here.
#
# These settings can be overridden in specific environments using the files
# in config/environments, which are processed later.
#

# Protect from "invalid byte sequence in UTF-8" errors,
# when a query or a cookie is a string with incorrect UTF-8 encoding.
config.middleware.insert_before(
Rack::Head,
Rack::UTF8Sanitizer,
only: %w[QUERY_STRING],
strategy: Sanitiser::Strategy,
)
end
end
18 changes: 18 additions & 0 deletions lib/sanitiser/strategy.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
module Sanitiser
class Strategy
class SanitisingError < StandardError; end

class << self
def call(input, sanitize_null_bytes: false)
input
.force_encoding(Encoding::ASCII_8BIT)
.encode!(Encoding::UTF_8)
if sanitize_null_bytes && Rack::UTF8Sanitizer::NULL_BYTE_REGEX.match?(input)
raise NullByteInString
end
rescue StandardError
raise SanitisingError, "Non-UTF-8 (or null) character in the query or in the cookie"
end
end
end
end
86 changes: 86 additions & 0 deletions spec/requests/sanitiser_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
require "slimmer/test"

RSpec.describe "Sanitiser" do
context "with query being correct percent-encoded UTF-8 string does not raise exception" do
it "does not raise error" do
get "/?%41"
expect(response).to have_http_status(:ok)
end
end

context "with query being incorrect percent-encoded UTF-8 string raises exception" do
it "raises Sanitiser::Strategy::SanitisingError" do
expect { get "/?%AD" }.to raise_error(Sanitiser::Strategy::SanitisingError)
end
end

context "with cookie key being correct UTF-8" do
it "does not raise error" do
get "/", headers: { Cookie: "\x41=value" }
expect(response).to have_http_status(:ok)
end
end

context "with cookie key being incorrect UTF-8" do
it "raises ArgumentError" do
expect { get "/", headers: { Cookie: "\xAD=value" } }

Check failure on line 26 in spec/requests/sanitiser_spec.rb

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

Layout/ExtraSpacing: Unnecessary spacing detected.
.to raise_error(ArgumentError, "invalid byte sequence in UTF-8")
end
end

context "with cookie value being correct UTF-8" do
it "does not raise error" do
get "/", headers: { Cookie: "key=\x41" }
expect(response).to have_http_status(:ok)
end
end

context "with cookie value being incorrect UTF-8" do
it "raises ArgumentError" do
expect { get "/", headers: { Cookie: "key=\xAD" } }

Check failure on line 40 in spec/requests/sanitiser_spec.rb

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

Layout/ExtraSpacing: Unnecessary spacing detected.
.to raise_error(ArgumentError, "invalid byte sequence in UTF-8")
end
end

context "with cookie path being correct UTF-8" do
it "does not raise error" do
get "/", headers: { Cookie: "key=value; Path=/\x41" }
expect(response).to have_http_status(:ok)
end
end

context "with cookie path being incorrect UTF-8" do
it "raises ArgumentError" do
expect { get "/", headers: { Cookie: "key=value; Path=/\xAD" } }

Check failure on line 54 in spec/requests/sanitiser_spec.rb

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

Layout/ExtraSpacing: Unnecessary spacing detected.
.to raise_error(ArgumentError, "invalid byte sequence in UTF-8")
end
end

context "with cookie path being correct percent-encoded UTF-8" do
it "does not raise error" do
get "/", headers: { Cookie: "key=value; Path=/%41" }
expect(response).to have_http_status(:ok)
end
end

context "with cookie path being incorrect percent-encoded UTF-8" do
it "raises Sanitiser::Strategy::SanitisingError" do
expect { get "/", headers: { Cookie: "key=value; Path=/%AD" } }

Check failure on line 68 in spec/requests/sanitiser_spec.rb

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

Layout/ExtraSpacing: Unnecessary spacing detected.
.to raise_error(Sanitiser::Strategy::SanitisingError)
end
end

context "with referrer header being correct percent-encoded UTF-8" do
it "does not raise error" do
get "/", headers: { Referer: "http://example.com/?%41" }
expect(response).to have_http_status(:ok)
end
end

context "with referrer header being incorrect percent-encoded UTF-8" do
it "does not raise error" do
get "/", headers: { Referer: "http://example.com/?%AD" }
expect(response).to have_http_status(:ok)
end
end
end

0 comments on commit 37135b7

Please sign in to comment.