Skip to content

Conversation

@MoshiMoshiMochi
Copy link

@MoshiMoshiMochi MoshiMoshiMochi commented Jan 19, 2026

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Resolves #2749

Overview of changes:
Fixed the unsafe token access & unskipped the testcases made in #2747

  • Also added additional check to cover the "empty radio button text" edge test case 😃
  • Added additional test cases such that it handles for cases when there is an empty radio button text with lowercase & uppercase, (x) & (X) respectively

Anything you'd like to highlight/discuss:
As @gerteck mentioned in the issue,

The plugin attempts to access tokens[i-5].content and tokens[i-4].content without checking if those tokens exist (lines 24-26 in markdown-it-radio-button.js)

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)

  • Kindly seeking clarification as for why the .indexOf checks 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.)
  • Hence, for now, I have decided to add the additional checks for the entire content so that I can preserve this logic in case I'm missing something.

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 by function isTodoItem(token, index) and its logic is somewhat repeated in function 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 makeRadioButton dependent on startsWithTodoMarkdown too. 🫤

Interested to hear your thoughts on this matter too! 😀

See resolved example

e.g. when serving this markdown file

<frontmatter>
  layout: default.md
  title: Hello World
  pageNav: 1
  pageNavTitle: "Chapters of This Page"
</frontmatter>

- ( ) Item 1
- ( ) Item 2
- (x)
- ( )

Hello world

Example of whats generated:
image

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Fix markdown-it radio button unsafe token access

Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

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):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

@codecov
Copy link

codecov bot commented Jan 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.72%. Comparing base (28b407f) to head (cd733dc).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2790      +/-   ##
==========================================
+ Coverage   71.67%   71.72%   +0.05%     
==========================================
  Files         134      134              
  Lines        7284     7297      +13     
  Branches     1514     1614     +100     
==========================================
+ Hits         5221     5234      +13     
  Misses       1936     1936              
  Partials      127      127              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@MoshiMoshiMochi MoshiMoshiMochi changed the title Bug radio button unsafe token access Fix radio button unsafe token access bug Jan 19, 2026
@gerteck gerteck requested a review from Copilot January 19, 2026 14:40

This comment was marked as outdated.

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.

Markdown-It Radio Button Plugin Crashes Due to Unsafe Token Access

1 participant