stop parsing YAML headers as ERB #31362
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
breville
reviewed
Oct 22, 2019
| theme: with_title | ||
| --- | ||
|
|
||
| # Hello |
There was a problem hiding this comment.
Maybe worth a comment mentioning that this is used in a test, and which test that is?
breville
reviewed
Oct 22, 2019
| # once we started allowing translators to translate entire files. | ||
| # | ||
| # This tests exists just to enforce that we don't revert back to the old | ||
| # functionality. |
breville
approved these changes
Oct 22, 2019
7 tasks
Hamms
added a commit
that referenced
this pull request
Oct 23, 2019
As it turns out, emails don't use the rendering logic I updated in #31362 and so don't need to stop using ERB in their YAML header just yet; they use https://github.com/code-dot-org/code-dot-org/blob/staging/lib/cdo/pegasus/text_render.rb, specifically https://github.com/code-dot-org/code-dot-org/blob/8bfb480e91fec4a6e39d3727634d26f85b73f4df/lib/cdo/pegasus/text_render.rb#L190-L205 which still supports ERB (see https://github.com/code-dot-org/code-dot-org/blob/8bfb480e91fec4a6e39d3727634d26f85b73f4df/bin/cron/deliver_poste_messages#L63-L74 for more details). In addition, they actually only define @Header if one is detected, so the change to assign metadata directly to that variable was failing because it never got initialized. Reverting these templates to their old format fixes things for now, and I'll track longer-term work to update YamlEngine
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Previously, all the YAML headers that we allow in all our pegasus documents could be treated as ERB. This not only led to some very messy scenarios (like this HAML template containing a YAML header containing an ERB expression), but also began to present a security risk once we started to allow translators to translate full files.
The fix is to simply parse YAML only as YAML and not as both YAML and ERB. Any functionality that formerly lived as ruby code in the headers can instead live as ruby code in the ERB and HAML templates themselves for those filetypes; filetypes that do not themselves allow for execution of ruby code will no longer be able to support the old functionality.
Links
Testing story
I added a test to ensure that we don't in the future revert back to the old functionality and explaining our reasoning. I'm also counting on our various "render every document in Pegasus and verify that none of them break" tests.
Reviewer Checklist: