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

Add network request interception #429

Merged
merged 1 commit into from
Aug 17, 2023
Merged

Add network request interception #429

merged 1 commit into from
Aug 17, 2023

Conversation

jgraham
Copy link
Member

@jgraham jgraham commented May 18, 2023

Add a feature for intercepting network requests.

Network request incercepts are created using the network.addIntercept command. This takes a URL pattern describing which resources to match, and a list of phases, which can be "beforeRequestSent", "responseStarted", or "authRequired". It returns a handle that can later be used with network.removeIntercept to unregister the intercept.

When a request is intercepted, the corresponding network events get their isBLocked property set to true, and the client has to respond with a command in order to unblock the request.

Blocking only occurs if a client is also subscribed to the relevant network events.

Each request inteception phase has a corresponding event, and one or more commands to continue the request from that point.

At the beforeRequestSent phase, the client may respond with continueRequest, which allows altering the request properties, but continues processing the request via the normal network stack. It may also respond with network.provideResponse, which allows providing a complete response body and prevents all further processing.

At the afterResponse stage, the client may respond with network.continueResponse, which allows altering the response status and headers, or network.provideResponse, which overrides the network response with the client-provided response.

At either phase the client may response with network.failRequest to cause the fetch to end with a network error.

At the authRequired phase, the client must respond with network.continueWithAuth, to handle the request for credentials.

Credentials can also be provided in the responseStarted phase (when the response has an approproate status code and authentication header), in which case those credentials will be used and a network.authRequired event will not be sent.


Preview | Diff

Base automatically changed from fix_references to master May 19, 2023 07:39
@jgraham jgraham marked this pull request as ready for review May 22, 2023 13:53
jimevans

This comment was marked as resolved.

index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
@whimboo
Copy link
Contributor

whimboo commented May 26, 2023

When a request is intercepted, the corresponding network events get their isBLocked property set to true, and the client has to respond with a command in order to unblock the request.

Blocking only occurs if a client is also subscribed to the relevant network events.

How would that work when multiple clients are actually connected at the same time and are subscribed with the same interception settings? Do each of them have to respond before the request is unblocked? Nevertheless I can see that different clients might override the behavior as defined by others.

@jgraham jgraham force-pushed the network_interception branch from f42f0c5 to 1c9a77f Compare May 26, 2023 15:08
@jgraham
Copy link
Member Author

jgraham commented May 26, 2023

How would that work when multiple clients are actually connected at the same time and are subscribed with the same interception settings?

In the current draft we basically iterate over all the sessions in order, and block every time a session has an intercept. That means each one sees the state as set by the previous sessions that modified the request/response.

It's not wonderful, but I think it's at least reasonable. We could also check what web extensions do in this case.

@thiagowfx
Copy link
Member

thiagowfx commented Jun 6, 2023

Is there any chance this PR could be broken into smaller ones? I find it difficult to review with so many changes at once.

Edit: I reviewed it anyway, but in the future it would be nicer to split PRs into smaller ones, when feasible (e.g. splitting by command, or sending pre-factoring changes in its own PR).

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved

<pre class="cddl remote-cddl">
network.AuthCredentials = {
type: "password",
Copy link
Member

Choose a reason for hiding this comment

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

Should this be type: "credentials"? When one sees "password", it sounds like there's only a single "password" field. However, in this case, there's also a username field. Compare e.g. with regex:

script.RegExpLocalValue = {
  type: "regexp",
  value: script.RegExpValue,
}

Or with date:

script.DateLocalValue = {
  type: "date",
  value: text
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would tend to agree in changing it to credentials. But also there could be authentication methods which are password-less and require some other kind of authenticated information. Would that require actually different types to be defined here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but this isn't a script type, it's an authentication type. And in particular it's for password-based authentication. Non-password auth schemes still require some kind of credentials e.g. a token or a certificate or whatever. So calling this "password" makes more sense to me.

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@jgraham
Copy link
Member Author

jgraham commented Jun 14, 2023

Is there any chance this PR could be broken into smaller ones? I find it difficult to review with so many changes at once.

It is a big change, but I don't know what a useful split would look like. Splitting into individual commands wouldn't show the overall structure of the API or lifecycle. I could, I suppose have split out the infra stuff that allows events to block, without actually defining any blocking events, but I don't know that would have made a large difference in this case.

Copy link
Member

@thiagowfx thiagowfx left a comment

Choose a reason for hiding this comment

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

Other than the open comments (some of them FYI only, non-actionable), it seems to be this is feasible to implement in Chromium.

Ensure to get approval from either @sadym-chromium or @OrKoN as well, though.

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

This is indeed a lot of modifications which make it hard to review. But I agree it's somewhat hard to separate out. Thanks for working on it @jgraham!! I left a couple of comments to clarify and fix. In some cases I might not have the full understanding yet how it would work, so excuse maybe silly questions. :)


<pre class="cddl local-cddl">
network.AuthChallenge = {
scheme: text,
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Instead of a free form text shouldn't we better maintain a list of supported authentication schemes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well the problem here is that https://datatracker.ietf.org/doc/html/rfc7235#section-4.1 makes the scheme open-ended. Maybe we should ignore all but certain schemes? It's not quite clear to me how this works in practice if you send a random scheme to a browser.

<pre class="cddl local-cddl">
network.AuthChallenge = {
scheme: text,
realm: text,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this could cause confusion with the script.Realm type.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point, but there's no other idiomatic term to use here, is there?

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, I think we just need to keep the term that's used everywhere for this.

index.bs Outdated

1. If |response|'s [=response/status=] is 401, let |header name| be
`WWW-Authenticate`. Otherwise if |response|'s [=response/status=] is 407, let
|header name| be `Proxy-Authenticate`. Otherwise return null.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that we most likely can ignore 511 Network Authentication Required given that it is barely used?

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't use HTTP Auth anyway, so it's not relevant here.


<pre class="cddl remote-cddl">
network.AuthCredentials = {
type: "password",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would tend to agree in changing it to credentials. But also there could be authentication methods which are password-less and require some other kind of authenticated information. Would that require actually different types to be defined here?

1. Let |timestamp| be a [=time value=] representing the current date and time in UTC.

1. If |intercepts| is not [=list/empty=], let |is blocked| be true, otherwise
let |is blocked| be false.
Copy link
Contributor

Choose a reason for hiding this comment

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

With intercepts only set when it's not an empty list do we really need this flag? The check if intercepts exist is also not expensive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we really need it? No. But it does seem helpful to me to have a simple boolean that code can check, because basically all network response handling code will need to have a "this is blocked" vs "this is not blocked" check, so I think it's worth spending a few bytes on making it more clear.

request: network.Request,
?credentials: network.AuthCredentials,
?headers: [*network.Header],
?reasonPhrase: text,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we better use statusMessage (similar to statusCode) instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

reasonPhrase is taken directly from https://httpwg.org/specs/rfc7230.html#status.line but I don't really mind either way.

index.bs Outdated Show resolved Hide resolved
with [=error code=] [=invalid argument=].

1. If |command parameters| "<code>action</code> is "<code>cancel</code>", set
|response|'s authentication credentials <!--TODO: link--> to
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you going to fix this TODO and the other below?

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 need to write the patch to fetch for there to be something to link to here.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated

1. Let |response| be a new [=network error=].

1. TODO: Allow setting the precise kind of error
Copy link
Contributor

Choose a reason for hiding this comment

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

If that's a follow-up maybe you can immediately file a new issue and reference it here?

index.bs Show resolved Hide resolved
index.bs Outdated
Comment on lines 3466 to 3725
? value: text,
? binaryValue: [ uint ]
? binaryValue: [ *(0..255) ]
Copy link
Member

Choose a reason for hiding this comment

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

Should Cookie make value and binaryValue mutually exclusive, like Header does?

@thiagowfx
Copy link
Member

thiagowfx commented Jul 3, 2023

I am intending to start the implementation of this shortly (in Chrome). When are you intending to submit this? Ideally I'd start the implementation once the PR is merged, to reduce the probability of throwaway work given potential changes in the meantime.

@jgraham jgraham force-pushed the network_interception branch from b0f7c9f to 0470741 Compare July 4, 2023 18:14
index.bs Outdated
A [=BiDi session=] has an <dfn>blocked request map</dfn>, used to track the
requests which are actively being blocked. It is an [=map=] between request id
and a [=tuple=] with fields <code>request</code>, <code>phase</code>, and
<code>response</code>. It is initially empty. It's
Copy link
Member

Choose a reason for hiding this comment

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

nit: extra It's

@jgraham jgraham force-pushed the network_interception branch from 0470741 to cc4ea59 Compare July 5, 2023 13:34
@jgraham jgraham changed the base branch from master to network_bytes_value July 5, 2023 13:45
@jrandolf-2 jrandolf-2 force-pushed the network_bytes_value branch from efa3987 to 664d2df Compare July 6, 2023 11:10
index.bs Outdated
Comment on lines 4630 to 4631
1. If |search| does not [=string/starts with|start with=] U+003F (?), then
append "<code>?</code>" to |pattern url|.
Copy link
Contributor

Choose a reason for hiding this comment

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

(just thinking out loud...) For the edge case where users want to use query parameters named ?, they might pass ?=1, then we won't add the ?. Meaning the the user won't have the expected search string in the end...

But it's quite hard to guess what the user really meant in that case, and using other default behaviors (eg always adding ?, or never adding ?) also can lead to confusing situations where it will be hard to understand the issue both for us and the users. In summary I think your current approach is the best compromise.

index.bs Outdated

1. Append "<code>:</code>" to |pattern url|.

1. If |scheme| [=is special=], append "<code>//</code>" to |pattern url|.
Copy link
Contributor

Choose a reason for hiding this comment

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

For non special schemes (let's take git for instance), the URL will be parsed differently depending whether we add // or not:

  • for git://github.com/whatwg/url.git, the host will be "github.com"
  • for git:github.com/whatwg/url.git the host is empty

With the current API users can't really specify the first variant if they use URLPatternPattern.
But maybe it doesn't matter? I imagine we are not actually interested in handling all possible URLs, but only whatever can be intercepted, so special schemes + a select few (eg data: ?). Do we need to surface this somehow in the spec?

index.bs Outdated
1. If |pattern|["<code>hostname</code>"] is the empty string, return [=error=]
with [=error code=] [=invalid argument=].

1. If |scheme| is "<code>file</code>" return [=error=] with [=error code=]
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the file scheme can also support hostnames, depending on how you use it.

  • file:/path -> no hostname
  • file:///path-> empty hostname
  • file://hostname/path -> hostname is "hostname"

With the current spec, file patterns defined using URLPatternPattern will all start with file:///(or file:// if users don't specify a path, but the basicURLParser should treat that as if it was file:///). Maybe we should lift this restriction?

@juliandescottes
Copy link
Contributor

juliandescottes commented Aug 1, 2023

@thiagowfx wanted to do a quick check about wdspec tests. Do you plan to submit tests soon? Otherwise I could provide the basic scaffolding for the network module (which has no command so far) + add intercept validation tests.

Let me know how you prefer to proceed.

(sorry for posting the question twice... Github was showing my comment as not submitted and it was not in the main feed either...)

@thiagowfx
Copy link
Member

@juliandescottes Thanks for asking! I'll be OOO for a couple of days starting this Friday. Therefore, unless I manage to write them until tomorrow EOD, feel free to scaffold them.

@juliandescottes
Copy link
Contributor

@juliandescottes Thanks for asking! I'll be OOO for a couple of days starting this Friday. Therefore, unless I manage to write them until tomorrow EOD, feel free to scaffold them.

Created web-platform-tests/wpt#41291 with my current tests

index.bs Outdated Show resolved Hide resolved
Comment on lines +4658 to +4685
1. If |url|'s [=url/scheme=] [=is special=] and |url|'s
[=url/scheme=]'s [=default port=] is not null, and |url|'s [=url/port=] is
null or is equal to [=url/scheme=]'s [=default port=]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I still feel like if no port is provided, we should match any port (ie have some has port logic). We do that for hostname and protocol, so maybe it would make sense here as well?

On top of that, we can have some odd scenarios when users did not provide a protocol. Since we default to http in that case, the algorithm will behave unexpectedly in some cases (eg if provided port is 80). Maybe we should also take has protocol into account in this condition?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe default to https those days?

Copy link
Contributor

@juliandescottes juliandescottes Aug 3, 2023

Choose a reason for hiding this comment

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

Should we maybe default to https those days?

Note that in theory, http here really only acts as a placeholder so that we can build a URL that we can parse later. You will still have the has protocol flag to false, which means the pattern will not attempt to match on the protocol. However since this is a special scheme, it does have some side effects:

  • has a default port
  • path defaulting to "/"

If we check has protocol in the condition as suggested above, then using http or https should be identical. And if not, https just moves the edge case to scenarios where users provide a URLPatternPattern such as { port: 443 } which would start matching http or ws requests on port 80, ftp requests on port 21, which is just as unexpected.

Copy link
Contributor

@juliandescottes juliandescottes left a comment

Choose a reason for hiding this comment

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

While I might still have comments as I go forward with the implementation, IMO this is in a good enough state to land and then iterate with follow ups. Happy to keep providing feedback here, but maybe we should already merge the current version?

@thiagowfx
Copy link
Member

While I might still have comments as I go forward with the implementation, IMO this is in a good enough state to land and then iterate with follow ups. Happy to keep providing feedback here, but maybe we should already merge the current version?

+1 - I am happy to approve to unblock/land this if warranted

There will definitely be more issues to be found here; personally I have only deeply reviewed AddIntercept and RemoveIntercept. I am hoping to have more comments as our implementation keeps going.

Comment on lines +4714 to +4740
1. If |url pattern|'s search is not null and is not equal to |url|'s
[=url/query=], return false.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to check if we are OK with this scenario as I stumbled on it while writing tests.

If I use a URLPatternString based on "http://example.com", the query returned by basicURLParser will be null, meaning that url pattern's search here will be null as well. So it will match URLs such as http://example.com?, http://example.com?hello=hi etc...

Which also means there is no way to say "match only URLs with a null search". Before starting the implementation I assumed that using a URLPatternString would be close to requesting an exact match, but it's not the case. Is this behavior what we want here?

Comment on lines +4711 to +4737
1. If |url pattern|'s pathname is not null and is not equal to the result of
running the [=URL path serializer=] with |url|, return false.
Copy link
Contributor

@juliandescottes juliandescottes Aug 4, 2023

Choose a reason for hiding this comment

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

Another interesting scenario, for a URLPatternPattern such as { protocol: "http" }, since this is a special scheme, the basic URL parser will set the path to a single segment empty string [""], meaning we will have a non-null pathname set to /. Such a URLPatternPattern will therefore match any URL using the http protocol and with an empty path. Meaning http://abc.com/?search will match, but http://abc.com/path?search will not.

I feel like it's unexpected, and we probably want a has path logic here?

Edit: well not "here" but rather in the parser algorithm, when we decide whether pathname should be set to null or not.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Network request interception update.

The full IRC log of that discussion <AutomatedTester> topic: Network request interception update
<AutomatedTester> github: https://github.com//pull/429
<AutomatedTester> jgraham: this is mostly an update. I think the initial PR of this is ready to land
<AutomatedTester> ... we know the URL matching is not 100% done yet but it's heading in the right direction
<AutomatedTester> ... my recommendation is we land the current PR and then any issues get followed up in a future PR
<JimEvans> q+
<orkon> q+
<AutomatedTester> ack next
<AutomatedTester> Jim Evans: I am happy with this process so we get as much landed now and future URL matching in a future PR
<jgraham> q+
<AutomatedTester> ack next
<jdescottes> q+
<AutomatedTester> orkon: on our side Diego is working on this but he is OOO this week. We were hoping to do some prototyping but it is looking good and we can get it landed sooner
<AutomatedTester> ack next
<AutomatedTester> jgraham: regarding the network response bodies I think that would be a good topic for TPAC. I didn't think it was a priority at the beginning but we can definitely follow up
<AutomatedTester> ack next
<AutomatedTester> jdescottes: Diego looked at this before going on PTO and he seemed happy with getting this landed as is
<AutomatedTester> q?
<orkon> Diego -> Thiago
<whimboo> s/Diego/Thiago

@jgraham jgraham force-pushed the network_interception branch from 4008cf5 to 4e18076 Compare August 17, 2023 09:02
@jgraham jgraham mentioned this pull request Aug 17, 2023
@jgraham jgraham force-pushed the network_interception branch from 4e18076 to 97ab136 Compare August 17, 2023 09:13
Add a feature for intercepting network requests.

Network request incercepts are created using the network.addIntercept
command. This takes a URL pattern describing which resources to match,
and a list of phases, which can be "beforeRequestSent",
"responseStarted", or "authRequired". It returns a handle that can
later be used with network.removeIntercept to unregister the
intercept.

When a request is intercepted, the corresponding network events get
their `isBLocked` property set to true, and the client has to respond
with a command in order to unblock the request.

Blocking only occurs if a client is also subscribed to the relevant
network events.

Each request inteception phase has a corresponding event, and one or
more commands to continue the request from that point.

At the beforeRequestSent phase, the client may respond with
continueRequest, which allows altering the request properties, but
continues processing the request via the normal network stack. It may
also respond with network.provideResponse, which allows providing a
complete response body and prevents all further processing.

At the afterResponse stage, the client may respond with
network.continueResponse, which allows altering the response status and
headers, or network.provideResponse, which overrides the network response with
the client-provided response.

At either phase the client may response with network.failRequest to
cause the fetch to end with a network error.

At the authRequired phase, the client must respond with
network.continueWithAuth, to handle the request for credentials.

Credentials can also be provided in the responseStarted phase (when
the response has an approproate status code and authentication
header), in which case those credentials will be used and a
network.authRequired event will not be sent.
@jgraham jgraham force-pushed the network_interception branch from 97ab136 to 88e622b Compare August 17, 2023 10:14
@jgraham jgraham dismissed thiagowfx’s stale review August 17, 2023 10:16

Agreement in the meeting to merge

@jgraham jgraham merged commit 0527e75 into main Aug 17, 2023
@jgraham jgraham deleted the network_interception branch August 17, 2023 10:17
thiagowfx pushed a commit to GoogleChromeLabs/chromium-bidi that referenced this pull request Aug 20, 2023
@thiagowfx
Copy link
Member

thiagowfx commented Sep 15, 2023

One question about the spec (if this is not a good medium to ask, let me know if I should open a new bug):

A BiDi session has a blocked request map, used to track the requests which are actively being blocked. It is an map between request id and a struct with fields request, phase, and response. It is initially empty.

Translating to pseudo typescript code, this would roughly look like:

  /** A map to track the requests which are actively being blocked. */
  readonly #blockedRequestMap = new Map<
    Network.Request,
    {
      request: Network.Request;
      phase: Network.InterceptPhase;
      response: Network.ResponseData;
    }
  >();

The question is: what is the difference between the map key (the request ID) and the first property of the map value (the request)? Are these fields the same? If so, isn't this redundant? If not, then to what "Request" does the value refer to?

(reference: https://w3c.github.io/webdriver-bidi/#type-network-Request)

@jgraham
Copy link
Member Author

jgraham commented Sep 19, 2023

This isn't the best place to ask (maybe just IRC/Matrix for this kind of question, or file an new issue), but the request here is https://fetch.spec.whatwg.org/#requests

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.

9 participants