BeamMW / beam-ui

Beam Desktop Wallet (Win / Mac / Linux)
https://beam.mw/
Apache License 2.0
27 stars 23 forks source link

Pasting a server address with long TLD is impossible - REWARD 300 BEAM #5

Closed VladislavKulyba closed 4 years ago

VladislavKulyba commented 4 years ago

Pre-condition: Go to Settings, switch to swap, switch to Electrum. Steps to reproduce:

  1. Copy the following text: electrum2.privateservers.network
  2. Paste it in Node Address.

Expected: Address is successfully pasted. Actual: Nothing happens.

echoby commented 4 years ago

To define UI for swap settings screen we use "SwapNodeSettings.qml" file. You can find it in "[your path]\beam-ui\ui\view\controls\SwapNodeSettings.qml". Find "Node Address" string

            SFText {
                visible:        !editElectrum
                font.pixelSize: 14
                color:          control.color
                //% "Node Address"
                text:           qsTrId("settings-node-address")
            }

            IPAddrInput {
                id:               addressInput
                visible:          !editElectrum
                Layout.fillWidth: true
                color:            Style.content_main
                underlineVisible: canEditNode
                readOnly:         !canEditNode
            }
       // electrum settings
            CustomCheckBox {
                id: selectServerAutomatically
                visible:        editElectrum
                Layout.columnSpan: 2
                Layout.fillWidth: true
                Layout.alignment: Qt.AlignHCenter | Qt.AlignLeft
                //% "Select server automatically"
                text: qsTrId("select-server-automatically")
            }

            SFText {
                visible:        editElectrum
                font.pixelSize: 14
                color:          control.color
                //% "Node Address"
                text:           qsTrId("settings-node-address")
            }

            IPAddrInput {
                visible:          editElectrum
                id:               addressInputElectrum
                Layout.fillWidth: true
                color:            Style.content_main
                ipOnly:           false
                readOnly:         selectServerAutomatically.checked
                underlineVisible: !selectServerAutomatically.checked
            }

Element "addressInputElectrum" responsible for electrum node address editing.

Open "[your path]\beam-ui\ui\view\controls\IPAddrInput.qml"

Property: "ipOnly" set as false For validation used "ipDomainValidator" as a result of next expression: validator: ipOnly ? ipValidator : ipDomainValidator

Regex for validating domain is "((([A-Za-z0-9-]{1,63}(?!-)\.)+[A-Za-z]{2,6}))"

Open RFC for Uniform Resource Identifier (URI): Generic Syntax https://tools.ietf.org/html/rfc3986#section-1.2.3 And try to fix the regex as it wants RFC

sgaragagghu commented 4 years ago

Hi, here's a javascript-regex of a domain following RFC952, RFC1034, RFC1123 and RFC2181 ^(?=.{0,255}$)(?=.*\.{1,})([\w](((?=[\w-]*[\w](\.|$))[\w-])|(?=[\w](\.|$))[\w]){0,62}($|\.))*$

Obviously electrum2.privateservers.network belongs to the language.

@echoby Why are you asking for a URI parser if you are taking just a domain?

Waiting for the answer to make PR.

echoby commented 4 years ago

@sgaragagghu Hi. You right. I made mistake writing "URI". This regex must validate only ip and domain.

sgaragagghu commented 4 years ago

@echoby Since RFC 1123 states that each label (the string between the dots) can start with a number too, each label can even be a numeric string, so IPs are accepted by the 'regex'. But even not legal IP will be accepted because they are valid domains, for example:

So if you prefer to keep domains and IPs well divided, i should ignore RFC 1123 and disallow labels to start with a number, this way these strange things will go away and only valid IPs will be accepted (as well as valid domains - excepted RFC 1123 domains).

echoby commented 4 years ago

@sgaragagghu Нou make a mistake, in my opinion. I'll try to explain my logic. rfc952: `

::= ` -> ` ::= *["."] ` -> ` ::= [*[]] ` so ` ::= <[*[]]>*["."<[*[]]>] ` By rfc1123 relaxed only first character of host name: ` ::= <[*[]]>*["."<[*[]]>] ` Folowing this logic your examples is invalid domains. IANA root zones database (https://www.iana.org/domains/root/db) approves my reasoning, I think. In case if you thing that I wrong: Fully ignoring RFC 1123 we will exclude many correct domains. By this reason I will prefer stay liberal and allow labels to start with a number. Except last, it must match one of IANA root zone.
Sergei-Beam commented 4 years ago

@sgaragagghu Hi. Here looks like a Bug in TLD as it should be forbidden to use underscores image

sgaragagghu commented 4 years ago

@Sergei-Beam Because RFC 2181 should allow it for domains, anyway i can forbid it if you want (or restrict it). Just tell me how do you prefer.

Sergei-Beam commented 4 years ago

Because RFC 2181 should allow it for domains, anyway i can forbid it if you want (or restrict it). Just tell me how do you prefer.

@sgaragagghu Let's forbid to use underscores for the Node address field

One more thing which we should allow to use as node address localhost and all one-word addresses as it is not forbidden by RFC image

sgaragagghu commented 4 years ago

@Sergei-Beam One more thing which we should allow to use as node address _localhost_ and all one-word addresses as it is not forbidden by RFC Yes, I forgot to write about it in #84

I'll fix it today.

Sergei-Beam commented 4 years ago

Checked, working as expected