Uh oh!
There was an error while loading. Please reload this page.
Add 'allow Symlinks' as an option to config.php - #24606
Conversation
Uh oh!
There was an error while loading. Please reload this page.
kesselb
commented
Dec 8, 2020
I don't see why we need such a option. Bind mounts seems to do the trick: #1257 (comment) |
noiob
commented
Dec 9, 2020
@kesselb The option is for our (hosting.de's) specific use case. I don't know the specifics but this would keep us from having to patch the code to enable that option |
3ddc8c4 to
bf8531dComparekesselb
commented
Dec 9, 2020
And you cannot use bind mounts because? |
PVince81
commented
Dec 9, 2020
I'm guessing because bind mounts are good for a small set of links, and ones controlled by the admin. In the case where there's either lots of symlinks (like 1000s of them) or where users would be able to create symlinks through some other means, like accessing the storage from outside, then bind mounts wouldn't work. In the latter case though I wouldn't want to let users create symlinks as they could point anywhere and be insecure. Just guessing here. |
fladi
commented
Dec 9, 2020
@kesselb To give you a scenario where following root-created symlinks makes sense: We moved all our 4000 users one by one from one storage to a newer, live. So we deactived one user after the other, So please just make it a configurable option because for us, carrying a patch for |
kesselb
commented
Dec 9, 2020
You are migrating users from A to B. Sounds acceptable to patch |
PVince81
commented
Dec 9, 2020
The migration might be done over several months during which NC updates might happen which would overwrite the patch. And without the patch some users would suddenly not see their data any more due to symlinks not being followed. I think the arguments above are a good reason to add it to config.php. I also understand the concern that less experienced users might introduce security troubles on their instances by setting it in the config. |
kesselb
commented
Dec 9, 2020
I'm not convinced. Luckily it's not my call to accept or reject this pull request ;) Please keep in mind that it's nearly impossible to remove such a configuration option in the future again. |
fladi
commented
Dec 9, 2020
@kesselb We migrated 15PiB of data... this took us 6 month to finish. We started on NC18 and upgraded to NC19 during this time. |
PVince81
commented
Dec 11, 2020
failing preview tests unrelated, added here: #22305 (comment) |
| * | ||
| * Defaults to ``false`` | ||
| */ | ||
| 'localstorage.allowsymlinks' => false, |
There was a problem hiding this comment.
We should think about some convention for those configuration options.
We have camelCase, snake_case sometimes . 🙈
PVince81
commented
Dec 14, 2020
I guess all CI issues are known and we can merge ? |
bf8531d to
b89f224ComparePVince81
commented
Jan 12, 2021
Psalm issue was fixed on master, another rebase should fix it. |
b89f224 to
f1978b2CompareSigned-off-by: Johannes Leuker <j.leuker@hosting.de>
f1978b2 to
ac88bcbCompare
I'm not sure whether the explanation in
config.sample.phpis sufficient, maybe it should include a warning about potential problems and risks of allowing symlinks