Skip to content

switch base64 encoding to be url safe #13

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 10, 2021
Merged

switch base64 encoding to be url safe #13

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d677252
switch base64 encoding to be url safe
ohnorobo Feb 9, 2021
0bb8086
add tests with non-latin passwords
ohnorobo Feb 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 5 additions & 11 deletions build/shadowsocks_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ var __extends = (this && this.__extends) || (function () {
};
Copy link

Choose a reason for hiding this comment

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

Why do we still generate this file? Can we get rid of this file? We should depend on the ts code instead.
It's very confusing. It can go out of sync and it's not clear what will actually be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to close out this thread- I looked into this and it's probably possible, but requires making a bunch of tricky config changes on both the ssconfig and server/client side (the client imports are especially tricky because of the way we use babel-loader.)

microsoft/TypeScript#12358 useful summary of the issue:

We got everything working for us but it became very obvious from reactions to this issue that it was not really a use case that was being considered and that we would always be fighting to tooling to achieve what we wanted.

So I think we should stick with generating the js/d.ts exports in the standard way.

})();
Object.defineProperty(exports, "__esModule", { value: true });
var punycode = require('punycode');
var js_base64_1 = require('js-base64');
var js_base64_1 = require("js-base64");
var punycode = require("punycode");
// Custom error base class
var ShadowsocksConfigError = /** @class */ (function (_super) {
__extends(ShadowsocksConfigError, _super);
Expand Down Expand Up @@ -284,14 +284,8 @@ exports.LEGACY_BASE64_URI = {
stringify: function (config) {
var host = config.host, port = config.port, method = config.method, password = config.password, tag = config.tag;
var hash = exports.SHADOWSOCKS_URI.getHash(tag);
var b64EncodedData = js_base64_1.Base64.encode(
method.data + ':' + password.data + '@' + host.data + ':' + port.data);
var dataLength = b64EncodedData.length;
var paddingLength = 0;
for (; b64EncodedData[dataLength - 1 - paddingLength] === '='; paddingLength++)
;
b64EncodedData = paddingLength === 0 ? b64EncodedData :
b64EncodedData.substring(0, dataLength - paddingLength);
var data = method.data + ":" + password.data + "@" + host.data + ":" + port.data;
var b64EncodedData = js_base64_1.Base64.encodeURI(data);
return "ss://" + b64EncodedData + hash;
},
};
Expand Down Expand Up @@ -342,7 +336,7 @@ exports.SIP002_URI = {
},
stringify: function (config) {
var host = config.host, port = config.port, method = config.method, password = config.password, tag = config.tag, extra = config.extra;
var userInfo = js_base64_1.Base64.encode(method.data + ':' + password.data);
var userInfo = js_base64_1.Base64.encodeURI(method.data + ":" + password.data);
var uriHost = exports.SHADOWSOCKS_URI.getUriFormattedHost(host);
var hash = exports.SHADOWSOCKS_URI.getHash(tag);
var queryString = '';
Expand Down
26 changes: 18 additions & 8 deletions src/shadowsocks_config.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ describe('shadowsocks_config', () => {
tag: 'Foo Bar',
});
expect(SIP002_URI.stringify(config)).toEqual(
'ss://YWVzLTEyOC1nY206dGVzdA==@192.168.100.1:8888/#Foo%20Bar');
'ss://[email protected]:8888/#Foo%20Bar');
});

it('can serialize a SIP002 URI with IPv6 host', () => {
Expand All @@ -207,7 +207,7 @@ describe('shadowsocks_config', () => {
tag: 'Foo Bar',
});
expect(SIP002_URI.stringify(config)).toEqual(
'ss://YWVzLTEyOC1nY206dGVzdA==@[2001:0:ce49:7601:e866:efff:62c3:fffe]:8888/#Foo%20Bar');
'ss://YWVzLTEyOC1nY206dGVzdA@[2001:0:ce49:7601:e866:efff:62c3:fffe]:8888/#Foo%20Bar');
});

it('can serialize a legacy base64 URI', () => {
Expand All @@ -230,6 +230,16 @@ describe('shadowsocks_config', () => {
});

it('can parse a valid SIP002 URI with IPv4 host', () => {
const input = 'ss://[email protected]:8888#Foo%20Bar';
const config = SHADOWSOCKS_URI.parse(input);
expect(config.method.data).toEqual('aes-128-gcm');
expect(config.password.data).toEqual('test');
expect(config.host.data).toEqual('192.168.100.1');
expect(config.port.data).toEqual(8888);
expect(config.tag.data).toEqual('Foo Bar');
});

it('can parse a SIP002 URI with non-uri-safe base64 padding', () => {
const input = 'ss://[email protected]:8888#Foo%20Bar';
const config = SHADOWSOCKS_URI.parse(input);
expect(config.method.data).toEqual('aes-128-gcm');
Expand All @@ -240,16 +250,16 @@ describe('shadowsocks_config', () => {
});

it('can parse a valid SIP002 URI with IPv6 host', () => {
const input = 'ss://YWVzLTEyOC1nY206dGVzdA==@[2001:0:ce49:7601:e866:efff:62c3:fffe]:8888';
const input = 'ss://YWVzLTEyOC1nY206dGVzdA@[2001:0:ce49:7601:e866:efff:62c3:fffe]:8888';
const config = SHADOWSOCKS_URI.parse(input);
expect(config.method.data).toEqual('aes-128-gcm');
expect(config.password.data).toEqual('test');
expect(config.host.data).toEqual('2001:0:ce49:7601:e866:efff:62c3:fffe');
expect(config.port.data).toEqual(8888);
});

it('can parse a valid SIP002 URI with an arbitray query param', () => {
const input = 'ss://cmM0LW1kNTpwYXNzd2Q=@192.168.100.1:8888/?foo=1';
it('can parse a valid SIP002 URI with an arbitrary query param', () => {
const input = 'ss://[email protected]:8888/?foo=1';
const config = SHADOWSOCKS_URI.parse(input);
expect(config.extra.foo!).toEqual('1');
});
Expand All @@ -262,7 +272,7 @@ describe('shadowsocks_config', () => {
});

it('can parse a valid SIP002 URI with a plugin param', () => {
const input = 'ss://cmM0LW1kNTpwYXNzd2Q=@192.168.100.1:8888/?plugin=obfs-local%3Bobfs%3Dhttp';
const input = 'ss://[email protected]:8888/?plugin=obfs-local%3Bobfs%3Dhttp';
const config = SHADOWSOCKS_URI.parse(input);
expect(config.method.data).toEqual('rc4-md5');
expect(config.password.data).toEqual('passwd');
Expand All @@ -272,7 +282,7 @@ describe('shadowsocks_config', () => {
});

it('can parse a valid SIP002 URI with the default HTTP port and no plugin parameters', () => {
const input = 'ss://cmM0LW1kNTpwYXNzd2Q=@192.168.100.1:80';
const input = 'ss://[email protected]:80';
const config = SHADOWSOCKS_URI.parse(input);
expect(config.method.data).toEqual('rc4-md5');
expect(config.password.data).toEqual('passwd');
Expand All @@ -281,7 +291,7 @@ describe('shadowsocks_config', () => {
});

it('can parse a valid SIP002 URI with the default HTTP port and parameters', () => {
const input = 'ss://cmM0LW1kNTpwYXNzd2Q=@192.168.100.1:80/?foo=1&bar=';
const input = 'ss://[email protected]:80/?foo=1&bar=';
const config = SHADOWSOCKS_URI.parse(input);
expect(config.method.data).toEqual('rc4-md5');
expect(config.password.data).toEqual('passwd');
Expand Down
10 changes: 3 additions & 7 deletions src/shadowsocks_config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,12 +264,8 @@ export const LEGACY_BASE64_URI = {
stringify: (config: Config) => {
const {host, port, method, password, tag} = config;
const hash = SHADOWSOCKS_URI.getHash(tag);
let b64EncodedData = Base64.encode(`${method.data}:${password.data}@${host.data}:${port.data}`);
const dataLength = b64EncodedData.length;
let paddingLength = 0;
for (; b64EncodedData[dataLength - 1 - paddingLength] === '='; paddingLength++);
b64EncodedData = paddingLength === 0 ? b64EncodedData :
b64EncodedData.substring(0, dataLength - paddingLength);
const data = `${method.data}:${password.data}@${host.data}:${port.data}`;
const b64EncodedData = Base64.encodeURI(data);
Copy link

Choose a reason for hiding this comment

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

I believe this should be Base64.encode(data). Notice that we use Base64.decode() above.

We should have a test where we decode the output of encode and make sure they match.

return `ss://${b64EncodedData}${hash}`;
},
};
Expand Down Expand Up @@ -320,7 +316,7 @@ export const SIP002_URI = {

stringify: (config: Config) => {
const {host, port, method, password, tag, extra} = config;
const userInfo = Base64.encode(`${method.data}:${password.data}`);
const userInfo = Base64.encodeURI(`${method.data}:${password.data}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

The js-base64 docs state that encodeURI uses "-" as padding for URIs, but I'm seeing that they actually follow the RFC recommendation to omit it for known length data.

Can we please add a comment to this end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a pr upstream to add more example of when they do and don't add padding dashes. dankogai/js-base64#139

Copy link

Choose a reason for hiding this comment

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

It seems that Base64.decode() in the parse code handles both base64 and base64url, so that's good. But let's add a comment there saying it expects base64url as input.

const uriHost = SHADOWSOCKS_URI.getUriFormattedHost(host);
const hash = SHADOWSOCKS_URI.getHash(tag);
let queryString = '';
Expand Down