Online config URL parsing #21
Conversation
src/shadowsocks_config.ts
Outdated
| // The built-in URL parser throws as desired when given URIs with invalid syntax. | ||
| const urlParserResult = new URL(inputForUrlParser); | ||
| // Use ValidatedConfigFields subclasses (Host, Port, Tag) to throw on validation failure. | ||
| const uriFormattedHost = urlParserResult.hostname; |
There was a problem hiding this comment.
If we do a regex replace like ...hostname.replace(/\[(.*)\]/, '$1') here then we don't need the try-catch
There was a problem hiding this comment.
unfortunately that doesn't quite work because some implementations fail without the brackets. I don't remember for which browsers/webviews we added the try catch in SIP002, but I thought it would make sense to replicate it here.
src/shadowsocks_config.ts
Outdated
| // The built-in URL parser throws as desired when given URIs with invalid syntax. | ||
| const urlParserResult = new URL(inputForUrlParser); | ||
| // Use ValidatedConfigFields subclasses (Host, Port, Tag) to throw on validation failure. | ||
| const uriFormattedHost = urlParserResult.hostname; |
There was a problem hiding this comment.
unfortunately that doesn't quite work because some implementations fail without the brackets. I don't remember for which browsers/webviews we added the try catch in SIP002, but I thought it would make sense to replicate it here.
src/shadowsocks_config.ts
Outdated
| } | ||
|
|
||
| // Ref: https://github.com/shadowsocks/shadowsocks-org/issues/89 | ||
| export const SIP008_URI = { |
There was a problem hiding this comment.
I don't think we should call this SIP008_URI. The SIP0008 is a bad standard. It's focused on the config format, not the URL format. They are conflating many things there. And we are kind of making it our own format.
Why do you need this object? Can't you just expose the method? I'd rename parse as parseOnlineConfigUrl to get ConfigFetchParams.
There was a problem hiding this comment.
The SIP0008 is a bad standard. It's focused on the config format, not the URL format.
SIP008 is focused on the config format because all existing solutions widely used by providers in China are just some random JSON encoded in base64 and then served by a typical web server. SIP008 was intended to address the situation by providing a standard JSON document format that is flexible and not encoded in any unnecessary means like base64.
As for the URL format, most people I know who set up their own servers for such purposes already own at least one domain name. And it's pretty easy to obtain a TLS certificate with Let's Encrypt. But I do agree utilizing self-signed certificates can be better in some situations, and that's why I mentioned this PR when discussing with V2Fly developers on V2Ray's upcoming subscription protocol that's based on SIP008ext with a custom URL format.
And we are kind of making it our own format.
I'm open to amendments to SIP008. What Outline plans to implement can be useful to the community as well. We are working towards the same goal after all.
There was a problem hiding this comment.
I'm sorry, I think I misspoke there. The standard meets a different need than the one in Outline, so it's bad for our use case.
- It doesn't address the need for the certificate fingerprint
- It doesn't address the need to handle redirects in a way that allows migration of the config location
- It doesn't address the need for a special URI protocol for better user experience
- The file format is overly complicated with many things we don't actually support. It includes thinks that are not config, like "bytes used". It has flaws like not knowing what can be safely ignored. Configuring services need more careful consideration, but we'd like to punt on that for now.
I've argued for all of those items in the SIP discussions. I'd like to implement what works for Outline, and then go back with it as a proof of concept.
src/shadowsocks_config.ts
Outdated
| } | ||
|
|
||
| // Ref: https://github.com/shadowsocks/shadowsocks-org/issues/89 | ||
| export const SIP008_URI = { |
src/shadowsocks_config.ts
Outdated
| } | ||
|
|
||
| // Ref: https://github.com/shadowsocks/shadowsocks-org/issues/89 | ||
| export const SIP008_URI = { |
There was a problem hiding this comment.
I'm sorry, I think I misspoke there. The standard meets a different need than the one in Outline, so it's bad for our use case.
- It doesn't address the need for the certificate fingerprint
- It doesn't address the need to handle redirects in a way that allows migration of the config location
- It doesn't address the need for a special URI protocol for better user experience
- The file format is overly complicated with many things we don't actually support. It includes thinks that are not config, like "bytes used". It has flaws like not knowing what can be safely ignored. Configuring services need more careful consideration, but we'd like to punt on that for now.
I've argued for all of those items in the SIP discussions. I'd like to implement what works for Outline, and then go back with it as a proof of concept.
src/shadowsocks_config.ts
Outdated
| // The default URL parser fails to recognize the default HTTPs port (443). | ||
| const port = new Port(urlParserResult.port || '443'); | ||
| // Parse extra parameters from the tag, which has the URL search parameters format. | ||
| const tag = new Tag(decodeURIComponent(urlParserResult.hash.substring(1))); |
There was a problem hiding this comment.
You are double decoding, which will break with the value has an encoded &, for example: 'https://dsfdsf.com#foo=a%26b'
Remove the decodeURIComponent call since URLSearchParams already decodes for you.
Also add a test for that case.
There was a problem hiding this comment.
Good catch. Done in fb81d34.
ssconfURL protocol to support encoding parameters.