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

Text fields should trim blanks #33294

Closed
brad-richards opened this issue Oct 25, 2018 · 20 comments · May be fixed by #40950
Closed

Text fields should trim blanks #33294

brad-richards opened this issue Oct 25, 2018 · 20 comments · May be fixed by #40950

Comments

@brad-richards
Copy link

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
Copy link
Contributor

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

@phil-davis
Copy link
Contributor

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
Copy link
Contributor

mmattel commented Oct 25, 2018

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
Copy link
Contributor

@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
Copy link
Contributor

mmattel commented Oct 25, 2018

🤦‍♂️

@PVince81 PVince81 added this to the backlog milestone Oct 31, 2018
@stale
Copy link

stale bot commented Sep 20, 2021

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.

@stale stale bot added the status/STALE label Sep 20, 2021
@phil-davis
Copy link
Contributor

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

@stale stale bot removed the status/STALE label Sep 20, 2021
@brad-richards
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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

@github-actions
Copy link

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
Copy link
Contributor

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

@soham-sagade
Copy link

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

@phil-davis
Copy link
Contributor

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
Copy link

github-actions bot commented Oct 8, 2022

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
Copy link
Contributor

mmattel commented Oct 8, 2022

dont close

@github-actions
Copy link

github-actions bot commented Apr 7, 2023

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
Copy link
Contributor

Unstale

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

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
Copy link

This issue has been automatically closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants