fix autoescape for non-string values; fixes #835

[devoidfury]

#835

[devoidfury]

Imo, a bugfix for the issue is release asap material (even if it's not this one). Maybe submit a CVE?

@SamyPesse @jlongster @vecmezoni @carljm @jbmoelker

[vecmezoni]

val.toString() will be clearer

[jbmoelker]

@devoidfury thanks for this! Looks solid. Could you update the CHANGELOG.md with the fix? For me that's all that's left before merging this PR.

I don't know if we need to submit a CVE. I understand the risk of this bug. But I don't know the protocol or procedure.

@carljm what's the procedure to release this? I've read MAINTENANCE.md, but I don't quite understand how the 2.x and 3.x branches are managed. Seeing the unreleased 2.x items I imagine we should release 2.5.0. Also I think @vecmezoni and I as 1 day old maintainers are able to create releases on GitHub, but are unable to publish a new version to npm.

[vecmezoni]

Probably would be better to publish 2.4.3 and 2.5.0 to fix vulnerability, so users with all the cases of package.json versioning ~ ^ will receive fix. Just need to check is there anything to bump a minor version or we might bump just patch (2.4.3)

[jbmoelker]

@vecmezoni well there are multiple unreleased features on 2.x, so that's why the minor bump. I agree that normally we'd merge this PR and release patch. But I'm not sure where to merge onto for that patch release. It's not master. But is it 2.x?

[vecmezoni]

We can make separate branch from 2.4.2 version, apply this patch and release 2.4.3. Then we can apply this patch to 2.x and decide to release or not to release 2.5. I've asked @carljm to give me access to npm, waiting.

[vecmezoni]

In this case people with "nunjucks": "^2.4" will get this update. Also we can add deprecation warning for everything less than 2.4.2 https://docs.npmjs.com/cli/deprecate

[vecmezoni]

@devoidfury add note to CHANGELOG.md and i'm going to merge this and prepare for release and deprecation note.

[matt-]

Should the escape filter also do the same?

[devoidfury]

Yes, thanks @matt- -- added another commit to fix this.

[jbmoelker]

Can be merged without, but credits where due: "Thanks Matt Austin and Thomas Hunkapiller.".

[vecmezoni]

👍 Merging this to master, then i'll create branch 2.4.3 from 2.4.2 release, backport this patch and rebase 2.x branch on 2.4.3.

[carljm]

@vecmezoni sounds good, but I'd avoid rebasing the 2.x branch. It's public, that could really mess up anyone else tracking it. just merge from your new 2.4.3 branch into 2.x instead.

[vecmezoni]

• Released 2.4.3
• Added deprecation note (i'm not sure that vulnerability is critical, but anyways)

> npm i nunjucks@2.4.2

npm WARN deprecated nunjucks@2.4.2: potential XSS vulnerability in autoescape mode, 
and with escape filter was fixed in v2.4.3

• Backported patch to 2.x, tomorrow i will review and test branch and issue 2.5.0 release.

[dlmr]

Seems like this change broke the use of the safe filter.

{{ value | safe }}

With 2.4.3 this will be escaped where I thought it should not be that. Could be that I have used it incorrectly in my application and that this was never supposed to work but wanted to give a heads up if this was not the case.

[devoidfury]

@dlmr Could you provide a simple test case here?

[dlmr]

@devoidfury I think I found the problem when trying to create a minimal example.

I was using React Helmet but did not use the toString() function on what I got back from it which means that the safe filter did nothing in that case and then when rendering the escape did not work, since it was not a string, and I got the unescaped output as I wanted.

So I was actually using this "bug" in my code. The more correct thing is of course to use toString and in the process be more correct.

Thanks for the quick reply and the fix in the first place! Sorry to blame the PR for this! 😄

[nelsonpecora]

Haha thanks @dimr for catching this. I just hit this with an old filter we were using for embedding optional svgs. It definitely woke me up this morning!

If I'm parsing the tests in this PR correctly, I should be doing something like this in my filter, correct?

'use strict';
var fs = require('fs');

module.exports = function (path) {
  try {
    return fs.readFileSync(path); // which has a .toString() method
  } catch () {
    return '';
  }
};

EDIT: Looks like I should just return the string itself. ☝️ noted for anyone that runs into this thread later.

[devoidfury]

@vecmezoni @carljm should we modify the SafeString constructor to automatically do a string conversion? Right now, it's no-op (identity) on non-strings, but after that point it's going to be cast to string anyway, and with autoescape, if it's not a SafeString, it will escape.

[fengmk2]

Should we fix this in 1.x?

[fengmk2]

this will throw TypeError: Cannot read property 'toString' of null when str is null or undefined

[atian25]

#843