Skip to content

Viewer fix: invalid html_widget resource paths #739

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.

Sign up for GitHub

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

Jump to bottom
Merged
renkun-ken merged 13 commits into from
Aug 4, 2021
Merged

Viewer fix: invalid html_widget resource paths #739

renkun-ken merged 13 commits into from
Aug 4, 2021

Conversation

@ElianHugh
Copy link
Collaborator

@ElianHugh ElianHugh commented Aug 1, 2021

Resolves #665

Adds a mutation observer to the webview body, which watches for changes in the DOM. Specifically, the filescheme vscode-webview:// is tested for 'URLness' and (if true) is converted to an https:// request.

This PR also defines some (minimal) content security policies for the webview.

Screenshot

image

How can I check this pull request?

Prior to the PR, the following led to a blank viewer page, which should now be fixed:

library(leaflet)
leaflet() |> addTiles()

Dynamic loading of resources is supported too:

library(mapview)
mapview(breweries)

@renkun-ken
Copy link
Member

renkun-ken commented Aug 1, 2021

Thanks for working on this! Works nicely already.

When I test the mapview example, it actually works, but I found the following error in the log:

fgb.js:103 Refused to connect to 'vscode-webview://8fe576af-ef9f-4237-9417-cd8966ff7eef/lib/breweries-0.0.1/breweries_layer.fgb' because it violates the following Content Security Policy directive: "default-src https: data: filesystem:". Note that 'connect-src' was not explicitly set, so 'default-src' is used as a fallback.

LeafletWidget.methods.addFlatGeoBuf @ fgb.js:103
fgb.js:103 Refused to connect to 'vscode-webview://8fe576af-ef9f-4237-9417-cd8966ff7eef/lib/breweries-0.0.1/breweries_layer.fgb' because it violates the document's Content Security Policy.
LeafletWidget.methods.addFlatGeoBuf @ fgb.js:103
fgb.js:103 Uncaught (in promise) TypeError: Failed to fetch
    at e.LeafletWidget.methods.addFlatGeoBuf (fgb.js:103)
    at Object.doRenderValue (leaflet.js:812)
    at Object.renderValue (leaflet.js:714)
    at Object.renderValue (htmlwidgets.js:886)
    at htmlwidgets.js:653
    at Array.forEach (<anonymous>)
    at forEach (htmlwidgets.js:55)
    at htmlwidgets.js:576
    at Array.forEach (<anonymous>)
    at forEach (htmlwidgets.js:55)

Not sure if it has any impact on the functionality. Anything we should do about this or we could ignore it?

renkun-ken
renkun-ken reviewed Aug 1, 2021
View reviewed changes
@ElianHugh
Copy link
Collaborator Author

ElianHugh commented Aug 1, 2021

Thanks for working on this! Works nicely already.

When I test the mapview example, it actually works, but I found the following error in the log:

fgb.js:103 Refused to connect to 'vscode-webview://8fe576af-ef9f-4237-9417-cd8966ff7eef/lib/breweries-0.0.1/breweries_layer.fgb' because it violates the following Content Security Policy directive: "default-src https: data: filesystem:". Note that 'connect-src' was not explicitly set, so 'default-src' is used as a fallback.

LeafletWidget.methods.addFlatGeoBuf @ fgb.js:103
fgb.js:103 Refused to connect to 'vscode-webview://8fe576af-ef9f-4237-9417-cd8966ff7eef/lib/breweries-0.0.1/breweries_layer.fgb' because it violates the document's Content Security Policy.
LeafletWidget.methods.addFlatGeoBuf @ fgb.js:103
fgb.js:103 Uncaught (in promise) TypeError: Failed to fetch
    at e.LeafletWidget.methods.addFlatGeoBuf (fgb.js:103)
    at Object.doRenderValue (leaflet.js:812)
    at Object.renderValue (leaflet.js:714)
    at Object.renderValue (htmlwidgets.js:886)
    at htmlwidgets.js:653
    at Array.forEach (<anonymous>)
    at forEach (htmlwidgets.js:55)
    at htmlwidgets.js:576
    at Array.forEach (<anonymous>)
    at forEach (htmlwidgets.js:55)

Not sure if it has any impact on the functionality. Anything we should do about this or we could ignore it?

Very weird, this doesn't occur on WSL, but does on Linux. Removing the CSP policies shows a fetch attempt to a non-existent filepath for some reason... I think it does effect the blue dots on the map, but is fixed if you type mapviewOptions(fgb = FALSE). In looking into this I came across this thread which appears to suggest this isn't just a VS Code thing.

ElianHugh and others added 4 commits August 2, 2021 02:22
@ElianHugh
Simplify regex usage
2131fd1 
- Remove superfluous console.log
- Simplify regex usage
@ElianHugh
(Misc) change directory
87ec772 
- Change webview/observer.js to session/webview/observer.js
@renkun-ken
Merge branch 'master' into webview-changes
fd359d4
@renkun-ken
Fix merge conflict
4dce16e
@renkun-ken
Copy link
Member

renkun-ken commented Aug 2, 2021

Very weird, this doesn't occur on WSL, but does on Linux. Removing the CSP policies shows a fetch attempt to a non-existent filepath for some reason... I think it does effect the blue dots on the map, but is fixed if you type mapviewOptions(fgb = FALSE). In looking into this I came across this thread which appears to suggest this isn't just a VS Code thing.

Do you think if there is anything we could do about it?

@ElianHugh
Copy link
Collaborator Author

ElianHugh commented Aug 2, 2021

Do you think if there is anything we could do about it?

image

I found this, so I guess it might be something that can be fixed. I'll have a look

@ElianHugh
Broaden regex, bug fix
8ab7998 
- Fix slowdown for DOMs that have many updates a second
- Fix more resource request paths
@ElianHugh
Only test specific tags
3b4717c 
- No need to test tags that do not typically have src values. This can be expanded if a tag is missing from the list
@ElianHugh
Debounce mut. observer
47c40db
@ElianHugh ElianHugh requested a review from renkun-ken August 4, 2021 04:38
renkun-ken
renkun-ken approved these changes Aug 4, 2021
View reviewed changes
Copy link
Member

@renkun-ken renkun-ken left a comment

Choose a reason for hiding this comment

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

Works nicely now.

LGTM

@renkun-ken renkun-ken merged commit 3b17836 into REditorSupport:master Aug 4, 2021
@renkun-ken renkun-ken mentioned this pull request Sep 3, 2021
Closed
ElianHugh added a commit to ElianHugh/vscode-R that referenced this pull request May 12, 2022
@ElianHugh @renkun-ken
Viewer fix: invalid html_widget resource paths (REditorSupport#739)
fbfd881 
* Implement mutation observer

Observe changes in the DOM, in order to ensure urls are appropriate for  webview restrictions

* pre -> span, clean up observer method

* Refactor webview

* Refactor html into func, observe content only

* Improve docs, change URL testing

* Minor update

* Simplify regex usage

- Remove superfluous console.log
- Simplify regex usage

* (Misc) change directory

- Change webview/observer.js to session/webview/observer.js

* Fix merge conflict

* Broaden regex, bug fix

- Fix slowdown for DOMs that have many updates a second
- Fix more resource request paths

* Only test specific tags

- No need to test tags that do not typically have src values. This can be expanded if a tag is missing from the list

* Debounce mut. observer

Co-authored-by: Kun Ren <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@renkun-ken renkun-ken renkun-ken approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Leaflet tiles not showing in web viewer

2 participants

@ElianHugh @renkun-ken