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.

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

Address limits on unconstrained inputs #520

Merged
merged 7 commits into from
Jan 16, 2023
Merged

Address limits on unconstrained inputs #520

merged 7 commits into from
Jan 16, 2023

Conversation

annevk
Copy link
Member

@annevk annevk commented Jan 9, 2023

The second half of this paragraph is from the HTML Standard.


Preview | Diff

The second half of this paragraph is from the HTML Standard.
@annevk annevk requested a review from domenic January 9, 2023 11:27
@annevk
Copy link
Member Author

annevk commented Jan 9, 2023

@jyasskin care to review as well?

annevk added a commit to whatwg/html that referenced this pull request Jan 9, 2023
infra.bs Outdated Show resolved Hide resolved
infra.bs Outdated Show resolved Hide resolved
@jyasskin
Copy link
Member

Oh, and even if we discourage exact limits, I think we should allow minimum sizes on the implementation-defined limits, like https://httpwg.org/specs/rfc9110.html#uri.references recommends for URIs.

@annevk
Copy link
Member Author

annevk commented Jan 10, 2023

@smaug---- @martinthomson care to comment on behalf of Firefox as suggested above?

infra.bs Show resolved Hide resolved
infra.bs Outdated Show resolved Hide resolved
@annevk annevk requested a review from domenic January 10, 2023 18:01
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with nits

infra.bs Outdated Show resolved Hide resolved
infra.bs Outdated Show resolved Hide resolved
annevk and others added 2 commits January 11, 2023 10:02
Co-authored-by: Domenic Denicola <d@domenic.me>
@annevk
Copy link
Member Author

annevk commented Jan 11, 2023

Anyone want to do another pass? I'll merge this Monday if there's no further feedback.

infra.bs Outdated Show resolved Hide resolved
Copy link
Member

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

2 minor suggestions:


<p tracking-vector>Nevertheless, user agents may impose <a>implementation-defined</a> limits on
otherwise unconstrained inputs. E.g., to prevent denial of service attacks, to guard against running
out of memory, or to work around platform-specific limitations.
Copy link
Member

Choose a reason for hiding this comment

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

Inspired by #520 (comment):

Suggested change
out of memory, or to work around platform-specific limitations.
out of memory, or to work around platform-specific limitations. Specifications should define what
happens when input exceeds such an <a>implementation-defined</a> limit.

I'm not sure that's exactly the right wording or location.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we can require this. At the very least it should be a follow-up discussion. E.g., consider out-of-memory. Currently implementations can impose limits and throw an exception or they can let the entire process crash. We wouldn't want specifications to constrain that.

Copy link
Member

Choose a reason for hiding this comment

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

It's already "should" rather than "must", but maybe with "if doing so is reasonable"? I agree that we won't be able to do it in all cases, but I wanted to encourage people to try. Even in the case of memory limits, something should specify the type of error thrown if one is thrown.

I have no problem making that a follow up discussion.

Copy link
Member Author

Choose a reason for hiding this comment

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

#523.

infra.bs Outdated
<p>There are at least two cases where a limit can be useful to define:

<ul class=brief>
<li>Ensuring all implementations can handle inputs of a given minimum size, i.e., a lower limit.
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit awkward. Maybe:

Suggested change
<li>Ensuring all implementations can handle inputs of a given minimum size, i.e., a lower limit.
<li>Constraining an <a>implementation-defined</a> limit to be at least a particular minimum size.

Even this wording isn't really a case "where a limit can be useful to define", so maybe it should move out of this list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not? Isn't a lower limit a limit?

Copy link
Member

Choose a reason for hiding this comment

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

It's a limit on the limits implementations impose (a meta-limit?), while the rest of this section is about specifying limits on user code. So it's a bit of equivocation to use the same word, but I also don't object strongly. Do what you like after having heard the argument. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, reworded.

@annevk annevk merged commit c6a1cc3 into main Jan 16, 2023
@annevk annevk deleted the annevk/limits branch January 16, 2023 09:04
annevk added a commit to whatwg/html that referenced this pull request Jan 16, 2023
noamr pushed a commit to noamr/html that referenced this pull request Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants