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

Only allow email attachments inside of EMAIL_ATTACHMENT_ROOT #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gavinwahl
Copy link
Member

This can limit the scope of vulnerabilities created by incorrect escaping in YAML templates. This could be backwards incompatible if you are including attachments outside of EMAIL_ATTACHMENT_ROOT, but I don't think anyone is doing this.

This can limit the scope of vulnerabilities created by incorrect
escaping in YAML templates.
@acatton
Copy link
Contributor

acatton commented Jun 14, 2013

I feel like this is sweeping the problem under the rug. The real problem here is that we have to escape everything in the email header. I'm working on a more general fix.

@davesque

@gavinwahl
Copy link
Member Author

It has to be escaped by you in the template -- templating YAML is difficult. There's not an automatic way to escape something you're going to embed in YAML.

@acatton
Copy link
Contributor

acatton commented Jun 14, 2013

After going through the code and trying to fix the way I want. I don't see any simple solution not breaking backward compatibility. I thing this is mergable.

But still, putting yaml like that can lead to issues. Even if we use |escapejs filter, having a colon sign will raise a YAML parsing error.

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 this pull request may close these issues.

2 participants