Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport #yogosha18281 #30915

Open
wants to merge 3 commits into
base: 18.0
Choose a base branch
from

Conversation

MaximilienR-easya
Copy link
Contributor

Backport yogosha [4b214b4]

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

Copy link
Contributor

@rycks rycks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// We need to urlencode the parameter after the dol_escape_js($tmpurl) because $tmpurl may contain n url with param file=abc%27def if file has a ' inside.
// and when we click on href with this javascript string, a urlcode is done by browser, converted the %27 of file param
return 'javascript:document_preview(\''.urlencode(dol_escape_js($tmpurl)).'\', \''.urlencode(dol_mimetype($relativepath)).'\', \''.urlencode(dol_escape_js($title)).'\')';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

image

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

image

without urlencode on mimetype and title everything seems good:

image

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

@eldy eldy added the PR waiting more user feedbacks We are waiting feedback of someone or more testers to validate this PR label Sep 13, 2024

// We need to urlencode the parameter after the dol_escape_js($tmpurl) because $tmpurl may contain n url with param file=abc%27def if file has a ' inside.
// and when we click on href with this javascript string, a urlcode is done by browser, converted the %27 of file param
return 'javascript:document_preview(\''.urlencode(dol_escape_js($tmpurl)).'\', \''.urlencode(dol_mimetype($relativepath)).'\', \''.urlencode(dol_escape_js($title)).'\')';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rycks
It works fine with urlencode.

@MaximilienR-easya
Copy link
Contributor Author

New commit to backport #29888

@MaximilienR-easya
Copy link
Contributor Author

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

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

or by droping the urlencode :
without_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
Copy link
Member

eldy commented Sep 13, 2024

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 ?

@eldy eldy added Bug or PR need more information This bug or PR needs more information (answer to a question or more accurate description) and removed PR waiting more user feedbacks We are waiting feedback of someone or more testers to validate this PR labels Sep 16, 2024
@MaximilienR-easya
Copy link
Contributor Author

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
Copy link
Contributor

rycks commented Sep 18, 2024

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
Copy link
Member

eldy commented Sep 19, 2024

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
Copy link
Member

eldy commented Sep 20, 2024

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
Copy link
Contributor Author

MaximilienR-easya commented Sep 20, 2024

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
    Screenshot from 2024-09-20 14-10-27

  2. go inside the parameter in the traduction
    Traduction

  3. Replace the traduction for the key “Preview” with the string of your choice
    Screenshot from 2024-09-20 14-08-51

  4. Return to your ticket to see the preview
    Screenshot from 2024-09-20 14-11-11

For the third point, you can retrieve the traduction key simply by research
Screenshot from 2024-09-20 14-24-02

@rycks
Copy link
Contributor

rycks commented Sep 26, 2024

@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
Copy link
Member

eldy commented Sep 27, 2024

@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
Copy link
Contributor

hregis commented Sep 28, 2024

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

Capture d’écran 2024-09-28 à 08 44 51

@MaximilienR-easya
Copy link
Contributor Author

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

Capture d’écran 2024-09-28 à 08 44 51

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

how to reproduce

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
Copy link
Contributor

hregis commented Sep 30, 2024

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

@MaximilienR-easya
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Contributor

hregis commented Sep 30, 2024

@MaximilienR-easya yes ok i see, thanks

@MaximilienR-easya
Copy link
Contributor Author

MaximilienR-easya commented Sep 30, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug or PR need more information This bug or PR needs more information (answer to a question or more accurate description)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants