1133120 - docker-worker: Don't put // in artifact names when folders are declared

Status: RESOLVED WONTFIX

(Taskcluster :: Workers, defect)

Description

[Jonas Finnemann Jensen (:jonasfj)]

When task.payload.artifacts is defined as follows:
-------
  "artifacts": {
    "public/logs/": {
      "path": "/home/worker/build/upload/logs/",
      "expires": "2016-02-13T22:12:13.807671Z",
      "type": "directory"
    },
    "public/test_info/": {
      "path": "/home/worker/build/blobber_upload_dir/",
      "expires": "2016-02-13T22:12:13.807742Z",
      "type": "directory"
    },
    "public/build": {
      "path": "/home/worker/artifacts/",
      "expires": "2016-02-13T22:12:13.801638Z",
      "type": "directory"
    }
  },
-------
Ie. type is "directory" and the name ends with a slash "/". as in "public/test_info/".
We get artifact names that contains two slashes, such as:

  public/test_info//emulator-5554.log

That is bad, because two slashes is valid in URLs, so you will be force to provide
it when fetching the artifact.

I suggest that we either forbid with a regular expression in the JSON schema. Or we sanitize it. The problem with sanitizing it is that names are truly just
strings so if you sanitize a script that looks at task.payload.artifacts will not
be able to find the artifacts.

Hence, I suggest we in the JSON schema for docker-worker payload forbid ending
artifact names with a slash. Or maybe I'm just crazy here...

Either way, something must be done, we can't have two slashes.

See task:
  https://tools.taskcluster.net/task-inspector/#NWWl5TISSkG72cI-H4pbKA/0

Comment 2

[Pete Moore [:pmoore][:pete]]

(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #0)

> I suggest that we either forbid with a regular expression in the JSON
> schema. Or we sanitize it. The problem with sanitizing it is that names are
> truly just
> strings so if you sanitize a script that looks at task.payload.artifacts
> will not
> be able to find the artifacts.
> 
> Hence, I suggest we in the JSON schema for docker-worker payload forbid
> ending
> artifact names with a slash. Or maybe I'm just crazy here...

See https://bugzilla.mozilla.org/show_bug.cgi?id=1217785#c2 - I tend to lean towards your first solution too (forbidding trailing slash).

Comment 3

[Pete Moore [:pmoore][:pete]]

Also note, we should consider handling this on the queue side too. What should the queue do if an artifact is published to a path containing multiple slashes? Even if we don't do it in our workers, other workers we don't control might do this...

So I think we should handle this in the workers *and* the queue.

Comment 4

[Pete Moore [:pmoore][:pete]]

gahhh - I meant

  "Even if we do handle it in our workers, other workers ...."

Comment 5

[Jonas Finnemann Jensen (:jonasfj)]

@pmoore,
> Also note, we should consider handling this on the queue side too.
> What should the queue do if an artifact is published to a path containing multiple slashes?

Nothing, if a user declares a file artifact:  "Hello//World/file.txt"
Then they've clearly asked for double slash, I see no reason to forbid it, it's a thing.

Generally double slash is only an issue when user declares a folder to upload,
as the slash rules aren't obvious.

Comment 6

[Pete Moore [:pmoore][:pete]]

Fair enough, I think that is sane.

Let's go with forbidding in the worker(s) then. As for the generic worker, it currently only supports file artifacts, not directory artifacts, so when that is implemented, we'll forbid the trailing slash.

I'm hoping to implement that next week, all being well.

@garndt, are you comfortable with this solution for docker worker?

Comment 7

[Greg Arndt [:garndt]]

I agree with forbidding it and reporting malformed payload (as discussed in bug 1217785).

Comment 8

[Pete Moore [:pmoore][:pete]]

NB: there seem to be some trailing slashes in-tree that will be affected by this change:

e.g. https://hg.mozilla.org/mozilla-central/rev/672da9a06334 is still present at time of writing in https://hg.mozilla.org/mozilla-central/file/default/testing/taskcluster/tasks/test.yml

We should fix these references before going live with the change.

Also this may impact anybody that assumes these paths don't change. But that doesn't seem like a good reason not to fix it.

Comment 9

[Brian Stack [:bstack]]

pmoore, can this be closed if we don't want to be making small updated to docker-worker anymore?

Comment 10

[Pete Moore [:pmoore][:pete]]

Fine for me.