Skip to content

Fix radio button unsafe token access bug #2790

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.

Sign up for GitHub

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

Merged
gerteck merged 6 commits into from
Jan 20, 2026
Merged

Fix radio button unsafe token access bug #2790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
59711b6
Fix empty text case
MoshiMoshiMochi Jan 19, 2026
7eadcba
Fix test cases name
MoshiMoshiMochi Jan 19, 2026
cd733dc
Add test cases & remove redundant initial fix
MoshiMoshiMochi Jan 19, 2026
13355c3
Revert logic to old implementation
MoshiMoshiMochi Jan 20, 2026
333ef1d
Removed redundant test cases
MoshiMoshiMochi Jan 20, 2026
c046805
Merge branch 'master' into bug-radio-button-unsafe-token-access
gerteck Jan 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions packages/core/src/lib/markdown-it/plugins/markdown-it-radio-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,14 @@ module.exports = function(md, options) {
if (group) {
group = group[1];
} else {
group = crypto.createHash('md5')
.update(tokens[i-5].content)
.update(tokens[i-4].content)
.update(tokens[i].content).digest('hex').substr(2, 5); // generate a deterministic group id
var hash = crypto.createHash('md5');
if (i >= 5 && tokens[i-5]) {
hash.update(tokens[i-5].content);
}
if (i >= 4 && tokens[i-4]) {
hash.update(tokens[i-4].content);
}
group = hash.update(tokens[i].content).digest('hex').substr(2, 5); // generate a deterministic group id
}
radioify(tokens[i], state.Token, group);
attrSet(tokens[i-2], 'class', 'radio-list-item');
Expand Down Expand Up @@ -86,9 +90,13 @@ function radioify(token, TokenConstructor, radioId) {
function makeRadioButton(token, TokenConstructor, radioId) {
var radio = new TokenConstructor('html_inline', '', 0);
var disabledAttr = disableRadio ? ' disabled="" ' : '';
if (token.content.indexOf('( ) ') === 0) {

Copy link
Member

@gerteck gerteck Jan 19, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@MoshiMoshiMochi MoshiMoshiMochi Jan 20, 2026

Choose a reason for hiding this comment

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

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

const isUnchecked = token.content.indexOf('( ) ') === 0;
const isChecked = token.content.indexOf('(x) ') === 0 ||
token.content.indexOf('(X) ') === 0;
if (isUnchecked) {
radio.content = '<input class="radio-list-input" name="' + radioId + '"' + disabledAttr + 'type="radio">';
} else if (token.content.indexOf('(x) ') === 0 || token.content.indexOf('(X) ') === 0) {
} else if (isChecked) {
radio.content = '<input class="radio-list-input" checked="" name="' + radioId + '"' + disabledAttr + 'type="radio">';
}
return radio;
Expand Down
33 changes: 12 additions & 21 deletions packages/core/test/unit/lib/markdown-it/plugins/markdown-it-radio-button.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ describe('markdown-it-radio-button plugin', () => {
});

describe('basic radio button syntax', () => {
// TODO: Fix plugin bug (issue #2749)
test.skip('should convert basic unchecked radio button syntax - disable till fix (#2749)', () => {
test('should convert basic unchecked radio button syntax', () => {
const source = [
'- ( ) Option 1',
'- ( ) Option 2',
Expand All @@ -28,7 +27,7 @@ describe('markdown-it-radio-button plugin', () => {
expect(result).not.toContain('checked=""');
});

test.skip('should convert checked radio button with lowercase x - disable till fix (#2749)', () => {
test('should convert checked radio button with lowercase x', () => {
const source = [
'- (x) Selected option',
'- ( ) Unselected option',
Expand All @@ -42,7 +41,7 @@ describe('markdown-it-radio-button plugin', () => {
expect(result.match(/checked=""/g)).toHaveLength(1);
});

test.skip('should convert checked radio button with uppercase X - disable till fix (#2749)', () => {
test('should convert checked radio button with uppercase X', () => {
const source = [
'- (X) Selected option',
'- ( ) Unselected option',
Expand All @@ -54,7 +53,7 @@ describe('markdown-it-radio-button plugin', () => {
expect(result.match(/checked=""/g)).toHaveLength(1);
});

test.skip('should handle single radio button - disable till fix (#2749)', () => {
test('should handle single radio button', () => {
const source = '- ( ) Single option';
const result = md.render(source);

Expand All @@ -63,14 +62,6 @@ describe('markdown-it-radio-button plugin', () => {
expect(result).toContain('class="radio-list-input"');
});

test.skip('should handle empty radio button text - disable till fix (#2749)', () => {
const source = '- ( )';
const result = md.render(source);

expect(result).toContain('type="radio"');
expect(result).toContain('class="radio-list-input"');
});

// This test works because it provides enough token context
test('should handle different lists with different group IDs', () => {
const source = [
Expand Down Expand Up @@ -98,7 +89,7 @@ describe('markdown-it-radio-button plugin', () => {
});

describe('radio button grouping and IDs', () => {
test.skip('should use same group ID for radio buttons in same list - disable till fix (#2749)', () => {
test('should use same group ID for radio buttons in same list', () => {
const source = [
'- ( ) Option 1',
'- ( ) Option 2',
Expand All @@ -114,7 +105,7 @@ describe('markdown-it-radio-button plugin', () => {
expect(matches[1][1]).toBe(matches[2][1]);
});

test.skip('should generate deterministic group IDs - disable till fix (#2749)', () => {
test('should generate deterministic group IDs', () => {
const source = [
'- ( ) Option 1',
'- ( ) Option 2',
Expand Down Expand Up @@ -161,7 +152,7 @@ describe('markdown-it-radio-button plugin', () => {
});

describe('plugin configuration options', () => {
test.skip('should wrap radio buttons in labels when label option is true - disable (#2749)', () => {
test('should wrap radio buttons in labels when label option is true', () => {
const source = '- ( ) Option with label';
const result = md.render(source);

Expand All @@ -170,7 +161,7 @@ describe('markdown-it-radio-button plugin', () => {
expect(result).toContain('type="radio"');
});

test.skip('should not wrap in labels when label option is false - disable till fix (#2749)', () => {
test('should not wrap in labels when label option is false', () => {
const mdNoLabel = markdownIt();
mdNoLabel.use(markdownItTaskLists, { enabled: true });
mdNoLabel.use(radioButtonPlugin, { enabled: true, label: false });
Expand All @@ -182,7 +173,7 @@ describe('markdown-it-radio-button plugin', () => {
expect(result).toContain('type="radio"');
});

test.skip('should disable radio buttons when enabled is false - disable till fix (#2749)', () => {
test('should disable radio buttons when enabled is false', () => {
const mdDisabled = markdownIt();
mdDisabled.use(markdownItTaskLists, { enabled: true });
mdDisabled.use(radioButtonPlugin, { enabled: false, label: true });
Expand Down Expand Up @@ -225,7 +216,7 @@ describe('markdown-it-radio-button plugin', () => {
});
});

describe.skip('content formatting and nested structures - disable till fix (#2749)', () => {
describe.skip('content formatting and nested structures', () => {
test('should handle rich content in radio button text', () => {
const source = [
'- ( ) Option with **bold** text',
Expand Down Expand Up @@ -318,7 +309,7 @@ describe('markdown-it-radio-button plugin', () => {
});
});

test.skip('should handle very long radio button text - disable till fix (#2749)', () => {
test('should handle very long radio button text', () => {
const longText = 'This is a very long radio button option that spans multiple words'
+ 'and contains lots of text to test how the plugin handles lengthy content';
const source = `- ( ) ${longText}`;
Expand All @@ -328,7 +319,7 @@ describe('markdown-it-radio-button plugin', () => {
expect(result).toContain(longText);
});

test.skip('should handle radio buttons with line breaks - disable till fix (#2749)', () => {
test('should handle radio buttons with line breaks', () => {
const source = [
'- ( ) Option with',
' continuation on next line',
Expand Down