net.request response is missing content-type header in 304 responses

[themadtitanmathos]

Preflight Checklist

• [x ] I have read the Contributing Guidelines for this project.
• [ x] I agree to follow the Code of Conduct that this project adheres to.
• [ x] I have searched the issue tracker for an issue that matches the one I want to file, without success.

Issue Details

• Electron Version:
  10.1.7
• Operating System:
  Windows 10 Enterprise 1909

When making a request with net.request, I can see that my response has a content-type header of application/json; charset=utf-8 inside of my webRequest.onHeadersReceived handler:

electron.session.defaultSession.webRequest.onHeadersReceived((details, callback) => {
  // details.responseHeaders['content-type'] === 'application/json; charset=utf-8'
});

But by the time the net.request request.on('response') handler receives the headers, the content-type is gone.

request.on('response', (response) => {
  /* 
  response.headers === {
    // no content-type
  };
  */
});

I haven't tested if this is an issue in all types of responses, but I can tell you this is occurring without fail for the case when the response.statusCode === 200, but the response.headers.status === 304.

Expected Behavior

For net.request response handler to receive the content-type that clearly should exist on the response (proved by webRequest.onHeadersReceived).

Actual Behavior

net.request response handler is receiving what seems to be a cached set of headers (or something?), which do not match what is seen in webRequest.onHeadersReceived.

[ckerr]

Thanks for reporting this and helping to make Electron better!

Would it be possible for you to make a standalone testcase with only the code necessary to reproduce the issue? For example, Electron Fiddle is a great tool for making small test cases and makes it easy to publish your test case to a gist that Electron maintainers can use.

Stand-alone test cases make fixing issues go more smoothly: it ensure everyone's looking at the same issue, it removes all unnecessary variables from the equation, and it can also provide the basis for automated regression tests.

I'm adding the blocked/needs-repro label for this reason. After you make a test case, please link to it in a followup comment.

Thanks in advance! Your help is appreciated.

[Mr-Wallet]

No repro example yet, but just to add some details we've found since: net seems to be adding the status header on its own. We've mostly (but not exclusively) been testing against GitHub's REST API, and we've confirmed through Postman that the servers we've hit don't include the status header, but it consistently comes back 304 on many requests, unless we obliterate the Electron/Cache folder from the user data. So far it seems like we're getting back legitimate 200s and net is arbitrarily adding status: 304 and deleting the content-type header (and removing custom headers, #27894 which may be the same issue). When we force GitHub to give us a 304 by including an ETag in If-None-Match, net.request gives us back a completely normal-looking 304 with no body. Basically we're just really perplexed by the inconsistent and arbitrary way that net sort-of-does-sort-of-doesn't transparently populate 304 results from the Electron cache, or just decides not to even attempt such a population at all. We haven't been able to find any existing issues or documentation that speak to its intended behavior.

[arantes555]

I just hit the same problem : electron does not repopulate content-type and content-length when a 304 is "transformed" in 200 from the cache.

Also, I can confirm that the behaviour was different on electron <= 6 : up to electron6, the 200 seen in this case correctly has content-type and content-length.

[arantes555]

Here is a standalone testcase :

https://gist.github.com/arantes555/99f8976266d340bad42b2894cb50d7d5

This shows that when the server responds with a 200, response correctly contains content-type and content-length. But when the server responds with a 304, response is shown to be a 200 but does not contain content-type and content-length.

I also showed the behaviour of onHeadersReceived that @themadtitanmathos noticed : on this callback, headers are correct.

Also, with electron@6, everything is correct.

[Mr-Wallet]

we never got to the point of setting up our own server to test, but testing against GitHub and GitLab APIs, we only got a repro while we had something in the cache. For whatever reason, when we manually set our own If-None-Match that would prompt a 304 from the server, we got back an unmolested 304 (i.e. 304 status code, no body). We came to the conclusion that it was because of a bad interaction with the Electron and/or Chrome request caches. I'm confident that if you cleared both caches and ran the 304 first, you would get back the content-type and content-length as expected. In other words, I believe it's some problematic way that the cache results are being "merged" into the final results that causes the headers to drop. The same mechanism is probably also preventing the onHeadersReceived callback from working (#27894) by overriding the results after the callback occurs.

[nornagon]

Thanks for the standalone repro @arantes555! With that I've been able to confirm that this also happens on macOS, and in 12.x and 13.x.

[nornagon]

This looks like a regression caused by #21552. As an experiment, I tried adding the non-"raw" headers to the internal response object:

diff --git a/shell/browser/api/electron_api_url_loader.cc b/shell/browser/api/electron_api_url_loader.cc
index afe48e40c..6a836f7af 100644
--- a/shell/browser/api/electron_api_url_loader.cc
+++ b/shell/browser/api/electron_api_url_loader.cc
@@ -68,6 +68,22 @@ struct Converter<network::mojom::CredentialsMode> {
   }
 };  // namespace gin

+template <>
+struct Converter<net::HttpResponseHeaders> {
+  static v8::Local<v8::Value> ToV8(
+      v8::Isolate* isolate,
+      const net::HttpResponseHeaders& headers) {
+    gin_helper::Dictionary dict = gin::Dictionary::CreateEmpty(isolate);
+    size_t i = 0;
+    std::string name;
+    std::string value;
+    while (headers.EnumerateHeaderLines(&i, &name, &value)) {
+      dict.Set(name, value);
+    }
+    return dict.GetHandle();
+  }
+};
+
 }  // namespace gin

 namespace electron {
@@ -561,6 +577,8 @@ void SimpleURLLoaderWrapper::OnResponseStarted(
   DCHECK(response_head.raw_request_response_info);
   dict.Set("rawHeaders",
            response_head.raw_request_response_info->response_headers);
+  dict.Set("headers",
+           *response_head.headers);
   Emit("response-started", final_url, dict);
 }
 

(NB. above patch is hacky and doesn't allow for repeated headers, it was just enough to let me see whether this theory about raw headers was correct.)

The headers object contained the correct content-type and content-length, even when the rawHeaders didn't. For example:

As further confirmation, the above PR was backported to 7.x in v7.1.10. When I run the fiddle on v7.1.9, it works as expected.

cc @zcbenz

[zcbenz]

Comparing the response.headers between 6 and 7, it seems that we should pass filtered headers instead of the raw headers to JS. So #21552 should be reverted, but we would then need a fix to #20631.

[Mr-Wallet]

We're not exactly sure why (this was reported from end users), but in very rare cases it's clear that this is also happening on non-304 responses as well, and no extra status header is added in those cases.

Right now our solution is to play fast and loose with content-type and simply attempt to parse anything missing its content-type as a JSON and see if it works. Hopefully nobody responds with plain-text null/true while the content-type is being dropped 🤞

[arantes555]

I can confirm this is still happening up to 16.0.0-beta.2 .

Also, here is a simplified test-case : https://gist.github.com/arantes555/4849273fe59b87313f0cb472ae65ae5b

[Mr-Wallet]

Thanks for checking, @arantes555.

For our part, despite our best efforts to work around this problem, we've continued to hear about mysterious proxy issues with users (which may be this exact issue, or an unrelated problem with net.request). We're planning to abandon net.request and try to do all our HTTP(S) requests strictly from renderers using more field-tested front-end browser APIs, and IPC to pipe data as necessary whenever the main process wants something from the internet. 🤷

[arantes555]

@Mr-Wallet Honestly, that is not a bad way to proceed ^^ the net library sometimes feels a little, let's say not a top priority for electron

[Mr-Wallet]

Yeah, not to throw shade on such an amazing free (!) project... it seems to me that if other Electron apps were trying to use net to the degree that my team is, there would be more open issues and way more noise on them.

Certainly, the limited attention available for Electron is going to mostly go with the majority of use cases, and most apps seem to be single-renderer and do absolutely everything from inside that single renderer. We're hoping that by acting more like a "normal" Electron app with regards to networking, we'll sidestep these issues entirely.

[anderklander]

This is still an issue in 18.2.2 (and 19.0.0-beta.5). Any news or plans to fix this?

[taras-dubyk]

I have encountered the same issue while working with GitHub API.
Content-type header and some GitHub custom headers are missing if a response is returned from the cache.
Is there any workaround except disabling caching using the 'Cache-Control': 'no-cache' header?

[github-actions]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

[arantes555]

bump

[RealAlphabet]

Any news ?
PS: details also does not return the list of headers that are supposed to be used by the request. This seems to be a precedence error. onBeforeSendHeaders should be triggered after the "default" headers have been populated.

[nornagon]

Right, seems like the problem is that we're using rawHeaders instead of headers to fill the header list, but the content-type / content-length only come back in headers. Seems we might have to merge these somehow in order to fix this but preserve the fix for #20631.