URI Parameter Encoding

[kergomard]

Hi @thibsy and @klees

This is a PR to start a discussion about fixing:

• https://mantis.ilias.de/view.php?id=45678 and
• https://mantis.ilias.de/view.php?id=45677

In a first discussion with @thibsy we came to the conclusion that it was wrong, that URI treated the uri-string passed through the constructor differently then when adding a parameter through URI::withParameter<s>(). I believe now, that I was wrong: These two cases should be treated differently as they are different.

From where I stand the problem is, that we do accept ':' and '@' in query parameters probably for legacy reasons.

I thus suggest to remove the exception for ':' in trunk. As I personally also think the exception for '@', but am not sure we want to tackle this right now, I remove this in a separate commit.

There are four commits here:

1. The first commit implements the original solution also encoding the query part of the uri-string in the constructor (c5ffd65).
2. The second commit roles the changes back and remove ':' from the allowed characters (cdc771e).
3. The third commit adapts ilCtrl to work with encoded ":" (0a860ef).
4. Also removes the '@' from the allowed characters (c2067a1).

Thank you very much and best,
@kergomard

PS: I will fix the failing test and add corresponding additional tests, as soon as we know what way we want to take.

[kergomard]

Sorry, there were unneeded changes and I had to rebase this. I ran into the same issue now, when working on #9516 .

[thibsy]

Hi @kergomard,

I went another way to address this, see https://mantis.ilias.de/view.php?id=45677.

However, I think for 12 we can still use the fix you propose for the data component. If you would discard all other changes I would be willing to shepherd here (since no one is responsible atm).

Thx still for your proposal!

Kind regards,
@thibsy

[thibsy]

Hi @kergomard,

Thx for updating the PR. The pipelines are failing due to ilCtrl targets passed along to data URI's. This should be resolved once you rebase onto latest trunk, since I changed the delimiter there.

A minor addition I would ask for:

• Unit test: please implement some unit test(s) for the removed characters.

Kind regards,
@thibsy

[kergomard]

Hi @thibsy

You were too quick for me as I had to run:
There is a little bit more to it, as there actually is a faulty test in the Bulky-Link (it really is faulty, as there should not be a second authority in the middle of the query) and there are hard-coded URIs in some tests in the LegalDocuments.

I fixed them all and added an additional test.

Best,
@kergomard

[thibsy]

Hi @kergomard

Good catch! LGTM now, I approve the changes to UI as coordinator and the changes to data as shepherd. Maybe @mjansenDatabay wants to check the legal documents.

Kind regards,
@thibsy

[mjansenDatabay]

The "LegalDocuments" changes LGTM, thanks a lot.

[mjansenDatabay]

Argghhh, this breaks the "Support Contact" feature in the ILIAS footer, which seems to be provided as an email address (at least in my local inst. this is set to mailto:mjansen@databay.de). This seems to be a valid input, see \ilSystemSupportContactsGUI::getFooterLink.

InvalidArgumentException thrown with message "ill-formated component "/mailto:mjansen@databay.de" expected "#^(?!//)(?!:)([A-Za-z0-9]|[\-\._~]|[\$,;=!&'\(\)\*\+]|%[0-9A-Fa-f][0-9A-Fa-f]|/)+$#""

Stacktrace:
#21 InvalidArgumentException in /var/www/ilias/trunk/components/ILIAS/Data/src/URI.php:195
#20 ILIAS\Data\URI:checkCorrectFormatOrThrow in /var/www/ilias/trunk/components/ILIAS/Data/src/URI.php:152
#19 ILIAS\Data\URI:digestPath in /var/www/ilias/trunk/components/ILIAS/Data/src/URI.php:115
#18 ILIAS\Data\URI:__construct in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/classes/UI/Footer/class.ilFooterStandardGroupsProvider.php:68
#17 ilFooterStandardGroupsProvider:buildURI in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/classes/UI/Footer/class.ilFooterStandardGroupsProvider.php:103
#16 ilFooterStandardGroupsProvider:getEntries in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/src/Scope/Footer/Collector/FooterMainCollector.php:61
#15 ILIAS\GlobalScreen\Scope\Footer\Collector\FooterMainCollector:collectStructure in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/src/Collector/AbstractBaseCollector.php:44
#14 ILIAS\GlobalScreen\Collector\AbstractBaseCollector:collectOnce in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/src/Scope/Layout/Provider/PagePart/StandardPagePartProvider.php:233
#13 ILIAS\GlobalScreen\Scope\Layout\Provider\PagePart\StandardPagePartProvider:getFooter in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/src/Scope/Layout/Provider/PagePart/DecoratedPagePartProvider.php:122
#12 ILIAS\GlobalScreen\Scope\Layout\Provider\PagePart\DecoratedPagePartProvider:getFooter in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/src/Scope/Layout/Provider/PagePart/DecoratedPagePartProvider.php:122
#11 ILIAS\GlobalScreen\Scope\Layout\Provider\PagePart\DecoratedPagePartProvider:getFooter in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/src/Scope/Layout/Provider/PagePart/DecoratedPagePartProvider.php:122
#10 ILIAS\GlobalScreen\Scope\Layout\Provider\PagePart\DecoratedPagePartProvider:getFooter in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/src/Scope/Layout/Provider/PagePart/DecoratedPagePartProvider.php:122
#9 ILIAS\GlobalScreen\Scope\Layout\Provider\PagePart\DecoratedPagePartProvider:getFooter in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/src/Scope/Layout/Builder/StandardPageBuilder.php:60
#8 ILIAS\GlobalScreen\Scope\Layout\Builder\StandardPageBuilder:build in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/src/Scope/Layout/ModificationHandler.php:148
#7 ILIAS\GlobalScreen\Scope\Layout\ModificationHandler:getPageWithPagePartProviders in /var/www/ilias/trunk/components/ILIAS/GlobalScreen/src/Scope/Layout/Collector/MainLayoutCollector.php:242
#6 ILIAS\GlobalScreen\Scope\Layout\Collector\MainLayoutCollector:getFinalPage in /var/www/ilias/trunk/components/ILIAS/UICore/classes/class.ilGlobalPageTemplate.php:136
#5 ilGlobalPageTemplate:printToStdout in /var/www/ilias/trunk/components/ILIAS/Init/classes/class.ilStartUpGUI.php:369
#4 ilStartUpGUI:printToGlobalTemplate in /var/www/ilias/trunk/components/ILIAS/Init/classes/class.ilStartUpGUI.php:358
#3 ilStartUpGUI:showLoginPage in /var/www/ilias/trunk/components/ILIAS/Init/classes/class.ilStartUpGUI.php:185
#2 ilStartUpGUI:executeCommand in /var/www/ilias/trunk/components/ILIAS/UICore/classes/class.ilCtrl.php:119
#1 ilCtrl:forwardCommand in /var/www/ilias/trunk/components/ILIAS/UICore/classes/class.ilCtrl.php:92
#0 ilCtrl:callBaseClass in /var/www/ilias/trunk/public/ilias.php:33

[thibsy]

@mjansenDatabay oh, good catch. I assume the Data\URI::PATH constant is wrong then? If I remember correctly Databay has taken over some authorities. Can I assume you are working on this?

[mjansenDatabay]

@mjansenDatabay oh, good catch. I assume the Data\URI::PATH constant is wrong then? If I remember correctly Databay has taken over some authorities. Can I assume you are working on this?

Yes, @lscharmer will provide a PR for this.

[lscharmer]

Ok this PR is actually fine, the issue is that it was already broken ^^. See PR #10748 this will fix the issue: #10043 (comment) when picked to trunk.