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

ethstats: fix URL parser for '@' or ':' in nodename #21640

Merged
merged 3 commits into from May 25, 2021

Conversation

meowsbits
Copy link
Contributor

Fixes the case (example below) where the value passed to --ethstats flag would be parsed wrongly because the nodename and/or password value contained "special" characters @ or :.

--ethstats "ETC Labs Metrics @meowsbits":mypass@ws://mordor.dash.fault.dev:3000

Stats server unreachable err="malformed ws or wss URL"

@holiman
Copy link
Contributor

holiman commented Sep 30, 2020

Hm, quirky that we didn't use the proper syntax from the beginning, with <protocol>username:password@<location>:

a = new URL("http://userna  me@baz:password@example.com")
URL
hash: ""
host: "example.com"
hostname: "example.com"
href: "http://userna%20%20me%40baz:password@example.com/"
origin: "http://example.com"
password: "password"
pathname: "/"
port: ""
protocol: "http:"
search: ""

Comment on lines +25 to +28
{
url: `name:@ws://mordor.dash.fault.dev:3000`,
node: "name", pass: "", host: "ws://mordor.dash.fault.dev:3000",
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name@ws://mordor.dash.fault.dev should also work (without the :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least if it worked before...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger. Not sure if it did before... but it does now 😂

Addresses review ethereum#21640 (review)

Signed-off-by: meows <b5c6@protonmail.com>
meowsbits added a commit to etclabscore/core-geth that referenced this pull request Sep 30, 2020
Addresses review ethereum/go-ethereum#21640 (review)

Signed-off-by: meows <b5c6@protonmail.com>
@fjl
Copy link
Contributor

fjl commented Oct 5, 2020

Damn. This is one of these cases where we should really use the stdlib URL parser. It can handle this.

@holiman
Copy link
Contributor

holiman commented Oct 6, 2020 via email

@karalabe
Copy link
Member

karalabe commented Oct 6, 2020

I think we can attempt to change it to the standard format. I don't think it's common for anyone to explicitly specify ws or wss. That was important to me to make it work without having to care about SSL or not. So if we assume it's not a used feature, we can move the position of ws:// from the middle out to the front, as long as we keep supporting prefix-less versions. That part is IMHO important because it's rare that stats pages are set up correctly with domains and SSL, so better leave the complexity to be auto-discovered vs dump it onto the user.

@meowsbits
Copy link
Contributor Author

meowsbits commented Oct 6, 2020

Another issue is the user info field. Currently the bastard pattern supports arbitrary unencoded user strings, like

"Foundation Benchmarking(mon02)":mypass@example.com:8888
"2Miners.com Ethereum SOLO 🇩🇪":mypass@example.com:8888

(with spaces and delimiters) and stuff. RFC3986 (and the standard lib url.Parse) will require percent-encoding for reserved characters.

Handling this on the backend (doing the encoding before parsing) is obviously possible, but will add complexity.

Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, and sorry for the very delayed review!

@fjl fjl removed their assignment May 25, 2021
@fjl fjl added this to the 1.10.4 milestone May 25, 2021
@fjl fjl merged commit 10962b6 into ethereum:master May 25, 2021
@fjl fjl changed the title Fix/ethstats parse url ethstats: fix URL parsers for '@' or ':' in nodename May 25, 2021
@fjl fjl changed the title ethstats: fix URL parsers for '@' or ':' in nodename ethstats: fix URL parser for '@' or ':' in nodename May 25, 2021
reds pushed a commit to reds/go-ethereum that referenced this pull request Aug 28, 2021
…um#21640)

Fixes the case (example below) where the value passed
to --ethstats flag would be parsed wrongly because the
node name and/or password value contained the special
characters '@' or ':'

    --ethstats "ETC Labs Metrics @meowsbits":mypass@ws://mordor.dash.fault.dev:3000
i-norden pushed a commit to cerc-io/go-ethereum that referenced this pull request Sep 10, 2021
…um#21640)

Fixes the case (example below) where the value passed
to --ethstats flag would be parsed wrongly because the
node name and/or password value contained the special
characters '@' or ':'

    --ethstats "ETC Labs Metrics @meowsbits":mypass@ws://mordor.dash.fault.dev:3000
atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
…um#21640)

Fixes the case (example below) where the value passed
to --ethstats flag would be parsed wrongly because the
node name and/or password value contained the special
characters '@' or ':'

    --ethstats "ETC Labs Metrics @meowsbits":mypass@ws://mordor.dash.fault.dev:3000
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants