Skip to content
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

normalize domains with trailing slashes #477

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 1 commit
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
14 changes: 14 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,19 @@ 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 accomplisheg the same.
keithamus marked this conversation as resolved.
Show resolved Hide resolved
# https://www.w3.org/TR/CSP3/#match-paths § 6.6.2.10 Step 2
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
Member 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/ c.example.net/foo/ b.example.co/bar)
}
csp = ContentSecurityPolicy.new(config)
expect(csp.value).to eq("default-src a.example.org b.example.com c.example.net/foo/ b.example.co/bar")
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