Skip to content
This repository

Fix proposal for issue #214 #228

Closed
wants to merge 2 commits into from

5 participants

Guillaume Castagnino booruguru Ryan McCue Robert Leverington AJ
Guillaume Castagnino

When SimplePie_IRI::absolutize is fed with a broken URL (for example
'http://https://www.couchsurfing.org/'), it returns bool(false)

But SimplePie_Misc::absolutize_url do not check this and
unconditionally call get_uri(), leading to a fatal error:
"PHP Fatal error: Call to a member function get_uri() on a non-object in
/home/www/common/simplepie/library/SimplePie/Misc.php on line 83"

Guillaume Castagnino Fix php fatal error
When SimplePie_IRI::absolutize is fed with a broken URL (for example
'http://https://www.couchsurfing.org/'), it returns bool(false)

But SimplePie_Misc::absolutize_url do not check this and
unconditionally call get_uri(), leading to a fatal error:
"PHP Fatal error:  Call to a member function get_uri() on a non-object in
/home/www/common/simplepie/library/SimplePie/Misc.php on line 83"
7c3b32a
booruguru

I just download the fix and it works. Thanks a lot!

Ryan McCue
Owner

Is it possible to get any unit tests for this please?

Guillaume Castagnino New unit test
This adds a unit test for Misc::absolutize_url: when the relative URL is
broken, the function should return the URL as-is. The underlying
IRI::absolutize returns false in such cases, but Misc::absolutize_url
used to unconditionally call the member function of this non-object,
leading to a PHP fatal error
0160587
Guillaume Castagnino

Here is a unit test proposal. I added this in the oldtests because I saw similar test cases here. But maybe you prefer to add a new test suite for this in the root of the tests directory ? This seems new fashioned tests suites.

Before the fix:
$ phpunit oldtests
PHPUnit 3.6.11 by Sebastian Bergmann.

..........................................................PHP Fatal error: Call to a member function get_uri() on a non-object in /home/casta/repository/simplepie/library/SimplePie/Misc.php on line 83

After my fix proposal:
$ phpunit oldtests
PHPUnit 3.6.11 by Sebastian Bergmann.

............................................................... 63 / 826 ( 7%)
............................................................... 126 / 826 ( 15%)
............................................................... 189 / 826 ( 22%)
............................................................... 252 / 826 ( 30%)
............................................................... 315 / 826 ( 38%)
............................................................... 378 / 826 ( 45%)
............................................................... 441 / 826 ( 53%)
............................................................... 504 / 826 ( 61%)
............................................................... 567 / 826 ( 68%)
............................................................... 630 / 826 ( 76%)
............................................................... 693 / 826 ( 83%)
............................................................... 756 / 826 ( 91%)
............................................................... 819 / 826 ( 99%)
.......

Time: 1 second, Memory: 25.75Mb

OK (826 tests, 826 assertions)

Thanks !

[EDIT]
I did not add nominal test cases for this since they are already all covered by the IRITest test suite.

Robert Leverington

Is it correct to return a relative URL when an absolute one cannot be created? Some instances where this method is used depend on the returned URL being absolute, and to me it is counter-intuitive that a function called "absolutize_url" would silently return a relative one.

I have an alternate commit that returns false when an absolute URL cannot be formed, and also handles these occurrences correctly in the application code. If this is the preferred approach I would be prepared to supply appropriate test cases.

Guillaume Castagnino

First I noticed this bug when simplepie try to sanityze/absolutize URLs contained in an article content. If absolutize_url returns false, this will break Sanityze::replace_url, unless I'm mistaken. So it have to be fixed here, and potentially in other places (you did it).

But today, all calls to Misc::absolutize_url expect to have an url as output. This I why I have chosen the option to return the input when absolutize fails. Moreover, If Misc::absolutize_url returns the same thing than IRI::absolutize, it's completely useless and has to be removed. I think it's a good option to have it try to absolutize and return the input in case of failure.
From a browser point of view too, you have to be the most tolerant and permissive as possible, even if the input do not conforms to the standards (you are a browser that has to give the information, not to be strict validating it and stripping it if it's not valid). It's for me an other good reason to do so.

Robert Leverington

What about having an argument to the SimplePie_Misc::absolutize_url method that causes it to fall back to a relative URL where an absolute one cannot be generated?

I agree that in some instances it might make sense not to be entirely strict, but on the other hand some uses of this method depend on it returning an absolute URL. For example, SimplePie_File::__construct treats its $url parameter (which comes from SimplePie_Misc::absolutize_url in at least two instances) as if it were an absolute URL, and then supplies the host to fsockopen - to supply it with a relative one would cause incorrect behaviour.

AJ
AJ commented October 05, 2012

This fix is not working for me for this feed currently: http://feed.torrentfreak.com/Torrentfreak/ unfortunately, I do not know how to get more information but the error persists even after the patch was applied


ETA: Please ignore the above. It works for me. I had edited the wrong copy of the misc.php file.

Thanks for providing this fix

Ryan McCue rmccue referenced this pull request October 28, 2012
Closed

Bug #213 fix #245

Ryan McCue rmccue closed this pull request from a commit October 30, 2012
Ryan McCue Fix issue #214 by ensuring we handle invalid URIs
Also closes #228, but doesn't fix the base problem where paths starting
with // aren't parsed by SimplePie_IRI.
267e396
Ryan McCue rmccue closed this in 267e396 October 30, 2012
Ryan McCue rmccue referenced this pull request from a commit October 30, 2012
Ryan McCue Fix issue #214 by ensuring we handle invalid URIs
Also closes #228, but doesn't fix the base problem where paths starting
with // aren't parsed by SimplePie_IRI.
7e6ae7c
Ryan Parman skyzyx referenced this pull request from a commit in skyzyx/simplepie October 30, 2012
Ryan McCue Fix issue #214 by ensuring we handle invalid URIs
Also closes #228, but doesn't fix the base problem where paths starting
with // aren't parsed by SimplePie_IRI.
6880c3d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 1 author.

Aug 23, 2012
Guillaume Castagnino Fix php fatal error
When SimplePie_IRI::absolutize is fed with a broken URL (for example
'http://https://www.couchsurfing.org/'), it returns bool(false)

But SimplePie_Misc::absolutize_url do not check this and
unconditionally call get_uri(), leading to a fatal error:
"PHP Fatal error:  Call to a member function get_uri() on a non-object in
/home/www/common/simplepie/library/SimplePie/Misc.php on line 83"
7c3b32a
Aug 25, 2012
Guillaume Castagnino New unit test
This adds a unit test for Misc::absolutize_url: when the relative URL is
broken, the function should return the URL as-is. The underlying
IRI::absolutize returns false in such cases, but Misc::absolutize_url
used to unconditionally call the member function of this non-object,
leading to a PHP fatal error
0160587
This page is out of date. Refresh to see the latest.
9  library/SimplePie/Misc.php
@@ -80,7 +80,14 @@ public static function time_hms($seconds)
80 80
 	public static function absolutize_url($relative, $base)
81 81
 	{
82 82
 		$iri = SimplePie_IRI::absolutize(new SimplePie_IRI($base), $relative);
83  
-		return $iri->get_uri();
  83
+		if ($iri !== false)
  84
+		{
  85
+			return $iri->get_uri();
  86
+		}
  87
+		else
  88
+		{
  89
+			return $relative;
  90
+		}
84 91
 	}
85 92
 
86 93
 	/**
17  tests/oldtests/absolutize/SPtests/bugs/invalid_relative.php
... ...
@@ -0,0 +1,17 @@
  1
+<?php
  2
+
  3
+class SimplePie_Absolutize_Test_Bug_Invalid_Relative extends SimplePie_Absolutize_Test
  4
+{
  5
+	function data()
  6
+	{
  7
+		$this->data['base'] = 'http://a/b/c/d';
  8
+		$this->data['relative'] = 'http://http://a/b';
  9
+	}
  10
+	
  11
+	function expected()
  12
+	{
  13
+		$this->expected = 'http://http://a/b';
  14
+	}
  15
+}
  16
+
  17
+?>
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.