GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Remove source expression deduplication. by lgarron · Pull Request #499 · github/secure_headers

Conversation

@lgarron

@lgarron lgarron commented Oct 19, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This PR removes dedup_source_list and replaces it with a simple .uniq call. This resolves #491, which is only the latest in a series of ongoing issues with source expression deduplication.

secure_headers has had this feature since 2015 that deduplicates redundant URL source expressions. For example, if *.github.com is listed as a source expression for a given directive, then the addition of example.github.com would have no effect, and so the latter can be safely removed by secure_headers to save bytes.

Unfortunately, this implementation has had various bugs due to the use of "impedance mismatched" APIs like URI1 and File.fnmatch2. For example, it made incorrect assumptions about source expression schemes, leading to the following series of events:

  • 2017-03: A bug was reported and confirmed
  • 2022-04: The bug was finally fixed by @keithamus (a Hubber) in 2022 due to our use of web sockets.
  • 2022-06: This fix in turn triggered a new bug with source expressions like data:.
  • 2022-06: An external contributor submitted a fix for the bew bug, but this still doesn't address some of the "fast and loose" semantic issues of the underlying implementation.
  • 2022-08: @lgarron drafted a new implementation that semantically parses and compares source expressions based on the specification for source expressions.
    • This implementation already proved to have some value in early testing, as its stricter validation caught an issue in github.com's CSP. However, it would take additional work to make this implementation fully aware of CSP syntax (e.g. not allowing URL source expressions in a source directive when only special keywords are allowed, and vice-versa), and it relies on a new regex-based implementation of source expression parsing that may very well lead to more subtle bugs.

In effect, this is a half feature whose maintenance cost has outweighed its functionality:

  • The relevant code has suffered from continued bugs, described as above.
  • Deduplication is purely a "nice-to-have" — it is not necessary for the security or correct functionality of secure_headers.
  • It was introduced by @oreoshake (the then-maintainer) without explanation in 2015, never "officially" documented. We have no concrete data on whether it has any performance impact on any real apps — for all we know, uncached deduplication calculations might even cost more than the saved header bytes.
  • Further, in response to the first relevant bug, @oreoshake himself said:

I've never been a fan of the deduplication based on * anyways. Maybe we should just rip that out.

Like people trying to save a few bytes can optimize elsewhere.

So this PR completely removes the functionality. If we learn of a use case where this was very important (and the app somehow can't preprocess the list before passing it to secure_headers), we can always resume consideration of one of:

  • fix source dedup breaking with port wildcards #490
  • Semantically parse and deduplicate source expressions #498

Footnotes

  1. Which allows wildcards in domains but not for ports, as it is not designed to parse URL source expressions. ↩

  2. Which has general glob matching that is not designed for URL source expressions either. ↩

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
vcsjones
vcsjones approved these changes Oct 21, 2022
View reviewed changes
@lgarron lgarron mentioned this pull request Oct 24, 2022
CI changes: run on push, drop Ruby 2.5 #500
Merged
lgarron added 3 commits October 24, 2022 11:01
@lgarron
Add a regression test for a current bug.
af92ce6
@lgarron
Remove dedup_source_list and replace with a "best-effort" .uniq c…
cdb40e5
…all.
@lgarron
Update tests.
cd73b02
@lgarron
lgarron force-pushed the lgarron/remove-source-expression-dedup branch from 8097494 to 70af13b Compare October 24, 2022 18:01
@lgarron
lgarron changed the base branch from main to lgarron/update-ci October 24, 2022 18:01
@lgarron
lgarron force-pushed the lgarron/remove-source-expression-dedup branch from 70af13b to cd73b02 Compare October 24, 2022 18:01
KyFaSt
KyFaSt approved these changes Oct 24, 2022
View reviewed changes
Base automatically changed from lgarron/update-ci to main October 24, 2022 19:01
lgarron added a commit that referenced this pull request Oct 24, 2022
@lgarron
CI changes: run on push, drop Ruby 2.5 (#500)
a488b4d
Ruby 2.5 has been failing on CI since
#499 and is no longer
supported.
@lgarron
lgarron merged commit b6ef2ed into main Oct 24, 2022
@lgarron
lgarron deleted the lgarron/remove-source-expression-dedup branch October 24, 2022 19:03
lgarron added a commit that referenced this pull request Oct 24, 2022
@lgarron
v6.5.0
5b12e41
Release notes:

- CSP: Remove source expression deduplication. (@lgarron) #499
@lgarron lgarron mentioned this pull request Oct 24, 2022
v6.5.0 #501
Merged
lgarron added a commit that referenced this pull request Oct 24, 2022
@lgarron
v6.5.0
3adb9ba
Release notes:

- CSP: Remove source expression deduplication. (@lgarron) #499
lgarron added a commit that referenced this pull request Oct 24, 2022
@lgarron
v6.5.0 (#501)
0678a74
Release notes:

- CSP: Remove source expression deduplication. (@lgarron)
#499
This was referenced Oct 25, 2022
fix source dedup breaking with port wildcards #490
Closed
Semantically parse and deduplicate source expressions #498
Closed
@lgarron lgarron mentioned this pull request Jan 3, 2023
Source Deduplication Doesn't Take Schemes into Account #317
Closed
fletchto99 added a commit that referenced this pull request Dec 16, 2025
@fletchto99
Fix tests (sources are no longer minified since #499)
a4a4b1b
denn-os pushed a commit that referenced this pull request Dec 16, 2025
@fletchto99 @denn-os
Fix tests (sources are no longer minified since #499)
407d8f8
denn-os pushed a commit that referenced this pull request Dec 16, 2025
@fletchto99 @denn-os
Fix tests (sources are no longer minified since #499)
fa4298e
denn-os pushed a commit that referenced this pull request Dec 17, 2025
@fletchto99 @denn-os
Fix tests (sources are no longer minified since #499)
7f19fb2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vcsjones vcsjones vcsjones approved these changes
@KyFaSt KyFaSt KyFaSt approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

URI::InvalidURIError: Invalid data URI

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@lgarron @vcsjones @KyFaSt
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.