switch base64 encoding to be url safe #13
Conversation
| 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}`); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
added a pr upstream to add more example of when they do and don't add padding dashes. dankogai/js-base64#139
| extendStatics(d, b); | ||
| function __() { this.constructor = d; } | ||
| d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __()); | ||
| }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| tag: 'Foo Bar', | ||
| }); | ||
| expect(LEGACY_BASE64_URI.stringify(config)).toEqual( | ||
| 'ss://YmYtY2ZiOuWwj-a0nuS4jeihpeWkp-a0nuWQg-iLpkAxOTIuMTY4LjEwMC4xOjg4ODg#Foo%20Bar'); |
There was a problem hiding this comment.
This is violating the standard. It should be using base64, not base64url. See Jigsaw-Code/outline-apps#913 (comment)
| b64EncodedData = paddingLength === 0 ? b64EncodedData : | ||
| b64EncodedData.substring(0, dataLength - paddingLength); | ||
| const data = `${method.data}:${password.data}@${host.data}:${port.data}`; | ||
| const b64EncodedData = Base64.encodeURI(data); |
There was a problem hiding this comment.
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.
| 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}`); |
There was a problem hiding this comment.
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.
3 participants
resolves Jigsaw-Code/outline-server#774