Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add limits to the number of router rule registration #1752

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add limits to the number of router rule registration #1752

Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
013e136
Editorial: Add notice on the _or condition depth.
yoshisatoyanagisawa Apr 30, 2024
9f380a1
Define 1024 as a upper limit of total registered router rules
sisidovski Jan 30, 2025
b5a530c
Check router rule recursion depth
sisidovski Jan 30, 2025
b1bdb83
Remove export from the struct
sisidovski Jan 31, 2025
cfe0428
Add error boolean and remove depth from the struct, and simplify algo…
sisidovski Feb 4, 2025
6843296
Merge branch 'main' into limit
sisidovski Feb 5, 2025
645a400
Update docs/index.bs
sisidovski Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions docs/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1602,6 +1602,10 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
};
</pre>

A <dfn id="dfn-count-router-condition-result">count router condition result</dfn> is a [=struct=] that consists of:
* A <dfn id="dfn-count-router-condition-result-condition-count" for="count router condition result">condition count</dfn> (a number).
* A <dfn id="dfn-dfn-count-router-condition-result-quota-exceeded" for="count router condition result">quota exceeded</dfn> (a boolean).

<section>
<h4 id="register-router-method">{{InstallEvent/addRoutes(rules)|event.addRoutes(rules)}}</h4>

Expand Down Expand Up @@ -1629,6 +1633,7 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
1. For each |rule| of |rules|:
1. Append |rule| to |allRules|.

1. If running the [=Check Router Registration Limit=] with |allRules| returns false, reject |promise| with a {{TypeError}}.
1. Set |serviceWorker|'s [=service worker/list of router rules=] to |allRules|.
1. Let |serviceWorkerEventLoop| be the [=current global object=]'s [=event loop=].
1. [=Queue a task=] to run the following steps on |serviceWorkerEventLoop| using the [=DOM manipulation task source=]:
Expand Down Expand Up @@ -3473,6 +3478,51 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
1. Return true.
</section>

<section algorithm>
<h3 id="check-router-registration-limit"><dfn>Check Router Registration Limit</dfn></h3>

: Input
:: |routerRules|, a [=list of router rules=]
: Output
:: a boolean

Note: Router conditions can be complex and nested using {{RouterCondition/_or}} and {{RouterCondition/not}}. To prevent excessive processing, this algorithm introduces two limits. First, the total number of conditions, counting all nested conditions, cannot exceed 1024. Second, the nesting depth is limited to 10 levels to avoid exponential computation.

1. Let |result| be a [=count router condition result=].
1. Set |result|'s [=count router condition result/condition count=] to 1024.
1. Set |result|'s [=count router condition result/quota exceeded=] to false.
1. [=list/For each=] |rule| of |routerRules|:
1. Set |result| to be the result of running [=Count Router Inner Conditions=] with |rule|["{{RouterRule/condition}}"], |result|, and 10.
1. If |result|'s [=count router condition result/quota exceeded=] is true, return false.
1. return true.
sisidovski marked this conversation as resolved.
Show resolved Hide resolved
</section>

<section algorithm>
<h3 id="count-router-inner-conditions"><dfn>Count Router Inner Conditions</dfn></h3>

: Input
:: |condition|, a {{RouterCondition}}
:: |result|, a [=count router condition result=]
:: |depth|, a number
: Output
:: |result|, a [=count router condition result=]

1. Decrement |result|'s [=count router condition result/condition count=] by one.
1. If |result|'s [=count router condition result/condition count=] is zero, or |depth| is zero, then:
1. Set |result|'s [=count router condition result/quota exceeded=] to be true.
1. Return |result|.
1. If |condition|["{{RouterCondition/_or}}"] [=map/exists=], then:
1. Decrement |depth| by one.
1. For each |orCondition| of |condition|["{{RouterCondition/_or}}"]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that the number of conditions under |orCondition| may not be counted?
e.g.
or: [{singleRule}, {singleRule}, {singleRule}]
|result| might be overwritten by the result for {singleRule} for three times, but we want |result|'s [=total count=] to be three.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe those are correctly counted? Let's walk through the current algorithm. btw, under the current design, or: [{singleRule}, {singleRule}, {singleRule}], this total count is expected to be four (or should be counted as one condition).

  1. We call count-router-inner-conditions with the root or condition. The total count will be one.
  2. Starts the for loop with the first {singleRule}.
  3. In the first sub-step, |result|'s total count will be two, because we pass "one" as total count to count-router-inner-conditions in this sub-step. total count will be incremented inside the algorithm.
  4. The second {singleRule} is handled. total count will be three.
  5. The third {singleRule} is handled. total count will be four.

Am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon the sync, I understand how this works.
The last result will be the |currentResult|'s total count, and it will be passed to the coming count-router-inner-conditions. Therefore, the value will be updated like:

  1. |currentResult| is passed to count-router-inner-conditions's argument
  2. in the count-router-inner-conditions, the given argument will be updated
  3. the updated value will be returned
  4. |currentResult|'s total count will be set to the returned result's total count.

Therefore, even if the algorithm does not have add x + y like operation, the count will be updated.
I feel the data flow is difficult to track at once, and hope some notes to explain that as you have already noted below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated to set |result| to be the result of count-router-inner-conditions directly, and now we don't have to set total count explicitly. Hence note is not required anymore. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?
After reviewing this, I got familiar with the algorithm well and can easily read this like so.
However, I am not sure how people first look feel on this. Let me delegate the decision to other reviewers on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm seems pretty intuitive to me now without needing a note. It is clear that result is mutated and the for-loop over all _or conditions means it will be mutated once for each sub-condition.

1. Set |result| to be the result of running [=Count Router Inner Conditions=] with |orCondition|, |result|, and |depth|.
1. If |result|'s [=count router condition result/quota exceeded=] is true, return |result|.
1. Else if |condition|["{{RouterCondition/not}}"] [=map/exists=], then:
1. Decrement |depth| by one.
1. Set |result| to be the result of running [=Count Router Inner Conditions=] with |condition|["{{RouterCondition/not}}"], |result|, and |depth|.
1. If |result|'s [=count router condition result/quota exceeded=] is true, return |result|.
1. Return |result|.
</section>

<section algorithm>
<h3 id="get-router-source-algorithm"><dfn>Get Router Source</dfn></h3>
: Input
Expand Down