1636855 - paste clipboard ignores maxlength attribute

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Jan Pluntke]

Attachment: input_maxlength.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:77.0) Gecko/20100101 Firefox/77.0

Steps to reproduce:

Firefox 77.0b3 on Windows
webpage with <input type="text" maxlength="3"> (example attached)
paste longer text into this input field

Actual results:

the complete text (more than three characters) was pasted into the input field

Expected results:

pasted text should have been truncated after three characters

Comment 1

[Firefox Bug Husbandry Bot]

Because this bug's Severity is normal and has not been changed, and this bug's priority is -- (none,) indicating it has has not been previously triaged, the bug's Severity is being updated to -- (default, untriaged.)

Comment 2

[Negritas Sergiu, Desktop QA]

Hello! I have managed to reproduce the issue following the STR provided in the description using Firefox Nightly 78.0a1 (2020-05-15) on Ubuntu 18.04 LTS, Windows 10 and MacOS 10.15.

Also I have noticed that only Firefox Nightly and Firefox Beta (77.0b6) are affected and on the Firefox Release (76.0.1) the issue doesen't occur.

I will set a component for this issue so one of our developers could look more into it. If it's not the right component please feel free to change it to an appropriate one.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

This was kind of an intentional change.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Masayuki, sanketh, thoughts?

Comment 5

[sanketh]

Yup, this is not a bug, it is a feature and is consistent with the spec. See Bug 1320229 for why we did this.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, it's intentional change and I believe that the new behavior is better than the traditional one.

Comment 7

[Thorsten Dencker]

Hi Masayuki and Sanketh ,

could you please open this issue again and re-discuss the issue. For SAP this is a showstopper and breaks our customer scenarios including data loss. The customer will not get any information and can't react accordingly.

Old Behavior: maxlength of a inputfield is always considered even in a paste action.
New behaviour: text can be pasted without any warning to the user and ignoring the maxlength attribute

As we didn’t send the data via form request to the backend the user will not get any warning and the long text is transferred. Depending on the application the application will dump and the user will lost all data without any warning.

Please revert this change bevor the next ESR will hit the market and discuss a solution which doesn’t break existing scenarios. Maybe you do it only for password fields and show an messages directly after paste etc.

Thanks
Thorsten Dencker, SAP SE

Comment 8

[sanketh]

As we didn’t send the data via form request to the backend the user will not get any warning and the long text is transferred.

Unless I am missing something, this seems like an easy fix on your end---use client-side form validation.

As we didn’t send the data via form request to the backend the user will not get any warning and the long text is transferred. Depending on the application the application will dump and the user will lost all data without any warning.

This seems like a security issue today since client-side validation does not stop a user from handcrafting a request.

(This is my personal opinion.)

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Ah, so this change is in fact (very) web observable, because of .value... This change landed in 77, so it'd be a bit weird to revert it in 78 just because it's an ESR... Though we have time for it if needed, it's still early in the 78 beta cycle...

I tend to agree with sanketh though... It seems like there are two issues:

• SAP submitting the values without checking input.validity.valid (or without truncating themselves) and not reporting any feedback back to the user.
• The underlying application not doing proper server-side validation.

Though I feel sympathetic about stuff "just working" before... Can you elaborate a bit on what the impact is Thorsten? How hard are those applications to get updated to check input.validity.valid / etc?

CC'ing Mike who takes care of enterprisey stuff.

Comment 10

[Thorsten Dencker]

Hi Sanketh,

even this is might be an easy fix we would have to introduce it only for Firefox. Other vendor doesn’t remove this feature. Did you discuss it in the browser community e.g. TAG review. I’m not sure whether I found all relevant places and did a correct interpretation but at least the whatwg/w3c talks about “must be equal or less”

https://html.spec.whatwg.org/multipage/input.html#attr-input-maxlength
https://www.w3.org/TR/html52/sec-forms.html#the-maxlength-and-minlength-attributes

If the input element has a maximum allowed value length, then the length of the value of the element's value attribute must be equal to or less than the element's maximum allowed value length.

Let’s ignore the fact of the official standards.

It is an incompatible change which breaks many customer scenarios with data loss. Even we provide today a fix all our customer must upgrade their business systems. Some of them running untouched since many years. Upgrading a business critical system needs a long planning period, very high testing effort, money etc and takes usually >= one year. Normally they do it one system after the other.

I believe the current implementation of the fix didn’t reflect all needs and constrains (my very personal opinion). See my statements in the Bug 1320229 about the red border and accessibility requirements.

We can debate how a backend system reacts on wrong user input if they should ignore it, fail etc but the fact it is working this way since many years (around twenty years maxlength is a very old property) and can’t easily be changed. Customers must upgrade their systems which would need a lot of time and money just for a maxlength property.

The client-side validation already exists part of the maxlength property., why should someone rebuild it again with JavaScript?

Thanks for considering my arguments.
Thorsten

Comment 11

[Emilio Cobos Álvarez (:emilio)]

The client-side validation already exists part of the maxlength property., why should someone rebuild it again with JavaScript?

You don't need to rebuild it, there's tons of ways that someone could end up with invalid values in an input which don't yield to auto-correcting the input (like setting .value via JS, typing in a way that doesn't match the pattern of the input, etc...).

We have cross-browser APIs to expose the current validity state of an input (input.validity, input.validity.valid, input.validity.tooLong, pretty much everything) so that you don't need to reimplement stuff in javascript if you're doing your own form submission.

Comment 12

[Thorsten Dencker]

@Emilio

to answer your questions - my answer overlaps your questions:

Question: SAP submitting the values without checking input.validity.valid (or without truncating themselves) and not reporting any feedback back to the user. The underlying application not doing proper server-side validation.

--> this is true but until today not necessary as the maxlength property is doing it. There was a good reason why this property was introduced and used. We can agree the server should not dump but this can't easily be changed as you have to think of we have around >> 100 000 very large business transactions and millions of transaction created by the customers it self. We would have to check/change all applications !!!

Therefore a fix must be done on the client-side but just introducing the maxlength feature again by JavaScript. Checking the value and shorten the paste string in the paste action. But customers need to implement such features and this takes a lot of time as I stated above.

Comment 13

[Thorsten Dencker]

Maybe there are many APIs available but you need to check the paste action. Just try it..it is not the value which will be shorted after the paste action it is the pasted string which will be shorten. But again technically this can be done no doubts...it is the life cycle management of business systems.

Comment 14

[Mike Kaply [:mkaply]]

Are we behaving differently than other browsers?

Comment 15

[sanketh]

(In reply to Mike Kaply [:mkaply] from comment #14)

Are we behaving differently than other browsers?

Yes.

Thorsten: if you are enterprise user, you can revert this behavior by setting the editor.truncate_user_pastes to true for the enterprise.

Comment 16

[Mike Kaply [:mkaply]]

(In reply to sanketh from comment #15)

(In reply to Mike Kaply [:mkaply] from comment #14)

Are we behaving differently than other browsers?

Yes.

Then we should back this out. This change should have gone through the web compatibility team which it did not.

Thorsten: if you are enterprise user, you can revert this behavior by setting the editor.truncate_user_pastes to true for the enterprise.

You're asking this change to be made for every SAP user. That's not feasible.

Comment 17

[Romain Testard [:RT]]

Breaking SAP is a significant issue for enterprise users that could drive churn - this is unfortunately bad timing with ESR78.
Mike can you please confirm if this is a webcompat issue that warrants backing out the change?

Comment 18

[Mike Taylor [:miketaylr]]

I'm not sure if this was something we anticipated being such a large breaking change (mconca can verify or not). But introducing a change like this (that differs from Blink/WebKit) is unfortunate.

Thorsten, can you explain the exact scenario how this can break? How common is it? Do SAP applications encourage users to paste more data than necessary?

Comment 19

[Thorsten Dencker]

Hi Mike and all others,

I definitely agree that the timing is sup-optimal shortly before a new ESR release. It would be easier if such incompatible changes are done shortly after a new ESR release to give enterprise customers more time. At least SAP recommend to use ESR Firefox release.

Could you help me with the question whether it is possible to distribute a parameter like “editor.truncate_user_pastes” centrally to all Firefox instances in enterprise network?

To give a bit more background: The maxlength attribute is widely used as the field value will end up in a database which have usually a character or byte length. If e.g. ten characters are allowed for a product-id in the database the maxlength attribute will be used to enforce it already on the client (like a client side validation).
If more characters are transferred like in the paste scenario it highly depends on the application logic whether it will dump --> all data lost until last database commit or the application is more friendly and just answer with a regular error message. As it is part of the application logic we can’t change the behavior centrally on the backend.

We must solve it on the client-side and enforce the old behavior via Javascript again. Of course this is possible, no doubts, but the life cycle management for companies is the main challenge.

My personal opinion is still this change especially for all inputfields is really risky, even I really understand the password manager issue.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

(The tracking flag got removed somehow)

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I wonder, if we can detect whether the <input maxlength> element in SAP with a special element (meaning it has special attribute value) in the document, it's possible to disable the new behavior only on SAP, like keypress event behavior change.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

New behaviour: text can be pasted without any warning to the user and ignoring the maxlength attribute

I don't understand this part. When pasting too long text, the <input> element must be rendered with red border with a warning message. That's why I considered the change was not so risky. How do SAP kill the warning?

Comment 23

[Jan Pluntke]

Attachment: input_maxlength_withtitle.html

Comment 24

[Jan Pluntke]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #22)

New behaviour: text can be pasted without any warning to the user and ignoring the maxlength attribute

I don't understand this part. When pasting too long text, the <input> element must be rendered with red border with a warning message. That's why I considered the change was not so risky. How do SAP kill the warning?

The red border is only displayed after leaving the input field (e.g. with TAB key). If the user pastes and then performs an action that will submit the data, they will not see it.
And the warning message is not displayed if the input field has a tooltip (title attribute). I have updated the example to show this.

Comment 25

[Mike Conca [:mconca]]

After discussions with a number of people, I'm recommending we back this change out.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Mike Conca [:mconca] from comment #25)

After discussions with a number of people, I'm recommending we back this change out.

Do you think that we should disable it with the pref in all channels? I think that we can enable it in Nightly and early beta for keep improving the issues around the original report.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d72f0aa300272089b55e78a295107cbb434f0e15

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1636855 - Disallow to paste longer text than `maxlength` value except in Nightly channel and early Beta

Fix of bug 1320229 allowed to paste longer text than maxlength attribute of
<input> and <textarea> because it's was thought that the longer text causes
"too long" invalidate state and makes it notify users and prevent to submit
form data.

However, according to Bug 1636855 comment 7 (*1), it breaks a major enterprise
web app, SAP, at least and it sends form data without checking validity of
each form data and discards invalid data on server side silently.

According to bug 1638855 comment 24 (*2), one of new behavior's fault is
on Gecko side too. The style of <input> element or <textarea> element
which has too long text after pasting is changed when it loses a focus.
Therefore, users can post the data before they know pasted data is too
long if sending the form data with Enter key or something immediately
after pasting (i.e., without moving focus).

On the other hand, the original bug report, bug 1320229 should be solved in
the future especially in password field because user may register password
which is cut by maxlength unexpectedly and they don't use builtin password
manager, only the pasted password is saved, and then, they won't be able to
login as the account. This is really long standing issue of the web forms.
An article (*3) warned this to web developers in 2011. Therefore, we should
keep going advance for solving this issue at least in Nightly channel to get
more feedback from testers and web developers.

1 https://bugzilla.mozilla.org/show_bug.cgi?id=1636855#c7
2 https://bugzilla.mozilla.org/show_bug.cgi?id=1636855#c24
3 https://www.christophermanning.org/writing/dont-use-maxlength-on-password-inputs

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de21e1cf9a9b
Disallow to paste longer text than `maxlength` value except in Nightly channel and early Beta r=emilio

Comment 30

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/de21e1cf9a9b

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9154795 [details]
Bug 1636855 - Disallow to paste longer text than maxlength value except in Nightly channel and early Beta

Beta/Release Uplift Approval Request

• User impact if declined: SAP users may get lost their data silently if user pasts longer text than maxlength value.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Check editor.truncate_user_pastes pref

2. Go to https://www.christophermanning.org/writing/dont-use-maxlength-on-password-inputs
3. Copy the generated random password in the page.
4. Paste it into the first <input> element
5. Move focus

If the pref is false, you'll see green text which let you know success to paste it.
If the pref is true, you'll see red text which let you know pasting password was truncated by maxlength.

On Nightly and early Beta, the default value should be false, but late Beta and Release channel, it should be true by default.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just taking it back the traditional behavior on late Beta and Release.
• String changes made/needed: No

Comment 32

[Julien Cristau [:jcristau]]

Comment on attachment 9154795 [details]
Bug 1636855 - Disallow to paste longer text than maxlength value except in Nightly channel and early Beta

I'm not sure we should keep this enabled at all, until we have a plan to ship without webcompat fallout, but approved for 78.0b5.

Comment 33

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d1ed81fa7533

Comment 34

[Mike Kaply [:mkaply]]

Are we doing the normandy pref flip as well?

Comment 35

[Julien Cristau [:jcristau]]

(In reply to Mike Kaply [:mkaply] from comment #34)

Are we doing the normandy pref flip as well?

Comment 36

[Mike Conca [:mconca]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #26)

(In reply to Mike Conca [:mconca] from comment #25)

After discussions with a number of people, I'm recommending we back this change out.

Do you think that we should disable it with the pref in all channels? I think that we can enable it in Nightly and early beta for keep improving the issues around the original report.

We know that this causes issues for certain sites and won't be shipping it in its current form, so I don't see why we'd want to keep this in nightly and early beta. Until we have a comprehensive solution, I think it makes sense to back it out of all channels.

Comment 37

[Pascal Chevrel:pascalc]

(In reply to Mike Kaply [:mkaply] from comment #34)

Are we doing the normandy pref flip as well?

I think we should as this would probably be the fastest way to solve the issue for users before having a dot release including this fix.

Comment 38

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Pascal Chevrel:pascalc from comment #37)

(In reply to Mike Kaply [:mkaply] from comment #34)

Are we doing the normandy pref flip as well?

I think we should as this would probably be the fastest way to solve the issue for users before having a dot release including this fix.

Normandy request filed https://bugzilla.mozilla.org/show_bug.cgi?id=1644263

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Mike Conca [:mconca] from comment #36)

We know that this causes issues for certain sites and won't be shipping it in its current form, so I don't see why we'd want to keep this in nightly and early beta. Until we have a comprehensive solution, I think it makes sense to back it out of all channels.

The patch was landed end of April and actual case of existing web apps is reported just 4 days ago and that's only with SAP. I.e., over a month, both Nightly testers and Beta testers didn't meet actual problem with this change. But I bet SAP is not the only one web app broken by the change in the world. So, I think:

• most users testers got only benefit for the long standing issue
• but there may be some unkown breakage

So, I believe that it's better to collect the broken cases.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Mozilla considered that it should be disabled even in Nightly channel. I'll post a follow up patch.

Comment 41

[Emilio Cobos Álvarez (:emilio)]

FWIW, I tend to agree with masayuki here...

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&selectedTaskRun=QcJwDaqET_OXZufwyNM2Yg-0&revision=cd1654f75f7128c9e620ad89283a282bb2877900

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1636855 - Enable `editor.truncate_user_pastes` even in Nightly channel r=emilio

Mozilla consider that we should protect even Nightly testers from the behavior
change of bug 1320229.

Comment 44

[Thorsten Dencker]

Hi Team,
I just want to say thank you for reacting so fast and removing the new feature again.
What is the best place / discussion forum to track the discussion around the maxlength? We are always willing to change/improve our coding but need in advance a kind of spec in best case agreed with other vendors and especially enough time for business customers to implement the changes in the system.
Thorsten

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9e0c2b6f407
Enable `editor.truncate_user_pastes` even in Nightly channel r=emilio

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9155166 [details]
Bug 1636855 - Enable editor.truncate_user_pastes even in Nightly channel r=emilio

Beta/Release Uplift Approval Request

• User impact if declined: See previous patch's uplift request for the detail. This patch includes a fix of mochitest which will become orange when beta becomes not early beta.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Same as the previous one.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just disabling the new feature.
• String changes made/needed:

Comment 47

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/12f4e93abed2

Comment 48

[Pascal Chevrel:pascalc]

Fixed on 77 with the rollout of a pref change via Normandy in bug 1644263

Comment 49

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/e9e0c2b6f407

Comment 50

[Pascal Chevrel:pascalc]

marking 77 as fix-optional in case we have a dot release and include the patch to replace the Normandy rollout.

Comment 51

[Alin Ilea, Desktop Test Engineering]

Reproduced the initial issue with an old Nightly build 78.0a1 (2020-05-15) using Windows 10.
Verified - Fixed in latest Beta 78.0b5 (build id: 20200609165924) and latest Nightly 79.0a1 (build id: 20200610043607) using Windows 10 and Ubuntu 18.04. The pref is true by default in both channels mentioned above.

Comment 52

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Thorsten Dencker from comment #44)

Hi Team,
I just want to say thank you for reacting so fast and removing the new feature again.
What is the best place / discussion forum to track the discussion around the maxlength? We are always willing to change/improve our coding but need in advance a kind of spec in best case agreed with other vendors and especially enough time for business customers to implement the changes in the system.
Thorsten

Hi!
I filed a new bug https://bugzilla.mozilla.org/show_bug.cgi?id=1645012 for the further discussion. We also appreciate your feedback and use case. Let's continue discussion there. Thank you!

Comment 53

[Julien Cristau [:jcristau]]

As we're entering 78 rc week, this won't be uplifted for a 77 dot release anymore.

Comment 54

[Alin Ilea, Desktop Test Engineering]

Verified - Fixed in Release 77 with the rollout of a pref change via Normandy in bug 1644263.