From 3ae62615f4248a874c5614098ccbd19444a73fce Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Fri, 24 Mar 2017 01:07:54 +0530 Subject: [PATCH 1/3] net: allow controlling redirects --- atom/browser/api/atom_api_url_request.cc | 37 ++++- atom/browser/api/atom_api_url_request.h | 6 + atom/browser/net/atom_url_request.cc | 55 ++++++- atom/browser/net/atom_url_request.h | 15 +- lib/browser/api/net.js | 12 +- spec/api-net-spec.js | 193 +++++++++++++++++++++++ 6 files changed, 312 insertions(+), 6 deletions(-) diff --git a/atom/browser/api/atom_api_url_request.cc b/atom/browser/api/atom_api_url_request.cc index 967ae50a7e1f..d3607e7283cc 100644 --- a/atom/browser/api/atom_api_url_request.cc +++ b/atom/browser/api/atom_api_url_request.cc @@ -8,6 +8,7 @@ #include "atom/browser/net/atom_url_request.h" #include "atom/common/api/event_emitter_caller.h" #include "atom/common/native_mate_converters/callback.h" +#include "atom/common/native_mate_converters/gurl_converter.h" #include "atom/common/native_mate_converters/net_converter.h" #include "atom/common/native_mate_converters/string16_converter.h" #include "atom/common/node_includes.h" @@ -145,6 +146,8 @@ mate::WrappableBase* URLRequest::New(mate::Arguments* args) { dict.Get("method", &method); std::string url; dict.Get("url", &url); + std::string redirect_policy; + dict.Get("redirect", &redirect_policy); std::string partition; mate::Handle session; if (dict.Get("session", &session)) { @@ -156,8 +159,8 @@ mate::WrappableBase* URLRequest::New(mate::Arguments* args) { } auto browser_context = session->browser_context(); auto api_url_request = new URLRequest(args->isolate(), args->GetThis()); - auto atom_url_request = - AtomURLRequest::Create(browser_context, method, url, api_url_request); + auto atom_url_request = AtomURLRequest::Create( + browser_context, method, url, redirect_policy, api_url_request); api_url_request->atom_request_ = atom_url_request; @@ -176,6 +179,7 @@ void URLRequest::BuildPrototype(v8::Isolate* isolate, .SetMethod("setExtraHeader", &URLRequest::SetExtraHeader) .SetMethod("removeExtraHeader", &URLRequest::RemoveExtraHeader) .SetMethod("setChunkedUpload", &URLRequest::SetChunkedUpload) + .SetMethod("followRedirect", &URLRequest::FollowRedirect) .SetMethod("_setLoadFlags", &URLRequest::SetLoadFlags) .SetProperty("notStarted", &URLRequest::NotStarted) .SetProperty("finished", &URLRequest::Finished) @@ -246,6 +250,17 @@ void URLRequest::Cancel() { Close(); } +void URLRequest::FollowRedirect() { + if (request_state_.Canceled() || request_state_.Closed()) { + return; + } + + DCHECK(atom_request_); + if (atom_request_) { + atom_request_->FollowRedirect(); + } +} + bool URLRequest::SetExtraHeader(const std::string& name, const std::string& value) { // Request state must be in the initial non started state. @@ -305,6 +320,24 @@ void URLRequest::SetLoadFlags(int flags) { } } +void URLRequest::OnReceivedRedirect( + int status_code, + const std::string& method, + const GURL& url, + scoped_refptr response_headers) { + if (request_state_.Canceled() || request_state_.Closed()) { + return; + } + + DCHECK(atom_request_); + if (!atom_request_) { + return; + } + + EmitRequestEvent(false, "redirect", status_code, method, url, + response_headers.get()); +} + void URLRequest::OnAuthenticationRequired( scoped_refptr auth_info) { if (request_state_.Canceled() || request_state_.Closed()) { diff --git a/atom/browser/api/atom_api_url_request.h b/atom/browser/api/atom_api_url_request.h index c92ac01961cd..372ac98ac657 100644 --- a/atom/browser/api/atom_api_url_request.h +++ b/atom/browser/api/atom_api_url_request.h @@ -99,6 +99,11 @@ class URLRequest : public mate::EventEmitter { v8::Local prototype); // Methods for reporting events into JavaScript. + void OnReceivedRedirect( + int status_code, + const std::string& method, + const GURL& url, + scoped_refptr response_headers); void OnAuthenticationRequired( scoped_refptr auth_info); void OnResponseStarted( @@ -170,6 +175,7 @@ class URLRequest : public mate::EventEmitter { bool Failed() const; bool Write(scoped_refptr buffer, bool is_last); void Cancel(); + void FollowRedirect(); bool SetExtraHeader(const std::string& name, const std::string& value); void RemoveExtraHeader(const std::string& name); void SetChunkedUpload(bool is_chunked_upload); diff --git a/atom/browser/net/atom_url_request.cc b/atom/browser/net/atom_url_request.cc index 2c7bb61da0b1..19c526700d20 100644 --- a/atom/browser/net/atom_url_request.cc +++ b/atom/browser/net/atom_url_request.cc @@ -13,6 +13,7 @@ #include "net/base/io_buffer.h" #include "net/base/load_flags.h" #include "net/base/upload_bytes_element_reader.h" +#include "net/url_request/redirect_info.h" namespace { const int kBufferSize = 4096; @@ -58,6 +59,7 @@ scoped_refptr AtomURLRequest::Create( AtomBrowserContext* browser_context, const std::string& method, const std::string& url, + const std::string& redirect_policy, api::URLRequest* delegate) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); @@ -76,7 +78,7 @@ scoped_refptr AtomURLRequest::Create( if (content::BrowserThread::PostTask( content::BrowserThread::IO, FROM_HERE, base::Bind(&AtomURLRequest::DoInitialize, atom_url_request, - request_context_getter, method, url))) { + request_context_getter, method, url, redirect_policy))) { return atom_url_request; } return nullptr; @@ -93,10 +95,12 @@ void AtomURLRequest::Terminate() { void AtomURLRequest::DoInitialize( scoped_refptr request_context_getter, const std::string& method, - const std::string& url) { + const std::string& url, + const std::string& redirect_policy) { DCHECK_CURRENTLY_ON(content::BrowserThread::IO); DCHECK(request_context_getter); + redirect_policy_ = redirect_policy; request_context_getter_ = request_context_getter; request_context_getter_->AddObserver(this); auto context = request_context_getter_->GetURLRequestContext(); @@ -150,6 +154,13 @@ void AtomURLRequest::Cancel() { base::Bind(&AtomURLRequest::DoCancel, this)); } +void AtomURLRequest::FollowRedirect() { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + content::BrowserThread::PostTask( + content::BrowserThread::IO, FROM_HERE, + base::Bind(&AtomURLRequest::DoFollowRedirect, this)); +} + void AtomURLRequest::SetExtraHeader(const std::string& name, const std::string& value) const { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); @@ -246,6 +257,13 @@ void AtomURLRequest::DoCancel() { DoTerminate(); } +void AtomURLRequest::DoFollowRedirect() { + DCHECK_CURRENTLY_ON(content::BrowserThread::IO); + if (request_ && request_->is_redirecting() && redirect_policy_ == "manual") { + request_->FollowDeferredRedirect(); + } +} + void AtomURLRequest::DoSetExtraHeader(const std::string& name, const std::string& value) const { DCHECK_CURRENTLY_ON(content::BrowserThread::IO); @@ -297,6 +315,29 @@ void AtomURLRequest::DoSetLoadFlags(int flags) const { request_->SetLoadFlags(request_->load_flags() | flags); } +void AtomURLRequest::OnReceivedRedirect(net::URLRequest* request, + const net::RedirectInfo& info, + bool* defer_redirect) { + DCHECK_CURRENTLY_ON(content::BrowserThread::IO); + if (!request_ || redirect_policy_ == "follow") + return; + + if (redirect_policy_ == "error") { + request->Cancel(); + DoCancelWithError( + "Request cannot follow redirect with the current redirect mode", true); + } else if (redirect_policy_ == "manual") { + *defer_redirect = true; + scoped_refptr response_headers = + request->response_headers(); + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::Bind(&AtomURLRequest::InformDelegateReceivedRedirect, this, + info.status_code, info.new_method, info.new_url, + response_headers)); + } +} + void AtomURLRequest::OnAuthRequired(net::URLRequest* request, net::AuthChallengeInfo* auth_info) { DCHECK_CURRENTLY_ON(content::BrowserThread::IO); @@ -399,6 +440,16 @@ bool AtomURLRequest::CopyAndPostBuffer(int bytes_read) { buffer_copy)); } +void AtomURLRequest::InformDelegateReceivedRedirect( + int status_code, + const std::string& method, + const GURL& url, + scoped_refptr response_headers) const { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + if (delegate_) + delegate_->OnReceivedRedirect(status_code, method, url, response_headers); +} + void AtomURLRequest::InformDelegateAuthenticationRequired( scoped_refptr auth_info) const { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); diff --git a/atom/browser/net/atom_url_request.h b/atom/browser/net/atom_url_request.h index db00390b955c..654798d8aac8 100644 --- a/atom/browser/net/atom_url_request.h +++ b/atom/browser/net/atom_url_request.h @@ -30,12 +30,14 @@ class AtomURLRequest : public base::RefCountedThreadSafe, AtomBrowserContext* browser_context, const std::string& method, const std::string& url, + const std::string& redirect_policy, api::URLRequest* delegate); void Terminate(); bool Write(scoped_refptr buffer, bool is_last); void SetChunkedUpload(bool is_chunked_upload); void Cancel(); + void FollowRedirect(); void SetExtraHeader(const std::string& name, const std::string& value) const; void RemoveExtraHeader(const std::string& name) const; void PassLoginInformation(const base::string16& username, @@ -44,6 +46,9 @@ class AtomURLRequest : public base::RefCountedThreadSafe, protected: // Overrides of net::URLRequest::Delegate + void OnReceivedRedirect(net::URLRequest* request, + const net::RedirectInfo& info, + bool* defer_redirect) override; void OnAuthRequired(net::URLRequest* request, net::AuthChallengeInfo* auth_info) override; void OnResponseStarted(net::URLRequest* request) override; @@ -60,11 +65,13 @@ class AtomURLRequest : public base::RefCountedThreadSafe, void DoInitialize(scoped_refptr, const std::string& method, - const std::string& url); + const std::string& url, + const std::string& redirect_policy); void DoTerminate(); void DoWriteBuffer(scoped_refptr buffer, bool is_last); void DoCancel(); + void DoFollowRedirect(); void DoSetExtraHeader(const std::string& name, const std::string& value) const; void DoRemoveExtraHeader(const std::string& name) const; @@ -77,6 +84,11 @@ class AtomURLRequest : public base::RefCountedThreadSafe, void ReadResponse(); bool CopyAndPostBuffer(int bytes_read); + void InformDelegateReceivedRedirect( + int status_code, + const std::string& method, + const GURL& url, + scoped_refptr response_headers) const; void InformDelegateAuthenticationRequired( scoped_refptr auth_info) const; void InformDelegateResponseStarted( @@ -92,6 +104,7 @@ class AtomURLRequest : public base::RefCountedThreadSafe, scoped_refptr request_context_getter_; bool is_chunked_upload_; + std::string redirect_policy_; std::unique_ptr chunked_stream_; std::unique_ptr chunked_stream_writer_; std::vector> diff --git a/lib/browser/api/net.js b/lib/browser/api/net.js index 10d903919e9d..049cef9e3ab4 100644 --- a/lib/browser/api/net.js +++ b/lib/browser/api/net.js @@ -156,9 +156,15 @@ class ClientRequest extends EventEmitter { urlStr = url.format(urlObj) } + const redirectPolicy = options.redirect || 'follow' + if (!['follow', 'error', 'manual'].includes(redirectPolicy)) { + throw new Error('redirect mode should be one of follow, error or manual') + } + let urlRequestOptions = { method: method, - url: urlStr + url: urlStr, + redirect: redirectPolicy } if (options.session) { if (options.session instanceof Session) { @@ -339,6 +345,10 @@ class ClientRequest extends EventEmitter { return this._write(data, encoding, callback, true) } + followRedirect () { + this.urlRequest.followRedirect() + } + abort () { this.urlRequest.cancel() } diff --git a/spec/api-net-spec.js b/spec/api-net-spec.js index 49fe2edadcce..80f24a46e9c4 100644 --- a/spec/api-net-spec.js +++ b/spec/api-net-spec.js @@ -906,6 +906,199 @@ describe('net module', function () { urlRequest.end() }) + it('should throw if given an invalid redirect mode', function (done) { + const requestUrl = '/requestUrl' + try { + const urlRequest = net.request({ + url: `${server.url}${requestUrl}`, + redirect: 'custom' + }) + urlRequest + } catch (exception) { + done() + } + }) + + it('should follow redirect when no redirect mode is provided', function (done) { + const requestUrl = '/301' + server.on('request', function (request, response) { + switch (request.url) { + case '/301': + response.statusCode = '301' + response.setHeader('Location', '/200') + response.end() + break + case '/200': + response.statusCode = '200' + response.end() + break + default: + assert(false) + } + }) + const urlRequest = net.request({ + url: `${server.url}${requestUrl}` + }) + urlRequest.on('response', function (response) { + assert.equal(response.statusCode, 200) + done() + }) + urlRequest.end() + }) + + it('should follow redirect chain when no redirect mode is provided', function (done) { + const requestUrl = '/redirectChain' + server.on('request', function (request, response) { + switch (request.url) { + case '/redirectChain': + response.statusCode = '301' + response.setHeader('Location', '/301') + response.end() + break + case '/301': + response.statusCode = '301' + response.setHeader('Location', '/200') + response.end() + break + case '/200': + response.statusCode = '200' + response.end() + break + default: + assert(false) + } + }) + const urlRequest = net.request({ + url: `${server.url}${requestUrl}` + }) + urlRequest.on('response', function (response) { + assert.equal(response.statusCode, 200) + done() + }) + urlRequest.end() + }) + + it('should not follow redirect when mode is error', function (done) { + const requestUrl = '/301' + server.on('request', function (request, response) { + switch (request.url) { + case '/301': + response.statusCode = '301' + response.setHeader('Location', '/200') + response.end() + break + case '/200': + response.statusCode = '200' + response.end() + break + default: + assert(false) + } + }) + const urlRequest = net.request({ + url: `${server.url}${requestUrl}`, + redirect: 'error' + }) + urlRequest.on('error', function (error) { + assert.equal(error.message, 'Request cannot follow redirect with the current redirect mode') + }) + urlRequest.on('close', function () { + done() + }) + urlRequest.end() + }) + + it('should allow follow redirect when mode is manual', function (done) { + const requestUrl = '/redirectChain' + let redirectCount = 0 + server.on('request', function (request, response) { + switch (request.url) { + case '/redirectChain': + response.statusCode = '301' + response.setHeader('Location', '/301') + response.end() + break + case '/301': + response.statusCode = '301' + response.setHeader('Location', '/200') + response.end() + break + case '/200': + response.statusCode = '200' + response.end() + break + default: + assert(false) + } + }) + const urlRequest = net.request({ + url: `${server.url}${requestUrl}`, + redirect: 'manual' + }) + urlRequest.on('response', function (response) { + assert.equal(response.statusCode, 200) + assert.equal(redirectCount, 2) + done() + }) + urlRequest.on('redirect', function (status, method, url) { + if (url === `${server.url}/301` || url === `${server.url}/200`) { + redirectCount += 1 + urlRequest.followRedirect() + } + }) + urlRequest.end() + }) + + it('should allow cancelling redirect when mode is manual', function (done) { + const requestUrl = '/redirectChain' + let redirectCount = 0 + server.on('request', function (request, response) { + switch (request.url) { + case '/redirectChain': + response.statusCode = '301' + response.setHeader('Location', '/redirect/1') + response.end() + break + case '/redirect/1': + response.statusCode = '200' + response.setHeader('Location', '/redirect/2') + response.end() + break + case '/redirect/2': + response.statusCode = '200' + response.end() + break + default: + assert(false) + } + }) + const urlRequest = net.request({ + url: `${server.url}${requestUrl}`, + redirect: 'manual' + }) + urlRequest.on('response', function (response) { + assert.equal(response.statusCode, 200) + response.pause() + response.on('data', function (chunk) { + }) + response.on('end', function () { + urlRequest.abort() + }) + response.resume() + }) + urlRequest.on('close', function () { + assert.equal(redirectCount, 1) + done() + }) + urlRequest.on('redirect', function (status, method, url) { + if (url === `${server.url}/redirect/1`) { + redirectCount += 1 + urlRequest.followRedirect() + } + }) + urlRequest.end() + }) + it('should throw if given an invalid session option', function (done) { const requestUrl = '/requestUrl' try { From 8db1eacd1c19f0570bd76783ac1077ae2e831f85 Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Sat, 25 Mar 2017 12:37:34 +0530 Subject: [PATCH 2/3] [skip ci] add docs --- docs/api/client-request.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/docs/api/client-request.md b/docs/api/client-request.md index 71ade14a8ef3..4a16d9510ec4 100644 --- a/docs/api/client-request.md +++ b/docs/api/client-request.md @@ -29,6 +29,11 @@ the hostname and the port number 'hostname:port' * `hostname` String (optional) - The server host name. * `port` Integer (optional) - The server's listening port number. * `path` String (optional) - The path part of the request URL. + * `redirect` String (optional) - The redirect mode for this request. Should be +one of `follow`, `error` or `manual`. Defaults to `follow`. When mode is `error`, +any redirection will be aborted. When mode is `manual` the redirection will be +deferred until [`request.followRedirect`](#requestfollowRedirect) is invoked. Listen for the [`redirect`](#event-redirect) event in +this mode to get more details about the redirect request. `options` properties such as `protocol`, `host`, `hostname`, `port` and `path` strictly follow the Node.js model as described in the @@ -121,6 +126,19 @@ Emitted as the last event in the HTTP request-response transaction. The `close` event indicates that no more events will be emitted on either the `request` or `response` objects. + +#### Event: 'redirect' + +Returns: + +* `statusCode` Integer +* `method` String +* `redirectUrl` String +* `responseHeaders` Object + +Emitted when there is redirection and the mode is `manual`. Calling +[`request.followRedirect`](#requestfollowRedirect) will continue with the redirection. + ### Instance Properties #### `request.chunkedEncoding` @@ -192,3 +210,7 @@ Cancels an ongoing HTTP transaction. If the request has already emitted the `close` event, the abort operation will have no effect. Otherwise an ongoing event will emit `abort` and `close` events. Additionally, if there is an ongoing response object,it will emit the `aborted` event. + +#### `request.followRedirect()` + +Continues any deferred redirection request when the redirection mode is `manual`. From b14c4dcdc060d0f2f0f7133ce371e94b7b00ee34 Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Tue, 28 Mar 2017 19:05:44 +0530 Subject: [PATCH 3/3] address review comments --- spec/api-net-spec.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/spec/api-net-spec.js b/spec/api-net-spec.js index 80f24a46e9c4..68db813d6495 100644 --- a/spec/api-net-spec.js +++ b/spec/api-net-spec.js @@ -906,17 +906,15 @@ describe('net module', function () { urlRequest.end() }) - it('should throw if given an invalid redirect mode', function (done) { + it('should throw if given an invalid redirect mode', function () { const requestUrl = '/requestUrl' - try { - const urlRequest = net.request({ - url: `${server.url}${requestUrl}`, - redirect: 'custom' - }) - urlRequest - } catch (exception) { - done() + const options = { + url: `${server.url}${requestUrl}`, + redirect: 'custom' } + assert.throws(function () { + net.request(options) + }, 'redirect mode should be one of follow, error or manual') }) it('should follow redirect when no redirect mode is provided', function (done) {