erp5_web_service: Do not change working directory on remote. (!1519) · Merge Requests · nexedi / erp5

erp5_web_service: Do not change working directory on remote.

Allowing callers to exit the path set on the connector seem wrong to me, was this intended ? So I would rather always construct absolute paths locally (which allows checking them) than let callers accidentally write out of the path configured on the connector. The intent being to have fewer surprises when we change the connector's base path.

Also, this saves one round-trip on every connection (although this is likely unnoticeable for most uses).

To illustrate the intended behaviour changes (using writeFile as it is the most complex use-case):

connector url	code	before	after
sftp://example.com/	writeFile(path='bar', filename='baz.txt')	/bar/baz.txt	/bar/baz.txt
sftp://example.com/foo	writeFile(path='bar', filename='baz.txt')	/foo/bar/baz.txt	/foo/bar/baz.txt
sftp://example.com/	writeFile(path='/bar', filename='baz.txt')	/bar/baz.txt	/bar/baz.txt
sftp://example.com/foo	writeFile(path='/bar', filename='baz.txt')	/bar/baz.txt ⚠	/foo/bar/baz.txt
sftp://example.com/	writeFile(path='../bar', filename='baz.txt')	/bar/baz.txt	/bar/baz.txt¹
sftp://example.com/foo	writeFile(path='../bar', filename='baz.txt')	/bar/baz.txt ⚠	`ValueError`
sftp://example.com/	writeFile(path='bar', filename='/baz.txt')	/baz.txt	/bar/baz.txt²
sftp://example.com/foo	writeFile(path='bar', filename='/baz.txt')	/baz.txt ⚠	/foo/bar/baz.txt²
sftp://example.com/	writeFile(path='bar', filename='../baz.txt')	/baz.txt	/baz.txt²
sftp://example.com/foo	writeFile(path='bar', filename='../baz.txt')	/foo/baz.txt	/foo/baz.txt²
sftp://example.com/	writeFile(path='bar', filename='../../baz.txt')	/baz.txt	/baz.txt²
sftp://example.com/foo	writeFile(path='bar', filename='../../baz.txt')	/baz.txt ⚠	`ValueError`

@kazuhiko @jerome @georgios.dagkakis @aurel

I would prefer to raise ValueError here, but the detection code would be quite complex just for this case... ↩
The desirable behaviour in these cases is unclear to me... Is caller responsible of providing non-conflicting arguments ? ↩

Also, this saves one round-trip on every connection (although this is likely unnoticeable for most uses).

To illustrate the intended behaviour changes (using `writeFile` as it is the most complex use-case):

| connector url | code | before | after |
| --- | --- | --- | --- |
| sftp://example.com/ | writeFile(path='bar', filename='baz.txt') | /bar/baz.txt | /bar/baz.txt |
| sftp://example.com/**foo** | writeFile(path='bar', filename='baz.txt') | /foo/bar/baz.txt | /foo/bar/baz.txt |
| sftp://example.com/ | writeFile(path='/bar', filename='baz.txt') | /bar/baz.txt | /bar/baz.txt |
| sftp://example.com/**foo** | writeFile(path='/bar', filename='baz.txt') | **/bar**/baz.txt ⚠ | /foo/bar/baz.txt |
| sftp://example.com/ | writeFile(path='../bar', filename='baz.txt') | /bar/baz.txt | /bar/baz.txt[^1] |
| sftp://example.com/**foo** | writeFile(path='../bar', filename='baz.txt') | **/bar**/baz.txt ⚠ | `ValueError` |
| sftp://example.com/ | writeFile(path='bar', filename='/baz.txt') | /baz.txt | /bar/baz.txt[^2] |
| sftp://example.com/**foo** | writeFile(path='bar', filename='/baz.txt') | **/baz.txt** ⚠ | /foo/bar/baz.txt[^2] |
| sftp://example.com/ | writeFile(path='bar', filename='../baz.txt') | /baz.txt | /baz.txt[^2] |
| sftp://example.com/**foo** | writeFile(path='bar', filename='../baz.txt') | /foo/baz.txt | /foo/baz.txt[^2] |
| sftp://example.com/ | writeFile(path='bar', filename='../../baz.txt') | /baz.txt | /baz.txt[^2] |
| sftp://example.com/**foo** | writeFile(path='bar', filename='../../baz.txt') | **/baz.txt** ⚠ | `ValueError` |

[^1]: I would prefer to raise `ValueError` here, but the detection code would be quite complex just for this case...

[^2]: The desirable behaviour in these cases is unclear to me... Is caller responsible of providing non-conflicting arguments ?

@kazuhiko @jerome @georgios.dagkakis @aurel

Edited Dec 02, 2021 by Vincent Pelletier

erp5_web_service: Do not change working directory on remote.

Check out, review, and merge locally