Backport #yogosha18281

[MaximilienR-easya]

Backport yogosha [4b214b4]

backport that fix a case when preview of ticket attached file on the card contain ' and break the preview link

[rycks]

ok for me (security backport), thanks for 18 LTS :-)

[rycks]

@lvessiller-opendsi show that's a strange thing with urlencode on mimetype and title

here is the result of my test in javascript console with manual data set with urlencode:

the result is a popup with no pdf displayed and a title with raw url encoded string :

without urlencode on mimetype and title everything seems good:

[rycks]

here is the code you can type into debug js console

javascript:document_preview('/document.php?modulepart=propal&attachment=0&file=PR2409-0059%2FPR2409-0059.pdf&&entity=1', 'application/pdf', 'test éric / fichier js'); 

and

javascript:document_preview('/document.php?modulepart=propal&attachment=0&file=PR2409-0059%2FPR2409-0059.pdf&&entity=1', 'application%2fpdf', 'test+%C3%A9ric+%2F+fichier+js'); 

@eldy original fix seems to be concerned by the same error ...

[eldy]

I don't understand how to reproduce your point @rycks.
You just upload a file with name "test éric / fichier js.pdf" on a proposal ?

[lvessiller-opendsi]

I think we shouldn't use urlencode for parameters "dol_mimetype" and $tiltle.
I think it would also be fix in develop.

[lvessiller-opendsi]

@rycks
It works fine with urlencode.

[MaximilienR-easya]

New commit to backport #29888

[MaximilienR-easya]

@eldy
I have case when using urlencode on title with space character, exemple :

We can solve this with using rawurlencode which replace the '+' of the urlencode by '%20' which is working with js parameter :

or by droping the urlencode :

Which of the solution do you think is the more appropriate approach ?
When I have your answer, I'm going to make a 2nd PR for develop to also change the urlencode on this parameter

[eldy]

I have case when using urlencode on title with space character, exemple

This is on developer side. But how, as a user, do you reproduce the bug ?

[MaximilienR-easya]

I have case when using urlencode on title with space character, exemple

This is on developer side. But how, as a user, do you reproduce the bug ?

It was user side, I just changed the translation for preview

[rycks]

ok, thanks to @MaximilienR-easya rawurlencode works better than urlencode for that sort of situations then a fix with rawurlencode is better than urlencode for js context

@eldy it could be a good idea to make a global review of all javascript calls where urlencode is used ? >> after a speed review there is no other part of dolibarr where there is javascript call with urlencode

[eldy]

ok, thanks to @MaximilienR-easya rawurlencode works better than urlencode for that sort of situations then a fix with rawurlencode is better than urlencode for js context

@eldy it could be a good idea to make a global review of all javascript calls where urlencode is used ? >> after a speed review there is no other part of dolibarr where there is javascript call with urlencode

Using rawurlencode should not be necessary for part of query string (urlencode should work)
rawurlencoee should be necessary for url path only, but in the use case of dolibarr we have url path into DOL_URL, so insee no reason to have rawurlencode in dolibarr.
When we want to escape an url, we should use urlencode, when we need to escape javascript, we should use dol_escape_js.
This is why i would like to know how to reproduce the bug because a fix using rawurlencode is suspicious.

[eldy]

I have case when using urlencode on title with space character, exemple

This is on developer side. But how, as a user, do you reproduce the bug ?

It was user side, I just changed the translation for preview

Sorry, the process to reproduce the bug is still not clear. Can you describe what you do to generate the bug (in a user point of view, only end user action) ?

[MaximilienR-easya]

I have case when using urlencode on title with space character, exemple

This is on developer side. But how, as a user, do you reproduce the bug ?

It was user side, I just changed the translation for preview

Sorry, the process to reproduce the bug is still not clear. Can you describe what you do to generate the bug (in a user point of view, only end user action) ?

To reproduce it with a user point of view, only end user action :

1. go inside the ticket to see the preview title default traduction
   

2. go inside the parameter in the traduction
   

3. Replace the traduction for the key “Preview” with the string of your choice
   

4. Return to your ticket to see the preview
   

For the third point, you can retrieve the traduction key simply by research

[rycks]

@eldy could you please explain to me why ?

a fix using rawurlencode is suspicious

because https://www.php.net/rawurlencode and https://www.php.net/urlencode explains the diff:

urlencode Returns a string in which all non-alphanumeric characters except -_. have been replaced with a percent (%) sign followed by two hex digits and spaces encoded as plus (+) signs. It is encoded the same way that the posted data from a WWW form is encoded, that is the same way as in application/x-www-form-urlencoded media type. This differs from the » RFC 3986 encoding (see rawurlencode()) in that for historical reasons, spaces are encoded as plus (+) signs.

then rawurlencode is better than urlencode in such situation where a js interpreter could be present (like in web browser on javascript://url) because rawurlencode implements RFC 3986 ... then i really can't understand why using rawurlencode is suspicious ?

[eldy]

@eldy could you please explain to me why ?

a fix using rawurlencode is suspicious

because https://www.php.net/rawurlencode and https://www.php.net/urlencode explains the diff:

urlencode Returns a string in which all non-alphanumeric characters except -_. have been replaced with a percent (%) sign followed by two hex digits and spaces encoded as plus (+) signs. It is encoded the same way that the posted data from a WWW form is encoded, that is the same way as in application/x-www-form-urlencoded media type. This differs from the » RFC 3986 encoding (see rawurlencode()) in that for historical reasons, spaces are encoded as plus (+) signs.

then rawurlencode is better than urlencode in such situation where a js interpreter could be present (like in web browser on javascript://url) because rawurlencode implements RFC 3986 ... then i really can't understand why using rawurlencode is suspicious ?

rawurlencode was designed to encode url part before the parameters, like the web domain name. In Dolibarr we should never have the part before the DOL_URL_ROOT appearing in html or js because we use relative path. So no need for rawurlencode. The other case is the rest of url berweend domain name and parameters, but for such part, the urlencode is recommanded. Also for filename part into an url that is under control of user, Dolibarr should have sanitized the filename before writing it on disk. So if we need rawurlencode, it probably mean we miss something else somewhere else.

But as long as there is no description on how to reproduce the bug i can't provide more information. So I insist another time, if there is a bug, providing a description on how to reproduce it is as important as providing a solution.

For the moment ticket is still not clear.
Is the bug appears before or after the suggested PR ?

So my final question remain the same: How can we reproduce it ? (It is necessary to know that to validate the change, and, if necessary, to implement the phpunit to avoid to have this bug back again).

[hregis]

@MaximilienR-easya @rycks i don't have your problem ?

[MaximilienR-easya]

@MaximilienR-easya @rycks i don't have your problem ?

@hregis I think you used the wrong link to open the preview:

The problem is different with the preview next to the file, it's not the same code. for this one the code is in htdocs/core/js/lib_foot.js.php

if ($conf->browser->layout != 'phone') {
	print "\n/* JS CODE TO ENABLE document_preview */\n"; // Function document_preview is into header
	print '		jQuery(document).ready(function () {
			        jQuery(".documentpreview").click(function () {
            		    console.log("We click on preview for element with href="+$(this).attr(\'href\')+" mime="+$(this).attr(\'mime\'));
            		    document_preview($(this).attr(\'href\'), $(this).attr(\'mime\'), \''.dol_escape_js($langs->transnoentities("Preview")).'\');
                		return false;
        			});
		});'."\n";
}

Where the title is not encoded at all.

[hregis]

@MaximilienR-easya I have the same result with this link, I don't have your error, with the current v18 branch.

[MaximilienR-easya]

@MaximilienR-easya I have the same result with this link, I don't have your error, with the current v18 branch.

The current v18 don't have the backport yogosha with urlencode at all. This is why you can't reproduce it with the current v18.

The discussion is about a modification added by the backport.
If we backport the same as in develop, we should use urlencode (which cause the '+' character to appear).
That is why I suggest modifying urlencode to rawurlencode inside the backport and once it is validated, apply the modification apported to the backport to the next version.

[MaximilienR-easya]

@MaximilienR-easya I have the same result with this link, I don't have your error, with the current v18 branch.

@hregis
If you want to test it with the backport with urlencode and not rawurlencode, this is this commit 1ee3c5e

[hregis]

@MaximilienR-easya yes ok i see, thanks

[MaximilienR-easya]

For the moment ticket is still not clear.
Is the bug appears before or after the suggested PR ?

@eldy You have an aswer to that question here

@MaximilienR-easya I have the same result with this link, I don't have your error, with the current v18 branch.

The current v18 don't have the backport yogosha with urlencode at all. This is why you can't reproduce it with the current v18.

The discussion is about a modification added by the backport. If we backport the same as in develop, we should use urlencode (which cause the '+' character to appear). That is why I suggest modifying urlencode to rawurlencode inside the backport and once it is validated, apply the modification apported to the backport to the next version.

@MaximilienR-easya I have the same result with this link, I don't have your error, with the current v18 branch.

@hregis If you want to test it with the backport with urlencode and not rawurlencode, this is this commit 1ee3c5e