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

[notifications] disallow arbitrary html in message content #7289

Merged
merged 1 commit into from
Mar 9, 2020

Conversation

AlexTugarev
Copy link
Contributor

@AlexTugarev AlexTugarev commented Mar 6, 2020

In alignment with vscode, this PR will change rendering of user notifications. HTML+JavaScript won't make into the rendered output 🎉 (Thanks to @luigigubello for reporting!)

I also checked that links with javascript: scheme are not translated to anchors for markdown.

Closes #7283

Review checklist

Reminder for reviewers

hot to verify

add a task like this an use the >Run Task command

{
  "tasks": [
    {
      "label": "<a href=\"javascript:alert('foo')\">click me</a>",
      "type": "shell",
      "command": "echo 'foo!'",
      "presentation": {
        "reveal": "always",
        "focus": false
      }
    }
  ]
}

See the HTML is escaped with this PR:
Screen Shot 2020-03-09 at 09 27 00

While it goes into the DOM on master:
Screen Shot 2020-03-09 at 09 25 59

this way we can be sure that no scripts can be executed.

Signed-off-by: Alex Tugarev <alex.tugarev@typefox.io>
@AlexTugarev AlexTugarev changed the title [notifications] disallow arbitrary html in message content [wip][notifications] disallow arbitrary html in message content Mar 6, 2020
@AlexTugarev AlexTugarev added notifications issues related to notifications vscode issues related to VSCode compatibility labels Mar 6, 2020
@AlexTugarev AlexTugarev changed the title [wip][notifications] disallow arbitrary html in message content [notifications] disallow arbitrary html in message content Mar 6, 2020
@akosyakov
Copy link
Member

@AlexTugarev please reference the relevant issue

@kittaakos
Copy link
Contributor

@AlexTugarev, how can I verify it? Thanks!

@kittaakos
Copy link
Contributor

how can I verify it?

Found it.

Reproduction Steps
Create a new project and create a new debugger configuration file launch.json
In the type field write the Javascript payload (e.g. <details open ontoggle=confirm(2)>)
Press F5 to launch the debugger and see the alert box

@AlexTugarev
Copy link
Contributor Author

@kittaakos, a task definition with a label containing HTML should now be escaped in the confirmation message Task '${taskIdentifier}' has been started. I expect it to be html content on master.

@kittaakos
Copy link
Contributor

Thanks, @AlexTugarev. I even checked the video attached to the issue, no idea how to verify. Can you please write down the steps? Thanks!

@AlexTugarev
Copy link
Contributor Author

@kittaakos, I've added a repro to verify ☝️

@kittaakos
Copy link
Contributor

@kittaakos, I've added a repro to verify ☝️

Thank you!

I have verified and compared the behavior with the master.

PR:
screencast 2020-03-09 09-38-43

master:
screencast 2020-03-09 09-43-04

Note: I see only an error when I do the same on the master.

Uncaught (in promise) Error: A resource provider for 'javascript:alert%28%27foo%27%29' is not registered.
    at DefaultResourceProvider.<anonymous> (bundle.js:131973)
    at step (bundle.js:131818)
    at Object.throw (bundle.js:131799)
    at rejected (bundle.js:131791)

@AlexTugarev
Copy link
Contributor Author

Note: I see only an error when I do the same on the master.

Oh yeah, but that doesn't matter. Link handler are using the command service. I should have written an example with onClick handler.

Thanks for verifying @kittaakos! 🙏

@AlexTugarev AlexTugarev merged commit a7ec808 into master Mar 9, 2020
@marcdumais-work marcdumais-work deleted the alextugarev/javascript-injection-7283 branch March 16, 2020 14:53
@luigigubello luigigubello mentioned this pull request Dec 16, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notifications issues related to notifications vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Javascript injection via notification messages
3 participants