bug#45162: 28.0.50; Messages scrambled by socks-filter

bug#45162: 28.0.50; Messages scrambled by socks-filter

[J.P.]

[-- Attachment #1: Type: text/plain, Size: 3958 bytes --]

Severity: minor
Tags: patch

Hi,

If this is indeed a bug, I'm guessing it's a pretty minor one because in
most cases, the bulk of a SOCKS reply doesn't seem to matter, so long as
the status code portion is correctly interpreted. But in rarer cases, for
example when using Tor's RESOLVE command, which is part of a nonstandard
extension suite, a response may need to be preserved intact.

Rather than try to explain, I've included a throwaway before/after test
showing current and expected behavior, as well as a small patch that
seems to fix the issue. If this is all nonsense, please forgive me. I am
no expert, just a grateful end user.

Thanks,
J.P.

In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.23, cairo version 1.16.0)
 of 2020-12-09 built on pc
Repository revision: 4f352ad6f1759ae6dcff6ba43847273491bf9c30
Repository branch: socks
Windowing system distributor 'Fedora Project', version 11.0.12010000
System Description: Fedora 33 (Workstation Edition)

Configured using:
 'configure --with-dbus --with-gif --with-jpeg --with-png --with-rsvg
 --with-tiff --with-xft --with-xpm --with-x-toolkit=gtk3 --with-gpm=no
 --with-xwidgets --with-modules --with-harfbuzz --with-cairo --with-json
 CC=gcc 'CFLAGS=-DMAIL_USE_LOCKF -O0 -g3 -Wall' LDFLAGS=-Wl,-z,relro
 PKG_CONFIG_PATH=:/usr/lib64/pkgconfig:/usr/share/pkgconfig'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF
ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS XWIDGETS
LIBSYSTEMD JSON PDUMPER LCMS2

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml easymenu mml-sec epa derived epg epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
iso-transl tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads
xwidget-internal dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting cairo move-toolbar gtk x-toolkit
x multi-tty make-network-process emacs)

Memory information:
((conses 16 51710 8248)
 (symbols 48 6739 1)
 (strings 32 18249 1932)
 (string-bytes 1 610727)
 (vectors 16 12311)
 (vector-slots 8 170737 6932)
 (floats 8 21 45)
 (intervals 56 219 0)
 (buffers 984 10))


[-- Attachment #2: 0001-DROP-ME-add-test-explaining-existing-bug.patch --]
[-- Type: text/x-patch, Size: 5345 bytes --]

From da49cec6391f282ff998e5ace022c74805dabcf8 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 10 Dec 2020 03:04:48 -0800
Subject: [PATCH 1/3] DROP ME: add test explaining existing bug

This is file is meant to demonstrate existing behavior. It is not
meant for inclusion in the test suite.
---
 test/lisp/net/socks-tests.el | 85 ++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644 test/lisp/net/socks-tests.el

diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el
new file mode 100644
index 00000000000..601da494a26
--- /dev/null
+++ b/test/lisp/net/socks-tests.el
@@ -0,0 +1,85 @@
+;;; socks-tests.el --- tests for SOCKS -*- coding: utf-8; lexical-binding: t; -*-
+
+;;; Commentary:
+
+;; This file is supposed to describe (what's believed to be) an existing bug.
+;;
+;; | (let ((default-directory (expand-file-name "../..")))
+;; |   (compile "make lisp/net/socks-tests.log"))
+
+(require 'socks)
+
+(ert-deftest socks-filter-test-existing-behavior-v4 ()
+  (let* ((buf (generate-new-buffer "*test-socks-filter*"))
+         (proc (start-process "test-socks-filter" buf "sleep" "1")))
+    (process-put proc 'socks t)
+    (process-put proc 'socks-state socks-state-waiting)
+    (process-put proc 'socks-server-protocol 4)
+    (ert-info ("Receive initial incomplete segment")
+      (should (equal (string-to-vector "\0Z\0\0]¸Ø") [0 90 0 0 93 184 216]))
+      ;; An IPv4 address from example.com     OK status ^      ^ msg start
+      (socks-filter proc "\0Z\0\0]¸Ø")
+      (ert-info ("State still set to waiting")
+        (should (eq (process-get proc 'socks-state) socks-state-waiting)))
+      (ert-info ("Response field is nil because processing incomplete")
+        (should-not (process-get proc 'socks-response)))
+      (ert-info ("Scratch field holds stashed partial payload")
+        (should (string= "\0Z\0\0]¸Ø" (process-get proc 'socks-scratch)))))
+    (ert-info ("Last part arrives")
+      (socks-filter proc "\42") ; ?\" 34
+      (ert-info ("State transitions to complete (proto length check passes)")
+        (should (eq (process-get proc 'socks-state) socks-state-connected)))
+      (ert-info ("Response field holds last scratch with new segment prepended")
+        (should (string= "\"\0Z\0\0]¸Ø" (process-get proc 'socks-response))))
+      (ert-info ("Scratch remains populated, matches response field")
+        (should (string= "\"\0Z\0\0]¸Ø" (process-get proc 'socks-scratch)))))
+    (should (equal (string-to-vector "\"\0Z\0\0]¸Ø") [34 0 90 0 0 93 184 216]))
+    (delete-process proc)
+    (set-process-query-on-exit-flag proc nil)
+    (kill-buffer buf)))
+
+(ert-deftest socks-filter-test-existing-behavior-v5 ()
+  (let* ((buf (generate-new-buffer "*test-socks-filter*"))
+         (proc (start-process "test-socks-filter" buf "sleep" "1")))
+    (process-put proc 'socks t)
+    (process-put proc 'socks-state socks-state-waiting)
+    (process-put proc 'socks-server-protocol 5)
+    (ert-info ("Receive initial incomplete segment")
+      ;; An Ipv6 addr for fedora.org: 2605:bc80:3010:600:dead:beef:cafe:fed9
+      (should (equal (string-to-vector (concat
+                                        "\5\0\0\4"
+                                        "\x26\x05\xbc\x80\x30\x10\x00\x60"
+                                        "\xde\xad\xbe\xef\xca\xfe\xfe\xd9"
+                                        "\0\0"))
+                     (vconcat
+                      [5 0 0 4] ; version status (OK) noop addr-type (4 -> 6!)
+                      [#x26 #x05 #xbc #x80 #x30 #x10 #x00 #x60]
+                      [#xde #xad #xbe #xef #xca #xfe #xfe #xd9]
+                      [0 0])))
+      (socks-filter proc "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60")
+      (ert-info ("State still set to waiting")
+        (should (eq (process-get proc 'socks-state) socks-state-waiting)))
+      (ert-info ("Response field is nil because processing incomplete")
+        (should-not (process-get proc 'socks-response)))
+      (ert-info ("Scratch field holds partial payload of pending msg")
+        (should (string= "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60"
+                         (process-get proc 'socks-scratch)))))
+    (ert-info ("Last part arrives")
+      (ert-info ("Error raised because parsing logic expects appended bytes")
+        (let ((err (should-error (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9"))))
+          (should (equal err '(wrong-type-argument number-or-marker-p nil)))))
+      (ert-info ("State untouched (still waiting)")
+        (should (eq (process-get proc 'socks-state) socks-state-waiting)))
+      (ert-info ("Response field untouched")
+        (should-not (process-get proc 'socks-response)))
+      (ert-info ("Scratch contains new arrival prepended")
+        (should (string=  (concat "\xde\xad\xbe\xef\xca\xfe\xfe\xd9"
+                                  "\5\0\0\4"
+                                  "\x26\x05\xbc\x80\x30\x10\x00\x60")
+                          (process-get proc 'socks-scratch)))))
+    (delete-process proc)
+    (set-process-query-on-exit-flag proc nil)
+    (kill-buffer buf)))
+
+
+;;; socks-tests.el ends here
-- 
2.28.0


[-- Attachment #3: 0002-DROP-ME-update-test-to-reflect-expected-behavior.patch --]
[-- Type: text/x-patch, Size: 4096 bytes --]

From b8467e34275f371e8e6381b929498fac7864f4a7 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 10 Dec 2020 03:11:38 -0800
Subject: [PATCH 2/3] DROP ME: update test to reflect expected behavior

The tests now show SOCKS v4 and v5 messages successfully received.
---
 test/lisp/net/socks-tests.el | 37 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el
index 601da494a26..af310b8e6e9 100644
--- a/test/lisp/net/socks-tests.el
+++ b/test/lisp/net/socks-tests.el
@@ -29,11 +29,11 @@ socks-filter-test-existing-behavior-v4
       (socks-filter proc "\42") ; ?\" 34
       (ert-info ("State transitions to complete (proto length check passes)")
         (should (eq (process-get proc 'socks-state) socks-state-connected)))
-      (ert-info ("Response field holds last scratch with new segment prepended")
-        (should (string= "\"\0Z\0\0]¸Ø" (process-get proc 'socks-response))))
+      (ert-info ("Response field holds last scratch with new segment appended")
+        (should (string= "\0Z\0\0]¸Ø\"" (process-get proc 'socks-response))))
       (ert-info ("Scratch remains populated, matches response field")
-        (should (string= "\"\0Z\0\0]¸Ø" (process-get proc 'socks-scratch)))))
-    (should (equal (string-to-vector "\"\0Z\0\0]¸Ø") [34 0 90 0 0 93 184 216]))
+        (should (string= "\0Z\0\0]¸Ø\"" (process-get proc 'socks-scratch)))))
+    (should (equal (string-to-vector "\0Z\0\0]¸Ø\"") [0 90 0 0 93 184 216 34]))
     (delete-process proc)
     (set-process-query-on-exit-flag proc nil)
     (kill-buffer buf)))
@@ -64,19 +64,30 @@ socks-filter-test-existing-behavior-v5
       (ert-info ("Scratch field holds partial payload of pending msg")
         (should (string= "\5\0\0\4\x26\x05\xbc\x80\x30\x10\x00\x60"
                          (process-get proc 'socks-scratch)))))
-    (ert-info ("Last part arrives")
-      (ert-info ("Error raised because parsing logic expects appended bytes")
-        (let ((err (should-error (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9"))))
-          (should (equal err '(wrong-type-argument number-or-marker-p nil)))))
-      (ert-info ("State untouched (still waiting)")
+    (ert-info ("Second part arrives")
+      (socks-filter proc "\xde\xad\xbe\xef\xca\xfe\xfe\xd9")
+      (ert-info ("State untouched")
         (should (eq (process-get proc 'socks-state) socks-state-waiting)))
       (ert-info ("Response field untouched")
         (should-not (process-get proc 'socks-response)))
-      (ert-info ("Scratch contains new arrival prepended")
-        (should (string=  (concat "\xde\xad\xbe\xef\xca\xfe\xfe\xd9"
-                                  "\5\0\0\4"
-                                  "\x26\x05\xbc\x80\x30\x10\x00\x60")
+      (ert-info ("Scratch contains new arrival appended")
+        (should (string=  (concat "\5\0\0\4"
+                                  "\x26\x05\xbc\x80\x30\x10\x00\x60"
+                                  "\xde\xad\xbe\xef\xca\xfe\xfe\xd9")
                           (process-get proc 'socks-scratch)))))
+    (ert-info ("Final part arrives (port number)")
+      (socks-filter proc "\0\0")
+      (ert-info ("State transitions to complete")
+        (should (eq (process-get proc 'socks-state) socks-state-connected)))
+      (ert-info ("Scratch contains final part appended")
+        (should (string=  (concat "\5\0\0\4"
+                                  "\x26\x05\xbc\x80\x30\x10\x00\x60"
+                                  "\xde\xad\xbe\xef\xca\xfe\xfe\xd9"
+                                  "\0\0")
+                          (process-get proc 'socks-scratch))))
+      (ert-info ("Response field updated with final message")
+        (should (string= (process-get proc 'socks-response)
+                         (process-get proc 'socks-scratch)))))
     (delete-process proc)
     (set-process-query-on-exit-flag proc nil)
     (kill-buffer buf)))
-- 
2.28.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Append-incremental-message-segments-in-socks-filter.patch --]
[-- Type: text/x-patch, Size: 1881 bytes --]

From ae5b7b4c19ebdf53f84e0bf4750a734ae1fe908d Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 10 Dec 2020 03:12:49 -0800
Subject: [PATCH 3/3] Append incremental message segments in socks-filter

Prior to this, arriving segments were prepended, resulting in a
scrambled message being stored in the socks-response property for the
network process.

This also fixes a bug in socks--open-network-stream involving the
socks-service property being set to the host instead of the port.
---
 lisp/net/socks.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/net/socks.el b/lisp/net/socks.el
index 9b22a5083fb..cb50a0adbea 100644
--- a/lisp/net/socks.el
+++ b/lisp/net/socks.el
@@ -260,7 +260,7 @@ socks-filter
     (setq state (process-get proc 'socks-state))
     (cond
      ((= state socks-state-waiting-for-auth)
-      (cl-callf (lambda (s) (setq string (concat string s)))
+      (cl-callf (lambda (s) (setq string (concat s string)))
           (process-get proc 'socks-scratch))
       (if (< (length string) 2)
 	  nil				; We need to spin some more
@@ -272,7 +272,7 @@ socks-filter
      ((= state socks-state-authenticated)
       )
      ((= state socks-state-waiting)
-      (cl-callf (lambda (s) (setq string (concat string s)))
+      (cl-callf (lambda (s) (setq string (concat s string)))
           (process-get proc 'socks-scratch))
       (setq version (process-get proc 'socks-server-protocol))
       (cond
@@ -542,7 +542,7 @@ socks--open-network-stream
 			      service))
         (process-put proc 'socks-buffer buffer)
         (process-put proc 'socks-host host)
-        (process-put proc 'socks-service host)
+        (process-put proc 'socks-service service)
         (set-process-filter proc nil)
         (set-process-buffer proc (if buffer (get-buffer-create buffer)))
         proc))))
-- 
2.28.0

bug#45162: 28.0.50; Messages scrambled by socks-filter

[Lars Ingebrigtsen]

"J.P." <jp@neverwas.me> writes:

> Rather than try to explain, I've included a throwaway before/after test
> showing current and expected behavior, as well as a small patch that
> seems to fix the issue. If this is all nonsense, please forgive me. I am
> no expert, just a grateful end user.

Thanks; the patch makes sense to me, so I've applied it to Emacs 28.

This change was small enough to apply without assigning copyright to the
FSF, but for future patches you want to submit, it might make sense to
get the paperwork started now, so that subsequent patches can be applied
speedily. Would you be willing to sign such paperwork?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45162: 28.0.50; Messages scrambled by socks-filter

[J.P.]

> Thanks; the patch makes sense to me, so I've applied it to Emacs 28.

Oh no, thank YOU, Lars and Emacs!

> ... Would you be willing to sign such paperwork?

Yes sir, I'd be willing.

bug#45162: 28.0.50; Messages scrambled by socks-filter

[Lars Ingebrigtsen]

"J.P." <jp@neverwas.me> writes:

>> ... Would you be willing to sign such paperwork?
>
> Yes sir, I'd be willing.

Great!  Here's the paperwork to get started:



Please email the following information to assign@gnu.org, and we
will send you the assignment form for your past and future changes.

Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES

[What is the name of the program or package you're contributing to?]
Emacs

[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]

[Do you have an employer who might have a basis to claim to own
your changes?  Do you attend a school which might make such a claim?]

[For the copyright registration, what country are you a citizen of?]

[What year were you born?]

[Please write your email address here.]

[Please write your postal address here.]

[Which files have you changed so far, and which new files have you written
so far?]

bug#45162: [PATCH] Fixup Re: bug#45162: 28.0.50; Messages scrambled by socks-filter

[J.P.]

[-- Attachment #1: Type: text/plain, Size: 2962 bytes --]

Lars and Emacs,

My sincerest apologies. I'm embarrassed/ashamed to admit that my
previous patch,

  Commit: abc8d6b9465fecb989170426756c7ee4b133fd40
  "Append incremental message segments in socks-filter"

was not only incomplete but effectively *broke* socks.el for anyone
who's tried using it since!

In a boneheaded oversight, I neglected to reset the "scratch" work area
after functions performing the SOCKS5 authentication subprotocol are
through with it.

This is a one-line follow-up that hopefully corrects that. Please see
the included test, which passes after this tweak is applied.

Below is a long-winded attempt to explain the awkward placement
of this change for anyone who cares, present or future.

Thanks (and again, so sorry!),
J.P.

P.S. Please let me know if I need to open a new bug report.


Summary of [1] using concrete values for SOCKS 5 and USER/PASS method [2]:

  0502 0002              c: auth offer, socks-ver len-methods methods ...
  0502                   s: auth choice, socks-version method
  0103 666f 6f03 6261 72 c: attempt, version len-user "foo" len-pass "bar"
  0100                   s: verdict, version status

1. (BG) In socks-filter, the WAITING-FOR-AUTH case stashes the latest
   chunk by extending the existing stash rightward. In practice, this is
   the first chunk, which contains the server's selection of one of the
   auth methods offered by the client (Emacs) in the initial request. It
   then transitions to SUBMETHOD-NEGOTIATION.

2. (FG) Once socks-open-connection has detected that transition, it
   calls the handler corresponding to the server's chosen auth method to
   handle the subnegotiation.

3. (FG) That handler, most likely socks-username/password-auth, swaps
   out the primary process filter, socks-filter, for a temporary one.

4. (BG) That temp filter, socks-username/password-auth-filter, parses
   out the status code and transitions to AUTHENTICATED (not sure why
   here, exactly)

5. (FG) The auth handler (3), having detected that transition, returns
   control (and a verdict) to the opener (2), which, on success, ensures
   the state is AUTHENTICATED and restores the primary filter
   (socks-filter).

6. (FG) socks-send-command transitions to WAITING before sending the
   main request (method call) as per the protocol, which is similar to
   SOCKS 4.

From the point of view of the socks-filter, the AUTHENTICATED case is
effectively invisible because it's preempted explicitly as described
above (as opposed to, e.g., left as a no-op to avoid a race). Thus, we
can't run anything here, however opportune it may seem. Instead, I
tacked the correction onto the end of the opener (which see).

Ironically, the original approach of prepending chunks on the left
papered over any need to zero out the scratch buffer after these opening
negotiations (for the CONNECT method, anyway).

[1]: https://tools.ietf.org/html/rfc1928#section-3 
[2]: https://tools.ietf.org/html/rfc1929

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Drop-me-add-test-for-SOCKS5-auth-protocol.patch --]
[-- Type: text/x-patch, Size: 4713 bytes --]

From 9145f6044a75b45807a1c2b9e1703da73d5ddf59 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 7 Jan 2021 01:10:49 -0800
Subject: [PATCH 1/2] Drop me: add test for SOCKS5 auth protocol

---
 test/lisp/net/socks-tests.el | 87 ++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 test/lisp/net/socks-tests.el

diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el
new file mode 100644
index 00000000000..94ba49ab487
--- /dev/null
+++ b/test/lisp/net/socks-tests.el
@@ -0,0 +1,87 @@
+;;; socks-tests.el --- tests for SOCKS -*- coding: utf-8; lexical-binding: t; -*-
+
+;;; Commentary:
+
+;; | (let ((default-directory (expand-file-name "../..")))
+;; |   (compile "make lisp/net/socks-tests.log"))
+
+;;; Code:
+
+(require 'socks)
+(require 'url-http)
+
+;; This ran successfully against curl 7.71 with the following options:
+;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com
+(defun socks-tests-fake-server ()
+  (let* ((lines '(([5 2 0 2] . [5 2]) ; Emacs
+                  ([5 3 0 1 2] . [5 2]) ; curl (+ gssapi)
+                  ([1 3 #x66 #x6f #x6f 3 #x62 #x61 #x72] . [1 0])
+                  ([5 1 0 3 11 ?e ?x ?a ?m ?p ?l ?e ?. ?c ?o ?m 0 80]
+                   . [5 0 0 1 0 0 0 0 0 0])))
+         (sentinel '(lambda (proc event)
+                      (message "[%s]: %s" proc event)
+                      (unless (string-match-p "^open" event)
+                        (message "Quitting")
+                        (kill-emacs 0))))
+         (content (concat "HTTP/1.1 200 OK\r\n"
+                          "Content-Type: text/plain; charset=UTF-8\r\n"
+                          "Content-Length: 13\r\n\r\n"
+                          "Hello World!\n"))
+         (filter `(lambda (proc line)
+                    (let* ((content ,content)
+                           (resp (or (assoc-default (vconcat line) test-lines)
+                                     (when (string-prefix-p "GET /" line)
+                                       content))))
+                      (message "-> %s" line)
+                      (message "<- %s" resp)
+                      (process-send-string proc (concat resp)))))
+         (program `(progn
+                     (defvar test-lines ',lines)
+                     (let ((proc (make-network-process :server 1
+                                                       :buffer (current-buffer)
+                                                       :filter ,filter
+                                                       :sentinel ,sentinel
+                                                       :name "testnet"
+                                                       :family 'ipv4
+                                                       :coding 'raw-text
+                                                       :host "127.0.0.1"
+                                                       :service 10080)))
+                       (message "[%s] Listening on 127.0.0.1:10080" proc)
+                       (while (process-live-p proc)
+                         (sleep-for 1)))))
+         (buffer (generate-new-buffer "*fake-socks-server*"))
+         (server (start-process "fake-socks-server" buffer
+                                (concat invocation-directory invocation-name)
+                                "-Q" "--batch" "--eval" (format "%S" program))))
+    (set-process-query-on-exit-flag server nil)
+    (with-current-buffer buffer
+      (while (not (string-match-p "Listening" (buffer-string)))
+        (sleep-for 0.1)))
+    server))
+
+(ert-deftest socks-tests-auth-filter-canned-e2e ()
+  ;; 0.31 secs on my machine but marked as expensive because involves IO
+  :tags '(:expensive-test)
+  (let* ((socks-server `("fake-server" "127.0.0.1" 10080 5))
+         (socks-username "foo")
+         (socks-password "bar")
+         (url-gateway-method 'socks)
+         (url (url-generic-parse-url "http://example.com"))
+         (server (socks-tests-fake-server))
+         ;;
+         done
+         ;;
+         (cb (lambda (&rest _r)
+               (goto-char (point-min))
+               (should (search-forward "Hello World" nil t))
+               (message "Found: %S" (buffer-substring (line-beginning-position)
+                                                      (line-end-position)))
+               (setq done t))))
+    (ignore url-gateway-method)
+    (ert-info ("Connect to HTTP endpoint over SOCKS5 with USER/PASS method")
+      (url-http url cb '(nil))
+      (while (not done) (sleep-for 0.1)))
+    (delete-process server)
+    (kill-buffer (process-buffer server))))
+
+;;; socks-tests.el ends here
-- 
2.29.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Clear-socks-protocol-scratch-after-authentication.patch --]
[-- Type: text/x-patch, Size: 943 bytes --]

From 81b83735f97383b4ff863a051ab695a321d3fd1c Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 7 Jan 2021 01:13:19 -0800
Subject: [PATCH 2/2] Clear socks protocol scratch after authentication

* lisp/net/socks.el (socks-open-connection): Fix incomplete patch
titled "Append incremental message segments in socks-filter," which
addressed chunk ordering but neglected to zero out the work area
following successful username/password authentication (bug#45162).

Copyright-paperwork-exempt: yes
---
 lisp/net/socks.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/net/socks.el b/lisp/net/socks.el
index 0d48fd7e05a..96fafc826b8 100644
--- a/lisp/net/socks.el
+++ b/lisp/net/socks.el
@@ -385,6 +385,7 @@ socks-open-connection
 	  )
 	 )
 	(process-put proc 'socks-state socks-state-authenticated)
+	(process-put proc 'socks-scratch "")
 	(set-process-filter proc #'socks-filter)))
       proc)))
 
-- 
2.29.2

bug#45162: [PATCH] Fixup Re: bug#45162: 28.0.50; Messages scrambled by socks-filter

[Lars Ingebrigtsen]

"J.P." <jp@neverwas.me> writes:

> In a boneheaded oversight, I neglected to reset the "scratch" work area
> after functions performing the SOCKS5 authentication subprotocol are
> through with it.
>
> This is a one-line follow-up that hopefully corrects that. Please see
> the included test, which passes after this tweak is applied.

Thanks; I've now applied the fix to Emacs 28, but not the test (because
the amount of code would require a copyright assignment).

I forgot whether I've asked before -- would you be willing to sign such
paperwork?  If so, reworking that test into an ert-deftest would be
nice.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45162: [PATCH] Fixup Re: bug#45162: 28.0.50; Messages scrambled by socks-filter

[J.P.]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I forgot whether I've asked before -- would you be willing to sign such
> paperwork?  If so, reworking that test into an ert-deftest would be
> nice.

I did indeed begin the paperwork process and sent the signed documents
on December 28. I'd be happy to rework the test portion to conform to
your standards and, when I hear back, resubmit it (in this thread?).

BTW, in case you didn't notice, the one test in the last attachment is
already an ert-deftest. But I'm sure I failed to adequately follow the
coding conventions. I'll definitely review those and revise accordingly.

Thanks so much, Lars, for being so gracious and tolerating my
sloppiness. I pledge to be more responsible in the future.

bug#45162: [PATCH] Fixup Re: bug#45162: 28.0.50; Messages scrambled by socks-filter

[J.P.]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

Just got my paperwork sorted, yay! (Famous last words.)

I've attempted to rework the test as suggested. It now runs in process
instead of spawning another Emacs. I wasn't sure whether having url-http
as a dependency made sense but left it as is. Happy to change whatever
needs changing.

Thanks again,
J.P.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Create-new-test-file-for-socks.el.patch --]
[-- Type: text/x-patch, Size: 5277 bytes --]

From 9b89dcdc8702afaebff61708be4c5d6da90d1108 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sat, 9 Jan 2021 02:52:08 -0800
Subject: [PATCH] Create new test file for socks.el

* test/lisp/net/socks-tests.el (socks-tests-auth-filter-url-http): Add
SOCKS5 authentication test and fake server.  Requires url-http.
---
 test/lisp/net/socks-tests.el | 103 +++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)
 create mode 100644 test/lisp/net/socks-tests.el

diff --git a/test/lisp/net/socks-tests.el b/test/lisp/net/socks-tests.el
new file mode 100644
index 00000000000..b378ed2964e
--- /dev/null
+++ b/test/lisp/net/socks-tests.el
@@ -0,0 +1,103 @@
+;;; socks-tests.el --- tests for SOCKS -*- coding: utf-8; lexical-binding: t; -*-
+
+;; Copyright (C) 2021 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'socks)
+(require 'url-http)
+
+(defvar socks-tests-canned-server-port nil)
+
+(defun socks-tests-canned-server-create (verbatim patterns)
+  "Create a fake SOCKS server and return the process.
+
+`VERBATIM' and `PATTERNS' are dotted alists containing responses.
+Requests are tried in order.  On failure, an error is raised."
+  (let* ((buf (generate-new-buffer "*canned-socks-server*"))
+         (filt (lambda (proc line)
+                 (let ((resp (or (assoc-default line verbatim
+                                                (lambda (k s) ; s is line
+                                                  (string= (concat k) s)))
+                                 (assoc-default line patterns
+                                                (lambda (p s)
+                                                  (string-match-p p s))))))
+                   (unless resp
+                     (error "Unknown request: %s" line))
+                   (let ((print-escape-control-characters t))
+                     (princ (format "<- %s\n" (prin1-to-string line)) buf)
+                     (princ (format "-> %s\n" (prin1-to-string resp)) buf))
+                   (process-send-string proc (concat resp)))))
+         (srv (make-network-process :server 1
+                                    :buffer buf
+                                    :filter filt
+                                    :name "server"
+                                    :family 'ipv4
+                                    :host 'local
+                                    :service socks-tests-canned-server-port)))
+    (set-process-query-on-exit-flag srv nil)
+    (princ (format "[%s] Listening on localhost:10080\n" srv) buf)
+    srv))
+
+;; Add ([5 3 0 1 2] . [5 2]) to the `verbatim' list below to validate
+;; against curl 7.71 with the following options:
+;; $ curl --verbose -U foo:bar --proxy socks5h://127.0.0.1:10080 example.com
+;;
+;; If later implementing version 4a, try these:
+;; [4 1 0 80 0 0 0 1 0 ?e ?x ?a ?m ?p ?l ?e ?. ?c ?o ?m 0] . [0 90 0 0 0 0 0 0]
+;; $ curl --verbose --proxy socks4a://127.0.0.1:10080 example.com
+
+(ert-deftest socks-tests-auth-filter-url-http ()
+  "Verify correct handling of SOCKS5 user/pass authentication."
+  (let* ((socks-server '("server" "127.0.0.1" 10080 5))
+         (socks-username "foo")
+         (socks-password "bar")
+         (url-gateway-method 'socks)
+         (url (url-generic-parse-url "http://example.com"))
+         (verbatim '(([5 2 0 2] . [5 2])
+                     ([1 3 ?f ?o ?o 3 ?b ?a ?r] . [1 0])
+                     ([5 1 0 3 11 ?e ?x ?a ?m ?p ?l ?e ?. ?c ?o ?m 0 80]
+                      . [5 0 0 1 0 0 0 0 0 0])))
+         (patterns
+          `(("^GET /" . ,(concat "HTTP/1.1 200 OK\r\n"
+                                 "Content-Type: text/plain; charset=UTF-8\r\n"
+                                 "Content-Length: 13\r\n\r\n"
+                                 "Hello World!\n"))))
+         (socks-tests-canned-server-port 10080)
+         (server (socks-tests-canned-server-create verbatim patterns))
+         (tries 10)
+         ;;
+         done
+         ;;
+         (cb (lambda (&rest _r)
+               (goto-char (point-min))
+               (should (search-forward "Hello World" nil t))
+               (setq done t)))
+         (buf (url-http url cb '(nil))))
+    (ert-info ("Connect to HTTP endpoint over SOCKS5 with USER/PASS method")
+      (while (and (not done) (< 0 (cl-decf tries))) ; cl-lib via url-http
+        (sleep-for 0.1)))
+    (should done)
+    (delete-process server)
+    (kill-buffer (process-buffer server))
+    (kill-buffer buf)
+    (ignore url-gateway-method)))
+
+;;; socks-tests.el ends here
-- 
2.29.2

bug#45162: [PATCH] Fixup Re: bug#45162: 28.0.50; Messages scrambled by socks-filter

[Lars Ingebrigtsen]

"J.P." <jp@neverwas.me> writes:

> Just got my paperwork sorted, yay! (Famous last words.)

Great timing.  :-)

> I've attempted to rework the test as suggested. It now runs in process
> instead of spawning another Emacs. I wasn't sure whether having url-http
> as a dependency made sense but left it as is. Happy to change whatever
> needs changing.

Looks good to me; pushed to Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no