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: ability to send headers via ws connection to browser in node.js environment #9314

Merged
merged 1 commit into from
Nov 24, 2022
Merged

feat: ability to send headers via ws connection to browser in node.js environment #9314

merged 1 commit into from
Nov 24, 2022

Conversation

DudaGod
Copy link
Contributor

@DudaGod DudaGod commented Nov 23, 2022 •
edited
Loading

What kind of change does this PR introduce?

I have browsers pool in some cloud. I want that only users with access will be able to connect to them. So they must provide token through headers. But puppeteer does not allow to send headers when connected to browser by ws connection. So I added this feature.

Closes #7218

@google-cla
Copy link

google-cla bot commented Nov 23, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@DudaGod DudaGod force-pushed the feat.cdp_connect_with_headers branch 2 times, most recently from 0b0ff8d to e79a660 Compare November 23, 2022 13:17
OrKoN
OrKoN reviewed Nov 23, 2022
View reviewed changes
packages/puppeteer-core/src/common/BrowserConnector.ts
@@ -102,7 +101,7 @@ export async function _connectToCDPBrowser(
} else if (browserWSEndpoint) {
const WebSocketClass = await getWebSocketTransportClass();
const connectionTransport: ConnectionTransport =
await WebSocketClass.create(browserWSEndpoint);
await WebSocketClass.create(browserWSEndpoint, headers);
Copy link
Collaborator

Choose a reason for hiding this comment

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

how difficult it is to provide a custom transport instead (already supported feature)? I guess there could be potentially many transport options and I am not sure we should include all of them in the connect options. Although headers is probably one of the most common options and might be worth including.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how difficult it is to provide a custom transport instead (already supported feature)?

It is not difficult because for this all I have to do is copy code from NodeWebSocketTransport - https://github.com/puppeteer/puppeteer/blob/main/packages/puppeteer-core/src/common/NodeWebSocketTransport.ts and add some headers. But it seems that such a need may arise not only for me. So it might be worth supporting it out of the box.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like it have been requested before, at least I found this one #7218. Do you know of any other open or closed issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know of any other open or closed issues?

No, I don't know any more

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add a line Closes #7218 to the end of your commit message/PR description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

OrKoN
OrKoN requested changes Nov 23, 2022
View reviewed changes
Copy link
Collaborator

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

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

In general, LGTM but let's to the following:

  1. see if we can add a test
  2. run npm run docs to regenerate documentation
  3. use headers when the transport is created from a browserURL

@DudaGod DudaGod force-pushed the feat.cdp_connect_with_headers branch 2 times, most recently from da8d892 to 86beabb Compare November 23, 2022 15:05
@DudaGod
Copy link
Contributor Author

DudaGod commented Nov 23, 2022

see if we can add a test

Before send a PR I tried to do it. But I didn't find a quick way to check that the appropriate headers were sent when connecting to the browser by websocket. Moreover, I did not find any similar tests. I can dig deeper if without tests we can't merge it.

@DudaGod DudaGod force-pushed the feat.cdp_connect_with_headers branch 2 times, most recently from faabad0 to e7883ab Compare November 24, 2022 09:15
@DudaGod DudaGod force-pushed the feat.cdp_connect_with_headers branch from e7883ab to dbea049 Compare November 24, 2022 09:21
@DudaGod DudaGod changed the title feat: ability to send headers via ws connection to browser feat: ability to send headers via ws connection to browser in node.js environment Nov 24, 2022
@OrKoN OrKoN enabled auto-merge (squash) November 24, 2022 09:23
@OrKoN OrKoN disabled auto-merge November 24, 2022 09:59
@OrKoN OrKoN merged commit 937fffa into puppeteer:main Nov 24, 2022
@release-please release-please bot mentioned this pull request Nov 24, 2022
Merged
@DudaGod
Copy link
Contributor Author

DudaGod commented Nov 24, 2022

Thank you for merge it. Can you tell me when the new version of puppeteer-core will be released?

@DudaGod
Copy link
Contributor Author

DudaGod commented Nov 24, 2022

oh, I see that new PR with bump version was created - #9322. So it looks like that new version will be released today.

@OrKoN
Copy link
Collaborator

OrKoN commented Nov 24, 2022

@DudaGod it will take a week or two before release as try to batch multiple changes in one release. You can subscribe to #9322 to get notified when a release happens.

@DudaGod
Copy link
Contributor Author

DudaGod commented Dec 7, 2022

@OrKoN hi. Is there any new information about the release date? Thank you.

@OrKoN
Copy link
Collaborator

OrKoN commented Dec 7, 2022

@DudaGod probably in the next few days

OrKoN pushed a commit that referenced this pull request Dec 7, 2022
@release-please
chore: release main (#9322)
931d4fc 
🤖 I have created a release *beep* *boop*
---


<details><summary>puppeteer: 19.4.0</summary>

##
[19.4.0](puppeteer-v19.3.0...puppeteer-v19.4.0)
(2022-12-07)


### Features

* **chromium:** roll to Chromium 109.0.5412.0 (r1069273)
([#9364](#9364))
([1875da6](1875da6)),
closes [#9233](#9233)


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * puppeteer-core bumped from 19.3.0 to 19.4.0
</details>

<details><summary>puppeteer-core: 19.4.0</summary>

##
[19.4.0](puppeteer-core-v19.3.0...puppeteer-core-v19.4.0)
(2022-12-07)


### Features

* ability to send headers via ws connection to browser in node.js
environment
([#9314](#9314))
([937fffa](937fffa)),
closes [#7218](#7218)
* **chromium:** roll to Chromium 109.0.5412.0 (r1069273)
([#9364](#9364))
([1875da6](1875da6)),
closes [#9233](#9233)
* **puppeteer-core:** keydown supports commands
([#9357](#9357))
([b7ebc5d](b7ebc5d))


### Bug Fixes

* **puppeteer-core:** avoid type instantiation errors
([#9370](#9370))
([17f31a9](17f31a9)),
closes [#9369](#9369)
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
@DudaGod DudaGod mentioned this pull request Dec 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OrKoN OrKoN OrKoN approved these changes

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

Successfully merging this pull request may close these issues.

Feature Request: Connect Options - Allow Authorization Header
2 participants
@DudaGod @OrKoN