Newsgroups, link: "Click here to remove all expired articles" stopped working since 52.0

RESOLVED FIXED in Thunderbird 55.0

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: andrew.skretvedt, Assigned: jorgk)

Tracking

(Blocks 1 bug, {regression})

Thunderbird 55.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr5254+ fixed, thunderbird54 wontfix, thunderbird55 fixed)

Details

(Whiteboard: [regression:TB52])

Attachments

(1 attachment, 15 obsolete attachments)

7.17 KB, patch
rkent
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170504112025

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 when clicked has no apparent effect. The current tab shows no changes. The list of articles for the newsgroup has no changes. It's possible to continue to attempt to reopen expired articles.


Expected results:

Before v.52, clicking the link caused the header text in the tab to disappear and the article list for the newsgroup would update to reflect the current actual content of the news server. The expired articles would drop from the listing.
Another regression from bug 968342.

What is the target of the link? Can you right click and "Copy Link Location". Actually, I found such a case and the link has:
news://news.mozilla.org/mozilla.test.multimedia?list-ids
Blocks: 968342
Component: Untriaged → Networking: NNTP
Keywords: regression
Product: Thunderbird → MailNews Core
Whiteboard: [regression:TB52]
Version: 52 Branch → 52
I can confirm this Bug.

(In reply to Jorg K (GMT+2) from comment #1)
> What is the target of the link? Can you right click and "Copy Link
> Location". Actually, I found such a case and the link has:
> news://news.mozilla.org/mozilla.test.multimedia?list-ids

Correct. 'news://<NEWS-SERVER>/<NEWSGROUP>?list-ids'

The link comes from:

https://dxr.mozilla.org/comm-central/source/mailnews/news/src/nsNNTPProtocol.cpp#1998

And leads to:

https://dxr.mozilla.org/comm-central/source/mailnews/news/src/nsNntpUrl.cpp#211
Posted patch content-policy-debug.patch (obsolete) — Splinter Review
Alfred, thanks for the analysis, very helpful and much appreciated.

Here is my content policy debug patch which I have attached to many bugs already.
With this patch I see:

=== nsMsgContentPolicy::ShouldLoad: URI=|news://news.mozilla.org/mozilla.test.multimedia?list-ids|
=== nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/0/1
(snip)
=== Spec content news://news.mozilla.org/mozilla.test.multimedia?list-ids
=== Spec request moz-nullprincipal:{296c6486-7407-426e-b973-3fd0d13b17a9}
=== returning (3B) -1

Before bug 968342 we would load just any content which had a mailnews URL. The test implemented in bug 968342 compared pre-paths, so:
=== Prepath content news://news.mozilla.org
=== Prepath request moz-nullprincipal:

but for message URLs we changed this again in bug 1361020 to compare "principal specs", but only if content and requester have message URLs and clearly here the content has a news (message) URL and the requester doesn't, hence the rejection.
Assignee: nobody → jorgk
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Let's punch a little hole into our recently tightened security so some things work again ;-)

Alfred, are you able to test this. I don't have any expired message but one. I can now click on the link, but nothing happens since there's something wrong with that newsgroup I believe. Should now work again.
Flags: needinfo?(infofrommozilla)
Attachment #8867554 - Flags: review?(rkent)
(In reply to Jorg K (GMT+2) from comment #4)
> Created attachment 8867554 [details] [diff] [review]
> 1364723-news-info-page.patch (v1).

> wrong with that newsgroup I believe. Should now work again.

Yes, works as before. Thanks for the quick Bugfix.
Flags: needinfo?(infofrommozilla)
That's not just a little hole, that's "If I can't understand what this is, then let's just assume it is OK"  That is rarely a good choice for a security check.

Can't you figure out some positive security check that we can legitimately say is OK?
Well, as I said in comment #3, before bug 968342 we would load just any content which had a mailnews URL.

That's been tightened a lot. Now if the load does not come from a code base principal, which is what messages are using, it must come from some "auxiliary" page which was created on the fly, like the news page here:

===
Error!
newsgroup server responded:no such article
Perhaps the article has expired
<mailman.557.1468268120.18164.test-multimedia@lists.mozilla.org> (3440)
Click here to remove all expired articles <=== Link.
===

So in the true spirit of bug 968342 and bug 1361020 we're not punching any hole into the security as far as loading from messages is concerned. And messages are what hackers/attackers can send.

But if some add-on or TB internal wants to display a HTML page with some links on it, I don't think we should get in the way. The load clearly does *NOT* come from a message, so I think it's OK to do pre-bug 968342 behaviour in this case.

> Can't you figure out some positive security check that we can legitimately say is OK?
Not really, all the information we have is in comment #3: Requester has a null principal, content requested is some news: URL. OK, that second part could be added to your "positive" test. But I don't think it's necessary.
(In reply to Jorg K (GMT+2) from comment #7)
> That's been tightened a lot. Now if the load does not come from a code base
> principal, which is what messages are using, it must come from some
> "auxiliary" page which was created on the fly, like the news page here:
> 
> ===
> Click here to remove all expired articles <=== Link.
> ===

I did some simple tests.
I copied the link into an article: he doesn't work.
Even as Link inside a HTML-Article, he has no function.

But inside an HTML-Page)¹, displayed in TB, he activates the functionality.

)¹ for example with the pref: 'mailnews.start_page.url'
As discussed with Kent on IRC, he prefers to create DisplayHTMLInMessagePaneWithPrincipal() to pass a decent principal when loading the HTML info page that contains the link.

With the debug patch applied I see:
=== nsMsgContentPolicy::ShouldLoad: URI=|news://news.mozilla.org:119/mozilla.test.multimedia?list-ids|
=== nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
=== nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|news://news.mozilla.org/mozilla.test.multimedia?list-ids|
=== Spec content news://news.mozilla.org:119/mozilla.test.multimedia?list-ids
=== Spec request news://news.mozilla.org/mozilla.test.multimedia?list-ids
=== Principal spec content news://news.mozilla.org:119/mozilla.test.multimedia?list-ids
=== Principal spec request news://news.mozilla.org/mozilla.test.multimedia?list-ids
=== returning (3B) -1

So that's still a rejection since somehow the link gets a port appended, although there is none in the link on the page. So Gecko must interpret that link and get the port from somewhere.

It's a pain, as Kent said on IRC, and too late to solve now at 1 AM.
Attachment #8869785 - Flags: feedback?(rkent)
The funny thing is: If you click the link a few times, you finally get:

=== Spec content news://news.mozilla.org/mozilla.test.multimedia?list-ids
=== Spec request news://news.mozilla.org/mozilla.test.multimedia?list-ids
=== Principal spec content news://news.mozilla.org/mozilla.test.multimedia?list-ids
=== Principal spec request news://news.mozilla.org/mozilla.test.multimedia?list-ids
=== returning (3B) 1
OK, this works now and the debug shows:
=== Spec content news://news.mozilla.org:119/mozilla.test.multimedia?list-ids
=== Spec request news://news.mozilla.org:119/mozilla.test.multimedia?list-ids
=== Principal spec content news://news.mozilla.org:119/mozilla.test.multimedia?list-ids
=== Principal spec request news://news.mozilla.org:119/mozilla.test.multimedia?list-ids
=== returning (3B) 1 <=== ShouldLoad() accepted.

Alfred, can you please test this again for me.
Attachment #8867554 - Attachment is obsolete: true
Attachment #8869785 - Attachment is obsolete: true
Attachment #8867554 - Flags: review?(rkent)
Attachment #8869785 - Flags: feedback?(rkent)
Flags: needinfo?(infofrommozilla)
Attachment #8869798 - Flags: review?(rkent)
(Moved variable declaration.)
Attachment #8869798 - Attachment is obsolete: true
Attachment #8869798 - Flags: review?(rkent)
Attachment #8869799 - Flags: review?(rkent)
Sorry about the noise, need to use GetAsciiSpec() if we're printing this as HTML onto a page.
Attachment #8869799 - Attachment is obsolete: true
Attachment #8869799 - Flags: review?(rkent)
Attachment #8869801 - Flags: review?(rkent)
(In reply to Jorg K (GMT+2) from comment #13)
> Created attachment 8869801 [details] [diff] [review]
> 1364723-with-principal.patch (v2c) - alternate approach

I get the same result as in Comment 8.

Also the link in an HTML page still works. Is that a safety risk?
Because: as I just had to realize, the link already worked without any patch.

If so, Bug 968342 is not completely fixed.
Flags: needinfo?(infofrommozilla)
(In reply to Alfred Peters from comment #14)
> (In reply to Jorg K (GMT+2) from comment #13)
> > Created attachment 8869801 [details] [diff] [review]
> > 1364723-with-principal.patch (v2c) - alternate approach
> 
> I get the same result as in Comment 8.

And yes, the Link out of the replacement article works as desired.
(In reply to Alfred Peters from comment #14)
> (In reply to Jorg K (GMT+2) from comment #13)
> > Created attachment 8869801 [details] [diff] [review]
> > 1364723-with-principal.patch (v2c) - alternate approach

> Also the link in an HTML page still works. Is that a safety risk?
> Because: as I just had to realize, the link already worked without any patch.

Ok, only with a 'file:' link:

user_pref("mailnews.start_page.url", "file:///D|/Path/page.html");

With a web link it is blocked.

> If so, Bug 968342 is not completely fixed.

-?-
OK, I set pref mailnews.start_page.url to http://www.jorgk.com/newslink.html to point to a page that contains news://news.mozilla.org:119/mozilla.test.multimedia?list-ids

My debug then shows:
=== Spec content news://news.mozilla.org:119/mozilla.test.multimedia?list-ids
=== Spec request http://www.jorgk.com/newslink.html
=== returning (3B) -1

So at least from the point of view of the ShouldLoad(), that load is denied.
Posted patch content-policy-debug.patch (obsolete) — Splinter Review
Debug patch used.
Attachment #8867553 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #17)
> === Spec request http://www.jorgk.com/newslink.html
> === returning (3B) -1

So here too.

With 'file:' link:
| === nsMsgContentPolicy::ShouldLoad: URI=|file:///D:/Path/page.css|
| === nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
| === nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|file:///D:/Path/page.html|
| === nsMsgContentPolicy::ShouldLoad: URI=|file:///D:/Path/page.css|
| === nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
| === nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|file:///D:/Path/page.html|
| === nsMsgContentPolicy::ShouldLoad: URI=|news://ham.gammasigma.xy/de.mein.test3?list-ids|
| === nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
| === nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|file:///D:/Path/page.html|
<click link>
| === nsMsgContentPolicy::ShouldLoad calling SetDisableItemsOnMailNewsUrlDocshells
| === SetDisableItemsOnMailNewsUrlDocshells: URI=|news://ham.gammasigma.xy/de.mein.test3?list-ids|
| === nsMsgContentPolicy::ShouldLoad: URI=|news://ham.gammasigma.xy:119/de.mein.test3?list-ids|
| === nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
| === nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|file:///D:/Path/page.html|
| === nsMsgContentPolicy::ShouldLoad calling SetDisableItemsOnMailNewsUrlDocshells
| === SetDisableItemsOnMailNewsUrlDocshells: URI=|news://ham.gammasigma.xy:119/de.mein.test3?list-ids|
| === SetDisableItemsOnMailNewsUrlDocshells: bailing early (1)
From an article, news URIs are also blocked. Perhaps a separate bug? 

<news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org>
| === nsMsgContentPolicy::ShouldLoad: URI=|news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org|
| === nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
| === nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|news://ham.gammasigma.xy:119/k3o9v7.j8.1%40gammasigma.xy?group=de.mein.test3&key=80|
| === nsMsgContentPolicy::ShouldLoad calling SetDisableItemsOnMailNewsUrlDocshells
| === SetDisableItemsOnMailNewsUrlDocshells: URI=|news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org|
| === Spec content news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org
| === Spec request news://ham.gammasigma.xy:119/k3o9v7.j8.1%40gammasigma.xy?group=de.mein.test3&key=80
| === Principal spec content news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org
| === Principal spec request news://ham.gammasigma.xy:119/k3o9v7.j8.1%40gammasigma.xy?group=de.mein.test3&key=80
| === returning (3B) -1

News URIs do not work very well anyway. See Bug 108948.
(In reply to Alfred Peters from comment #20)
> From an article, news URIs are also blocked. Perhaps a separate bug? 

=== Principal spec content news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org
=== Principal spec request news://ham.gammasigma.xy:119/k3o9v7.j8.1%40gammasigma.xy?group=de.mein.test3&key=80

OK, so you're viewing a news article at news://ham.gammasigma.xy which has a reference to news://news.mozilla.org, right?

So yes, we block the reference since obviously this doesn't match at all. That already went broken in bug 1361020 where we're started requiring the principal specs to match. That's OK for mailbox: and IMAP: protocols to block "cross-loading" of messages, but it surely isn't OK for news: URLs since news is like HTML. I think what we should do instead is undo bug 968342 and 108948 for news completely and revert this to the pre-bug 968342 state and always load news: URLs unless they are loaded from a an e-mail (!) (mailbox, IMAP or JsAccount) message.
Comment on attachment 8869801 [details] [diff] [review]
1364723-with-principal.patch (v2c) - alternate approach

I think we messed this up completely in bug 968342 and bug 108948 at least for news. News is more like http, so of course you can load an article from a completely different site.
Attachment #8869801 - Attachment is obsolete: true
Attachment #8869801 - Flags: review?(rkent)
Damn, we messed this up in bug 968342 and bug 1361020 and need to undo large chunks of this.
OK, here comes the new approach that undoes most of bug 968342 and limits what was done in bug 1361020 to mail message cross-checking. For clarity, here is what I think we should do:

// \--------\  requester    |               |              |
// content   \------------\ |               |              |
// requested               \| mail message  | news message | http(s)/data etc.
// -------------------------+---------------+--------------+------------------
// mail message content     | load if same  | don't load   | don't load
// mailbox, imap, JsAccount | message (1)   | (2)          | (3)
// -------------------------+---------------+--------------+------------------
// news message             | don't load (4)| load (5)     | load (6)
// -------------------------+---------------+--------------+------------------
// http(s)/data, etc.       | (default)     | (default)    | (default)
// -------------------------+---------------+--------------+------------------

That's the fix for bug 1361020 is (1). We needed to load
content mailbox://tbtest%40jorgk%2Ecom@server.de/Inbox?number=1 from
request mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/5gfhfw7f.POP%20tbtest/Mail/server.de/Inbox?number=1
If mail message content is requested, then the load must come from the same mail message (and we use the normalised principal spec to identify them as the same).

The fix for bug 968342 is (4), not loading news messages if requested from a mail message.

The fix for this bug here is (6), so news messages can be loaded otherwise, even from the data: URL info page.

And the fix for the complaint in comment #20, where you can't load news message from another news message is (5):
=== Principal spec content news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org
=== Principal spec request news://ham.gammasigma.xy:119/k3o9v7.j8.1%40gammasigma.xy?group=de.mein.test3&key=80

So Alfred, testing welcome!

===

Unrelated:
For the record: I tested bug 1361020 again on trunk, and confusingly this time it doesn't work again, so I believe we still need to look into the proxy issue, although in bug 1361020 comment #18 I said that I couldn't see it any more, so that needs more investigation later.
Attachment #8869833 - Attachment is obsolete: true
Attachment #8869861 - Flags: review?(rkent)
Posted patch content-policy-debug.patch (obsolete) — Splinter Review
Rebased debug patch.
Comment tweak, sorry about the noise.
Attachment #8869861 - Attachment is obsolete: true
Attachment #8869861 - Flags: review?(rkent)
Attachment #8869866 - Flags: review?(rkent)
Posted patch content-policy-debug.patch (obsolete) — Splinter Review
Updated.
Attachment #8869864 - Attachment is obsolete: true
(In reply to Alfred Peters from comment #20)
> From an article, news URIs are also blocked. Perhaps a separate bug? 
Yes, we broke this and I'm undoing the damage now. Where can I find a suitable test case?
Well, I created one in mozilla.test.multimedia called "Link to message". In TB 45 I can click onto the link, but nothing useful happens, it offers a download. In TB 5X the link is blocked.
(In reply to Jorg K (GMT+2) from comment #28)
> (In reply to Alfred Peters from comment #20)
> > From an article, news URIs are also blocked. Perhaps a separate bug? 
> Yes, we broke this and I'm undoing the damage now. Where can I find a
> suitable test case?
> Well, I created one in mozilla.test.multimedia called "Link to message". In
> TB 45 I can click onto the link, but nothing useful happens, it offers a
> download. In TB 5X the link is blocked.

In mozilla.test.multimedia is a Mailing-list involved. So it is a
bad example to test news. My answer didn't show up till now.

Therefore I reposted my answer in mozilla.test.

News are usually plain-text.

Long link to a message-ID:
<news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org>

The server is usually not very useful because most of the readers use a
different one. So common is the shorter version:
<news:lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org>

Normally all news servers of the world are connected and the Message-Id is unique.
This means that an article can be queried from any server using only the Message-ID.
(In reply to Alfred Peters from comment #29)
> Long link to a message-ID:
> <news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org>

| === nsMsgContentPolicy::ShouldLoad: URI=|news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org|
| === nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
| === nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|news://ham.gammasigma.xy:119/2aWdnQS24YBmk77EnZ2dnUU7-W3NnZ2d%40mozilla.org?group=mozilla.test&key=4238|
| === nsMsgContentPolicy::ShouldLoad calling SetDisableItemsOnMailNewsUrlDocshells
| === SetDisableItemsOnMailNewsUrlDocshells: URI=|news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org|

> <news:lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org>

| === nsMsgContentPolicy::ShouldLoad calling SetDisableItemsOnMailNewsUrlDocshells
| === SetDisableItemsOnMailNewsUrlDocshells: URI=|news:///lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org|

Both links didn't work.
Thanks for the reply. So attachment 8869801 [details] [diff] [review] is too restrictive.

With attachment 8869866 [details] [diff] [review] the links in the message you posted to mozilla.test also don't work, as you stated, I'm onto it.
OK, using TB 45 on this message:
===
News are usually plain-text.
Long link to a message-ID:
<news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org>
The server is usually not very useful because most of the readers use a
different one. So common is the shorter version:
<news:lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org>
Alfred
===

Clicking on the first link brings up the open/save dialogue, clicking in the second link does nothing. Both not really useful.

What am I doing wrong?
Posted patch content-policy-debug.patch (obsolete) — Splinter Review
OK, the code was right, the debug patch was wrong, since it made it:
if (!requestNntpURL)
  printf("=== returning (3C) %d\n", (int) *aDecision); return NS_OK; // (4)
Doh!

With the correct debug patch and the correct code we're reverting to TB 45 and pre-bug bug 968342 and pre-bug 1361020 behaviour as it should be!

Now, as I said in comment #32, that's not exactly useful either. Please enlighten me. Alfred, are you on IRC?
Attachment #8869868 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #33)
> With the correct debug patch and the correct code we're reverting to TB 45
> and pre-bug bug 968342 and pre-bug 1361020 behaviour as it should be!
This is also TB 24 and TB 31 behaviour: Click onto the first link
<news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org>
and get a open/save dialogue. Not useful unless I'm doing something wrong.
(In reply to Jorg K (GMT+2) from comment #32)
> OK, using TB 45 on this message:
> ===
> News are usually plain-text.
> Long link to a message-ID:
> <news://news.mozilla.org:119/lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org>
> The server is usually not very useful because most of the readers use a
> different one. So common is the shorter version:
> <news:lJGdnWgY2eWzCanQnZ2dnUVZ_tydnZ2d@mozilla.org>
> Alfred
> ===
> 
> Clicking on the first link brings up the open/save dialogue, clicking in the

> What am I doing wrong?

Nothing. TB fetches the article from news.mozilla.org an saves it as text file with htm extension.
So you can read it with a browser <sic!>.

This is a pretty old behavior - see Bug 11076 Comment 20

(Bug 72969 - the 'create a new account' mechanism was removed again meanwhile - I think)

> second link does nothing. Both not really useful.

Agreed.
OK, useful or not, at least working as before. So now I can just patiently wait for the review.
The issue reported here is fixed and also news links in news articles work as before.
Thanks Alfred for your valuable help!
OK, I knew that Kent wouldn't be comfortable with "just load news from any other news source". So here's the previous approach again where the info page gets a news: principal to match the link.

But in nsMsgContentPolicy.cpp I don't apply the "same principal spec" to news, but only to mail messages (mailbox, imap, JsAccount related ones).

For news, I do Neil's original pre-path test (which wasn't as bad as I thought), to allow loads of news articles from the same server.

So this repairs all shortcomings from bug 968342 and bug 1361020, the former destroying the load via the info page, the latter destroying the load of articles from the server via a link.

I got bored with redoing the debug patch, so the debug here is integrated and will be removed before landing.
Attachment #8869866 - Attachment is obsolete: true
Attachment #8870105 - Attachment is obsolete: true
Attachment #8869866 - Flags: review?(rkent)
Attachment #8870143 - Flags: review?(rkent)
Comment on attachment 8869866 [details] [diff] [review]
1364723-message-cross-loading2.patch (v1b).

Review of attachment 8869866 [details] [diff] [review]:
-----------------------------------------------------------------

The issue of loading has two aspects: load authenticated or load plain (so to say). 

Haven't thought through if that's an issue here, but wanted to mention it. No message should be allowed to do authenticated loads if it's not same domain.

::: mailnews/base/src/nsMsgContentPolicy.cpp
@@ +266,5 @@
> +        if (contentPrincipalSpec.Equals(requestPrincipalSpec))
> +          *aDecision = nsIContentPolicy::ACCEPT; // (1)
> +      }
> +      return NS_OK; // (2) and (3)
> +    } else {

No else after return.

@@ +436,4 @@
>    // to this function by matching content location and requesting location.
>    if (MsgLowerCaseEqualsLiteral(contentScheme, "mailto") ||
>        MsgLowerCaseEqualsLiteral(contentScheme, "addbook") ||
> +      MsgLowerCaseEqualsLiteral(contentScheme, "pop") ||

I don't think this exists? That's mailbox:// right?
Attachment #8869866 - Attachment is obsolete: false
Comment on attachment 8869866 [details] [diff] [review]
1364723-message-cross-loading2.patch (v1b).

Sorry, patches change quickly here, this one has been withdrawn.
Attachment #8869866 - Attachment is obsolete: true
There is another option:
Instead of providing a principal spec for news: URLs, we just don't. Then we don't have to special-case those URLs in nsMsgContentPolicy.cpp and the check will just default back to the pre-path check.

As we saw, the principal spec is not real useful for news: URLs, so perhaps this is the better approach.
Attachment #8870298 - Flags: review?(rkent)
(In reply to Jorg K (GMT+2) from comment #40)
> Created attachment 8870298 [details] [diff] [review]
> 1364723-with-principal.patch (v4) - alternate approach - take 2

'remove all expired articles'-link does work

'get message-id' with account server does work

'get message-id' with other server doesn't work
Alfred, thanks for testing. (v3) does the same, right?
> 'remove all expired articles'-link does work - expected.
> 'get message-id' with account server does work - expected.
> 'get message-id' with other server doesn't work - expected.
Well, in bug 968342 Neil, a former very well skilled mailnews peer, introduced the pre-path check. He would have been aware that that would stop retrieval from other servers, since clearly, the URL has a different pre-path.
Since we're tightening security, it might be a good thing to restrict access to the server where the newsgroup is hosted, don't you think? If that's not a desired restriction, then we need to use the patch in attachment 8869866 [details] [diff] [review].
(In reply to Jorg K (GMT+2) from comment #42)
> Alfred, thanks for testing. (v3) does the same, right?
Compilation is running.

> > 'get message-id' with other server doesn't work - expected.
> Well, in bug 968342 Neil, a former very well skilled mailnews peer,
> introduced the pre-path check. He would have been aware that that would stop
> retrieval from other servers, since clearly, the URL has a different
> pre-path.
> Since we're tightening security, it might be a good thing to restrict access
> to the server where the newsgroup is hosted, don't you think?

Where exactly is the security breach? If there is one it's OK.

But:
As I wrote in mozilla.test, the link with the server specification is rather an exception.

Normally, NNTP servers are connected worldwide and articles are distributed worldwide. In this case, it is enough to specify only the Message-ID (<news:$message-id$>) to identify an article. Any server running the containing group can deliver this Article.
That this doesn't work in the TB is a completely different bug.

On the other hand, there are servers with local groups. 'News.mozilla.org' with his mozilla.* groups is such a server.

If someone wants to link an article from a 'mozilla.*' Group inside a non-mozilla group, he must specify the server so that other peoples clients can request the article directly from the mozilla server. Exactly this will unfortunately no longer work in TB.
(In reply to Jorg K (GMT+2) from comment #42)
> Alfred, thanks for testing. (v3) does the same, right?

Yes.
(In reply to Alfred Peters from comment #43)
> Where exactly is the security breach? If there is one it's OK.
I don't think there is any, it's just that we've gone a little paranoid lately ;-(

> Normally, NNTP servers are connected worldwide and articles are distributed
> worldwide. In this case, it is enough to specify only the Message-ID
> (<news:$message-id$>) to identify an article. Any server running the
> containing group can deliver this Article.
> That this doesn't work in the TB is a completely different bug.
Right.

> If someone wants to link an article from a 'mozilla.*' Group inside a
> non-mozilla group, he must specify the server so that other peoples clients
> can request the article directly from the mozilla server. Exactly this will
> unfortunately no longer work in TB.
Nothing has landed here so far. There is still the "lenient" approach in attachment 8869866 [details] [diff] [review].

Sadly I haven't heard from Magnus or Kent where they want to take it.

Magnus, do you have a preferred approach? Leave the pre-path check and limit references to the same server or go with the approach you commented on in attachment 8869866 [details] [diff] [review]? I can polish this further.
Flags: needinfo?(mkmelin+mozilla)
(In reply to Magnus Melin from comment #38)
> > +      MsgLowerCaseEqualsLiteral(contentScheme, "pop") ||
> I don't think this exists? That's mailbox:// right?
I'm restoring the original code here:
https://hg.mozilla.org/comm-central/rev/c322afd9bf99c3b9c168e1319ccd4b419c209564#l1.64
I agree, "pop" don't make much sense, we use "pop3" for a URL that describes a pop3 server. I don't know whether I need that in  the ShouldLoad() function.
OK, so by popular demand (of a single gentleman, Alfred, who's been lobbying free news access) I've polished the lenient patch by taking Magnus' comments on board.
Flags: needinfo?(mkmelin+mozilla)
Attachment #8870552 - Flags: review?(rkent)
Attachment #8870552 - Flags: review?(mkmelin+mozilla)
Blocks: 1361020
Rebased.

The other two patches have bitrotted, too, but I guess they are not acceptable solutions anyway, so I won't rebase them.
Attachment #8870552 - Attachment is obsolete: true
Attachment #8870552 - Flags: review?(rkent)
Attachment #8870552 - Flags: review?(mkmelin+mozilla)
Attachment #8872824 - Flags: review?(rkent)
Attachment #8872824 - Flags: review?(mkmelin+mozilla)
Attachment #8870143 - Attachment is obsolete: true
Attachment #8870143 - Flags: review?(rkent)
Comment on attachment 8872824 [details] [diff] [review]
1364723-message-cross-loading2.patch (v1c) - rebased

Review of attachment 8872824 [details] [diff] [review]:
-----------------------------------------------------------------

Yes this looks like a decent solution to me to move forward without abandoning other improvements. Thanks for working on this!
Attachment #8872824 - Flags: review?(rkent) → review+
Comment on attachment 8870298 [details] [diff] [review]
1364723-with-principal.patch (v4) - alternate approach - take 2

We won't be using this approach.
Attachment #8870298 - Attachment is obsolete: true
Attachment #8870298 - Flags: review?(rkent)
Attachment #8872824 - Flags: review?(mkmelin+mozilla)
https://hg.mozilla.org/comm-central/rev/e915a8b2f1f505d7c4fa1820f35ce99d1164b293
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Comment on attachment 8872824 [details] [diff] [review]
1364723-message-cross-loading2.patch (v1c) - rebased

[Approval Request Comment]
Regression caused by (bug #): bug 968342.
Attachment #8872824 - Flags: approval-comm-esr52?
Attachment #8872824 - Flags: approval-comm-beta+
Comment on attachment 8872824 [details] [diff] [review]
1364723-message-cross-loading2.patch (v1c) - rebased

I heard that we're not doing another beta.
Attachment #8872824 - Flags: approval-comm-esr52?
Attachment #8872824 - Flags: approval-comm-esr52+
Attachment #8872824 - Flags: approval-comm-beta+
Beta (TB 54):
https://hg.mozilla.org/releases/comm-beta/rev/0efb66a4b2cb16e05d20473a64eb8c6f92ea9b45
Not marking TB 54 fixed since it never shipped in a TB 54 beta.
Blocks: 1372411
Blocks: 1372414
As followup, filed:
Bug 1372411 - Fix UI properly, so that this bug doesn't even appear in the first place.
Bug 1372414 - Tighten security again (essentially revert this bug here. But the new code in the patch here is nice, so the future fix would not be a revert, but change a few lines only)
No longer blocks: 1372411
(In reply to Alfred Peters from comment #8)
> I did some simple tests.
> I copied the link into an article: he doesn't work.

(In reply to Alfred Peters from comment #14)
> > Created attachment 8869801 [details] [diff] [review]
> > 1364723-with-principal.patch (v2c) - alternate approach
> I get the same result as in Comment 8.

Unfortunately, I lost sight of this. With the later versions of the patch, this point has unfortunately changed.

'news://<NEWS-SERVER>/<NEWSGROUP>?list-ids'

The action is executed by clicking on this link in a normal article.
Yes, as you requested (see comment #47), any news: URL is now accessible from any news article. The downside is that even administrative URLs like this one are always accessible.

Ben filed bug 1372411 for the fact that this mechanism isn't optimal. Let's continue the discussion over there.

With bug 492329 landed, it should be easier to address bug 1372411 since we can change the mechanism that purges expired articles from the message store.
You need to log in before you can comment on or make changes to this bug.