Skip to content

Conversation

@yciabaud
Copy link
Contributor

@yciabaud yciabaud commented Mar 3, 2016

It might be useful to have some information about the peer on the tracker.
Since the HTTP server already provides ip address, I did the same in the websocket server.
To go further, I also provided the HTTP headers in both servers.

My use case is GeoIP and User Agent parsing to have statistics on the network.

@DiegoRBaquero
Copy link
Member

Support +1


params.ip = opts.trustProxy
? socket.upgradeReq.headers['x-forwarded-for'] || socket.upgradeReq.connection.remoteAddress
: (socket.upgradeReq.connection.remoteAddress && socket.upgradeReq.connection.remoteAddress.replace(common.REMOVE_IPV4_MAPPED_IPV6_RE, '')) // force ipv4
Copy link
Member

Choose a reason for hiding this comment

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

Why was the extra socket.upgradeReq.connection.remoteAddress && check needed here? The http case doesn't have that. Can it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was afraid that socket.upgradeReq.connection.remoteAddress could not be exposed in some strange webrtc implementation and we raise an error.

But in that case why not check every member? You're right I will remove it.

@feross
Copy link
Member

feross commented Mar 11, 2016

Actually, come to think of it... any webrtc peer can send a message that contains an addr property and cause any key to be used here: https://github.com/feross/bittorrent-tracker/blob/7406750b747de119ce9025855f945cb6ff998344/lib/server/swarm.js#L16

Can you update your PR to be more specific about each type of peer by adding an explicit type property to params? Then you can replace:

var peer = self.peers[params.addr || params.peer_id]

with this instead:

var id = params.type === 'ws' ? params.peer_id : params.addr
var peer = self.peers[id]

Thoughts?

@yciabaud
Copy link
Contributor Author

I think that adding a type to the params makes sense, some might want to know the source of the peer and having consistent keys seems good too. I agree that addr and port does not have the same signification in WS and other servers but I like the idea to have it available.

I will update this PR soon to add a type param and remove extra check.

Thanks for the review.


if (opts.action === 'announce' || s[0] === '/announce') {
params.action = common.ACTIONS.ANNOUNCE
params.type = common.PEER_TYPES.http
Copy link
Member

Choose a reason for hiding this comment

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

Going to move this out of the if-statement so it's set when the action is 'scrape', too.

feross added a commit that referenced this pull request Mar 12, 2016
Provide IP and HTTP headers in both HTTP and Websocket server
@feross feross merged commit bea1021 into webtorrent:master Mar 12, 2016
@feross
Copy link
Member

feross commented Mar 12, 2016

7.4.0.

feross added a commit that referenced this pull request Mar 12, 2016
@feross
Copy link
Member

feross commented Mar 12, 2016

@yciabaud Added you as a collaborator on this repo 👍 Thanks for the excellent PR! Contributor guidelines are here.

@yciabaud yciabaud deleted the client-infos branch March 12, 2016 11:31
@yciabaud
Copy link
Contributor Author

I was not sure about creating the enum in common, just wanted to follow existing patterns. Thank you

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.

3 participants