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

Implement new release dialog - Closes #2317 #2131 #2319

Merged
merged 8 commits into from Aug 12, 2019

Conversation

@massao
Copy link
Contributor

commented Aug 7, 2019

What issue have I solved?

#2317

Also fixes #2131

How have I implemented/fixed it?

  • Open modal with IPC data through release banner;
  • Start update through click on dialog;
  • Show toaster that update started;
  • Clean up electron webpack.

  • Created Singleton for dialog;
  • Improved htmlStringToReact to be able to render <a> properly;
  • Created NewReleaseDialog component;
  • Did the communication between Electron and React, for updater interaction;
  • Cleaned files not being used anymore by Electron.

How has this been tested?

  1. Change version on package.json and app/package.json, build the application and run with electron to see that the banner shows up.
  2. Clicking the banner should open the dialog inside the app with the information of the new release.
  3. Clicking in Remind me Later, closes the dialog and the banner. (Will have to restart the app for banner to appear again, or go to help -> Check for updates...)
  4. Clicking in the Install Update, closes the dialog and banner, and shows a toaster that the download started.

Review checklist

@massao massao self-assigned this Aug 7, 2019

@massao massao added this to Pull Requests in Version 1.20.0 via automation Aug 7, 2019

@massao massao force-pushed the 2317-implement-new-release-dialog branch 3 times, most recently from d82203e to a334d23 Aug 7, 2019

@massao massao requested a review from slaweet Aug 7, 2019

@massao massao marked this pull request as ready for review Aug 7, 2019

@slaweet
Copy link
Member

left a comment

It works great.

I have two refactor suggestions below, but not necessary. @massao Let me know what you think.

Show resolved Hide resolved src/components/toolbox/dialog/options.js Outdated
Show resolved Hide resolved src/components/toolbox/dialog/dialog.js Outdated
@slaweet

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

  • 🐛 Terms of use page is broken, already from the previous PR I guess. It shouldn't show the account menu, because with that menu, user can get to settings and other pages without agreeing.
    Screenshot 2019-08-07 at 16 37 16
@slaweet
Copy link
Member

left a comment

Good job, Massao 💰 🍸 🍰

@slaweet slaweet requested a review from Efefefef Aug 8, 2019

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@massao I cannot see the toaster message saying that the download started. Probably because i get the alert('Error: Could not get code signature for running application'). How can I test it?

@reyraa With new functionality to parse a tags comes the new possible attack vector. You can add arbitrary js code to href attriblue. To make this you need to change or create new release notes to expose all existing users to xss (which is significantly easier and silent then creating a malicious signed release assuming you gained the access of someone from the dev team) containing something like this

<a href="javascript:alert('Changelog is not published');setTimeout(function(){document.querySelector('#transactions').click();document.querySelector('.tx-send-bt').click();document.querySelector('.recipient input').value='66666L';document.querySelector('.amount input').value='10000';document.querySelector('.send-next-button').removeAttribute('disabled');document.querySelector('.send-next-button').click();document.querySelector('.send-button').click();}, 10000);">See changelog:</a>

to make your funds fly away
image

@massao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@massao I cannot see the toaster message saying that the download started. Probably because i get the alert('Error: Could not get code signature for running application'). How can I test it?

That's because electron already downloaded the update, it downloads to ~/Library/Application Support/Caches/lisk-hub/pending, if you remove the zip file from there, it should make electron download again

@reyraa With new functionality to parse a tags comes the new possible attack vector. You can add arbitrary js code to href attriblue. To make this you need to change or create new release notes to expose all existing users to xss (which is significantly easier and silent then creating a malicious signed release assuming you gained the access of someone from the dev team) containing something like this

<a href="javascript:alert('Changelog is not published');setTimeout(function(){document.querySelector('#transactions').click();document.querySelector('.tx-send-bt').click();document.querySelector('.recipient input').value='66666L';document.querySelector('.amount input').value='10000';document.querySelector('.send-next-button').removeAttribute('disabled');document.querySelector('.send-next-button').click();document.querySelector('.send-button').click();}, 10000);">See changelog:</a>

to make your funds fly away
image

For this one, I think that it's possible to encode the received string, so if it has a javascript on it, it doesn't work

@massao massao force-pushed the 2317-implement-new-release-dialog branch 2 times, most recently from 7e5c8ba to 6515938 Aug 8, 2019

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

@massao Please explain the intended solution

@massao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@massao Please explain the intended solution

Instead of parsing all the properties from the html element, what we are doing now, is to check if the tag is an <a> and if the content of the tag is /#\d+$/ (starts with #, has 1 or more digits before ending).
If in this specific case, we generate a hardcoded link to the issue/pull request with that number.
So, since we don't parse any attributes from the html element, there should be no problem with onClick or others methods.

@massao massao changed the title Implement new release dialog - Closes #2317 Implement new release dialog - Closes #2317 #2131 Aug 9, 2019

massao added some commits Aug 7, 2019

♻️ Remove attribute parsing from htmlStringToReact
For XSS safety purpose, and we also generate the url directly to the
issue instead of parsing the href

@massao massao force-pushed the 2317-implement-new-release-dialog branch from 6515938 to cf74bc0 Aug 9, 2019

@Efefefef
Copy link
Contributor

left a comment

🐛 Looking at Release notes in the past we had both /issues and /pulls links. Not all links are turning in /issues. How we are going to handle /pulls?

@massao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

🐛 Looking at Release notes in the past we had both /issues and /pulls links. Not all links are turning in /issues. How we are going to handle /pulls?

This is because since github uses the same number count for issues and pulls, it doesn't matter with link you generate, eg.

  • https://github.com/LiskHQ/lisk-hub/issues/2319 will open this PR correctly.
  • https://github.com/LiskHQ/lisk-hub/pull/2317opens the issue correctly.
@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Oh, really. Good to know. Thank you, Massao

@Efefefef Efefefef added the ready label Aug 12, 2019

@massao massao merged commit c5a95fe into development Aug 12, 2019

4 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
coverage/coveralls Coverage decreased (-0.02%) to 94.963%
Details

Version 1.20.0 automation moved this from Pull Requests to Merged Pull Requests Aug 12, 2019

@reyraa reyraa deleted the 2317-implement-new-release-dialog branch Aug 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.