1803739 - Feedreader should not send Sec-Fetch-Site as "cross-site" (Feeds from Feedburner no longer work)

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Bachsau]

Steps to reproduce:

Fetch a feed from Feedburner

Actual results:

It shows an exclamation mark instead of the Favicon. After clicking the check feed format button, it complains about failed authentification.

Expected results:

New feed items should be fetched. The feed does not require login credentials and is accessible when using a web browser. Either Thunderbird started sending bogus login credentials, or it always did that, and Google ignored it, but does not anymore. Or maybe, the error message states a wrong cause.

Comment 1

[marcel]

I can confirm that this broke sometime in fall. It appears the Sec-Fetch-* headers that TB sends are to blame.

See https://groups.google.com/g/feedburner/c/YvIA5cYyfIM for details.

Comment 2

[onitake]

I wonder if this isn't actually a bug in TB.
In particular, "Sec-Fetch-Site: cross-site" seems wrong to me. Wouldn't "none" make more sense here? Why does TB think this is a cross-site request?

Comment 3

[Bachsau]

There is no reason to send any such extended header information at all. The most basic HTTP headers should suffice to fetch a public newsfeed.

Comment 4

[marcel]

(In reply to onitake from comment #2)

I wonder if this isn't actually a bug in TB.

Oh definitely, you can't really blame Feedburner here. I have so failed to get someone from the TB team to acknowledge that there's an issue. This report still in status UNCONFIRMED.

Comment 5

[Adalbert Chamisso]

If you set preference dom.security.secFetch.enabled to false, https://feeds.feedburner.com/uncrate works again. I'm not sure whether that is a good fix or workaround. Mozilla implemented those headers in bug 1508292 and TB also sends them (apparently).

Comment 6

[Bachsau]

(In reply to Adalbert Chamisso from comment #5)

If you set preference dom.security.secFetch.enabled to false, https://feeds.feedburner.com/uncrate works again. I'm not sure whether that is a good fix or workaround. Mozilla implemented those headers in bug 1508292 and TB also sends them (apparently).

Nice fix. I don't think this has any security implications for Thunderbird. It definitely should not send these headers. They are most likely an unintended side effect of the GUI being mostly written in JavaScript itself.

Comment 7

[marcel]

(In reply to Adalbert Chamisso from comment #5)

set preference dom.security.secFetch.enabled to false

Oh, nice one, thanks!

Comment 8

[Adalbert Chamisso]

(In reply to Bachsau from comment #6)

I don't think this has any security implications for Thunderbird.

Not for fetching feeds, but for showing (feed) web pages in a content tab. Sec-Fetch-Site seems to be a forbidden header to set in the request:
https://searchfox.org/comm-central/rev/9b4c04700fc7f0df72b89654bf4c1deb691ac594/mailnews/extensions/newsblog/Feed.jsm#176-200,
but the code could be moved to using fetch() where this appears to be valid, see a very recent example here:
https://phabricator.services.mozilla.com/D167449

Comment 9

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Freddy, do we have someone who can help debug this?

Comment 10

[Frederik Braun [:freddy]]

Thanks, Christoph!

As per comment 2, it looks like Thunderbird is sending sec-fetch-site: cross-site. This is going to make the request look illegitimate and therefore Thunderbird should override this for feed viewing purposes.

Given that fetching a feed is not part of normal web browsing, I would actually recommend Thunderbird stops sending these headers completely.
Ideally, we would only do so for the feed reader's XMLHttpRequest() while still allowing this for typical web browsing.

Given that the code is in privileged JavaScript (cf. https://searchfox.org/comm-central/rev/9b4c04700fc7f0df72b89654bf4c1deb691ac594/mailnews/extensions/newsblog/Feed.jsm#176-200 - thanks Adalbert!), one could pass extra loadflags on the channel or a flag on the loadinfo. The function dom::SecFetch::AddSecFetchHeader() could then bail our early based on this flag.

As a temporary workaround, people can indeed disable secFetch globally. But this may break some other websites while browsing :(

Not sure who in Thunderbird would be able to drive this to conclusion. Can you find an owner, Kai?

Comment 11

[Adalbert Chamisso]

(In reply to Frederik Braun [:freddy] from comment #10)

... thanks Adalbert!

Just for the record, some of my comments were based on observations of a former longtime TB feeds maintainer, so thanks to this person we got rolling here.

Comment 12

[Kai Engert (:KaiE:)]

Does anyone understand WHY thunderbird sends those sec-fetch-site headers?
I cannot find the string sec-fetch-site in Thunderbird code.

Is this something that firefox core code enables by default when we use XMLHttpRequest?

I'm not an expert on XMLHttpRequest.
Is there an API that can be used to easily disable sending of that?
Would that be a minimal change to the code that Freddy linked?

Comment 13

[Kai Engert (:KaiE:)]

(In reply to Adalbert Chamisso from comment #5)

If you set preference dom.security.secFetch.enabled to false, https://feeds.feedburner.com/uncrate works again. I'm not sure whether that is a good fix or workaround. Mozilla implemented those headers in bug 1508292 and TB also sends them (apparently).

That change to Firefox was made in early 2020.
It cannot be directly responsible for this regression, if you said it started 2 years later, in fall 2022.
Something else must have triggered it.

It seems those headers are being added unconditionally to every http request:
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#501

Is there any way to disable those headers per request?

Comment 14

[Kai Engert (:KaiE:)]

(Well, maybe that server started to reject requests with that header in fall 2022, and we've been sending it since 2020.)

Comment 15

[Kai Engert (:KaiE:)]

Ok, Freddy's analysis suggests, it cannot yet be disabled, but he suggested to implement new flags, to prevent the headers.

(Finally I understood the request, sorry for my unnecessary comments.)

Comment 16

[Kai Engert (:KaiE:)]

So, I think the suggestion (from Freddy) is:

• in this file:
  https://searchfox.org/mozilla-central/source/netwerk/base/nsIRequest.idl
  add a new flag, maybe NO_SEC_FETCH_HEADERS

• probably here:
  https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#501
  skip the call if the flag is set

• add that flag to TB here:
  https://searchfox.org/comm-central/rev/9b4c04700fc7f0df72b89654bf4c1deb691ac594/mailnews/extensions/newsblog/Feed.jsm#180

Comment 17

[Frederik Braun [:freddy]]

To reply on comment #13, it pretty much looks like a server change. Honoring Sec-Fetch allows for a decent CSRF-protection.
In Thunderbird's case, the whole request should not be perceived as a cross-site request at all. You're trying to do a download really, rather than a cross-site fetch() in web platform terms.

So ideally, you wouldn't create a "NO_SEC_FETCH" opt-out but rather a "treat this as a download" flag, that can address other existing and future security mechanisms (e.g., the Origin header).

Hope that helps! Feel free to flag us for additional review on top of the mandatory necko review.

Comment 18

[Kai Engert (:KaiE:)]

Attachment: WIP: Bug 1803739 - Use LOAD2_NO_SEC_FETCH_HEADERS and implement loadFlags2.

Comment 19

[Kai Engert (:KaiE:)]

Thanks Freddy, very helpful. Valentin also helped me, and I found an existing flag that we might be able to reuse.

Comment 20

[Kai Engert (:KaiE:)]

Attachment: WIP: Bug 1803739 - Set nsIHttpChannelInternal.noSecurityContext when loading a feed.

Comment 21

[Kai Engert (:KaiE:)]

This patch, together with the change from bug 1819592, fixes the issue in my testing.

Not yet asking for review, we need to wait if the change in bug 1819592 is accepted.

Comment 22

[Kai Engert (:KaiE:)]

Help wanted to implement the required tests for the Firefox change.

Comment 24

[Magnus Melin [:mkmelin]]

Bug 1819592 fixed this. No c-c changes required.