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
"safe" option #253
Comments
It should be number 2: the resulting HTML output will be safe because any raw HTML in the input will be escaped. This option defaults to
I'm open to any alternate names or other feedback on this. |
👍 sounds good, but would I'm going a bit off topic but is there any spec on the behavior when
Expected:
Actual:
In the documents I'm processing it's common for users to just write code in text (not use backticks), I'm guessing I might not be the only one? Would it make sense to have 3 behaviors:
or is there a reason not to? |
I totally agree with you. IMO there are two questions that need to be answered:
I'd happily make the proposed change if I was 100% convinced the answer to both questions is "yes", but unfortunately I'm not. Perhaps the README and documentation should make this configuration highly visible so users are aware of it? An alternative solution might be raising some kind of if (!isset($options['safe']) && !isset($options['allow_raw_html'])) {
trigger_error('Failing to set the "allow_raw_html" option could make your site vulnerable to XSS attacks. See http://commonmark.thephpleague.com/configuration#allow_raw_html for more information.', E_USER_NOTICE);
} That link would describe the option and which value to choose. While this technically is a BC break, it's not nearly as severe. It also causes the user to explicitly state what functionality they want. Thoughts?
Yeah I really like that! What about this: a new configuration option
(Naming things is hard...) If you like the if (isset($options['safe'])) {
trigger_error('The "safe" option has been deprecated, use "html_input" instead. See http://commonmark.thephpleague.com/configuration#html_input for more information.', E_USER_DEPRECATED);
$options['html_input'] = $options['safe'] ? 'strip' : 'preserve';
} elseif (!isset($options['html_input'])) {
trigger_error('Failing to set the "html_input" option could make your site vulnerable to XSS attacks. See http://commonmark.thephpleague.com/configuration#html_input for more information.', E_USER_NOTICE);
$options['html_input'] = 'escape';
} Thoughts? |
Agreed, documenting it more visibly might be a good first step. Errors can be a pain to deal with, I'm afraid it would be a burden to some users. An |
That would be amazing!
Search the codebase for There are two renders which are responsible for converting HTML AST nodes into the final output - right now they simply I haven't yet thought about the best location to implement the BTW it looks like
I apologize for not having the time to provide better, more-coherent information but perhaps later tonight or tomorrow I can do that. |
As you said there's also the "safe links" behavior (only allow safe links), and I'm not sure an |
Yeah that makes sense to me. On Tue, Jun 28, 2016 at 3:06 PM Matthieu Napoli notifications@github.com
|
Fixes thephpleague#253. The new `html_input` option allows 3 different behaviors: - `strip` HTML input - `allow` HTML input - `escape` HTML input
See thephpleague#253 and thephpleague#255. The `safe` option is now deprecated in favor of the `allow_unsafe_links` and `html_input` options. The `allow_unsafe_links` is `true` by default for BC reasons.
I think I'm confused with the
safe
option: if set totrue
, does that mean:I think I accidentally misinterpreted that and ended up opening XSS on my application. I'm afraid it might be wrongly interpreted by others. Would it make sense to rename that option (keeping BC of course) to something very explicit?
Cheers!
The text was updated successfully, but these errors were encountered: