Skip to content

Commit

Permalink
feat(request_overrider): Set IncomingMessage.client for parity with…
Browse files Browse the repository at this point in the history
… real requests

`ClientMessage.socket` is an instance of our `socket` class. As that class’ constructor sets `authorized` to `true` on `https` requests, the code here that sets `response.socket.authorized` is pointless.

`ClientMessage.client` was deprecated in iojs 2.2 and per request/request#1615  but is still aliased: https://github.com/nodejs/node/blob/2e613a9c301165d121b19b86e382860323abc22f/lib/_http_incoming.js#L67

The old code, which sets `response.client.authorized`, is being replaced with new code which aliases `response.client`. This ensures that `response.client` and `response.socket` point to the same thing, which provides parity with a non-overridden request. This may help with compatibility with libraries which depend on that undocumented property, such as some very old versions of `request`.

This also ensures that `response.client.authorized` remains set, without needing to set it directly.

This is being changed now to remove two bits of unreachable code in the conditional.
  • Loading branch information
paulmelnikow authored and gr2m committed Jan 19, 2019
1 parent 6d2a312 commit dc71a3b
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 11 deletions.
22 changes: 11 additions & 11 deletions lib/request_overrider.js
Original file line number Diff line number Diff line change
Expand Up @@ -471,17 +471,17 @@ function RequestOverrider(req, options, interceptors, remove, cb) {
return
}

/// response.client.authorized = true
/// fixes https://github.com/pgte/nock/issues/158
response.client = _.extend(response.client || {}, {
authorized: true,
})

// Account for updates to Node.js response interface
// cf https://github.com/request/request/pull/1615
response.socket = _.extend(response.socket || {}, {
authorized: true,
})
// `IncomingMessage.client` is an undocumented alias for
// `IncomingMessage.socket`. Assigning it here may help with
// compatibility, including with very old versions of `request` which
// inspect `response.client.authorized`. Modern versions of request
// inspect `response.socket.authorized` which is set to true in our
// `Socket` constructor.
// https://github.com/pgte/nock/issues/158
// https://github.com/request/request/pull/1615
// https://nodejs.org/api/http.html#http_response_socket
// https://github.com/nodejs/node/blob/2e613a9c301165d121b19b86e382860323abc22f/lib/_http_incoming.js#L67
response.client = response.socket

// Evaluate functional headers.
const evaluatedHeaders = {}
Expand Down
1 change: 1 addition & 0 deletions lib/socket.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ function Socket(options) {
EventEmitter.apply(this)

if (options.proto === 'https') {
// https://github.com/nock/nock/issues/158
this.authorized = true
}

Expand Down
1 change: 1 addition & 0 deletions tests/test_intercept.js
Original file line number Diff line number Diff line change
Expand Up @@ -3828,6 +3828,7 @@ test('mocking succeeds even when host request header is not specified', t => {
)
})

// https://github.com/nock/nock/issues/158
test('mikeal/request with strictSSL: true', t => {
nock('https://strictssl.com')
.post('/what')
Expand Down

0 comments on commit dc71a3b

Please sign in to comment.