Skip to content

normalize domains with trailing slashes #477

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.

Sign up for GitHub

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
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

normalize domains with trailing slashes #477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d44152
normalize domains with trailing slashes
keithamus Mar 30, 2022
6ffcee7
fix: allow URIs with schema to have trailing slashes normalised
keithamus Apr 4, 2022
67ca492
fix typo
keithamus Apr 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions lib/secure_headers/headers/content_security_policy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ def minify_source_list(directive, source_list)
else
source_list = populate_nonces(directive, source_list)
source_list = reject_all_values_if_none(source_list)
source_list = normalize_uri_paths(source_list)

unless directive == REPORT_URI || @preserve_schemes
source_list = strip_source_schemes(source_list)
Expand All @@ -147,6 +148,27 @@ def reject_all_values_if_none(source_list)
end
end

def normalize_uri_paths(source_list)
source_list.map do |source|
# Normalize domains ending in a single / as without omitting the slash accomplishes the same.
# https://www.w3.org/TR/CSP3/#match-paths § 6.6.2.10 Step 2
begin
uri = URI(source)
if uri.path == "/"
next source.chomp("/")
end
rescue URI::InvalidURIError
Comment on lines +156 to +160
Copy link
Contributor

Choose a reason for hiding this comment

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

@keithamus I know you wrote this code several years ago, but do you remember what edge case we are looking catch here?

It seems like we might be able to get away with something like:

source_list.map do |source|
    source.chomp("/")
end

I believe .chomp will either remove the last char if it exists or return the original string unmodified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t think that’s sufficient as it shouldn’t always remove the trailing / - only when the entire path is just /. For example “example.com/foo/“ should remain as is, but “example.com/“ should be trimmed so it becomes “example.com”.

The logic could be described as: if the entire contents of the URL after the domain name are “/“ then that is the equivalent of a URL with no path (so the contents after the domain name are “”).

end

if source.chomp("/").include?("/")
Copy link
Member

Choose a reason for hiding this comment

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

One thing this might not be normalizing is a CSP directive like https://www.example.com/ since the protocol includes /.

The quoted spec is about matching on the path-part, not the entire URL that is in the directive.

It would be good to address this, with unit tests as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a test and some code to fix this. The code doesn't feel great, I'd welcome more elegant solutions here!

source
else
source.chomp("/")
end
end
end


# Removes duplicates and sources that already match an existing wild card.
#
# e.g. *.github.com asdf.github.com becomes *.github.com
Expand Down
10 changes: 9 additions & 1 deletion spec/lib/secure_headers/headers/content_security_policy_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,17 @@ module SecureHeaders
expect(csp.value).to eq("default-src * 'unsafe-inline' 'unsafe-eval' data: blob:")
end

it "normalizes source expressions that end with a trailing /" do
config = {
default_src: %w(a.example.org/ b.example.com/ wss://c.example.com/ c.example.net/foo/ b.example.co/bar wss://b.example.co/)
}
csp = ContentSecurityPolicy.new(config)
expect(csp.value).to eq("default-src a.example.org b.example.com wss://c.example.com c.example.net/foo/ b.example.co/bar wss://b.example.co")
end

it "minifies source expressions based on overlapping wildcards" do
config = {
default_src: %w(a.example.org b.example.org *.example.org https://*.example.org)
default_src: %w(a.example.org b.example.org *.example.org https://*.example.org c.example.org/)
}
csp = ContentSecurityPolicy.new(config)
expect(csp.value).to eq("default-src *.example.org")
Expand Down