LeastAuthority / destiny

Destiny – Cross-platform Magic Wormhole graphical client
MIT License
236 stars 13 forks source link

Editable config #209

Closed wuan closed 1 year ago

wuan commented 1 year ago

Fixes #206


Code Review Checklist

donpui commented 1 year ago

We need to update copy/paste/selectall design as it is more actual here and now looks unreadable in at least android:

image
donpui commented 1 year ago

One more thing, during Security Audit, one finding was that the code by default has unsecure connection urls. During mitigation of issue, we agreed that when we will make url editable, we should either notify users if they use unsecure connection either deny to enter: Original OP task: https://leastauthority.openproject.com/projects/destiny/work_packages/1325/activity

Maybe we could go minimum, and show additional text if user enters ws:/ .

donpui commented 1 year ago

Overall looks good, we would need to make more testing on all platforms.

Optional thing, if I set wrong address, we should friendly message that destiny could not connect server, but no details to view what is exactly wrong (it can be mistype in url, not available port and etc..) on sender and receiver side.

wuan commented 1 year ago

We need to update copy/paste/selectall design as it is more actual here and now looks unreadable in at least android:

We did not declare that our color scheme is of type "dark". This should be fixed now.

wuan commented 1 year ago

Overall looks good, we would need to make more testing on all platforms.

Optional thing, if I set wrong address, we should friendly message that destiny could not connect server, but no details to view what is exactly wrong (it can be mistype in url, not available port and etc..) on sender and receiver side.

We can add simple verification of the response being an URL for now. We have to be more verbose regarding errors which happen during a transmission attempt.

donpui commented 1 year ago

Windows setting page is blank, but I don't get any errors printed.

Screenshot 2022-12-19 at 09 05 42

Used windows zip file generated by actions.

Linux - OK, MacOS - OK, Android - OK

wuan commented 1 year ago

Now we even have a Widget test checking the basic functionality of value editing.

donpui commented 1 year ago

Now windows version works.

meejah commented 1 year ago

we should either notify users if they use unsecure connection either deny to enter:

There are currently two well-known public mailbox servers, and one of them uses ws:// only. So if you deny them from editing that, they can't use the software with the default server. If you warn them, what will the warning say that they can meaningfully take action upon?

wuan commented 1 year ago

No windows version works.

Hope you meant "Now" ;-)

wuan commented 1 year ago

we should either notify users if they use unsecure connection either deny to enter:

There are currently two well-known public mailbox servers, and one of them uses ws:// only. So if you deny them from editing that, they can't use the software with the default server. If you warn them, what will the warning say that they can meaningfully take action upon?

Then we need to separate warning from validation.

meejah commented 1 year ago

Then we need to separate warning from validation.

Yeah I think that's what @donpui was suggesting: but what, why and how will a user take meaningful action.

That is, there's currently really only two things they might type in that field: our mailbox (the default) and Brian's mailbox. And now you want to "warn" them about something to do with the public default sever, but it's not clear what or why to me...

meejah commented 1 year ago

We can add simple verification of the response being an URL for now. We have to be more verbose regarding errors which happen during a transmission attempt.

I think what @donpui was getting at here is that we could test whether the URL the user typed in is immediately reachable and speaks the right protocol -- thus saving them from a bunch of "couldn't send file" etc errors when they actually do get around to trying it.

(Obviously, this could be a followup ticket .. I'll add a comment to #206 though).

donpui commented 1 year ago

There are currently two well-known public mailbox servers, and one of them uses ws:// only. So if you deny them from editing that, they can't use the software with the default server. If you warn them, what will the warning say that they can meaningfully take action upon?

We can warn the same as browsers does, only change text to be more correct:

image

To my understanding, unencrypted traffic expose at least app_id, nameplate, maybe something more (if someone can elaborate here)

wuan commented 1 year ago

Looks good and works, fine from my side. Warning notification can be discussed and implemented separately.

Whenever you are ready. There will be some conflicts with the iOS changes, but I can help to resolve them.

donpui commented 1 year ago

Whenever you are ready. There will be some conflicts with the iOS changes, but I can help to resolve them.

Lets merge and I will try update iOS. After iOS PR review, we could release new version.

wuan commented 1 year ago

Whenever you are ready. There will be some conflicts with the iOS changes, but I can help to resolve them.

Lets merge and I will try update iOS. After iOS PR review, we could release new version.

Let me know when I can help you.

meejah commented 1 year ago

We can warn the same as browsers does, only change text to be more correct:

What are we warning about, though? None of that applies here.

I believe the point of Brian running the public server on ws:// is to "prove" that the server doesn't have any advantage attacking clients. That is, you're only revealing (to network listeners) those things you'd reveal to the server anyway -- and that's okay! It's still secure!

I suppose another way to state this is: what attacks are you actually preventing by using wss:// for the mailbox communications?

(I believe the answer is: none, and if the answer isn't "none" then we should probably also look at fixing something in the protocol and/or server).