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

"Content with characters ', " or & may need to be escaped with three brackets" unnecesary warning #965

Closed
noway opened this issue Aug 12, 2019 · 20 comments
Labels
type: community enhancement feature request not on Twilio's roadmap

Comments

@noway
Copy link

noway commented Aug 12, 2019

Issue Summary

Content with characters ', " or & may need to be escaped with three brackets is logged even if I have {{{ 3 brackets in the templates

The bug has been introduced here: #790

I'm trying to keep my logs clean so that i can grep them and look at them, and this seems to be quite a thing which pops out unnecessarily. I also use my own debug.js-based logger, so a wild console.log is something that straight away goes against my logging policies. I appreciate the warning, but after I fixed the handlebars template it is still complaining to me.

Steps to Reproduce

  1. Try to send any email with dynamicTemplate prop which have any of those characters

Technical details:

  • sendgrid-nodejs Version: 6.4.0
  • Node.js Version: 12.6
@ErwinAI
Copy link

ErwinAI commented Aug 13, 2019

I am having this problem as well and would like this to change for the same reasons as @noway mentioned. I did see a function called 'setHideWarnings' in the helper, but not sure how to use this.

@broofa
Copy link

broofa commented Oct 2, 2019

FWIW, #932 adds a hideWarning flag that can be used to suppress this. See https://github.com/sendgrid/sendgrid-nodejs/blob/master/use-cases/hide-warnings.md
... however this is not yet published (still!)

@thinkingserious The above not withstanding, #790 and #932 should both be reverted because, frankly, this feature is completely broken as it stands.

  • There is no explanation of why the warning is logged or what exactly should be done about it(the explanatory URL is 404'ing)
  • There is no indication of which dynamic token contains the problematic characters
  • The warning occurs even if there's no actual problem. Even if the token in question is triple-bracketed in the template, you still get this warning! 😢

Fundamentally, the problem here is that the JS client code is being overly aggressive with these warnings because it doesn't know which tokens are triple-bracketed in the template. For developers that like clean logs (pretty much all of us), this is alarming and frustrating.

This warning logic should either be removed or moved to the server and only applied for double-bracketed tokens.

@dockleryxk
Copy link

@broofa just so you know, I have templates that use exclusively triple brackets. I still get this warning!

@thakichowdhury
Copy link

thakichowdhury commented Oct 30, 2019

I'm experiencing the same thing. We have automated check-in/confirmation emails and these warnings are polluting our server logs.

I've found that you can potentially hide any warnings that might surface with a hideWarning field. https://github.com/sendgrid/sendgrid-nodejs/blob/master/use-cases/hide-warnings.md

UPDATE: I tried to the above hideWarning field and it didn't work for the reasons outlined by @dockleryxk in the post below.

@dockleryxk
Copy link

@thakichowdhury it turns out your suggestion doesn't work currently. The latest release of the SDK on NPM is May 6, 2019. The functionality was added in this commit 10 days later.

@broofa
Copy link

broofa commented Oct 30, 2019

@brandt Sorry to bother you, but is there anyone from @sendgrid triaging these issues?

@andresmijares
Copy link

have anyone found a solution for this?

@ErwinAI
Copy link

ErwinAI commented Nov 16, 2019

@broofa It seems like that is not the case. I'll try to reach out to dx@sendgrid.com to see if a new release can be made.

@dockleryxk
Copy link

@ErwinAI thank you for this. If you hear anything back please let us know.

@LukeXF
Copy link

LukeXF commented Nov 28, 2019

I'm also facing this issue, did you get anything back from them @ErwinAI?

@3sanket3
Copy link

3sanket3 commented Dec 6, 2019

Is it just a warning? I my case it's error. It breaks the code and not sending the email

@jamielob
Copy link

We still have this polluting our logs, also in some templates that exclusively use triple brackets. Anyone seen any fixes for this being worked on?

@childish-sambino childish-sambino added the type: community enhancement feature request not on Twilio's roadmap label Feb 13, 2020
@childish-sambino
Copy link
Contributor

Fixed by #932 which was included in the 6.5.0 release.

@adamreisnz
Copy link
Contributor

Well this sent me off on a wild goose chase, trying to find faulty characters.... >.<

@Kostanos
Copy link

Kostanos commented Sep 16, 2020

@sendgrid/mail": "^7.2.5
Still, have this issue, am I using a different library? any recommendations?

@childish-sambino
Copy link
Contributor

@Kostanos Are you hiding the warnings? https://github.com/sendgrid/sendgrid-nodejs/blob/main/docs/use-cases/hide-warnings.md

@Kostanos
Copy link

I missed this one. It works. I hope it will not disable some important warnings.
Thx.

@LukeXF
Copy link

LukeXF commented Apr 7, 2021

I added hideWarning: true and this hid the warning, but I would like to know why the warning is showing up in the first place? All my templates have {{{ }}} so I don't think the warning handling is correct?

Is anyone able to shed some light on this?

@AngelFHC
Copy link

AngelFHC commented Feb 17, 2022

Warning still showing up as of version 7.6.0

Using only one instance of triple handlebars (none doble) on a dynamic template, but perhaps it's the specific use case?

<a href="{{{link_string}}}">
  Click here
</a>
<a href={{{link_string}}}>
  Click here
</a>

Both versions show the warning

@yoitsro
Copy link

yoitsro commented Mar 23, 2022

@LukeXF @AngelFHC It looks like the library doesn't consider the content of the templates. It is just checking the template data for specific characters](

Object.values(dynamicTemplateData).forEach(value => {
if (/['"&]/.test(value)) {
console.warn(DYNAMIC_TEMPLATE_CHAR_WARNING);
}
});
) and warning us.

I mean, it's useful, but perhaps should only be displayed once per process execution: https://nodejs.org/docs/latest-v16.x/api/process.html#avoiding-duplicate-warnings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

No branches or pull requests