Closed Bug 1398860 Opened 2 years ago Closed 2 years ago

rewrite socks server/auth test

Categories

(WebExtensions :: Request Handling, enhancement, P3)

49 Branch
enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(1 file)

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.
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Priority: -- → P3
Assignee: nobody → mixedpuppy
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(...)`
Attachment #8906686 - Flags: review?(kmaglione+bmo) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b6ac85aab325
add a socks authentication test, r=kmag
https://hg.mozilla.org/mozilla-central/rev/b6ac85aab325
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.