1372411 - Newsgroups "Click here to remove all expired articles" should be in chrome UI, not in content

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[Ben Bucksch (:BenB)]

This is a follow-up of bug 1364723, to fix the UI properly

Steps to reproduce:

I visit a newsgroup which expires articles after 5 days. To reproduce, you must have a known expired article to attempt to open. In my case, I visit the group to get the current list of articles; then return later, >5 days since the posting date of a given article. Open the entry for the expired article.


Actual results:

Since v.52 (to best of my knowledge), when the message tab opens, because it cannot return the expired article, you get this text:
---
Error!
newsgroup server responded:Bad article number

Perhaps the article has expired

<XnsA76E7B2651C9Bmikedmsncom@4.79.142.203> (115502)

Click here to remove all expired articles 

---
The last line is a link, which purges expired articles.


Expected results:

* The text is in the header pane (which has XUL chrome privileges), not the content page (which has the same rights as any poster).
* Alternatively: The link stays in the content pane, but the content has an error UI implemented with a privileged chrome URL, not nntp: or data:
* The link does not say "click here".

Importance:
1. Security
The action changes your news store, so it should only be possible to trigger this from chrome UI, never from news messages. The latter is currently the case. A news poster might construct a message with such a URL as link, with an innocently looking link text. That should not be possible.
2. "Click here":
https://www.w3.org/QA/Tips/noClickHere
https://www.smashingmagazine.com/2012/06/links-should-never-say-click-here/

Comment 1

[Jorg K (CEST = GMT+2)]

(In reply to Ben Bucksch (:BenB) from comment #0)
> * Alternatively: The link stays in the content pane, but the content has an
> error UI implemented with a privileged chrome URL, not nntp: or data:
Indeed, a news: URL is not useful since they need to be loadable.
For the record, the link to clear expired messages is for example:
news://news.mozilla.org:119/mozilla.test.multimedia?list-ids

Ben, can you please elaborate on
  "the content has an error UI implemented with a privileged chrome URL".

Since we surely don't want to introduce a new protocol here, are you suggesting to use a chrome: URL and make sure the content displayed has enough privileged to load it, since usually chrome: URLs don't load from content?

Comment 2

[Ben Bucksch (:BenB)]

What I'm suggesting is:

* Put a "Clear expired articles" button into the header pane, and remove it from the content.

* Only if that turns out not to be viable, then I would suggest, as alternative, to implement the entire error UI in the content pane which says "This message could not load, because it expired" as chrome:// page. Given that messages are normally loaded from chrome://, it should work and the error UI should load with chrome privileges. Now, if you click on a news: link inside another message, and that target produces the error, then it wouldn't work. We might accept that, and just not show the button in this case, or we might decide to use alternative 1. In any case, I recommend against this latter approach. We should leave privileged actions in chrome UI, not in the content pane.

Comment 3

[Richard Marti (:Paenglab)]

Could this be related to bug 492329?

Comment 4

[Jorg K (CEST = GMT+2)]

Looks like it, I'll take a look.

Comment 5

[Jorg K (CEST = GMT+2)]

I've looked at it and refreshed the patch over there in bug 492329. The problem is that the overall logic doesn't change: You place a link in a content page (mockup: attachment 377227 [details]) and a click in content will/should trigger a privileged command.

Bug 492329 only makes a nice content page, the overall concept and mechanism remains the same, see this URL which will be triggered:

function removeExpired() {
  document.location.href = folderUri + "?list-ids";
}

Comment 6

[Jorg K (CEST = GMT+2)]

Actually, when bug 492329 lands real soon now, the "error page" will have a button instead of a link. We can therefore change the mechanism that purges expired articles from the message store by changing the button action and disable the "list-ids" trigger.

Comment 7

[Hartmut Welpmann [:welpy-cw]]

Attachment: Bug 1372411 - Replace NNTP ?list-ids URL mechanism with direct removeExpiredArticles API. r=mkmelin

The "Remove All Expired Articles" button in about:newserror previously
worked by navigating to a news:// URL with a ?list-ids query parameter
that NntpChannel interpreted specially. Replace this with a direct method
call on NntpIncomingServer to close a theoretical injection path
where a crafted news: URI could trigger privileged operations.

Changes:

• Add NntpIncomingServer.removeExpiredArticles(groupName) which uses
  LISTGROUP to fetch current article keys from the server and removes
  any local keys no longer present.
• Update newsError.js to call the new method directly instead of
  navigating to a news:?list-ids URL.
• Remove the ?list-ids handling and _removeExpired() from NntpChannel.
• Add xpcshell test for removeExpiredArticles.
• Remove the now-obsolete test_listIds test.

Comment 8

[Ben Bucksch (:BenB)]

@Hartmut: Thanks for fixing the old security bug!

I also appreciate that you found this bug report and fix it here, instead of just ingoring the old bugs and filing new bugs all the time.

Comment 9

[Francesco]

How about "Download the rest of the message" in POP? That button is in content, too, right?

Comment 10

[Hartmut Welpmann [:welpy-cw]]

(In reply to Ben Bucksch (:BenB) from comment #8)

@Hartmut: Thanks for fixing the old security bug!

I also appreciate that you found this bug report and fix it here, instead of just ingoring the old bugs and filing new bugs all the time.

My pleasure. While 'news:' URIs from non-privileged sources are sanitized since bug 1886132, so that a news message spoofing an news:error page wouldn't mean any actual harm anymore, having this expire action still even relying on a URI doesn't make much sense.

Comment 11

[Hartmut Welpmann [:welpy-cw]]

(In reply to Francesco from comment #9)

How about "Download the rest of the message" in POP? That button is in content, too, right?

I think such a message could be as easily spoofed, but I don't know it there is an actual attack vector. Isn't there already be a distinct bug for that?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/91639fef0c0b
Replace NNTP ?list-ids URL mechanism with direct removeExpiredArticles API. r=mkmelin