373546 - URIs in <guid isPermaLink="false"> are recognized as valid links of feed items in the feed reading view

Status: VERIFIED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Franklin Tse]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2

URIs in <guid isPermaLink="false"> are recognized as valid links of feed items in the feed reading view. Only URLs in <guid isPermaLink="true">, <guid> and <link> should be recognized as valid links.

Reproducible: Always

Steps to Reproduce:
1.Visit http://feeds.feedburner.com/RSS2-XMLBaseTest
2.Check the link of the only item
Actual Results:  
The item is currently linking to urn:uuid:5b0d36c0-af87-11db-8a0c-0002a5d5c51b

Expected Results:  
No link should be generated

The issue can also be reproduced in the latest build (2007-03-11) of Firefox 3

Comment 1

[Robert Sayre]

we'll fix this for 2.0.0.4

Comment 2

[Robert Sayre]

Attachment: make guid checking more robust

Comment 3

[Franklin Tse]

Robert,

Is values in the isPermaLink attribute case-sensitive? If it is, I don't think allowing TrUe is good.

Comment 4

[Phil Ringnalda (:philor)]

The value is utterly under-specified, but insensitive is the most reasonable conclusion. While "==='false' is not a link, any other value or no value is a link" is one reasonable interpretation of the intention of the spec author, guessing his intentions is a loser's game, and treating isPermaLink="False" as true isn't useful.

However, if I were reviewing, I'd say that check is the wrong way, and it should be !guid.getProperty("isPermaLink").toLowerCase() == "false" - "it's true unless the value is false" makes me think that while "false" and "FaLse" should be false, "true", "TrUe" and "meatcake" should all be true.

Comment 5

[Robert Sayre]

(In reply to comment #3)
> Robert,
> 
> Is values in the isPermaLink attribute case-sensitive? If it is, I don't think
> allowing TrUe is good.

RSS2 specifies no error handling.

(In reply to comment #4)
> "true", "TrUe" and "meatcake" should all be true.

An eminently sensible approach for server-side software driven by things like feedparser.py. For clients that we have to live with for years, a more conservative approach is called for, otherwise we could break the Web later. So, no "meatcake" at this time.

Comment 6

[Franklin Tse]

Unless "true" (perhaps case insensitively since it seems that RSS 2 specification hasn't stated whether the value in the attribute is case-sensitive or not) is detected, all other values should be treated as "false".

Comment 7

[Asaf Romano (gone)]

Comment on attachment 259722 [details] [diff] [review]
make guid checking more robust

If the default behavior (i.e. when the attribute is _not_ set) is to interpret the element as a real link, Phil is right - we should not treat the attribute unless it is set to "false" (whatever you decide to with "fALSE" is fine by me). Well, unless some RSS spec tells you to do something insane (common, eh?).

Comment 8

[Franklin Tse]

There has been a <link> element to indicate the URL of the item, and according to the RSS 2 specification, "guid stands for globally unique identifier. It's a string that uniquely identifies the item.". I do not think that it is suitable for a feed reader to use the identifier as the link unless there is isPermaLink="true" or no isPermaLink attribute and thus the guid is assumed to be the URL.

Taking <atom:link> as an example, when there is no rel attribute, the link is assumed to be equal to <atom:link rel="alternate">. Does it mean that when no <atom:link rel="alternate"> is found, Firefox can use <atom:link rel="replies">, <atom:link rel="via"> or something else as the link to the document?

Comment 9

[Phil Ringnalda (:philor)]

atom:link isn't a valid comparison - there are infinite possible defined values for its rel attribute, while isPermaLink only has three: true, false, error-recovery.

However, in another bug, yes we probably should fall back to grovelling for an atom:link, any atom:link - rel "does not impose any behavioral requirements on Atom Processors" and at least the livemarks consumer is completely reliant on having some sort of link.

Comment 10

[Franklin Tse]

I don't see any needs to recover isPermaLink="unknown". Feed writers should use the <link> element to indicate the URL of an item.

Regarding atom:link, please don't change the existing behavior - only atom:link with no rel attribute or rel="alternate" should be generated as a link to an entry at the moment. Otherwise, it will break the compatibility with future "rel" values.

Comment 11

[Robert Sayre]

(In reply to comment #7)
> (From update of attachment 259722 [details] [diff] [review])
> If the default behavior (i.e. when the attribute is _not_ set) is to interpret
> the element as a real link, Phil is right - we should not treat the attribute
> unless it is set to "false" (whatever you decide to with "fALSE" is fine by
> me). Well, unless some RSS spec tells you to do something insane (common, eh?).

Well, I was more worried about unilaterally authored specs declaring us broken for some new value, but I don't feel strongly about it. new patch in a bit.

Comment 12

[Robert Sayre]

Attachment: all but case variations on "false" are considered links

Comment 13

[Asaf Romano (gone)]

Comment on attachment 261966 [details] [diff] [review]
all but case variations on "false" are considered links

r=mano

Comment 14

[Robert Sayre]

Checking in src/FeedProcessor.js;
/cvsroot/mozilla/toolkit/components/feeds/src/FeedProcessor.js,v  <--  FeedProcessor.js
new revision: 1.26; previous revision: 1.25
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_false.xml,v
done
Checking in test/xml/rss2/item_guid_isPermaLink_false.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_false.xml,v  <--  item_guid_isPermaLink_false.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_false_uppercase.xml,v
done
Checking in test/xml/rss2/item_guid_isPermaLink_false_uppercase.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_false_uppercase.xml,v  <--  item_guid_isPermaLink_false_uppercase.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_true_uppercase.xml,v
done
Checking in test/xml/rss2/item_guid_isPermaLink_true_uppercase.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_true_uppercase.xml,v  <--  item_guid_isPermaLink_true_uppercase.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_unknown_value.xml,v
done
Checking in test/xml/rss2/item_guid_isPermaLink_unknown_value.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_unknown_value.xml,v  <--  item_guid_isPermaLink_unknown_value.xml
initial revision: 1.1
done

Comment 15

[Daniel Veditz [:dveditz] out until Nov 10]

Comment on attachment 261966 [details] [diff] [review]
all but case variations on "false" are considered links

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 16

[juan becerra [:juanb]]

Robert, Please land your 1.8.1.4 approved patches as soon as possible. Code freeze was pushed out to this Friday, April 27. 

Comment 17

[Franklin Tse]

What will happen if the patch fails to land on time?

Comment 18

[Robert Sayre]

(In reply to comment #17)
> What will happen if the patch fails to land on time?
> 

It would miss 2.0.0.4 and be included in 2.0.0.5. But don't worry, I'm dealing with this right now.

Comment 19

[Robert Sayre]

cvs commit: Examining test/xml/rss2
Checking in src/FeedProcessor.js;
/cvsroot/mozilla/toolkit/components/feeds/src/FeedProcessor.js,v  <--  FeedProcessor.js
new revision: 1.1.2.18; previous revision: 1.1.2.17
done
Checking in test/xml/rss2/item_guid_isPermaLink_false.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_false.xml,v  <--  item_guid_isPermaLink_false.xml
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in test/xml/rss2/item_guid_isPermaLink_false_uppercase.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_false_uppercase.xml,v  <--  item_guid_isPermaLink_false_uppercase.xml
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in test/xml/rss2/item_guid_isPermaLink_true_uppercase.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_true_uppercase.xml,v  <--  item_guid_isPermaLink_true_uppercase.xml
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in test/xml/rss2/item_guid_isPermaLink_unknown_value.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rss2/item_guid_isPermaLink_unknown_value.xml,v  <--  item_guid_isPermaLink_unknown_value.xml
new revision: 1.1.2.1; previous revision: 1.1
done

Comment 20

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.4 using:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4pre) Gecko/2007042803 BonEcho/2.0.0.4pre

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.4pre) Gecko/2007042805 BonEcho/2.0.0.4pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.4pre) Gecko/2007042805 BonEcho/2.0.0.4pre

There is no link generated on this test site from comment #0 -> adding verified keyword.

Comment 21

[Franklin Tse]

Verified Fixed:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4