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

Replay download link #19

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Replay download link #19

wants to merge 3 commits into from

Conversation

max-torch
Copy link
Contributor

There is code autoformatting included in this PR.

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

Closes #3

@max-torch max-torch requested a review from soliton- May 3, 2024 15:02
@soliton-
Copy link
Member

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.

+ x["END_TIME"].dt.strftime("%Y/%m/%d")
+ "/"
+ x["REPLAY_NAME"]
+ ")", # Markdown link to replay file download
Copy link
Member

Choose a reason for hiding this comment

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

While the replay filename is not entirely arbitrary this essentially concatenates arbitrary data to hopefully get the desired markdown syntax. In general this kind of injection should be avoided but probably can't here.

If there is a convenient way to properly generate a markdown link that would be preferable. Otherwise need to check what can happen in the worst case. Given that brackets are currently not allowed in a replay filename probably not too much.

Of course another option would be to just not generate markdown and leave the url as a plain string.

(Adding this as proper review comment, 3d6b0e6#comments was easy to miss.)

@max-torch
Copy link
Contributor Author

max-torch commented May 24, 2024

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-
Copy link
Member

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

@max-torch
Copy link
Contributor Author

max-torch commented Jul 23, 2024

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-
Copy link
Member

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.

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.

Request For Replay Link/Title from dashboard-exported CSV data
2 participants