Closed Bug 1636855 Opened 4 years ago Closed 4 years ago

paste clipboard ignores maxlength attribute

Categories

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

Product:
Core
Component:
DOM: Editor
Platform:
Desktop
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla79
Tracking Flags:
Tracking Status
firefox-esr68 --- unaffected
firefox76 --- unaffected
firefox77 + verified
firefox78 + verified
firefox79 --- verified

People

(Reporter: jan.pluntke, Assigned: masayuki)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

input_maxlength_withtitle.html
92 bytes, text/html
Details
Bug 1636855 - Disallow to paste longer text than `maxlength` value except in Nightly channel and early Beta
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
jcristau
: approval-mozilla-release-
Details | Review
Bug 1636855 - Enable `editor.truncate_user_pastes` even in Nightly channel r=emilio
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
jcristau
: approval-mozilla-release-
Details | Review
Attached file input_maxlength.html (obsolete) —

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

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.)

Severity: normal → --

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.

Severity: -- → S3
Status: UNCONFIRMED → NEW
status-firefox76: --- → unaffected
status-firefox77: --- → affected
status-firefox78: --- → affected
Component: Untriaged → DOM: Editor
Ever confirmed: true
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → Desktop
Version: 77 Branch → Trunk

This was kind of an intentional change.

Regressed by: 1320229
Has Regression Range: --- → yes

Masayuki, sanketh, thoughts?

Flags: needinfo?(sgmenda)
Flags: needinfo?(masayuki)

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

Flags: needinfo?(sgmenda)

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

Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(masayuki)
Resolution: --- → INVALID

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

Flags: needinfo?(masayuki)

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.)

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.

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

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.

@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.

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.

Are we behaving differently than other browsers?

(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.

Flags: needinfo?(thorsten.dencker)

(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.

Status: RESOLVED → REOPENED
Resolution: INVALID → ---

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?

Flags: needinfo?(miket)

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?

Flags: needinfo?(miket) → needinfo?(mconca)

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.

tracking-firefox77: + → ---
Flags: needinfo?(thorsten.dencker)

(The tracking flag got removed somehow)

tracking-firefox77: --- → ?

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.

Flags: needinfo?(masayuki)

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?

Flags: needinfo?(thorsten.dencker)
Attachment #9147170 - Attachment is obsolete: true

(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.

Severity: S3 → S1
Priority: -- → P1

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

Flags: needinfo?(mconca)

(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.

Flags: needinfo?(mconca)
Assignee: nobody → masayuki
Status: REOPENED → ASSIGNED

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

Pushed by masayuki@d-toybox.com:
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

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago → 4 years ago
status-firefox79: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla79

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
  1. Go to https://www.christophermanning.org/writing/dont-use-maxlength-on-password-inputs
  2. Copy the generated random password in the page.
  3. Paste it into the first <input> element
  4. 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
Attachment #9154795 - Flags: approval-mozilla-release?
Attachment #9154795 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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.

Attachment #9154795 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Are we doing the normandy pref flip as well?

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

Are we doing the normandy pref flip as well?

Flags: needinfo?(pascalc)

(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.

Flags: needinfo?(mconca)

(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.

Flags: needinfo?(pascalc)

(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

(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.

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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

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

QA Whiteboard: [qa-triaged]

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

Flags: needinfo?(thorsten.dencker)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/e9e0c2b6f407
Enable `editor.truncate_user_pastes` even in Nightly channel r=emilio

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:
Attachment #9155166 - Flags: approval-mozilla-release?
Attachment #9155166 - Flags: approval-mozilla-beta?
Attachment #9155166 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

status-firefox77: affected → fixed

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

Status: REOPENED → RESOLVED
Closed: 4 years ago → 4 years ago
Resolution: --- → FIXED

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

status-firefox77: fixed → fix-optional

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.

status-firefox78: fixed → verified
status-firefox79: fixed → verified

(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!

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

status-firefox77: fix-optional → fixed
Attachment #9154795 - Flags: approval-mozilla-release? → approval-mozilla-release-
Attachment #9155166 - Flags: approval-mozilla-release? → approval-mozilla-release-

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

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
status-firefox77: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: