Repository: mmcdole/gofeed
Pull request #222 (2024-02-24 23:08:03)

Atom: use correct xml:base for decoded elements
The issue is that goxpp's DecodeElement() pop's the BaseStack after unmarshaling an element -- as it should to keep the BaseStack in sync with the current document scope. But that means the atom parser needs to keep a reference to the current base before calling DecodeElement() so that it can correctly resolve URLs.

Without this fix, elements with xml:base attributes will be erroneously resolved with the parent xml:base.

This fix is a little awkward with all of its saving and restoring of BaseStacks. A simpler solution would be to simply get the `xml:base` attribute from the decoded element and resolve against that. However, that would require changes to `goxpp`: either change `XmlBaseResolveUrl()` to accept a string rather than acting as a method, or maybe adding a separate `ResolveUrl()` function that takes a base and a relative URL that `gofeed` could use.

mmcdole: Did this issue with base decoding only impact Atom relative URL parsing @cristoper or RSS as well?

I agree that I think we should strive for a solution that avoids the need to save the old element and do the record keeping in the gofeed parser if we can avoid it. 

I'm for modifying XPP to support this functionality if it is less awkward for it to be the one to do it.
mmcdole: > This fix is a little awkward with all of its saving and restoring of BaseStacks. A simpler solution would be to simply get the xml:base attribute from the decoded element and resolve against that. However, that would require changes to goxpp: either change XmlBaseResolveUrl() to accept a string rather than acting as a method, or maybe adding a separate ResolveUrl() function that takes a base and a relative URL that gofeed could use.

Maybe this makes sense? 

You are suggesting that in `goxpp` we have:

```
func (p *XMLPullParser) ResolveUrl(baseURL, relativeUrl string) (string, error) {
    // Logic to resolve relativeUrl against baseURL + the current BaseStack URLs?
}
```

Then in Atom Parser, something like this?:

```
func (ap *Parser) parseAtomText(p *xpp.XMLPullParser) (string, error) {
    var text struct {
        Type     string `xml:"type,attr"`
        Mode     string `xml:"mode,attr"`
        Base     string `xml:"base,attr"` // Added to capture xml:base attribute
        InnerXML string `xml:",innerxml"`
    }

    // DecodeElement is used to unmarshal the element
    err := p.DecodeElement(&text)
    if err != nil {
        return "", err
    }

    // Use the captured xml:base if present; otherwise, use the current base URL
    baseURL := text.Base
    if baseURL == "" {
        baseURL = p.GetCurrentBaseURL() // Fallback to current base URL from the parser
    }

    // Resolve URLs in InnerXML using the determined base URL
    resolvedInnerXML, err := p.ResolveURL(baseURL, text.InnerXML)
    if err != nil {
        return "", err
    }

    return strings.TrimSpace(resolvedInnerXML), nil
}
```

cristoper: Yes, that's approximately what I had in mind. I just pushed an update to the PR which does similar but copies `XmlBaseResolveUrl` to `gofeed` so that we don't have to change `goxpp`.

However, if you'd prefer not duplicating code from `goxpp` we could instead make the change there (even by introducing a new function so we don't make any backward-incompatible change to the `goxpp` API).

Patch:
diff --git a/atom/parser.go b/atom/parser.go
index 0908114..1908203 100644
--- a/atom/parser.go
+++ b/atom/parser.go
@@ -658,6 +658,8 @@ func (ap *Parser) parseAtomText(p *xpp.XMLPullParser) (string, error) {
 		InnerXML string `xml:",innerxml"`
 	}
 
+	// get current base URL before it is clobbered by DecodeElement
+	base := p.BaseStack.Top()
 	err := p.DecodeElement(&text)
 	if err != nil {
 		return "", err
@@ -672,7 +674,7 @@ func (ap *Parser) parseAtomText(p *xpp.XMLPullParser) (string, error) {
 	if strings.Contains(result, "<![CDATA[") {
 		result = shared.StripCDATA(result)
 		if lowerType == "html" || strings.Contains(lowerType, "xhtml") {
-			result, _ = shared.ResolveHTML(p, result)
+			result, _ = shared.ResolveHTML(base, result)
 		}
 	} else {
 		// decode non-CDATA contents depending on type
@@ -683,12 +685,12 @@ func (ap *Parser) parseAtomText(p *xpp.XMLPullParser) (string, error) {
 			result, err = shared.DecodeEntities(result)
 		} else if strings.Contains(lowerType, "xhtml") {
 			result = ap.stripWrappingDiv(result)
-			result, _ = shared.ResolveHTML(p, result)
+			result, _ = shared.ResolveHTML(base, result)
 		} else if lowerType == "html" {
 			result = ap.stripWrappingDiv(result)
 			result, err = shared.DecodeEntities(result)
 			if err == nil {
-				result, _ = shared.ResolveHTML(p, result)
+				result, _ = shared.ResolveHTML(base, result)
 			}
 		} else {
 			decodedStr, err := base64.StdEncoding.DecodeString(result)
@@ -701,7 +703,7 @@ func (ap *Parser) parseAtomText(p *xpp.XMLPullParser) (string, error) {
 	// resolve relative URIs in URI-containing elements according to xml:base
 	name := strings.ToLower(p.Name)
 	if atomUriElements[name] {
-		resolved, err := p.XmlBaseResolveUrl(result)
+		resolved, err := shared.XmlBaseResolveUrl(base, result)
 		if resolved != nil && err == nil {
 			result = resolved.String()
 		}
diff --git a/go.mod b/go.mod
index 68eb85f..326fcef 100644
--- a/go.mod
+++ b/go.mod
@@ -5,7 +5,7 @@ go 1.19
 require (
 	github.com/PuerkitoBio/goquery v1.8.0
 	github.com/json-iterator/go v1.1.12
-	github.com/mmcdole/goxpp v1.1.0
+	github.com/mmcdole/goxpp v1.1.1-0.20240225020742-a0c311522b23
 	github.com/stretchr/testify v1.8.1
 	github.com/urfave/cli v1.22.3
 	golang.org/x/net v0.4.0
diff --git a/go.sum b/go.sum
index bdcf661..62f5ad7 100644
--- a/go.sum
+++ b/go.sum
@@ -11,8 +11,8 @@ github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs
 github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM=
 github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo=
-github.com/mmcdole/goxpp v1.1.0 h1:WwslZNF7KNAXTFuzRtn/OKZxFLJAAyOA9w82mDz2ZGI=
-github.com/mmcdole/goxpp v1.1.0/go.mod h1:v+25+lT2ViuQ7mVxcncQ8ch1URund48oH+jhjiwEgS8=
+github.com/mmcdole/goxpp v1.1.1-0.20240225020742-a0c311522b23 h1:Zr92CAlFhy2gL+V1F+EyIuzbQNbSgP4xhTODZtrXUtk=
+github.com/mmcdole/goxpp v1.1.1-0.20240225020742-a0c311522b23/go.mod h1:v+25+lT2ViuQ7mVxcncQ8ch1URund48oH+jhjiwEgS8=
 github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
 github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg=
 github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
diff --git a/internal/shared/extparser.go b/internal/shared/extparser.go
index 79c8d5a..cb522b1 100644
--- a/internal/shared/extparser.go
+++ b/internal/shared/extparser.go
@@ -12,18 +12,15 @@ import (
 // non empty prefix)
 func IsExtension(p *xpp.XMLPullParser) bool {
 	space := strings.TrimSpace(p.Space)
-	if prefix, ok := p.Spaces[space]; ok {
-		return !(prefix == "" || prefix == "rss" || prefix == "rdf" || prefix == "content")
-	}
-
-	return p.Space != ""
+	prefix := PrefixForNamespace(space, p)
+	return !(prefix == "" || prefix == "rss" || prefix == "rdf" || prefix == "content")
 }
 
 // ParseExtension parses the current element of the
 // XMLPullParser as an extension element and updates
 // the extension map
 func ParseExtension(fe ext.Extensions, p *xpp.XMLPullParser) (ext.Extensions, error) {
-	prefix := prefixForNamespace(p.Space, p)
+	prefix := PrefixForNamespace(p.Space, p)
 
 	result, err := parseExtensionElement(p)
 	if err != nil {
@@ -93,7 +90,7 @@ func parseExtensionElement(p *xpp.XMLPullParser) (e ext.Extension, err error) {
 	return e, nil
 }
 
-func prefixForNamespace(space string, p *xpp.XMLPullParser) string {
+func PrefixForNamespace(space string, p *xpp.XMLPullParser) string {
 	// First we check if the global namespace map
 	// contains an entry for this namespace/prefix.
 	// This way we can use the canonical prefix for this
diff --git a/internal/shared/xmlbase.go b/internal/shared/xmlbase.go
index 88c856f..1100202 100644
--- a/internal/shared/xmlbase.go
+++ b/internal/shared/xmlbase.go
@@ -3,6 +3,7 @@ package shared
 import (
 	"bytes"
 	"fmt"
+	"net/url"
 	"strings"
 
 	xpp "github.com/mmcdole/goxpp"
@@ -83,7 +84,7 @@ func resolveAttrs(p *xpp.XMLPullParser) error {
 	for i, attr := range p.Attrs {
 		lowerName := strings.ToLower(attr.Name.Local)
 		if uriAttrs[lowerName] {
-			absURL, err := p.XmlBaseResolveUrl(attr.Value)
+			absURL, err := XmlBaseResolveUrl(p.BaseStack.Top(), attr.Value)
 			if err != nil {
 				return err
 			}
@@ -95,11 +96,31 @@ func resolveAttrs(p *xpp.XMLPullParser) error {
 	return nil
 }
 
+// resolve u relative to b
+func XmlBaseResolveUrl(b *url.URL, u string) (*url.URL, error) {
+	relURL, err := url.Parse(u)
+	if err != nil {
+		return nil, err
+	}
+
+	if b == nil {
+		return relURL, nil
+	}
+
+	if b.Path != "" && u != "" && b.Path[len(b.Path)-1] != '/' {
+		// There's no reason someone would use a path in xml:base if they
+		// didn't mean for it to be a directory
+		b.Path = b.Path + "/"
+	}
+	absURL := b.ResolveReference(relURL)
+	return absURL, nil
+}
+
 // Transforms html by resolving any relative URIs in attributes
 // if an error occurs during parsing or serialization, then the original string
 // is returned along with the error.
-func ResolveHTML(p *xpp.XMLPullParser, relHTML string) (string, error) {
-	if p.BaseStack.Top() == nil {
+func ResolveHTML(base *url.URL, relHTML string) (string, error) {
+	if base == nil {
 		return relHTML, nil
 	}
 
@@ -117,7 +138,7 @@ func ResolveHTML(p *xpp.XMLPullParser, relHTML string) (string, error) {
 		if n.Type == html.ElementNode {
 			for i, a := range n.Attr {
 				if htmlURIAttrs[a.Key] {
-					absVal, err := p.XmlBaseResolveUrl(a.Val)
+					absVal, err := XmlBaseResolveUrl(base, a.Val)
 					if absVal != nil && err == nil {
 						n.Attr[i].Val = absVal.String()
 					}
diff --git a/rss/parser.go b/rss/parser.go
index 4371d31..575896e 100644
--- a/rss/parser.go
+++ b/rss/parser.go
@@ -357,7 +357,8 @@ func (rp *Parser) parseItem(p *xpp.XMLPullParser) (item *Item, err error) {
 				item.Description = result
 			} else if name == "encoded" {
 				space := strings.TrimSpace(p.Space)
-				if prefix, ok := p.Spaces[space]; ok && prefix == "content" {
+				prefix := shared.PrefixForNamespace(space, p)
+				if prefix == "content" {
 					result, err := shared.ParseText(p)
 					if err != nil {
 						return nil, err


Test patch:
diff --git a/testdata/parser/rss/rss_channel_item_content_encoded.json b/testdata/parser/rss/rss_channel_item_content_encoded.json
new file mode 100644
index 0000000..57474bb
--- /dev/null
+++ b/testdata/parser/rss/rss_channel_item_content_encoded.json
@@ -0,0 +1,8 @@
+{
+    "items": [
+        {
+            "content": "Item Description"
+        }
+    ],
+    "version": "2.0"
+}
diff --git a/testdata/parser/rss/rss_channel_item_content_encoded.xml b/testdata/parser/rss/rss_channel_item_content_encoded.xml
new file mode 100644
index 0000000..391d2d3
--- /dev/null
+++ b/testdata/parser/rss/rss_channel_item_content_encoded.xml
@@ -0,0 +1,10 @@
+<!--
+Description: rss item content encoded 
+-->
+<rss version="2.0" xmlns:content="http://purl.org/rss/1.0/modules/content/">
+    <channel>
+      <item>
+        <content:encoded xmlns="http://purl.org/rss/1.0/modules/content/">Item Description</content:encoded>
+      </item>
+    </channel>
+  </rss>
diff --git a/testdata/translator/rss/feed_item_image_-_rss_channel_item_content.xml b/testdata/translator/rss/feed_item_image_-_rss_channel_item_content.xml
index 76ce6e7..413ef69 100644
--- a/testdata/translator/rss/feed_item_image_-_rss_channel_item_content.xml
+++ b/testdata/translator/rss/feed_item_image_-_rss_channel_item_content.xml
@@ -4,7 +4,7 @@ Description: item image from content
 <rss version="0.91">
   <channel>
     <item>
-      <content><![CDATA[<img src="http://example.com/content.png">]]></content>
+      <content:encoded><![CDATA[<img src="http://example.com/content.png">]]></content:encoded>
     </item>
   </channel>
 </rss>