Skip to content
This repository has been archived by the owner on Aug 18, 2021. It is now read-only.

[Bug] serialize and deserialize trojan:// link incorrectly #18

Closed
dyhkwong opened this issue Jan 7, 2021 · 9 comments
Closed

[Bug] serialize and deserialize trojan:// link incorrectly #18

dyhkwong opened this issue Jan 7, 2021 · 9 comments

Comments

@dyhkwong
Copy link
Contributor

dyhkwong commented Jan 7, 2021

% 加两位十六进制数 被认为是已进行 url-encode 的字符
Example: 新建一个密码为 %25 的连接,导出的链接是 trojan://%25@example.com:443#foobar 而不是 trojan://%2525@example.com:443#foobar

: 后的字符被认为是 url password 的一部分
Example: 导入链接 trojan://1:2@example.com:443#foobar,导入后密码变为 1 而不是 1:2

@ghost
Copy link

ghost commented Jan 7, 2021

We may need QUrl::userInfo() https://doc.qt.io/qt-5/qurl.html#userInfo instead of QUrl::password

@DuckSoft
Copy link
Member

DuckSoft commented Jan 8, 2021

diff --git a/core/Serializer.hpp b/core/Serializer.hpp
index cca3979..024bfae 100644
--- a/core/Serializer.hpp
+++ b/core/Serializer.hpp
@@ -69,7 +69,7 @@ class TrojanOutboundHandler : public Qv2rayPlugin::PluginOutboundHandler
         //
         TrojanObject result;
         result.address = trojanUrl.host();
-        result.password = trojanUrl.userName();
+        result.password = trojanUrl.userInfo(QUrl::FullyDecoded);
         result.port = trojanUrl.port();
         result.sni = getQueryValue("sni");
         //

@DuckSoft
Copy link
Member

DuckSoft commented Jan 8, 2021

That's a great mess. This URI specification seemed to be brought by Shadowrocket, but they never explicitly share the details and is close-sourced.

Well in that case, I just want to say that, you got to escape your special characters in passwords. I think Qv2ray will be doing correct things.

@DuckSoft
Copy link
Member

DuckSoft commented Jan 8, 2021

Patch is submitted in 86b5763 on dev.
The deserialization problem should be resolved.

As for the serialization, I guess it's not a problem if we explicitly escape :. If not, it would be a tragedy for the people who've designed this ******* schema.

@DuckSoft DuckSoft closed this as completed Jan 8, 2021
@dyhkwong
Copy link
Contributor Author

dyhkwong commented Jan 8, 2021

How about the issue "新建一个密码为 %25 的连接,导出的链接是 trojan://%25@example.com:443#foobar 而不是 trojan://%2525@example.com:443#foobar"?
Maybe it can be fixed by

            QUrl link;
            if (!o.password.isEmpty())
-                 link.setUserInfo(o.password);
+                 link.setUserInfo(o.password, QUrl::DecodedMode);

ps: QUrl::DecodedMode is not permitted in setUserInfo, use setUserName instead.

@DuckSoft
Copy link
Member

DuckSoft commented Jan 8, 2021

How about the issue "新建一个密码为 %25 的连接,导出的链接是 trojan://%25@example.com:443#foobar 而不是 trojan://%2525@example.com:443#foobar"?
Maybe it can be fixed by

            QUrl link;
            if (!o.password.isEmpty())
-                 link.setUserInfo(o.password);
+                 link.setUserInfo(o.password, QUrl::DecodedMode);

Yes. Just do it.
Don't forget to bump the buildversion.

@DuckSoft
Copy link
Member

DuckSoft commented Jan 8, 2021

@DuckSoft We should also launch a check task on the whole project. Maybe there's other positions where we can use QUrl::DecodedMode.

@dyhkwong
Copy link
Contributor Author

dyhkwong commented Jan 8, 2021

@DuckSoft Is your patch tested? It doesn't work because QUrl::FullyDecoded is not permitted in QUrl.userInfo(). If you must do so, QUrl.userInfo() will return an empty value.
Check 4f5d2ac instead.

@DuckSoft
Copy link
Member

DuckSoft commented Jan 8, 2021

@dyhkwong got it. I tested against setFragment that day, so I thought it would work the same.
Big Surprise:

QUrl::setUserInfo(): QUrl::DecodedMode is not permitted in this function
QUrl::userInfo(): QUrl::FullyDecoded is not permitted in this function

QUrl is awesome.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

2 participants