search

wesnoth / wesnoth-multiplayer-data-dashboard

This is a dashboard that allows you to query and visualize data that the Wesnoth Project collects about multiplayer games played on the official server.
2 stars 0 forks source link

Replay download link #19

Closed max-torch closed 1 week ago

max-torch commented 5 months ago

There is code autoformatting included in this PR.

The main change feature implementation is 3d6b0e6834d12927be85a310a29c3ce5b450c95d All the rest are document style formatting only.

Closes #3

soliton- commented 5 months ago

As a general comment I would not put reformatting commits in a PR. They can be committed directly with a more sensible commit message and no reason to do a commit per file.

max-torch commented 5 months ago

As a general comment I would not put reformatting commits in a PR. They can be committed directly with a more sensible commit message and no reason to do a commit per file.

Yeah, this is okay, just that some teams like minimizing direct commits to main. For reformatting, I don't mind. The ideal end goal by the way, hopefully, is to fix up #6 to automate the code formatting.

soliton- commented 5 months ago

I mean you can still make a PR but it should be its own PR then.

max-torch commented 3 months ago

need to check what can happen in the worst case

After analyzing and testing, I found that the code in this PR currently could be susceptible to a Markdown injection. Example case: image

[[Game Replay](http://example.com)](https://replays.wesnoth.org/2023/04/01/%5BGame%20Replay%5D%28http%3A%2F%2Fexample.com%29.bz2)

In the example, the replay filename contains a Markdown link, which overrides the link that the app is trying to make. The link goes to example.com instead.

Solition said:

Given that brackets are currently not allowed in a replay filename

Good thing this is the case. Someone cannot name a game in the Wesnoth game client as a Markdown link.

I found a relevant thread asking about almost the same problem in the context of Streamlit (equivalent alternative to Plotly Dash) https://discuss.streamlit.io/t/how-to-sanitize-user-input-for-markdown/828/4 In that thread, the Streamlit guys are saying that you have to define your own markdown/url sanitizing function, and they show how, which is also more or less applicable in our case.

So it seems there is some sanitization and formatting that can be done to improve this code before we merge it.

Not ready to show a solution implemented for this PR yet, but im posting this useful contextual information now. Will also need to demonstrate test cases to show that the function is working properly with various filename strings. Have more insight now what to do next.

Additional reference for markdown sanitization: markdown special characters https://tech.saigonist.com/b/code/escaping-special-characters-markdown.html

soliton- commented 3 months ago

After analyzing and testing, I found that the code in this PR currently could be susceptible to a Markdown injection.

The code is doing markdown injection. (I thought that's what I said above.)

To reiterate you can also just make the URL a plain string for now. It's not particularly inconvenient to copy&paste the URL. Especially if you need to custom write proper markdown generation then I think that might not be worth it.

max-torch commented 1 week ago

After analyzing and testing, I found that the code in this PR currently could be susceptible to a Markdown injection.

The code is doing markdown injection. (I thought that's what I said above.)

To reiterate you can also just make the URL a plain string for now. It's not particularly inconvenient to copy&paste the URL. Especially if you need to custom write proper markdown generation then I think that might not be worth it.

Seconded. Will close.

soliton- commented 1 week ago

To be clear currently it's just the replay name not an URL. I'm a bit confused why you're closing this. Just don't use markdown...