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

Fix installing updates which need escalated privileges #207

Merged
merged 9 commits into from
Jul 20, 2017

Conversation

joshaber
Copy link
Contributor

@joshaber joshaber commented Jul 17, 2017

⚠️ Dependent on #206 ⚠️

Fixes #157
Fixes #131

There were two problems:

  1. We checked if we could write to the app bundle itself and escalated, but we didn't check if we could write to its parent directory to replace the bundle.
  2. If we escalated, our user-scoped paths wouldn't be valid since ShipIt would run as root.

I still need to find a way to test this, even if just by hand, to verify the above issues really are fixed.

@joshaber joshaber changed the title [WIP] Fix installing updates Fix installing updates Jul 18, 2017
@joshaber
Copy link
Contributor Author

joshaber commented Jul 18, 2017

I've manually tested the following scenarios:

  1. User-owned app bundle and parent directory. Succeeds 👍
  2. Root-owned app bundle and user-owned parent directory. Escalates and succeeds 👍
  3. Root-owner parent directory. Escalates and succeeds 👍

@joshaber joshaber changed the title Fix installing updates Fix installing updates which need escalated privileges Jul 18, 2017
@joshaber joshaber changed the base branch from update-xcode to master July 18, 2017 17:48
@joshaber joshaber merged commit 203315f into master Jul 20, 2017
@joshaber joshaber deleted the escalators-are-dangerous branch July 20, 2017 13:46
@hhff
Copy link

hhff commented Jul 29, 2017

Hi @joshaber - a big big thankyou for tackling this! Really great stuff 👍

@devtobo
Copy link

devtobo commented Jul 31, 2017

Is it fixed in Electron (not sure if they or I need to do something to have this fix for my users)?

@joshaber
Copy link
Contributor Author

No, Electron hasn't yet released an update which includes this fix.

@Roverclover
Copy link

Y

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants