fixed formatting a file:// URI

[vividos]

add the two forward slashes even when the authority part of the URI is empty
do this only for "file" scheme, to not break other schemes like mailto: or tel:
see RFC 3986 for details

part of a fix for #8665

[cesium-concierge]

Thank you so much for the pull request @vividos! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

• ❌ Missing CONTRIBUTORS.md entry.
  • Please add yourself to the contributors file!
• ❌ Missing CLA.
  • Please send in a Contributor License Agreement (CLA) and comment back here to let us know to check this!
• ❔ CHANGES.md was not updated.
  • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
• ❔ Changes to third party files were made.
  • Looks like a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version.
• ❔ Unit tests were not updated.
  • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

• Cesium Viewer works.
• Works in 2D/CV.
• Works (or fails gracefully) in IE11.

[vividos]

I already signed the CLA, hope the PR updates itself...

[vividos]

I rebased the changes on current master branch

[vividos]

@OmarShehata can you take a look at this? Thanks!

[vividos]

Rebased on latest master

[OmarShehata]

@vividos I'm planning on testing this and giving it a fuller review by Monday but for now I left a quick style comment.

[OmarShehata]

So I wasn't actually able to verify that this fixes the Cesium ion credit image when building with Cordova and running on my Pixel 3.

I set my Cesium base url to window.CESIUM_BASE_URL = 'file:///android_asset/www/CesiumUnminified/';. I confirmed the new code is running by verifying that window.alert(new Cesium.Uri('file:///android_asset/').toString()) produced file:///. Regardless of whether the fix was applied I saw:

• The Cesium ion credit at the bottom always had a blank src
• The imagery base layer picker at the top right always had a src of file:///android_asset/www/CesiumUnminified/Widgets/Images/ImageryProviders/bingAerial.png.

@vividos did you have a different setup than what I described here?

I'm not sure why the Cesium ion credit has a blank src, but this fix to Uri.toString seems to produce the correct behavior from my reading of RFC 3986 - that authority is optional, but if we always appended // regardless of whether authority was present or not, that would produce incorrect behavior for urls with a mailto scheme.

This is also consistent with the behavior in URI.js. Here's how they do it:

https://github.com/medialize/URI.js/blob/73c10412ce97b760d1cb143080bee25e99d12f5b/src/URI.js#L670-L700

[vividos]

Omar, I didn't have a chance to test the fix in my own app. I just tried to come up with a fix for my Sandcastle example in the original issue #8665. Is there a minified version of Cesium.js from this PR? Then I could drop this into my app and also test. My guess is that it would behave like your Cordova test, though.

About other URIs: I thought about this and also read RFC 3986. With this PR, the mailto: and other URIs should be unaffected by the fix, and only "file" URIs are adjusted (notice the extra "if" in the else part).

About fixing the image: In my app it seems this is the only instance where local resources couldn't be resolved. I have no problems referencing other images (as you also suggested) and referencing the Service Workers, which I guess also use the CESIUM_BASE_URL. From reading

cesium/Source/Scene/CreditDisplay.js

Line 490 in cbc0909

var logo = buildModuleUrl('Assets/Images/ion-credit.png');

I couldn't think of a way how the resulting image would be empty...

[vividos]

I just tested with my Android app, an unminified Cesium 1.67 version, my patched URI.toString() and some console.log() lines that the logo variable in CreditDisplay.js line 490 (see above) is correctly formatted. So the fix is valid.

The icon doesn't show up, though. Investigating with more console.log calls...

[vividos]

I think I found the reason why the <img> tag has no src attribute:
https://github.com/CesiumGS/cesium/blob/master/Source/Core/Credit.js#L93
This line tries to sanitize the Credit's _html property, which is accessed from this line:
https://github.com/CesiumGS/cesium/blob/master/Source/Scene/CreditDisplay.js#L39

Unfortunately I don't know why it's working in the browser, but not in the Android WebView...

[vividos]

@OmarShehata I can confirm that replacing this line fixes showing the image on Android:

cesium/Source/Core/Credit.js

Line 93 in 5a4a66a

var html = DOMPurify.sanitize(this._html);

(replaced with: var html = this._html)

I also tested on Windows Firefox, and the img tag is also replaced there when accessing element. My guess is that at this time the Ion credit is already added to the DOM and the DOM elements are not replaced by the desktop browser.

What should we do with this PR? I would like to have it in 1.68, even when it doesn't fix #8665 yet. Thanks!

[vividos]

It's definitely DOMpurify, it strips out all src attributes that don't contain http or https links. I think I will solve the original ticket by manually adding the Ion credit string without a full path. CreditDisplay should recognize it as Ion credit, not showing the other Cesium credit, and DOMpurify will not sanitize a relative link.

[OmarShehata]

@vividos thank you so much for investigating and resolving this! Can you please add or link to your conclusions here in the original issue #8665 ? So someone can either suggest or implement a fix based on your findings. There may be other areas in the code that don't correctly handle links without http or https.

[OmarShehata]

For what to do with this PR, @mramato the behavior for parsing URIs with file:/// (three slashes, no authority or hostname) is incorrect in master, and is fixed in this PR. Sandcastle with test case here #8665 (comment).

I'm not aware of a way to fix this without modifying the third party code, since ThirdParty/Uri.js incorrectly skips the // when the authority and hostname are blank. I would merge this after unit tests are added for it but wanted to get a second opinion here.

[vividos]

Omar, I will comment in the initial issue. Thanks!

[vividos]

@OmarShehata I also rebased this PR on top of master, but it isn't strictly necessary to fix #8665, now that I have PR #8758 in place. What should we do with this PR?

[vividos]

I'm closing this PR, as my initial issue #8665 is fixed.