-
Notifications
You must be signed in to change notification settings - Fork 862
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
kwargs are not checked for unexpected parameters #1238
Comments
Over the years we have changed the list of accepted keys. When a key is deprecated, we will raise a warning for a few versions, then remove it. However, we can't do that when adding a key. The point of accepting In fact, sometimes it can be useful. For example, in #816 we are passing all kwargs to the Pygments lexer. However, the lexer for each language has a different set of keys (most have none at all). Because Pygments' lexers don't error on unknown keys, it works correctly. If they did raise errors, we would need to add complexity to our config to clearly define which options go with which lexers. I realize that is not calling |
Hmm, so with Pygments and such you're basically stuck with it being a mostly-blind passthrough. To give an example of the safety concerns, there's a Python JWT library that accepts an "algorithms" keyword. If it isn't specified, tokens are allowed to declare what algorithm they're using, which is a severe vulnerability (both in the library, and the spec). But compounding this, the decode() function takes a kwargs and passes it to a shared utility without further validation. I've seen someone pass Again, I'm not aware of a specific vulnerability like that in Python-Markdown, but I did want to give an example of the kind of trouble this pattern can bring. In my case it was just about 15 minutes of confusion, but warnings on unknown keys would have cleared it up very quickly. So there's also an argument to be made from the developer experience side. |
return markdown.markdown(post, output='html5')
For a while I had the code
return markdown.markdown(post, output='html5')
, which seemed to be working OK. However, it turns out that was a typo -- I should have been usingoutput_format
. Normally, the runtime would catch this, but instead**kwargs
are collected and passed to theMarkdown
class, where keys are retrieved as needed.It's not a security issue in this library, as far as I can tell, but this pattern has lead to security issues elsewhere. (Imagine if there were a
safe_output
kwarg that someone typo'd.)I think this could be as simple as having a known-keys set that the kwargs dict's keys are checked against before processing. I'd be happy to contribute a PR if this would be an acceptable approach.
The text was updated successfully, but these errors were encountered: