feat(har): record remote IP:PORT and SSL details by rwoll · Pull Request #6631 · microsoft/playwright

Ross Wollman (rwoll) · 2021-05-18T07:46:35Z

Updates in the HAR capture Log Entries:

serverIPAddress: (string) This now gets populated.
_serverPort: (number) This now gets populated. There isn't an
official field according to this w3 doc (and I didn't see
a suitable field in a HAR download from Chromium DevTools).
I used the field name from WK WebInspector HARBuilder.js.
_securityDetails: non-standard info that is helpful in making
assertions about cert expiries.

This also introduces new APIs on network.Response exposing the
new info.

NB: At first these APIs were synchronous and populated with info that is
readily available at the beginning of the Response creation, but WK does
not emit IP address info until _loadingFinished, so the API's were all
turned into Promise-based approaches.

Closes#6624.

Ross Wollman (rwoll)

/cc Joel Einbinder (@JoelEinbinder) 🎉

Ross Wollman (rwoll) · 2021-05-19T04:20:04Z

Joel Einbinder (@JoelEinbinder) I cleaned up the types (including deleting some of the awkward code dealing with the non-standard zero values.) I also added a comment to the WebKit address parsing code with some examples from my testing.

I haven't yet figured out the appropriate changes to emit the Response at the correct time after _onLoadingFinished where the IP address details are available for WebKit so I left the signal barrier in place in the HAR code. I'll remove that once we sort out dealing Response emission timing (directly or indirectly through guarding the newly added fields behind a Promise api). If we can avoid adding more external Promises to Response, I agree that would be much nicer.

Ross Wollman (rwoll) · 2021-05-20T21:20:37Z

Joel Einbinder (@JoelEinbinder) Here's the latest pass! Promise-based APIs because of WK. See #6631 (comment) with updated description.

Ross Wollman (rwoll) · 2021-05-21T19:26:42Z

Joel Einbinder (@JoelEinbinder): This should now pass on all the browser+platform variants. (Thanks platform-dependent WK networking! 😉 )

Let me know your feedback and I will make any necessary adjustments.

It's also worth skimming through #6695 to what additional information will be coming in next.

This changeset exposes * `not-valid-before` * `not-valid-after` * `subject-name` * `issuer-name` on GTlsCertificate provided by the underlying TLS Backend. In order to make use of these changes, see the related [glib-networking MR][glib-networking]. This change aims to help populate more of the [`Certificate`][wk-cert] info in the WebKit Inspector Protocol on Linux. This changeset stems from work in Microsoft Playwright to [add more info into its HAR capture][pw] generated from the Inspector Protocol events and will bring feature parity across WebKit platforms. [wk-cert]: https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/JavaScriptCore/inspector/protocol/Security.json [pw]: microsoft/playwright#6631 [glib-networking]: https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/156

Ross Wollman (rwoll) · 2021-06-02T18:24:26Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

Ross Wollman (rwoll) · 2021-06-02T18:28:39Z

 requestStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.requestStart) : -1,
 responseStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.responseStart) : -1,
 };
+


There are a number of errant extra newlines that I'll clean up. They are an artifact of refactoring within this MR.

Ross Wollman (rwoll) · 2021-06-02T21:03:48Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)
Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this in _requestFinished(…).

Ross Wollman (rwoll) · 2021-06-02T22:10:06Z

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))
I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this [before] _requestFinished(…).

Here's what that might look like: rwoll#2. Joel Einbinder (@JoelEinbinder) Let me know which approach you think is cleaner:

Everything in loading finished modulo fixing Chromium breakage of the postData test cases: e29de4c
Using callbacks like WIP: feat(har): experiment with finish callbacks rwoll/playwright#2 which are invoked next to createResponse for CR/FF and then _loadingFinished for WK).

I like doing everything in request finished for simplicity/unity, but I'm not sure how to compromise between Chrome and WebKit. I don't love rwoll#2 since—while it's simple for Chrome and FF—you can accidentally hang the HAR capture in WK if the finish callbacks aren't invoked in all terminal response paths (although I've added tests to make sure we are good here).

Ross Wollman (rwoll) · 2021-06-10T23:25:43Z

Updated with option 2 since fixing Chromium not firing loading finished is out of scope.

Joel Einbinder (JoelEinbinder) · 2021-06-14T16:11:03Z

 }

+ _serverIPAddressAndPortFinished(serverIPAddress?: IPAddressAndPort) {
+ this._serverIPAddressAndPortPromiseCallback(helper.filterEmpties(serverIPAddress));


I don't think you need filterEmpties here and below. Everything is eventually going to be JSON.stringified into the har.

I've updated this, but we will get some empty objects for securityDetails in the HAR. (Tests updated accordingly.)

Max Schmitt (mxschmitt)

Awesome work! (don't forget to rebase)

Updates in the HAR capture Log Entries: * `serverIPAddress`: (string) This now gets populated. * `_serverPort`: (number) This now gets populated. There isn't an official field according to this [w3 doc][w3] (and I didn't see a suitable field in a HAR download from Chromium DevTools). I used the field name from [WK WebInspector HARBuilder.js][wk]. * _securityDetails: non-standard info that is helpful in making assertions about cert expiries. This also introduces new APIs on `network.Response` exposing the new info. NB: At first these APIs were synchonous and populated with info that is readily available at the beginning of the Response creation, but WK does not emit IP address info until _loadingFinished, so the API's were all turned into Promise-based approaches. Closesmicrosoft#6624. [w3]: https://www.w3.org/community/bigdata-tools/files/2017/10/HAR_Spec_TO_HAR_Vocabulary.pdf [wk]: https://opensource.apple.com/source/WebInspectorUI/WebInspectorUI-7608.4.9.1.3/UserInterface/Controllers/HARBuilder.js.auto.html

Ross Wollman (rwoll) · 2021-06-14T18:59:09Z

All feedback addressed! Thanks for the reviews Joel Einbinder (@JoelEinbinder) and Max Schmitt (@mxschmitt)!

Ross Wollman (rwoll) · 2021-06-14T21:03:51Z

 expect(port).toBe(httpsServer.PORT);
 if (browserName === 'webkit' && platform === 'win32')
- expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863});
+ expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863, validTo: -1});


Not sure what the best approach is here…should I just mark this test as fully failing on Win32? filterEmpties used to filter out the -1's we were getting from WK.

This allows users to access the connection information outside of the HAR file in realtime. Closesmicrosoft#7147. Relates microsoft#6631.

Ross Wollman (rwoll) · 2021-05-18T07:46:35Z

Updates in the HAR capture Log Entries:

serverIPAddress: (string) This now gets populated.
_serverPort: (number) This now gets populated. There isn't an
official field according to this w3 doc (and I didn't see
a suitable field in a HAR download from Chromium DevTools).
I used the field name from WK WebInspector HARBuilder.js.
_securityDetails: non-standard info that is helpful in making
assertions about cert expiries.

This also introduces new APIs on network.Response exposing the
new info.

NB: At first these APIs were synchronous and populated with info that is
readily available at the beginning of the Response creation, but WK does
not emit IP address info until _loadingFinished, so the API's were all
turned into Promise-based approaches.

Closes#6624.

Ross Wollman (rwoll)

/cc Joel Einbinder (@JoelEinbinder) 🎉

Ross Wollman (rwoll) · 2021-05-19T04:20:04Z

Joel Einbinder (@JoelEinbinder) I cleaned up the types (including deleting some of the awkward code dealing with the non-standard zero values.) I also added a comment to the WebKit address parsing code with some examples from my testing.

I haven't yet figured out the appropriate changes to emit the Response at the correct time after _onLoadingFinished where the IP address details are available for WebKit so I left the signal barrier in place in the HAR code. I'll remove that once we sort out dealing Response emission timing (directly or indirectly through guarding the newly added fields behind a Promise api). If we can avoid adding more external Promises to Response, I agree that would be much nicer.

Ross Wollman (rwoll) · 2021-05-20T21:20:37Z

Joel Einbinder (@JoelEinbinder) Here's the latest pass! Promise-based APIs because of WK. See #6631 (comment) with updated description.

Ross Wollman (rwoll) · 2021-05-21T19:26:42Z

Joel Einbinder (@JoelEinbinder): This should now pass on all the browser+platform variants. (Thanks platform-dependent WK networking! 😉 )

Let me know your feedback and I will make any necessary adjustments.

It's also worth skimming through #6695 to what additional information will be coming in next.

This changeset exposes * `not-valid-before` * `not-valid-after` * `subject-name` * `issuer-name` on GTlsCertificate provided by the underlying TLS Backend. In order to make use of these changes, see the related [glib-networking MR][glib-networking]. This change aims to help populate more of the [`Certificate`][wk-cert] info in the WebKit Inspector Protocol on Linux. This changeset stems from work in Microsoft Playwright to [add more info into its HAR capture][pw] generated from the Inspector Protocol events and will bring feature parity across WebKit platforms. [wk-cert]: https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/JavaScriptCore/inspector/protocol/Security.json [pw]: microsoft/playwright#6631 [glib-networking]: https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/156

Ross Wollman (rwoll) · 2021-06-02T18:24:26Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

Ross Wollman (rwoll) · 2021-06-02T18:28:39Z

 requestStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.requestStart) : -1,
 responseStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.responseStart) : -1,
 };
+


There are a number of errant extra newlines that I'll clean up. They are an artifact of refactoring within this MR.

Ross Wollman (rwoll) · 2021-06-02T21:03:48Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)
Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this in _requestFinished(…).

Ross Wollman (rwoll) · 2021-06-02T22:10:06Z

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))
I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this [before] _requestFinished(…).

Here's what that might look like: rwoll#2. Joel Einbinder (@JoelEinbinder) Let me know which approach you think is cleaner:

Everything in loading finished modulo fixing Chromium breakage of the postData test cases: e29de4c
Using callbacks like WIP: feat(har): experiment with finish callbacks rwoll/playwright#2 which are invoked next to createResponse for CR/FF and then _loadingFinished for WK).

I like doing everything in request finished for simplicity/unity, but I'm not sure how to compromise between Chrome and WebKit. I don't love rwoll#2 since—while it's simple for Chrome and FF—you can accidentally hang the HAR capture in WK if the finish callbacks aren't invoked in all terminal response paths (although I've added tests to make sure we are good here).

Ross Wollman (rwoll) · 2021-06-10T23:25:43Z

Updated with option 2 since fixing Chromium not firing loading finished is out of scope.

Joel Einbinder (JoelEinbinder) · 2021-06-14T16:11:03Z

 }

+ _serverIPAddressAndPortFinished(serverIPAddress?: IPAddressAndPort) {
+ this._serverIPAddressAndPortPromiseCallback(helper.filterEmpties(serverIPAddress));


I don't think you need filterEmpties here and below. Everything is eventually going to be JSON.stringified into the har.

I've updated this, but we will get some empty objects for securityDetails in the HAR. (Tests updated accordingly.)

Max Schmitt (mxschmitt)

Awesome work! (don't forget to rebase)

Updates in the HAR capture Log Entries: * `serverIPAddress`: (string) This now gets populated. * `_serverPort`: (number) This now gets populated. There isn't an official field according to this [w3 doc][w3] (and I didn't see a suitable field in a HAR download from Chromium DevTools). I used the field name from [WK WebInspector HARBuilder.js][wk]. * _securityDetails: non-standard info that is helpful in making assertions about cert expiries. This also introduces new APIs on `network.Response` exposing the new info. NB: At first these APIs were synchonous and populated with info that is readily available at the beginning of the Response creation, but WK does not emit IP address info until _loadingFinished, so the API's were all turned into Promise-based approaches. Closesmicrosoft#6624. [w3]: https://www.w3.org/community/bigdata-tools/files/2017/10/HAR_Spec_TO_HAR_Vocabulary.pdf [wk]: https://opensource.apple.com/source/WebInspectorUI/WebInspectorUI-7608.4.9.1.3/UserInterface/Controllers/HARBuilder.js.auto.html

Ross Wollman (rwoll) · 2021-06-14T18:59:09Z

All feedback addressed! Thanks for the reviews Joel Einbinder (@JoelEinbinder) and Max Schmitt (@mxschmitt)!

Ross Wollman (rwoll) · 2021-06-14T21:03:51Z

 expect(port).toBe(httpsServer.PORT);
 if (browserName === 'webkit' && platform === 'win32')
- expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863});
+ expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863, validTo: -1});


Not sure what the best approach is here…should I just mark this test as fully failing on Win32? filterEmpties used to filter out the -1's we were getting from WK.

This allows users to access the connection information outside of the HAR file in realtime. Closesmicrosoft#7147. Relates microsoft#6631.

Ross Wollman (rwoll) · 2021-05-18T07:46:35Z

Updates in the HAR capture Log Entries:

serverIPAddress: (string) This now gets populated.
_serverPort: (number) This now gets populated. There isn't an
official field according to this w3 doc (and I didn't see
a suitable field in a HAR download from Chromium DevTools).
I used the field name from WK WebInspector HARBuilder.js.
_securityDetails: non-standard info that is helpful in making
assertions about cert expiries.

This also introduces new APIs on network.Response exposing the
new info.

NB: At first these APIs were synchronous and populated with info that is
readily available at the beginning of the Response creation, but WK does
not emit IP address info until _loadingFinished, so the API's were all
turned into Promise-based approaches.

Closes#6624.

Ross Wollman (rwoll)

/cc Joel Einbinder (@JoelEinbinder) 🎉

Ross Wollman (rwoll) · 2021-05-19T04:20:04Z

Joel Einbinder (@JoelEinbinder) I cleaned up the types (including deleting some of the awkward code dealing with the non-standard zero values.) I also added a comment to the WebKit address parsing code with some examples from my testing.

I haven't yet figured out the appropriate changes to emit the Response at the correct time after _onLoadingFinished where the IP address details are available for WebKit so I left the signal barrier in place in the HAR code. I'll remove that once we sort out dealing Response emission timing (directly or indirectly through guarding the newly added fields behind a Promise api). If we can avoid adding more external Promises to Response, I agree that would be much nicer.

Ross Wollman (rwoll) · 2021-05-20T21:20:37Z

Joel Einbinder (@JoelEinbinder) Here's the latest pass! Promise-based APIs because of WK. See #6631 (comment) with updated description.

Ross Wollman (rwoll) · 2021-05-21T19:26:42Z

Joel Einbinder (@JoelEinbinder): This should now pass on all the browser+platform variants. (Thanks platform-dependent WK networking! 😉 )

Let me know your feedback and I will make any necessary adjustments.

It's also worth skimming through #6695 to what additional information will be coming in next.

This changeset exposes * `not-valid-before` * `not-valid-after` * `subject-name` * `issuer-name` on GTlsCertificate provided by the underlying TLS Backend. In order to make use of these changes, see the related [glib-networking MR][glib-networking]. This change aims to help populate more of the [`Certificate`][wk-cert] info in the WebKit Inspector Protocol on Linux. This changeset stems from work in Microsoft Playwright to [add more info into its HAR capture][pw] generated from the Inspector Protocol events and will bring feature parity across WebKit platforms. [wk-cert]: https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/JavaScriptCore/inspector/protocol/Security.json [pw]: microsoft/playwright#6631 [glib-networking]: https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/156

Ross Wollman (rwoll) · 2021-06-02T18:24:26Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

Ross Wollman (rwoll) · 2021-06-02T18:28:39Z

 requestStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.requestStart) : -1,
 responseStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.responseStart) : -1,
 };
+


There are a number of errant extra newlines that I'll clean up. They are an artifact of refactoring within this MR.

Ross Wollman (rwoll) · 2021-06-02T21:03:48Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)
Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this in _requestFinished(…).

Ross Wollman (rwoll) · 2021-06-02T22:10:06Z

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))
I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this [before] _requestFinished(…).

Here's what that might look like: rwoll#2. Joel Einbinder (@JoelEinbinder) Let me know which approach you think is cleaner:

Everything in loading finished modulo fixing Chromium breakage of the postData test cases: e29de4c
Using callbacks like WIP: feat(har): experiment with finish callbacks rwoll/playwright#2 which are invoked next to createResponse for CR/FF and then _loadingFinished for WK).

I like doing everything in request finished for simplicity/unity, but I'm not sure how to compromise between Chrome and WebKit. I don't love rwoll#2 since—while it's simple for Chrome and FF—you can accidentally hang the HAR capture in WK if the finish callbacks aren't invoked in all terminal response paths (although I've added tests to make sure we are good here).

Ross Wollman (rwoll) · 2021-06-10T23:25:43Z

Updated with option 2 since fixing Chromium not firing loading finished is out of scope.

Joel Einbinder (JoelEinbinder) · 2021-06-14T16:11:03Z

 }

+ _serverIPAddressAndPortFinished(serverIPAddress?: IPAddressAndPort) {
+ this._serverIPAddressAndPortPromiseCallback(helper.filterEmpties(serverIPAddress));


I don't think you need filterEmpties here and below. Everything is eventually going to be JSON.stringified into the har.

I've updated this, but we will get some empty objects for securityDetails in the HAR. (Tests updated accordingly.)

Max Schmitt (mxschmitt)

Awesome work! (don't forget to rebase)

Updates in the HAR capture Log Entries: * `serverIPAddress`: (string) This now gets populated. * `_serverPort`: (number) This now gets populated. There isn't an official field according to this [w3 doc][w3] (and I didn't see a suitable field in a HAR download from Chromium DevTools). I used the field name from [WK WebInspector HARBuilder.js][wk]. * _securityDetails: non-standard info that is helpful in making assertions about cert expiries. This also introduces new APIs on `network.Response` exposing the new info. NB: At first these APIs were synchonous and populated with info that is readily available at the beginning of the Response creation, but WK does not emit IP address info until _loadingFinished, so the API's were all turned into Promise-based approaches. Closesmicrosoft#6624. [w3]: https://www.w3.org/community/bigdata-tools/files/2017/10/HAR_Spec_TO_HAR_Vocabulary.pdf [wk]: https://opensource.apple.com/source/WebInspectorUI/WebInspectorUI-7608.4.9.1.3/UserInterface/Controllers/HARBuilder.js.auto.html

Ross Wollman (rwoll) · 2021-06-14T18:59:09Z

All feedback addressed! Thanks for the reviews Joel Einbinder (@JoelEinbinder) and Max Schmitt (@mxschmitt)!

Ross Wollman (rwoll) · 2021-06-14T21:03:51Z

 expect(port).toBe(httpsServer.PORT);
 if (browserName === 'webkit' && platform === 'win32')
- expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863});
+ expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863, validTo: -1});


Not sure what the best approach is here…should I just mark this test as fully failing on Win32? filterEmpties used to filter out the -1's we were getting from WK.

This allows users to access the connection information outside of the HAR file in realtime. Closesmicrosoft#7147. Relates microsoft#6631.

Ross Wollman (rwoll) · 2021-05-18T07:46:35Z

Updates in the HAR capture Log Entries:

serverIPAddress: (string) This now gets populated.
_serverPort: (number) This now gets populated. There isn't an
official field according to this w3 doc (and I didn't see
a suitable field in a HAR download from Chromium DevTools).
I used the field name from WK WebInspector HARBuilder.js.
_securityDetails: non-standard info that is helpful in making
assertions about cert expiries.

This also introduces new APIs on network.Response exposing the
new info.

NB: At first these APIs were synchronous and populated with info that is
readily available at the beginning of the Response creation, but WK does
not emit IP address info until _loadingFinished, so the API's were all
turned into Promise-based approaches.

Closes#6624.

Ross Wollman (rwoll)

/cc Joel Einbinder (@JoelEinbinder) 🎉

Ross Wollman (rwoll) · 2021-05-19T04:20:04Z

Joel Einbinder (@JoelEinbinder) I cleaned up the types (including deleting some of the awkward code dealing with the non-standard zero values.) I also added a comment to the WebKit address parsing code with some examples from my testing.

I haven't yet figured out the appropriate changes to emit the Response at the correct time after _onLoadingFinished where the IP address details are available for WebKit so I left the signal barrier in place in the HAR code. I'll remove that once we sort out dealing Response emission timing (directly or indirectly through guarding the newly added fields behind a Promise api). If we can avoid adding more external Promises to Response, I agree that would be much nicer.

Ross Wollman (rwoll) · 2021-05-20T21:20:37Z

Joel Einbinder (@JoelEinbinder) Here's the latest pass! Promise-based APIs because of WK. See #6631 (comment) with updated description.

Ross Wollman (rwoll) · 2021-05-21T19:26:42Z

Joel Einbinder (@JoelEinbinder): This should now pass on all the browser+platform variants. (Thanks platform-dependent WK networking! 😉 )

Let me know your feedback and I will make any necessary adjustments.

It's also worth skimming through #6695 to what additional information will be coming in next.

This changeset exposes * `not-valid-before` * `not-valid-after` * `subject-name` * `issuer-name` on GTlsCertificate provided by the underlying TLS Backend. In order to make use of these changes, see the related [glib-networking MR][glib-networking]. This change aims to help populate more of the [`Certificate`][wk-cert] info in the WebKit Inspector Protocol on Linux. This changeset stems from work in Microsoft Playwright to [add more info into its HAR capture][pw] generated from the Inspector Protocol events and will bring feature parity across WebKit platforms. [wk-cert]: https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/JavaScriptCore/inspector/protocol/Security.json [pw]: microsoft/playwright#6631 [glib-networking]: https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/156

Ross Wollman (rwoll) · 2021-06-02T18:24:26Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

Ross Wollman (rwoll) · 2021-06-02T18:28:39Z

 requestStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.requestStart) : -1,
 responseStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.responseStart) : -1,
 };
+


There are a number of errant extra newlines that I'll clean up. They are an artifact of refactoring within this MR.

Ross Wollman (rwoll) · 2021-06-02T21:03:48Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)
Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this in _requestFinished(…).

Ross Wollman (rwoll) · 2021-06-02T22:10:06Z

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))
I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this [before] _requestFinished(…).

Here's what that might look like: rwoll#2. Joel Einbinder (@JoelEinbinder) Let me know which approach you think is cleaner:

Everything in loading finished modulo fixing Chromium breakage of the postData test cases: e29de4c
Using callbacks like WIP: feat(har): experiment with finish callbacks rwoll/playwright#2 which are invoked next to createResponse for CR/FF and then _loadingFinished for WK).

I like doing everything in request finished for simplicity/unity, but I'm not sure how to compromise between Chrome and WebKit. I don't love rwoll#2 since—while it's simple for Chrome and FF—you can accidentally hang the HAR capture in WK if the finish callbacks aren't invoked in all terminal response paths (although I've added tests to make sure we are good here).

Ross Wollman (rwoll) · 2021-06-10T23:25:43Z

Updated with option 2 since fixing Chromium not firing loading finished is out of scope.

Joel Einbinder (JoelEinbinder) · 2021-06-14T16:11:03Z

 }

+ _serverIPAddressAndPortFinished(serverIPAddress?: IPAddressAndPort) {
+ this._serverIPAddressAndPortPromiseCallback(helper.filterEmpties(serverIPAddress));


I don't think you need filterEmpties here and below. Everything is eventually going to be JSON.stringified into the har.

I've updated this, but we will get some empty objects for securityDetails in the HAR. (Tests updated accordingly.)

Max Schmitt (mxschmitt)

Awesome work! (don't forget to rebase)

Updates in the HAR capture Log Entries: * `serverIPAddress`: (string) This now gets populated. * `_serverPort`: (number) This now gets populated. There isn't an official field according to this [w3 doc][w3] (and I didn't see a suitable field in a HAR download from Chromium DevTools). I used the field name from [WK WebInspector HARBuilder.js][wk]. * _securityDetails: non-standard info that is helpful in making assertions about cert expiries. This also introduces new APIs on `network.Response` exposing the new info. NB: At first these APIs were synchonous and populated with info that is readily available at the beginning of the Response creation, but WK does not emit IP address info until _loadingFinished, so the API's were all turned into Promise-based approaches. Closesmicrosoft#6624. [w3]: https://www.w3.org/community/bigdata-tools/files/2017/10/HAR_Spec_TO_HAR_Vocabulary.pdf [wk]: https://opensource.apple.com/source/WebInspectorUI/WebInspectorUI-7608.4.9.1.3/UserInterface/Controllers/HARBuilder.js.auto.html

Ross Wollman (rwoll) · 2021-06-14T18:59:09Z

All feedback addressed! Thanks for the reviews Joel Einbinder (@JoelEinbinder) and Max Schmitt (@mxschmitt)!

Ross Wollman (rwoll) · 2021-06-14T21:03:51Z

 expect(port).toBe(httpsServer.PORT);
 if (browserName === 'webkit' && platform === 'win32')
- expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863});
+ expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863, validTo: -1});


Not sure what the best approach is here…should I just mark this test as fully failing on Win32? filterEmpties used to filter out the -1's we were getting from WK.

This allows users to access the connection information outside of the HAR file in realtime. Closesmicrosoft#7147. Relates microsoft#6631.

Ross Wollman (rwoll) · 2021-05-18T07:46:35Z

Updates in the HAR capture Log Entries:

serverIPAddress: (string) This now gets populated.
_serverPort: (number) This now gets populated. There isn't an
official field according to this w3 doc (and I didn't see
a suitable field in a HAR download from Chromium DevTools).
I used the field name from WK WebInspector HARBuilder.js.
_securityDetails: non-standard info that is helpful in making
assertions about cert expiries.

This also introduces new APIs on network.Response exposing the
new info.

NB: At first these APIs were synchronous and populated with info that is
readily available at the beginning of the Response creation, but WK does
not emit IP address info until _loadingFinished, so the API's were all
turned into Promise-based approaches.

Closes#6624.

Ross Wollman (rwoll)

/cc Joel Einbinder (@JoelEinbinder) 🎉

Ross Wollman (rwoll) · 2021-05-19T04:20:04Z

Joel Einbinder (@JoelEinbinder) I cleaned up the types (including deleting some of the awkward code dealing with the non-standard zero values.) I also added a comment to the WebKit address parsing code with some examples from my testing.

I haven't yet figured out the appropriate changes to emit the Response at the correct time after _onLoadingFinished where the IP address details are available for WebKit so I left the signal barrier in place in the HAR code. I'll remove that once we sort out dealing Response emission timing (directly or indirectly through guarding the newly added fields behind a Promise api). If we can avoid adding more external Promises to Response, I agree that would be much nicer.

Ross Wollman (rwoll) · 2021-05-20T21:20:37Z

Joel Einbinder (@JoelEinbinder) Here's the latest pass! Promise-based APIs because of WK. See #6631 (comment) with updated description.

Ross Wollman (rwoll) · 2021-05-21T19:26:42Z

Joel Einbinder (@JoelEinbinder): This should now pass on all the browser+platform variants. (Thanks platform-dependent WK networking! 😉 )

Let me know your feedback and I will make any necessary adjustments.

It's also worth skimming through #6695 to what additional information will be coming in next.

This changeset exposes * `not-valid-before` * `not-valid-after` * `subject-name` * `issuer-name` on GTlsCertificate provided by the underlying TLS Backend. In order to make use of these changes, see the related [glib-networking MR][glib-networking]. This change aims to help populate more of the [`Certificate`][wk-cert] info in the WebKit Inspector Protocol on Linux. This changeset stems from work in Microsoft Playwright to [add more info into its HAR capture][pw] generated from the Inspector Protocol events and will bring feature parity across WebKit platforms. [wk-cert]: https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/JavaScriptCore/inspector/protocol/Security.json [pw]: microsoft/playwright#6631 [glib-networking]: https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/156

Ross Wollman (rwoll) · 2021-06-02T18:24:26Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

Ross Wollman (rwoll) · 2021-06-02T18:28:39Z

 requestStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.requestStart) : -1,
 responseStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.responseStart) : -1,
 };
+


There are a number of errant extra newlines that I'll clean up. They are an artifact of refactoring within this MR.

Ross Wollman (rwoll) · 2021-06-02T21:03:48Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)
Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this in _requestFinished(…).

Ross Wollman (rwoll) · 2021-06-02T22:10:06Z

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))
I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this [before] _requestFinished(…).

Here's what that might look like: rwoll#2. Joel Einbinder (@JoelEinbinder) Let me know which approach you think is cleaner:

Everything in loading finished modulo fixing Chromium breakage of the postData test cases: e29de4c
Using callbacks like WIP: feat(har): experiment with finish callbacks rwoll/playwright#2 which are invoked next to createResponse for CR/FF and then _loadingFinished for WK).

I like doing everything in request finished for simplicity/unity, but I'm not sure how to compromise between Chrome and WebKit. I don't love rwoll#2 since—while it's simple for Chrome and FF—you can accidentally hang the HAR capture in WK if the finish callbacks aren't invoked in all terminal response paths (although I've added tests to make sure we are good here).

Ross Wollman (rwoll) · 2021-06-10T23:25:43Z

Updated with option 2 since fixing Chromium not firing loading finished is out of scope.

Joel Einbinder (JoelEinbinder) · 2021-06-14T16:11:03Z

 }

+ _serverIPAddressAndPortFinished(serverIPAddress?: IPAddressAndPort) {
+ this._serverIPAddressAndPortPromiseCallback(helper.filterEmpties(serverIPAddress));


I don't think you need filterEmpties here and below. Everything is eventually going to be JSON.stringified into the har.

I've updated this, but we will get some empty objects for securityDetails in the HAR. (Tests updated accordingly.)

Max Schmitt (mxschmitt)

Awesome work! (don't forget to rebase)

Updates in the HAR capture Log Entries: * `serverIPAddress`: (string) This now gets populated. * `_serverPort`: (number) This now gets populated. There isn't an official field according to this [w3 doc][w3] (and I didn't see a suitable field in a HAR download from Chromium DevTools). I used the field name from [WK WebInspector HARBuilder.js][wk]. * _securityDetails: non-standard info that is helpful in making assertions about cert expiries. This also introduces new APIs on `network.Response` exposing the new info. NB: At first these APIs were synchonous and populated with info that is readily available at the beginning of the Response creation, but WK does not emit IP address info until _loadingFinished, so the API's were all turned into Promise-based approaches. Closesmicrosoft#6624. [w3]: https://www.w3.org/community/bigdata-tools/files/2017/10/HAR_Spec_TO_HAR_Vocabulary.pdf [wk]: https://opensource.apple.com/source/WebInspectorUI/WebInspectorUI-7608.4.9.1.3/UserInterface/Controllers/HARBuilder.js.auto.html

Ross Wollman (rwoll) · 2021-06-14T18:59:09Z

All feedback addressed! Thanks for the reviews Joel Einbinder (@JoelEinbinder) and Max Schmitt (@mxschmitt)!

Ross Wollman (rwoll) · 2021-06-14T21:03:51Z

 expect(port).toBe(httpsServer.PORT);
 if (browserName === 'webkit' && platform === 'win32')
- expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863});
+ expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863, validTo: -1});


Not sure what the best approach is here…should I just mark this test as fully failing on Win32? filterEmpties used to filter out the -1's we were getting from WK.

This allows users to access the connection information outside of the HAR file in realtime. Closesmicrosoft#7147. Relates microsoft#6631.

Ross Wollman (rwoll) · 2021-05-18T07:46:35Z

Updates in the HAR capture Log Entries:

serverIPAddress: (string) This now gets populated.
_serverPort: (number) This now gets populated. There isn't an
official field according to this w3 doc (and I didn't see
a suitable field in a HAR download from Chromium DevTools).
I used the field name from WK WebInspector HARBuilder.js.
_securityDetails: non-standard info that is helpful in making
assertions about cert expiries.

This also introduces new APIs on network.Response exposing the
new info.

NB: At first these APIs were synchronous and populated with info that is
readily available at the beginning of the Response creation, but WK does
not emit IP address info until _loadingFinished, so the API's were all
turned into Promise-based approaches.

Closes#6624.

Ross Wollman (rwoll)

/cc Joel Einbinder (@JoelEinbinder) 🎉

Ross Wollman (rwoll) · 2021-05-19T04:20:04Z

Joel Einbinder (@JoelEinbinder) I cleaned up the types (including deleting some of the awkward code dealing with the non-standard zero values.) I also added a comment to the WebKit address parsing code with some examples from my testing.

I haven't yet figured out the appropriate changes to emit the Response at the correct time after _onLoadingFinished where the IP address details are available for WebKit so I left the signal barrier in place in the HAR code. I'll remove that once we sort out dealing Response emission timing (directly or indirectly through guarding the newly added fields behind a Promise api). If we can avoid adding more external Promises to Response, I agree that would be much nicer.

Ross Wollman (rwoll) · 2021-05-20T21:20:37Z

Joel Einbinder (@JoelEinbinder) Here's the latest pass! Promise-based APIs because of WK. See #6631 (comment) with updated description.

Ross Wollman (rwoll) · 2021-05-21T19:26:42Z

Joel Einbinder (@JoelEinbinder): This should now pass on all the browser+platform variants. (Thanks platform-dependent WK networking! 😉 )

Let me know your feedback and I will make any necessary adjustments.

It's also worth skimming through #6695 to what additional information will be coming in next.

This changeset exposes * `not-valid-before` * `not-valid-after` * `subject-name` * `issuer-name` on GTlsCertificate provided by the underlying TLS Backend. In order to make use of these changes, see the related [glib-networking MR][glib-networking]. This change aims to help populate more of the [`Certificate`][wk-cert] info in the WebKit Inspector Protocol on Linux. This changeset stems from work in Microsoft Playwright to [add more info into its HAR capture][pw] generated from the Inspector Protocol events and will bring feature parity across WebKit platforms. [wk-cert]: https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/JavaScriptCore/inspector/protocol/Security.json [pw]: microsoft/playwright#6631 [glib-networking]: https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/156

Ross Wollman (rwoll) · 2021-06-02T18:24:26Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

Ross Wollman (rwoll) · 2021-06-02T18:28:39Z

 requestStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.requestStart) : -1,
 responseStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.responseStart) : -1,
 };
+


There are a number of errant extra newlines that I'll clean up. They are an artifact of refactoring within this MR.

Ross Wollman (rwoll) · 2021-06-02T21:03:48Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)
Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this in _requestFinished(…).

Ross Wollman (rwoll) · 2021-06-02T22:10:06Z

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))
I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this [before] _requestFinished(…).

Here's what that might look like: rwoll#2. Joel Einbinder (@JoelEinbinder) Let me know which approach you think is cleaner:

Everything in loading finished modulo fixing Chromium breakage of the postData test cases: e29de4c
Using callbacks like WIP: feat(har): experiment with finish callbacks rwoll/playwright#2 which are invoked next to createResponse for CR/FF and then _loadingFinished for WK).

I like doing everything in request finished for simplicity/unity, but I'm not sure how to compromise between Chrome and WebKit. I don't love rwoll#2 since—while it's simple for Chrome and FF—you can accidentally hang the HAR capture in WK if the finish callbacks aren't invoked in all terminal response paths (although I've added tests to make sure we are good here).

Ross Wollman (rwoll) · 2021-06-10T23:25:43Z

Updated with option 2 since fixing Chromium not firing loading finished is out of scope.

Joel Einbinder (JoelEinbinder) · 2021-06-14T16:11:03Z

 }

+ _serverIPAddressAndPortFinished(serverIPAddress?: IPAddressAndPort) {
+ this._serverIPAddressAndPortPromiseCallback(helper.filterEmpties(serverIPAddress));


I don't think you need filterEmpties here and below. Everything is eventually going to be JSON.stringified into the har.

I've updated this, but we will get some empty objects for securityDetails in the HAR. (Tests updated accordingly.)

Max Schmitt (mxschmitt)

Awesome work! (don't forget to rebase)

Updates in the HAR capture Log Entries: * `serverIPAddress`: (string) This now gets populated. * `_serverPort`: (number) This now gets populated. There isn't an official field according to this [w3 doc][w3] (and I didn't see a suitable field in a HAR download from Chromium DevTools). I used the field name from [WK WebInspector HARBuilder.js][wk]. * _securityDetails: non-standard info that is helpful in making assertions about cert expiries. This also introduces new APIs on `network.Response` exposing the new info. NB: At first these APIs were synchonous and populated with info that is readily available at the beginning of the Response creation, but WK does not emit IP address info until _loadingFinished, so the API's were all turned into Promise-based approaches. Closesmicrosoft#6624. [w3]: https://www.w3.org/community/bigdata-tools/files/2017/10/HAR_Spec_TO_HAR_Vocabulary.pdf [wk]: https://opensource.apple.com/source/WebInspectorUI/WebInspectorUI-7608.4.9.1.3/UserInterface/Controllers/HARBuilder.js.auto.html

Ross Wollman (rwoll) · 2021-06-14T18:59:09Z

All feedback addressed! Thanks for the reviews Joel Einbinder (@JoelEinbinder) and Max Schmitt (@mxschmitt)!

Ross Wollman (rwoll) · 2021-06-14T21:03:51Z

 expect(port).toBe(httpsServer.PORT);
 if (browserName === 'webkit' && platform === 'win32')
- expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863});
+ expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863, validTo: -1});


Not sure what the best approach is here…should I just mark this test as fully failing on Win32? filterEmpties used to filter out the -1's we were getting from WK.

This allows users to access the connection information outside of the HAR file in realtime. Closesmicrosoft#7147. Relates microsoft#6631.

Ross Wollman (rwoll) · 2021-05-18T07:46:35Z

Updates in the HAR capture Log Entries:

serverIPAddress: (string) This now gets populated.
_serverPort: (number) This now gets populated. There isn't an
official field according to this w3 doc (and I didn't see
a suitable field in a HAR download from Chromium DevTools).
I used the field name from WK WebInspector HARBuilder.js.
_securityDetails: non-standard info that is helpful in making
assertions about cert expiries.

This also introduces new APIs on network.Response exposing the
new info.

NB: At first these APIs were synchronous and populated with info that is
readily available at the beginning of the Response creation, but WK does
not emit IP address info until _loadingFinished, so the API's were all
turned into Promise-based approaches.

Closes#6624.

Ross Wollman (rwoll)

/cc Joel Einbinder (@JoelEinbinder) 🎉

Ross Wollman (rwoll) · 2021-05-19T04:20:04Z

Joel Einbinder (@JoelEinbinder) I cleaned up the types (including deleting some of the awkward code dealing with the non-standard zero values.) I also added a comment to the WebKit address parsing code with some examples from my testing.

I haven't yet figured out the appropriate changes to emit the Response at the correct time after _onLoadingFinished where the IP address details are available for WebKit so I left the signal barrier in place in the HAR code. I'll remove that once we sort out dealing Response emission timing (directly or indirectly through guarding the newly added fields behind a Promise api). If we can avoid adding more external Promises to Response, I agree that would be much nicer.

Ross Wollman (rwoll) · 2021-05-20T21:20:37Z

Joel Einbinder (@JoelEinbinder) Here's the latest pass! Promise-based APIs because of WK. See #6631 (comment) with updated description.

Ross Wollman (rwoll) · 2021-05-21T19:26:42Z

Joel Einbinder (@JoelEinbinder): This should now pass on all the browser+platform variants. (Thanks platform-dependent WK networking! 😉 )

Let me know your feedback and I will make any necessary adjustments.

It's also worth skimming through #6695 to what additional information will be coming in next.

This changeset exposes * `not-valid-before` * `not-valid-after` * `subject-name` * `issuer-name` on GTlsCertificate provided by the underlying TLS Backend. In order to make use of these changes, see the related [glib-networking MR][glib-networking]. This change aims to help populate more of the [`Certificate`][wk-cert] info in the WebKit Inspector Protocol on Linux. This changeset stems from work in Microsoft Playwright to [add more info into its HAR capture][pw] generated from the Inspector Protocol events and will bring feature parity across WebKit platforms. [wk-cert]: https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/JavaScriptCore/inspector/protocol/Security.json [pw]: microsoft/playwright#6631 [glib-networking]: https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/156

Ross Wollman (rwoll) · 2021-06-02T18:24:26Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

Ross Wollman (rwoll) · 2021-06-02T18:28:39Z

 requestStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.requestStart) : -1,
 responseStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.responseStart) : -1,
 };
+


There are a number of errant extra newlines that I'll clean up. They are an artifact of refactoring within this MR.

Ross Wollman (rwoll) · 2021-06-02T21:03:48Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)
Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this in _requestFinished(…).

Ross Wollman (rwoll) · 2021-06-02T22:10:06Z

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))
I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this [before] _requestFinished(…).

Here's what that might look like: rwoll#2. Joel Einbinder (@JoelEinbinder) Let me know which approach you think is cleaner:

Everything in loading finished modulo fixing Chromium breakage of the postData test cases: e29de4c
Using callbacks like WIP: feat(har): experiment with finish callbacks rwoll/playwright#2 which are invoked next to createResponse for CR/FF and then _loadingFinished for WK).

I like doing everything in request finished for simplicity/unity, but I'm not sure how to compromise between Chrome and WebKit. I don't love rwoll#2 since—while it's simple for Chrome and FF—you can accidentally hang the HAR capture in WK if the finish callbacks aren't invoked in all terminal response paths (although I've added tests to make sure we are good here).

Ross Wollman (rwoll) · 2021-06-10T23:25:43Z

Updated with option 2 since fixing Chromium not firing loading finished is out of scope.

Joel Einbinder (JoelEinbinder) · 2021-06-14T16:11:03Z

 }

+ _serverIPAddressAndPortFinished(serverIPAddress?: IPAddressAndPort) {
+ this._serverIPAddressAndPortPromiseCallback(helper.filterEmpties(serverIPAddress));


I don't think you need filterEmpties here and below. Everything is eventually going to be JSON.stringified into the har.

I've updated this, but we will get some empty objects for securityDetails in the HAR. (Tests updated accordingly.)

Max Schmitt (mxschmitt)

Awesome work! (don't forget to rebase)

Updates in the HAR capture Log Entries: * `serverIPAddress`: (string) This now gets populated. * `_serverPort`: (number) This now gets populated. There isn't an official field according to this [w3 doc][w3] (and I didn't see a suitable field in a HAR download from Chromium DevTools). I used the field name from [WK WebInspector HARBuilder.js][wk]. * _securityDetails: non-standard info that is helpful in making assertions about cert expiries. This also introduces new APIs on `network.Response` exposing the new info. NB: At first these APIs were synchonous and populated with info that is readily available at the beginning of the Response creation, but WK does not emit IP address info until _loadingFinished, so the API's were all turned into Promise-based approaches. Closesmicrosoft#6624. [w3]: https://www.w3.org/community/bigdata-tools/files/2017/10/HAR_Spec_TO_HAR_Vocabulary.pdf [wk]: https://opensource.apple.com/source/WebInspectorUI/WebInspectorUI-7608.4.9.1.3/UserInterface/Controllers/HARBuilder.js.auto.html

Ross Wollman (rwoll) · 2021-06-14T18:59:09Z

All feedback addressed! Thanks for the reviews Joel Einbinder (@JoelEinbinder) and Max Schmitt (@mxschmitt)!

Ross Wollman (rwoll) · 2021-06-14T21:03:51Z

 expect(port).toBe(httpsServer.PORT);
 if (browserName === 'webkit' && platform === 'win32')
- expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863});
+ expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863, validTo: -1});


Not sure what the best approach is here…should I just mark this test as fully failing on Win32? filterEmpties used to filter out the -1's we were getting from WK.

This allows users to access the connection information outside of the HAR file in realtime. Closesmicrosoft#7147. Relates microsoft#6631.

Ross Wollman (rwoll) · 2021-05-18T07:46:35Z

Updates in the HAR capture Log Entries:

serverIPAddress: (string) This now gets populated.
_serverPort: (number) This now gets populated. There isn't an
official field according to this w3 doc (and I didn't see
a suitable field in a HAR download from Chromium DevTools).
I used the field name from WK WebInspector HARBuilder.js.
_securityDetails: non-standard info that is helpful in making
assertions about cert expiries.

This also introduces new APIs on network.Response exposing the
new info.

NB: At first these APIs were synchronous and populated with info that is
readily available at the beginning of the Response creation, but WK does
not emit IP address info until _loadingFinished, so the API's were all
turned into Promise-based approaches.

Closes#6624.

Ross Wollman (rwoll)

/cc Joel Einbinder (@JoelEinbinder) 🎉

Ross Wollman (rwoll) · 2021-05-19T04:20:04Z

Joel Einbinder (@JoelEinbinder) I cleaned up the types (including deleting some of the awkward code dealing with the non-standard zero values.) I also added a comment to the WebKit address parsing code with some examples from my testing.

I haven't yet figured out the appropriate changes to emit the Response at the correct time after _onLoadingFinished where the IP address details are available for WebKit so I left the signal barrier in place in the HAR code. I'll remove that once we sort out dealing Response emission timing (directly or indirectly through guarding the newly added fields behind a Promise api). If we can avoid adding more external Promises to Response, I agree that would be much nicer.

Ross Wollman (rwoll) · 2021-05-20T21:20:37Z

Joel Einbinder (@JoelEinbinder) Here's the latest pass! Promise-based APIs because of WK. See #6631 (comment) with updated description.

Ross Wollman (rwoll) · 2021-05-21T19:26:42Z

Joel Einbinder (@JoelEinbinder): This should now pass on all the browser+platform variants. (Thanks platform-dependent WK networking! 😉 )

Let me know your feedback and I will make any necessary adjustments.

It's also worth skimming through #6695 to what additional information will be coming in next.

This changeset exposes * `not-valid-before` * `not-valid-after` * `subject-name` * `issuer-name` on GTlsCertificate provided by the underlying TLS Backend. In order to make use of these changes, see the related [glib-networking MR][glib-networking]. This change aims to help populate more of the [`Certificate`][wk-cert] info in the WebKit Inspector Protocol on Linux. This changeset stems from work in Microsoft Playwright to [add more info into its HAR capture][pw] generated from the Inspector Protocol events and will bring feature parity across WebKit platforms. [wk-cert]: https://github.com/WebKit/WebKit/blob/8afe31a018b11741abdf9b4d5bb973d7c1d9ff05/Source/JavaScriptCore/inspector/protocol/Security.json [pw]: microsoft/playwright#6631 [glib-networking]: https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/156

Ross Wollman (rwoll) · 2021-06-02T18:24:26Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

Ross Wollman (rwoll) · 2021-06-02T18:28:39Z

 requestStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.requestStart) : -1,
 responseStart: timingPayload ? wkMillisToRoundishMillis(timingPayload.responseStart) : -1,
 };
+


There are a number of errant extra newlines that I'll clean up. They are an artifact of refactoring within this MR.

Ross Wollman (rwoll) · 2021-06-02T21:03:48Z

Joel Einbinder (@JoelEinbinder) I took a quick pass to refactor to defer populating the information across all browser impls in _onRequestFinished. Please let me know if the general approach looks good. (i.e. save off intermediate request data in a map until we need it later in request finished.) If all looks kosher, I will clean up (i.e. ensure we're deleting out of the map not to leak/bloat mem; additionally, we likely need to do something in some of the other terminal request handlers (i.e. loading failed)—I'll add some more test cases, too.)
Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))

I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this in _requestFinished(…).

Ross Wollman (rwoll) · 2021-06-02T22:10:06Z

Unfortunately, this brings me back to why I ended up implementing this and then needing to change to the hybrid approach you saw in the last review: should include postData and should include binary postData now hang because we never fire request finished. Do you have tips to ensure we don't fail on these? (relates: #6631 (comment))
I suppose one approach would be to add response._connectionDetailsFinished(serverIPAddressAndPort, securityDetails) that FF and CR can call immediately after createResponse, and WK can call this [before] _requestFinished(…).

Here's what that might look like: rwoll#2. Joel Einbinder (@JoelEinbinder) Let me know which approach you think is cleaner:

Everything in loading finished modulo fixing Chromium breakage of the postData test cases: e29de4c
Using callbacks like WIP: feat(har): experiment with finish callbacks rwoll/playwright#2 which are invoked next to createResponse for CR/FF and then _loadingFinished for WK).

I like doing everything in request finished for simplicity/unity, but I'm not sure how to compromise between Chrome and WebKit. I don't love rwoll#2 since—while it's simple for Chrome and FF—you can accidentally hang the HAR capture in WK if the finish callbacks aren't invoked in all terminal response paths (although I've added tests to make sure we are good here).

Ross Wollman (rwoll) · 2021-06-10T23:25:43Z

Updated with option 2 since fixing Chromium not firing loading finished is out of scope.

Joel Einbinder (JoelEinbinder) · 2021-06-14T16:11:03Z

 }

+ _serverIPAddressAndPortFinished(serverIPAddress?: IPAddressAndPort) {
+ this._serverIPAddressAndPortPromiseCallback(helper.filterEmpties(serverIPAddress));


I don't think you need filterEmpties here and below. Everything is eventually going to be JSON.stringified into the har.

I've updated this, but we will get some empty objects for securityDetails in the HAR. (Tests updated accordingly.)

Max Schmitt (mxschmitt)

Awesome work! (don't forget to rebase)

Updates in the HAR capture Log Entries: * `serverIPAddress`: (string) This now gets populated. * `_serverPort`: (number) This now gets populated. There isn't an official field according to this [w3 doc][w3] (and I didn't see a suitable field in a HAR download from Chromium DevTools). I used the field name from [WK WebInspector HARBuilder.js][wk]. * _securityDetails: non-standard info that is helpful in making assertions about cert expiries. This also introduces new APIs on `network.Response` exposing the new info. NB: At first these APIs were synchonous and populated with info that is readily available at the beginning of the Response creation, but WK does not emit IP address info until _loadingFinished, so the API's were all turned into Promise-based approaches. Closesmicrosoft#6624. [w3]: https://www.w3.org/community/bigdata-tools/files/2017/10/HAR_Spec_TO_HAR_Vocabulary.pdf [wk]: https://opensource.apple.com/source/WebInspectorUI/WebInspectorUI-7608.4.9.1.3/UserInterface/Controllers/HARBuilder.js.auto.html

Ross Wollman (rwoll) · 2021-06-14T18:59:09Z

All feedback addressed! Thanks for the reviews Joel Einbinder (@JoelEinbinder) and Max Schmitt (@mxschmitt)!

Ross Wollman (rwoll) · 2021-06-14T21:03:51Z

 expect(port).toBe(httpsServer.PORT);
 if (browserName === 'webkit' && platform === 'win32')
- expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863});
+ expect(securityDetails).toEqual({subjectName: 'puppeteer-tests', validFrom: 1550084863, validTo: -1});


Not sure what the best approach is here…should I just mark this test as fully failing on Win32? filterEmpties used to filter out the -1's we were getting from WK.

This allows users to access the connection information outside of the HAR file in realtime. Closesmicrosoft#7147. Relates microsoft#6631.

Ross Wollman (rwoll) commented May 18, 2021
View reviewed changes

Ross Wollman (rwoll) commented May 19, 2021
View reviewed changes

Comment threadsrc/server/helper.ts Outdated
Comment threadsrc/server/supplements/har/harTracer.ts Outdated

Ross Wollman (rwoll) marked this pull request as ready for review May 19, 2021 00:44

Joel Einbinder (JoelEinbinder) reviewed May 19, 2021
View reviewed changes

Comment threadsrc/server/helper.ts Outdated

Ross Wollman (rwoll) commented May 19, 2021
View reviewed changes

Comment threadsrc/server/network.ts Outdated

Ross Wollman (rwoll) commented May 19, 2021
View reviewed changes

Comment threadsrc/server/network.ts Outdated

Ross Wollman (rwoll) commented May 19, 2021
View reviewed changes

Comment threadtests/har.spec.ts Outdated

Ross Wollman (rwoll) commented May 19, 2021
View reviewed changes

Comment threadsrc/server/supplements/har/harTracer.ts Outdated

Ross Wollman (rwoll)force-pushed the feature/har-ip-and-ssl branch 2 times, most recently from 21c91c9 to 9ce3a01Compare May 20, 2021 21:13

Ross Wollman (rwoll)force-pushed the feature/har-ip-and-ssl branch 3 times, most recently from d2d1c15 to c7f30eeCompare May 21, 2021 00:31

Max Schmitt (mxschmitt) added the CQ1 label May 21, 2021

Ross Wollman (rwoll)force-pushed the feature/har-ip-and-ssl branch 2 times, most recently from 38e1f04 to b0342f5Compare May 21, 2021 07:14

Max Schmitt (mxschmitt) added CQ1 and removed CQ1 labels May 21, 2021

Ross Wollman (rwoll) commented May 21, 2021
View reviewed changes

Comment threadsrc/server/network.ts Outdated

Max Schmitt (mxschmitt) added CQ1 and removed CQ1 labels May 23, 2021

Max Schmitt (mxschmitt) reviewed May 24, 2021
View reviewed changes

Comment threadsrc/server/network.ts Outdated
Comment threadsrc/server/helper.ts Outdated
Comment threadsrc/server/network.ts Outdated
Comment threadtests/har.spec.ts Outdated

Ross Wollman (rwoll) mentioned this pull request May 26, 2021
[Feature] Linux WebKit Connection Details Feature Parity (TLS info, Cert Info) #6759
Closed

Joel Einbinder (JoelEinbinder) reviewed Jun 2, 2021
View reviewed changes

Comment threadtests/har.spec.ts Outdated

Joel Einbinder (JoelEinbinder) reviewed Jun 2, 2021
View reviewed changes

Comment threadsrc/server/network.ts Outdated
Comment threadsrc/server/network.ts Outdated
Comment threadsrc/server/helper.ts Outdated
Comment threadsrc/server/network.ts Outdated
Comment threadsrc/server/network.ts

Ross Wollman (rwoll) commented Jun 2, 2021
View reviewed changes

Joel Einbinder (JoelEinbinder) reviewed Jun 14, 2021
View reviewed changes

Joel Einbinder (JoelEinbinder) approved these changes Jun 14, 2021
View reviewed changes

Comment threadsrc/server/network.ts Outdated
Comment threadsrc/server/supplements/har/harTracer.ts Outdated
Comment threadsrc/server/helper.ts Outdated

Max Schmitt (mxschmitt) approved these changes Jun 14, 2021
View reviewed changes

Comment threadsrc/server/chromium/crNetworkManager.ts Outdated
Comment threadtests/har.spec.ts

Ross Wollman (rwoll)force-pushed the feature/har-ip-and-ssl branch 2 times, most recently from 17eca1b to 2de1397Compare June 14, 2021 18:58

Max Schmitt (mxschmitt) added CQ1 and removed CQ1 labels Jun 14, 2021

Ross Wollman (rwoll)force-pushed the feature/har-ip-and-ssl branch from 2de1397 to 782de28Compare June 14, 2021 19:45

Max Schmitt (mxschmitt) added CQ1 and removed CQ1 labels Jun 14, 2021

test(har): fix failing win32 test
2b6cde9

Ross Wollman (rwoll) commented Jun 14, 2021
View reviewed changes

Max Schmitt (mxschmitt) added CQ1 and removed CQ1 labels Jun 14, 2021

Max Schmitt (mxschmitt) merged commit 195eab8 into microsoft:masterJun 15, 2021

Ross Wollman (rwoll) mentioned this pull request Jun 15, 2021
feat(response): expose connection details in API #7150
Merged

Manabu Niseki (ninoseki) mentioned this pull request Jul 30, 2021
feat: record remote IP:PORT and SSL details ninoseki/playwright-har-tracer#12
Merged