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

exposing proxySocket on socket to support sniffing messages coming from proxy target #706

Merged
merged 1 commit into from
Sep 30, 2014
Merged

exposing proxySocket on socket to support sniffing messages coming from proxy target #706

merged 1 commit into from
Sep 30, 2014

Conversation

thlorenz
Copy link
Contributor

Basically I need access to the proxySocket in order to listen to messages coming back.

I realize the implementation is a bit hacky, but it's the only way I could think of ATM. I'm open for other suggestions.

Here is an example use case taken from chromium-remote-debugging-proxy

socket.on('data', function ondata(data) {
    if (socket.proxySocket) {
      socket.proxySocket.on('data', function ondata(data) {
        retHybi.parse(data)
      })
     socket.proxySocket = undefined;
    }
})

If there is an existing way to make this work, please let me know as well.

@jcrugzz
Copy link
Contributor

jcrugzz commented Sep 29, 2014

@thlorenz what if we emit the proxySocket in an event on the httpProxy instance to be handled separately. It seems like that could work.

@thlorenz
Copy link
Contributor Author

@jcrugzz that'd be even better cause then I'd know when I can subscribe to its event instead of having to test if its there yet or not.

Do you want me to update the PR to do that? Only question is what's our event emitter? The only one I see inside ws-incoming.js is the server. That'd also be hacky to emit things on the server I guess.

Any other emitters you see?

@thlorenz
Copy link
Contributor Author

Well, I guess the socket is also an emitter, but that'd be almost equally hacky to emit some event from it.

emit the proxySocket in an event on the httpProxy instance

obviously that'd be ideal, but I don't see that we have access to it from within ws-incoming.js.

@jcrugzz
Copy link
Contributor

jcrugzz commented Sep 29, 2014

@thlorenz naming is just terrible here. The server object thats passed in is the actual proxy instance.

@thlorenz
Copy link
Contributor Author

OK, will update the PR tonight (will just rewrite it). .emit('proxy-socket', proxySocket) is ok - I mean the event name?

@jcrugzz
Copy link
Contributor

jcrugzz commented Sep 29, 2014

@thlorenz make it proxySocket for consistency with other events. Other than that, just document the event in the readme and I'll merge when ready :).

@thlorenz
Copy link
Contributor Author

I updated the PR to emit the proxySocket and updated the Readme, but I used a method in the example (hybiParseAndLogMessage) to indicate that you can't just log raw websocket data.
However implementation of that method isn't shown in the example to keep it simple.

Hope that's ok.

- emitted once proxySocket was created and socket was piped into it
- needed to support sniffing messages coming from proxy target
@jcrugzz
Copy link
Contributor

jcrugzz commented Sep 30, 2014

LGTM

jcrugzz added a commit that referenced this pull request Sep 30, 2014
exposing proxySocket on socket to support sniffing messages coming from proxy target
@jcrugzz jcrugzz merged commit 9210b56 into http-party:master Sep 30, 2014
@jcrugzz
Copy link
Contributor

jcrugzz commented Sep 30, 2014

@thlorenz thanks!

@@ -108,6 +108,7 @@ var passes = exports;
return i + ": " + proxyRes.headers[i];
}).join('\r\n') + '\r\n\r\n');
proxySocket.pipe(socket).pipe(proxySocket);
server.emit('proxySocket', proxySocket);
Copy link
Contributor

Choose a reason for hiding this comment

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

I get TypeError: Object false has no method 'emit' on this line in my code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is odd, why would the server be a boolean?
We could add a check to keep in from blowing up:

if (typeof server.emit === 'function') server.emit('proxySocket', proxySocket);

but then it'd still suck cause it would emit only sometimes.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may well be a bug in my code. I create an HTTP server separately, and register proxy methods in its handlers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh i missed that, fixed in 1.5.1. Its because when a callback is used on the web or ws method, it doesn't pass the instance into the function. Sub-ideal as it is always an event emitter instance now but it didn't always use to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

@thlorenz its because of this which should be changed. the callback mechanics may need to be tweaked in web-incoming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jcrugzz thanks for fixing and sorry I missed that myself as well.

@glasser
Copy link
Contributor

glasser commented Dec 29, 2014

I'm a little confused as to how people are making use of this feature (renamed to open event in a later release). Am I missing something or is there no way of figuring out which call to proxy.ws this corresponds to (eg by also including req or something like that as an event argument, like the error event gets)?

(Use case: we're proxying to many backends, and want to place a readable event on the proxySocket so that we can learn if a given backend has had any recent traffic.)

@jcrugzz
Copy link
Contributor

jcrugzz commented Dec 30, 2014

@glasser id be ok with emitting multiple arguments on that event. Submit a PR with an ordering that makes sense.

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.

4 participants