-
Notifications
You must be signed in to change notification settings - Fork 55
Add network.setExtraHeaders command #960
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
base: main
Are you sure you want to change the base?
Conversation
This allows appending, or overriding, HTTP headers for all requests, or for all requests originating from a specific user context or a specific navigable.
Note that the sematics here are to always overwrite. We could support more options e.g. appending or extending additional headers, but there is ofc a complexity tradeoff. |
@jgraham please rebase your PR on main to get the node.js upgrade, which will fix the test failure. |
FWIW, that seems to match what the current CDP API is doing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits. My main question is about iframes. Right now the API works at the navigable level, meaning if you want to apply extra headers for a page and all iframes under this page, you need to setExtraHeaders for the top level traversable and all frame contexts (which might change during the lifecycle of the page).
Should we restrict the API to only accept top level contexts, and in update request headers
resolve the top level context for the related navigable?
1. [=Update request headers=] with |session|, |request| and | ||
|related navigables|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For multisession, this means that the order in which we will apply headers (and potentially overwrite them) is based on the order of sessions in the active BiDi sessions
, not based on the order in which setExtraHeaders commands have been called. I'm just noting it here - especially since multi session support is not implemented anywhere.
Co-authored-by: Julian Descottes <[email protected]>
Co-authored-by: Julian Descottes <[email protected]>
I'm not sure. I think there are use cases where you really only want a request in a specific iframe to contain extra headers (consider e.g. a cross-origin iframe that's being used for auth). On the other hand one could implement that using request interception, and not inheriting does mean that you don't get any extra headers when an iframe first loads, unless you override at the user context level (which is even less specific). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the answer and update. Inheriting headers could be problematic indeed. If needed we could always add an argument later on.
Heh, I think you convinced me that the right starting point is probably inheriting into subframes (and that's easier if we only allow setting it on top-level traversables), since harder cases can always be handled by request interception. Do we know what clients expect here? |
Ah 😅 I haven't found puppeteer or playwright tests covering the iframe scenarios. Will try to build something, in the meantime cc @OrKoN , do you know if in CDP |
CDP sessions are always implicitly scoped to a CDP target (main frame including local sub-frames, worker, out-of-process frames including their local sub-frames), you can send the same command to a different session and it does not change the command params. Puppeteer exposes it one the page level https://pptr.dev/api/puppeteer.page.setextrahttpheaders and that implies all the frames (local and cross-process) as well dedicated workers. I cannot remember off the top of my head if all kinds of workers actually support it but ideally they should. |
This allows appending, or overriding, HTTP headers for all requests, or for all requests originating from a specific user context or a specific navigable.
Preview | Diff