List should return 404

[bkrodgers]

Currently if I do a list on a path that either does not exist or is a terminal node that holds data rather than a "folder" path, I get this:

{"lease_id":"","renewable":false,"lease_duration":0,"data":{},"warnings":null,"auth":null}

When you do a GET on a path that doesn't exist, it returns a 404. I think it'd be more natural/RESTful for Vault to return a 404 on list as well, at a minimum for a path that doesn't exist at all.

I can see an argument that an empty response for a terminal node should return an empty set, since the path does exist but just doesn't contain any children. However, there too I'd argue that the behavior should change. A list on a "folder" path returns its data as:

{"lease_id":"","renewable":false,"lease_duration":0,"data":{"keys":["path", "path2", path3"]},"warnings":null,"auth":null}

For consistency (and ease of deserialization), it'd make more sense to return an empty set on the keys element, not just an empty data element. That way an empty list is just an empty set of keys, as opposed to a data structure that terminates one level higher (by being empty). In other words:

{"lease_id":"","renewable":false,"lease_duration":0,"data":{"keys":[]},"warnings":null,"auth":null}

I don't have a strong preference for that vs a 404 though, just throwing out different options. If you don't think there's a strong need to show a difference between path not found vs path not having children, I'd probably say a 404 for both cases would be fine. But the structure above (with 404 still for path not found) would make that difference clear.

[jefferai]

@bkrodgers I agree that 404 should be thrown if the path is invalid. Since listing is only defined on directories, if the path is a leaf, I'm tempted to throw a Bad Request/400 with a descriptive error message. Does that sound reasonable?

[bkrodgers]

Yeah, as long as it's documented what the 400 means, that makes sense.

How about a legit empty set? I know that for the most part that can't really happen since you don't create directories in a backend typically, and they're instead defined by the paths that make up the actual locations of stored secrets. But one example that comes to mind is a backend that's been mounted but has nothing in it yet, like /secret/?list=true. In that case, I think the empty keys response would make sense over an empty data response:

{"lease_id":"","renewable":false,"lease_duration":0,"data":{"keys":[]},"warnings":null,"auth":null}

[jefferai]

For reasons I really, really don't want to get into, I need to make all of these end up the same -- so any time that no keys would be returned, use one status code or another. @sethvargo pointed out that technically 204 is most accurate, because no content was found at that path and, as @bkrodgers noted, paths are wishy-washy anyways in that they're defined by what they hold -- so in a sense all paths exist at any given time. However, Seth also pointed out that 404 is easier to deal with from a library standpoint, and GET is really (unless you use the LIST verb) GET?list.

I'm going to go with "make it like GET" and "do what's easiest for libraries" and make them all 404. So any time an empty set of keys would be returned, a 404 will come instead.

@bkrodgers does that work for you?

[bkrodgers]

Yeah, if that's the best you can (easily) do at this point, it's certainly an improvement in my mind from returning a 200.