OEmbed providers: Tighten up resource URL matching for providers #21583
+1,304
−46
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndyButland
changed the title
AndyButland
force-pushed
the
v17/fix-bug/validation-oembed
branch
from
c17821d to
20db9a8
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances security for OEmbed providers by tightening URL validation to prevent Server-Side Request Forgery (SSRF) attacks. The changes ensure that resource URLs are matched only on legitimate provider domains, not paths within untrusted domains.
Changes:
- Updated all OEmbed provider regex patterns to use anchored patterns starting with
^https?:\/\/, preventing path-based SSRF attacks - Added comprehensive test coverage for all providers with tests for valid URLs, SSRF protection, and unrelated URL rejection
- Refactored some providers to use generated regex for performance and extracted markup building logic for testability
- Removed unused
using System.Xml;statements from providers that don't directly use XML classes
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| OEmbedProviderTestBase.cs | New base class for provider tests that uses OEmbedService.MatchesUrlScheme |
| YouTubeEmbedProviderTests.cs | Tests for YouTube URL validation including SSRF protection |
| XEmbedProviderTests.cs | Tests for X/Twitter URL validation including SSRF protection |
| VimeoEmbedProviderTests.cs | Tests for Vimeo URL validation including SSRF protection |
| TedEmbedProviderTests.cs | Tests for TED URL validation including SSRF protection |
| SoundCloudEmbedProviderTests.cs | Tests for SoundCloud URL validation including SSRF protection |
| SlideshareEmbedProviderTests.cs | Tests for Slideshare URL validation including SSRF protection |
| LottieFilesEmbedProviderTests.cs | Tests for LottieFiles URL validation and markup building logic |
| KickstarterEmbedProviderTests.cs | Tests for Kickstarter URL validation including SSRF protection |
| IssuuEmbedProviderTests.cs | Tests for Issuu URL validation including SSRF protection |
| HuluEmbedProviderTests.cs | Tests for Hulu URL validation including SSRF protection |
| GiphyEmbedProviderTests.cs | Tests for Giphy URL validation including SSRF protection |
| GettyImagesEmbedProviderTests.cs | Tests for Getty Images URL validation including SSRF protection |
| FlickrEmbedProviderTests.cs | Tests for Flickr URL validation and markup building logic |
| DailyMotionEmbedProviderTests.cs | Tests for DailyMotion URL validation including SSRF protection |
| OEmbedService.cs | Extracted MatchesUrlScheme method as internal static for testability |
| Youtube.cs | Updated regex patterns to use anchored domain matching, added XML documentation |
| X.cs | Updated regex to use anchored domain matching, modern collection syntax, added XML documentation |
| Vimeo.cs | Updated regex to use anchored domain matching, removed unused using, added XML documentation |
| Ted.cs | Updated regex to use anchored domain matching, removed unused using, added XML documentation |
| SoundCloud.cs | Updated regex to use anchored domain matching, removed unused using, added XML documentation |
| Slideshare.cs | Updated regex to use anchored domain matching, removed unused using, added XML documentation |
| LottieFiles.cs | Updated regex to use anchored domain matching, extracted BuildMarkup for testing, uses generated regex, added XML documentation |
| Kickstarter.cs | Updated regex to use anchored domain matching, modern collection syntax, added XML documentation |
| Issuu.cs | Updated regex to use anchored domain matching, removed unused using, added XML documentation |
| Hulu.cs | Updated regex to use anchored domain matching, modern collection syntax, added XML documentation |
| Giphy.cs | Updated regex to use anchored domain matching, modern collection syntax, added XML documentation |
| GettyImages.cs | Updated regex to use anchored domain matching, modern collection syntax, added XML documentation |
| Flickr.cs | Updated regex to use anchored domain matching (removed flic.kr support), changed API endpoint to HTTPS, extracted BuildMarkup for testing, added XML documentation |
| DailyMotion.cs | Updated regex to use anchored domain matching, removed unused using, added XML documentation |
| UdiGetterExtensionsTests.cs | Added fully qualified type name to resolve ambiguity caused by new test file namespaces |
tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/UdiGetterExtensionsTests.cs
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This PR tightens up the validation on the matching of resource URLs to OEmbed providers, ensuring we only match on domains and not paths, ensuring only legitimate provider domains are matched.
We've also improved test coverage of these embed providers, adding tests for the URL matching logic as well as the building of markup for those providers with significant logic. Finally, some small optimisations were applied.
Testing
The unit tests should cover changes across the providers, so manual test of embedding into the RTE of a couple of providers should suffice.