Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static content as markdown #1291

Merged
merged 2 commits into from
Dec 22, 2023
Merged

Static content as markdown #1291

merged 2 commits into from
Dec 22, 2023

Conversation

jellybob
Copy link
Contributor

This allows us to easily take content put together on pads and turn it into web content, without a tedious step of translating to EMF flavoured HTML.

@Jonty
Copy link
Member

Jonty commented Dec 17, 2023

This is great. Love that you casually rewrote all the pages in markdown without even calling it out ♥️

templates/_nav.html Outdated Show resolved Hide resolved
@Jonty
Copy link
Member

Jonty commented Dec 17, 2023

Shouldn't the upstream branch be main rather than devcontainer on this PR?

@jellybob
Copy link
Contributor Author

I'm upstreaming it on devcontainer for now because I didn't want to muddy things with a bunch of VSCode config. Once that's merged this'll rebase to main.

@jellybob jellybob force-pushed the static-content-as-markdown branch 3 times, most recently from a9eee73 to 183c16e Compare December 17, 2023 16:28
Base automatically changed from devcontainer to main December 17, 2023 18:33
apps/base/about.py Fixed Show fixed Hide fixed
apps/base/about.py Fixed Show fixed Hide fixed
Copy link
Member

@russss russss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few yaks in the markdown loading logic:

Firstly, I'm not sure if you can always assume that the current working directory is the root of the flask app and I don't want to find out the hard way when that isn't the case. It took me way too long to work this out, but os.path.join(app.root_path, app.template_folder) appears to be the canonical way of getting the template folder path in flask.

Secondly, the potential path traversal issues here make me (and CodeQL) slightly uneasy. Flask won't pass us any URL components with a forwardslash in, so it's probably not an exploitable path traversal on our current setup, but I wonder if it could be exploitable on Windows. There's also the risk that render_markdown gets re-used in a situation where the input isn't sanitised. We should defend against path traversal here.

I believe the best way of handling this these days is something along the lines of:

from pathlib import Path
...
if not Path(markdown_file_path).resolve().is_relative_to(Path(template_dir).resolve()):
	return abort(404)

Who knows if that will satisfy CodeQL.

(pathlib is great and it could be used for more of these path operations, although flask doesn't use it for its paths internally.)

apps/base/about.py Dismissed Show dismissed Hide dismissed
apps/base/about.py Dismissed Show dismissed Hide dismissed
Mostly because life is too short for translating things into HTML when
a computer can do it for us.
apps/base/about.py Dismissed Show dismissed Hide dismissed
@jellybob
Copy link
Contributor Author

I think this is sorted now, CodeQL is still unhappy about it but I've poked at trying to force a path traversal and have been unable to do so.

@russss russss merged commit 81c15e6 into main Dec 22, 2023
3 checks passed
@russss russss deleted the static-content-as-markdown branch December 22, 2023 12:06
russss added a commit that referenced this pull request Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants