-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
!!![TASK] Breaking: Disallow raw directive #943
Conversation
Using the raw directive is a potencial securtiy risk. releases: main, 1.0
f532bf4
to
fbad95c
Compare
/** {@inheritDoc} */ | ||
public function process( | ||
BlockContext $blockContext, | ||
Directive $directive, | ||
): Node|null { | ||
return new RawNode(implode("\n", $blockContext->getDocumentIterator()->toArray())); | ||
public function processAction(BlockContext $blockContext, Directive $directive): void | ||
{ | ||
$this->logger->error('The raw directive is not supported for security reasons. ', $blockContext->getLoggerInformation()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% convinced here. I see a security issue when using reStructured Text for "user" input. For projects like the Symfony docs, where the only parsed docs are text maintained by the doc team, there is no security issue as far as I'm aware. I guess this applies to most usages of Guides.
Implementing the raw node also seems to be non-trivial. One needs to learn how to create a custom Guides extension and how to create a custom directive.
If we want this, I think we should do 2 things:
- Be more explicit about the security issue the raw node causes, so users can make informed decisions.
- Add a config option to disable the raw node, like docutils also has.
We then have to think about the default, docutils defaults to true
which we might want to follow for compatibility reasons. Or we can decide to default to false
if we think the security issues are common enough to protect users against.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not remove this directive from the code base but allow a config option to enable it. Or log the error when disabled. By making it an option people could still overwrite the behavior using configuration. but this could be enforced by a listener in an extension to ensure people cannot overwrite the configuration.
The security risk here is on the typo3 side, as the documentation is published on their website, but maintained by approved third-party developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The security risk here is on the typo3 side, as the documentation is published on their website, but maintained by approved third-party developers.
We have a similar situation for some community bundles where we render their docs in https://symfony.com/bundles, but we need the raw directive for the main Symfony docs. So a config option would be useful for us, we render the bundles section separately from the main docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when you have to use the raw directive that means, that there is something missing in your pipline. It is also very hard to change such HTML output automatically lateron. So I would rather suggest to write specialized directives for your use cases...
I will make another PR |
Using the raw directive is a potencial securtiy risk.
resolves:#942
releases: main, 1.0