Bug #15836

Escape / convert forward slashes in collection names accessed via WebDAV

Added by Tom Morris 9 months ago. Updated 7 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Target version:
Start date:
01/08/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

We have a customer report that the Mountain Goat WebDAV client doesn't display collections which have slashes (/) in their names. That's not terribly surprising considering it's use as a path delimiter, but it's not great to not be able to access the collection at all. We should consider a configurable option to replace the slash with another character such as underscore (_) or hyphen (-).


Subtasks

Task #15974: Review 15836-slash-in-collection-nameResolvedPeter Amstutz

Associated revisions

Revision 4237a24f
Added by Tom Clegg 7 months ago

Merge branch '15836-slash-in-collection-name'

fixes #15836

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Tom Clegg 9 months ago

The WebDAV server doesn't show collection/project names that have slashes (nor ones named "." or "..").

permittedName() -- source:sdk/go/arvados/fs_base.go#L619

I'm not sure this can be fixed with an escape char: either the escape char is an invalid filename char and clients can't send/receive it, or it is and clients get confused when we treat it as an escape char. Exporting it as something horrible like "<solidus>" would at least make it easier to figure out what the problem is, even if it doesn't make web/filesystem clients behave smoothly.

Perhaps the best course is to disallow these names on the API side so users don't fall into the trap at all.

#2 Updated by Ward Vandewege 8 months ago

Desired outcome is to replace unallowed characters with something else (e.g. _) in webdav, just like we do for FUSE. That way they will be listed in the client. We should still allow creation of collection with those characters, because they are valid (and in use!).

#3 Updated by Peter Amstutz 8 months ago

  • Target version changed from To Be Groomed to 2020-01-15 Sprint

#4 Updated by Tom Clegg 7 months ago

  • Assigned To set to Tom Clegg

#5 Updated by Tom Clegg 7 months ago

I think we should allow but discourage this, since it seems impossible to implement it without surprising/confusing corner cases. Perhaps a config variable "string to replace slashes with in readonly webdav listings" -- where "" means don't list them at all.

If the replacement string is "_":
  • When looking up "/home/foo__bar/baz", we look up "foo__bar" by name, rather than populating the entire "/home" path. If looking up "foo__bar" and that doesn't exist, we'll need to check for collections/projects named "foo_/bar" or "foo//bar" as well, before giving up.
  • If a new "foo_bar" collection/project is created in a project where "foo/bar" already exists, URLs that used to reach the old "foo/bar" will now reach the new "foo_bar".
  • In future when "mkdir /home/foo_bar" is possible to do via webdav, and "foo/bar" already exists, we'll need to decide whether that should create a new collection that masks the old one, or return "directory already exists" (probably the latter)

#6 Updated by Peter Amstutz 7 months ago

  • Category set to Keep

#7 Updated by Lucas Di Pentima 7 months ago

I've tried using Mountain Duck client version 3.3.3 and the replacement string '%2F' on a collection name is displayed as is and without issues.

#9 Updated by Tom Clegg 7 months ago

  • Status changed from New to In Progress

#11 Updated by Peter Amstutz 7 months ago

  • Target version changed from 2020-01-15 Sprint to 2020-01-29 Sprint

#12 Updated by Peter Amstutz 7 months ago

From the config.yml comment

  1. Possible values include "%2f" and "{slash}"

Should be "example" values, otherwise could be read as those are the only two possible values.

If the default empty value is used, names containing "/"
cannot be used when creating or renaming a collection or
project.

Passive voice. Should be 'creating or renaming a collection with a name containing "/" will be rejected by the API server'

for tryURL, expectRegexp := range map[string]string{
base: `(?ms).*href="./` + nameShownEscaped + `/"\S+` + nameShown + `.*`,
base + nameShown + "/": `(?ms).*href="./filename"\S+filename.*`,
} {

Should the path "base + nameShownEscaped" be tested as well? Or is the URL encoding happening in url.Parse() or http.Request() ?

Could you merge/rebase master again so I can do some manual testing?

#13 Updated by Tom Clegg 7 months ago

Peter Amstutz wrote:

From the config.yml comment

  1. Possible values include "%2f" and "{slash}"

Should be "example" values, otherwise could be read as those are the only two possible values.

Reworded.

      # WebDAV. Example values are "%2f" and "{slash}". Names that
      # contain the substitution string itself may result in confusing
      # behavior, so a value like "_" is not recommended.

If the default empty value is used, names containing "/"
cannot be used when creating or renaming a collection or
project.

Passive voice. Should be 'creating or renaming a collection with a name containing "/" will be rejected by the API server'

Reworded.

      # If the default empty value is used, the server will reject                                                                                               
      # requests to create or rename a collection when the new name                                                                                              
      # contains "/".                                                                                                                                            

for tryURL, expectRegexp := range map[string]string{
base: `(?ms).*href="./` + nameShownEscaped + `/"\S+` + nameShown + `.*`,
base + nameShown + "/": `(?ms).*href="./filename"\S+filename.*`,
} {

Should the path "base + nameShownEscaped" be tested as well? Or is the URL encoding happening in url.Parse() or http.Request() ?

Right, nameShownEscaped is more correct here -- an unescaped "{" is not allowed in an URL path segment, even though it happens to be accepted by Go.

Could you merge/rebase master again so I can do some manual testing?

Sure, merged master.

15836-slash-in-collection-name @ 4164f38ef0f900962e3c365745c59d5c20a63c66

#14 Updated by Tom Clegg 7 months ago

Added an entry to the upgrade notes.

15836-slash-in-collection-name @ d766bbe3770d56f8ef391999ec51c42cb751b319

#15 Updated by Peter Amstutz 7 months ago

Tom Clegg wrote:

Added an entry to the upgrade notes.

15836-slash-in-collection-name @ d766bbe3770d56f8ef391999ec51c42cb751b319

This LGTM

#16 Updated by Anonymous 7 months ago

  • Status changed from In Progress to Resolved

#17 Updated by Peter Amstutz 7 months ago

  • Release set to 22

Also available in: Atom PDF