Made it possible to edit htaccess from the admin panel

[Ibochkarev]

What does it do?

Made it possible to edit htaccess from the admin panel

Why is it needed?

For ease of editing from the admin panel

How to test

Add htaccess to the upload_files system setting. And rename ht.access to .htaccess

Related issue(s)/PR(s)

https://github.com/modxcms/revolution/issues/16221

[JoshuaLuckers]

Use this Regex ^(\.{2,}|/)+ to match 2 or more dots, otherwise any uneven amount of dots would not be matched

@benni1516 With preg_replace the regex '#^(\.{2}|/)+#u seems to work fine for me. E.g: .....htaccess ends up in .htaccess which is the expected result.

[benniledl]

@JoshuaLuckers hello, sorry if I'm wrong, I thought about something like this https://regex101.com/r/JPrLdx/1

[smg6511]

@benni1516 - Just curious (and not making any assumptions); have you pulled this down and tested this or are you commenting based solely upon looking at the code changes? I ask because if you've not tested this I'd really like to encourage you to do so. While comments and observations are helpful, we're really in need of folks willing to go the extra mile and test PRs [shout out to anyone reading this who's not testing ... yet ;-) ].

@Ibochkarev, @JoshuaLuckers, @benni1516 - Anyway, after playing with this a bit more, if you really push it with a file name like ./../.../..../..htaccess, then you end up with likely an unwanted file ..htaccess. Interestingly, it's the presence of the various dots and slashes ahead of the last part of the path that causes ..htaccess to be created. Bottom line is you can end up with a file where part of the name consists of dots. Here's the rub: That does not happen without the change this PR applies. That said, the regex could be thought through more to avoid this issue OR this could be considered a non-issue under the assumption that virtually no one is going to enter file names with a bunch of dots and slashes.

[opengeek]

The proper way to avoid directory traversal is to compare the realpath() of the path provided by the user with a pre-defined base path for the operation. I don't see any defined base path here, but it absolutely should be using one.

[benniledl]

@smg6511 sorry I didn't test it out, I will test things out before commenting in the future :)

[smg6511]

@benni1516 - No apologies necessary ... just wanted to encourage you test ;-)

All - FYI, changed status as a little more needs to be done with the regex. I'm playing with something to propose and will put that up as a requested change (or in a comment) momentarily...

[opengeek]

If you can specify any directory, why are we preventing directory traversal with regex? This makes no sense. We need to be using basename() to get the filename and using realpath() to compare the provided path with a properly limited "base" path for the operation.

[smg6511]

What I'm trying to do is, taking into account that you can traverse (and dynamically create) directories moving downward, keep the resulting sanitized path as clean as possible (and non-helpful mysterious error messages) while allowing .files and .directories to be written and edited. Upward traversal wasn't possible anyway.

Take for instance creating a new file with this path: newdir5/../test.txt. With the current regex, that path will make it past this initial sanitize (in Browser) and ultimately create a file at newdir5/text.txt because a second pass is made by sanitizePath (in modMediaSource) when createObject is called. However, an error is triggered along the way — a possible explanation is when the file is read back in, the initial invalid path (due to /../) is perhaps being used in that process (unconfirmed). At any rate, I adjusted the regex to avoid this sort of thing, as the on-screen error is confusing and useless IMO (there's no error given in the modx log).