fix(static): avoid user-provided data in Error messages being interpreted as sprintf codes #1384

trentm · 2017-06-26T22:49:08Z

The 'static' plugin had a few cases where the path in a request would be
passed as the first ("message") field to a RestError constructor.
RestError uses verror.WError, which uses extsprintf to render the given
arguments. If the "message" includes "%...s" or similar printf codes,
then it will error output.

Also bump to 4.3.1.
Fixes #1382.

…erpreted as sprintf codes The 'static' plugin had a few cases where the path in a request would be passed as the first ("message") field to a RestError constructor. RestError uses verror.WError, which uses extsprintf to render the given arguments. If the "message" includes "%...s" or similar printf codes, then it will error output. Also bump to 4.3.1.

retrohacker · 2017-06-26T23:25:28Z

Hey @trentm! Thank you for the thorough bug report paired with a PR! ❤️

I'm tagging this as unconfirmed until a maintainer runs the repro case (which should be soon ™️)

At a quick glance, updating the CHANGES.md file and incrementing version numbers should be done by our CI/CD (using conventional changelogs) in the near future and manually by maintainers at the current point in time. We don't have a CONTRIBUTING.md file yet, and this bit of feedback definitely belongs there so this is a shortcoming in the projects documentation. I'll be working on that document and CI/CD this week 😄 would you mind removing the CONTRIBUTING.md and package.json updates from this PR?

trentm · 2017-06-27T18:40:35Z

@retrohacker Sure I could remove those parts if you like. FWIW, I am a maintainer of restify (though not the most active) and have commit rights. I don't speak regularly with the other maintainers and haven't for a while, so I was feeling out the process a bit. I was hoping to get a review/+1 and then quickly do a patch level release with this change.

trentm · 2017-06-27T18:43:48Z

The check failure looks like a spurious timing-related test failure unrelated to my change:

✔ audit logger timer test
✖ audit logger anonymous timer test

Assertion Message: handler-0 should be > 1000000
AssertionError: false == true
    at Object.ok (/home/travis/build/restify/node-restify/node_modules/nodeunit/lib/types.js:83:39)
    at /home/travis/build/restify/node-restify/test/plugins.test.js:1103:11
    at parseResponse (/home/travis/build/restify/node-restify/lib/clients/json_client.js:91:9)
    at IncomingMessage.done (/home/travis/build/restify/node-restify/lib/clients/string_client.js:165:13)
    at IncomingMessage.g (events.js:260:16)
    at emitNone (events.js:72:20)
    at IncomingMessage.emit (events.js:166:7)
    at endReadableNT (_stream_readable.js:923:12)
    at nextTickCallbackWith2Args (node.js:511:9)
    at process._tickDomainCallback (node.js:466:17)

✔ GH-812 audit logger has query params string

from https://travis-ci.org/restify/node-restify/jobs/247301081
Do you know if there is a separate ticket to look into that?

make test was working fine for me testing with node v4 on my Mac, at least.

retrohacker · 2017-06-27T18:47:20Z

Oh snap 😊 I'm new, sorry!

And yeah, the audit logger tests have transient failures. I opened an issue here but haven't had time to take a look. Re-triggering almost always gets the test to pass but it is very irritating: https://github.com/restify/node-restify/issues/created_by/retrohacker

trentm · 2017-06-27T18:56:41Z

Oh snap 😊 I'm new, sorry!

No problem at all. :) Thanks for working on restify issues!

retrohacker · 2017-06-27T19:11:48Z

test/plugins.test.js

@@ -984,6 +984,39 @@ test('GH-719 serve a specific static file', function (t) {
    serveStaticTest(t, false, '.tmp', null, true);
 });

+test('static responds 404 for missing file', function (t) {


Good catch, this test was missing

retrohacker · 2017-06-27T19:12:05Z

test/plugins.test.js

+//      > new restify.ResourceNotFoundError('/%22s not found')
+//      Error: too few args to sprintf
+test('static responds 404 for missing file with percent-codes', function (t) {
+    var p = '/public/no-%22such-file.json';


Great test!

retrohacker · 2017-06-27T19:13:26Z

test/plugins.test.js

+    });
+});
+
+// Earlier versons would throw:


Comment makes sense in the context of this PR, but may be confusing to future readers who don't have this context. Also, test name should begin with GH-1382.

retrohacker · 2017-06-27T19:14:56Z

Everything here looks good. Note that we are going to be shipping 5.x this week ^_^ will leave #1382 open until a PR is submitted for the 5.x branch ❤️

…view comments

trentm · 2017-06-27T19:17:51Z

@retrohacker Do you want a PR for this for 5.x before that release?

retrohacker · 2017-06-27T20:40:07Z

@trentm That would be awesome 🤘

retrohacker · 2017-06-27T20:48:59Z

Actually, it looks like we are shipping 5.x today ^_^ a review will probably take longer than that

this can be shipped as a bug fix to the 5.x release later ❤️

trentm · 2017-06-27T20:55:07Z

@retrohacker Cool, thanks.

Are we waiting for anything on this ticket, do you know? I'd love to merge this and get a 4.3.1 release out with it.

retrohacker · 2017-06-27T23:08:43Z

@trentm everything LGTM. I'm still getting a feel for what consensus looks like, but I'm okay merging with 1 review unless @yunong @DonutEspresso or @rajatkumar have an objection to that policy.

trentm · 2017-06-27T23:21:03Z

Objections to me (squash) merging this and doing a 4.3.1 release? I'll do that tomorrow morning unless someone screams. :)

Thanks @retrohacker !

trentm · 2017-06-28T17:39:44Z

Sorry about the long (and slightly inaccurate) commit message for that merge. I missed seeing all that content for the commit message that was below the fold (scroll down) in the message box UI that GitHub provided.

trentm · 2017-06-28T17:55:09Z

This is published in restify@4.3.1 now.

retrohacker · 2017-06-28T18:47:47Z

🎉

…eted as sprintf codes (#1384) The 'static' plugin had a few cases where the path in a request would be passed as the first ("message") field to a RestError constructor. RestError uses verror.WError, which uses extsprintf to render the given arguments. If the "message" includes "%...s" or similar printf codes, then it will error output. Also bump to 4.3.1. * drop ver bump changes, per review comments * update test name and drop comment (unnecessary with issue ref) per review comments

…eted as sprintf codes (#1384) (#1472) The 'static' plugin had a few cases where the path in a request would be passed as the first ("message") field to a RestError constructor. RestError uses verror.WError, which uses extsprintf to render the given arguments. If the "message" includes "%...s" or similar printf codes, then it will error output.

trentm requested a review from micahr June 26, 2017 22:49

retrohacker self-requested a review June 26, 2017 23:19

retrohacker added Bug Unconfirmed Waiting on Response labels Jun 26, 2017

drop ver bump changes, per review comments

492416e

retrohacker requested changes Jun 27, 2017

View reviewed changes

retrohacker removed Unconfirmed Waiting on Response labels Jun 27, 2017

retrohacker requested a review from rajatkumar June 27, 2017 19:15

update test name and drop comment (unnecessary with issue ref) per re…

5ee1008

…view comments

retrohacker approved these changes Jun 27, 2017

View reviewed changes

trentm merged commit b9637bd into restify:4.x Jun 28, 2017

trentm mentioned this pull request Jun 28, 2017

static plugin ResourceNotFoundError throws on sprintf-like percent code in req.path #1382

Closed

retrohacker mentioned this pull request Jul 18, 2017

jsonBodyParser fails with invalid JSON when the first invalid character is % #1411

Closed

DonutEspresso mentioned this pull request Sep 3, 2017

fix(static): avoid user-provided data in Error messages being interpreted as sprintf codes #1472

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(static): avoid user-provided data in Error messages being interpreted as sprintf codes #1384

fix(static): avoid user-provided data in Error messages being interpreted as sprintf codes #1384

trentm commented Jun 26, 2017

retrohacker commented Jun 26, 2017 •

edited

trentm commented Jun 27, 2017

trentm commented Jun 27, 2017

retrohacker commented Jun 27, 2017

trentm commented Jun 27, 2017

retrohacker Jun 27, 2017

retrohacker Jun 27, 2017

retrohacker Jun 27, 2017

retrohacker commented Jun 27, 2017

trentm commented Jun 27, 2017

retrohacker commented Jun 27, 2017

retrohacker commented Jun 27, 2017

trentm commented Jun 27, 2017

retrohacker commented Jun 27, 2017

trentm commented Jun 27, 2017

trentm commented Jun 28, 2017

trentm commented Jun 28, 2017

retrohacker commented Jun 28, 2017

fix(static): avoid user-provided data in Error messages being interpreted as sprintf codes #1384

fix(static): avoid user-provided data in Error messages being interpreted as sprintf codes #1384

Conversation

trentm commented Jun 26, 2017

retrohacker commented Jun 26, 2017 • edited

trentm commented Jun 27, 2017

trentm commented Jun 27, 2017

retrohacker commented Jun 27, 2017

trentm commented Jun 27, 2017

retrohacker Jun 27, 2017

Choose a reason for hiding this comment

retrohacker Jun 27, 2017

Choose a reason for hiding this comment

retrohacker Jun 27, 2017

Choose a reason for hiding this comment

retrohacker commented Jun 27, 2017

trentm commented Jun 27, 2017

retrohacker commented Jun 27, 2017

retrohacker commented Jun 27, 2017

trentm commented Jun 27, 2017

retrohacker commented Jun 27, 2017

trentm commented Jun 27, 2017

trentm commented Jun 28, 2017

trentm commented Jun 28, 2017

retrohacker commented Jun 28, 2017

retrohacker commented Jun 26, 2017 •

edited