Re: [w3c/ServiceWorker] Add a static routing API support. (PR #1686)

@domenic commented on this pull request.



> +        Promise<undefined> registerRouter((RouterRule or sequence<RouterRule>) rules);
+      };
+
+      dictionary RouterRule {
+        required RouterCondition condition;
+        required RouterSourceEnum source;
+      };
+
+      dictionary RouterCondition {
+        USVString urlPattern;
+      };
+
+      enum RouterSourceEnum { "network" };
+    </pre>
+
+    Each {{RouterCondition/urlPattern}} object has an associated <dfn>hostname component</dfn>, a [=urlpattern/component=], which is initially unset.

This doesn't really work. You can't associate a component with a string inside a dictionary.

I think the way to do this is to create a parallel hierarchy of [structs](https://infra.spec.whatwg.org/#structs) mirroring your dictionaries, that represent the parsed result. E.g. a "router rule" struct with:

- source (a `RouterSourceEnum`)
- condition (a router condition struct)

and then a "router condition" struct with

- urlPattern, a component

Then the static router rules object should be a list of parsed "router rules", not a list of `RouterRule`s.

> +      };
+
+      enum RouterSourceEnum { "network" };
+    </pre>
+
+    Each {{RouterCondition/urlPattern}} object has an associated <dfn>hostname component</dfn>, a [=urlpattern/component=], which is initially unset.
+
+    <section>
+      <h4 id="register-router-method">{{InstallEvent/registerRouter(rules)|event.registerRouter(rules)}}</h4>
+
+      {{InstallEvent/registerRouter(rules)}} registers this service worker the rules to offload simple tasks that the fetch handler does.
+
+      <dfn method for="InstallEvent"><code>registerRouter(|rules|)</code></dfn> method *must* run these steps:
+
+        1. Let |routerRules| be a list of {{RouterRule}} dictionaries.
+        1. If |rules| is a {{RouterRule}} dictionary:

It seems simpler to have one step be

1. If |rules| is a {{RouterRule}} dictionary, set |rules| to « |rules| ».

(this is using the notation from https://infra.spec.whatwg.org/#lists).

> @@ -3177,6 +3231,41 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
       1. Return |response|.
   </section>
 
+  <section algorithm>
+    <h3 id="verify-router-rule-algorithm"><dfn>VerifyRouterRule</dfn></h3>
+
+      : Input
+      :: |rule|, a {{RouterRule}}
+      : Output
+      :: a boolean
+
+      1. If |rule|["{{RouterRule/condition}}"]["{{RouterCondition/urlPattern}}"] is the empty string, return false.
+      1. Let |urlPattern| be a result of [=urlpattern/parsing=] |rule|["{{RouterRule/condition}}"]["{{RouterCondition/urlPattern}}"]. If a <code>TypeError</code> is thrown, return false.
+      1. [=list/For each=] |part| of |urlPattern|:
+          1. If |part|'s [=urlpattern-part/type=] is "regexp", return false.

```suggestion
          1. If |part|'s [=urlpattern-part/type=] is "<code>regexp</code>", return false.
```

> +  <section algorithm>
+    <h3 id="verify-router-rule-algorithm"><dfn>VerifyRouterRule</dfn></h3>
+
+      : Input
+      :: |rule|, a {{RouterRule}}
+      : Output
+      :: a boolean
+
+      1. If |rule|["{{RouterRule/condition}}"]["{{RouterCondition/urlPattern}}"] is the empty string, return false.
+      1. Let |urlPattern| be a result of [=urlpattern/parsing=] |rule|["{{RouterRule/condition}}"]["{{RouterCondition/urlPattern}}"]. If a <code>TypeError</code> is thrown, return false.
+      1. [=list/For each=] |part| of |urlPattern|:
+          1. If |part|'s [=urlpattern-part/type=] is "regexp", return false.
+
+              Note: Since running a user-defined regular expression has a security concern, it is prohibited.
+
+      1. Set |urlPattern| to |rule|["{{RouterRule/condition}}"]["{{RouterCondition/urlPattern}}"]'s associated [=hostname component=].

You need to get the hostname component from somewhere. Right now you have a list of parts (|urlPattern|) and |rule|["{{RouterRule/condition}}"]["{{RouterCondition/urlPattern}}"], which is just a string. I don't know how to get a hostname component from either of those.

> +
+      : Input
+      :: |rule|, a {{RouterRule}}
+      : Output
+      :: a boolean
+
+      1. If |rule|["{{RouterRule/condition}}"]["{{RouterCondition/urlPattern}}"] is the empty string, return false.
+      1. Let |urlPattern| be a result of [=urlpattern/parsing=] |rule|["{{RouterRule/condition}}"]["{{RouterCondition/urlPattern}}"]. If a <code>TypeError</code> is thrown, return false.
+      1. [=list/For each=] |part| of |urlPattern|:
+          1. If |part|'s [=urlpattern-part/type=] is "regexp", return false.
+
+              Note: Since running a user-defined regular expression has a security concern, it is prohibited.
+
+      1. Set |urlPattern| to |rule|["{{RouterRule/condition}}"]["{{RouterCondition/urlPattern}}"]'s associated [=hostname component=].
+      1. Return true.
+  </section>

Probably we should return false if the pattern has any nontrivial components besides hostname, right? Otherwise people might try to use patterns like `/:foo` and not realize that will do nothing.

(I don't know how to do that precisely since, per the above, I don't know how to get from a list of parts and a string to the relevant components.)

-- 
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1686#pullrequestreview-1529781080
You are receiving this because you are subscribed to this thread.

Message ID: <w3c/ServiceWorker/pull/1686/review/1529781080@github.com>