Support requests with non-'utf-8' charsets #16

theopak · 2015-09-24T18:16:15Z

~~Not ready to merge.~~ ~~EDIT: Ready, pending approval.~~ UPDATE: Ready.

TODO

"fix the silent failures in the Travis-CI build"
- EDIT: thanks Ryan Fix Java test by forcing all formencoded content-types to have no charset theopak/oauth_reverse_proxy#4
skip body_parser middleware in case of empty body
add pre-mutator to stash original content type
~~add pre-mutator to convert content to UTF-8 so that body_parser.urlencoded() parses it~~
- wontfix
add post-mutator to restore original content type
verify that non-utf-8 charsets are no longer silently failed
verify that, if removed, non-utf-8 charsets are always restored after body_parser.urlencoded() does its thing.
verify that requests with utf-8 query strings (e.g., emoji) are proxied correctly
- unit tests exist and pass

…ts thing.

theopak · 2015-09-24T18:17:54Z

test/message_integrity_header_test.js

+      it ("should support " + verb + " requests with non-'utf-8' charsets declared in the header", function(done) {
+        var intended_content_type_header = 'text/html; charset=ISO-8859-8';
+        var custom_headers = { headers: {'Content-Type': intended_content_type_header } };
+        request_sender.sendRequest(verb, 'http://localhost:8080/livecheck', custom_headers, 200, function(err, res, body) {


@ryanbreen What job_server endpoint would be appropriate to test against in this case?

I think POST /job would be good. You want to make sure that a url-encoded post gets processed correctly.

…dash" on the Travis-CI build).

…ests in particular.

…x test case

theopak · 2015-09-28T20:54:46Z

It took me waaaay too long to figure this out, but request.js (since v2.64.1) automatically overrides form requests to have Content-Type: "application/x-www-form-urlencoded" when they are sent to the server.

request.js will not send the kind of requests that this PR is looking to test. Therefore I'll have to take a different approach in forming the test.

…ch is used to form test requests

…conditions.

theopak · 2015-09-29T18:08:44Z

@ryanbreen Could you take a look at the test case and make sure that it adequately captures the exact issue you told me about? My progress is stalled on this PR because I'm having trouble replicating the exact issue that you described to me in the chat last Wednesday.

theopak · 2015-09-29T18:12:14Z

test/message_integrity_formencoded_test.js

@@ -1,10 +1,6 @@
 var _ = require('underscore');
 var fs = require('fs');
 var path = require('path');
-
-var request_sender = require('./utils/request_sender.js');
-var validation_tools = require('./utils/validation_tools.js');


Removed because these are repeated in lines 8 and 9 (of the new file).

theopak · 2015-10-01T20:34:36Z

Inspecting req.argument_pairs during the unit tests, I notice that all the Java client's nonces are integers, and some of those are negative integers. This seems unusual to me. Two example values from failed requests:

[2015-10-01T20:09:48.988Z] TRACE: oauth_reverse_proxy/54621 on lexmac1909.vistaprint.net: (service_name=jobsservice, module=lib/proxy/validators/oauth_signature_validator.js)
    req.argument_pairs:
    [ [ 'oauth_consumer_key', 'java-test-key' ],
      [ 'oauth_nonce', '1244941739633823073' ],
      [ 'oauth_signature_method', 'HMAC-SHA1' ],
      [ 'oauth_timestamp', '1443730188' ],
      [ 'oauth_version', '1.0' ],
      [ 'this', 'is' ],
      [ 'fun', 'right' ] ]
[2015-10-01T20:12:37.531Z] TRACE: oauth_reverse_proxy/54666 on lexmac1909.vistaprint.net: (service_name=jobsservice, module=lib/proxy/validators/oauth_signature_validator.js)
    req.argument_pairs:
    [ [ 'oauth_consumer_key', 'java-test-key' ],
      [ 'oauth_nonce', '-4825279163585566192' ],
      [ 'oauth_signature_method', 'HMAC-SHA1' ],
      [ 'oauth_timestamp', '1443730357' ],
      [ 'oauth_version', '1.0' ],
      [ 'this', 'is' ],
      [ 'fun', 'right' ] ]

"A nonce is a random string," according to http://oauth.net/core/1.0a/#nonce, which means that the other clients are following spec when they generate alphanumeric nonces as base64 encoded UTF-8 data.

@rb Do you think this indicate a problem with the way that the client's requests are received?

ryanbreen · 2015-10-01T20:46:50Z

I wouldn't think that would cause an issue. An integer is still a random string.

…g conversion.

…ser.urlencoded() to throw an error due to unsupported charset.

…ins: /home/travis/perl5/lib/perl5/x86_64-linux-gnu-thread-multi /home/travis/perl5/lib/perl5 /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .) at client.pl line 5.

…{no value}'.

… future. Not spending any more time on this.

…d never restored.

theopak · 2015-10-05T20:29:29Z

@ryanbreen The client library tests pass, with your changes, as of 7675b31

However, your changes have a typo: theopak#4 (comment)

The tests fail if I fix the typo

…ress-MCPGH-15 * commit '5a6f3af47e6c46a10641af9c505ec5eb904bb858': Also munge charsets on the jobserver test side.

theopak · 2015-10-05T22:51:01Z

test/oauth_proxy_outbound_test.js

@@ -21,7 +21,7 @@ describe('oauth_proxy outbound message integrity: verbs', function() {
    ['GET', 'DELETE'].forEach(function(verb) {


The latter 4 test templates are the ones that still fail in the build, even though they pass on master.

$ set OAUTH_REVERSE_PROXY_LOG_LEVEL="trace"; mocha --reporter spec -t 10000 ./test/oauth_proxy_outbound_test.js | bunyan 64 passing (494ms) 8 failing

@ryanbreen any idea why? I've tried rolling back my changes one by one locally, and looking through the diff in this PR, but I can't pinpoint the line(s) that cause these tests to fail.

All the failures are here:

8) should accept a properly signed DELETE over IPv6 with query via https Error: Protocol "https:" not supported. Expected "http:". at new ClientRequest (_http_client.js:53:11) at Object.exports.request (http.js:31:10) at Object.exports.request (https.js:163:15) at Array.stream (/Users/wrb/tmp/oauth_reverse_proxy/node_modules/http-proxy/lib/http-proxy/passes/web-incoming.js:108:74) at ProxyServer.<anonymous> (/Users/wrb/tmp/oauth_reverse_proxy/node_modules/http-proxy/lib/http-proxy/index.js:80:21)

I'm not sure what might have changed between this version and master. Looks like an https request is being sent with http.js. Not sure why that would happen.

Looks like it might be a node > 0.11.0 discrepancy: SamDecrock/node-http-ntlm#21

You might need to check the node-http-proxy version and see if there's an upgrade to address this, assuming the issue is from that package.

Builds now thanks to your fix. Evidently the failures weren't directly related to using node > 0.11.0, but actually a change that happened when I upgraded dependencies and combined query_parser.js and url_parser.js without noticing all of the resulting differences.

Make test less vulnerable to differences in output between versions.

Set the proper agent for outbound requests.

ryanbreen · 2015-10-06T19:21:45Z

LGTM!

Support requests with non-'utf-8' charsets

theopak added 4 commits September 23, 2015 18:06

Strip 'Content-Type' , and then add it again after body_parser does i…

d5f3403

…ts thing.

Code quality

448c14f

Combine pre- and post-urlencoded mutators into a single middleware.

2983594

Add unit test to verify support of non-utf-8 charset requests.

04eceb3

theopak self-assigned this Sep 24, 2015

theopak mentioned this pull request Sep 24, 2015

Requests silently fail when they do not have charset === utf-8 (with the dash) #15

Closed

theopak reviewed Sep 24, 2015
View reviewed changes

theopak added 5 commits September 24, 2015 14:27

Fix build validity failure caused by 538607c

b855c9a

Use a test endpoint that actually does work

25e8208

Require bash, because the calling shell may be something else (e.g. "…

a17271d

…dash" on the Travis-CI build).

Remove bad test, add test that verifies form urlencoded PUT/POST requ…

183b079

…ests in particular.

Stash/unstash "Content-Encoding" and "Content-Type" headers. TODO: fi…

676bd14

…x test case

theopak mentioned this pull request Sep 28, 2015

Add JWT support #18

Open

Expect these tests to fail due to request/request@86571c7

80f96fd

theopak added 6 commits September 28, 2015 17:07

Override Request.js form header modification in request_sender.js whi…

e65d784

…ch is used to form test requests

Clean up

307b940

Repeat request charset validation by explicitly each of the possible …

9bb6f33

…conditions.

Don't stash charset === 'utf-8'.

e13d618

Remove Request.js customization

ee521ee

Expand test cases

6d49b56

theopak changed the title ~~Support requests with non--utf-8 charsets~~ Support requests with non-'utf-8' charsets Sep 29, 2015

theopak reviewed Sep 29, 2015
View reviewed changes

Restore java client.

491cad3

theopak added 2 commits October 5, 2015 14:48

Failing test: enable header charset stash/unstash and content encodin…

5c202c4

…g conversion.

Comment out previous commit, resulting in a test that causes body_par…

44c756d

…ser.urlencoded() to throw an error due to unsupported charset.

theopak added 9 commits October 5, 2015 15:10

fixup! Make Travis before_script faster

85d65fb

Minor: Add missing require statement

1abdca2

Remove unused Request.js overload

370c0d6

Fix 'should accept a properly signed GET|DELETE with {a funky query}|…

bfbc255

…{no value}'.

Minor: add logging, clarify wording of unit test

d76ed90

Print env info in Travis build to help debug Perl dependencies in the…

fcb5cae

… future. Not spending any more time on this.

Remove Perl client tests from Travis build.

6063651

Client library tests pass as long as headers are removed for good, an…

7675b31

…d never restored.

theopak and others added 8 commits October 5, 2015 16:43

Minor

31daa0e

Also munge charsets on the jobserver test side.

5a6f3af

Log descriptive errors

298d649

Merge commit '5a6f3af47e6c46a10641af9c505ec5eb904bb858' into bug/Cimp…

8bf7359

…ress-MCPGH-15 * commit '5a6f3af47e6c46a10641af9c505ec5eb904bb858': Also munge charsets on the jobserver test side.

TO-DONT do that

cbf2397

Comment out some diffs

7efb801

fixup! Comment out some diffs

5adf936

Fix hardcoded value which made a test use GET twice instead of DELETE

d0884ee

theopak reviewed Oct 5, 2015
View reviewed changes

ryanbreen and others added 8 commits October 6, 2015 10:11

Make test less vulnerable to differences in output between versions.

97c7fad

Merge pull request #6 from ryanbreen/bug/GH-15_rb

8445ad7

Make test less vulnerable to differences in output between versions.

Set the proper agent for outbound requests.

f1557be

Merge pull request #7 from ryanbreen/bug/GH-15_rb

0d9d9b2

Set the proper agent for outbound requests.

Cleanup

59d9b73

Adds tests to validate that emoji are allowed in query strings

236c01e

Encode URL components

f4f5ee4

Reorder test

d37d0b1

ryanbreen added a commit that referenced this pull request Oct 7, 2015

Merge pull request #16 from theopak/bug/GH-15

c9e7358

Support requests with non-'utf-8' charsets

ryanbreen merged commit c9e7358 into Cimpress-MCP:master Oct 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support requests with non-'utf-8' charsets #16

Support requests with non-'utf-8' charsets #16

theopak commented Sep 24, 2015

theopak Sep 24, 2015

ryanbreen Sep 24, 2015

theopak commented Sep 28, 2015

theopak commented Sep 29, 2015

theopak Sep 29, 2015

theopak commented Oct 1, 2015

ryanbreen commented Oct 1, 2015

theopak commented Oct 5, 2015

theopak Oct 5, 2015

ryanbreen Oct 6, 2015

ryanbreen Oct 6, 2015

theopak Oct 6, 2015

ryanbreen commented Oct 6, 2015

		@@ -21,7 +21,7 @@ describe('oauth_proxy outbound message integrity: verbs', function() {
		['GET', 'DELETE'].forEach(function(verb) {

Support requests with non-'utf-8' charsets #16

Support requests with non-'utf-8' charsets #16

Conversation

theopak commented Sep 24, 2015

TODO

theopak Sep 24, 2015

Choose a reason for hiding this comment

ryanbreen Sep 24, 2015

Choose a reason for hiding this comment

theopak commented Sep 28, 2015

theopak commented Sep 29, 2015

theopak Sep 29, 2015

Choose a reason for hiding this comment

theopak commented Oct 1, 2015

ryanbreen commented Oct 1, 2015

theopak commented Oct 5, 2015

theopak Oct 5, 2015

Choose a reason for hiding this comment

ryanbreen Oct 6, 2015

Choose a reason for hiding this comment

ryanbreen Oct 6, 2015

Choose a reason for hiding this comment

theopak Oct 6, 2015

Choose a reason for hiding this comment

ryanbreen commented Oct 6, 2015