search

owncloud / core

:cloud: ownCloud web server core (Files, DAV, etc.)
https://owncloud.com
GNU Affero General Public License v3.0
8.38k stars 2.06k forks source link

Text fields should trim blanks #33294

Closed brad-richards closed 1 year ago

brad-richards commented 6 years ago

Text fields should trim blanks. When spaces are accidentally entered (as often happens with copy-paste), if they are not trimmed, the resulting error is very difficult for the user to identify.

This may be a general issue. Here is documentation of a specific instance where it occurs. When setting up the SMTP server, so that OwnCloud can send notifications. If the server address has a leading or trailing space, OwnCloud cannot send email.

Steps to reproduce

  1. Go to Settings:Admin:General
  2. In the section "Email server" enter correct information for an SMTP server
  3. Verify that the information works, by clicking "Send email"
  4. In the field "Server address": add a space either at the beginning or the end of the field
  5. Click "Send email" again. Note that it no longer works.

Expected behaviour

Spaces should be trimmed from the start and end of all text fields, before the data is used. In this example, the server address should work even with leading or trailing blanks, because those should be deleted before the data is used.

Actual behaviour

Leading or trailing spaces are retained as part of the server address.

Server configuration

Operating system: Ubuntu 18.04

Web server: Apache 2.4.29

Database: MySQL 5.7.24

PHP version: 7.2

ownCloud version: (see ownCloud admin page) 10.0.10.4

Updated from an older ownCloud or fresh install: Fresh install

Where did you install ownCloud from: Package manager after adding key

Signing status (ownCloud 9.0 and above): No errors have been found.

Remaining items omitted as not relevant

ownclouders commented 6 years ago

GitMate.io thinks possibly related issues are https://github.com/owncloud/core/issues/12174 (blank screen), https://github.com/owncloud/core/issues/3569 (Blank screen), https://github.com/owncloud/core/issues/15176 (Confirm function for text fields), https://github.com/owncloud/core/issues/6113 (Creating a new text file resulting in blank text), and https://github.com/owncloud/core/issues/18820 (Asterisks Fill Blank Password Field in IE8).

phil-davis commented 6 years ago

Agree - I have done similar in other projects.

IMO the place to do it is in the backend that receives and processes the API POST/PUT... calls. For the appropriate fields that should not have space at the front or back, then "be nice to the caller" and trim the crap. That way any client (webUI, android, iOS, desktop sync) gets the fields trimmed the same way and automagically.

Need to be careful for file/folder name fields - I think it is technically valid to have file names that have a space on the end "abc " and that is a different file to "abc"! So don't go overboard and trim those.

mmattel commented 6 years ago

https://en.wikipedia.org/wiki/Filename#Filename_extensions see Comparison of filename limitations NTFS (eg mounted via SMB)

The Win32 API strips trailing space and period (full-stop) characters from filenames, 
except when UNC paths are used.

Linux can do that Windows cant

Interesting situation if Backend mounted is Linux style Frontend is connected via client Windows style ... The result would be, without having that tested, that NTFS in front will do a cut on files recieved and triggers by that an upload to the backend. If there are files only differenciated by the quantity of blanks at the end, this would end up in a mess.

In case we would restrict creation in core we would be safe on the creation side. The thing remains, what to do if you mount a linux style FS like you can do with FTP ...must follow the file naming conventions of the file systems involved in the transfer.

@PVince81 @DeepDiver1975

phil-davis commented 6 years ago

@mmattel yes, in the desktop client repo there has been a lot of discussion about file-system file-name differences. It always causes hassles for people with files on Windows, Max and *nix with different mixes of file names that are not valid on all platforms.

Anyway, for the purposes of this issue, "it is not about trimming file name fields" but about trimming other "general" fields.

mmattel commented 6 years ago

:man_facepalming:

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

phil-davis commented 3 years ago

It would be nice to tidy this sort of thing in various APIs, just to be nice to users.

brad-richards commented 3 years ago

As the original author, just a brief comment: Extending the discussion to the complexity of file names is wrong - this has nothing to do with files, but simply with the input of information to OwnCloud itself. Trimming spaces from things like server names is basic good practice, and could be easily addressed.

phil-davis commented 3 years ago

Yes, IMO the discussion above ended up clarifying that: "Anyway, for the purposes of this issue, "it is not about trimming file name fields" but about trimming other "general" fields."

Anyone can submit a PR to "clean" incoming data.

AlexAndBear commented 3 years ago

This is a lot of work, but quite simple to do, adding junior job label

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

phil-davis commented 2 years ago

Removed the stale label, because this is "a good thing" that could be done some day.

soham-sagade commented 2 years ago

is this resolved? I'd have liked to work here.

phil-davis commented 2 years ago

is this resolved? I'd have liked to work here.

"Anyone can submit a PR to "clean" incoming data." - this job has not been done. Feel free to make a start.

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

mmattel commented 2 years ago

dont close

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

AlexAndBear commented 1 year ago

Unstale

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 1 year ago

This issue has been automatically closed.