Ability to disable printf formatting

[gabegorelick]

Lots of use cases for VError have no need for printf. But VError doesn't seem to have a way to opt out of printf.

This is especially inconvenient given that a string with % in it can cause VError to throw an Error, which has been a source of frustration for lots of users.

Potential API:

new VError({
  printf: false,
  message: 'This can have %s in it without crashing'
});

[jclulow]

There's an easy way to get the behaviour you're after with the existing API:

var mod_verror = require('verror');

var unsanitised_input = '......%.....'; /* Some string with a % in it, e.g. from user input. */
var err = new mod_verror.VError('%s', unsanitised_input);

[gabegorelick]

There's an easy way to get the behaviour you're after with the existing API

I'm aware of using %s, but it seems kind of silly from a client's perspective to have to use every time when you otherwise don't want printf.

With the rise of template literals, I've seen a lot of codebases use

new VError(`${foo} ${bar}`)

In other words, users can completely ignore VError's printf behavior, because it doesn't seem necessary, until they later run into the % issue. Arguably, VError should be secure from this type of thing out of the box. I understand that's impossible to retrofit without breaking the API, but exposing an option to disable printf seems like a good way to start.

[davepacheco]

We could provide a new top-level constructor for this.

[gabegorelick]

I threw up https://github.com/joyent/node-verror/pull/58 to add printf: false. If there's consensus on adding a new constructor (TError?) I can add that too.

[bertho-zero]

This works fine:

new VError('This can have % in it without crashing'.replace('%', '%%'));

https://runkit.com/embed/xizdhrqtal66

[gabegorelick]

This works fine

That actually fails in a few ways:

new VError('this will still throw %%s'.replace('%', '%%'));

new VError('you forgot to handle multiple placeholders: %s %s'.replace('%', '%%'));

I think this illustrates how tricky getting this right can be. All the more reason VError should offer some help. And even if you could sanitize reliably, it would suffer the same problem that new VError('%s', '...') suffers from, namely callers need to remember to invoke VError in the safe way. Compare that to adding a new top-level constructor that's safe for this kind of use by default.

[bertho-zero]

This is just to escape the message, I can not know when it can be useful and when it is useful just add a few characters to solve the problem.

.replace(/%/g, '%%')

[davepacheco]

@gabegorelick I assume you do want to add a new top-level constructor -- isn't that the primary goal?

I'd suggest calling the option something like skipPrintf so that false and undefined don't mean opposites. There's probably other review feedback, but it'll be easier to do that through Gerrit.

[gabegorelick]

I assume you do want to add a new top-level constructor -- isn't that the primary goal

Correct, but starting small first. I'll try to get something up to Gerrit now.

[gabegorelick]

https://cr.joyent.us/#/c/4394/ (didn't take as long as I thought 😄).

[davepacheco]

(copying from the Gerrit review, just in case)

I'm very sorry that this fell off my radar. It's been way too long. I've made a few comments on the changes. Broadly, they still look great. I really appreciate your willingness to put this change together, and I imagine some of this seems pretty nitty -- if you'd like, I'd be happy to make these suggested changes.

[gabegorelick]

@davepacheco Sorry, this fell off my radar too.

if you'd like, I'd be happy to make these suggested changes.

If you have the bandwidth, go for it!

[davepacheco]

This was picked up by another change described under #63.

[bradennapier]

This just caused my team days of investigation due to an insanely hard to re-create error situation where an error message eventually had ...52% percent of... and no arguments are given at all to the message in restify...

We would get timeouts and users would report them but couldn't find out why without a deep dive into the logs (we have millions of requests a hour) and singling out what could possibly have caused some random error with our code. Have try/catch protection carefully but when errors happen in the error handler that sure is extra difficult to deal with..

bottom line if I send a message and no arguments then it shouldn't use the printf formatting style - I didnt even know that was an option.

Indeed when we found out what was happening we did end up doing ("%s", message) but since this is completely unknown behavior that a lib of a lib was forcing upon users so it was completely undocumented that % symbols are not allowed in any errors without deep understanding of the mechanics of some deeper library.

---

( Yes i realize "52% percent" is odd ;-), not my area or concern haha )

tl;dr

make printf opt-in not opt-out

[jclulow]

I'd like to open by pointing out in no uncertain terms that this is the kind of oddly misplaced aggression that completely exhausts maintainers.

Our library is (I feel) clearly documented to take sprintf()-style arguments. It sounds like you're not using our library directly, though. Given that, it seems more appropriate to direct this grievance toward the first library up the stack that isn't telling you about the sprintf() format, or that isn't correctly using a %s token to sanitise the non-sprintf()-style input it's accepting.

In addition, as @davepacheco noted above, it seems that the work on #63 is going to include a new option to disable sprintf() processing. It looks like it will also include a new top-level constructor, PError, which you can use instead of VError if you don't want sprintf() formatting.

tl;dr MAKE SPRINTF OPT-IN NOT OPT-OUT

We're unlikely to make such a breaking change to the existing VError API. There are a lot of consumers, many or most of which are correctly using sprintf()-style formatting: it's one of the key reasons we produced this library!

[bradennapier]

Is there a single case that not providing a second argument would provide the desired effect with sprintf?

Should (“%”) not work fine? Doesn’t seem breaking since those using the library would have an error anyway in the case they aren’t providing the argument, unless I’m missing something ofc

[jclulow]

Is there a single case that not providing a second argument would provide the desired effect with sprintf?

I really encourage you to read #63, where more discussion about this has occurred. There is some mention of other times we've hashed this out already, in particular this question and answer on #8.

[davepacheco]

Reminder for myself about the history here:

@gabegorelick submitted #58 for this, then moved that to cr.joyent.us as https://cr.joyent.us/#/c/4394/. That seemed to stall after I took an embarrassingly longtime to provide feedback.

Then @hekike submitted #63, which is pretty similar. There's a proposed change on cr.joyent.us at https://cr.joyent.us/#/c/5360/. That also seems to have stalled.

To avoid future confusion I'm going to close this issue but the request is still open under #63. Moving discussion there.