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

Fix info_hash url encoding #537

Closed
wants to merge 3 commits into from
Closed

Conversation

YenForYang
Copy link
Contributor

No description provided.

@YenForYang
Copy link
Contributor Author

YenForYang commented Jul 30, 2021

Ah after some testing it looks like even PathEscape sometimes uses + instead of %20. I think resorting to strings.Replace() may be the only way. In addition it looks like PathEscape produces non-working results as well sometimes, so I decided to revert back to QueryEscape. It seems to be most reliable when paired with a strings.ReplaceAll(...,"+","%20")

As an aside, I think it's a really good idea to actually log the full announce url (with query parameters) instead of simply the base in debug mode:

torrent/tracker_scraper.go

Lines 145 to 158 in 1d53c17

me.t.logger.WithDefaultLevel(log.Debug).Printf("announcing to %q: %#v", me.u.String(), req)
res, err := tracker.Announce{
Context: ctx,
HTTPProxy: me.t.cl.config.HTTPProxy,
UserAgent: me.t.cl.config.HTTPUserAgent,
TrackerUrl: me.trackerUrl(ip),
Request: req,
HostHeader: me.u.Host,
ServerName: me.u.Hostname(),
UdpNetwork: me.u.Scheme,
ClientIp4: krpc.NodeAddr{IP: me.t.cl.config.PublicIp4},
ClientIp6: krpc.NodeAddr{IP: me.t.cl.config.PublicIp6},
}.Do()
me.t.logger.WithDefaultLevel(log.Debug).Printf("announce to %q returned %#v: %v", me.u.String(), res, err)

@anacrolix
Copy link
Owner

I'm confused, doesn't QueryEscape already encode spaces to %20? You're welcome to add the logging change if it helps.

@anacrolix
Copy link
Owner

@YenForYang any update on this?

@YenForYang
Copy link
Contributor Author

YenForYang commented Sep 10, 2021

Doesn't look like QueryEscape uses %20 consistently -- i.e. only specific parts. I've been doing an explicit strings.Replace and it works great. This issue provides the explanation I believe: golang/go#4013. The issues referencing that issue also may be relevant, for example: hawkular/hawkular-client-go#12

@YenForYang
Copy link
Contributor Author

I'll submit my current changes, but I doubt you'll like the looks of it. I basically ditched using url.Values and decided to build the string from scratch. I actually find it more readable and it hides less "magic" IMO. I think finer control over building the query is ultimately needed; the url package simply doesn't provide this.

@YenForYang
Copy link
Contributor Author

Just a side note -- We probably won't get more control of the percent encoding with QueryEscape, given this issue: golang/go#13982

@anacrolix
Copy link
Owner

@YenForYang what about #591 instead?

@anacrolix anacrolix closed this Sep 12, 2021
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

2 participants