Fix radio button unsafe token access bug #2790
Open
+54
−21
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the purpose of this pull request?
Resolves #2749
Overview of changes:
Fixed the unsafe token access & unskipped the testcases made in #2747
(x)&(X)respectivelyAnything you'd like to highlight/discuss:
As @gerteck mentioned in the issue,
Hence, I fixed it by adding bounds checks before accessing
tokens[i-5]&tokens[i-4]which prevents the crash when those tokens don't exist.Additionally, the previous logic only handled the radio button cases if there was a space after the parentheses, i,.e.
'( ) '. However, will not be the case dealing with empty radio buttons. Hence, I added the cases to explicitly when the radio button is the entire content of the line (i.e. empty radio button).indexOfchecks were initially for'( ) '& not'( )'to begin with (,because from my testing, using the latter fixes this issue for the most part, but I might be missing the bigger picture here.)Aside from that, kindly let me know if my changes are suffice and if there is anything else that I missed/could improve. 🤓
PS: Potential code quality improvement?
I was thinking of repurposing
function startsWithTodoMarkdown()since it is only used byfunction isTodoItem(token, index)and its logic is somewhat repeated infunction makeRadioButton(token, TokenConstructor, radioId).But at the same time, I'm not sure if doing so will be worth it if it decreases the clarity / makes
makeRadioButtondependent onstartsWithTodoMarkdowntoo. 🫤Interested to hear your thoughts on this matter too! 😀
See resolved example
e.g. when serving this markdown file
Example of whats generated:

Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Fix markdown-it radio button unsafe token access
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):