Fix item get_base by Alkarex · Pull Request #744 · simplepie/simplepie

Alkarex · 2022-08-30T22:03:55Z

SimplePie was not using the best base link for an item content (instead, it was directly using the feed base).
See <xml:base> example in https://datatracker.ietf.org/doc/html/rfc4287#section-1.1
First uses item <xml:base> if it exists, or the item own link, or the feed's base URL rules (feed URL, or Web site URL)
Downstream issue FreshRSS/FreshRSS#4562
Downstream PR FreshRSS/FreshRSS#4565

SimplePie was not using the best base link for an item content (instead, it was directly using the feed base). See `<xml:base>` example in https://datatracker.ietf.org/doc/html/rfc4287#section-1.1 First uses item `<xml:base>` if it exists, or the item own link, or the feed's base URL rules (feed URL, or Web site URL) Downstream issue FreshRSS/FreshRSS#4562 Downstream PR FreshRSS/FreshRSS#4565

Alkarex · 2022-08-30T22:07:02Z

Example of buggy feed https://about.gitlab.com/atom.xml

https://simplepie.org/demo/?feed=https%3A%2F%2Fabout.gitlab.com%2Fatom.xml

See article https://about.gitlab.com/releases/2022/08/30/critical-security-release-gitlab-15-3-2-released

<a href="#remote-command-execution-via-github-import">Remote Command Execution via GitHub import</a>

Should resolve to https://about.gitlab.com/releases/2022/08/30/critical-security-release-gitlab-15-3-2-released/#remote-command-execution-via-github-import but SimplePie produced https://about.gitlab.com/blog/#remote-command-execution-via-github-import

Alkarex · 2022-08-30T22:34:20Z

P.S. I cannot think of a rationale for the current logic of falling back directly to the feed's URL instead of using the item's URL.

Art4 · 2022-08-31T07:48:12Z

Would it be possible to create a unit test for it? This would help to document and enforce the behavior in the future.

Alkarex · 2022-08-31T08:23:26Z

Would it be possible to create a unit test for it? This would help to document and enforce the behavior in the future.

Most likely, but independently of this PR, tests are failing on my machine and I did not look into the details (probably a wrong version constraint in composer.json and/or lack of composer.lock):

$ php -v
PHP 8.1.2 (cli) (built: Jul 21 2022 12:10:37) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.2, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.2, Copyright (c), by Zend Technologies

$ composer test
> phpunit
PHPUnit 9.5.16 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

.............................................................   61 / 1892 (  3%)
.............................................................  122 / 1892 (  6%)
.............................................................  183 / 1892 (  9%)
.............................................................  244 / 1892 ( 12%)
.............................................................  305 / 1892 ( 16%)
.............................................................  366 / 1892 ( 19%)
.............................................................  427 / 1892 ( 22%)
.............................................................  488 / 1892 ( 25%)
.............................................................  549 / 1892 ( 29%)
.............................................................  610 / 1892 ( 32%)
.............................................................  671 / 1892 ( 35%)
.............................................................  732 / 1892 ( 38%)
.............................................................  793 / 1892 ( 41%)
.............................................................  854 / 1892 ( 45%)
.............................................................  915 / 1892 ( 48%)
.............................................................  976 / 1892 ( 51%)
............................................................. 1037 / 1892 ( 54%)
............................................................. 1098 / 1892 ( 58%)
............................................................. 1159 / 1892 ( 61%)
............................................................. 1220 / 1892 ( 64%)
............................................................. 1281 / 1892 ( 67%)
............................................................. 1342 / 1892 ( 70%)
............EEEEEEEEEE..............EEE...................... 1403 / 1892 ( 74%)
............................................................. 1464 / 1892 ( 77%)
............................................................. 1525 / 1892 ( 80%)
............................................................. 1586 / 1892 ( 83%)
............................................................. 1647 / 1892 ( 87%)
.FFF......................................................... 1708 / 1892 ( 90%)
.....PHP Notice:  is invalid XML, likely due to invalid characters. XML error: Invalid document end at line 3, column 8 in /home/alexandre/GitHub/simplepie/src/SimplePie.php on line 1511
.PHP Notice:  is invalid XML, likely due to invalid characters. XML error: Invalid document end at line 3, column 8 in /home/alexandre/GitHub/simplepie/src/SimplePie.php on line 1511
.PHP Notice:  is invalid XML, likely due to invalid characters. XML error: Invalid document end at line 5, column 11 in /home/alexandre/GitHub/simplepie/src/SimplePie.php on line 1511
.PHP Notice:  is invalid XML, likely due to invalid characters. XML error: Invalid document end at line 5, column 11 in /home/alexandre/GitHub/simplepie/src/SimplePie.php on line 1511
.PHP Notice:  is invalid XML, likely due to invalid characters. XML error: Invalid document end at line 5, column 11 in /home/alexandre/GitHub/simplepie/src/SimplePie.php on line 1511
.................................................... 1769 / 1892 ( 93%)
............................................................. 1830 / 1892 ( 96%)
............................................................. 1891 / 1892 ( 99%)
.                                                             1892 / 1892 (100%)

Time: 00:05.429, Memory: 22.00 MB

There were 13 errors:

1) SimplePie\Tests\Unit\LocatorTest::testAutodiscoverOnFeed with data set #0 ('application/rss+xml')
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\FileMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/LocatorTest.php:86

2) SimplePie\Tests\Unit\LocatorTest::testAutodiscoverOnFeed with data set #1 ('application/rdf+xml')
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\FileMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/LocatorTest.php:86

3) SimplePie\Tests\Unit\LocatorTest::testAutodiscoverOnFeed with data set #2 ('text/rdf')
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\FileMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/LocatorTest.php:86

4) SimplePie\Tests\Unit\LocatorTest::testAutodiscoverOnFeed with data set #3 ('application/atom+xml')
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\FileMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/LocatorTest.php:86

5) SimplePie\Tests\Unit\LocatorTest::testAutodiscoverOnFeed with data set #4 ('text/xml')
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\FileMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/LocatorTest.php:86

6) SimplePie\Tests\Unit\LocatorTest::testAutodiscoverOnFeed with data set #5 ('application/xml')
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\FileMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/LocatorTest.php:86

7) SimplePie\Tests\Unit\LocatorTest::testInvalidMIMEType
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\FileMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/LocatorTest.php:101

8) SimplePie\Tests\Unit\LocatorTest::testDirectNoDOM
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\FileMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/LocatorTest.php:116

9) SimplePie\Tests\Unit\LocatorTest::testFailDiscoveryNoDOM
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\FileMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/LocatorTest.php:131

10) SimplePie\Tests\Unit\LocatorTest::test_from_file with data set #0 (SimplePie\File Object (...))
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\FileMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/LocatorTest.php:186

11) SimplePie\Tests\Unit\MiscTest::test_convert_UTF8_mbstring with data set #0 ('��', '∞', 'EUC-KR')
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\MiscWithPublicStaticMethodsMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/MiscTest.php:131

12) SimplePie\Tests\Unit\MiscTest::test_convert_UTF8_iconv with data set #0 (Binary String: 0xfeff221e, '∞', 'UTF-16')
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\MiscWithPublicStaticMethodsMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/MiscTest.php:157

13) SimplePie\Tests\Unit\MiscTest::test_convert_UTF8_uconverter with data set #0 (Binary String: 0xfeff221e, '∞', 'UTF-16')
include(/home/alexandre/GitHub/simplepie/library/SimplePie\Tests\Fixtures\MiscWithPublicStaticMethodsMock.php): Failed to open stream: No such file or directory

/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/autoloader.php:126
/home/alexandre/GitHub/simplepie/tests/Unit/MiscTest.php:183

--

There were 3 failures:

1) SimplePie\Tests\Unit\SimplePieTest::testLegacyCallOfSetCacheClass
Failed asserting that exception of type "PHPUnit\Framework\Error\Error" is thrown.

2) SimplePie\Tests\Unit\SimplePieTest::testDirectOverrideNew
Failed asserting that exception of type "SimplePie\Tests\Fixtures\Exception\SuccessException" is thrown.

3) SimplePie\Tests\Unit\SimplePieTest::testDirectOverrideLegacy
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'https://example.com/feed/2019-10-07'
+'http://example.com/feed/'

/home/alexandre/GitHub/simplepie/tests/Unit/SimplePieTest.php:281

ERRORS!
Tests: 1892, Assertions: 2652, Errors: 13, Failures: 3.
Script phpunit handling the test event returned with error code 2

Alkarex · 2022-08-31T09:14:47Z

I tried again and this time it worked (though with some warnings). Maybe some unreliable auto-loading?

$ composer test
> phpunit
PHPUnit 9.5.16 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

.............................................................   61 / 1892 (  3%)
.............................................................  122 / 1892 (  6%)
.............................................................  183 / 1892 (  9%)
.............................................................  244 / 1892 ( 12%)
.............................................................  305 / 1892 ( 16%)
.............................................................  366 / 1892 ( 19%)
.............................................................  427 / 1892 ( 22%)
.............................................................  488 / 1892 ( 25%)
.............................................................  549 / 1892 ( 29%)
.............................................................  610 / 1892 ( 32%)
.............................................................  671 / 1892 ( 35%)
.............................................................  732 / 1892 ( 38%)
.............................................................  793 / 1892 ( 41%)
.............................................................  854 / 1892 ( 45%)
.............................................................  915 / 1892 ( 48%)
.............................................................  976 / 1892 ( 51%)
............................................................. 1037 / 1892 ( 54%)
............................................................. 1098 / 1892 ( 58%)
............................................................. 1159 / 1892 ( 61%)
............................................................. 1220 / 1892 ( 64%)
............................................................. 1281 / 1892 ( 67%)
............................................................. 1342 / 1892 ( 70%)
............................................................. 1403 / 1892 ( 74%)
............................................................. 1464 / 1892 ( 77%)
............................................................. 1525 / 1892 ( 80%)
............................................................. 1586 / 1892 ( 83%)
............................................................. 1647 / 1892 ( 87%)
............................................................. 1708 / 1892 ( 90%)
.....PHP Notice:  is invalid XML, likely due to invalid characters. XML error: Invalid document end at line 3, column 8 in /home/alexandre/GitHub/simplepie/src/SimplePie.php on line 1511
.PHP Notice:  is invalid XML, likely due to invalid characters. XML error: Invalid document end at line 3, column 8 in /home/alexandre/GitHub/simplepie/src/SimplePie.php on line 1511
.PHP Notice:  is invalid XML, likely due to invalid characters. XML error: Invalid document end at line 5, column 11 in /home/alexandre/GitHub/simplepie/src/SimplePie.php on line 1511
.PHP Notice:  is invalid XML, likely due to invalid characters. XML error: Invalid document end at line 5, column 11 in /home/alexandre/GitHub/simplepie/src/SimplePie.php on line 1511
.PHP Notice:  is invalid XML, likely due to invalid characters. XML error: Invalid document end at line 5, column 11 in /home/alexandre/GitHub/simplepie/src/SimplePie.php on line 1511
.................................................... 1769 / 1892 ( 93%)
............................................................. 1830 / 1892 ( 96%)
............................................................. 1891 / 1892 ( 99%)
.                                                             1892 / 1892 (100%)

Time: 00:00.335, Memory: 22.00 MB

OK (1892 tests, 2670 assertions)

jtojnar · 2022-08-31T09:57:33Z

src/Item.php

     */
    public function get_base($element = [])
    {
+        if (!empty($element['xml_base_explicit']) && isset($element['xml_base'])) {


The xml_base_explicit is a boolean so it might be slightly easier to understand, if more verbose:

Suggested change

if (!empty($element['xml_base_explicit']) && isset($element['xml_base'])) {

if (isset($element['xml_base_explicit']) && $element['xml_base_explicit'] && isset($element['xml_base'])) {

Same code as

simplepie/src/SimplePie.php

Lines 2071 to 2080 in cce6ad2

public function get_base($element = [])

{

if (!empty($element['xml_base_explicit']) && isset($element['xml_base'])) {

return $element['xml_base'];

} elseif ($this->get_link() !== null) {

return $this->get_link();

}

return $this->subscribe_url();

}

I am not found of the proposition, because that is exactly what a standard empty() is for, and I personally find it easier to read, but I do not mind much 🤓

jtojnar · 2022-08-31T10:00:51Z

src/Item.php

+        if (!empty($element['xml_base_explicit']) && isset($element['xml_base'])) {
+            return $element['xml_base'];
+        }
+        $link = $this->get_permalink();


I wonder (have not checked) if some pathological feed can create an infinite loop (get_permalink → get_link → get_links → get_base).

I do not believe it can lead to a loop

jtojnar · 2022-08-31T10:07:41Z

src/Item.php

+        if ($link != null) {
+            return $link;
+        }
        return $this->feed->get_base($element);


This method looks to me like it already checks the xml:base attribute on $element, so this PR makes the order as follows:

xml:base

Item’s permalink

xml:base

First rel="alternate" link in the feed

Feed URL

Indeed, but I did not want to change the feeds own method, as it is called from several other places

Alkarex · 2022-08-31T10:07:59Z

Would it be possible to create a unit test for it? This would help to document and enforce the behavior in the future.

Here you are 55ccddd

Maybe can help with strange CI bugs

pull-request-size bot added the size/S label Aug 30, 2022

Alkarex mentioned this pull request Aug 30, 2022

SimplePie fix base FreshRSS/FreshRSS#4565

Merged

tests

55ccddd

pull-request-size bot added size/M and removed size/S labels Aug 31, 2022

jtojnar reviewed Aug 31, 2022

View reviewed changes

Same indenting as other tests

560fbac

Maybe can help with strange CI bugs

Art4 approved these changes Sep 1, 2022

View reviewed changes

mblaney merged commit 33f86e6 into simplepie:master Sep 22, 2022

Art4 mentioned this pull request Sep 22, 2022

Release 1.7.0 #733

Merged

Alkarex deleted the fix-base-url branch September 22, 2022 08:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix item get_base#744

Fix item get_base#744
mblaney merged 3 commits intosimplepie:masterfrom
FreshRSS:fix-base-url

Alkarex commented Aug 30, 2022

Uh oh!

Alkarex commented Aug 30, 2022

Uh oh!

Alkarex commented Aug 30, 2022

Uh oh!

Art4 commented Aug 31, 2022

Uh oh!

Alkarex commented Aug 31, 2022

Uh oh!

Alkarex commented Aug 31, 2022

Uh oh!

jtojnar Aug 31, 2022

Uh oh!

Alkarex Aug 31, 2022

Uh oh!

jtojnar Aug 31, 2022

Uh oh!

Alkarex Aug 31, 2022

Uh oh!

jtojnar Aug 31, 2022

Uh oh!

Alkarex Aug 31, 2022

Uh oh!

Alkarex commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if (!empty($element['xml_base_explicit']) && isset($element['xml_base'])) {
	if (isset($element['xml_base_explicit']) && $element['xml_base_explicit'] && isset($element['xml_base'])) {

	public function get_base($element = [])
	{
	if (!empty($element['xml_base_explicit']) && isset($element['xml_base'])) {
	return $element['xml_base'];
	} elseif ($this->get_link() !== null) {
	return $this->get_link();
	}

	return $this->subscribe_url();
	}

Conversation

Alkarex commented Aug 30, 2022

Uh oh!

Alkarex commented Aug 30, 2022

Uh oh!

Alkarex commented Aug 30, 2022

Uh oh!

Art4 commented Aug 31, 2022

Uh oh!

Alkarex commented Aug 31, 2022

Uh oh!

Alkarex commented Aug 31, 2022

Uh oh!

jtojnar Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

Alkarex Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

jtojnar Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

Alkarex Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

jtojnar Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

Alkarex Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

Alkarex commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants