Repository: mmcdole/gofeed
Pull request #120 (2019-04-06 18:25:13)

Better support for CDATA
- adds a share function to strip CDATA tags with decoding of entities outside of the tags
- adds a basic test

Open questions:
- not sure if it's a standard way to parse CDATA. Any ideas?
- anything else that this affects?

OrKoN: It might make sense to introduce `shared.ContainsCDATA` because the check is in multiple places. Also, I am not sure if the CDATA parsing needs to depend on the content type.
mmcdole: @OrKoN thanks for your work on this.  This is definitely better than the current CDATA handling.

My original plan was a character-by-character parser of text elements which could handle CDATA elements without the need for contains() or indexof(), etc, and would have context to know when you were within an XML comment but I never got around to writing it.

However, this link has a lot of great examples/information on CDATA handling:
https://stackoverflow.com/questions/2784183/what-does-cdata-in-xml-mean

The following might make good testcases:

- Data inside the CDATA elements is not processed as XML characters
```xml
<exampleOfACDATA>
<![CDATA[
    Since this is a CDATA section
    I can use all sorts of reserved characters
    like > < " and &
    or write things like
    <foo></bar>
    but my document is still well formed!
]]>
</exampleOfACDATA>
```
or
```xml
<![CDATA[
Within this Character Data block I can
use double dashes as much as I want (along with <, &, ', and ")
*and* %MyParamEntity; will be expanded to the text
"Has been expanded" ... however, I can't use
the CEND sequence. If I need to use CEND I must escape one of the
brackets or the greater-than sign using concatenated CDATA sections.
]]>
```

- A comment following a CDATA element with CDATA end string (note that ]]> should not be stripped from the comment):
```xml
<!--
Within this comment I can use ]]>
and other reserved characters like <
&, ', and ", but %MyParamEntity; will not be expanded
(if I retrieve the text of this node it will contain
%MyParamEntity; and not "Has been expanded")
and I can't place two dashes next to each other.
-->
```

- Correct handling of escaped CENDs
```xml
<description>An example of escaped CENDs</description>
<!-- This text contains a CEND ]]> -->
<!-- In this first case we put the ]] at the end of the first CDATA block
     and the > in the second CDATA block -->
<data><![CDATA[This text contains a CEND ]]]]><![CDATA[>]]></data>
<!-- In this second case we put a ] at the end of the first CDATA block
     and the ]> in the second CDATA block -->
<alternative><![CDATA[This text contains a CEND ]]]><![CDATA[]>]]></alternative>
```
OrKoN: @mmcdole thanks for reviewing and the test cases. I have added them and it seems that the test case containing comment blocks is the only one that fails because the parser does not support comments (I didn't know there could be comments too) and `DecodeEntities` call fails on reserved characters. So comments are swallowed silently if they contain reserved characters.

Also, I am not sure that I got the expected results for test cases correctly. Perhaps some other is failing too.

Is there a way to re-use existing XML parsers to parse text nodes? 


Patch:
diff --git a/atom/parser.go b/atom/parser.go
index 2309cb8..000ace6 100644
--- a/atom/parser.go
+++ b/atom/parser.go
@@ -6,9 +6,9 @@ import (
 	"strings"
 
 	"github.com/PuerkitoBio/goquery"
-	"github.com/mmcdole/gofeed/extensions"
+	ext "github.com/mmcdole/gofeed/extensions"
 	"github.com/mmcdole/gofeed/internal/shared"
-	"github.com/mmcdole/goxpp"
+	xpp "github.com/mmcdole/goxpp"
 )
 
 var (
@@ -681,10 +681,8 @@ func (ap *Parser) parseAtomText(p *xpp.XMLPullParser) (string, error) {
 	lowerType := strings.ToLower(text.Type)
 	lowerMode := strings.ToLower(text.Mode)
 
-	if strings.HasPrefix(result, "<![CDATA[") &&
-		strings.HasSuffix(result, "]]>") {
-		result = strings.TrimPrefix(result, "<![CDATA[")
-		result = strings.TrimSuffix(result, "]]>")
+	if strings.Contains(result, "<![CDATA[") {
+		result = shared.StripCDATA(result)
 		if lowerType == "html" || strings.Contains(lowerType, "xhtml") {
 			result, _ = ap.base.ResolveHTML(result)
 		}
diff --git a/detector.go b/detector.go
index 6f0eae4..a9e665e 100644
--- a/detector.go
+++ b/detector.go
@@ -5,7 +5,7 @@ import (
 	"strings"
 
 	"github.com/mmcdole/gofeed/internal/shared"
-	"github.com/mmcdole/goxpp"
+	xpp "github.com/mmcdole/goxpp"
 )
 
 // FeedType represents one of the possible feed
diff --git a/internal/shared/parseutils.go b/internal/shared/parseutils.go
index 8c523bf..475343b 100644
--- a/internal/shared/parseutils.go
+++ b/internal/shared/parseutils.go
@@ -8,7 +8,7 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/mmcdole/goxpp"
+	xpp "github.com/mmcdole/goxpp"
 )
 
 var (
@@ -21,6 +21,9 @@ var (
 	InvalidNumericReference = errors.New("invalid numeric reference")
 )
 
+const CDATA_START = "<![CDATA["
+const CDATA_END = "]]>"
+
 // ParseText is a helper function for parsing the text
 // from the current element of the XMLPullParser.
 // This function can handle parsing naked XML text from
@@ -39,16 +42,46 @@ func ParseText(p *xpp.XMLPullParser) (string, error) {
 	result := text.InnerXML
 	result = strings.TrimSpace(result)
 
-	if strings.HasPrefix(result, "<![CDATA[") &&
-		strings.HasSuffix(result, "]]>") {
-		result = strings.TrimPrefix(result, "<![CDATA[")
-		result = strings.TrimSuffix(result, "]]>")
-		return result, nil
+	if strings.Contains(result, CDATA_START) {
+		return StripCDATA(result), nil
 	}
 
 	return DecodeEntities(result)
 }
 
+// StripCDATA removes CDATA tags from the string
+// content outside of CDATA tags is passed via DecodeEntities
+func StripCDATA(str string) string {
+	buf := bytes.NewBuffer([]byte{})
+
+	curr := 0
+
+	for curr < len(str) {
+
+		start := indexAt(str, CDATA_START, curr)
+
+		if start == -1 {
+			dec, _ := DecodeEntities(str[curr:])
+			buf.Write([]byte(dec))
+			return buf.String()
+		}
+
+		end := indexAt(str, CDATA_END, start)
+
+		if end == -1 {
+			dec, _ := DecodeEntities(str[curr:])
+			buf.Write([]byte(dec))
+			return buf.String()
+		}
+
+		buf.Write([]byte(str[start+len(CDATA_START) : end]))
+
+		curr = curr + end + len(CDATA_END)
+	}
+
+	return buf.String()
+}
+
 // DecodeEntities decodes escaped XML entities
 // in a string and returns the unescaped string
 func DecodeEntities(str string) (string, error) {
@@ -151,3 +184,11 @@ func ParseNameAddress(nameAddressText string) (name string, address string) {
 	}
 	return
 }
+
+func indexAt(str, substr string, start int) int {
+	idx := strings.Index(str[start:], substr)
+	if idx > -1 {
+		idx += start
+	}
+	return idx
+}
diff --git a/rss/parser.go b/rss/parser.go
index 9fe9029..684d160 100644
--- a/rss/parser.go
+++ b/rss/parser.go
@@ -5,9 +5,9 @@ import (
 	"io"
 	"strings"
 
-	"github.com/mmcdole/gofeed/extensions"
+	ext "github.com/mmcdole/gofeed/extensions"
 	"github.com/mmcdole/gofeed/internal/shared"
-	"github.com/mmcdole/goxpp"
+	xpp "github.com/mmcdole/goxpp"
 )
 
 // Parser is a RSS Parser
diff --git a/translator.go b/translator.go
index 244ce75..4756b4b 100644
--- a/translator.go
+++ b/translator.go
@@ -6,7 +6,7 @@ import (
 	"time"
 
 	"github.com/mmcdole/gofeed/atom"
-	"github.com/mmcdole/gofeed/extensions"
+	ext "github.com/mmcdole/gofeed/extensions"
 	"github.com/mmcdole/gofeed/internal/shared"
 	"github.com/mmcdole/gofeed/rss"
 )


Test patch:
diff --git a/internal/shared/parseutils_test.go b/internal/shared/parseutils_test.go
index 267cbb7..e2af298 100644
--- a/internal/shared/parseutils_test.go
+++ b/internal/shared/parseutils_test.go
@@ -53,3 +53,73 @@ func TestDecodeEntitiesInvalid(t *testing.T) {
 		assert.NotNil(t, err, "%q was decoded to %q", test, res)
 	}
 }
+
+func TestStripCDATA(t *testing.T) {
+	tests := []struct {
+		str string
+		res string
+	}{
+		{"<![CDATA[ test ]]>test", " test test"},
+		{"<![CDATA[test &]]> &lt;", "test & <"},
+		{"", ""},
+		{"test", "test"},
+		{"]]>", "]]>"},
+		{"<![CDATA[", "<![CDATA["},
+		{"<![CDATA[testtest", "<![CDATA[testtest"},
+		{`<![CDATA[
+    Since this is a CDATA section
+    I can use all sorts of reserved characters
+    like > < " and &
+    or write things like
+    <foo></bar>
+    but my document is still well formed!
+]]>`, `
+    Since this is a CDATA section
+    I can use all sorts of reserved characters
+    like > < " and &
+    or write things like
+    <foo></bar>
+    but my document is still well formed!
+`},
+		{`<![CDATA[
+Within this Character Data block I can
+use double dashes as much as I want (along with <, &, ', and ")
+*and* %MyParamEntity; will be expanded to the text
+"Has been expanded" ... however, I can't use
+the CEND sequence. If I need to use CEND I must escape one of the
+brackets or the greater-than sign using concatenated CDATA sections.
+]]>`, `
+Within this Character Data block I can
+use double dashes as much as I want (along with <, &, ', and ")
+*and* %MyParamEntity; will be expanded to the text
+"Has been expanded" ... however, I can't use
+the CEND sequence. If I need to use CEND I must escape one of the
+brackets or the greater-than sign using concatenated CDATA sections.
+`},
+		// 		{`<![CDATA[ test ]]><!--
+		// Within this comment I can use ]]>
+		// and other reserved characters like <
+		// &, ', and ", but %MyParamEntity; will not be expanded
+		// (if I retrieve the text of this node it will contain
+		// %MyParamEntity; and not "Has been expanded")
+		// and I can't place two dashes next to each other.
+		// -->`, ` test <!--
+		// Within this comment I can use ]]>
+		// and other reserved characters like <
+		// &, ', and ", but %MyParamEntity; will not be expanded
+		// (if I retrieve the text of this node it will contain
+		// %MyParamEntity; and not "Has been expanded")
+		// and I can't place two dashes next to each other.
+		// -->`,
+		// 		},
+		{`<![CDATA[ test ]]><!-- test -->`, ` test <!-- test -->`}, // TODO: probably wrong
+		{`An example of escaped CENDs`, `An example of escaped CENDs`},
+		{`<![CDATA[This text contains a CEND ]]]]><![CDATA[>]]>`, `This text contains a CEND ]]>`},
+		{`<![CDATA[This text contains a CEND ]]]><![CDATA[]>]]>`, `This text contains a CEND ]]>`},
+	}
+
+	for _, test := range tests {
+		res := StripCDATA(test.str)
+		assert.Equal(t, test.res, res)
+	}
+}