jariz / Noti

Receive Android notifications on your mac. (w/PushBullet) ⛺
MIT License
911 stars 63 forks source link

Move authorization flow to external browser #87

Open JaCzekanski opened 5 years ago

JaCzekanski commented 5 years ago

Problem

Original flow of authentication using embedded Webview doesn't work for Google accounts that are secured using U2F key like Yubikey. Screenshot 2019-08-22 at 14 00 25 comment: WebKit does not support U2F api

Solution

Move authorization flow to default browser which will allow the user to sign to U2F secured accounts. Noti will receive access token using a deep link to schema noti://redirect. To use different redirect_uri separate client_id for Pushbullet had to be generated.

Screenshots

Screenshot 2019-08-22 at 13 57 17 Screenshot 2019-08-22 at 13 58 17 Screenshot 2019-08-22 at 14 06 22

jariz commented 5 years ago

Very cool! Thanks a lot.

A minor security comment though: please include a nonce in the callback url so noti can verify it generated the URL itself rather than a different application. I just tested this and even though the redirect_uri is set in the app configuration, pushbullet allows you to pass additional query parameters like so:

https://www.pushbullet.com/authorize?client_id=....&redirect_uri=noti%3A%2F%2Fredirect%3Fnonce%3D6345242353&response_type=token&scope=everything
                                                                 ^
JaCzekanski commented 5 years ago

I added secure nonce generation which will prevent reusing callback URIs. It's my first time writing Swift so if there are any comments regarding code style or bad practices - don't hesitate to point them.

jariz commented 5 years ago

Just FYI, any (unsandboxed) app can write to an other app's defaults.

defaults write io.jari.Noti nonce 1337
open "noti://redirect?nonce=1337#access_token=evil"

I was more thinking about just keeping it in memory. But, your fix already greatly decreases the attack surface, just thought I'd ensure you're aware of this. I'm fine with merging this.

About the codestyle, it's fine really. This is not exactly a great example on how to do swift development correctly as it was my first swift project as well 😛

JaCzekanski commented 5 years ago

You're right - storing nonce in memory would suffice. Apart from that I can't think of any real security threat with current solution. Worst case scenario: some one opens Noti using deep link by accident and is relogged into another account with access token, but nonce will prevent it right now.

I'm happy with current solution - I'm able to log in and notification mirroring is working again for my phone. If you have any ideas for improvements I'll be happy to include them in this PR.

JaCzekanski commented 5 years ago

Sorry for the lack of activity - I completely forgot about this PR. I made a small change which and now nonce is stored in AppDelegate during the authentication process.

IanVS commented 5 years ago

Where's this stand now? Seems like a great change to me!

agucova commented 4 years ago

@jariz Is the project no longer maintained? I can fork and merge the pull request.