Fix radio button unsafe token access bug #2790

MoshiMoshiMochi · 2026-01-19T13:56:04Z

What is the purpose of this pull request?

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:

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 · 2026-01-19T13:58:55Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.70%. Comparing base (a12f57a) to head (c046805).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2790      +/-   ##
==========================================
+ Coverage   71.67%   71.70%   +0.03%     
==========================================
  Files         134      134              
  Lines        7286     7294       +8     
  Branches     1609     1587      -22     
==========================================
+ Hits         5222     5230       +8     
- Misses       1937     2018      +81     
+ Partials      127       46      -81

☔ 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.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

gerteck · 2026-01-19T16:26:09Z

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.

If I understand it correctly, your change parses empty (checked or unchecked) radios

- ( )
- (x)

to be recognized and rendered, while the previous implementation does not do so.

I think we should stick to the old implementation for the following reasons:

the change creates divergence between checkbox - [ ] and radio button - ( ) behavior. (- [ ] does not create an empty checkbox in markbind or github comments)
Why not update checkbox behavior as well? Since MarkBind docs states it provides GitHub-Flavored Markdown, let's stick to how GitHub supports it (i.e. no checkboxes without text -> no radio without text, also probably no use case for checkboxes and radios w/o text anyway)
Technically, it causes a breaking change, which we try to avoid as we break existing user's code, unless there is strong justification to do so.

See:

- [ ] hello
- [ ]

hello
[ ]

Edit:
Apologies, it was possibly the testcase previously that I added that threw you off, can delete that testcase 🙏

gerteck

lgtm with some small changes

packages/core/src/lib/markdown-it/plugins/markdown-it-radio-button.js

gerteck · 2026-01-19T16:36:17Z

packages/core/src/lib/markdown-it/plugins/markdown-it-radio-button.js

  var radio = new TokenConstructor('html_inline', '', 0);
  var disabledAttr = disableRadio ? ' disabled="" ' : '';
-  if (token.content.indexOf('( ) ') === 0) {
+


Let's not cause a change in functionality (see comment in main thread)

I think the refactoring is good (naming checked and unChecked), can keep that.

Probably can consider using const instead of var, var is legacy and shouldn't be used anymore.

Though if you do might as well refactor the whole file which is legacy but that would go beyond the scope of the PR, and you culd and should probably do another PR for it. Tons of refactoring improvement possibilities:

Use startsWith instead of indexOf === 0

change all var to const

use template literatls instead of string concat
etc etc

Maybe tackle broader refactoring if you wish, in another PR

Noted! Shall clean this up and work on refactoring in another PR in the future.

packages/core/src/lib/markdown-it/plugins/markdown-it-radio-button.js

packages/core/test/unit/lib/markdown-it/plugins/markdown-it-radio-button.test.ts

MoshiMoshiMochi · 2026-01-20T03:14:27Z

As requested, I have reverted the logic changes in markdown & removed the redundant test cases

Kindly see the previously used example.

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

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

Hello world

gerteck

LGTM, thanks for the quick update!

github-actions · 2026-01-20T03:33:05Z

Welcome, @MoshiMoshiMochi! 🎉 Thank you for your contribution to the MarkBind project!

@gerteck, please remember to add @MoshiMoshiMochi as an official contributor to our repository.

See the full list of contributors here. ✨

gerteck · 2026-01-20T03:33:51Z

@all-contributors please add MoshiMoshiMochi for code

allcontributors · 2026-01-20T03:34:08Z

@gerteck

I've put up a pull request to add @MoshiMoshiMochi! 🎉

MoshiMoshiMochi added 3 commits January 19, 2026 18:05

Fix empty text case

59711b6

Fix test cases name

7eadcba

Add test cases & remove redundant initial fix

cd733dc

MoshiMoshiMochi changed the title ~~Bug radio button unsafe token access~~ Fix radio button unsafe token access bug Jan 19, 2026

gerteck requested a review from Copilot January 19, 2026 14:40

Copilot started reviewing on behalf of gerteck January 19, 2026 14:40 View session

This comment was marked as outdated.

Sign in to view

gerteck requested a review from Copilot January 19, 2026 15:14

Copilot started reviewing on behalf of gerteck January 19, 2026 15:15 View session

Copilot AI reviewed Jan 19, 2026

View reviewed changes

gerteck requested changes Jan 19, 2026

View reviewed changes

MoshiMoshiMochi added 2 commits January 20, 2026 10:58

Revert logic to old implementation

13355c3

Removed redundant test cases

333ef1d

MoshiMoshiMochi requested a review from gerteck January 20, 2026 03:14

Merge branch 'master' into bug-radio-button-unsafe-token-access

c046805

gerteck approved these changes Jan 20, 2026

View reviewed changes

gerteck merged commit 24cbff7 into MarkBind:master Jan 20, 2026
11 checks passed

github-actions bot added the r.Patch Version resolver: increment by 0.0.1 label Jan 20, 2026

allcontributors bot mentioned this pull request Jan 20, 2026

add MoshiMoshiMochi as a contributor for code #2791

Merged

Fix radio button unsafe token access bug #2790

Fix radio button unsafe token access bug #2790

Conversation

MoshiMoshiMochi commented Jan 19, 2026 • edited by gerteck Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

gerteck commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gerteck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gerteck Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

MoshiMoshiMochi Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MoshiMoshiMochi commented Jan 20, 2026

Uh oh!

gerteck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jan 20, 2026

Uh oh!

gerteck commented Jan 20, 2026

Uh oh!

allcontributors bot commented Jan 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MoshiMoshiMochi commented Jan 19, 2026 •

edited by gerteck

Loading

codecov bot commented Jan 19, 2026 •

edited

Loading

gerteck commented Jan 19, 2026 •

edited

Loading