From 02acd349f9938c66c503f7d0985950745a0e96b5 Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 08:20:55 -0800
Subject: [PATCH 01/19] Rename `urlString` -> `targetString`.

---
 .../export/ProtocolWrangler.js                |  2 +-
 src/network-protocol/export/Request.js        | 35 ++++++++++---------
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/src/network-protocol/export/ProtocolWrangler.js b/src/network-protocol/export/ProtocolWrangler.js
index 6a7ba0615..6b0497148 100644
--- a/src/network-protocol/export/ProtocolWrangler.js
+++ b/src/network-protocol/export/ProtocolWrangler.js
@@ -381,7 +381,7 @@ export class ProtocolWrangler {
     // legitimately be the form used when talking to a proxy (see RFC7230,
     // section 5.3). `Request` does its own sanity check and will reject those,
     // but we should catch it here, in a less ad-hoc way, before trying to
-    // construct the `Request`.
+    // construct the `Request`. And we should log it!
     let request;
     try {
       request = new Request(context, req, res, this.#requestLogger);
diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index 9c5e091d2..5ab2644d9 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -241,7 +241,7 @@ export class Request {
   }
 
   /**
-   * @returns {string} The path portion of {@link #urlString}, as a string.
+   * @returns {string} The path portion of {@link #targetString}, as a string.
    * This starts with a slash (`/`) and omits the search a/k/a query (`?...`),
    * if any. This also includes "resolving" away any `.` or `..` components.
    *
@@ -258,7 +258,7 @@ export class Request {
   }
 
   /**
-   * @returns {string} The search a/k/a query portion of {@link #urlString},
+   * @returns {string} The search a/k/a query portion of {@link #targetString},
    * as an unparsed string, or `''` (the empty string) if there is no search
    * string. The result includes anything at or after the first question mark
    * (`?`) in the URL.
@@ -288,9 +288,9 @@ export class Request {
    * meaningful computation (hence the name).
    */
   get urlForLogging() {
-    const { protocol, host, urlString } = this;
+    const { protocol, host, targetString } = this;
 
-    return `${protocol}://${host.nameString}${urlString}`;
+    return `${protocol}://${host.nameString}${targetString}`;
   }
 
   /**
@@ -300,11 +300,11 @@ export class Request {
    *
    * For example, for the requested URL
    * `https://example.com:123/foo/bar?baz=10`, this would be `/foo/bar?baz=10`.
-   * This field name, though arguably confusing, is as such so as to harmonize
-   * with the standard Node field `IncomingRequest.url`. The `url` name with
-   * similar semantics is also used by Express.
+   * This property name corresponds to the standard Node field
+   * `IncomingRequest.url`, even though it's not actually a URL per se. We chose
+   * to diverge from Node for the sake of clarity.
    */
-  get urlString() {
+  get targetString() {
     // Note: Though this framework uses Express under the covers (as of this
     // writing), and Express _does_ rewrite the underlying request's `.url` in
     // some circumstances, the way we use Express should never cause it to do
@@ -424,7 +424,7 @@ export class Request {
   async notFound(contentType = null, body = null) {
     const sendOpts = body
       ? { contentType, body }
-      : { bodyExtra: `  ${this.urlString}\n` };
+      : { bodyExtra: `  ${this.targetString}\n` };
 
     return this.#sendNonContentResponse(404, sendOpts);
   }
@@ -725,19 +725,20 @@ export class Request {
   }
 
   /**
-   * @returns {URL} The parsed version of {@link #urlString}. This is a
+   * @returns {URL} The parsed version of {@link #targetString}. This is a
    * private getter because the return value is mutable, and we don't want to
    * allow clients to actually mutate it.
    */
   get #parsedUrl() {
     if (!this.#parsedUrlObject) {
-      // Note: An earlier version of this code said `new URL(this.urlString,
-      // 'x://x')`, so as to make it possible for `urlString` to omit the scheme
-      // and host. However, that was totally incorrect, because the _real_
-      // requirement is for `urlString` to _always_ be the path. The most
-      // notable case where the old code failed was in parsing a path that began
-      // with two slashes, which would get incorrectly parsed as having a host.
-      const urlObj = new URL(`x://x${this.urlString}`);
+      // Note: An earlier version of this code said `new URL(this.targetString,
+      // 'x://x')`, so as to make it possible for `targetString` to omit the
+      // scheme and host. However, that was totally incorrect, because the
+      // _real_ requirement is for `targetString` to _always_ be the path. The
+      // most notable case where the old code failed was in parsing a path that
+      // began with two slashes, which would get incorrectly parsed as having a
+      // host.
+      const urlObj = new URL(`x://x${this.targetString}`);
 
       if (urlObj.pathname === '') {
         // Shouldn't normally happen, but tolerate an empty pathname, converting

From 092004081a290d97abaf88d81bba0bdc66d5b611 Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 08:25:36 -0800
Subject: [PATCH 02/19] Rename `parsedUrl` -> `parsedTarget`.

---
 src/network-protocol/export/Request.js | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index 5ab2644d9..b9aa42682 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -71,12 +71,12 @@ export class Request {
   #host = null;
 
   /**
-   * @type {?URL} The parsed version of `.#expressRequest.url`, or `null` if not
-   * yet calculated. **Note:** Despite its name, `.url` doesn't contain any of
-   * the usual URL bits before the start of the path, so those fields are
-   * meaningless here.
+   * @type {?URL} The parsed version of {@link #targetString}, or `null` if not
+   * yet calculated. **Note:** Despite it being an instance of `URL`, the
+   * `target` doesn't ever contain the parts of a URL before the path, so those
+   * fields are meaningless here.
    */
-  #parsedUrlObject = null;
+  #parsedTargetObject = null;
 
   /**
    * @type {?TreePathKey} The parsed version of {@link #pathnameString}, or
@@ -249,7 +249,7 @@ export class Request {
    * standard `URL` class.
    */
   get pathnameString() {
-    return this.#parsedUrl.pathname;
+    return this.#parsedTarget.pathname;
   }
 
   /** @returns {string} The name of the protocol which spawned this instance. */
@@ -267,7 +267,7 @@ export class Request {
    * standard `URL` class.
    */
   get searchString() {
-    return this.#parsedUrl.search;
+    return this.#parsedTarget.search;
   }
 
   /**
@@ -729,8 +729,8 @@ export class Request {
    * private getter because the return value is mutable, and we don't want to
    * allow clients to actually mutate it.
    */
-  get #parsedUrl() {
-    if (!this.#parsedUrlObject) {
+  get #parsedTarget() {
+    if (!this.#parsedTargetObject) {
       // Note: An earlier version of this code said `new URL(this.targetString,
       // 'x://x')`, so as to make it possible for `targetString` to omit the
       // scheme and host. However, that was totally incorrect, because the
@@ -747,10 +747,10 @@ export class Request {
         urlObj.pathname = '/';
       }
 
-      this.#parsedUrlObject = urlObj;
+      this.#parsedTargetObject = urlObj;
     }
 
-    return this.#parsedUrlObject;
+    return this.#parsedTargetObject;
   }
 
   /**

From 68c42960b4ec45c5a6efcec56d99f918cbb23775 Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 08:26:24 -0800
Subject: [PATCH 03/19] Sort.

---
 src/network-protocol/export/Request.js | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index b9aa42682..945aebb46 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -281,18 +281,6 @@ export class Request {
     return this.#expressRequest.secureCookies ?? null;
   }
 
-  /**
-   * @returns {string} A reasonably-suggestive but possibly incomplete
-   * representation of the incoming request, in the form of an URL. This is
-   * meant for logging, and specifically _not_ for any routing or other more
-   * meaningful computation (hence the name).
-   */
-  get urlForLogging() {
-    const { protocol, host, targetString } = this;
-
-    return `${protocol}://${host.nameString}${targetString}`;
-  }
-
   /**
    * @returns {string} The unparsed URL path that was passed in to the original
    * HTTP(ish) request. Colloquially, this is the suffix of the URL-per-se
@@ -314,6 +302,18 @@ export class Request {
     return this.#expressRequest.url;
   }
 
+  /**
+   * @returns {string} A reasonably-suggestive but possibly incomplete
+   * representation of the incoming request, in the form of an URL. This is
+   * meant for logging, and specifically _not_ for any routing or other more
+   * meaningful computation (hence the name).
+   */
+  get urlForLogging() {
+    const { protocol, host, targetString } = this;
+
+    return `${protocol}://${host.nameString}${targetString}`;
+  }
+
   /**
    * Gets all reasonably-logged info about the response that was made. This
    * method async-returns after the response has been completed, either

From 9b75ea04b3301ada4ebd81fe1d226cdf323608ef Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 08:28:41 -0800
Subject: [PATCH 04/19] Better note.

---
 src/network-protocol/export/Request.js | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index 945aebb46..db2ee9d45 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -732,12 +732,12 @@ export class Request {
   get #parsedTarget() {
     if (!this.#parsedTargetObject) {
       // Note: An earlier version of this code said `new URL(this.targetString,
-      // 'x://x')`, so as to make it possible for `targetString` to omit the
-      // scheme and host. However, that was totally incorrect, because the
-      // _real_ requirement is for `targetString` to _always_ be the path. The
-      // most notable case where the old code failed was in parsing a path that
-      // began with two slashes, which would get incorrectly parsed as having a
-      // host.
+      // 'x://x')`, so as to make the constructor work given that `targetString`
+      // should omit the scheme and host. However, that was totally incorrect,
+      // because the _real_ requirement is for `targetString` to _always_ be
+      // _just_ the path. The most notable case where the old code failed was in
+      // parsing a path that began with two slashes, which would get incorrectly
+      // parsed as having a host.
       const urlObj = new URL(`x://x${this.targetString}`);
 
       if (urlObj.pathname === '') {

From 9c75085afec6550fa4b29fdc09a96b9ebb6b34b9 Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 08:36:36 -0800
Subject: [PATCH 05/19] Explain!

---
 src/network-protocol/export/Request.js | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index db2ee9d45..d447ca46b 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -282,9 +282,13 @@ export class Request {
   }
 
   /**
-   * @returns {string} The unparsed URL path that was passed in to the original
-   * HTTP(ish) request. Colloquially, this is the suffix of the URL-per-se
-   * starting at the first slash (`/`) after the host identifier.
+   * @returns {string} The unparsed target that was passed in to the original
+   * HTTP(ish) request. In the common case of the target being a path to a
+   * resource, colloquially speaking, this is the suffix of the URL-per-se
+   * starting at the first slash (`/`) after the host identifier. That said,
+   * there are other non-path forms for a target. See
+   * <https://www.rfc-editor.org/rfc/rfc7230#section-5.3> for the excruciating
+   * details.
    *
    * For example, for the requested URL
    * `https://example.com:123/foo/bar?baz=10`, this would be `/foo/bar?baz=10`.
@@ -293,6 +297,9 @@ export class Request {
    * to diverge from Node for the sake of clarity.
    */
   get targetString() {
+    // Note: Node calls the target the `.url`, but it's totes _not_ actually a
+    // URL, bless their innocent hearts.
+
     // Note: Though this framework uses Express under the covers (as of this
     // writing), and Express _does_ rewrite the underlying request's `.url` in
     // some circumstances, the way we use Express should never cause it to do

From f740b2f14ca98a88e9f2399fdba99bcddc79d626 Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 09:05:58 -0800
Subject: [PATCH 06/19] Handle all the different `target` forms.

---
 src/network-protocol/export/Request.js | 87 ++++++++++++++++++--------
 1 file changed, 62 insertions(+), 25 deletions(-)

diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index d447ca46b..bdbc74585 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -220,7 +220,9 @@ export class Request {
   }
 
   /**
-   * @returns {TreePathKey} Parsed path key form of {@link #pathnameString}.
+   * @returns {?TreePathKey} Parsed path key form of {@link #pathnameString}, or
+   * `null` if this instance doesn't represent a usual `origin` request.
+   *
    * **Note:** If the original incoming pathname was just `'/'` (e.g., it was
    * from an HTTP request of literally `GET /`), then the value here is a
    * single-element key with empty value, that is `['']`, and _not_ an empty
@@ -229,9 +231,14 @@ export class Request {
    */
   get pathname() {
     if (!this.#parsedPathname) {
+      const { type, pathnameString: pathStr } = this.#parsedTarget;
+
+      if (type !== 'origin') {
+        return null;
+      }
+
       // `slice(1)` to avoid having an empty component as the first element.
-      const pathStr = this.pathnameString;
-      const parts   = pathStr.slice(1).split('/');
+      const parts = pathStr.slice(1).split('/');
 
       // Freezing `parts` lets `new TreePathKey()` avoid making a copy.
       this.#parsedPathname = new TreePathKey(Object.freeze(parts), false);
@@ -241,15 +248,17 @@ export class Request {
   }
 
   /**
-   * @returns {string} The path portion of {@link #targetString}, as a string.
-   * This starts with a slash (`/`) and omits the search a/k/a query (`?...`),
-   * if any. This also includes "resolving" away any `.` or `..` components.
+   * @returns {?string} The path portion of {@link #targetString}, as a string,
+   * or `null` if this instance doesn't represent a usual `origin` request (that
+   * is, the kind that includes a path). This starts with a slash (`/`) and
+   * omits the search a/k/a query (`?...`), if any. This also includes
+   * "resolving" away any `.` or `..` components.
    *
    * **Note:** The name of this field matches the equivalent field of the
    * standard `URL` class.
    */
   get pathnameString() {
-    return this.#parsedTarget.pathname;
+    return this.#parsedTarget.pathnameString ?? null;
   }
 
   /** @returns {string} The name of the protocol which spawned this instance. */
@@ -267,7 +276,7 @@ export class Request {
    * standard `URL` class.
    */
   get searchString() {
-    return this.#parsedTarget.search;
+    return this.#parsedTarget.searchString;
   }
 
   /**
@@ -297,16 +306,7 @@ export class Request {
    * to diverge from Node for the sake of clarity.
    */
   get targetString() {
-    // Note: Node calls the target the `.url`, but it's totes _not_ actually a
-    // URL, bless their innocent hearts.
-
-    // Note: Though this framework uses Express under the covers (as of this
-    // writing), and Express _does_ rewrite the underlying request's `.url` in
-    // some circumstances, the way we use Express should never cause it to do
-    // such rewriting. As such, it's appropriate for us to just use `.url`, and
-    // not the Express-specific `.originalUrl`. (Ultimately, the hope is to drop
-    // use of Express, as it provides little value to this project.)
-    return this.#expressRequest.url;
+    return this.#parsedTarget.targetString;
   }
 
   /**
@@ -732,12 +732,33 @@ export class Request {
   }
 
   /**
-   * @returns {URL} The parsed version of {@link #targetString}. This is a
-   * private getter because the return value is mutable, and we don't want to
-   * allow clients to actually mutate it.
+   * @returns {object} The parsed version of {@link #targetString}. This is a
+   * private getter because the return value is pretty ad-hoc, and we don't want
+   * to expose it as part of this class's API.
    */
   get #parsedTarget() {
-    if (!this.#parsedTargetObject) {
+    if (this.#parsedTargetObject) {
+      return this.#parsedTargetObject;
+    }
+
+    // Note: Node calls the target the `.url`, but it's totes _not_ actually a
+    // URL, bless their innocent hearts.
+    //
+    // Also note: Though this framework uses Express under the covers (as of
+    // this writing), and Express _does_ rewrite the underlying request's `.url`
+    // in some circumstances, the way we use Express should never cause it to do
+    // such rewriting. As such, it's appropriate for us to just use `.url`, and
+    // not the Express-specific `.originalUrl`. (Ultimately, the hope is to drop
+    // use of Express, as it provides little value to this project.)
+    const targetString = this.#expressRequest.url;
+    const result       = { targetString };
+
+    if (targetString.startsWith('/')) {
+      // It's the usual (most common) form for a target, namely an absolute
+      // path. Use `new URL(...)` to parse and canonicalize it. This is the
+      // `origin-form` as defined by
+      // <https://www.rfc-editor.org/rfc/rfc7230#section-5.3.1>.
+
       // Note: An earlier version of this code said `new URL(this.targetString,
       // 'x://x')`, so as to make the constructor work given that `targetString`
       // should omit the scheme and host. However, that was totally incorrect,
@@ -745,7 +766,7 @@ export class Request {
       // _just_ the path. The most notable case where the old code failed was in
       // parsing a path that began with two slashes, which would get incorrectly
       // parsed as having a host.
-      const urlObj = new URL(`x://x${this.targetString}`);
+      const urlObj = new URL(`x://x${targetString}`);
 
       if (urlObj.pathname === '') {
         // Shouldn't normally happen, but tolerate an empty pathname, converting
@@ -754,10 +775,26 @@ export class Request {
         urlObj.pathname = '/';
       }
 
-      this.#parsedTargetObject = urlObj;
+      result.type           = 'origin';
+      result.pathnameString = urlObj.pathname;
+      result.searchString   = urlObj.searc;
+    } else if (targetString === '*') {
+      // This is the `asterisk-form` as defined by
+      // <https://www.rfc-editor.org/rfc/rfc7230#section-5.3.4>.
+      result.type = 'asterisk';
+    } else {
+      // This can be either the `authority-form` or the `absolute-form`, as
+      // defined by <https://www.rfc-editor.org/rfc/rfc7230#section-5.3.2> and
+      // <https://www.rfc-editor.org/rfc/rfc7230#section-5.3.3>. These two
+      // forms have some overlap, so it's not possible to easily tell for sure
+      // which one this is (or if it's just invalid).
+      result.type = 'other';
     }
 
-    return this.#parsedTargetObject;
+    Object.freeze(result);
+    this.#parsedTargetObject = result;
+
+    return result;
   }
 
   /**

From a5b314fedae22487e03da225ef1658ef160845ea Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 09:11:09 -0800
Subject: [PATCH 07/19] Pull more stuff into the target parser.

---
 src/network-protocol/export/Request.js | 44 ++++++++------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index bdbc74585..ac64d10c4 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -78,12 +78,6 @@ export class Request {
    */
   #parsedTargetObject = null;
 
-  /**
-   * @type {?TreePathKey} The parsed version of {@link #pathnameString}, or
-   * `null` if not yet calculated.
-   */
-  #parsedPathname = null;
-
   /**
    * Constructs an instance.
    *
@@ -230,21 +224,7 @@ export class Request {
    * requests end with an empty path element.
    */
   get pathname() {
-    if (!this.#parsedPathname) {
-      const { type, pathnameString: pathStr } = this.#parsedTarget;
-
-      if (type !== 'origin') {
-        return null;
-      }
-
-      // `slice(1)` to avoid having an empty component as the first element.
-      const parts = pathStr.slice(1).split('/');
-
-      // Freezing `parts` lets `new TreePathKey()` avoid making a copy.
-      this.#parsedPathname = new TreePathKey(Object.freeze(parts), false);
-    }
-
-    return this.#parsedPathname;
+    return this.#parsedTarget.pathname ?? null;
   }
 
   /**
@@ -766,18 +746,22 @@ export class Request {
       // _just_ the path. The most notable case where the old code failed was in
       // parsing a path that began with two slashes, which would get incorrectly
       // parsed as having a host.
-      const urlObj = new URL(`x://x${targetString}`);
+      const urlObj  = new URL(`x://x${targetString}`);
 
-      if (urlObj.pathname === '') {
-        // Shouldn't normally happen, but tolerate an empty pathname, converting
-        // it to `/`. (`new URL()` will return an instance like this if there's
-        // no slash after the hostname.)
-        urlObj.pathname = '/';
-      }
+      // Shouldn't normally happen, but tolerate an empty pathname, converting
+      // it to `/`. (`new URL()` will return an instance like this if there's
+      // no slash after the hostname, but by the time we're here,
+      // `targetString` is supposed to start with a slash).
+      const pathnameString = (urlObj.pathname === '') ? '/' : urlObj.pathname;
+
+      // `slice(1)` to avoid having an empty component as the first element. And
+      // Freezing `parts` lets `new TreePathKey()` avoid making a copy.
+      const pathParts = Object.freeze(pathnameString.slice(1).split('/'));
 
       result.type           = 'origin';
-      result.pathnameString = urlObj.pathname;
-      result.searchString   = urlObj.searc;
+      result.pathname       = new TreePathKey(pathParts, false);
+      result.pathnameString = pathnameString;
+      result.searchString   = urlObj.search;
     } else if (targetString === '*') {
       // This is the `asterisk-form` as defined by
       // <https://www.rfc-editor.org/rfc/rfc7230#section-5.3.4>.

From 30d4fa2bcbc84e6891502ed87192f34f8bcebe16 Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 09:12:55 -0800
Subject: [PATCH 08/19] Changelog.

---
 CHANGELOG.md | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index bd68e1c1e..c7d3b85e0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,7 +10,10 @@ Breaking changes:
 * None.
 
 Other notable changes:
-* None.
+* Fixed handling of non-`origin` request targets. Before v0.6.1, these were
+  treated as if they were `origin` requests (that is, the usual kind that
+  specify a resource path), and in v0.6.1 they started causing crashes. Now,
+  they're properly classified.
 
 ### v0.6.1 -- 2024-01-19
 

From f8c0ed2504ce868927da7383332674b113a2564d Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 10:44:57 -0800
Subject: [PATCH 09/19] Add support to log non-`origin` requests.

---
 src/network-protocol/export/Request.js | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index ac64d10c4..341af47b0 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -291,14 +291,18 @@ export class Request {
 
   /**
    * @returns {string} A reasonably-suggestive but possibly incomplete
-   * representation of the incoming request, in the form of an URL. This is
-   * meant for logging, and specifically _not_ for any routing or other more
+   * representation of the incoming request, in the form of a URL. This is meant
+   * for logging, and specifically _not_ for any routing or other more
    * meaningful computation (hence the name).
    */
   get urlForLogging() {
-    const { protocol, host, targetString } = this;
+    const { protocol, host }     = this;
+    const { targetString, type } = this.#parsedTarget;
+    const protoHost              = `${protocol}://${host.nameString}`;
 
-    return `${protocol}://${host.nameString}${targetString}`;
+    return (type === 'origin')
+      ? `${protoHost}${targetString}`
+      : `${protoHost}:${type}=${targetString}`;
   }
 
   /**

From 1f062d23fadeaba38ffb35d1723d69b972295e70 Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 10:49:54 -0800
Subject: [PATCH 10/19] Rename `redirect*()` -> `sendRedirect*()`.

---
 src/builtin-applications/export/Redirector.js  | 2 +-
 src/builtin-applications/export/StaticFiles.js | 2 +-
 src/network-protocol/export/Request.js         | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/builtin-applications/export/Redirector.js b/src/builtin-applications/export/Redirector.js
index 1460eb4da..b7cee7f09 100644
--- a/src/builtin-applications/export/Redirector.js
+++ b/src/builtin-applications/export/Redirector.js
@@ -37,7 +37,7 @@ export class Redirector extends BaseApplication {
 
   /** @override */
   async _impl_handleRequest(request, dispatch) {
-    return request.redirect(
+    return request.sendRedirect(
       `${this.#target}${dispatch.extraString}`,
       this.#statusCode);
   }
diff --git a/src/builtin-applications/export/StaticFiles.js b/src/builtin-applications/export/StaticFiles.js
index 39adb5c18..07a599037 100644
--- a/src/builtin-applications/export/StaticFiles.js
+++ b/src/builtin-applications/export/StaticFiles.js
@@ -60,7 +60,7 @@ export class StaticFiles extends BaseApplication {
 
     if (resolved.redirect) {
       const redirectTo = resolved.redirect;
-      return request.redirect(redirectTo, 301);
+      return request.sendRedirect(redirectTo, 301);
     } else if (resolved.path) {
       return await request.sendFile(resolved.path, StaticFiles.#SEND_OPTIONS);
     } else {
diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index 341af47b0..0d0cd3a24 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -436,7 +436,7 @@ export class Request {
    * @param {number} [status] Status code.
    * @returns {boolean} `true` when the response is completed.
    */
-  async redirect(target, status = 302) {
+  async sendRedirect(target, status = 302) {
     // Note: This method avoids using `express.Response.redirect()` (a) to avoid
     // ambiguity with the argument `"back"`, and (b) generally with an eye
     // towards dropping Express entirely as a dependency.
@@ -459,9 +459,9 @@ export class Request {
    * @param {number} [status] Status code.
    * @returns {boolean} `true` when the response is completed.
    */
-  async redirectBack(status = 302) {
+  async sendRedirectBack(status = 302) {
     const target = this.#expressRequest.header('referrer') ?? '/';
-    return this.redirect(target, status);
+    return this.sendRedirect(target, status);
   }
 
   /**

From d7197f396ce0ab348141eb20e783afcc1de04bf1 Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 10:50:45 -0800
Subject: [PATCH 11/19] Sort.

---
 src/network-protocol/export/Request.js | 88 +++++++++++++-------------
 1 file changed, 44 insertions(+), 44 deletions(-)

diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index 0d0cd3a24..fb93a8ebe 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -420,50 +420,6 @@ export class Request {
     return this.#sendNonContentResponse(404, sendOpts);
   }
 
-  /**
-   * Issues a redirect response, with a standard response message and plain text
-   * body. The response message depends on the status code.
-   *
-   * Calling this method results in this request being considered complete, and
-   * as such no additional response-related methods will work.
-   *
-   * **Note:** This method does _not_ do any URL-encoding on the given `target`.
-   * It is assumed to be valid and already encoded if necessary. (This is unlike
-   * Express which tries to be "smart" about encoding, which can ultimately be
-   * more like "confusing.")
-   *
-   * @param {string} target Possibly-relative target URL.
-   * @param {number} [status] Status code.
-   * @returns {boolean} `true` when the response is completed.
-   */
-  async sendRedirect(target, status = 302) {
-    // Note: This method avoids using `express.Response.redirect()` (a) to avoid
-    // ambiguity with the argument `"back"`, and (b) generally with an eye
-    // towards dropping Express entirely as a dependency.
-
-    MustBe.string(target);
-
-    return this.#sendNonContentResponse(status, {
-      bodyExtra: `  ${target}\n`,
-      headers:   { 'Location': target }
-    });
-  }
-
-  /**
-   * Issues a redirect response targeted at the original request's referrer. If
-   * there was no referrer, this redirects to `/`.
-   *
-   * Calling this method results in this request being considered complete, and
-   * as such no additional response-related methods will work.
-   *
-   * @param {number} [status] Status code.
-   * @returns {boolean} `true` when the response is completed.
-   */
-  async sendRedirectBack(status = 302) {
-    const target = this.#expressRequest.header('referrer') ?? '/';
-    return this.sendRedirect(target, status);
-  }
-
   /**
    * Issues a successful response, with the given body contents or with an empty
    * body as appropriate. The actual reported status will be one of:
@@ -671,6 +627,50 @@ export class Request {
     return this.whenResponseDone();
   }
 
+  /**
+   * Issues a redirect response, with a standard response message and plain text
+   * body. The response message depends on the status code.
+   *
+   * Calling this method results in this request being considered complete, and
+   * as such no additional response-related methods will work.
+   *
+   * **Note:** This method does _not_ do any URL-encoding on the given `target`.
+   * It is assumed to be valid and already encoded if necessary. (This is unlike
+   * Express which tries to be "smart" about encoding, which can ultimately be
+   * more like "confusing.")
+   *
+   * @param {string} target Possibly-relative target URL.
+   * @param {number} [status] Status code.
+   * @returns {boolean} `true` when the response is completed.
+   */
+  async sendRedirect(target, status = 302) {
+    // Note: This method avoids using `express.Response.redirect()` (a) to avoid
+    // ambiguity with the argument `"back"`, and (b) generally with an eye
+    // towards dropping Express entirely as a dependency.
+
+    MustBe.string(target);
+
+    return this.#sendNonContentResponse(status, {
+      bodyExtra: `  ${target}\n`,
+      headers:   { 'Location': target }
+    });
+  }
+
+  /**
+   * Issues a redirect response targeted at the original request's referrer. If
+   * there was no referrer, this redirects to `/`.
+   *
+   * Calling this method results in this request being considered complete, and
+   * as such no additional response-related methods will work.
+   *
+   * @param {number} [status] Status code.
+   * @returns {boolean} `true` when the response is completed.
+   */
+  async sendRedirectBack(status = 302) {
+    const target = this.#expressRequest.header('referrer') ?? '/';
+    return this.sendRedirect(target, status);
+  }
+
   /**
    * Returns when the underlying response has been closed successfully or has
    * errored. Returns `true` for a normal close, or throws whatever error the

From 9ae02ea7cc5b8746e78dd529e8c9f4e33b0ecbe7 Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 10:53:39 -0800
Subject: [PATCH 12/19] Rename `notFound()` -> `sendNotFound()`.

---
 src/builtin-applications/export/StaticFiles.js | 2 +-
 src/network-protocol/export/Request.js         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/builtin-applications/export/StaticFiles.js b/src/builtin-applications/export/StaticFiles.js
index 07a599037..1ca993362 100644
--- a/src/builtin-applications/export/StaticFiles.js
+++ b/src/builtin-applications/export/StaticFiles.js
@@ -52,7 +52,7 @@ export class StaticFiles extends BaseApplication {
 
     if (!resolved) {
       if (this.#notFoundType) {
-        return request.notFound(this.#notFoundType, this.#notFoundContents);
+        return request.sendNotFound(this.#notFoundType, this.#notFoundContents);
       } else {
         return false;
       }
diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index fb93a8ebe..b4e00f714 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -412,7 +412,7 @@ export class Request {
    * @param {string|Buffer} [body] Body content.
    * @returns {boolean} `true` when the response is completed.
    */
-  async notFound(contentType = null, body = null) {
+  async sendNotFound(contentType = null, body = null) {
     const sendOpts = body
       ? { contentType, body }
       : { bodyExtra: `  ${this.targetString}\n` };

From 4037364b2151998b571040e61b06a50db70c8416 Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 10:54:08 -0800
Subject: [PATCH 13/19] Sort.

---
 src/network-protocol/export/Request.js | 40 +++++++++++++-------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index b4e00f714..35ccac25c 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -400,26 +400,6 @@ export class Request {
       Request.#extractHeaders(responseHeaders, 'etag', 'last-modified'));
   }
 
-  /**
-   * Issues a "not found" (status `404`) response, with optional body. If no
-   * body is provided, a simple default plain-text body is used. The response
-   * includes the single content/cache-related header `Cache-Control: no-store,
-   * must-revalidate`. If the request method is `HEAD`, this will _not_ send the
-   * body as part of the response.
-   *
-   * @param {string} [contentType] Content type for the body. Must be valid if
-   *  `body` is passed as non-`null`.
-   * @param {string|Buffer} [body] Body content.
-   * @returns {boolean} `true` when the response is completed.
-   */
-  async sendNotFound(contentType = null, body = null) {
-    const sendOpts = body
-      ? { contentType, body }
-      : { bodyExtra: `  ${this.targetString}\n` };
-
-    return this.#sendNonContentResponse(404, sendOpts);
-  }
-
   /**
    * Issues a successful response, with the given body contents or with an empty
    * body as appropriate. The actual reported status will be one of:
@@ -627,6 +607,26 @@ export class Request {
     return this.whenResponseDone();
   }
 
+  /**
+   * Issues a "not found" (status `404`) response, with optional body. If no
+   * body is provided, a simple default plain-text body is used. The response
+   * includes the single content/cache-related header `Cache-Control: no-store,
+   * must-revalidate`. If the request method is `HEAD`, this will _not_ send the
+   * body as part of the response.
+   *
+   * @param {string} [contentType] Content type for the body. Must be valid if
+   *  `body` is passed as non-`null`.
+   * @param {string|Buffer} [body] Body content.
+   * @returns {boolean} `true` when the response is completed.
+   */
+  async sendNotFound(contentType = null, body = null) {
+    const sendOpts = body
+      ? { contentType, body }
+      : { bodyExtra: `  ${this.targetString}\n` };
+
+    return this.#sendNonContentResponse(404, sendOpts);
+  }
+
   /**
    * Issues a redirect response, with a standard response message and plain text
    * body. The response message depends on the status code.

From 37b485dec45d50cea350a33439640154623db4aa Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 10:58:57 -0800
Subject: [PATCH 14/19] Add `sendError()`.

---
 src/network-protocol/export/Request.js | 39 ++++++++++++++++++--------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index 35ccac25c..1770264b6 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -506,6 +506,28 @@ export class Request {
     return this.whenResponseDone();
   }
 
+  /**
+   * Issues an error (status `4xx` or `5xx`) response, with optional body. If no
+   * body is provided, a simple default plain-text body is used. The response
+   * includes the single content/cache-related header `Cache-Control: no-store,
+   * must-revalidate`. If the request method is `HEAD`, this will _not_ send the
+   * body as part of the response.
+   *
+   * @param {number} statusCode The status code.
+   * @param {?string} [contentType] Content type for the body. Must be valid if
+   *  `body` is passed as non-`null`.
+   * @param {?string|Buffer} [body] Body content.
+   * @returns {boolean} `true` when the response is completed.
+   */
+  async sendError(statusCode, contentType = null, body = null) {
+    MustBe.number(statusCode, { safeInteger: true, minInclusive: 400, maxInclusive: 599 });
+    const sendOpts = body
+      ? { contentType, body }
+      : { bodyExtra: `  ${this.targetString}\n` };
+
+    return this.#sendNonContentResponse(statusCode, sendOpts);
+  }
+
   /**
    * Issues a successful response, with the contents of the given file or with
    * an empty body as appropriate. The actual reported status will vary, with
@@ -608,23 +630,16 @@ export class Request {
   }
 
   /**
-   * Issues a "not found" (status `404`) response, with optional body. If no
-   * body is provided, a simple default plain-text body is used. The response
-   * includes the single content/cache-related header `Cache-Control: no-store,
-   * must-revalidate`. If the request method is `HEAD`, this will _not_ send the
-   * body as part of the response.
+   * Issues a "not found" (status `404`) response, with optional body. This is
+   * just a convenient shorthand for `sendError(404, ...)`.
    *
-   * @param {string} [contentType] Content type for the body. Must be valid if
+   * @param {?string} [contentType] Content type for the body. Must be valid if
    *  `body` is passed as non-`null`.
-   * @param {string|Buffer} [body] Body content.
+   * @param {?string|Buffer} [body] Body content.
    * @returns {boolean} `true` when the response is completed.
    */
   async sendNotFound(contentType = null, body = null) {
-    const sendOpts = body
-      ? { contentType, body }
-      : { bodyExtra: `  ${this.targetString}\n` };
-
-    return this.#sendNonContentResponse(404, sendOpts);
+    return this.sendError(404, contentType, body);
   }
 
   /**

From dcb2d6dca9ce22c076a472df5eb0d799d300657e Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 11:26:07 -0800
Subject: [PATCH 15/19] Reject non-`origin` requests in a better way.

---
 src/network-protocol/export/ProtocolWrangler.js | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/network-protocol/export/ProtocolWrangler.js b/src/network-protocol/export/ProtocolWrangler.js
index 6b0497148..af6069ec8 100644
--- a/src/network-protocol/export/ProtocolWrangler.js
+++ b/src/network-protocol/export/ProtocolWrangler.js
@@ -399,10 +399,14 @@ export class ProtocolWrangler {
 
     res.set('Server', this.#serverHeader);
 
-    if (this.#rateLimiter) {
+    if (!request.pathnameString) {
+      // It's not an `origin` request. We don't handle any other type of
+      // target... yet.
+      return request.sendError(400); // "Bad Request."
+    } else if (this.#rateLimiter) {
       const granted = await this.#rateLimiter.newRequest(reqLogger);
       if (!granted) {
-        res.sendStatus(503);
+        res.sendStatus(503); // "Service Unavailable."
 
         // Wait for the response to have been at least nominally sent before
         // closing the socket, in the hope that there is a good chance that it

From 9adfb02c948f549621fdfcb45577f96c99a6e80a Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 11:28:03 -0800
Subject: [PATCH 16/19] Remove temporary hack.

---
 src/network-protocol/export/ProtocolWrangler.js | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/src/network-protocol/export/ProtocolWrangler.js b/src/network-protocol/export/ProtocolWrangler.js
index af6069ec8..ad0268c0e 100644
--- a/src/network-protocol/export/ProtocolWrangler.js
+++ b/src/network-protocol/export/ProtocolWrangler.js
@@ -375,21 +375,8 @@ export class ProtocolWrangler {
    *   to run.
    */
   async #handleExpressRequest(req, res, next) {
-    const context = WranglerContext.getNonNull(req.socket, req.stream?.session);
-
-    // TODO: `request.url` is not sanitized by Node, and furthermore it could
-    // legitimately be the form used when talking to a proxy (see RFC7230,
-    // section 5.3). `Request` does its own sanity check and will reject those,
-    // but we should catch it here, in a less ad-hoc way, before trying to
-    // construct the `Request`. And we should log it!
-    let request;
-    try {
-      request = new Request(context, req, res, this.#requestLogger);
-    } catch (e) {
-      res.sendStatus(400); // "Bad Request."
-      return;
-    }
-
+    const context   = WranglerContext.getNonNull(req.socket, req.stream?.session);
+    const request   = new Request(context, req, res, this.#requestLogger);
     const reqLogger = request.logger;
 
     const reqCtx = WranglerContext.forRequest(context, request);

From 81535f92274da10ceb971cafc9672d3b10e1520b Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 11:29:27 -0800
Subject: [PATCH 17/19] Use `Request`.

---
 src/network-protocol/export/ProtocolWrangler.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/network-protocol/export/ProtocolWrangler.js b/src/network-protocol/export/ProtocolWrangler.js
index ad0268c0e..a599ee6b3 100644
--- a/src/network-protocol/export/ProtocolWrangler.js
+++ b/src/network-protocol/export/ProtocolWrangler.js
@@ -393,7 +393,7 @@ export class ProtocolWrangler {
     } else if (this.#rateLimiter) {
       const granted = await this.#rateLimiter.newRequest(reqLogger);
       if (!granted) {
-        res.sendStatus(503); // "Service Unavailable."
+        await request.sendError(503); // "Service Unavailable."
 
         // Wait for the response to have been at least nominally sent before
         // closing the socket, in the hope that there is a good chance that it

From 2579bd30a005df8f0964979dd541d985952ea4cd Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 13:03:42 -0800
Subject: [PATCH 18/19] Remove temporary debugging code.

---
 src/network-protocol/export/Request.js | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/src/network-protocol/export/Request.js b/src/network-protocol/export/Request.js
index 1770264b6..844567fec 100644
--- a/src/network-protocol/export/Request.js
+++ b/src/network-protocol/export/Request.js
@@ -105,20 +105,6 @@ export class Request {
       this.#id     = logger.$meta.makeId();
       this.#logger = logger[this.#id];
     }
-
-    if (!/^[/]/.test(request.url)) {
-      // Sanity check. If we end up here, it's a bug and not (in particular) a
-      // malformed request (which never should have made it this far).
-      // TODO: In practice this is happening, and it's not clear why. Log it,
-      // so we can figure out what's going on.
-      this.#logger.strangeOriginalUrl({
-        hostname: request.hostname,
-        method:   request.method,
-        protocol: request.protocol,
-        url:      request.url
-      });
-      throw new Error(`Shouldn't happen: ${request.url}`);
-    }
   }
 
   /**

From 299c72c23d0d8e16e79470389102c5089a73b7fa Mon Sep 17 00:00:00 2001
From: Dan Bornstein <danfuzz@murtbo.com>
Date: Sat, 20 Jan 2024 13:12:07 -0800
Subject: [PATCH 19/19] Appease the linter.

---
 src/network-protocol/export/ProtocolWrangler.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/network-protocol/export/ProtocolWrangler.js b/src/network-protocol/export/ProtocolWrangler.js
index a599ee6b3..d002bfe5a 100644
--- a/src/network-protocol/export/ProtocolWrangler.js
+++ b/src/network-protocol/export/ProtocolWrangler.js
@@ -389,7 +389,8 @@ export class ProtocolWrangler {
     if (!request.pathnameString) {
       // It's not an `origin` request. We don't handle any other type of
       // target... yet.
-      return request.sendError(400); // "Bad Request."
+      await request.sendError(400); // "Bad Request."
+      return;
     } else if (this.#rateLimiter) {
       const granted = await this.#rateLimiter.newRequest(reqLogger);
       if (!granted) {