Intent to Implement and Ship: Do not use web page provided strings for unbeforeunload

5 249 wyświetleń
Przejdź do pierwszej nieodczytanej wiadomości

Avi Drissman

nieprzeczytany,
18 lut 2016, 18:21:3418.02.2016
do blink-dev

Contact emails

a...@chromium.org


Spec

The relevant link is https://html.spec.whatwg.org/#prompt-to-unload-a-document .


Summary

http://crbug.com/587940


onbeforeunload dialogs are used for two things on the Modern Web:

1. Preventing users from inadvertently losing data.

2. Scamming users.


In an attempt to restrict their use for the latter while not stopping the former, we are going to not display the string provided by the web page. Instead, we are going to use a generic string.


Firefox already does this (see the attachment on the bug).


This does not violate the spec. Per https://html.spec.whatwg.org/#prompt-to-unload-a-document, step 7:


"The prompt shown by the user agent may include the string of the returnValue attribute, or some leading subset thereof. (A user agent may want to truncate the string to 1024 characters for display, for instance.)"


The prompt MAY include the string. We would no longer do so.


Motivation

Users are being harmed by bad actors. This mitigates much of the abuse of the API by bad actors while preserving the data-saving aspects that this API can be used for.


Interoperability and Compatibility Risk

Firefox already does this, https://bugzilla.mozilla.org/show_bug.cgi?id=641509 . We would be following them.


Ongoing technical constraints

None.


Will this feature be supported on all six Blink platforms (Windows, Mac, Linux, Chrome OS, Android, and Android WebView)?

Yes.


OWP launch tracking bug

http://crbug.com/587940


Link to entry on the feature dashboard

This is a change to existing behavior that remains spec-compliant. I don't believe it requires a feature dashboard entry.


Requesting approval to ship?

Yes.

ben.m...@gmail.com

nieprzeczytany,
18 lut 2016, 18:50:5118.02.2016
do blink-dev
It'd be sad to see this replaced with more genetic and less useful messages. For example, on Facebook the dialog can tell you that there's a partially typed chat message, comment, etc. Right now the UX of this dialog sucks to the one we create when the navigation is internal to our page (and thus we can render the dialog in HTML).

ben.m...@gmail.com

nieprzeczytany,
18 lut 2016, 18:57:5818.02.2016
do blink-dev, ben.m...@gmail.com
As a more detailed example of where this could cause user confusion -- right now on Facebook if you send a chat message we instantly echo that message back into the chat window even before the server has acknowledged it's receipt of the message. We'll warn the user before they leave the page unless the server sends us an ACK. It would be completely unclear to a user why they are getting the dialog unless the dialog is able to say "your message has not completed sending".

Granted, I can't say that we've gotten a huge number of complaints about this from firefox users. But it's a user experience that's not great.

Kenji Baheux

nieprzeczytany,
18 lut 2016, 19:00:2618.02.2016
do ben.m...@gmail.com, blink-dev, Owen Campbell-Moore
Wouldn't this be a use case cut for the Background Sync API?

David Benjamin

nieprzeczytany,
18 lut 2016, 19:08:2418.02.2016
do Kenji Baheux, ben.m...@gmail.com, blink-dev, Owen Campbell-Moore
Rather than show that prompt, would it not be better UX to save that partially typed chat message or comment and restore it later? For a message the server hasn't yet ACK'd, it seems you could communicate most of that to the user by echoing the message in some kind of grayed out in-progress state and only solidifying it when ACK'd. You can also save the message locally and, as Kenji noted, there are APIs like Background Sync or maybe even sendBeacon which can let you deliver your message.

For the substitute message, Firefox uses "This page is asking you to confirm that you want to leave - data you have entered may not be saved" which seems suitable enough for both your uses. Presumably the user just typed or sent that message, so they'll usually know what it is. In comparison to other APIs, this prompt is maximally abusive with negligible legitimate uses, so I think a slight loss of fidelity for the semi-legitimate cases is worth stamping out the abuse.

One of the hallmarks of the web's security model is that if a page is being annoying, users can just go elsewhere or close the tab. It lets us give you lots of APIs without having to make them 100% abuse-free because users always have that safety hatch. And yet here is an API whose sole purpose is to break that.

David

Elliott Sprehn

nieprzeczytany,
18 lut 2016, 19:12:1718.02.2016
do Avi Drissman, blink-dev
beforeunload fires on 9.1849% of pages, that's *massive*. I think we need to understand the breakage this causes first. What pages break? What content are we hiding? Does this make someone's banking or health care experience broken?

I don't buy the argument that beforeunload is broken on mobile, there's billions of pages and users on desktop, and that's where beforeunload is clearly very popular with a percentage like 9%.

The web is full of terrible APIs that harm the user experience, you can't just remove them though.

David Benjamin

nieprzeczytany,
18 lut 2016, 19:17:3918.02.2016
do Elliott Sprehn, Avi Drissman, blink-dev
Anecdotally, a lot of folks (incorrectly) think that beforeunload and unload are interchangeable for non-prompting cleanup code, so I suspect most of that 9.18% isn't actually this case, but it would be good to get numbers whether the prompt is actually shown. (Although that too isn't quite right because one might register beforeunload, with the intent to prompt, but only fire the prompt in rare cases. Ideally we'd include that but exclude the sites using it to fire some analytics XHR.)

This isn't proposing to remove the prompt though. It's only proposing to ignore the string in place of a fairly generic one, something Firefox already does.

ben.m...@gmail.com

nieprzeczytany,
18 lut 2016, 19:20:2118.02.2016
do blink-dev, kenji...@chromium.org, ben.m...@gmail.com, owe...@google.com


On Thursday, February 18, 2016 at 4:08:24 PM UTC-8, David Benjamin wrote:
Rather than show that prompt, would it not be better UX to save that partially typed chat message or comment and restore it later?

We actually do this on mobile. It's hard to get the experience exactly correct -- for example, if you send every message grayed out until it is delivered it makes the latency noticeable and the interface feel less instant. Getting this right is a fairly complex exercise -- and it's something that many sites would probably never bother to implement.

For the substitute message, Firefox uses "This page is asking you to confirm that you want to leave - data you have entered may not be saved" which seems suitable enough for both your uses. Presumably the user just typed or sent that message, so they'll usually know what it is. In comparison to other APIs, this prompt is maximally abusive with negligible legitimate uses, so I think a slight loss of fidelity for the semi-legitimate cases is worth stamping out the abuse.

I guess my question here is if there's a way to display the site's message so that it's fairly clear the message is coming from the site, not from chrome. 

Brett Wilson

nieprzeczytany,
18 lut 2016, 19:21:1718.02.2016
do David Benjamin, Elliott Sprehn, Avi Drissman, blink-dev
On Thu, Feb 18, 2016 at 4:17 PM, David Benjamin <davi...@chromium.org> wrote:
Anecdotally, a lot of folks (incorrectly) think that beforeunload and unload are interchangeable for non-prompting cleanup code, so I suspect most of that 9.18% isn't actually this case, but it would be good to get numbers whether the prompt is actually shown. (Although that too isn't quite right because one might register beforeunload, with the intent to prompt, but only fire the prompt in rare cases. Ideally we'd include that but exclude the sites using it to fire some analytics XHR.)

This isn't proposing to remove the prompt though. It's only proposing to ignore the string in place of a fairly generic one, something Firefox already does.

Yes, I think following Firefox and prompting with a generic "unsaved content" string seems very low risk in terms of actual compatibility.

Brett

Jochen Eisinger

nieprzeczytany,
19 lut 2016, 03:18:0519.02.2016
do Brett Wilson, David Benjamin, Elliott Sprehn, Avi Drissman, blink-dev
lgtm1 to ship

The functionality pages are relying upon is preserved by this change, and it brings us in line with what firefox does.

PhistucK

nieprzeczytany,
19 lut 2016, 07:06:3819.02.2016
do ben.m...@gmail.com, blink-dev, kenji...@chromium.org, Owen Campbell-Moore
If I remember correctly, when the message is shown, the tab must be focused so the users sees it. Is it possible to display an HTML message in the background of the beforeunload prompt (you set it up within the event listener), or will it not be shown due to the synchronicity of things? If it does display, most desktop users will see it, I believe. Granted, it is a little clumsy, but it should accomplish your goal.


PhistucK

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

PhistucK

nieprzeczytany,
19 lut 2016, 07:09:0819.02.2016
do ben.m...@gmail.com, blink-dev, kenji...@chromium.org, Owen Campbell-Moore


PhistucK

Chris Palmer

nieprzeczytany,
22 lut 2016, 17:08:4922.02.2016
do blink-dev, bre...@chromium.org, davi...@chromium.org, esp...@chromium.org, a...@chromium.org
+1 from me for the same reasons.

Owen Campbell-Moore

nieprzeczytany,
22 lut 2016, 17:27:5022.02.2016
do PhistucK, ben.m...@gmail.com, blink-dev, kenji...@chromium.org
Ben - I think Kenji is right that this problem can be solved well with Background Sync, as can all use cases for "don't close this page or you'll lose content" so far as I'm aware

Here's an explainer.

Ben - I know Nate S is basically up to speed with background sync so you should be able to chat with him about it, but feel free to reach out otherwise if you have questions.

Avi Drissman

nieprzeczytany,
22 lut 2016, 17:36:0822.02.2016
do Owen Campbell-Moore, PhistucK, ben.m...@gmail.com, blink-dev, kenji...@chromium.org
I'd like to focus this thread on the Intent, that we should only use our own string for onbeforeunload dialogs. If you have other technical discussions, please move them to their own threads.

I have one LG, but am not sure how to gain more. API_OWNERS, what are your concerns?

Avi

TAMURA, Kent

nieprzeczytany,
24 lut 2016, 00:03:2024.02.2016
do Avi Drissman, Owen Campbell-Moore, PhistucK, ben.m...@gmail.com, blink-dev, kenji...@chromium.org
LGTM2.
Following Firefox behavior is safe.

--
TAMURA Kent
Software Engineer, Google


Philip Jägenstedt

nieprzeczytany,
24 lut 2016, 06:16:3024.02.2016
do TAMURA, Kent, Avi Drissman, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
LGTM3

I think we should expect some pushback on this based on the kinds of concerns raised by Ben Maurer. Background Sync sounds like a great solution, but I wouldn't be surprised to hear that it takes some time to implement, and complaints about the removal being too quick. Unfortunately a deprecation message in beforeunload is very likely to be seen, so that probably wouldn't help.

If there's pushback once this starts trickling through dev and beta, can you report back on this thread?

Elliott Sprehn

nieprzeczytany,
24 lut 2016, 11:25:3724.02.2016
do Philip Jägenstedt, Owen Campbell-Moore, Avi Drissman, kenji...@chromium.org, TAMURA, Kent, PhistucK, blink-dev, Ben Maurer

Can we log the message at event registration time?

Philip Jägenstedt

nieprzeczytany,
24 lut 2016, 11:28:2824.02.2016
do Elliott Sprehn, Owen Campbell-Moore, Avi Drissman, kenji...@chromium.org, TAMURA, Kent, PhistucK, blink-dev, Ben Maurer
That would work, but perhaps a bit too well, almost 10% of page loads would see that message:

Avi Drissman

nieprzeczytany,
24 lut 2016, 11:49:4924.02.2016
do Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
I'm happy to report back, but I anticipate that people will not like it. See, e.g.:


UX wants to write the string, but my contact is out until next Monday so the CL will have to wait until then.

I will keep this thread updated.

Avi

Rick Byers

nieprzeczytany,
24 lut 2016, 12:00:2624.02.2016
do Avi Drissman, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
Yeah I don't think it's a good idea to warn on usage of onbeforeunload events, we don't have any concrete plan to remove that event entirely.  There's also not going to be any way to feature detect this, so we can't exactly argue that you should NEVER return a string (since if the UA supports showing it, it's reasonable for example for Facebook to provide one).

So I don't see any better alternative than:
1) publishing an article / sample for how to use background sync to replace the need for unload blocking and discourage use of unbeforeunload when BG sync is available
2) use a well-written but hard-coded message (maybe pre-flighted with interested developers like Ben from Facebook).

Rick

Avi Drissman

nieprzeczytany,
24 lut 2016, 13:48:3524.02.2016
do Rick Byers, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
I have a question.

In my CL to do this, I got some pushback that I wasn't ripping out enough infrastructure, and my initial response was that I wanted to preserve the ability for embedders to make the choice themselves.

While it looks like I have approval, it seems like I got more approval than I was intending to ask for. I intended to get approval to do this for Chrome/Chromium, but it seems like people are OK with doing for every Blink/content embedder.

Is that the case? Should I remove the ability to specify the message all the way down? Or just remove it from Chromium and let other embedders decide?

Avi

Elliott Sprehn

nieprzeczytany,
24 lut 2016, 14:08:0124.02.2016
do Avi Drissman, Rick Byers, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
In general we don't leave things in for "other embedders", blink is the rendering engine for Chrome and is not a general purpose rendering engine you can use without the rest of our stack. ex. we're tightly coupled to net, cc, mojo, and make no attempts to allow using another system. We have certainly made exceptions, for example leaving in spatial navigation for Opera.

This needs to be decided on a per feature basis, but the general guideline to assume that blink is not some generic system holds. For example in the case of alert(), if we plan to truncate it to something as short as 100 where we might end up truncating very often we should definitely do it in blink as you're changing the "normal" operation of the web, and not just some exceptional case. That needs to be logged to the console so authors know what's up. :)

I'm also skeptical of leaving in the beforeunload string feature. If Chrome doesn't support it, and Firefox doesn't support it, it's not a big jump to where Edge doesn't support it and at that point we've change the shape of the web and what authors are willing to do in such a way that the "standard" is to not show the message. At that point there's no sense in keeping some vestigial web feature the majority of other browsers (and Chrome) don't support, why would authors use it?. That said this feature is also low cost on our binary, so we can leave it for now.

Rick Byers

nieprzeczytany,
24 lut 2016, 14:12:5624.02.2016
do Elliott Sprehn, Avi Drissman, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
Also the default assumption should be that the decision of the blink community represents the consensus of the embedders unless we've heard otherwise.  Did someone explicitly ask for this to be left in?  If not (and if no other embedder speaks up now), then yeah don't leave dead code around for no concrete reason.

Rick

Marshall Greenblatt

nieprzeczytany,
24 lut 2016, 14:25:2524.02.2016
do Avi Drissman, Rick Byers, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
On Wed, Feb 24, 2016 at 1:48 PM, Avi Drissman <a...@chromium.org> wrote:
I have a question.

In my CL to do this, I got some pushback that I wasn't ripping out enough infrastructure, and my initial response was that I wanted to preserve the ability for embedders to make the choice themselves.

While it looks like I have approval, it seems like I got more approval than I was intending to ask for. I intended to get approval to do this for Chrome/Chromium, but it seems like people are OK with doing for every Blink/content embedder.

Is that the case? Should I remove the ability to specify the message all the way down? Or just remove it from Chromium and let other embedders decide?

As a Content API consumer I would like to continue receiving the message string in content::JavaScriptDialogManager::RunBeforeUnloadDialog. CEF has been exposing that value for a while and applications might be depending on it. However, I don't personally feel so strongly about it that I would want Chromium/Blink to keep the code around if the maintenance cost is a concern.

Avi Drissman

nieprzeczytany,
24 lut 2016, 16:31:0624.02.2016
do Elliott Sprehn, Rick Byers, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
On Wed, Feb 24, 2016 at 2:07 PM, Elliott Sprehn <esp...@chromium.org> wrote:
In general we don't leave things in for "other embedders", blink is the rendering engine for Chrome and is not a general purpose rendering engine you can use without the rest of our stack.

My apologies, I allowed a terminology conflict.

In Chromium-land, "embedder" means an embedder of content, not an embedder of blink. My question is whether we want to allow other embedders of content/ to make independent decisions.

I uploaded a new patch set on https://codereview.chromium.org/1714573002/ that completely removes the message from Blink on up. Compare patch set 2 with patch set 3. Either one is fine with me.

Avi

Daniel Cheng

nieprzeczytany,
24 lut 2016, 16:54:2924.02.2016
do Avi Drissman, Elliott Sprehn, Rick Byers, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
Personally, if we've decided Blink doesn't want to support this, I'd rather see it go completely. One particularly unfortunate example is NPAPI… for Reasons, it's not yet completely removed, and there are Chromium forks out there that intentionally re-enable it, despite being pretty much completely unsupported at this point.

Daniel

Chris Harrelson

nieprzeczytany,
24 lut 2016, 18:57:0124.02.2016
do Daniel Cheng, Avi Drissman, Elliott Sprehn, Rick Byers, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
+1
 

Daniel

Avi Drissman

nieprzeczytany,
16 mar 2016, 16:20:5016.03.2016
do Chris Harrelson, Daniel Cheng, Elliott Sprehn, Rick Byers, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
An update on onbeforeunload strings:

This change was committed with https://codereview.chromium.org/1714573002/ at r380755, and will likely ship in M51.

I was pointed to Safari 9.1's release notes which notes that Safari is doing this too. This means that this behavior will be in Safari >= 9.1, Firefox >= 4, Chrome >= 51, and Opera >= ? (I'm afraid I don't know the Opera version mappings), so this appears to be the new de-facto standard for handling this.

Because this is user-facing, I made an entry on chromestatus for it. I've not done that before, and it was a little hard to fit into the given slots, but I did my best. Feel free to tweak the entry if it can be improved.

Thank you to everyone involved.

Avi

Rick Byers

nieprzeczytany,
16 mar 2016, 16:47:3516.03.2016
do Avi Drissman, Chris Harrelson, Daniel Cheng, Elliott Sprehn, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
Thanks for the update, and for pushing this forward!

The chromestatus entry looks fine to me, we can leave the polish/wordsmithing to the M51 blog post :-)

Fredrik Söderquist

nieprzeczytany,
16 mar 2016, 17:31:5416.03.2016
do Avi Drissman, Chris Harrelson, Daniel Cheng, Elliott Sprehn, Rick Byers, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
On Wed, Mar 16, 2016 at 9:20 PM, Avi Drissman <a...@chromium.org> wrote:
An update on onbeforeunload strings:

This change was committed with https://codereview.chromium.org/1714573002/ at r380755, and will likely ship in M51.

I was pointed to Safari 9.1's release notes which notes that Safari is doing this too. This means that this behavior will be in Safari >= 9.1, Firefox >= 4, Chrome >= 51, and Opera >= ? (I'm afraid I don't know the Opera version mappings), so this appears to be the new de-facto standard for handling this.
 
Chromestatus knows =), but in case it's worth mentioning here too: Opera >= 38


/fs

Avi Drissman

nieprzeczytany,
16 mar 2016, 17:36:0116.03.2016
do Fredrik Söderquist, Chris Harrelson, Daniel Cheng, Elliott Sprehn, Rick Byers, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
I put in 38 as a guess. I guessed well, then :)

Thanks!

Avi Drissman

nieprzeczytany,
21 mar 2016, 22:04:0521.03.2016
do Fredrik Söderquist, Chris Harrelson, Daniel Cheng, Elliott Sprehn, Rick Byers, Philip Jägenstedt, TAMURA, Kent, Owen Campbell-Moore, PhistucK, Ben Maurer, blink-dev, kenji...@chromium.org
As an additional note, Apple shipped Safari 9.1 today. Among its security fixes it no longer uses the onbeforeunload string. Based on previous announcements by them we were expecting that. What surprised me there is that they assigned it a CVE:

Available for: OS X Mavericks v10.9.5, OS X Yosemite v10.10.5, OS X El Capitan v10.11 to v10.11.3
Impact: Visiting a malicious website may lead to user interface spoofing
Description: An issue existed where the text of a dialog included page-supplied text. This issue was addressed by no longer including that text.
CVE-ID
CVE-2009-2197 : Alexios Fakos of n.runs AG

I'm pretty happy with our choice here. Will update chromestatus once I manually verify.

Avi

roleba...@gmail.com

nieprzeczytany,
17 lip 2016, 10:04:5717.07.2016
do blink-dev
I seems to me that whole idea is to fix the security issue as fast as possible.  Browser manufacturers can decide between two types of efforts:

1. Removing the custom string: about 1 day effort
2. Making the custom string completely harmles: about 3 days effort

On the long term there are more then enough reasons to keep a custom string while being safe for the user. I hope the string to came back in future.


Odpowiedz wszystkim
Odpowiedz autorowi
Przekaż
Nowe wiadomości: 0