motioneye-project / motioneye

A web frontend for the motion daemon.
GNU General Public License v3.0
3.97k stars 651 forks source link

Any reason why dots aren't allowed in filenames as of 0.41? #1497

Open jwheeler91 opened 5 years ago

jwheeler91 commented 5 years ago

https://github.com/ccrisan/motioneye/blob/f0318bbba1527d06e8510fd3996d7f996309cbb4/motioneye/static/js/main.js#L20

zagrim commented 5 years ago

Umm, it might be that I just neglected that when adding validation patterns. I don't recall if I noticed any side-effects with having dots there. I assume you can confirm that dots have been working ok?

jwheeler91 commented 5 years ago

I have been using dots yes :D No issues. In fact it still works, I just can't save any other setting lol.

zagrim commented 5 years ago

I can issue a PR to fix that. Can you think of any other character that should be also allowed (without risking issues with special meanings or character encodings)? I couldn't think of any such, except maybe for comma, plus and hash/pound sign ( , + # ) (Motion doesn't seem to allow trailing comments in config files so # should be safe).

jwheeler91 commented 5 years ago

Personally, I would restrict to the POSIX Portable Filename Character Set: https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.2.0/com.ibm.zos.v2r2.bpxa400/bpxug469.htm

So: ^[A-Za-z0-9._-]+$

Also note the plus '+' (1 or more). This would mean that the filename would have to be at least one character long. However this might not work when the movie or image section is disabled and empty, depending on how the validation is applied in that scenario? This can easily be switched for a star '*' (0 or more)

To support sub directories, we can include the forward slash. But supporting the percent specials, but not percent in general is more complex. This is what I came up with:

<script>
    function valid(filename) {
        var regex = new RegExp('^([A-Za-z0-9/._-]|%[YmdHMSqv])+$');
        //var regex = new RegExp('^([A-Za-z0-9 \(\)/._-]|%[YmdHMSqv])+$');
        var isValid = (filename.match(regex) ? "Valid" : "Invalid");
        document.write("<p>" + isValid + " - " + filename + "</p>");
    }

    valid('hello/abc%d.orig');
    valid('%Y-%m-%d/%H-%M-%S.orig'); // My config

    valid('%Y-%m-%d/%H-%M-%S Spaced'); // Spaces
    valid('%Y-%m-%d/%H-%M-%S_(Bracketed)'); // Parenthesis

    valid('hell%o2'); // Should be invalid due to incorect '%o'
    valid('hello2\\abc%d.orig'); // Should be invalid due to incorect '\'
</script>

Run using https://js.do

Note the commented second regex to support spaces and parenthesis. I'm not sure that that is a good idea, perhaps @ccrisan can chime in on his preference?

I do note the use of toLowerCase which is a bit inefficient given that regex is already being used. I would add A-Z to the regexes that use this instead.

P.s. I would also set dirnameValidRegExp to '^[A-Za-z0-9/._-]+$' (again no spaces or parenthesis)

zagrim commented 5 years ago

Thanks for the input! I'd also like to hear what Calin Crisan thinks of this.

My own preference (despite omitting the dot and some other safe characters just by mistake) would be to restrict the user as little as possible without risking application functionality (which would mean allowing everything which can not be proven to be dangerous for stability). Myself I try to avoid spaces, parenthesis, braces and brackets in file names, but someone else might have a use for some of those and enforcing strict validation despite no known adverse effects for a given "semi-special" character would only mean bad user experience.

ccrisan commented 5 years ago

Also note the plus '+' (1 or more). This would mean that the filename would have to be at least one character long. However this might not work when the movie or image section is disabled and empty, depending on how the validation is applied in that scenario? This can easily be switched for a star '*' (0 or more)

When the field is not available, the UI doesn't (or shouldn't, as far as I remember) apply any kind of validation. So I think we're good with + there. Needs to be tested, though.

My own preference (despite omitting the dot and some other safe characters just by mistake) would be to restrict the user as little as possible without risking application functionality (which would mean allowing everything which can not be proven to be dangerous for stability).

I totally agree that the minimal set of restrictions should be imposed on the filenames. However it's not that easy to find out which ones actually do harm. Of course spaces and parentheses are not welcome. Maybe we could try starting with the POSIX compliant charset that @jwheeler91 mentioned and extend it with more stuff that we know shouldn't do harm.

Let's just make sure / is actually allowed since it's the way we build subfolders structures.

jwheeler91 commented 5 years ago

I've updated the PR: https://github.com/ccrisan/motioneye/pull/1502

I've also dumped the main.js into my live site and confirm that it works from a filename perspective (when blank it marks the field as invalid, but disabling the section allows it to save)

Email seems to validate correctly, but I can't test this properly as I don't have smtp set up.

Out of interest, line 18 has the hat symbol '^' inside the character group (the square brackets) Is that correct? Or was it supposed to validate from the beginning of the line, and not allow hat characters in the signature?

ccrisan commented 5 years ago

@jwheeler91 no idea about that one. In any case, it won't do any harm. @zagrim are you OK with all the changes in #1502?

zagrim commented 5 years ago

@ccrisan I'm ok with the changes.