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

yaml.safeLoad() #65

Closed
peterbe opened this issue May 13, 2020 · 6 comments · Fixed by #67
Closed

yaml.safeLoad() #65

peterbe opened this issue May 13, 2020 · 6 comments · Fixed by #67

Comments

@peterbe
Copy link
Contributor

peterbe commented May 13, 2020

yaml.load() is insecure and dangerous. (first google result)
We should be using yaml.safeLoad().

@peterbe
Copy link
Contributor Author

peterbe commented May 13, 2020

I don't know much about it but mdify uses yaml.safeLoad.

@peterbe
Copy link
Contributor Author

peterbe commented May 13, 2020

I tried to write a quick patch but I noticed that one of the examples uses un-"safe" YAML which would break if you use yaml.safeLoad().
So I guess it would need to be something like this:

fm(payload, { yaml: 'unsafe' })

if you need to support unsafe Yaml parsing.

@jxson
Copy link
Owner

jxson commented May 13, 2020

I don't mind changing the example to make the code more secure. If you submit a PR I will publish it as a new major version to prevent breaking folks who relied on the old functionality.

I like your idea of passing in an option to toggle safe/unsafe and defaulting to safe parsing.

@ajinabraham
Copy link

ajinabraham commented May 15, 2020

This is a widely used library. If it is not required, please switch to safeLoad() otherwise warn people not to run fm with untrusted user input.

A PoC
example.md

---
toString: !<tag:yaml.org,2002:js/function>"function(){console.log(1)}"
description: Nothing to see here 
---

test.js

var fs = require('fs')
  , fm = require('front-matter')

fs.readFile('./example.md', 'utf8', function(err, data){
  if (err) throw err
  var content = fm(data)
  var x = content.attributes + 'Hello';
})
$ node test.js
1

@peterbe peterbe mentioned this issue May 15, 2020
@jxson
Copy link
Owner

jxson commented May 18, 2020

@ajinabraham ACK, I think the PR from @peterbe will cover your asks.

@ajinabraham
Copy link

PR looks good to me 👍

@jxson jxson closed this as completed in 1d9094f May 19, 2020
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 a pull request may close this issue.

3 participants