[2.2.x] Symphony breaks on certain URLs

[michael-e]

Using a URL like http://example.com/&/, Symphony breaks with an XSLT error like the following:

Line 23
loadXML(): error parsing attribute name in Entity, line: 23
Line 23
loadXML(): attributes construct error in Entity, line: 23
Line 23
loadXML(): Couldn't find end of Start Tag url- line 23 in Entity, line: 23

This behaviour must have been introduced between 2.1.2.1 (no problem) and 2.2.2 (problem confirmed). It is alive in the current 2.2.x branch.

[michael-e]

I found out that it also breaks on question marks, like so: http://example.com/?/

[michael-e]

I vaguely remember that this has been discussed berfore. It has to do with the url-param getting added to the XML. Symphony will create invalid nodes like <url-/ />...

[nilshoerrmann]

Might it be related that & is a reserved character in XSLT (and thus should be escaped as &)?

[michael-e]

As far as I see, when retrieving the url-param, & and ? will be removed by Symphony. But this procedure is error-prone if no valid characters follow, resulting in invalid nodes like <url-/ />.

[brendo]

Have made 'bad' params not be added to the XML which prevents this error from occurring. As #, & and ? are 'special' to HTML (either a fragment or query string identifier) I don't think throwing a 404 is necessary.

[michael-e]

Cool, thanks!

[kalvind]

(Symphony 2.2.5, Windows XP, XAMPP 1.7.4)

http://localhost/domain/?%C9 breaks and results in XSLT error. Once again the <url-/ /> appears:

XSLT Processing Error

This page could not be rendered due to the following XSLT processing errors.

XML

Line 23
    loadXML(): StartTag: invalid element name in Entity, line: 23

[rgb-]

Don't know if it's related but :

http://localhost/?: also breaks with a slightly different error.

XSLTProcessor::transformToXml(): runtime error
XSLTProcessor::transformToXml(): user param : no namespace bound to prefix url-

If this is done on http://localhost/workspace/bootstrap/img/?:

I get a full backtrace with Symphony Fatal Error and all informations coming with it like database information.

[brendo]

I can confirm on both accounts, ?%C9 and ?: both break the page.

This will be fixed for 2.3.

Out of curiosity, how are you getting these URL's?

[rgb-]

It's just some random testing.

By the way, all special encoded characters break symphony.

  À              %C0
  Á              %C1
  Â              %C2
  Ã              %C3
  Ä              %C4
  Å              %C5
  Æ              %C6
  Ç              %C7
  È              %C8
  É              %C9
  Ê              %CA
  Ë              %CB
  Ì              %CC
  Í              %CD
  Î              %CE
  Ï              %CF
  Ð              %D0

And many more.

Thanks.

[kanduvisla]

A second approach regarding the :? made me realize that the URL-parameter was set to url-: in this scenario, which tranfers to a parameter in the XML-pool called <url-></url->, which is invalid XML, causing the error. My second commit fixes this.

Also, it fixes the issue where ?%09 would cause a similar error. Only ?a=%C9 now still causes an error. I think this can be fixed by utf8_encode()-ing the $value before it gets sanitized. Should this be something what class.frontendpage.php should handle? Or is this a modification that should be made in the General::sanitize()-function?

[kanduvisla]

I've spent a couple of hours debugging tonight to figure out what was going wrong and how to fix it.

Issue 1: ?: caused an XML-tag to be created like <url->...</url->, which is invalid. This is fixed by checking if a handle not ends up being empty when being sanitized:

Issue 2: When using a URL like ?a=%C9, the content wasn't properly UTF8-encoded for the General::sanitize()-function. Wrapping the logic in utf8_encode(url_decode($value)) fixes this.

Issue 3: This one came along on the ride and was the biggest brain-breaker: When using a URL like ?a=%0C, Symphony would throw another error, since %0C would translate to a lower ASCII-characters which isn't allowed in XML. Turns out, there's a whole range of characters that's not allowed in XML. Therefore, the XSL Processor would throw an error, since the XML was not well-formed. This was when the <params />-tree was built, but also in the setRuntimeParam()-function of class.xsltpage.php. To fix this, I added an extra function in class.xmlelement.php (thanks to this guy). This is used by XMLElement->setValue(), and as a static function inside XSLTPage. The setValue()-function hád to be adjusted to make sure all XML-values are valid from now on. So you might even consider this a new bug that popped up when I was trying to fix the original bug.

Well to make a long story short, my last commit fixes the issues of Symphony crashing on weird URL's, so I hope we can check that one of our list now... ;-)

I'll try to make a clean pull request, but just like my previous pull requests it keeps on pushing 11 previous commits, although there are only 3 files changed :-/

[kanduvisla]

please merge from TwistedInteractive@c2ab6f5 because I can't seem to make a decent pull request.

Each time I try to make a pull request I got a list of commits of several months ago, but the files changed are only 3 files...

Can anybody please explain my why this is? Or point me to an article on how to decently setup a pull request? I'm now spending more time for setting up this pull request that it took me to fix this bug! This take the fun out of coding for me! I just ... don't ... GET IT!!!! Why are there 12 commits when only 3 files are changed?!?!?!?!

[brendo]

It's because your history is different the main integration branch
(they are both called integration, but the histories are wildly
different). Usually it's a good idea to rebase your changes against
integration before you submit a pull request (or pull for the latest
regularly)

Typically I create a new branch to do fixes in and then before merging
with integration I will:

1. Checkout integration and pull for the latest
2. Rebase my fixes branch against integration
3. Merge fixes into integration
4. Submit pull

Here's a Github article on rebasing.
I'm sure @nils-werner has some pro tips too (Git's his bitch ;)).

[kanduvisla]

fixed it!

The problem was indeed that the history of my origin/integration branch wasn't in sync with upstream/integration.