Skip to content

Conversation

bdbai
Copy link
Contributor

@bdbai bdbai commented Aug 20, 2020

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly

  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])

  • Use Preview tab to see how your pull request will actually look like

  • Searched for similar pull requests

  • Compiled the code with Visual Studio

  • Require translation update

  • Require document update (readme.md, wikipage, etc)

What is the purpose of your pull request?

  • Bug fix
  • Improvement
  • New feature

Description of your pull request and other information

Current implementation of SIP002 URI Scheme parser produces wrong output for a URI with spaces in its fragment section.

Expected Behavior

Given URI ss://YmYtY2ZiOnRlc3Q@192.168.100.1:8888/?plugin=obfs-local%3bobfs%3dhttp%3bobfs-host%3dgoogle.com#example-server+1, a Server with remark example-server 1 is created.

Current Behavior

Given URI ss://YmYtY2ZiOnRlc3Q@192.168.100.1:8888/?plugin=obfs-local%3bobfs%3dhttp%3bobfs-host%3dgoogle.com#example-server+1, a Server with remark example-server+1 is created.

Note

Current behavior was also observed in other SIP002 implementations, such as Shadowrocket. This fix may break potential compatibility with these clients.

@ghost ghost requested review from celeron533, chenshaoju and Stzx August 20, 2020 10:22
Copy link
Collaborator

@chenshaoju chenshaoju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 Looks good to me, but someone else must approve.

@celeron533
Copy link
Contributor

This fix may break potential compatibility with these clients

Thank you @bdbai
It is okay. The remark text should have no impact on server connections. Unless some of the 3rd party subscription updater would check the remarks strictly.

@celeron533 celeron533 merged commit 2acac49 into shadowsocks:master Aug 20, 2020
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.

4 participants