Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

https requests failing when using a proxy #89

Closed
nabeelamir-defra opened this issue Feb 17, 2021 · 7 comments
Closed

https requests failing when using a proxy #89

nabeelamir-defra opened this issue Feb 17, 2021 · 7 comments

Comments

@nabeelamir-defra
Copy link

I'm using the dynamics-web-api to make https requests via the squid proxy. The https request works fine when doing a curl command. But I get the following error "The requested URL could not be retrieved".

When the curl command is run in squid I can see:
225 172.17.0.1 TCP_TUNNEL/200 5122 CONNECT example.com:443 - HIER_DIRECT/40.110.1111.111 -
Whereas when using the dynamics API I get
0 172.17.0.1 TAG_NONE/501 5910 GET example.com - HIER_NONE/- text/html

I presume it's down to this https://github.com/AleksandrRogov/DynamicsWebApi/blob/v1.7.1/lib/requests/http.js#L51. But just wanted to confirm. It probably needs to make a CONNECT request first then perform the request. https://stackoverflow.com/questions/34078995/how-can-i-use-an-https-proxy-with-node-js-https-request-client

Would the ideal solution be to use something like axios to perform the requests?

@AleksandrRogov
Copy link
Owner

hi @nabeelamir-defra,
I don't use proxies in my day-to-day job, so this change was made by one of my contributors and will be quite difficult for me to test, because I don't have a proxy to test this.

I can try and include "tls" package in there, only the questions is, how do we know if tls is needed during the request?

@nabeelamir-defra
Copy link
Author

Hi @AleksandrRogov, thank you for your quick reply! I've created a repo here to demonstrate the issue dynamics-web-api-proxy-error. It creates the squid proxy in docker, so you won't have to install it separately.

I guess if the request contains https, then use tls? I think something like axios instead of the http and https node library will solve this. I can take a look at this, but due to other commitments it may take a while.

@AleksandrRogov
Copy link
Owner

@nabeelamir-defra you are probably right, I'll need to look into rewriting request module for node and replace core http/https with something better.

nabeelamir-defra added a commit to DEFRA/rod-licensing-tests that referenced this issue Feb 19, 2021
* Easy Renewal Acceptance tests failing when run in Jenkins

https://eaflood.atlassian.net/browse/IWTF-1903

Fix for the easy renewal acceptance failing in jenkins

* commenting out easy renewals test for now until this issue is resolved AleksandrRogov/DynamicsWebApi#89
@AleksandrRogov
Copy link
Owner

@nabeelamir-defra I've just replaced http/https with axios, but it still does not work (same error).

Probably because of this issue: axios/axios#925

Any thoughts?

I don't want to commit this (because it's definitely broken) but here's an http.js code for you to test it in your small package.

Just install axios and replace this file: lib/requests/http.js

var axios = require('axios').default;
var http = require('http');
var https = require('https');
var url = require('url');
var parseResponse = require('./helpers/parseResponse');
var ErrorHelper = require('../helpers/ErrorHelper');

var axiosInstance;

function execRequest(requestConfig) {
	if (!axiosInstance) {
		var axiosConfig = {
			//disable json parsing
			transformResponse: function (res) {
				return res;
			},
			// always resolve
			validateStatus: function () {
				return true;
			},
			//keepAlive pools and reuses TCP connections, so it's faster
			httpAgent: new http.Agent({ keepAlive: true }),
			httpsAgent: new https.Agent({ keepAlive: true })
		};

		axiosInstance = axios.create(axiosConfig);
	}

	return axiosInstance(requestConfig);
}

/**
 * Sends a request to given URL with given parameters
 *
 */
var httpRequest = function (options) {
	var method = options.method;
	var uri = options.uri;
	var data = options.data;
	var additionalHeaders = options.additionalHeaders;
	var responseParams = options.responseParams;
	var successCallback = options.successCallback;
	var errorCallback = options.errorCallback;
	var timeout = options.timeout;
	var requestId = options.requestId;

	var headers = {};

	if (data) {
		headers["Content-Type"] = additionalHeaders['Content-Type'];
		headers["Content-Length"] = data.length;

		delete additionalHeaders['Content-Type'];
	}

	//set additional headers
	for (var key in additionalHeaders) {
		headers[key] = additionalHeaders[key];
	}

	var parsedUrl = url.parse(uri);
	var protocol = parsedUrl.protocol.replace(':', '');

	var axiosConfig = {
		method: method.toLowerCase(),
		url: uri,
		timeout: timeout
	}

	if (process.env[`${protocol}_proxy`]) {
		headers.host = parsedUrl.host;

		var proxyUrl = url.parse(process.env.http_proxy);
		axiosConfig.proxy = {
			protocol: proxyUrl.protocol,
			host: proxyUrl.hostname,
			port: proxyUrl.port
		}
	}

	axiosConfig.headers = headers;

	if (data) {
		axiosConfig.data = data;
	}

	execRequest(axiosConfig).then(function (res) {
		switch (res.status) {
			case 200: // Success with content returned in response body.
			case 201: // Success with content returned in response body.
			case 204: // Success with no content returned in response body.
			case 206: //Success with partial content
			case 304: {// Success with Not Modified
				var responseData = parseResponse(res.data, res.headers, responseParams[requestId]);

				var response = {
					data: responseData,
					headers: res.headers,
					status: res.status
				};

				delete responseParams[requestId];

				successCallback(response);
				break;
			}
			default: // All other statuses are error cases.
				var crmError;
				try {
					var errorParsed = parseResponse(res.data, res.headers, responseParams[requestId]);

					if (Array.isArray(errorParsed)) {
						errorCallback(errorParsed);
						break;
					}

					crmError = errorParsed.hasOwnProperty('error') && errorParsed.error
						? errorParsed.error
						: { message: errorParsed.Message };

				} catch (e) {
					if (res.data.length > 0) {
						crmError = { message: res.data };
					}
					else {
						crmError = { message: "Unexpected Error" };
					}
				}

				delete responseParams[requestId];

				errorCallback(ErrorHelper.handleHttpError(crmError, {
					status: res.status, statusText: res.statusText, statusMessage: null, headers: res.headers
				}));

				break;
		}
	}).catch(function (error) {
		//catch timeout and other unexpected errors
		delete responseParams[requestId];

		var crmError = {};

		if (error.code)
			crmError.code = error.code;

		if (error.message)
			crmError.message = error.message;

		errorCallback(ErrorHelper.handleHttpError(crmError));
	});
};

module.exports = httpRequest;

@AleksandrRogov
Copy link
Owner

@nabeelamir-defra I have resolved this issue, had to use "https-proxy-agent" package. I will review the changes and make a patch as soon as possible.

@nabeelamir-defra
Copy link
Author

Sorry for getting back to you so late. Did you use the https-proxy-agent with the existing solution or with axios?

Using axios seem to be more robust and require less code. The axios code looks ok (I haven't tested it), but you don't need to configure the proxy.

if (process.env[`${protocol}_proxy`]) {
		headers.host = parsedUrl.host;

		var proxyUrl = url.parse(process.env.http_proxy);
		axiosConfig.proxy = {
			protocol: proxyUrl.protocol,
			host: proxyUrl.hostname,
			port: proxyUrl.port
		}
	}

axios respects the http_proxy, https_proxy and no_proxy environment variables already. From the docs:

// You can also define your proxy using the conventional `http_proxy` and
  // `https_proxy` environment variables. If you are using environment variables
  // for your proxy configuration, you can also define a `no_proxy` environment
  // variable as a comma-separated list of domains that should not be proxied.

And the code
https://github.com/axios/axios/blob/master/lib/adapters/http.js#L125

@AleksandrRogov
Copy link
Owner

@nabeelamir-defra I am not going to use axios, because those packages do not require it and I don't want to introduce another dependecy (I've already added 2). Hope it will work for you fine. The patch will be ready soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants