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 MEGAsync by mega.co.nz #5338

Closed
wants to merge 1 commit into from
Closed

Conversation

eugenesan
Copy link
Contributor

No description provided.

@eugenesan
Copy link
Contributor Author

Please don't merge yet.

Seems like app won't start unless installed in /Applications.
Adding "system '/usr/bin/defaults', 'write', 'mega.mac', 'moveToApplicationsFolderAlertSuppress', '-bool', 'true'" doesn't help.

Any ideas?

Thanks.

@eugenesan
Copy link
Contributor Author

Fixed by linking directly to /Applications.

@vitorgalvao
Copy link
Member

This is an unusual solution, and could cause unexpected results (to users, not technically). Do you know why the software has this limitation? We had a similar case some time ago (trying to remember what cask it was), that also needed to be installed to /Applications, but for security reasons (made sense for the app). Trying to find it so we can see what was the resolution then. Would definitely like more input on this, before merging.

@eugenesan
Copy link
Contributor Author

Main binary contains the following:

chmod 4755 /Applications/MEGAsync.app/Contents/MacOS/MEGAsync && chown root /Applications/MEGAsync.app/Contents/MacOS/MEGAsync && echo true

Seems like this applications requires suid to operate and has hardcoded path to it's binary.
I guess we could accept this behavior as security measure to avoid "suid"ing unexpected/malicious binary.

@radeksimko
Copy link
Contributor

We had a similar case some time ago (trying to remember what cask it was)

I think it is tunnelblick.

@vitorgalvao
Copy link
Member

I think it is tunnelblick.

It is. Thank you. Apparently we still have no definitive answer on how it went.

The caveat is a solution I’m fine with, though. @eugenesan Does MEGAsync ask to be moved to /Applications on first launch, or something similar?

@eugenesan
Copy link
Contributor Author

@vitorgalvao MEGAsync silently refuses to start from any other location.

@vitorgalvao
Copy link
Member

Failing silently is really a crappy experience, even if we add a caveat. It would likely be less confusing (to users) to just outright move the whole app bundle into /Applications (something they might be used from casks that use install), than linking it somewhere atypical (which would be weird according to their expectations).

I somewhat doubt they really need the app in /Applications, particularly since with your solution it’s not there, it’s working via a symlink. The lack of any warning (since it’s failing silently) makes me doubt it even more. It seems they simply haven’t considered the possibility of someone having the app bundle in a place other than /Applications. It’s clearly possible to handle the situation well, as tunnelblick showed. Do you have any resource where they officially explain why this works this way?

@radeksimko
Copy link
Contributor

I have just fired a question to their support, I will keep you posted here if they reply.

@radeksimko
Copy link
Contributor

Fri, Jul 18, 2014 at 1:44 AM BST:

We have escalated your enquiry to our developers with a view to having a resolution for you as soon as possible.

@javiserrano
Copy link

Greetings from the MEGA team.

There are two reasons that led us to use absolute paths to executables in /Applications/MEGAsync.
One of them is the one commented by eugenesan: the main binary (MEGAclient) uses that command to set the "suid" bit for a little loader (MEGAsync) of 4 lines of code. That loader needs those privileges to open /dev/fsevents, that is the source that MEGAsync uses to get filesystem notifications.

MEGAsync fails to start in other locations because the little loader also uses an absolute path to start the main binary. We could get the path of the current executable and start the other one from the same folder but that would require to do it at runtime using a system framework or an external library. That would add complexity to this loader and would make it an easier target for malicious attacks that could try to exploit it to gain root access.

Anyway, we are open to any suggestions to improve this, keeping the security as the main priority.

@vitorgalvao
Copy link
Member

Thank you @javiserrano, for taking the time to come here and clarifying your reasonings (and thank you @radeksimko for contacting support).

As stated, failing silently is the real issue, especially since you don’t really need the app bundle itself to be in /Applications (as @eugenesan showed, the app bundle can be anywhere, as long as there’s a symlink to it in /Applications). Tunnelblick (mentioned in previous comments) only launches if the app bundle itself is in /Applications (a symlink doesn’t suffice). It simply checks at runtime if it’s there, and if it is not, it offers to move itself (they ask for your password to also perform extra tasks)

That, to me, sounds like the easiest solution. Instead of failing silently, ask to move the app to where you need it, and nothing else needs to change. There are other apps that do something similar (although in those cases, it’s usually more of a suggestion, and not mandatory).

@javiserrano
Copy link

I fully agree with you @vitorgalvao. MEGAsync should ask to move the app to /Applications or at least should show an error message. I will explain why we don't immediately improve this:

The current version starts running the loader as root, the loader opens /dev/fsevents and finally it launches MEGAsync (the MEGAclient binary) without admin privileges using a hardcoded path. In order to improve this, we would have to change the loader and, when users update to the new version, they would have to enter their root password again to restore the setuid bit.

We are currently working on Finder integration to show overlay icons and context menus for files/folders. Those changes will also require to change the loader and thus to request the root password. As these improvements will be probably ready in a couple of months, we think that it's better to wait a bit and to put all improvements regarding the root loader in that update to not ask for the root password twice (many users don't like to type his root password and we prefer to avoid it if it isn't absolutely necessary).

During this time, I think that we will also improve the update feature to be able to update the loader without asking for the root password. That way we won't have these problems again.

Switching to full variant since light variant is not available.
jeroenseegers pushed a commit to jeroenseegers/homebrew-cask that referenced this pull request Jan 10, 2015
This is a follow-up to Homebrew#5338. I had no issues installing and running
the MEGAsync client using this caskfile.
@vitorgalvao
Copy link
Member

A better solution from upstream would still be preferred, but in the meantime #8804 adds a caveat for this.

@adidalal adidalal removed the awaiting maintainer feedback Issue needs response from a maintainer. label Jan 3, 2016
@miccal miccal removed the upstream Issue which needs to be resolved by the upstream project. label Dec 23, 2016
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
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.

7 participants