Icingaweb2: Icingaweb2 can't handle properly escaped URIs

[mhutter]

Expected Behavior

Icingaweb2 recognizes %7C in filter queries as the pipe character.

Current Behavior

Invalid filter "((service=service1&host=host1)%7C(service=service2&host=host2))", unexpected ( at pos 34

#0 /usr/share/php/Icinga/Data/Filter/FilterQueryString.php(215): Icinga\Data\Filter\FilterQueryString->parseError('(')
#1 /usr/share/php/Icinga/Data/Filter/FilterQueryString.php(176): Icinga\Data\Filter\FilterQueryString->readFilters(1, NULL)
#2 /usr/share/php/Icinga/Data/Filter/FilterQueryString.php(260): Icinga\Data\Filter\FilterQueryString->readFilters()
#3 /usr/share/php/Icinga/Data/Filter/FilterQueryString.php(40): Icinga\Data\Filter\FilterQueryString->parseQueryString('((service=servi...')
#4 /usr/share/php/Icinga/Data/Filter/Filter.php(253): Icinga\Data\Filter\FilterQueryString::parse('((service=servi...')
#5 /usr/share/icingaweb2/modules/monitoring/application/controllers/ServicesController.php(35): Icinga\Data\Filter\Filter::fromQueryString('((service=servi...')
...

Possible Solution

un-escape query string using urldecode() before parsing

Steps to Reproduce (for bugs)

1. Select 2 services
2. copy URL into $chat (eg. https://monitoring.example.net/dashboard#!/monitoring/services/show?((service=service1&host=host1)|(service=service2&host=host2)) )
3. open URL from $chat

Context

Most chat client do escaping on URLs as they should. This leads to occurences of | to be replaced with %7C.

We often send each other links to multiple checks in chat. Clicking those links leads to an icingaweb2 stacktrace.

also see #1803

Your Environment

• Icinga Web 2 version 2.5.0
• Google Chrome, Safari, Firefox

[mhutter]

I'm on it, sending the PR once I could test it

[Al2Klimov]

Hello @mhutter and thank you for reporting and providing a solution with unit tests!

We'll have a look at it ASAP.

Best,
AK

[lippserd]

Hi,

Thanks for the report and the PR.

@nilmerg Already closed and commented the PR but I'd like to share my thoughts on this.

First, there is no doubt that according to RFC 3986 the pipe character should be encoded. We use other characters unencoded that don't conform with the RFC as well. But we have our reasons:

1. Roughly, there is no other state than the Icinga Web 2 URL for defining the view.
2. We want to have more than one URL in a URL. I refer to our column mechanics here.
3. We want to have complex filters in our URLs.
4. We want that it is easy to read and write URLs. I guess this is most important point.
5. We don't want to fiddle with double encoding. That would solve the issue but it's a mess. We just can't guarantee that a URL is correctly double encoded and decompose it accordingly.
6. All browsers and HTTP clients I can think of don't automatically encode the characters we use unencoded, e.g. |, <, and >.
7. Without double encoding, it is necessary for us to have them unencoded because we use them as control characters.

I don't know which chat tool you use. I know and use some which don't parse my content and modify it. Is there an option in your chat tool to paste content raw?

So, to sum it up, we won't introduce double encoding which would solve the issue because we would introduce too much complexity and live with the fact that some tools try to be smart and modify our URLs.

Best regards,
Eric

[mhutter]

Hey @lippserd, thanks for the feedback.

However I still think this is an issue. Maybe my fix was flawed, but it's still broken:

The problem occurs as soon as you want to send the link to someone. I have yet to find a communications tool which does NOT encode the URIs, and rightly so: most tools have Web frontends at which point they _must_ encode URLs to avoid HTML injection.

Example for tools where sharing Icinga links does not work:

• Slack
• RocketChat
• Mattermost
• Thunderbird
• Apple Mail
• SoGO (completely breaks, so rather a SoGO problem)
• OpsGenie
• GitHub (see top of this Ticket)
• Telegram
• Whatsapp
• Jira
• Confluence
• Gitlab

[Al2Klimov]

No, no web app has to URL-encode any URL I write to somebody else because:

1. To prevent HTML injection it's enough to do e.g. htmlspecialchars().
2. What if I want an actually broken URL for e.g. explaining a (IT) trainee of mine how URLs shall not look like?
3. It's violating the freedom of speech.

If your apps behave like GitHub, you could still copy-and-paste the text (not the link).

[Thomas-Gelf]

@mhutter: I second that @Al2Klimov and @lippserd sait. I gave Mattermost a quick try, URLs like the following work fine:

https://monitoring.example.com/icingaweb2/monitoring/list/hosts?host=(mysql1.lxd|web1.lxd)

However, this one breaks:

https://monitoring.example.com/icingaweb2/monitoring/list/hosts?host=(mysql1.lxd|web1.lxd)#!/icingaweb2/monitoring/hosts/show?((host=mysql1.lxd)|(host=web1.lxd))

So, Mattermost stops on ?(, but ?a=( is fine. Multi-selecting two columns is the easiest way to reproduce this. First I thought Mattermost would stops interpreting the link after the question mark in the URL fragment while RFC3986 explicitly states:

The characters slash ("/") and question mark ("?") are allowed to represent data within the fragment identifier.

But this is not the case, The problem seems to be simpler. To me it seems that Mattermost is clearly wrong here. Blind guess: it fails in the included commonmark.js, like in this place:

https://github.com/hmhealey/commonmark.js/blob/master/lib/inlines.js#L784

Using a regular expression is IMO not the right tool to deal with URLs, but that's OT. All weird links I tried worked fine in Thunderbird, could you please show what is failing there for you? Also, examples showing where those other tools are struggling would be helpful. Eventually some of the are also based on CommonMark.js, so fixing that library could fix more of the above.

The bug (in MatterMost) seems to trigger only when a special character follows the quotation mark. This link fails:

https://monitoring.example.com/icingaweb2/monitoring/list/hosts?host=(mysql1.lxd|web1.lxd)#!/icingaweb2/monitoring/hosts/show?((host=mysql1.lxd)|(host=web1.lxd))

But this one is fine:

https://monitoring.example.comt/icingaweb2/monitoring/list/hosts?host=(mysql1.lxd|web1.lxd)#!/icingaweb2/monitoring/hosts/show?host=*&((host=mysql1.lxd)|(host=web1.lxd))

I'd prefer to see those bugs fixed at their origin, but of course the more we understand what problems different tools are facing, the better we can understand what kind of workarounds would eventually help. But as @lippserd already pointed out, all kind of black magic and making assumptions when decoding input are dangerous and can potentially lead to security issues. So, being strict should be the preferred way to go. Hope this helps.

Cheers,
Thomas

[lippserd]

This regex looks solid 😆

[mhutter]

I did some more digging... there seems to be a lot more stuff broken than I initially thought. For example, when opening links in Chrome from another app, the | always gets replaced... but no Idea if this broken in Chrome, the OS or somewhere else 😂

[Al2Klimov]

For the record: It never gets replaced.

[Sec42]

I just stumbled across this problem. On MacOS if I click on an URL anywhere (outside of a browser) that contains a |, that gets escaped into %7C, and then icingaweb barfs and fails to deal with it.

This means that any way of sharing icingaweb URLS (chat, email, etc.) does not work.

The behavior of escaping the pipe is correct according to RFC 3986, so I would expect icingaweb to handle this properly.