Skip to content
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.

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

fix(gatsby-plugin-mdx): don't allow JS frontmatter by default #35830

Merged
merged 1 commit into from Jun 2, 2022

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Jun 2, 2022

Description

Currently gatsby-plugin-mdx by default supports the "JS engine" for frontmatter. The example syntax for it is:

---js
{
  foo: require(`fs`).readFileSync('something', 'utf-8')
}
---

Your regular MDX body

This was never intended default behavior because in some cases it can open up an attack vector for remote code execution (on the build server). As long as sourced content is secure and actually owned by same party as site owner, this doesn't cause problems, but we should be explicit with this, so we disable it by default with option to re-enable it (if someone actually relied on this unintended "feature")

In new default mode ( JS frontmatter engine disabled), we will show warning if there is any content that use JS frontmatter that this will not be processed/executed:

Screenshot 2022-06-02 at 18 55 18

Because this is security risk we will continously show warning as reminder that it might not be safe to use it (it's not guaranteed it's not safe - context matter a lot):
Screenshot 2022-06-02 at 18 58 04

Documentation

Small stub added to README.md about new plugin option referencing not yet published advisory with some details - this could likely be made nicer, but for the sake if getting fix out, this will have to do for now.

Co-authored-by: Ty Hopp <hopp.ty.c@gmail.com>
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 2, 2022
@pieh pieh added topic: remark/mdx Related to Markdown, remark & MDX ecosystem and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 2, 2022
@mlgualtieri mlgualtieri self-requested a review Jun 2, 2022
@pieh pieh merged commit b3690fb into master Jun 2, 2022
32 of 36 checks passed
@pieh pieh deleted the fix/mdx-js-frontmatter-vuln branch Jun 2, 2022
@pieh pieh added this to To cherry-pick in V3 Release Hotfixes via automation Jun 2, 2022
@pieh pieh added this to To cherry-pick in Release candidate via automation Jun 2, 2022
@pieh pieh added this to To cherry-pick in V4 Release hotfixes via automation Jun 2, 2022
pieh added a commit that referenced this issue Jun 2, 2022
Co-authored-by: Ty Hopp <hopp.ty.c@gmail.com>
(cherry picked from commit b3690fb)
pieh added a commit that referenced this issue Jun 2, 2022
Co-authored-by: Ty Hopp <hopp.ty.c@gmail.com>
(cherry picked from commit b3690fb)
pieh added a commit that referenced this issue Jun 2, 2022
Co-authored-by: Ty Hopp <hopp.ty.c@gmail.com>
(cherry picked from commit b3690fb)
@pieh pieh moved this from To cherry-pick to Backport PR opened in Release candidate Jun 2, 2022
@pieh pieh moved this from To cherry-pick to Backport PR opened in V4 Release hotfixes Jun 2, 2022
@pieh pieh moved this from To cherry-pick to Backport PR opened in V3 Release Hotfixes Jun 2, 2022
@pieh pieh moved this from Backport PR opened to Backported in Release candidate Jun 2, 2022
pieh added a commit that referenced this issue Jun 2, 2022
#35833)

Co-authored-by: Ty Hopp <hopp.ty.c@gmail.com>
Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
pieh added a commit that referenced this issue Jun 2, 2022
#35834)

Co-authored-by: Ty Hopp <hopp.ty.c@gmail.com>
Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
pieh added a commit that referenced this issue Jun 2, 2022
#35832)

Co-authored-by: Ty Hopp <hopp.ty.c@gmail.com>
Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
@mosesoak
Copy link

mosesoak commented Jun 3, 2022

@pieh @mlgualtieri Thanks for this addition. Wanted to let you know that the link to the security issue is a Github 404.

GHSA-mj46-r4gr-5x83

It appears 3 times in this PR.

@mlgualtieri
Copy link
Contributor

mlgualtieri commented Jun 3, 2022

Thanks @mosesoak! It was not an oversight, but we were waiting on GitHub to assign a CVE to the issue before publishing the advisory. But, it doesn't appear that this will happen today so I went ahead and published the advisory. The link should work for you now.

@pieh pieh moved this from Backport PR opened to Published in V3 Release Hotfixes Jun 3, 2022
@pieh pieh moved this from Backport PR opened to Published in V4 Release hotfixes Jun 3, 2022
@karlhorky
Copy link
Contributor

karlhorky commented Jun 4, 2022

@pieh after upgrading from gatsby-plugin-mdx@3.15.1 to gatsby-plugin-mdx@3.15.2, we're seeing our Frontmatter code show up in the content of the page 😱

Screen Shot 2022-06-04 at 20 14 57

This appears regardless of whether we have the JSFrontmatterEngine set to true or unconfigured...

@karlhorky
Copy link
Contributor

karlhorky commented Jun 4, 2022

Edit: this only works for gatsby develop, see below for a solution for gatsby build

It seems like commenting out line 187 with options prevents this from happening:

let code = content
// after running mdx, the code *always* has a default export, so this
// check needs to happen first.
if (!hasDefaultExport(content, options) && !!defaultLayout) {
debug(`inserting default layout`, defaultLayout)
const { content: contentWithoutFrontmatter, matter } = grayMatter(
content,
options
)
code = `${matter ? matter : ``}
import DefaultLayout from "${slash(defaultLayout)}"
export default DefaultLayout
${contentWithoutFrontmatter}`

Also, it seems like the options being passed to gray-matter on this line don't match the valid gray-matter options at all... They are Gatsby config options.


Edit: Apparently commenting out the above line only works in gatsby develop.

A solution for gatsby build (SSG) requires:

  1. Commenting out passing all options to gray-matter in the mdx-loader.js, utils/mdx.js, utils/gen-mdx.js files (using patch-package)
  2. And then ALSO enabling the JSFrontmatterEngine option

Super strange, wonder what's causing this Frontmatter to show up in the content.


@pieh have any idea what's happening here? Or do you think that I should open a new issue?

@marvinjude marvinjude moved this from Backported to Archived in Release candidate Jun 7, 2022
@karlhorky
Copy link
Contributor

karlhorky commented Jun 9, 2022

Ok @pieh I created a new issue over here: #35901 - just need to get around to creating a reproduction...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: remark/mdx Related to Markdown, remark & MDX ecosystem
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants