-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: 18.0
Are you sure you want to change the base?
Backport #yogosha18281 #30915
Conversation
There was a problem hiding this 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 :-)
htdocs/core/lib/functions.lib.php
Outdated
|
||
// 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)).'\')'; |
There was a problem hiding this comment.
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:
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:
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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 ?
htdocs/core/lib/functions.lib.php
Outdated
|
||
// 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)).'\')'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
New commit to backport #29888 |
@eldy We can solve this with using rawurlencode which replace the '+' of the urlencode by '%20' which is working with js parameter : Which of the solution do you think is the more appropriate approach ? |
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 |
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) |
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) ? |
@eldy could you please explain to me why ?
because https://www.php.net/rawurlencode and https://www.php.net/urlencode explains the diff:
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. 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). |
@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
Where the title is not encoded at all. |
@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. |
@hregis |
@MaximilienR-easya yes ok i see, thanks |
@eldy You have an aswer to that question here
|
Backport yogosha [4b214b4]
backport that fix a case when preview of ticket attached file on the card contain ' and break the preview link