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.

Sign up for GitHub

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

Jump to bottom

feat(browserServer): forward local ports #6375

Merged
merged 9 commits into from May 25, 2021
Merged

feat(browserServer): forward local ports #6375

merged 9 commits into from May 25, 2021

Conversation

mxschmitt
Copy link
Member

No description provided.

@mxschmitt mxschmitt force-pushed the feature/browser-server-forward-local-ports branch 3 times, most recently from 691db25 to 23e9297 Compare May 6, 2021 14:46
@mxschmitt mxschmitt marked this pull request as ready for review May 6, 2021 14:57
dgozman
dgozman reviewed May 7, 2021
View reviewed changes
Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

A few comments, in addition to moving stuff from Browser to Playwright in the protocol.

package.json Outdated Show resolved Hide resolved
src/client/forwardingProxy.ts Outdated Show resolved Hide resolved
src/browserServerImpl.ts Outdated Show resolved Hide resolved
src/dispatchers/browserDispatcher.ts Outdated Show resolved Hide resolved
tests/forwardingProxy.spec.ts Outdated Show resolved Hide resolved
src/client/forwardingProxy.ts Outdated Show resolved Hide resolved
src/client/forwardingProxy.ts Outdated Show resolved Hide resolved
src/client/forwardingProxy.ts Outdated Show resolved Hide resolved
src/client/forwardingProxy.ts Outdated Show resolved Hide resolved
src/client/forwardingProxy.ts Outdated Show resolved Hide resolved
@mxschmitt mxschmitt force-pushed the feature/browser-server-forward-local-ports branch 5 times, most recently from 29a5bdb to a421fe4 Compare May 10, 2021 14:51
@mxschmitt mxschmitt requested a review from dgozman May 10, 2021 15:03
dgozman
dgozman reviewed May 10, 2021
View reviewed changes
src/utils/network.ts Outdated Show resolved Hide resolved
@mxschmitt mxschmitt force-pushed the feature/browser-server-forward-local-ports branch 3 times, most recently from fe68b9b to 025829d Compare May 12, 2021 19:08
@mxschmitt mxschmitt added the CQ1 label May 14, 2021
@mxschmitt mxschmitt force-pushed the feature/browser-server-forward-local-ports branch 2 times, most recently from 527c221 to c9ec6c3 Compare May 14, 2021 11:49
@mxschmitt mxschmitt added CQ1 and removed CQ1 labels May 14, 2021
@mxschmitt mxschmitt force-pushed the feature/browser-server-forward-local-ports branch from a4e2852 to bd46c01 Compare May 17, 2021 13:17
@mxschmitt mxschmitt added CQ1 and removed CQ1 labels May 17, 2021
@mxschmitt mxschmitt force-pushed the feature/browser-server-forward-local-ports branch from bd46c01 to 1f15b4d Compare May 21, 2021 15:08
@mxschmitt mxschmitt force-pushed the feature/browser-server-forward-local-ports branch from 1f15b4d to 694336e Compare May 21, 2021 15:09
@mxschmitt mxschmitt added CQ1 and removed CQ1 labels May 21, 2021
@mxschmitt mxschmitt force-pushed the feature/browser-server-forward-local-ports branch from 694336e to df0e178 Compare May 21, 2021 15:28
dgozman
dgozman reviewed May 21, 2021
View reviewed changes
Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

I didn't read the socks implementation thoroughly yet, but I will follow up.

src/client/playwright.ts
@@ -59,6 +60,8 @@ export class Playwright extends ChannelOwner<channels.PlaywrightChannel, channel

this._selectorsOwner = SelectorsOwner.from(initializer.selectors);
this.selectors._addChannel(this._selectorsOwner);

this._channel.on('incomingSocksSocket', ({socket}) => SocksSocket.from(socket));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's save the ports on Playwright object, so that we can validate dstPort on the client side as well.

src/server/socksSocket.ts
import { SocksProxyServer, SocksConnectionInfo, SocksInterceptedHandler } from './socksServer';
import { LaunchOptions } from './types';

export class BrowserServerPortForwardingServer extends EventEmitter {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd inline this file into browserServerImpl.ts, and merge SocksSocket with SocksSocketDispatcher right away.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have the assumption that this get's also in the future used in playwrightclient, so I would keep it separate for now.

src/browserServerImpl.ts Outdated Show resolved Hide resolved
src/server/socksSocket.ts Outdated Show resolved Hide resolved
src/server/socksSocket.ts Outdated Show resolved Hide resolved
src/server/socksServer.ts Outdated Show resolved Hide resolved
tests/portForwardingServer.spec.ts Outdated Show resolved Hide resolved
tests/portForwardingServer.spec.ts Outdated Show resolved Hide resolved
tests/portForwardingServer.spec.ts Outdated Show resolved Hide resolved
tests/portForwardingServer.spec.ts Outdated Show resolved Hide resolved
dgozman
dgozman approved these changes May 24, 2021
View reviewed changes
src/dispatchers/socksSocketDispatcher.ts Outdated Show resolved Hide resolved
src/server/socksSocket.ts Outdated Show resolved Hide resolved
src/server/socksSocket.ts Outdated Show resolved Hide resolved
src/server/socksServer.ts Outdated
if (this._authenticated)
this._phase = ConnectionPhases.REQ_CMD;
else
this._phase++;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite like incrementing, can we explicitly assign the new phase?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah we can do that. Seems then clearer.

src/server/socksServer.ts Outdated
case ConnectionPhases.METHODS: {
assert(this._authMethods);
chunk.copy(this._authMethods, 0, i, i + chunk.length);
assert(chunk.includes(SOCKS_AUTH_METHOD.NO_AUTH));
Copy link
Contributor

Choose a reason for hiding this comment

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

chunk.includes -> this._authMethods.includes ?

Copy link
Member Author

Choose a reason for hiding this comment

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

should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was not very clear. I think that chunk buffer may include a lot more than just methods, so the chance of 00 appearing in chunk is large, and that defeats the purpose of "assert that we have 'no auth required' in the methods".

src/server/socksServer.ts
case ConnectionPhases.REQ_ATYP:
this._phase = ConnectionPhases.REQ_DSTADDR;
this._addressType = chunk[i];
assert(this._addressType in SOCKS_ATYP);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should turn all assert() code into socket closure?

src/server/socksServer.ts Outdated
this._methodsp += minLen;
i += minLen;
if (this._methodsp === this._authMethods.length) {
this._phase = ConnectionPhases.VERSION;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we go back to version?

Copy link
Member Author

Choose a reason for hiding this comment

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

refactored the handling, should be more readable now. changes:

  • introduced a readByte method
  • don't jump between the phases

src/server/socksServer.ts
dstSocket.on('error', (err: NodeJS.ErrnoException) => writeSocksSocketError(this._socket, String(err)));
dstSocket.on('connect', () => {
this._socket.write(BUF_REP_INTR_SUCCESS);
this._socket.pipe(dstSocket).pipe(this._socket);
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting! So we are supposed to pipe even the "pass-through" sockets? I was under impression we can just reply with the "BND == DST" and let the client connect on its own, but maybe I misread that.

@mxschmitt mxschmitt merged commit 3f43db5 into microsoft:master May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgozman dgozman dgozman approved these changes

@kumaraditya303 kumaraditya303 kumaraditya303 left review comments

Assignees
No one assigned
Labels
CQ1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mxschmitt @dgozman @kumaraditya303