361531 - Ampersands in URL aren't escaped when inserted in generated base attribute

Status: VERIFIED FIXED

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

Description

[Sander]

Loading the atom feed at http://www.travellerspoint.com/photo_gallery_feed.cfm?tags=Canada&onlyShowFeatured=true the feed preview screen shows blank (feed subscription block, title "Canada - Travellerspoint Travel Photo Gallery : Featured Photos" and then nothing), whereas http://www.travellerspoint.com/photo_gallery_feed.cfm?tags=Canada shows a lot of photos.

The ever helpful error console shows 5 errors:
==============================================
Error: not well-formed
Source File: 
Line: 1, Column: 135
Source Code:
<div xmlns="http://www.w3.org/1999/xhtml" xml:base="http://www.travellerspoint.com/photo_gallery_feed.cfm?tags=Canada&onlyShowFeatured=true">--------------------------------------------------------------------------------------------------------------------------------------^

XML Parsing Error: not well-formed
Location: 
Line Number 1, Column 135:

Error: [Exception... "An invalid or illegal string was specified"  code: "12" nsresult: "0x8053000c (NS_ERROR_DOM_SYNTAX_ERR)"  location: "file:///C:/mozilla/firefox%202.0/components/FeedProcessor.js Line: 514"]
Source File: file:///C:/mozilla/firefox%202.0/components/FeedProcessor.js
Line: 514

Error: [Exception... "An invalid or illegal string was specified"  code: "12" nsresult: "0x8053000c (NS_ERROR_DOM_SYNTAX_ERR)"  location: "file:///C:/mozilla/firefox%202.0/components/FeedProcessor.js Line: 514"]
Source File: file:///C:/mozilla/firefox%202.0/components/FeedProcessor.js
Line: 514

Error: uncaught exception: [Exception... "An invalid or illegal string was specified"  code: "12" nsresult: "0x8053000c (NS_ERROR_DOM_SYNTAX_ERR)"  location: "file:///C:/mozilla/firefox%202.0/components/FeedProcessor.js Line: 514"]
==============================================

The funny thing is - that base attribute isn't present in the source; Firefox is adding it, and then not escaping the ampersand!

Tested with 2.0, with aja in #developers confirming this still to happen on trunk. (I seem to be running into bug 330271 on trunk, so can't test.)

Comment 1

[Robert Sayre]

Ok, this is a pretty atrocious error on my part, in setting up the tag stack for createDocumentFragment in nsScriptableUnescape. This will only happen with Atom feeds that have XHTML content served from a base containing an ampersand.

Reporter: the workaround is to use type="html" on your text until this is fixed.

Comment 2

[Robert Sayre]

Attachment: escape the spec on the tagStack

Comment 3

[Asaf Romano (gone)]

Comment on attachment 246310 [details] [diff] [review]
escape the spec on the tagStack

>Index: src/nsScriptableUnescapeHTML.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/feeds/src/nsScriptableUnescapeHTML.cpp,v
>retrieving revision 1.4
>diff -u -8 -p -r1.4 nsScriptableUnescapeHTML.cpp
>--- src/nsScriptableUnescapeHTML.cpp	4 Nov 2006 05:45:02 -0000	1.4
>+++ src/nsScriptableUnescapeHTML.cpp	22 Nov 2006 18:54:25 -0000
>@@ -37,17 +37,17 @@
> #include "nsString.h"
> #include "nsISupportsArray.h"
> #include "nsIComponentManager.h"
> #include "nsCOMPtr.h"
> #include "nsXPCOM.h"
> #include "nsISupportsPrimitives.h"
> #include "nsXPIDLString.h"
> #include "nsScriptLoader.h"
>-
>+#include "nsEscape.h"
> #include "nsIParser.h"
> #include "nsIDTD.h"
> #include "nsNetCID.h"
> #include "nsNetUtil.h"
> #include "nsParserCIID.h"
> #include "nsParserCIID.h"
> #include "nsIContentSink.h"
> #include "nsIHTMLToTextSink.h"
>@@ -152,17 +152,21 @@ nsScriptableUnescapeHTML::ParseFragment(
>   nsVoidArray tagStack;
>   nsCAutoString base, spec;
>   if (aIsXML) {
>     // XHTML
>     if (aBaseURI) {
>       base.Append(NS_LITERAL_CSTRING(XHTML_DIV_TAG));
>       base.Append(NS_LITERAL_CSTRING(" xml:base=\""));
>       aBaseURI->GetSpec(spec);
>-      base = base + spec;
>+      // nsEscapeHTML is good enough, because we only need to get
>+      // quotes, ampersands, and angle brackets
>+      char* escapedSpec = nsEscapeHTML(spec.get());
>+      base += escapedSpec;
>+      NS_Free(escapedSpec);

null-check escapedSpec.

r=mano otherwise assuming you've tested this.

Comment 4

[Robert Sayre]

Attachment: patch to check in

added null check

Comment 5

[Jay Patel [:jay]]

Comment on attachment 246327 [details] [diff] [review]
patch to check in

Assuming this is the correct patch, approving it for 1.8.1 branch.  a=jay for drivers, please land asap.  Thanks!

Comment 6

[Robert Sayre]

Checking in src/nsScriptableUnescapeHTML.cpp;
/cvsroot/mozilla/toolkit/components/feeds/src/nsScriptableUnescapeHTML.cpp,v  <--  nsScriptableUnescapeHTML.cpp
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/toolkit/components/feeds/test/xml/rfc4287/entry_xhtml_baseURI_with_amp.xml,v
done
Checking in test/xml/rfc4287/entry_xhtml_baseURI_with_amp.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rfc4287/entry_xhtml_baseURI_with_amp.xml,v  <--  entry_xhtml_baseURI_with_amp.xml
initial revision: 1.1
done

Comment 7

[Robert Sayre]

Checking in src/nsScriptableUnescapeHTML.cpp;
/cvsroot/mozilla/toolkit/components/feeds/src/nsScriptableUnescapeHTML.cpp,v  <--  nsScriptableUnescapeHTML.cpp
new revision: 1.1.2.5; previous revision: 1.1.2.4
done
Checking in test/xml/rfc4287/entry_xhtml_baseURI_with_amp.xml;
/cvsroot/mozilla/toolkit/components/feeds/test/xml/rfc4287/entry_xhtml_baseURI_with_amp.xml,v  <--  entry_xhtml_baseURI_with_amp.xml
new revision: 1.1.2.1; previous revision: 1.1
done

Comment 8

[Sander]

Verified fixed in 20061128, both branch and trunk. Thanks for the swift patch!

Comment 9

[juan becerra [:juanb]]

Replacing "fixed1.8.1.1" keyword with "verified1.8.1.1" to keep track of verifications.