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 improperly parsed remark section #2935

Merged
merged 1 commit into from
Aug 20, 2020
Merged

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://[email protected]: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://[email protected]: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