Authentication does not work after #259

[goekce]

After #259 authentication using the HTML5 client does not work anymore.

If I use xpra attach wss://[email protected] authentication is fine but using HTML5 client fails. I could not find a way to print the password string which is sent to PAM.

@oskarbraten did you test the PR? If yes: I don't have experience with Javascript however I can try to print the encoded string before and after the PR.

[totaam]

I could not find a way to print the password string which is sent to PAM

This is generally considered to be a good thing.
You can manually enable it by editing the following code:
https://github.com/Xpra-org/xpra/blob/9d699dea1293fd4ba00ae722c21f79b996b3c399/xpra/server/auth/sys_auth_base.py#L204-L206

@goekce: what version are you running?
It's likely that the clipboard code can switch to this new code, but other packets cannot - at least not yet.

[goekce]

#259 replaces String.fromCharCode with TextDecoder().decode(). The former expects a UTF-16 sequence and the latter UTF-8 as default. I set a breakpoint inside Uint8ToString and extracted a sequence which differs for these two functions:

temp1 = new Uint8Array([186, 38, 43, 41, 33, 251, 143, 178]);
console.log(String.fromCharCode.apply(null, temp1));
console.log(new TextDecoder().decode(temp1));

output:

º&+)!û�²
�&+)!���

If we try to decode using UTF-16 to match their output:

temp1 = new Uint8Array([186, 38, 43, 41, 33, 251, 143, 178]);
console.log(String.fromCharCode.apply(null, temp1));
console.log(new TextDecoder('utf-16').decode(temp1));

output:

º&+)!û�²
⚺⤫ﬡ늏

Still different :( . We see four characters in the latter case because UTF-16 requires two bytes for a single char. But why does the first not work similarly? The reason is that we map the fromCharCode to every item in the Uint8Array individually, so fromCharCode regards every byte as a UTF-16 encoded data. So if we want to create the same behavior, we have to pad extra zeroes for TextEncoder:

temp1 = new Uint8Array([186, 38, 43, 41, 33, 251, 143, 178]);
console.log(String.fromCharCode.apply(null, temp1));
console.log(new TextDecoder('utf-16').decode(new Uint16Array(temp1)));

output:

º&+)!û�²
º&+)!û�²

Above would fix the problem I believe but why did the code encode uint8 arrays using UTF-16 (fromCharCode)?

---

I am running v4.4.5, but I believe this function is also used for encoding the password. I will try to print the password tomorrow.

[goekce]

It's likely that the clipboard code can switch to this new code, but other packets cannot - at least not yet.

Oh, you mean actually I should not use the latest HTML5 with 4.4.5? I probably thought that the latest xpra has also problems due to Xpra-org/xpra#3939 , however this issue is not related. I will test the latest HTML5 against the latest xpra regarding this issue and let you know.

[totaam]

Oh, you mean actually I should not use the latest HTML5 with 4.4.5?

No, I probably just misunderstood this bug.
All recent xpra-html5 versions should be compatible with all recent versions of xpra.
We never ever want to sent utf-16 over the network.
We did switch to rencodeplus as default encoder recently, which should allow clients and server to send strings without first encoding to bytes. The packet encoder should deal with this properly - perhaps it does not?

xpra-html5/html5/js/lib/rencode.js

Line 46 in 6f6295c

function utf8ByteArrayToString(bytes) {

xpra-html5/html5/js/lib/rencode.js

Line 72 in 6f6295c

function stringToUtf8ByteArray(str) {

FYI: the rencode_legacy_mode is going to be removed soon since xpra servers will no longer be using it.

[goekce]

I activated password logging. #259 sends corrupted data to the v4.4.6 server.

I tried to understand when rencode and Utilities.Uint8toString are used. At first rencode.js is involved but for authentication Utilities.Uint8toString seem to be used:

First call stack involving rencode

stringToUtf8ByteArray (https://server.de/js/lib/rencode.js:formatted#72)
rencode_string (https://server.de/js/lib/rencode.js:formatted#75)
rencode (https://server.de/js/lib/rencode.js:formatted#207)
rencode_list (https://server.de/js/lib/rencode.js:formatted#152)
rencode (https://server.de/js/lib/rencode.js:formatted#198)
rencodeplus (https://server.de/js/lib/rencode.js:formatted#222)
process_send_queue (https://server.de/js/Protocol.js#1)
send (https://server.de/js/Protocol.js#1)
send (https://server.de/js/Protocol.js#1)
 (https://server.de/js/Protocol.js#1)
 (https://server.de/js/Protocol.js#1)

Second call stack involving Utilities

Uint8ToString (https://server.de/js/Utilities.js:formatted#243)
s (https://server.de/js/Utilities.js:formatted#247)
_process_challenge (https://server.de/js/Client.js#2)
_route_packet (https://server.de/js/Client.js#1)
open_protocol (https://server.de/js/Client.js#1)
open (https://server.de/js/Protocol.js#1)
open (https://server.de/js/Protocol.js#1)
open_protocol (https://server.de/js/Client.js#1)
_do_connect (https://server.de/js/Client.js#1)
initialize_workers (https://server.de/js/Client.js#1)
initialize_workers (https://server.de/js/Client.js#1)
connect (https://server.de/js/Client.js#1)
init_page (https://server.de/index.html#1520)
onload (https://server.de/index.html#1654)
load_default_settings (https://server.de/index.html#1644)
 (https://server.de/index.html#1678)
mightThrow (https://server.de/js/lib/jquery.js#1)
process (https://server.de/js/lib/jquery.js#1)
resolve (https://server.de/js/lib/jquery.js#1)
fire (https://server.de/js/lib/jquery.js#1)
fireWith (https://server.de/js/lib/jquery.js#1)
fire (https://server.de/js/lib/jquery.js#1)
fire (https://server.de/js/lib/jquery.js#1)
fireWith (https://server.de/js/lib/jquery.js#1)
ready (https://server.de/js/lib/jquery.js#1)
completed (https://server.de/js/lib/jquery.js#1)
 (https://server.de/js/lib/jquery.js#1)
 (https://server.de/js/lib/jquery.js#1)
 (https://server.de/js/lib/jquery.js#1)

You mean that Utilities.Uint8toString will not be additionally required and rencodeplus should be used solely?

What I don't understand:

• Why is Uint8toString executed first and not StringtoUint8. First the password needs to be encoded, right?
• fromCharCode used to decode single bytes using UTF-16 which seem to be worked until now. How?

I can send a PR which fixes the Textencoder using UTF-8 which would be a quick fix.

[totaam]

Your backtrace was from a minimized version of the source, so it's difficult to know which exact line triggered which call.
(you may want to just copy the xpra-html5/html5/* into /usr/share/xpra/www/)

IIRC, _process_challenge:

xpra-html5/html5/js/Client.js

Line 2956 in 6f6295c

_process_challenge(packet) {

Calls Utilities.s to ensure that the value is a string, even when receiving data using a legacy packet encoder that only sends bytes. It may be redundant / unhelpful nowadays with rencodeplus.
Perhaps the correct fix is to remove these hacks.

[goekce]

Then v4.4.6 must be using the legacy packet encoder. So the idea is to ensure that Utilities.s never receives a Uint8Array but only strings. Then we don't have to decode it using Utilities.

This issue is related to websockets. Due to Xpra-org/xpra#3939 I cannot establish a websocket connection to the latest server so I cannot fix this without fixing Xpra-org/xpra#3939. I'll try Xpra-org/xpra#3939 first.

[oskarbraten]

@goekce I did not perform any tests with authentication enabled.
I’ll do some debugging. It seems quite odd that the clipboard works as intended, while the challenge fails. Perhaps there are other unintended side-effects as well.
@totaam Yes, removing the hacks is the way to go long term. I’ll see what I can do.

[totaam]

@goekce Xpra-org/xpra#3939 is closed as fixed so I don't see how it could be blocking anything.

[goekce]

@goekce Xpra-org/xpra#3939 is closed as fixed so I don't see how it could be blocking anything.

this is related to ArchLinux which is unsupported, so I closed the issue before fixing.

---

I’ll see what I can do.

Great @oskarbraten !

[totaam]

so I closed the issue before fixing.

No. AFAIK, the issue was already fixed, you're just running an older version.