Skip to content

Commit 0032a7a

Browse files
authored
fix: disable javascript execution in frontmatter (#1396)
## 🧰 Changes Javascript execution in frontmatter could lead to security problems, so disable it ## 🧬 QA & Testing See tests. I verified that they fail without this patch applied: ``` FAIL __tests__/lib/frontmatter.test.ts > #readPage > should not execute JavaScript in frontmatter (RCE prevention) AssertionError: expected { Object (title, malicious) } to deeply equal {} - Expected + Received - {} + { + "malicious": "executed", + "title": "Test", + } + + ```
1 parent 2884298 commit 0032a7a

File tree

2 files changed

+68
-1
lines changed

2 files changed

+68
-1
lines changed

__tests__/lib/frontmatter.test.ts

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import DocsUploadCommand from '../../src/commands/docs/upload.js';
1313
import RefUploadCommand from '../../src/commands/reference/upload.js';
1414
import { fix, writeFixes } from '../../src/lib/frontmatter.js';
1515
import { emptyMappings, fetchSchema } from '../../src/lib/readmeAPIFetch.js';
16+
import { readPage } from '../../src/lib/readPage.js';
1617
import { setupOclifConfig } from '../helpers/oclif.js';
1718

1819
describe.each([
@@ -350,3 +351,63 @@ describe('#writeFixes', () => {
350351
});
351352
});
352353
});
354+
355+
describe('#readPage', () => {
356+
let command: DocsUploadCommand;
357+
358+
beforeEach(async () => {
359+
const oclifConfig = await setupOclifConfig();
360+
command = new DocsUploadCommand([], oclifConfig);
361+
});
362+
363+
afterEach(() => {
364+
vi.restoreAllMocks();
365+
});
366+
367+
it('should not execute JavaScript in frontmatter', () => {
368+
const maliciousContent = `---js
369+
{
370+
title: "Test",
371+
malicious: (function() {
372+
// This should never execute
373+
return "executed";
374+
})()
375+
}
376+
---
377+
378+
# Page content`;
379+
380+
const testFilePath = 'test-malicious.md';
381+
382+
// Mock the file system to return our malicious content
383+
vi.spyOn(fs, 'readFileSync').mockReturnValue(maliciousContent);
384+
385+
const result = readPage.call(command, testFilePath);
386+
387+
// The data should be empty because we disabled the javascript engine
388+
// by providing a parse function that returns an empty object
389+
expect(result.data).toEqual({});
390+
expect(result.content).toContain('# Page content');
391+
});
392+
393+
it('should still parse regular YAML frontmatter', () => {
394+
const normalContent = `---
395+
title: "Normal Page"
396+
category: some-category
397+
---
398+
399+
# Page content`;
400+
401+
const testFilePath = 'test-normal.md';
402+
403+
vi.spyOn(fs, 'readFileSync').mockReturnValue(normalContent);
404+
405+
const result = readPage.call(command, testFilePath);
406+
407+
expect(result.data).toEqual({
408+
title: 'Normal Page',
409+
category: 'some-category',
410+
});
411+
expect(result.content).toContain('# Page content');
412+
});
413+
});

src/lib/readPage.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,13 @@ export function readPage(
6060
// by default, grayMatter maintains a buggy cache with the page data,
6161
// so we pass an empty object as second argument to avoid it entirely
6262
// (so far we've seen this issue crop up in tests)
63-
const matter = grayMatter(rawFileContents, {});
63+
const matter = grayMatter(rawFileContents, {
64+
// disable the javascript engine, which is a built-in RCE
65+
//
66+
// * @see https://github.com/readmeio/rdme/security/advisories/GHSA-f65r-8r74-m6v5
67+
// * @see https://github.com/jonschlinkert/gray-matter/issues/131#issuecomment-3566662412
68+
engines: { javascript: { parse: () => ({}) } },
69+
});
6470
const { content, data } = matter;
6571
this.debug(`frontmatter for ${filePath}: ${JSON.stringify(matter)}`);
6672

0 commit comments

Comments
 (0)