fix: Fix `.matchHeader()` with `allowUnmocked` #1480

ww-daniel-mora · 2019-03-23T12:50:45Z

Currently, the header filter fails if there is not a getHeader method present on the options object. This sometimes appears to be the case. This is a simple node program written that should work as a repo

import httpProxy from 'http-proxy';
import nock from 'nock';

nock('http://example.com')
  .matchHeader('any-string', val => true)
  .get('/hello'),
  reply(200, 'goodbye');

let proxyServer = httpProxy.createProxyServer({
	changeOrigin: true,
	selfHandleResponse: false,
	target: 'http://example.com',
  });
proxyServer.listen(8080);

curl -X GET http://localhost:8080/hello

This change also makes the two checkHeader methods consistent. Where one previously respected functions and the other did not.

paulmelnikow · 2019-03-23T21:38:20Z

Hi! Thanks for opening this.

What's the expected behavior for the repro program?

options.getHeader sure is a bit odd. It doesn't seem to be part of the defined options for an http request, which makes me think it's a nock internal thing…

I just played with matchHeader, and couldn't confirm this bug, though I found another bug, and some unexpected behavior. I will open a PR with the new tests.

ww-daniel-mora · 2019-03-24T13:08:13Z

@paulmelnikow Thank you for your speedy reply! I think this is the getHeader method that nock is calling https://github.com/nodejs/node/blob/169b7f1f3b3751289f24678930e6a5731464ebc9/lib/_http_outgoing.js#L495. The above example basically does the following:

Creates a proxy server that will forward requests from localhost:8080 to example.com
Creates a nock interceptor that will catch requests sent to example.com/hello and reply with 200 'goodbye'
Lastly, there is curl call to the local proxy server which should then attempt to forward the request to example.com/hello which should then be intercepted by nock. This actually works unless you try to apply a header filter.

The outgoing request that the http-proxy code creates does not have a getHeader method defined on it. In the version of nock that I am using, if you have a header filter and there is no getHeader method then nock will not intercept any calls. Basically, everything comes back as not matching the header filter. This change basically attempts to coerce the object to conform to nocks expectations.

ww-daniel-mora · 2019-03-24T13:41:29Z

@paulmelnikow let me do a little more digging and see if I can pin down what is happening a bit better.

paulmelnikow · 2019-03-24T15:51:39Z

When I run the test I see "goodbye" in the terminal. Is that the expected behavior?

ww-daniel-mora · 2019-03-24T19:16:34Z

@paulmelnikow yes that is the expected behavior. I did a bit more playing around to make a minimal example and this is a working minimal example.

import httpProxy from 'http-proxy';
import nock from 'nock';

nock('http://example.com', {allowUnmocked: true})
  .matchHeader('any-string', val => true)
  .get('/hello')
  .reply(200, 'goodbye');

let proxyServer = httpProxy.createProxyServer({
	changeOrigin: true,
	selfHandleResponse: false,
	target: 'http://example.com',
  });
proxyServer.listen(8080);

What I have found is that if run without allowUnmocked to be false then nock will hit both of the checkHeader methods that I edit in this PR. If you run with allowUnmocked to be true then nock will actually only hit one of the checkHeader methods. At the time it hits this method getHeader does not exist on the options object.

ww-daniel-mora · 2019-03-24T19:34:07Z

The first hit to checkHeader comes from the newRequest call here

    module.request = function(options, callback) {
      // debug('request options:', options);
      return newRequest(proto, overriddenRequest.bind(module), options, callback);
    };

... and then goes to matchIndependentOfBody. During this invocation getHeader never appears to be defined to the check header call fails.

The second checkHeader gets triggered from this call to req.end

  req.end = function(buffer, encoding, callback) {
    debug('req.end');
    if (!req.aborted && !ended) {
      req.write(buffer, encoding, function () {
        if (typeof callback === 'function') {
          callback();
        }

... and then goes to match. At this point when we reach the checkHeader logic getHeader is defined on options.

When allowUnmocked is true the second call is never triggered and the first call appears broken. I think this shows two bugs but you can better diagnose. Two things are surprising though.

The call to matchIndependantOfBody does not work (at least with some combination of tools/versions). This code change would fix this bug.
The call to match (which given the names I assume would include body matches) does not appear to be called when allowUnmocked is set to true. This may be intentional, but, it is at least surprising.

paulmelnikow · 2019-03-24T20:03:11Z

I'm getting "goodbye" without any code changes. I guess I'm confused about what the bug is.

The matchHeader function always returns true, so it should match every request. The "goodbye" means the mock is matching. It sounds like the code in nock, confusing it may be, is working as intended.

Could you clarify the expected + observed behavior of the test?

ww-daniel-mora · 2019-03-24T20:04:28Z

@paulmelnikow Does that include my recent comments with allowUnmocked?

paulmelnikow · 2019-03-24T20:22:44Z

Sorry, I'm not sure what you're asking! 😀 I'd like to understand what the bug is first. Otherwise it's hard to make sense of the fix / implementation change.

ww-daniel-mora · 2019-03-25T20:33:31Z

Let me try to restate and see if that makes it more clear.

Create a node server with the following code (I used typescript but you can translate to JS if you like)

import httpProxy from 'http-proxy';
import nock from 'nock';

nock('http://example.com', {allowUnmocked: true})
  .matchHeader('any-string', val => true)
  .get('/hello')
  .reply(200, 'goodbye');

let proxyServer = httpProxy.createProxyServer({
  	    changeOrigin: true,
    selfHandleResponse: false,
    target: 'http://example.com',
  });
proxyServer.listen(8080);

Make a curl call

 curl -X GET http://localhost:8080/hello

EXPECTED
Nock intercpets the call and you see 'goodbye'
ACTUAL
Nock does not intercept the call and you see the example.com page

This is different from the original example because I added the allowUnmocked: true setting.

ww-daniel-mora · 2019-03-25T20:54:26Z

@paulmelnikow I've added a test as well. If you just add the test I added you will also see the bug.

paulmelnikow · 2019-03-26T01:18:44Z

Gooooot it! The test in the top post actually passes and is working as expected; it's the one with { allowUnmocked: true } that is failing. Thanks for adding the test. The bug is much easier to reason about in that form 😀

I am not convinced that options is ever used interchangeably with OutgoingMessage so I don't think it's quite parallel with https://github.com/nodejs/node/blob/169b7f1f3b3751289f24678930e6a5731464ebc9/lib/_http_outgoing.js#L495.

I hate to spread more layers on top of existing cruft. What would you think about removing these lines:

nock/lib/request_overrider.js

Lines 101 to 103 in 75cceb6

    
           options.getHeader = function(name) { 
        
             return getHeader(req, name) 
        
           }

And proceeding from there, to get the matching working again?

paulmelnikow · 2019-03-26T01:20:00Z

lib/interceptor.js

-    return (
-      options.getHeader &&
-      common.matchStringOrRegexp(options.getHeader(header.name), header.value)
+    if (_.isFunction(header.value)) {


This looks like a bug. When this condition is true, the function returns without executing common.matchStringOrRegexp().

paulmelnikow · 2019-03-26T01:22:24Z

lib/intercept.js

@@ -388,7 +388,17 @@ function activate() {
      options = urlParse(options.toString())
    }
    options.proto = proto
-


As I mentioned I'm not sure we need to insert getHeader into options at all. I've found no evidence that this is for parity with an existing API or compatibility with some existing library.

Could you remove this code, and update the code in the places where options.getHeader is called, instead? It's fine to delegate the repeated code to a helper function, though let's not unnecessarily stick that helper function into options.

paulmelnikow · 2019-03-26T01:23:25Z

tests/test_header_matching.js

@@ -95,6 +95,21 @@ test('match headers with function', async t => {
  scope.done()
 })

+test('match headers with function and allowUnmocked', async t => {


If you merge master into this file, there are some new tests which are a bit clearer and more specific. If you'd like, you could update this to use one of those.

Currently, the header filter fails if there is not a `getHeader` method present on the options object. This sometimes appears to be the case. This is a simple node program written that should work as a repo ```typescript import httpProxy from 'http-proxy'; import nock from 'nock'; let proxyServer = httpProxy.createProxyServer({ changeOrigin: true, selfHandleResponse: false, target: 'http://example.com', }); proxyServer.listen(8080); nock('http://example.com') .matchHeader('any-string', val => true) .get('hello'), reply(200, 'goodbye') ``` ```bash curl -X GET http://localhost:8080/hello ``` This change also make the two checkHeader methods consistent. Where one previously respected functions and the other did not.

Fix lint errors.

…lities later

…he incomming object * Reverted changes to intercept.js * Moved shared header logic to a helper function in interceptor.js * Added test to cover header checks with allowUnmocked set to true

ww-daniel-mora · 2019-03-27T02:34:29Z

@paulmelnikow I believe these changes meet your criteria. I suspect there is still a bug on body filters with allowUnmocked set to true, and I am unclear on why nock checks things differently depending on this flag, but, that is an investigation for another day.

Fix typo

paulmelnikow

This is looking good.

paulmelnikow · 2019-03-27T15:33:28Z

tests/test_header_matching.js

+  t.equal(statusCode, 200)
+  t.equal(body, 'Hello World!')
+  scope.done()
+})


paulmelnikow · 2019-03-27T15:33:49Z

tests/test_header_matching.js

+  t.equal(statusCode, 200)
+  t.equal(body, 'Hello World!')
+  scope.done()
+})


paulmelnikow · 2019-03-27T15:34:00Z

lib/interceptor.js

-      common.matchStringOrRegexp(options.getHeader(header.name), header.value)
-    )
+  const checkHeaders = function(filter) {
+    return filterMatches(filter, options)


This looks great!

paulmelnikow · 2019-03-27T15:35:03Z

tests/test_header_matching.js

@@ -82,7 +82,7 @@ test('match headers on number with regexp', async t => {
 })

 test('match header on scope with function: gets the expected argument', async t => {
-  t.plan(3)
+  t.plan(4) // The check in matchHeader should run twice


Hmm, is there a reason it should? It seems confusing for the check to run more than once per request.

I ran the debug and what is happening is it is called once from matchIndependentOfBody and once from match. Previously the two functions used different header matching logic. Now they use the same logic so line 92 will be called twice.

Okay. I can dig into it a bit more and see if I can get all the tests passing with the function only being called once. Ideally I'd like to fix that before merging because I don't think there's a good reason to evaluate it twice and it seems confusing for the user.

So matchIndependantOfBody is used here https://github.com/nock/nock/blob/beta/lib/intercept.js#L398 to detect if we should let the request through. But if all the check pass on matchIndependantOfBody we don't really do anything with it because we wait for the body when we execute match. I think what we would basically need to do to get down to 3 executions rather than 4 is to drop the whole matchIndependantOfBody line of execution and only executing matching once. Inside the match method. This would simplify nock but it looked like a lot more than I was really willing to bite off.

Gotcha. Thank you so much for looking into it this far! I can pick it up from here.

Since the headers aren't part of the body, it seems like the check belongs in matchIndependentOfBody, though maybe doing it just once inside of match will be a more reasonable incremental change.

You can't only check in the first because there might be I rejecting body filter. So you could only check in the second method when you have all the data. As an alternative, you could store the results of matchIndependant... on the object and then use the result in the match call rather than rerunning all the checks.

As a contributor I do find the separate matches confusing so executing the checks once would read better, but, it was so odd I assumed it was done for good reason.

If possible you could also consider merging this change in as is and addressing the extra call separately. We would trade the duplicate call for working header matching.

…calls it

…tent

paulmelnikow · 2019-04-12T21:09:56Z

@ww-daniel-mora Hi! Thanks for your patience! I've moved the header check out of matchIndependentOfBody() which I've renamed to matchAddress(), and now there's a single set of header checks happening in match().

Before I ship it, would you like to have another look and/or take it for a spin?

If you don't have time, no worries! I know you've worked hard on this.

paulmelnikow

Going to give OP a chance to look at this if he wants!

ww-daniel-mora · 2019-04-15T14:20:56Z

@paulmelnikow looks good. Thanks for your help!

paulmelnikow · 2019-04-15T16:29:35Z

Coverage % is down in interceptor.js, though it's because code was removed. There are no new uncovered lines.

nockbot · 2019-04-15T16:38:47Z

🎉 This PR is included in version 11.0.0-beta.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

nockbot · 2019-08-12T23:59:28Z

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

- Move header checks into `match()`. - Rename internal method `matchIndependentOfBody()` to `matchAddress()`

paulmelnikow mentioned this pull request Mar 23, 2019

test: Improve tests for header matching with function; adopt new promise-rejection assertion #1481

Merged

paulmelnikow reviewed Mar 26, 2019

View reviewed changes

ww-daniel-mora and others added 5 commits March 26, 2019 20:12

Update interceptor.js

6471ec7

Fix lint errors.

Safely check for header array as well as getHeader method

70da46a

Switch to normalize options instead of checking for different possibi…

a1eda82

…lities later

Update header checks to be more robust without adding a function to t…

c068380

…he incomming object * Reverted changes to intercept.js * Moved shared header logic to a helper function in interceptor.js * Added test to cover header checks with allowUnmocked set to true

Update interceptor.js

307b594

Fix typo

paulmelnikow reviewed Mar 27, 2019

View reviewed changes

paulmelnikow changed the title ~~Make header filters more robust~~ Fix .matchHeader() with allowUnmocked Mar 27, 2019

paulmelnikow added 3 commits April 12, 2019 15:53

Match headers in match(), not matchIndependentOfBody()

147be10

Merge branch 'beta' into patch-2

499a9d7

Wet the slightest bit to remove indirection

5e0dc68

paulmelnikow mentioned this pull request Apr 12, 2019

Avoid inserting extra stuff like getHeader() into options object #1502

Closed

Remove unreachable code and move helper just preceding function that …

181e9e9

…calls it

paulmelnikow changed the title ~~Fix .matchHeader() with allowUnmocked~~ fix: Fix .matchHeader() with allowUnmocked Apr 12, 2019

Rename matchIndependentOfBody to matchAddress to better convey its in…

7a98cef

…tent

Inline and tighten up the diff

1a68c69

Fix typo; use a verbose and consistent name

2876c96

paulmelnikow approved these changes Apr 12, 2019

View reviewed changes

paulmelnikow merged commit d6667f0 into nock:beta Apr 15, 2019

nockbot added the released label Apr 15, 2019

gr2m pushed a commit that referenced this pull request Sep 4, 2019

fix: Fix .matchHeader() with allowUnmocked (#1480)

2f4baed

- Move header checks into `match()`. - Rename internal method `matchIndependentOfBody()` to `matchAddress()`

gr2m pushed a commit that referenced this pull request Sep 4, 2019

fix: Fix .matchHeader() with allowUnmocked (#1480)

f158159

- Move header checks into `match()`. - Rename internal method `matchIndependentOfBody()` to `matchAddress()`

nockbot mentioned this pull request Sep 4, 2019

The automated release is failing 🚨 #1704

Closed

gr2m pushed a commit that referenced this pull request Sep 5, 2019

fix: Fix .matchHeader() with allowUnmocked (#1480)

af99806

- Move header checks into `match()`. - Rename internal method `matchIndependentOfBody()` to `matchAddress()`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix `.matchHeader()` with `allowUnmocked` #1480

fix: Fix `.matchHeader()` with `allowUnmocked` #1480

ww-daniel-mora commented Mar 23, 2019 •

edited

paulmelnikow commented Mar 23, 2019

ww-daniel-mora commented Mar 24, 2019

ww-daniel-mora commented Mar 24, 2019

paulmelnikow commented Mar 24, 2019

ww-daniel-mora commented Mar 24, 2019

ww-daniel-mora commented Mar 24, 2019 •

edited

paulmelnikow commented Mar 24, 2019

ww-daniel-mora commented Mar 24, 2019

paulmelnikow commented Mar 24, 2019

ww-daniel-mora commented Mar 25, 2019 •

edited

ww-daniel-mora commented Mar 25, 2019

paulmelnikow commented Mar 26, 2019

paulmelnikow Mar 26, 2019

paulmelnikow Mar 26, 2019

paulmelnikow Mar 26, 2019

ww-daniel-mora commented Mar 27, 2019

paulmelnikow left a comment

paulmelnikow Mar 27, 2019

paulmelnikow Mar 27, 2019

paulmelnikow Mar 27, 2019

paulmelnikow Mar 27, 2019

ww-daniel-mora Mar 27, 2019

paulmelnikow Mar 27, 2019

ww-daniel-mora Mar 27, 2019

paulmelnikow Mar 27, 2019

ww-daniel-mora Mar 27, 2019 •

edited

ww-daniel-mora Mar 27, 2019 •

edited

paulmelnikow commented Apr 12, 2019

paulmelnikow left a comment

ww-daniel-mora commented Apr 15, 2019

paulmelnikow commented Apr 15, 2019

nockbot commented Apr 15, 2019

nockbot commented Aug 12, 2019

fix: Fix .matchHeader() with allowUnmocked #1480

fix: Fix .matchHeader() with allowUnmocked #1480

Conversation

ww-daniel-mora commented Mar 23, 2019 • edited

paulmelnikow commented Mar 23, 2019

ww-daniel-mora commented Mar 24, 2019

ww-daniel-mora commented Mar 24, 2019

paulmelnikow commented Mar 24, 2019

ww-daniel-mora commented Mar 24, 2019

ww-daniel-mora commented Mar 24, 2019 • edited

paulmelnikow commented Mar 24, 2019

ww-daniel-mora commented Mar 24, 2019

paulmelnikow commented Mar 24, 2019

ww-daniel-mora commented Mar 25, 2019 • edited

ww-daniel-mora commented Mar 25, 2019

paulmelnikow commented Mar 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ww-daniel-mora commented Mar 27, 2019

paulmelnikow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ww-daniel-mora Mar 27, 2019 • edited

Choose a reason for hiding this comment

ww-daniel-mora Mar 27, 2019 • edited

Choose a reason for hiding this comment

paulmelnikow commented Apr 12, 2019

paulmelnikow left a comment

Choose a reason for hiding this comment

ww-daniel-mora commented Apr 15, 2019

paulmelnikow commented Apr 15, 2019

nockbot commented Apr 15, 2019

nockbot commented Aug 12, 2019

fix: Fix `.matchHeader()` with `allowUnmocked` #1480

fix: Fix `.matchHeader()` with `allowUnmocked` #1480

ww-daniel-mora commented Mar 23, 2019 •

edited

ww-daniel-mora commented Mar 24, 2019 •

edited

ww-daniel-mora commented Mar 25, 2019 •

edited

ww-daniel-mora Mar 27, 2019 •

edited

ww-daniel-mora Mar 27, 2019 •

edited