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

pipe workspace boolean for opener service validator #155545

Merged
merged 2 commits into from Jul 21, 2022

Conversation

sbatten
Copy link
Member

@sbatten sbatten commented Jul 18, 2022

fixes #150828

after trying to implement the proposal I realize that the opener service needs to know validator specific knowledge to execute this flow properly. in order to avoid pollution of knowledge, I went with something like fromUserGesture but fromWorkspace. I adopted it for clicking links in webviews (markdown preview) and editors link detection.

@sbatten sbatten enabled auto-merge (squash) July 18, 2022 21:27
@sbatten sbatten self-assigned this Jul 18, 2022
@sbatten sbatten requested a review from bpasero July 18, 2022 21:27
@VSCodeTriageBot VSCodeTriageBot added this to the July 2022 milestone Jul 18, 2022
@sbatten sbatten requested a review from isidorn July 18, 2022 21:29
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

So when would opening a link not be from the workspace? I am not sure I fully understand when this is true and when this is false, maybe the name is just confusing.

@sbatten
Copy link
Member Author

sbatten commented Jul 19, 2022

The case that spawned this change is the sponsor link from the extension description page. That is not from the workspace. I also did not add this for link detection in terminals since they can be created by tooling very easily.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

For editors: is it possible that you open an untrusted file in a trusted workspace and then we wrongly set fromWorkspace? Also, what if the file is outside of the workspace, does it matter?

For webview: I am not entirely sure this covers only the cases we expect it to, so I suggest to let more people review this change, including someone from the editor team and someone from the webview team?

Finally, is the sponsor link considered fromWorkspace or not? And does fromWorkspace bring up a dialog or not?

@sbatten
Copy link
Member Author

sbatten commented Jul 19, 2022

For editors: is it possible that you open an untrusted file in a trusted workspace and then we wrongly set fromWorkspace? Also, what if the file is outside of the workspace, does it matter?

Once an untrusted file is brought into the workspace, it can be treated as part of the workspace. This is the current model workspace trust uses. However, it is bleeding that assumption into the opener service's model which is a problem with the change as a whole. Though, the alternative is to remove this feature entirely or associate an source with every open call and have validators figure that out. Though we have exactly 1 validator.

Finally, is the sponsor link considered fromWorkspace or not? And does fromWorkspace bring up a dialog or not?

fromWorkspace is false and will be validated by trusted domains. Whether or not a prompt shows depends on the domain.

@mjbvz mjbvz self-requested a review July 19, 2022 21:34
@sbatten sbatten requested review from mjbvz and rebornix July 20, 2022 18:06
@sbatten sbatten merged commit b51955e into main Jul 21, 2022
@sbatten sbatten deleted the sbatten/institutional-toad branch July 21, 2022 18:48
@sbatten
Copy link
Member Author

sbatten commented Jul 21, 2022

@bpasero had this set to auto merge so it got merged, if you have additional comments, we can revert or make additional changes

@rebornix
Copy link
Member

The changes to editor and notebook look safe to me but as @mjbvz pointed out above, setting workspace: "true" is not going to 100% accurate in practice as

  • Editor links can be provided by both builtin link detector, and link provider extensions
  • Notebook cell output and markdown renderers can come from extensions

In both cases, we don't guarantee that the extensions don't generate on their own. With that said, there is no way to know unless we treat all content from extensions as "non-workspace".

@github-actions github-actions bot locked and limited conversation to collaborators Sep 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open sponsor link should ask user if they want to open an external URL
5 participants