Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct identification of uri scheme according to rfc3986 #8832

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

vzhukovs
Copy link
Contributor

@vzhukovs vzhukovs commented Dec 7, 2020

What it does

This changes proposal fix identification of uri scheme. The problem is in regular expression, that matches uri scheme, it is trying to find scheme according to pattern \w+ which matches any word character (alphanumeric and underscore).

According to https://tools.ietf.org/html/rfc3986#section-3.1 underscore cannot be used as part of uri scheme:

Scheme names consist of a sequence of characters beginning with a
letter and followed by any combination of letters, digits, plus
("+"), period ("."), or hyphen ("-").

Proposed changes is to align matching to rfc specification and use regular expression: /^\/([a-zA-Z0-9.\-+]+)\/(.+)$/ instead of /^\/(\w+)\/(.+)$/.

This might be helpful for extensions, that provides custom resource scheme instead of vscode-resource://. Especially when extension that provides webview is located in different container/user machine and content to the remote resource is given by provided scheme.

Signed-off-by: Vladyslav Zhukovskyi [email protected]

How to test

To check this Theia has to be run in multi-container environment, like OpenShift: create and run workspace.
This workspace configuration also provides an extension that runs in dedicate container: redhat.vscode-didact.
After workspace start try to open any sample didact tutorial, resources from this webview (js, css) will be loaded using different uri scheme and webview will be displayed the same way as it runs locally on single node environment.

Review checklist

Reminder for reviewers

@vzhukovs vzhukovs added bug bugs found in the application webviews issues related to webviews labels Dec 7, 2020
@vzhukovs vzhukovs self-assigned this Dec 7, 2020
@paul-marechal
Copy link
Member

Looks good to me, can you just post an example of URI that used to trigger the old regex when it should have not?

@vzhukovs
Copy link
Contributor Author

Hi @marechal-p, sure, sorry for late response.

Here is the flow how to test this PR:

How does it work in VS Code:
vscode

How does it work from current PR:
theia-branch

How does it work from master branch:
theia-master

Sample extension provides two subjects:

  • Sample In Memory File System Provider with scheme file-sidecar-memfs:/. This File System Provider already contains the file called styles.css with css content. It can be accessed by URI: file-sidecar-memfs:/styles.css. This file is included into webview html content.
  • Sample webview with included above styles.

When we change file scheme from file-sidecar-memfs:/ to file_sidecar_memfs:/ which is wrong according to RFC specification, we'll get the following behavior, which now works in master branch:

Example of VS Code:
vscode_2

Example of Theia from current PR:
theia_branch_2

Example of Theia from master branch:
theia_master_2

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@vzhukovs I verified the changes given the example plugin and it worked correctly 👍


I verified that:

master: incorrectly displays the codicon webview:

image

pull-request: the codicon webview is correctly displayed, like vscode:

image

@vzhukovs vzhukovs merged commit 742c0da into master Dec 15, 2020
@vzhukovs vzhukovs deleted the uri_scheme_specification branch December 15, 2020 08:04
@github-actions github-actions bot added this to the 1.9.0 milestone Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application webviews issues related to webviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants