OpenArchive / Save-app-android-old

This is the Save app for Android
https://open-archive.org
GNU General Public License v3.0
94 stars 26 forks source link

Using special characters ("/") in new folder name breaks the upload #524

Closed losalim closed 4 months ago

losalim commented 8 months ago

Describe the bug Created a new folder on Nextcloud titled "ls/nc test_Nov_23". The folder did not get created in Nextcloud, and it doesn't let me upload photos to this folder. There is no error that says invalid folder name. I think the character "/" is not allowed in folder names?

Environment (please complete the following information):

losalim commented 8 months ago

You may know this already @tladesignz sorry! Update: Natalie and Lauren tested the following "/" and "\" both don't work in folder names.
We've tested other characters, including ~, ^, :, <,>, !, *, #, =, ., +, $,%, (,), ", ?, [], {}, and @, which are working.

ryjen commented 4 months ago

as a bonus, the fix addresses the concern mentioned by @uniqx: https://github.com/OpenArchive/Save-app-android/issues/516#issuecomment-1789685685

tladesignz commented 4 months ago

You may know this already @tladesignz sorry! Update: Natalie and Lauren tested the following "/" and "" both don't work in folder names.

Oh, hell, yeah. The exact list of special characters in file system names varies slightly, but the forward slash "/" definitely is a no go. This is a typical case of something so well known for a programmer, that we wouldn't even think about trying to do that.

We should coordinate with the iOS app for a general replacement filter of critical characters.

as a bonus, the fix addresses the concern mentioned by

Which fix? There's no commit linked in this issue.

ryjen commented 4 months ago

Can we address the security concern of allowing user input in a separate ticket instead of holding up this one?

Especially as a bait ticket given that WebDav supports adding folders?

The implementation was calling the folder creation API for each individual folder, when it only needed to call it once. This was perhapes the delay that was mentioned in the comment.

tladesignz commented 4 months ago

Can we address the security concern of allowing user input in a separate ticket instead of holding up this one?

"Holding up"? I'm sorry, if you feel that I'm holding up anything or holding up you. I just commented here, because I was mentioned, and because this seems to be a relevant thing cross-platform.

I'm not sure what your move aims towards? It doesn't exactly feel cooperative. Your whole communication doesn't exactly feel like your interested in working together with the folks who are already here. Instead it feels like you want to push them out. Happy to talk, if you feel misunderstood.

Especially as a bait ticket given that WebDav supports adding folders?

No idea what you're talking about. "Bait ticket"? What do you mean by that? "WebDav supports adding folders"? Again, what do you mean? Every backend supports adding folders except the Internet Archive. But what's the point?

AFAIU, we're talking about special characters in folder names, where I have the strong feeling, that we're currently not doing anything about it in any implementation. Hence I suggest to coordinate a thought-thru filter in both implementations.

I asked you about the "fix" you mentioned, but you seem unwilling to point me to it. Maybe you already did a clean solution which I can just copy? Or maybe you just did a very quick-and-dirty job, which needs rethinking? Do you feel threatened by that?

The implementation was calling the folder creation API for each individual folder, when it only needed to call it once. This was perhapes the delay that was mentioned in the comment.

Again: What are you talking about? What is the "implementation" you're referring to? With "delay comment" you mean this one?

The only fix I see towards issue 516 is this one: https://github.com/OpenArchive/Save-app-android/pull/557/commits/b3a5e4178d08a0df257b141d38837a3eaed1ffe7

Can you point me to anything else which is relevant for this discussion?

ryjen commented 4 months ago

I don't think I understood your "no go" comment before, but I assumed it was another push back on this ticket.

There is a push to get these little things cleaned up on android, to spend time on better things, as my hours bumped up to full time. So I have been merging fixes into an integration branch with the time difference. I would much prefer we can discuss over a PR before merging.

Your right, I did not link a change. I put a quick fix is in caf1a34. Upon second thought, I could understand not letting user input dictate additional folders, even if supported.

Willing to communicate more, but I don't think it will help trust issues much. I still don't understand what the hell the issue is now.

The apps are allowing characters because they get URL encoded, probably by accident, but whatever. The user in this case wants to create a folder structure for their content using a '/' character.

What other scope is there to this ticket?

  1. user should not be able to create folders?
  2. apps should be filtering?
  3. ios/android is not in parity?

If there is more scope, can we create a new ticket and close this one?

ryjen commented 4 months ago

As a user, I would want to make folders, but perhaps this is not the right approach, and the other backends behave differently for that feature.

In an ideal world, I would SPIKE into how to better accomplish on all backends, and with better UX than a text field and a '/'.

Then create a ticket to restrict special characters instead,on both platforms, and close this ticket.

Do you agree?

The problem with the current backend conduit implementation is that inheritance is not good enough. Composition would be more appropriate.

tladesignz commented 4 months ago

I have a feeling that we're actually not talking the same language. This might partly due to the fact, that I'm not a native speaker. But there's more to it. Our ways of thinking seem completely different.

The user in this case wants to create a folder structure for their content using a '/' character.

Wrong. The user in this case wants to create a folder which contains a forward slash "/". That is unfortunately not possible, because that is one of the very few characters which are forbidden in filenames across most all of the available file systems today.

One of the key features of this app is to upload content in a very structured manner. Allowing nested folder creation counters that intention. Also, creating nested folders by just adding slashes in a text box is highly unusual for most any GUI I know. I don't think we should start with that here. This behaviour also is the opposite of what is communicated in the dialog (It's "Folder" - singular)

TBH: I don't like your cowboy-style approach of shoot-first-ask-questions-later.

ryjen commented 4 months ago

TBH: thats all this project is

ryjen commented 4 months ago

I am reverting, and closing for #569 until we can figure out how it would work for all backends / platforms, or if it should.