Qv2ray/QvPlugin-Trojan

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

dyhkwong opened this issue · 9 comments

% 加两位十六进制数 被认为是已进行 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

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

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");
         //

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.

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.

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.

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 We should also launch a check task on the whole project. Maybe there's other positions where we can use QUrl::DecodedMode.

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

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