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

Do 'triggered by user activation' check earlier #118

Closed
wants to merge 2 commits into from

Conversation

marcoscaceres
Copy link
Member

For normative changes, the following tasks have been completed:

  • Modified Web platform tests (link to pull request)

Implementation commitment:

@marcoscaceres
Copy link
Member Author

Would prefer to do this check earlier in Gecko if possible.

@marcoscaceres
Copy link
Member Author

Safari does the check later, so Gecko will follow Chrome and Safari.

@mgiuca
Copy link
Collaborator

mgiuca commented Sep 16, 2019

IIRC the reason why this check comes after all the TypeErrors is a little dubious but deliberate: it makes it possible to write automated error tests that check for TypeErrors in the WPT suite. All tests that expect us to go past the user activation check have to be manual tests, because they will all fail if automated.

If the user activation check is moved up ahead, all the automated tests will fail with NotAllowedError.

@marcoscaceres
Copy link
Member Author

IIRC the reason why this check comes after all the TypeErrors is a little dubious but deliberate: it makes it possible to write automated error tests that check for TypeErrors in the WPT suite. All tests that expect us to go past the user activation check have to be manual tests, because they will all fail if automated.

This was solved on WTP via Web Driver and .bless() (or just using .click(), which fires trusted events when run under automation).

@marcoscaceres
Copy link
Member Author

This came up again during review on the Gecko side... given that we can deal with this correctly on WTP, @mgiuca would you be willing to change the Blink implementation?

@marcoscaceres marcoscaceres reopened this Sep 24, 2019
@ewilligers
Copy link
Collaborator

Are you also proposing that canShare (if merged) would also require user activation? If not, what is the benefit of moving the check up?

I don't mind changing the order in the Blink implementation if that is the consensus. Any feedback from Safari?

@marcoscaceres
Copy link
Member Author

Are you also proposing that canShare (if merged) would also require user activation?

No. But in the related issue (I think #5), I discussed how to separate the concerns between sharing from canShare.

If not, what is the benefit of moving the check up?

It's just convention in Gecko code and other APIs to do the "triggered by user activation" checks first - before doing all the data checks. So the benefit would be consistency with other APIs (and less Gecko reviewers telling me "move this up" 😂).

@mgiuca
Copy link
Collaborator

mgiuca commented Sep 26, 2019

It doesn't make sense for canShare to require user activation.

I think the reason Eric raises this is that canShare is currently defined in the L2 spec as:

canShare(data) MUST return true unless share(data) would reject with TypeError, in which case it MUST return false.

That definition wouldn't work if we moved the user activation check up. But I think Marcos also objected to that definition (somewhere else) and that we should define it properly by abstracting out the type checking. Which I can get behind.

That aside, it also seems logical to me that the method would do input validation (i.e., checks that are pure mathematical function of the input) before doing extrinsic checks that rely on the state-of-the-world (i.e., checks about user activation).

It's just convention in Gecko code and other APIs to do the "triggered by user activation" checks first - before doing all the data checks.

If that's the case, is it also a convention in other web APIs (specs)? Can you point to other web specs that do user activation testing before input validation? If so, I'd be happy to change it.

@marcoscaceres
Copy link
Member Author

That definition wouldn't work if we moved the user activation check up. But I think Marcos also objected to that definition (somewhere else)

Indeed. I think it was #108.

and that we should define it properly by abstracting out the type checking. Which I can get behind.

That would be great. It means doing less gymnastics in our implementation.

That aside, it also seems logical to me that the method would do input validation (i.e., checks that are pure mathematical function of the input) before doing extrinsic checks that rely on the state-of-the-world (i.e., checks about user activation).

I don't disagree... we kinda do this already:

  1. binding layer does type checks, which, if you squint just right, would be the "pure mathematical function of the input".

But then we diverge on:

  1. Security/input checks, including triggered by user activation, possibly feature policy checks, etc..
  2. Do actual data mutations (e.g., resolve URL).
  3. IPC... and so on....

If that's the case, is it also a convention in other web APIs (specs)? Can you point to other web specs that do user activation testing before input validation? If so, I'd be happy to change it.

Some examples:

Somewhat in the middle:

@mgiuca
Copy link
Collaborator

mgiuca commented Sep 27, 2019

Discussed offline; we are OK to keep it for now (those examples aren't particularly strong since they don't go TypeErrors after the activation check).

@mgiuca mgiuca closed this Sep 27, 2019
@marcoscaceres marcoscaceres deleted the early-check-trigger branch June 12, 2020 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants