create_folder does nothing (and does not throw) if filepath is an empty string

[joshuagl]

Description of issue or feature request:

In order to support existing behaviour in tuf the create_folder method of FilesystemBackend does nothing and does not raise an error if passed an empty string.

This feels like counter-intuitive behaviour that doesn't make sense for the storage backend. Ideally this would be resolved in tuf by removing calls to create_folder with an empty path i.e. performing any special casing in the caller.

See #240

Current behavior:

When securesystemslib.storage.FilesystemBackend.create_folder() is called with an empty string it returns immediately.

Expected behavior:

When securesystemslib.storage.FilesystemBackend.create_folder() is called with an empty string it raises an error, which better mimics the behaviour of os.mkdir():

>>> os.mkdir('')
Traceback (most recent call last):
  File "<input>", line 1, in <module>
    os.mkdir('')
FileNotFoundError: [Errno 2] No such file or directory: ''

[joshuagl]

If memory serves this change will require fixes in tuf to not call create_folder with an empty string as the path argument. We should also update the TestStorageBackend implementation so that it does not encode this behaviour.

[woodruffw]

Thanks for opening this! I agree that this behavior should be resolved in tuf and that the invariant should be modified (to raising, as expected).

[lukpueh]

I second that motion. Let's raise a StorageError here.

[MVrachev]

I will push a pr to fix this.

In my changes, I will raise

raise securesystemslib.exceptions.StorageError(
            "Can't create a folder with an empty filepath!")

exception.

I am not sure how we are going to handle the special case when we call create_folder with empty string in tuf.
It sounds to me that it's normal to not catch that exception and receive it. After all, how are we supposed to recover if create_folder was called with an empty string in tuf?

[woodruffw]

Sounds good to me. It sounded like there were some historical reasons for "" being silently ignored, but those reasons are opaque to me 🙂

[joshuagl]

I am not sure how we are going to handle the special case when we call create_folder with empty string in tuf.
It sounds to me that it's normal to not catch that exception and receive it. After all, how are we supposed to recover if create_folder was called with an empty string in tuf?

I don't think we can catch the Exception and handle it in tuf, because we're throwing a generic error from securesystemlib. We might compare the messages and behave differently, but that feels too brittle.

We only use create_folder in 3 places in tuf and it looks like it would be a genuine error if they were called with an empty string, each one claims to be ensuring an expected folder exists.

[woodruffw]

Looked at this a bit closer: of the 3 create_folder calls in TUF, only one of them can ever actually be "". The other two are called with the result of an os.path.join with a guaranteed nonempty string. I think what this calls for is a new schema type, something like AnyNonemptyString, which is used for PATH_SCHEMA instead of SCHEMA.AnyString.

[woodruffw]

I went ahead and opened #244 as a PoC for the above. Not actually sure if it's the best approach, but it should avoid the problem of potentially empty strings by catching them in the schema itself 😅

[MVrachev]

Making PATH as AnyNonemptyString fixes the issue for TUF, but still, I am wondering don't we expect to get an exception if we pass an empty path by any chance as will happen if that happens when calling os.mkdir?

[lukpueh]

Making PATH as AnyNonemptyString fixes the issue for TUF, but still, I am wondering don't we expect to get an exception if we pass an empty path by any chance as will happen if that happens when calling os.mkdir?

Are you saying that create_folder should raise an exception? If so, I still agree. IIUC #244 is only an additional counter-measure to prevent TUF from passing empty strings to this function in the first place.

[woodruffw]

IIUC #244 is only an additional counter-measure to prevent TUF from passing empty strings to this function in the first place.

This understanding is correct!

I also agree 100% about create_folder raising an exception now that we've limited the ability of TUF itself to pass "" as an argument to it.

[joshuagl]

Resolved with #252