oEmbed automatically lowercases scheme urls making it impossible to use schemes that should have capital letters in them. It also makes it impossible to accept any urls with capital letters in them (when these capital letters are part of the scheme).
Drupal relies on https://oembed.com/providers.json to indicate the possible schemes for streaming video urls from various services. Vimeo has the scheme "https:\/\/player.vimeo.com\/video\/*" etc. You can also implement your own oEmbed providers.json.
The bug is that the oEmbed Endpoint class lowercases the schemes automatically, but compares them against a non-lowercased user input with a case sensitive regex. This means that any capital letters in the scheme will cause the regex to fail, making it impossible to create your own scheme that has capital letters.
I discovered the bug while creating a custom oEmbed provider for Panopto (academia video streaming service). The scheme looks like this:
"https:\/\/demo.hosted.panopto.com\/Panopto\/Pages\/Viewer.aspx*"
As you can see, there are uppercase words in the url scheme and lowercasing the scheme automatically and then comparing it case sensitively to a Panopto supplied video url will always fail.
The offending code is in Drupal\media\OEmbed\Endpoint.php on line #72 (array map of schemes to mb_strtolower) and in the method matchUrl (line #155) where the case sensitive regex is made.
Unfortunately this is a chore to reproduce, since there are currently no official oembed schemes with capital letters.
- You need to implement your own @MediaSource class that extends OEmbed.
- You also need to implement your own oEmbed providers.json and configure a provider (such as Panopto) with a variable case scheme.
- You also need to override media.settings for your custom oEmbed providers.json so Drupal reads it instead of the canonical one.
- Don't forget to clear your cache to load the new oEmbed providers.
- Now add your own media type (structure / media types / add) using the MediaSource class you just implemented.
- In the case of Panopto, which doesn't have its own oEmbed endpoint, we have to simulate / implement one ourselves in Drupal.
- Now try to add a Panopto video using the example above. It will never be accepted, because Drupal lowercases your scheme and compares it against your Panopto url with capital letters in it.
I expect the media entity to be accepted when its url matches my oEmbed provider scheme even if the scheme has capital letters in it.
The media is rejected with the error message "The provided URL does not represent a valid oEmbed resource.".
I can think of two easy ways:
- Change the regex to be case insensitive in Endpoint.php #155 OR
- Remove the lowercasing of schemes in Endpoint.php #72
At first glance, the latter seems the more appropriate one. We should not lowercase schemes if there is a chance a valid url (such as in the case of Panopto) has capital letters in it.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 3054888-10-FAIL.patch | 794 bytes | phenaproxima |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !1043
Comments
Comment #2
jcalais commentedComment #3
jcalais commentedJust a quick patch that removes the lowercasing of schemes.
Comment #4
cilefen commentedComment #7
jay.dansand commented+1 for RTBC. I just found this issue as I was about to submit my own similar patch. It still applies cleanly as of Drupal 9.1.5, so I'm bumping the issue's Drupal version to the now-active version. Thanks for your work!
Adding my 2 cents for the pro-patch argument: There's no logical reason to force endpoint URIs (the things site builders can actually control) to be lower-cased before comparing against the user's input (the thing we cannot control). If anything, assume the user may error when entering the URI (an argument for a case-insensitive match), not the other way around.
Either we want a case-insensitive comparison or a strict match, and the current method accomplishes neither while breaking endpoints with mixed-case URIs (the problem I just hit up against). Maintaining the current strict (but currently broken) match seems the least invasive step, so let's go for it!
Comment #8
chris matthews commentedComment #9
phenaproxima🤔 Yeah, I'm not sure why we lowercased the schemes in the Endpoint constructor. I wish we'd put a comment there to explain the reasoning. Since we don't have that, I'm okay with removing the mb_strtolower() call.
This is going to need an automated test in any event; I think we could easily do this by adding a simple unit test for Endpoint (a test class already exists at https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/modules/medi..., so we could modify that).
Comment #10
phenaproximaWrote a test. Here's a fail patch to prove it catches the bug. I'm pushing that, plus the fix in #3, to the merge request.
Comment #12
phenaproximaRe-titling for clarity.
Comment #14
phenaproximaFail patch failed as expected.
Comment #15
lendudeWill removing mb_strtolower() give a BC break if sites currently depend on this happening?
Comment #16
phenaproximaReasons I'm not worried about BC in this case:
Right now in HEAD, if someone tried to match a mixed-case URL (
httpS://TWiTTer.com/OembeD/...or some other goofy thing) against a legitimate lowercased endpoint, it would fail anyway since the regex is case-sensitive.So thank you for asking that question, @Lendude. 🙏 Maybe I'm missing something, but I don't really see any realistic BC break here.
Comment #17
lendude@phenaproxima excellent, in that case this looks ready to me.
Comment #18
catchIf we wanted to restrict to lowercase I think we'd want to add validation somewhere, since we don't have that seems fine to remove the normalisation.
Leaving this as 9.3.x-only though, just in case we have missed something.