Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.59k stars 799 forks source link

Syntax error in module geo-location in Wordpress plugin #9885

Closed jlkreiss closed 6 years ago

jlkreiss commented 6 years ago

Hi!

Since activating the latest version of Jetpack plugin on my website, i noticed my RSS feeds became invalid.

Upon investigation, i found out that the geo-location module breaks the RSS feed in adding a new xmlns on line 237:

public function rss_namespace() {
    echo 'xmlns:georss="http://www.georss.org/georss" xmlns:geo="http://www.w3.org/2003/01/geo/wgs84_pos#" ';
    }

There is a missing space just before the xmlns:georss so as to separate it from previous namespace declarations.

The right code should be:

public function rss_namespace() {
    echo ' xmlns:georss="http://www.georss.org/georss" xmlns:geo="http://www.w3.org/2003/01/geo/wgs84_pos#" ';
    }

Thank you for correcting this bug...

Cheers Jack

brbrr commented 6 years ago

Hi @jlkreiss Thanks for the report.

I have tried to reproduce this bug on my test site, but couldn't. Could you provide an invalid RSS feed so we can test it?

I've used https://www.xmlvalidation.com/ for XML validation, and http://jetpack6test.mystagingwebsite.com/feed/ for testing

zinigor commented 6 years ago

I was able to reproduce this problem by installing another plugin that defines the geo scope for feeds, like geo-mashup. You need to install it, activate it, and then try loading any feed, like /rss, you will get a YSOD in Firefox. @griffbrad could you take a look please? I can see two ways of correcting this:

jlkreiss commented 6 years ago

Many thanks for investigating this issue!

I see I can trust you :-)

Le mar. 17 juil. 2018 à 17:34, Igor Zinovyev notifications@github.com a écrit :

I was able to reproduce this problem by installing another plugin that defines the geo scope for feeds, like geo-mashup https://wordpress.org/plugins/geo-mashup/. You need to install it, activate it, and then try loading any feed, like /rss, you will get a YSOD in Firefox. @griffbrad https://github.com/griffbrad could you take a look please? I can see two ways of correcting this:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Automattic/jetpack/issues/9885#issuecomment-405626020, or mute the thread https://github.com/notifications/unsubscribe-auth/AkuKIcsX_PTTqoeRwg9a6xdHBjfERcYPks5uHgQEgaJpZM4VGo3v .

jlkreiss commented 6 years ago

The problem is still there, see attached copy of my feed file.

https://pastebin.com/raw/PU5bteCp

wigglemuff commented 6 years ago

@zinigor @griffbrad any clue how soon this can be fixed? :) 1334472-zen

zinigor commented 6 years ago

Thanks for the heads up about this! We'll try to get it in 6.5.

Spylberg commented 6 years ago

Hey there @zinigor I do have the same problem, same line (georss) This problem must be fixed quickly please (in 6.4) ! I use rss feed with Mailchimp for my newsletter, so Mailchimp can't send my weekly mailing because of this broken rss feed. Thanks ! https://www.weelz.fr/fr/feed/

csonnek commented 6 years ago

Leaving a note to email user in 1334472-zen when this fix has been released.

mancamping commented 6 years ago

Has anyone found a solution to this yet? I have this same issue and have updated to latest Jetpack version and the problem still lingers for me!

zinigor commented 6 years ago

Looking at this again I can say that it is not as simple to solve as I initially thought. The problem is that WordPress does not let us see what namespaces were already added before, so we won't know if the one we're trying to add already exists.

An easy way to fix the problem for affected sites currently is to disable geo location support in Jetpack by using this filter:

add_filter( 'jetpack_tools_to_include', function( $tools ) {
    return array_diff( $tools, array( 'geo-location.php' ) );
} );

This will disable loading of this specific tool to avoid conflicting with other plugins.

mancamping commented 6 years ago

Do I just put this in the functions file? .... Sorry if that a noob question lol

I added this to the functions.php file but i am still getting the same error. Is there something else I need to do?

zinigor commented 6 years ago

@mancamping yes, you could also use a code snippet plugin to make it work. What error are you experiencing exactly? Do you still see the namespace attributes added by Jetpack in your feed?

mancamping commented 6 years ago

This is what i am getting from the feed validator....

This feed does not validate.

line 8, column 44: XML parsing error: :8:44: not well-formed (invalid token) [help]

xmlns:media="http://search.yahoo.com/mrss/"xmlns:georss="http://www.georss. ...
                                        ^

Source: https://www.mancamping.ca/feed/ <?xml version="1.0" encoding="UTF-8"?><rss version="2.0" xmlns:content="http://purl.org/rss/1.0/modules/content/" xmlns:wfw="http://wellformedweb.org/CommentAPI/" xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:atom="http://www.w3.org/2005/Atom" xmlns:sy="http://purl.org/rss/1.0/modules/syndication/" xmlns:slash="http://purl.org/rss/1.0/modules/slash/" xmlns:media="http://search.yahoo.com/mrss/"xmlns:georss="http://www.georss.org/georss" xmlns:geo="http://www.w3.org/2003/01/geo/wgs84_pos#" >

ManCamping.ca https://www.mancamping.ca Outdoor Gear Store | Paddle Far - Push Your Limits Tue, 28 Aug 2018 12:30:43 +0000 en-CA hourly 1 https://wordpress.org/?v=4.9.8 https://i1.wp.com/www.mancamping.ca/wp-content/uploads/2016/03/ManCamping-Logo-BW2-res.jpg?fit=32%2C32&ssl=1 ManCamping.ca https://www.mancamping.ca 32 32 67339936 3 Day-Trips That Are Absolutely Worth It In Ontario https://www.mancamping.ca/3-day-trips-that-are-absolutely-worth-it-in-ontario/ https://www.mancamping.ca/3-day-trips-that-are-absolutely-worth-it-in-ontario/#respond Tue, 28 Aug 2018 12:24:54 +0000
Spylberg commented 6 years ago

@zinigor The add_filter doesn't work...

zinigor commented 6 years ago

@Spylberg can you please contact support about this? I'm afraid the GitHub issue is not the best place to deal with this. When contacting our Happiness Engineers, can you please include you problem description as well as what you expected to happen, and what happened instead? Thank you!

aheckler commented 6 years ago

1379731-zen is the continuation of the above

onewil commented 5 years ago

Hi @zinigor did this get closed but not fixed? Still seeing this issue.

jeherve commented 5 years ago

@onewil This should have been fixed in 23645223c94fcca76eaeb255f184fd536c240220. If you still experience issues on your site when running the most recent version of the Jetpack plugin, could you contact our support team so we can investigate things further? https://jetpack.com/contact-support/?rel=support

Thank you.

jbjhjm commented 5 years ago

I think the solution could be improved. Currently the generated tag looks like this for me:

<?xml version="1.0" encoding="UTF-8"?><rss version="2.0"
    xmlns:content="http://purl.org/rss/1.0/modules/content/"
    xmlns:wfw="http://wellformedweb.org/CommentAPI/"
    xmlns:dc="http://purl.org/dc/elements/1.1/"
    xmlns:atom="http://www.w3.org/2005/Atom"
    xmlns:sy="http://purl.org/rss/1.0/modules/syndication/"
    xmlns:slash="http://purl.org/rss/1.0/modules/slash/"

xmlns:georss="http://www.georss.org/georss" xmlns:geo="http://www.w3.org/2003/01/geo/wgs84_pos#"
>

Which I find to be suboptimal. xmlns:slash and xmlns:georss are now only separated by new line. All others are separated by new line plus a tab. In my special case, the rss reader compresses the file by stripping line breaks and replacing 1+n tabs by a space. Which would work well with a document generated by simple_xml entirely as you can be sure there will be tabs in front of every sub-node.

So to insert safely into the generated document, I suggest to add a tab char in front of xmlns:georss.

jeherve commented 5 years ago

@jbjhjm I created #12051 to track your suggestion.