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.

Sign up for GitHub

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

Jump to bottom

Warn if dynamic template contains non-escaped character #793

Merged
merged 1 commit into from
Oct 22, 2018
Merged

Warn if dynamic template contains non-escaped character #793

merged 1 commit into from
Oct 22, 2018

Conversation

reedsa
Copy link
Contributor

@reedsa reedsa commented Oct 9, 2018

Closes #790

Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the master branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

  • Warns if any of the ', " or & characters are used in a dynamic template that does not escape with triple-stash syntax

If you have questions, please send an email to Sendgrid, or file a Github Issue in this repository.

@reedsa reedsa added the status: code review request requesting a community code review or review from Twilio label Oct 9, 2018
@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty type: twilio enhancement feature request on Twilio's roadmap hacktoberfest labels Oct 10, 2018
@thinkingserious
Copy link
Contributor

Hello @reedsa,

Thanks again for the PR!

It's HACKTOBERFEST! We want to show our appreciation by sending you some special Hacktoberfest swag. If you have not already, could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

Ditofry
Ditofry reviewed Oct 10, 2018
View reviewed changes
packages/helpers/classes/mail.spec.js
@@ -154,4 +155,63 @@ describe('Mail', function() {
});

});

describe('dynamic template handlebars substitutions', () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

righteous

Ditofry
Ditofry approved these changes Oct 10, 2018
View reviewed changes
Copy link

@Ditofry Ditofry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

@reedsa reedsa added status: ready for deploy code ready to be released in next deploy and removed status: code review request requesting a community code review or review from Twilio labels Oct 10, 2018
@thinkingserious thinkingserious merged commit d6c8c75 into sendgrid:master Oct 22, 2018
@thinkingserious
Copy link
Contributor

Hello @reedsa,

Thanks again for the PR!

We noticed that you have earned 3 out of the 5 points needed to receive glorious SendGrid Hacktoberfest swag.

Please take a moment to checkout this link to find more issues to get you past the required threshold.

Also, please be sure that you have officially registered here.

Thank you and Happy Hacktobering!

Team SendGrid DX

@thinkingserious
Copy link
Contributor

Hello @reedsa,

Thanks again for the PR!

You have earned 6 out of the 5 points needed to receive glorious SendGrid Hacktoberfest swag.

Please take a moment to checkout this link to find more issues to get you past the required threshold or to simply continue the celebration.

Also, please be sure you have officially registered with us here by November 1, 2018 to qualify.

If you have any questions you can email us at dx+hacktoberfest2018@sendgrid.com.

Thank you and Happy Hacktobering!

Team SendGrid

@umarhussain15
Copy link

Any option to stop this warning message other than escaping characters?
Firebase shows this warning as an error in logs which makes it difficult to monitor logs for debugging.

Thanks.

@thinkingserious
Copy link
Contributor

Hello @umarhussain15,

Would you mind creating a new ticket for your request? That way we can properly track progress. Thanks!

With best regards,

Elmer

@umarhussain15
Copy link

umarhussain15 commented Jun 28, 2019 •
edited
Loading

Hi @thinkingserious I just checked latest version of code and #932 has added a solution to hide warnings for dynamic template data. So I think there is no need to create a new ticket.
But I think its not yet released, so I will wait for this change to be released and I will use that.
Thanks for the response.

@thinkingserious
Copy link
Contributor

Awesome, thanks for the follow up!

@prBigBrother
Copy link

Hi @thinkingserious!

can you push forward this pull request with new release?
Right now it's makes really difficult to read logs because our app sending too much emails.

Thank you a lot

@noway
Copy link

noway commented Aug 12, 2019

How do I get rid of that warning? I have 3 curly brackets, why is it littering my logs? There's no way to disable it? I need my logs clean for logging purposes.

Cheers

ekeklol
ekeklol reviewed Nov 11, 2019
View reviewed changes
packages/helpers/constants/index.js
const DYNAMIC_TEMPLATE_CHAR_WARNING = `
Content with characters ', " or & may need to be escaped with three brackets
{{{ content }}}
See https://sendgrid.com/docs/ui/sending-email/using-handlebars/ for more information.`;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url leads to -> 404 not found page

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by #943

@snyk-bot snyk-bot mentioned this pull request Dec 5, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Feb 7, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Mar 19, 2020
Merged
This was referenced Apr 17, 2020
Closed
Open
@childish-sambino childish-sambino mentioned this pull request Aug 31, 2020
Closed
6 tasks
@sarahmhale sarahmhale mentioned this pull request Oct 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekeklol ekeklol ekeklol left review comments

@childish-sambino childish-sambino childish-sambino left review comments

@Ditofry Ditofry Ditofry approved these changes

Assignees
No one assigned
Labels
difficulty: medium fix is medium in difficulty status: ready for deploy code ready to be released in next deploy type: twilio enhancement feature request on Twilio's roadmap
Projects
None yet
Milestone
No milestone
8 participants
@reedsa @thinkingserious @umarhussain15 @prBigBrother @noway @Ditofry @ekeklol @childish-sambino