-
Notifications
You must be signed in to change notification settings - Fork 92
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
ui: add home screen and update sessions list #401
Conversation
It's been a while since I've read typescript or thought about web security, but I think that this is fine since the scheme isn't really changing besides adding |
)} | ||
<QRCodeModal | ||
url={session.terminalConnectUrl} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if a user wants to pair to a mobile app other than Terminal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. The mobile app dev can parse the Terminal url to extract the encoded phrase & server, then decode it.
If we used a different format for the QR data, it would prevent the user from scanning the QR with any scanning app on their device and being directed to Terminal in the browser. From a UX perspective, I think most users immediately open their phone's camera app when they see a QR code in the wild. With a different format, they'd have to know to go to the Terminal website first, then click on a "Scan QR" button. This would have to be explicitly spelled out for them in the modal. We just eliminated the extra step.
Do you think this prohibits other apps from using the QR codes? Do you have any suggestions on how to improve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The option to use the pairing phrase is still available, correct? If a user wants to connect a different app they could simply type the pairing phrase. Not as user friendly but it means users aren't totally locked out from using 3rd party apps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mobile apps would be able to parse it, just have to decode it.
I'll have to put some thought into how we could do this better.
One potential security concern with putting a pairing phrase in the URL is the persistence of browser history. As far as I know, it's not possible for a website to to clear a URL out of browser history. This means the pairing phrase would end up being stored in the user's autocomplete history indefinitely, unless the user manually cleared out their browser cache. This is a situation where it would be very helpful to have a minimal back-end for terminal-web. We could generate a temporary symmetrical encryption key which is stored on our servers and used to decrypt the URL in terminal, preventing anyone with access to the URL (besides us) from extracting any useful information. |
Very good point @itsrachelfish. Data after the |
Been thinking on this for the past hour and I think the biggest attack surface would be on the
|
Great feedback @itsrachelfish, thanks so much for taking a look. Below are my responses to your questions.
The encoded url segment is always treated as a string by the code that decodes it. It is not possible to pass in binary or null bytes via the querystring AFAIK. There once was a vulnerability in the
We use the
This is a good question. I am not sure if any sensitive data is leaked during the handshake with the proxy server. @ellemouton may be better suited to answer this. One thing we can do on the frontend is display the proxy server and a warning message if the server address is different from our standard one.
What do you imagine could be exploited by providing the wrong info. I suppose the worst case is the same as your previous question where the proxy server is malicious. Other than that, the wrong data would just be populated in the form fields of the connect page. |
Response from Elle via DM.
Elle: No the attacker wont be able to know the users pairing phrase. The attacker will only know the mailbox ID that the user is using which is the hash of the pairing phrase. But the attacker wont be able to use this to derive the paring phrase and thus wont be able to decrypt any of the data flowing through the stream |
@itsrachelfish: review reminder |
After looking into this, it appears that this is a pretty big deal. We are essentially storing the pairing phrase on the machine permanently in an insecure format. Anyone with access to the machine could view the browser history, obtain the pairing phrase, then have full access to the node. No bueno! I've been investigating how to prevent the browser from persisting the history entry on the client side. There are two browser APIs that can manipulate the history, History.replaceState and Location.replace. These will update the session history so that the back button will not navigate to the url with the encoded phrase, but they do not alter the persisted global history in the browser. The only way I could find to prevent the browser from persisting urls in the global history is to use an HTTP 301 redirect, but this requires a backend. In our current use case, having a backend wouldn't work either because that would leak the node's pairing phrase to our server. With all of that said, it seems to me that passing unsecured sensitive data via the url is just not going to work. We have to figure out another way to pass the phrase from Alternative approaches:
I am trying to think of more solutions but I just wanted to post on update on where I am with this PR. If anyone else has any ideas, please do share. |
worth noting that the thing we are encrypting, the pairing phrase, is a one-time use thing iff the litd node is on a version using second handshake. So the only situation we really need to worry about is if an attacker gets their hands on the link before the user is able to use it or if the initial connection to the node didnt work and the user waits for a while before retrying..... this also makes me think that with second handshake, Litd should timeout/expire a session after a few mins if no initial connection (via tha passphrase) is made in a timely manner. ie - give the user a very short window in which they can switch the connection over to using second handshake. |
Thank you for providing a much better solution @ellemouton 🙌
This new link functionality will only be available on future versions of Given that the pairing phrases will now timeout after a short period of time, I will need to make some small changes to this PR. Currently the frontend will detect if there is an available session that isn't revoked, expired, or in use. If there isn't one available, it automatically creates a new one when the home screen loads. With the sessions expiring quickly, this could cause many new sessions being repeatedly created then revoked. I am going to update the home screen so that a new session isn't created until the user expresses intent to connect to TW by either clicking the "Connect to Terminal" or "Connect with QR" buttons. |
Based on the conversation we had today, I will make the following changes to this PR:
|
Doing a proper review this evening/tomorrow morning, so should have some nits and comments in parallel with the latest set of TODOs as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incredible work as usual!
Will update this tomorrow morning .. have to run 1-2 things by Michael, will confirm on KB.
-
Update sub-heading copy from
Connect your node to Terminal on the web via the link below. For mobile, generate a QR code to connect.
toSecurely and privately connect your node to Terminal on the web via the link below. For mobile, generate a QR code to connect.
-
Set
max-width
of the two image columns, not just the image asset, there would still be that extra column space I'd like to avoid, to480px
, looks best. -
Adjust the modal header font to match our headers on the Home view with 32px font-size & 40px line-height .. not sure if this a theme font-size, currently it's 45px on the
XXL
. -
Adjust the padding of the modal header section from
40px
to32px 40px
Awesome, got sign off on the copy change as well. Will re-review here momentarily. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sessions are no longer automatically created since the pairing phrase must be used within 10 minutes of creation. With these changes, the session is only created when the user clicks on the connect buttons.
Rebased on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 🔥
This PR adds a new home screen to showcase new features recently added to Terminal on the web. It also adds the ability to quickly pass session info to your node via hyperlinks or QR codes for mobile.
Looking for feedback on security:
The session data is currently encoded with
base64("<phrase>||<proxy>")
then passed to Terminal via the URL in the formathttps//terminal.lightning.engineering/#/connect/pair/<encoded-data>
. The data is just decoded and pre-filled into the Connect page's form fields. The user will need to click the Connect button to perform the handshake.From a security perspective, I believe passing the encoded data in the URL is fine since the data after the
#
never leaves the user's browser. It's probably a bit more secure than copying the phrase to the clipboard since every app on the device has clipboard access.Is this encoding secure enough of should we do more?