Open-EO / openeo-processes

Interoperable processes for openEO's big Earth observation cloud processing.
https://processes.openeo.org
Apache License 2.0
49 stars 15 forks source link

only allow relative paths in `load_uploaded_files? #461

Open soxofaan opened 11 months ago

soxofaan commented 11 months ago

https://github.com/Open-EO/openeo-processes/blob/965bbaebd4d5984203a0437076c85a66a72a23e0/proposals/load_uploaded_files.json#L12-L20

Current regex allows paths that escape the user workspace, like /etc/passwd and ../../etc/passwd. Wouldn't it be cleaner to at least forbid absolute paths (starting with /)?

m-mohr commented 11 months ago

The file paths are not meant to be mapped to internal structures. The base path that should always be the user workspace, so I'd expect a folder/file.txt in the user workspace to be accessible via /folder/file.txt and folder/file.txt and ./folder/file.txt. A backend should ensure a user can never escape that base path. Maybe we need to clarify this further.

soxofaan commented 11 months ago

I'm also thinking about being future proof (e.g. loading files from another user), or allowing back-ends to go further than the official spec and workflows (e.g. supporting an out-of-band upload mechanism that uploads to something like /projects/ESA-123/data456.nc). If the standard regex is too liberal, there might be not enough wiggle room for these extensions. That's why I'm suggesting to make sure the current standard only allows pure relative paths.

m-mohr commented 11 months ago

For other users, I'd rather use either a new parameter to select the data source or load_url. Encoding that in the path and as such exposing internals doesn't seem like a good idea. Similarly, if you have other data sources, or alternatively a new process.

m-mohr commented 7 months ago

I don't see a direct todo here, closing for now. If you think differently, please open a PR with a proposal.

soxofaan commented 7 months ago

A backend should ensure a user can never escape that base path. Maybe we need to clarify this further.

I think this is at least a todo here

m-mohr commented 7 months ago

PRs are welcome.