Skip to content

CSP: Port upgrade matching should only apply to insecure source schemes#62322

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
roberto-apple:eng/308747
Apr 13, 2026
Merged

CSP: Port upgrade matching should only apply to insecure source schemes#62322
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
roberto-apple:eng/308747

Conversation

@roberto-apple
Copy link
Copy Markdown
Contributor

@roberto-apple roberto-apple commented Apr 9, 2026

c82619c

CSP: Port upgrade matching should only apply to insecure source schemes
https://bugs.webkit.org/show_bug.cgi?id=308747
rdar://171265255

Reviewed by Brent Fulgham.

A CSP source expression such as "frame-src https://host:80" incorrectly
matches https://host (port 443). The mismatch (80 ≠ 443) should cause
a block, but WebKit has scheme upgrade logic that treats default ports
as equivalent during http-to-https transitions. This logic does not
verify the source scheme is insecure, so it treats any source with
port 80 as upgradable — even when the source scheme is already HTTPS.

Have schemeMatches() return a SchemeMatchResult enum that distinguishes
exact matches from insecure-to-secure upgrades, and pass this to
portMatches() so the upgrade path only fires when a scheme upgrade is
actually occurring. This also fixes an edge case with schemeless source
expressions. Both functions are annotated with CSP3 spec step references.

* LayoutTests/http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number-expected.txt:
* LayoutTests/http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number.html:
* LayoutTests/platform/wk2/TestExpectations:
* Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:
(WebCore::ContentSecurityPolicySource::portMatches const):

Canonical link: https://commits.webkit.org/311148@main

9692000

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe 🛠 win ⏳ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 🧪 win-tests ⏳ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ⏳ 🛠 vision-apple
✅ 🧪 ios-wk2-wpt ✅ 🧪 api-mac-debug ✅ 🛠 gtk3-libwebrtc
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 ios-safer-cpp ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🛠 playstation
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@roberto-apple roberto-apple self-assigned this Apr 9, 2026
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 9, 2026
@roberto-apple roberto-apple removed the merging-blocked Applied to prevent a change from being merged label Apr 9, 2026
@roberto-apple roberto-apple changed the title CSP: Fix port upgrade logic matching https://host:80 against https://host CSP: Port upgrade matching should only apply to insecure source schemes Apr 9, 2026
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 9, 2026
@roberto-apple roberto-apple removed the merging-blocked Applied to prevent a change from being merged label Apr 10, 2026
@roberto-apple
Copy link
Copy Markdown
Contributor Author

The original reporter identified two cases where CSP source expressions with cross-scheme default ports were unexpectedly allowing resource loads:

  1. frame-src https://localhost:80 matching https://localhost/run.html
  2. frame-src http://localhost:443 matching https://localhost/run.html

After investigation, only case 1 is a bug. The source expression already specifies https, so there is no insecure-to-secure scheme upgrade involved. Since both the source expression and resource urls have an https scheme, port 80 should simply not match port 443. This has been fixed and the iframe is now correctly blocked.

Case 2 is correct behavior per spec. The source expression http scheme upgrades to https (§6.7.2.9 step 1.2), and port 443 matches the resource url's default port (443 for https) (§6.7.2.11 step 5). No change needed.

@roberto-apple roberto-apple marked this pull request as ready for review April 10, 2026 22:49
@roberto-apple roberto-apple requested a review from cdumez as a code owner April 10, 2026 22:49
@roberto-apple roberto-apple requested review from annevk and rreno April 10, 2026 22:49
Copy link
Copy Markdown
Contributor

@brentfulgham brentfulgham left a comment

Choose a reason for hiding this comment

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

This is a great change. Thank you for tackling it. I had a few minor nits about variable names and the source of the test cases, but otherwise looks great!

}

bool ContentSecurityPolicySource::portMatches(const URL& url) const
bool ContentSecurityPolicySource::portMatches(const URL& url, bool isSchemeUpgrade) const
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm confused by the name isSchemeUpgrade. Is this a directive that we should upgrade any relevant scheme before comparing? Or does it mean the url being passed in has already BEEN upgraded? Or something else?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, we try to avoid bool arguments in favor of class enums.

["yes", "script-src 127.0.0.1:8000", "https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js"],

// Tests that HTTPS URL does not match WSS source expression with HTTP default port (secure scheme, no port upgrade).
["no", "script-src wss://127.0.0.1:8000", "https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js"],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this block of new tests something we're merging from upstream WPT? Or did you design these tests yourself?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Custom tests. This bug involves default-port upgrade logic (port 80 vs 443), which requires internals.registerDefaultPortForProtocol() to test for which AFAICT WPT doesn't have an equivalent. The closest WPT test (embedded-enforcement/subsumption_algorithm-host_sources-ports.html) cover the subsumption algorithm, not source matching.

Copy link
Copy Markdown
Contributor

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Also concerned about these not being WPTs.

auto& scheme = m_scheme.isEmpty() ? m_policy->selfProtocol() : m_scheme;
auto urlScheme = url.protocol();
// A is the source expression's scheme-part (m_scheme, or selfProtocol if empty).
// B is the URL's scheme being checked against the policy.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don’t think these comments add much.

}

bool ContentSecurityPolicySource::portMatches(const URL& url) const
bool ContentSecurityPolicySource::portMatches(const URL& url, bool isSchemeUpgrade) const
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, we try to avoid bool arguments in favor of class enums.

// A is the source expression's scheme-part (m_scheme, or selfProtocol if empty).
// B is the URL's scheme being checked against the policy.
const auto& scheme = m_scheme.isEmpty() ? m_policy->selfProtocol() : m_scheme;
const auto urlScheme = url.protocol();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think our typical style is to not use const for local variables

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also concerned about these not being WPTs.

Testing this bug requires overriding default port mappings via internals.registerDefaultPortForProtocol(), which AFAICT WPT can't do. Maybe there are WPT test servers that run on ports 80/443? Otherwise I'm not sure about feasibility for this to be WPT.

@roberto-apple roberto-apple added the merge-queue Applied to send a pull request to merge-queue label Apr 13, 2026
https://bugs.webkit.org/show_bug.cgi?id=308747
rdar://171265255

Reviewed by Brent Fulgham.

A CSP source expression such as "frame-src https://host:80" incorrectly
matches https://host (port 443). The mismatch (80 ≠ 443) should cause
a block, but WebKit has scheme upgrade logic that treats default ports
as equivalent during http-to-https transitions. This logic does not
verify the source scheme is insecure, so it treats any source with
port 80 as upgradable — even when the source scheme is already HTTPS.

Have schemeMatches() return a SchemeMatchResult enum that distinguishes
exact matches from insecure-to-secure upgrades, and pass this to
portMatches() so the upgrade path only fires when a scheme upgrade is
actually occurring. This also fixes an edge case with schemeless source
expressions. Both functions are annotated with CSP3 spec step references.

* LayoutTests/http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number-expected.txt:
* LayoutTests/http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number.html:
* LayoutTests/platform/wk2/TestExpectations:
* Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:
(WebCore::ContentSecurityPolicySource::portMatches const):

Canonical link: https://commits.webkit.org/311148@main
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 311148@main (c82619c): https://commits.webkit.org/311148@main

Reviewed commits have been landed. Closing PR #62322 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit c82619c into WebKit:main Apr 13, 2026
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants