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

Add feature to manage plugins without using the command line #1824

Merged
merged 3 commits into from
Dec 16, 2022

Conversation

BenSurgisonGDS
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS commented Nov 30, 2022

See: Design and build a means to automatically install, upgrade, and uninstall a plugin with a push of a button

  • Links in Plugins page changed to buttons for accessability
  • No client-side javascript maintains previous functionality including instructions of how to install a plugin
  • Additional client-side javascript asset:
    • initiate install via ajax and display processing panel
    • polls status of install until complete or failure
    • display appropriate confirmation or failure panel
    • account for user refreshing page after confirmation panel is displayed
  • Server-side changes to initiate install mode via ajax and return status of install
  • Make sure the above works for uninstall, and upgrade modes as well as install
  • Add Acceptance tests to test all the above in Linux, Windows, and MacOS for both node 16 and 18
  • Implement CSRF Protection
  • Add warning and confirmation button if plugins install page linked from external site
  • Add console logging to aid in debugging

@BenSurgisonGDS BenSurgisonGDS force-pushed the perform-install-plugin-alternative-journey branch from c005231 to e92dec7 Compare November 30, 2022 11:33
@BenSurgisonGDS BenSurgisonGDS force-pushed the perform-install-plugin-alternative-journey branch 5 times, most recently from 5d12e9d to 4d2e991 Compare December 5, 2022 16:46
@BenSurgisonGDS BenSurgisonGDS force-pushed the perform-install-plugin-alternative-journey branch 4 times, most recently from f6e861a to 7d3d2d8 Compare December 9, 2022 17:58
@BenSurgisonGDS BenSurgisonGDS marked this pull request as ready for review December 9, 2022 18:24
@BenSurgisonGDS BenSurgisonGDS requested a review from a team December 9, 2022 18:24
@BenSurgisonGDS BenSurgisonGDS force-pushed the perform-install-plugin-alternative-journey branch 4 times, most recently from 2b4e38d to 74b1297 Compare December 12, 2022 13:19
@BenSurgisonGDS BenSurgisonGDS changed the title Perform install plugin alternative journey Add feature to manage plugins without using the command line Dec 12, 2022
@BenSurgisonGDS BenSurgisonGDS force-pushed the perform-install-plugin-alternative-journey branch from 2fbfeac to 019079b Compare December 12, 2022 15:29
@lfdebrux
Copy link
Member

lfdebrux commented Dec 12, 2022

@BenSurgisonGDS thanks for the demo earlier, this is coming along well. I've been thinking a bit more about the security of this. I'm thinking about the fact that this feature lets the user install code onto their machine, and could be used by third parties to install code on to the users' machine.

At the moment, a user could click a link on an external website, and the kit would immediately start installing the plugin. The user may not necessarily know or expect that based on the text of the link, and would have no way to cancel the action if they weren't expecting or didn't want the change. I know that at the moment we have some protection because we only install packages from our list, but that might not stay the case in future, and it's generally better to be prudent now.

I’d like to see this feature to have the following restriction: if someone clicks a link to install a plugin from outside the kit, or types a URL into the browser, they will see an ‘are you sure?’ page to confirm (I think we can keep it so that if they click the existing button from the kit website they don’t see that page).

I think we can do this by a) requiring that a POST is made before the JavaScript kicks in and b) use some kind of CSRF mitigation to ensure the POST request is coming from the kit website (preferably the synchronizer token pattern) [1].

We should also have tests that check we can't install a plugin with a link without first getting user authorization.

@joelanman
Copy link
Contributor

We just met about this, I think the outcome was it was easier to show everyone the page with the button on it, regardless of how they got there. I've added to the Figma:

https://www.figma.com/file/sWnqT4u4uup91TuTe9T3tp/GOV.UK-Prototype-Kit?node-id=325%3A68&t=fRtdngYlr08O97Mu-0

@BenSurgisonGDS BenSurgisonGDS force-pushed the perform-install-plugin-alternative-journey branch 2 times, most recently from d1e9362 to 98ee821 Compare December 14, 2022 09:30
@joelanman
Copy link
Contributor

found a tutorial to do csrf by hand, might be helpful:
https://levelup.gitconnected.com/how-to-implement-csrf-tokens-in-express-f867c9e95af0

@lfdebrux
Copy link
Member

lfdebrux commented Dec 14, 2022

We've decided to go with the csurf package for our CSRF token middleware. Although it is deprecated [1], we could not find any well-supported alternative or single fork that has been chosen by the community (a large number of projects, including new projects, still rely on csurf).

Because the code is stable and has been for 3 years, and there are no known vulnerabilities that would affect our use case, the risk of using the deprecated library is in my opinion lower than the risk of picking an alternative or a fork that is more liable to change. There is possibly a greater risk of hijacking of the csurf npm package than of a well maintained package, however with the npm shrinkwrap mitigation we have in place (see #1654) that would be unlikely to affect our users.

An alternative would be to create our own fork of csurf, or our own alternative implementation; however we would then not benefit from the community scrutiny of our code that is a major benefit of using open source software, and we would have an additional maintenance burden that we do not want.

Note: there is a known vulnerability in the implementation of the double submit cookie pattern implementation in csurf [3], however as we are using cookie: false we are using the synchronization token pattern implementation [4], which is not vulnerable.

@BenSurgisonGDS
Copy link
Contributor Author

@BenSurgisonGDS thanks for the demo earlier, this is coming along well. I've been thinking a bit more about the security of this. I'm thinking about the fact that this feature lets the user install code onto their machine, and could be used by third parties to install code on to the users' machine.

At the moment, a user could click a link on an external website, and the kit would immediately start installing the plugin. The user may not necessarily know or expect that based on the text of the link, and would have no way to cancel the action if they weren't expecting or didn't want the change. I know that at the moment we have some protection because we only install packages from our list, but that might not stay the case in future, and it's generally better to be prudent now.

I’d like to see this feature to have the following restriction: if someone clicks a link to install a plugin from outside the kit, or types a URL into the browser, they will see an ‘are you sure?’ page to confirm (I think we can keep it so that if they click the existing button from the kit website they don’t see that page).

I think we can do this by a) requiring that a POST is made before the JavaScript kicks in and b) use some kind of CSRF mitigation to ensure the POST request is coming from the kit website (preferably the synchronizer token pattern) [1].

We should also have tests that check we can't install a plugin with a link without first getting user authorization.

All the above now implemented.

@BenSurgisonGDS BenSurgisonGDS force-pushed the perform-install-plugin-alternative-journey branch 3 times, most recently from 791b32e to 9885dbe Compare December 14, 2022 14:25
cypress/support/e2e.js Outdated Show resolved Hide resolved
lib/config.js Outdated Show resolved Hide resolved
@BenSurgisonGDS BenSurgisonGDS force-pushed the perform-install-plugin-alternative-journey branch from da05e0b to 9885dbe Compare December 14, 2022 14:58
@BenSurgisonGDS BenSurgisonGDS force-pushed the perform-install-plugin-alternative-journey branch 2 times, most recently from bcea70e to 5c6e56c Compare December 14, 2022 15:29
Comment on lines 11 to 15
const show = (id) => document.getElementById(id)
.classList.remove('govuk-!-display-none')

const hide = (id) => document.getElementById(id)
.classList.add('govuk-!-display-none')
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be preferable to use the hidden attribute instead of a CSS class, that way the page is more likely to look correct if the stylesheets haven't loaded or have been overridden for some reason.

Currently without stylesheets the page to install a plugin will read:

Install Step By Step from GOV.UK Prototype Kit

In terminal, press ctrl + c to stop your prototype, then run:

npm install @govuk-prototype-kit/step-by-step

When you've installed the plugin, restart your prototype in the terminal by typing:

npm run dev

Install
Installing ...
Install complete
There was a problem installing
[Please contact support](https://prototype-kit.service.gov.uk/docs/support)

Copy link
Member

Choose a reason for hiding this comment

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

Ideally this page would work without CSS (in fact it does, if you click the button! You just can't tell from the browser...)

Copy link
Contributor Author

@BenSurgisonGDS BenSurgisonGDS Dec 14, 2022

Choose a reason for hiding this comment

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

The hidden attribute works when it ia done on a div so I have wrapped the confirmation button in a div to make it work with only the hidden attribute. Css is now unnecessary for this functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed using the hidden attribute and some small markup changes

@BenSurgisonGDS BenSurgisonGDS force-pushed the perform-install-plugin-alternative-journey branch from 8365654 to e7f9fcf Compare December 14, 2022 16:15
@BenSurgisonGDS BenSurgisonGDS force-pushed the perform-install-plugin-alternative-journey branch 3 times, most recently from eea8a38 to 4dbe57f Compare December 15, 2022 12:54
@joelanman
Copy link
Contributor

Seems to be a problem in Safari on Mac, the process does not start automatically but instead shows the install button. Thats not a blocking issue so happy to raise it separately if you prefer.

@BenSurgisonGDS
Copy link
Contributor Author

BenSurgisonGDS commented Dec 15, 2022

Seems to be a problem in Safari on Mac, the process does not start automatically but instead shows the install button. Thats not a blocking issue so happy to raise it separately if you prefer.

Yes, please raise separately.
Please see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Sec-Fetch-Dest

Copy link
Contributor

@joelanman joelanman left a comment

Choose a reason for hiding this comment

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

awesome! Great work all round

Handle user pressing refresh in plugin confirmation page
Add CSRF protection
Added warning when plugin action button is shown
Replace css with the hidden attribute
Convert button markup to use nunjucks macros
Add some console logging for plugin install status
@BenSurgisonGDS BenSurgisonGDS merged commit e4dac2e into main Dec 16, 2022
@BenSurgisonGDS BenSurgisonGDS deleted the perform-install-plugin-alternative-journey branch December 16, 2022 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Design and build a means to automatically install, upgrade, and uninstall a plugin with a push of a button
4 participants