change wildcard regex to allow for dots in filename

[jj-style]

Implementing a fix for #317 - error when md file name contains dot.

I'm not sure if there was a reason dots were excluded from paths to match the wildcard - I was unable to perform a directory traversal but maybe there was another reason? If so let me know and I'll abandon this! But with some local testing it seems to be behaving fine and files with a dot in the name now load

[ryanlelek]

Yep, good points. I'll double-check with some testing this weekend.

Small changes can have big effects!

[ryanlelek]

(Spongebob meme... "One year later...")

Ok, so some interesting findings.

Background

I'm going into detail for others that find this issue and want to weigh in. My concern was around security, specifically directory traversal. By allowing dots in the name, that can open up to attacks with "double dot" which means parent directory. Effectively by providing "../../not-accessible.csv" as the page might allow that file to be accessible. Read more at this link

When attempting to exploit the issue in the PR you submitted, it was not successful. My guess is something in the express stack, due to its popularity, has some protections against paths with ".." in the filename. I couldnt find it exactly, but it likely has to do with the pathToRegexp function in the main express repo (can be searched) which is based on this module https://github.com/pillarjs/path-to-regexp

So your solution works. However we should not solely rely on Express or other projects for protection.
Instead, preferring a layered approach and protecting whenever we can.

One Option

In hopes of finding a compromise to allow dots in the file name (one dot? many dots?), here's one potential solution: Changing RegExp to: ^([^.]+)\.?(md|html)?$ This would allow for blocking the dot elsewhere (with the [^.] part that was there originally) but allow for one optional dot (\.?) and the "md", "html", or other allowed extensions.

Try it out

RegEx101 Tester Note: Had to enable the "g" and "m" (global and multiline) option flags for debugging to the right... next to the copy button. RegExp: ^([^.]+)\.?(md|html)?$ Data set tested

/normal-operation
/whatever-with-a-dot.md
/more/things/and-stuff.md
/more/things/and-stuff.html
/../../../i/am/../hacked.md

All pass except for the last line

[jj-style]

I had forgotten about this PR!

You make a very good point in not relying solely on the Express stack as a line of defense. The solution definitely needs more thought than I gave my initial naive attempt.

I like the more restrictive regex though when testing it doesn't match the file given in the initial issue (content/monitoring/healtcheck.io.md). Though I'm sure we can come up with a regex that permits files like this but not with 2 consecutive dots.

There are wider issues I hadn't previously considered such as where the path is canonicalised to a standard form. If it is done before the regex check then the regex solution would be okay. But if it isn't then an exploit may be possible with URL encoding for example path names with %2e%2e decodes to .. but would slip through the regex. I would assume Express does the URL decoding but we'd want to test that to be sure.

In the meantime, for the interest of security shall I close this PR?

[ryanlelek]

If you want to work on this, we can leave this open for now, since we're likely close to a solution. If you don't have the time/interest, please close and open an issue.

We may just have to write a simple function (or use a module) to safeguard and write tests against that. Then use that function as the first function in the router wildcard.

[jj-style]

Hey,

Sorry I forgot I left this PR open. Unfortunately I can not work on this issue at this time so will close the PR for now.

Cheers