1398860 - rewrite socks server/auth test

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, enhancement, P3)

Description

[Shane Caraveo (:mixedpuppy)]

In bug 1381290 I borrowed heavily from the one other socks server test in the code base I could find.  It still needs some heavy modernization.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1398860 add a socks authentication test,

Review commit: https://reviewboard.mozilla.org/r/178412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/178412/

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8906686 [details]
Bug 1398860  add a socks authentication test,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178412/diff/1-2/

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8906686 [details]
Bug 1398860  add a socks authentication test,

https://reviewboard.mozilla.org/r/178412/#review194264

Note: I mostly just sanity checked the SOCKS client/server code. I didn't review the protocol handling in detail.

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_socks.js:140
(Diff revision 2)
> +    let i = this.inbuf.indexOf(0);
> +    let str = null;
> +
> +    if (i >= 0) {
> +      let buf = this.inbuf.slice(0, i);
> +      str = buf2str(buf);

let decoder = new TextDecoder();
    str = decoder.decode(this.inbuf.slice(0, i);

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_socks.js:222
(Diff revision 2)
> +      this.write([5, 0]);
> +    }
> +  }
> +
> +  checkSocks5Auth() {
> +    do_check_eq(this.inbuf[0], 0x01, "subnegotiation version");

Nit: `equal(...)`

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_socks.js:228
(Diff revision 2)
> +    let username = buf2str(unnamebuf);
> +    let password = buf2str(pwordbuf);

`decoder.decode(...)`

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_socks.js:231
(Diff revision 2)
> +    do_check_eq(username, this.server.username, "socks auth username");
> +    do_check_eq(password, this.server.password, "socks auth password");

`equal(...)`

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_socks.js:273
(Diff revision 2)
> +      if (this.inbuf.length < 4 + len + 1 + 2) {
> +        return;
> +      }
> +
> +      let buf = this.inbuf.slice(5, 5 + len);
> +      this.dest_name = buf2str(buf);

`decoder.decode(...)`

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_socks.js:304
(Diff revision 2)
> +    this.inbuf = [];
> +    this.write(buf);
> +  }
> +
> +  checkRequest() {
> +    let request = buf2str(this.inbuf);

`decoder.decode(...)`

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_socks.js:360
(Diff revision 2)
> +    const ProtocolProxyService = CC("@mozilla.org/network/protocol-proxy-service;1",
> +                                    "nsIProtocolProxyService");

You should never call `createInstance` on a service. This only works because this service happens to cache its singleton instance.

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_socks.js:397
(Diff revision 2)
> +      this.reject(e);
> +      return;
> +    }
> +    let bin = new BinaryInputStream(stream);
> +    let data = bin.readByteArray(len);
> +    let result = buf2str(data);

`decoder.decode(...)`

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8906686 [details]
Bug 1398860  add a socks authentication test,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178412/diff/2-3/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6ac85aab325
add a socks authentication test, r=kmag

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b6ac85aab325

Comment 7

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.