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

fix(ssurl): use percent_decode_str for userinfo encoded data #985

Merged
merged 2 commits into from Oct 26, 2022

Conversation

RAX7
Copy link
Contributor

@RAX7 RAX7 commented Oct 25, 2022

Some URLs contain a base64 string with = char (e.g: ss://YWVzLTEyOC1nY206c2hhZG93c29ja3M=@1.2.3.4:8000#tag) which breaks the ssurl tool.

@zonyitoo
Copy link
Collaborator

zonyitoo commented Oct 25, 2022

This is impossible. The userinfo has to be encoded with base64 url safe encoding, which is definitely don't have any =. ss://YWVzLTEyOC1nY206c2hhZG93c29ja3M=@1.2.3.4:8000#tag is not a valid SIP002 URL.

You should make a PR to fix where the URL was generated. Follow the spec carefully: https://shadowsocks.org/guide/sip002.html

@RAX7
Copy link
Contributor Author

RAX7 commented Oct 25, 2022

@zonyitoo base64 may contain = for padding

= padding characters might be added to make the last encoded block contain four Base64 characters.
https://en.wikipedia.org/wiki/Base64

@zonyitoo
Copy link
Collaborator

You are messing up "base64" and "base64 url safe" encoding.

  1. Read the "URL Applications" section in your provided wiki: https://en.wikipedia.org/wiki/Base64#URL_applications
  2. RFC4648 base64url: https://datatracker.ietf.org/doc/html/rfc4648#section-5

@database64128
Copy link
Contributor

database64128 commented Oct 25, 2022

Your example is an invalid URL with characters outside the allowed set but not percent encoded. This won't even work with some URL parsers. I don't know how you obtained this "URL", but you should tell whoever generated it to use a proper URL builder, not string concatenation.

Edit: It seems = is actually allowed in userinfo.

2nd edit: Now I'm confused: https://url.spec.whatwg.org/#userinfo-percent-encode-set

@RAX7
Copy link
Contributor Author

RAX7 commented Oct 25, 2022

Oh yes you are right.

You should make a PR to fix where the URL was generated.

I can't do that, I'm just trying to use outline keys and ssurl breaks with error.
May I create another PR witch remove all = chars from userinfo to make this tool more user friendly?

@zonyitoo
Copy link
Collaborator

zonyitoo commented Oct 25, 2022

Well, the code in PR actually trying to decode the userinfo with percent encoding, so the actual example should be: ss://YWVzLTEyOC1nY206c2hhZG93c29ja3M%3D@1.2.3.4:8000#tag

On the other hand, is that what your original URL? @RAX7 , the = is encoded as %3D. If the answer is no, then this PR couldn't solve your problem.

@zonyitoo
Copy link
Collaborator

zonyitoo commented Oct 25, 2022

May I create another PR witch remove all = chars from userinfo to make this tool more user friendly?

No you should not do that. You may also need to check if there is any characters that are not in the valid base64 url safe character set and try to fix it to fulfill the specification requirement.

You will never be able to compatible all kinds of errors, which is why we need a specification.

But on the otherhand, I noticed that some base64 encoder, such as the one in Python ,will still add tailing =s for url safe encoding. So I think this PR is still valid for this situation.

@database64128
Copy link
Contributor

https://github.com/Jigsaw-Code/outline-shadowsocksconfig/blob/master/src/shadowsocks_config.ts#L338

It seems they are indeed using string concatenation. You should also open an issue over there.

@zonyitoo
Copy link
Collaborator

zonyitoo commented Oct 25, 2022

const userInfo = Base64.encodeURI(`${method.data}:${password.data}`);

https://github.com/dankogai/js-base64/blob/68f12dea403e8b0031b09fa5edd0904066fb7ab4/base64.ts#L121-L133

const _mkUriSafe = (src: string) => src
    .replace(/=/g, '').replace(/[+\/]/g, (m0) => m0 == '+' ? '-' : '_');

/**
 * converts a UTF-8-encoded string to a Base64 string.
 * @param {boolean} [urlsafe] if `true` make the result URL-safe
 * @returns {string} Base64 string
 */
const encode = (src: string, urlsafe = false) => urlsafe
    ? _mkUriSafe(_encode(src))
    : _encode(src);
/**
 * converts a UTF-8-encoded string to URL-safe Base64 RFC4648 §5.
 * @returns {string} Base64 string
 */
const encodeURI = (src: string) => encode(src, true);

So the outline generated URL shouldn't contain any =. It is ok to do a pure string concatenation.

@RAX7
Copy link
Contributor Author

RAX7 commented Oct 25, 2022

@zonyitoo the original URL I took here and it's: ss://Y2hhY2hhMjAtaWV0Zi1wb2x5MTMwNTpHIXlCd1BXSDNWYW8=@ak1489.free.www.outline.network:806#www.outline.network%20(canada)

@zonyitoo
Copy link
Collaborator

Interesting, the code in outline-server is Ok but the console is running a different code version.

Please checkout the comments in the review, I am Ok to merge this PR if they are fixed.

@database64128
Copy link
Contributor

/cc @fortuna

@RAX7
Copy link
Contributor Author

RAX7 commented Oct 25, 2022

@zonyitoo, @database64128 I guess they using outline-shadowsocksconfig only for desktop client, but not for website
https://github.com/Jigsaw-Code/outline-client/blob/master/package.json#L54

Please checkout the comments in the review, I am Ok to merge this PR if they are fixed.

Test cases fails on windows with:

Error: There is not enough space on the disk. : 'D:\a\_temp\_runner_file_commands\step_summary_7596271d-8edb-440d-9476-a7e3c69f2199'

@zonyitoo
Copy link
Collaborator

Just ignore the Windows test case.

crates/shadowsocks/src/config.rs Outdated Show resolved Hide resolved
crates/shadowsocks/src/config.rs Outdated Show resolved Hide resolved
@zonyitoo zonyitoo merged commit ab6a6f6 into shadowsocks:master Oct 26, 2022
@RAX7 RAX7 deleted the fix-ssurl branch October 26, 2022 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants