All: Resolves #13254: Always fully encode URL for the Nextcloud and WebDAV #13727
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
recheck |
|
@laurent22, your bot seems blind... |
|
I have read the CLA Document and I hereby sign the CLA |
|
@Wohlstand Did you commit using --no-verify? The default commit hooks run a linter and it looks like the bot has posted comments about linting issues on the PR which need fixing. On another note, do you think this change would be compatible if added for the WebDAV sync target as well? That would be helpful so users can use whatever sub directory they like for a WebDAV server, not just for NextCloud |
I think ye, it would be best to apply such change just everywhere, where are HTTP queries formed from user's input data that supposed to be properly encoded.
Nope, I commited just with my local defaults. |
|
By the way, I attempted to run this locally, but it just fails to install dependencies via |
Huh! Just now I paid attention on the code of commit, and I see. Interesting thing. Let me try to fix these warnings... |
So you didn't test this change then? |
I wanted to test it, but local build is faulty (I said that EDIT: I see, it doesn't. just builds, verifies and cleans, heck. |
Be aware that Joplin uses |
|
Okay now, I managed to read this manual, then I built the thing, tested with |
Any chance of adding for SyncTargetWebDAV as well? To avoid duplication, I'd suggest adding a method to BaseSyncTarget.ts and then reference that for both WebDAV and NextCloud |
This would be an off-topic for THIS PR. But okay, I will need to adjust everything here then (redo the thing, and rename the PR). I'll try tomorrow as now I should go sleep, it's too late time at me now (UTC+3 time zone). |
|
Okay, I made some check, and it seems to fix both WebDAW and NextCloud it's fully enough to apply fix on WebDAV side since NextCloud just explicitly forwards init to WebDAV, and the same call is performed. |
|
I just moved the fix to WebDAV side, and this thing fixes both Nextcloud and plain WebDAV. It's should be enough. |
| // the syncPath might contain non-ASCII characters | ||
| // /[^\u0021-\u00ff]/ is used in Node.js to detect the unescaped characters. | ||
| // See https://github.com/nodejs/node/blob/bbbf97b6dae63697371082475dc8651a6a220336/lib/_http_client.js#L176 | ||
| const charsReg = /[^\u0021-\u00ff]/; | ||
| const pathUrl = charsReg.exec(options.path()) !== null ? encodeURI(options.path()) : options.path(); | ||
| const apiOptions = { | ||
| baseUrl: () => options.path(), | ||
| baseUrl: () => pathUrl, |
There was a problem hiding this comment.
I'm worried that may break backward compatibility in unexpected ways or lead to double-encoding.
I think a safer way would be something like:
- Decode the URL
- Check if the decoded URL is different from actual URL
- If it's not - don't do anything. Otherwise encode it.
This would at least prevent double-encoding.
I think the error href ${href} not in baseUrl ${baseUrl} nor relativeBaseUrl ${relativeBaseUrl} might also be triggered depending on the WebDAV implementation but at least that shouldn't relate to backward compatibility
There was a problem hiding this comment.
Let me check across the code WHO actually uses WebDAV's initializer to make the full sure...
There was a problem hiding this comment.
Anyway, the double-encoding is impossible, since charsReg check is a condition that is only valid when input path contains inapropriate characters. If string is already encoded, so it will never be valid for this condition, and it will never double-encoding happen.
There was a problem hiding this comment.
For your sure, I checked via grep:
And WebDAV is only used by Nextcloud, so, I am fully sure it will never been double-encoding in this case.
There was a problem hiding this comment.
Other APIs like OneDrive perform encoding on their own and they don't use WebDAV at all.
There was a problem hiding this comment.
Actually! It's might be possible that URL can be PARTIALLY encoded, and therefore it should be fully encoded. Feels like it's more effective to write a custom encoder function that will skip parts that are already encoded (valid %XX bytes).
There was a problem hiding this comment.
Okay, I made the custom function that should SAFELY encode and don't encode already encoded characters twice!
function finishEncodeURI(path)
{
let out = "";
let begin = 0;
let i;
if(path == undefined || path === null || path.length === 0)
return path;
// Collecting ranges that should be encoded
for(i = 0; i < path.length; )
{
// If valid percent encoding is detected, don't encode it again!
if(path[i] === '%' && i + 3 < path.length && /[a-fA-F0-9]{2}/.test(path.substring(i + 1, i + 3)))
{
if(begin !== i)
out += encodeURI(path.substring(begin, i));
out += path.substring(i, i + 3);
begin = i + 3;
i += 3;
}
else
++i;
}
// If it's a piece at end of string that can be encoded too
if(begin < i)
out += encodeURI(path.substring(begin, i));
return out;
}And my test case that confirms the code works:
function printTest(orig)
{
let enc = finishEncodeURI(orig);
console.log("Orig: " + orig)
console.log("Enc1: " + encodeURI(orig))
console.log("Enc2: " + enc)
console.log("Dec: " + decodeURI(enc) + "\n")
}
finishEncodeURI(); // damn
finishEncodeURI(""); // damn
printTest("f");
printTest("ж");
printTest("Мыши!");
printTest("#5hj35h53hj5335j53j53j53j53j");
printTest("https://server.uk/Pre%20Encoded%20Path%20and%20%D0%9C%D1%8B%D1%88%D0%B8!/Что-то пошло не%20так/");
printTest("ЕРц335р35ро3о35о35о№%НР325н235нг23г25г2г55555");
printTest("ЕРц335р35ро3о35о35о№%НР325н235нг23г25г2г55555жжжжжж");
printTest("Just%20A Freaking Test%20Чтобы проверить, что всё щик%20У%20а не%20одни пр%ППоценты!вввffff%20");So, it encodes parts that needs to be encoded, but it does NOT encodes parts that are already encoded! And it does this with a sniper accuracy!
There was a problem hiding this comment.
And the output:
Orig - Input string as-is
Enc1 - Encoded by plain encodeURI()
Enc2 - Encoded by my custom function
Dec - Decoded result of my custom function
Orig: f
Enc1: f
Enc2: f
Dec: f
Orig: ж
Enc1: %D0%B6
Enc2: %D0%B6
Dec: ж
Orig: Мыши!
Enc1: %D0%9C%D1%8B%D1%88%D0%B8!
Enc2: %D0%9C%D1%8B%D1%88%D0%B8!
Dec: Мыши!
Orig: #5hj35h53hj5335j53j53j53j53j
Enc1: #5hj35h53hj5335j53j53j53j53j
Enc2: #5hj35h53hj5335j53j53j53j53j
Dec: #5hj35h53hj5335j53j53j53j53j
Orig: https://server.uk/Pre%20Encoded%20Path%20and%20%D0%9C%D1%8B%D1%88%D0%B8!/Что-то пошло не%20так/
Enc1: https://server.uk/Pre%2520Encoded%2520Path%2520and%2520%25D0%259C%25D1%258B%25D1%2588%25D0%25B8!/%D0%A7%D1%82%D0%BE-%D1%82%D0%BE%20%D0%BF%D0%BE%D1%88%D0%BB%D0%BE%20%D0%BD%D0%B5%2520%D1%82%D0%B0%D0%BA/
Enc2: https://server.uk/Pre%20Encoded%20Path%20and%20%D0%9C%D1%8B%D1%88%D0%B8!/%D0%A7%D1%82%D0%BE-%D1%82%D0%BE%20%D0%BF%D0%BE%D1%88%D0%BB%D0%BE%20%D0%BD%D0%B5%20%D1%82%D0%B0%D0%BA/
Dec: https://server.uk/Pre Encoded Path and Мыши!/Что-то пошло не так/
Orig: ЕРц335р35ро3о35о35о№%НР325н235нг23г25г2г55555
Enc1: %D0%95%D0%A0%D1%86335%D1%8035%D1%80%D0%BE3%D0%BE35%D0%BE35%D0%BE%E2%84%96%25%D0%9D%D0%A0325%D0%BD235%D0%BD%D0%B323%D0%B325%D0%B32%D0%B355555
Enc2: %D0%95%D0%A0%D1%86335%D1%8035%D1%80%D0%BE3%D0%BE35%D0%BE35%D0%BE%E2%84%96%25%D0%9D%D0%A0325%D0%BD235%D0%BD%D0%B323%D0%B325%D0%B32%D0%B355555
Dec: ЕРц335р35ро3о35о35о№%НР325н235нг23г25г2г55555
Orig: ЕРц335р35ро3о35о35о№%НР325н235нг23г25г2г55555жжжжжж
Enc1: %D0%95%D0%A0%D1%86335%D1%8035%D1%80%D0%BE3%D0%BE35%D0%BE35%D0%BE%E2%84%96%25%D0%9D%D0%A0325%D0%BD235%D0%BD%D0%B323%D0%B325%D0%B32%D0%B355555%D0%B6%D0%B6%D0%B6%D0%B6%D0%B6%D0%B6
Enc2: %D0%95%D0%A0%D1%86335%D1%8035%D1%80%D0%BE3%D0%BE35%D0%BE35%D0%BE%E2%84%96%25%D0%9D%D0%A0325%D0%BD235%D0%BD%D0%B323%D0%B325%D0%B32%D0%B355555%D0%B6%D0%B6%D0%B6%D0%B6%D0%B6%D0%B6
Dec: ЕРц335р35ро3о35о35о№%НР325н235нг23г25г2г55555жжжжжж
Orig: Just%20A Freaking Test%20Чтобы проверить, что всё щик%20У%20а не%20одни пр%ППоценты!вввffff%20
Enc1: Just%2520A%20Freaking%20Test%2520%D0%A7%D1%82%D0%BE%D0%B1%D1%8B%20%D0%BF%D1%80%D0%BE%D0%B2%D0%B5%D1%80%D0%B8%D1%82%D1%8C,%20%D1%87%D1%82%D0%BE%20%D0%B2%D1%81%D1%91%20%D1%89%D0%B8%D0%BA%2520%D0%A3%2520%D0%B0%20%D0%BD%D0%B5%2520%D0%BE%D0%B4%D0%BD%D0%B8%20%D0%BF%D1%80%25%D0%9F%D0%9F%D0%BE%D1%86%D0%B5%D0%BD%D1%82%D1%8B!%D0%B2%D0%B2%D0%B2ffff%2520
Enc2: Just%20A%20Freaking%20Test%20%D0%A7%D1%82%D0%BE%D0%B1%D1%8B%20%D0%BF%D1%80%D0%BE%D0%B2%D0%B5%D1%80%D0%B8%D1%82%D1%8C,%20%D1%87%D1%82%D0%BE%20%D0%B2%D1%81%D1%91%20%D1%89%D0%B8%D0%BA%20%D0%A3%20%D0%B0%20%D0%BD%D0%B5%20%D0%BE%D0%B4%D0%BD%D0%B8%20%D0%BF%D1%80%25%D0%9F%D0%9F%D0%BE%D1%86%D0%B5%D0%BD%D1%82%D1%8B!%D0%B2%D0%B2%D0%B2ffff%2520
Dec: Just A Freaking Test Чтобы проверить, что всё щик У а не одни пр%ППоценты!вввffff%20
There was a problem hiding this comment.
The question, @laurent22 , which file I should place this function? It feels like can be used at other places too since it's a general purpose thing rather than WebDAV specific.
There was a problem hiding this comment.
Ping?
Just a conclusion, that your original idea will fail if URL is encoded partially, not fully (it will lead that unencoded parts will pass and will cause the same failure as I reported). It will work if URL is already encoded fully. However, I developed the alternative solution that scans entire string and encodes only parts are absolutely not encoded bytes (any valid percent-encoded bytes). So, I still have a question: WHICH place is good to store the new-made function since it can be useful at other places too?
This is the similar fix to laurent22#7868 done for OneDrive, but it's fully suitable for the Nextcloud too, allowing Unicode paths used normally.
|
Okay, I did the next:
I had to temporarily rename ".husky" directory, otherwise it prevents me to commit because of missing "corepack". I am unable to install "corepack" since it's incompatible to my system-wide installed nodejs 18 (it requires too high version). |
|
... and I fixed all the code style warnings, so it should be fine now. Is everything okay now, @laurent22 ? |
@mrjo118, Lately replying with a new info: The hook started to work since some unexpected moment, BUT, it always fails because it fails to find |
This is the similar fix to #7868 done for OneDrive, but it's fully suitable for the Nextcloud too, allowing Unicode paths used normally. URL field at the UI expected to support unicode paths, and they should be URL-encoded and Punycoded at the backend during the work. Without making unexpected situations like described in the #13254 and other similar.
I have read the CLA Document and I hereby sign the CLA
recheck