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

Update handlebars to version 4.7.1 #2802

Closed
znarf opened this issue Jan 13, 2020 · 15 comments · Fixed by opencollective/opencollective-api#3693
Closed

Update handlebars to version 4.7.1 #2802

znarf opened this issue Jan 13, 2020 · 15 comments · Fixed by opencollective/opencollective-api#3693
Assignees
Labels
api Issues that require some work on the API (https://github.com/opencollective/opencollective-api) bounty complexity → simple technical-debt Deprecated code to migrate and other necessary refactors $200 simple complexity bounty (v2)

Comments

@znarf
Copy link
Member

znarf commented Jan 13, 2020

See: opencollective/opencollective-api#3176

handlebars has some breaking changes in version 4.6, this is related to security issues.

See the comment from @Betree:

Let's wait, the new runtime options will allow us to stay up to date with handlebars. But we should create a follow-up issue to see why we can't access our variables in emails templates with this new config and eventually whitelist the props if that's what is needed.

And the follow up comment from the library author:

Handlebars 4.7.0 has been release with options to disable prototype restrictions:
https://handlebarsjs.com/api-reference/runtime-options.html#options-to-control-prototype-access. Please be aware the you are potentially opening up ways to crash your servers or maybe even remote code execution for people who can inject templates into your system.

We now want to upgrade in a way that is taking care of security concerns.

Warning: Before submitting a contribution, make sure you understand the security aspects and you take care of it in your implementation.

@znarf znarf added technical-debt Deprecated code to migrate and other necessary refactors api Issues that require some work on the API (https://github.com/opencollective/opencollective-api) complexity → simple bounty candidate Potential bounty, to be reviewed. Having this tag on an issue does not qualify it for a bounty. labels Jan 13, 2020
@znarf znarf added the osca label Feb 20, 2020
@znarf
Copy link
Member Author

znarf commented Feb 21, 2020

A $200 bounty was attached to this issue. Anyone submitting a Pull Request will be rewarded with $200 when the Pull Request is reviewed, accepted and merged. More info

This bounty is currently reserved to participants of the Open Source Festival event. If not completed there, it will be later open to everyone.

@znarf znarf added the $200 simple complexity bounty (v2) label Feb 21, 2020
@Oghenebrume50

This comment has been minimized.

@znarf

This comment has been minimized.

@hacktivist123

This comment has been minimized.

@znarf

This comment has been minimized.

@Oghenebrume50

This comment has been minimized.

@znarf

This comment has been minimized.

@ghost

This comment has been minimized.

@Oghenebrume50

This comment has been minimized.

@ghost

This comment has been minimized.

@znarf znarf added osca bounty and removed osca bounty candidate Potential bounty, to be reviewed. Having this tag on an issue does not qualify it for a bounty. labels Mar 25, 2020
@znarf
Copy link
Member Author

znarf commented Mar 25, 2020

This bounty is open to everyone. Please be aware of the warning, you need to take care properly of the security aspects of this issue. If you don't understand what that means, please pass. We will not give feedback or help unless you did your own research and prove you have a clear understanding of the security implications.

@brymut brymut self-assigned this Mar 25, 2020
@bolariin
Copy link

bolariin commented Apr 6, 2020

I have an idea on how to resolve this. Is it okay if I resolve this issue?

@brymut
Copy link

brymut commented Apr 6, 2020

Sorry @bolariin , I've already started work on this.

@brymut
Copy link

brymut commented Apr 10, 2020

Hi again @bolariin or anyone interested, looks like I won't be able to sort this out to completion over the next week. Unassigning myself, feel free to resolve it.

My possible solutions most revolved around adding the allowedProtoProperties or allowProtoPropertiesByDefault in:
https://github.com/opencollective/opencollective-api/blob/03e81c877af3463040484c28703c5fbbc4af362f/server/lib/email.js#L47
&
https://github.com/opencollective/opencollective-api/blob/03e81c877af3463040484c28703c5fbbc4af362f/server/lib/utils.js#L344
as per the handlebars docs using a rudimentary "whitelisting" function that looked messy and inefficient.

Also, solutions were based on the assumption that all handlebar templates would be authored/ reviewed by a core contributor as per OC needs/requirements and not added by end-users. (@znarf would this be safe to assume ?)

Will still follow this to see how its resolved as I'm interested to see how fresh eyes on this would think of it.

@brymut brymut removed their assignment Apr 10, 2020
@bolariin bolariin self-assigned this Apr 10, 2020
@Betree
Copy link
Member

Betree commented Apr 23, 2020

🏆 This issue has been completed by @bolariin

To claim the $200 bounty, you can either:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issues that require some work on the API (https://github.com/opencollective/opencollective-api) bounty complexity → simple technical-debt Deprecated code to migrate and other necessary refactors $200 simple complexity bounty (v2)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants