Commit f4345313 authored by Lukasz Anforowicz's avatar Lukasz Anforowicz Committed by Commit Bot

New test: Plugins (e.g. Flash) VS Cross-Origin Read Blocking (CORB).

This CL adds tests that verify that Cross-Origin Read Blocking (CORB)
won't block HTTP responses for requests initiated by plugins that have
been granted universal access (like Flash which has its own CORS-like
mechanism - crossdomain.xml).

In addition to running tryjobs, I've also tested this CL by temporarily
editing CrossSiteDocumentResourceHandler::ShouldBlockBasedOnHeaders to
comment out the exception carved out for plugins (the new
TestTrustedCorbEligibleRequest test expectedly failed with such an edit
both with and without NetworkService).

Bug: 846339
Change-Id: I7b7befff3018ed5a7bd834b2bf0507c2e1f6365b
Reviewed-on: https://chromium-review.googlesource.com/1101300
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: default avatarJohn Abd-El-Malek <jam@chromium.org>
Reviewed-by: default avatarRaymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568081}
parent 31ea1563
...@@ -420,6 +420,7 @@ TEST_PPAPI_NACL(HostResolverPrivate_ResolveIPv4) ...@@ -420,6 +420,7 @@ TEST_PPAPI_NACL(HostResolverPrivate_ResolveIPv4)
RunTestViaHTTP( \ RunTestViaHTTP( \
LIST_TEST(URLLoader_UntrustedSameOriginRestriction) \ LIST_TEST(URLLoader_UntrustedSameOriginRestriction) \
LIST_TEST(URLLoader_UntrustedCrossOriginRequest) \ LIST_TEST(URLLoader_UntrustedCrossOriginRequest) \
LIST_TEST(URLLoader_UntrustedCorbEligibleRequest) \
LIST_TEST(URLLoader_UntrustedJavascriptURLRestriction) \ LIST_TEST(URLLoader_UntrustedJavascriptURLRestriction) \
LIST_TEST(DISABLED_URLLoader_TrustedJavascriptURLRestriction) \ LIST_TEST(DISABLED_URLLoader_TrustedJavascriptURLRestriction) \
) )
...@@ -428,6 +429,7 @@ TEST_PPAPI_NACL(HostResolverPrivate_ResolveIPv4) ...@@ -428,6 +429,7 @@ TEST_PPAPI_NACL(HostResolverPrivate_ResolveIPv4)
RunTestViaHTTP( \ RunTestViaHTTP( \
LIST_TEST(URLLoader_UntrustedSameOriginRestriction) \ LIST_TEST(URLLoader_UntrustedSameOriginRestriction) \
LIST_TEST(URLLoader_UntrustedCrossOriginRequest) \ LIST_TEST(URLLoader_UntrustedCrossOriginRequest) \
LIST_TEST(URLLoader_UntrustedCorbEligibleRequest) \
LIST_TEST(URLLoader_UntrustedJavascriptURLRestriction) \ LIST_TEST(URLLoader_UntrustedJavascriptURLRestriction) \
LIST_TEST(DISABLED_URLLoader_TrustedJavascriptURLRestriction) \ LIST_TEST(DISABLED_URLLoader_TrustedJavascriptURLRestriction) \
) )
...@@ -449,6 +451,7 @@ TEST_PPAPI_NACL(HostResolverPrivate_ResolveIPv4) ...@@ -449,6 +451,7 @@ TEST_PPAPI_NACL(HostResolverPrivate_ResolveIPv4)
RunTestViaHTTP( \ RunTestViaHTTP( \
LIST_TEST(URLLoader_TrustedSameOriginRestriction) \ LIST_TEST(URLLoader_TrustedSameOriginRestriction) \
LIST_TEST(URLLoader_TrustedCrossOriginRequest) \ LIST_TEST(URLLoader_TrustedCrossOriginRequest) \
LIST_TEST(URLLoader_TrustedCorbEligibleRequest) \
LIST_TEST(URLLoader_TrustedHttpRequests) \ LIST_TEST(URLLoader_TrustedHttpRequests) \
LIST_TEST(URLLoader_XRequestedWithHeader) \ LIST_TEST(URLLoader_XRequestedWithHeader) \
) )
......
...@@ -578,6 +578,11 @@ bool CrossSiteDocumentResourceHandler::ShouldBlockBasedOnHeaders( ...@@ -578,6 +578,11 @@ bool CrossSiteDocumentResourceHandler::ShouldBlockBasedOnHeaders(
// header. Other plugin requests (e.g., NaCl) are made using CORS and have an // header. Other plugin requests (e.g., NaCl) are made using CORS and have an
// Origin request header. If they fail the CORS check above, they should be // Origin request header. If they fail the CORS check above, they should be
// blocked. // blocked.
// TODO(lukasza): Only disable CORB for plugins with universal access (see
// PepperURLLoaderHost::has_universal_access_), because only such plugins may
// have their own CORS-like mechanisms - e.g. crossdomain.xml in Flash). We
// should still enforce CORB for other kinds of plugins (i.e. ones without
// universal access).
if (info->GetResourceType() == RESOURCE_TYPE_PLUGIN_RESOURCE && if (info->GetResourceType() == RESOURCE_TYPE_PLUGIN_RESOURCE &&
is_nocors_plugin_request_) { is_nocors_plugin_request_) {
return false; return false;
......
...@@ -149,7 +149,11 @@ test_common_source_files = [ ...@@ -149,7 +149,11 @@ test_common_source_files = [
copy("copy_test_files") { copy("copy_test_files") {
visibility = [ ":*" ] visibility = [ ":*" ]
sources = [ sources = [
# Keep "test_case.html.mock-http-headers" with "test_case.html". # Keep "test_case.html.mock-http-headers" with "test_case.html"
# and "corb_eligible_resource.json.mock-http-headers" with
# "corb_eligible_resource.json".
"tests/corb_eligible_resource.json",
"tests/corb_eligible_resource.json.mock-http-headers",
"tests/test_case.html", "tests/test_case.html",
"tests/test_case.html.mock-http-headers", "tests/test_case.html.mock-http-headers",
"tests/test_page.css", "tests/test_page.css",
......
HTTP/1.1 200 OK
Content-Type: text/json
X-Content-Type-Options: nosniff
...@@ -147,9 +147,11 @@ void TestURLLoader::RunTests(const std::string& filter) { ...@@ -147,9 +147,11 @@ void TestURLLoader::RunTests(const std::string& filter) {
// These tests connect to localhost with privacy mode enabled: // These tests connect to localhost with privacy mode enabled:
RUN_CALLBACK_TEST(TestURLLoader, UntrustedSameOriginRestriction, filter); RUN_CALLBACK_TEST(TestURLLoader, UntrustedSameOriginRestriction, filter);
RUN_CALLBACK_TEST(TestURLLoader, UntrustedCrossOriginRequest, filter); RUN_CALLBACK_TEST(TestURLLoader, UntrustedCrossOriginRequest, filter);
RUN_CALLBACK_TEST(TestURLLoader, UntrustedCorbEligibleRequest, filter);
// These tests connect to localhost with privacy mode disabled: // These tests connect to localhost with privacy mode disabled:
RUN_CALLBACK_TEST(TestURLLoader, TrustedSameOriginRestriction, filter); RUN_CALLBACK_TEST(TestURLLoader, TrustedSameOriginRestriction, filter);
RUN_CALLBACK_TEST(TestURLLoader, TrustedCrossOriginRequest, filter); RUN_CALLBACK_TEST(TestURLLoader, TrustedCrossOriginRequest, filter);
RUN_CALLBACK_TEST(TestURLLoader, TrustedCorbEligibleRequest, filter);
} }
std::string TestURLLoader::ReadEntireFile(pp::FileIO* file_io, std::string TestURLLoader::ReadEntireFile(pp::FileIO* file_io,
...@@ -302,7 +304,7 @@ int32_t TestURLLoader::OpenUntrusted(const std::string& method, ...@@ -302,7 +304,7 @@ int32_t TestURLLoader::OpenUntrusted(const std::string& method,
request.SetMethod(method); request.SetMethod(method);
request.SetHeaders(header); request.SetHeaders(header);
return OpenUntrusted(request); return OpenUntrusted(request, NULL);
} }
int32_t TestURLLoader::OpenTrusted(const std::string& method, int32_t TestURLLoader::OpenTrusted(const std::string& method,
...@@ -312,25 +314,50 @@ int32_t TestURLLoader::OpenTrusted(const std::string& method, ...@@ -312,25 +314,50 @@ int32_t TestURLLoader::OpenTrusted(const std::string& method,
request.SetMethod(method); request.SetMethod(method);
request.SetHeaders(header); request.SetHeaders(header);
return OpenTrusted(request); return OpenTrusted(request, NULL);
} }
int32_t TestURLLoader::OpenUntrusted(const pp::URLRequestInfo& request) { int32_t TestURLLoader::OpenUntrusted(const pp::URLRequestInfo& request,
return Open(request, false); std::string* response_body) {
return Open(request, false, response_body);
} }
int32_t TestURLLoader::OpenTrusted(const pp::URLRequestInfo& request) { int32_t TestURLLoader::OpenTrusted(const pp::URLRequestInfo& request,
return Open(request, true); std::string* response_body) {
return Open(request, true, response_body);
} }
int32_t TestURLLoader::Open(const pp::URLRequestInfo& request, int32_t TestURLLoader::Open(const pp::URLRequestInfo& request,
bool trusted) { bool trusted,
std::string* response_body) {
pp::URLLoader loader(instance_); pp::URLLoader loader(instance_);
if (trusted) if (trusted)
url_loader_trusted_interface_->GrantUniversalAccess(loader.pp_resource()); url_loader_trusted_interface_->GrantUniversalAccess(loader.pp_resource());
TestCompletionCallback callback(instance_->pp_instance(), callback_type());
callback.WaitForResult(loader.Open(request, callback.GetCallback())); {
return callback.result(); TestCompletionCallback open_callback(instance_->pp_instance(),
callback_type());
open_callback.WaitForResult(
loader.Open(request, open_callback.GetCallback()));
if (open_callback.result() != PP_OK)
return open_callback.result();
}
int32_t bytes_read = 0;
do {
char buffer[1024];
TestCompletionCallback read_callback(instance_->pp_instance(),
callback_type());
read_callback.WaitForResult(loader.ReadResponseBody(
&buffer, sizeof(buffer), read_callback.GetCallback()));
bytes_read = read_callback.result();
if (bytes_read < 0)
return bytes_read;
if (response_body)
response_body->append(std::string(buffer, bytes_read));
} while (bytes_read > 0);
return PP_OK;
} }
std::string TestURLLoader::TestBasicGET() { std::string TestURLLoader::TestBasicGET() {
...@@ -442,7 +469,7 @@ std::string TestURLLoader::TestFailsBogusContentLength() { ...@@ -442,7 +469,7 @@ std::string TestURLLoader::TestFailsBogusContentLength() {
static_cast<uint32_t>(postdata.length())); static_cast<uint32_t>(postdata.length()));
int32_t rv; int32_t rv;
rv = OpenUntrusted(request); rv = OpenUntrusted(request, NULL);
if (rv != PP_ERROR_NOACCESS) if (rv != PP_ERROR_NOACCESS)
return ReportError( return ReportError(
"Untrusted request with bogus Content-Length restriction", rv); "Untrusted request with bogus Content-Length restriction", rv);
...@@ -485,7 +512,7 @@ std::string TestURLLoader::TestUntrustedSameOriginRestriction() { ...@@ -485,7 +512,7 @@ std::string TestURLLoader::TestUntrustedSameOriginRestriction() {
std::string cross_origin_url = GetReachableCrossOriginURL("test_case.html"); std::string cross_origin_url = GetReachableCrossOriginURL("test_case.html");
request.SetURL(cross_origin_url); request.SetURL(cross_origin_url);
int32_t rv = OpenUntrusted(request); int32_t rv = OpenUntrusted(request, NULL);
if (rv != PP_ERROR_NOACCESS) if (rv != PP_ERROR_NOACCESS)
return ReportError( return ReportError(
"Untrusted, unintended cross-origin request restriction", rv); "Untrusted, unintended cross-origin request restriction", rv);
...@@ -499,7 +526,7 @@ std::string TestURLLoader::TestTrustedSameOriginRestriction() { ...@@ -499,7 +526,7 @@ std::string TestURLLoader::TestTrustedSameOriginRestriction() {
std::string cross_origin_url = GetReachableCrossOriginURL("test_case.html"); std::string cross_origin_url = GetReachableCrossOriginURL("test_case.html");
request.SetURL(cross_origin_url); request.SetURL(cross_origin_url);
int32_t rv = OpenTrusted(request); int32_t rv = OpenTrusted(request, NULL);
if (rv != PP_OK) if (rv != PP_OK)
return ReportError("Trusted cross-origin request failed", rv); return ReportError("Trusted cross-origin request failed", rv);
...@@ -513,7 +540,7 @@ std::string TestURLLoader::TestUntrustedCrossOriginRequest() { ...@@ -513,7 +540,7 @@ std::string TestURLLoader::TestUntrustedCrossOriginRequest() {
request.SetURL(cross_origin_url); request.SetURL(cross_origin_url);
request.SetAllowCrossOriginRequests(true); request.SetAllowCrossOriginRequests(true);
int32_t rv = OpenUntrusted(request); int32_t rv = OpenUntrusted(request, NULL);
if (rv != PP_OK) if (rv != PP_OK)
return ReportError( return ReportError(
"Untrusted, intended cross-origin request failed", rv); "Untrusted, intended cross-origin request failed", rv);
...@@ -528,19 +555,71 @@ std::string TestURLLoader::TestTrustedCrossOriginRequest() { ...@@ -528,19 +555,71 @@ std::string TestURLLoader::TestTrustedCrossOriginRequest() {
request.SetURL(cross_origin_url); request.SetURL(cross_origin_url);
request.SetAllowCrossOriginRequests(true); request.SetAllowCrossOriginRequests(true);
int32_t rv = OpenTrusted(request); int32_t rv = OpenTrusted(request, NULL);
if (rv != PP_OK) if (rv != PP_OK)
return ReportError("Trusted cross-origin request failed", rv); return ReportError("Trusted cross-origin request failed", rv);
PASS(); PASS();
} }
// CORB (Cross-Origin Read Blocking) should apply to plugins without universal
// access. This test is very similar to TestUntrustedSameOriginRestriction, but
// explicitly uses a CORB-eligible response (test/json + nosniff).
std::string TestURLLoader::TestUntrustedCorbEligibleRequest() {
// It is important to use a CORB-eligible response here: text/json + nosniff.
std::string cross_origin_url =
GetReachableCrossOriginURL("corb_eligible_resource.json");
pp::URLRequestInfo request(instance_);
request.SetURL(cross_origin_url);
request.SetAllowCrossOriginRequests(true);
std::string response_body;
int32_t rv = OpenUntrusted(request, &response_body);
// Main verification - the response should be blocked. Ideally the blocking
// should be done before the data leaves the browser and/or network-service
// process (the test doesn't verify this though).
if (rv != PP_ERROR_NOACCESS) {
return ReportError("Untrusted Javascript URL request restriction failed",
rv);
}
ASSERT_EQ("", response_body);
PASS();
}
// CORB (Cross-Origin Read Blocking) shouldn't apply to plugins with universal
// access (see PepperURLLoaderHost::has_universal_access_) - such plugins may
// have their own CORS-like mechanisms - e.g. crossdomain.xml in Flash).
// This test is quite similar to TestTrustedSameOriginRestriction, but it
// explicitly uses a CORB-eligible response (test/json + nosniff) and also
// explicitly verifies that the response body was not blocked.
std::string TestURLLoader::TestTrustedCorbEligibleRequest() {
// It is important to use a CORB-eligible response here: text/json + nosniff.
std::string cross_origin_url =
GetReachableCrossOriginURL("corb_eligible_resource.json");
pp::URLRequestInfo request(instance_);
request.SetURL(cross_origin_url);
request.SetAllowCrossOriginRequests(true);
std::string response_body;
int32_t rv = OpenTrusted(request, &response_body);
if (rv != PP_OK)
return ReportError("Trusted CORB-eligible request failed", rv);
// Main verification - if CORB blocked the response, then |response_body|
// would be empty.
ASSERT_EQ("{ \"foo\": \"bar\" }\n", response_body);
PASS();
}
// Untrusted Javascript URLs requests should fail. // Untrusted Javascript URLs requests should fail.
std::string TestURLLoader::TestUntrustedJavascriptURLRestriction() { std::string TestURLLoader::TestUntrustedJavascriptURLRestriction() {
pp::URLRequestInfo request(instance_); pp::URLRequestInfo request(instance_);
request.SetURL("javascript:foo = bar"); request.SetURL("javascript:foo = bar");
int32_t rv = OpenUntrusted(request); int32_t rv = OpenUntrusted(request, NULL);
if (rv != PP_ERROR_NOACCESS) if (rv != PP_ERROR_NOACCESS)
return ReportError( return ReportError(
"Untrusted Javascript URL request restriction failed", rv); "Untrusted Javascript URL request restriction failed", rv);
...@@ -553,7 +632,7 @@ std::string TestURLLoader::TestTrustedJavascriptURLRestriction() { ...@@ -553,7 +632,7 @@ std::string TestURLLoader::TestTrustedJavascriptURLRestriction() {
pp::URLRequestInfo request(instance_); pp::URLRequestInfo request(instance_);
request.SetURL("javascript:foo = bar"); request.SetURL("javascript:foo = bar");
int32_t rv = OpenTrusted(request); int32_t rv = OpenTrusted(request, NULL);
if (rv == PP_ERROR_NOACCESS) if (rv == PP_ERROR_NOACCESS)
return ReportError( return ReportError(
"Trusted Javascript URL request", rv); "Trusted Javascript URL request", rv);
...@@ -604,7 +683,7 @@ std::string TestURLLoader::TestUntrustedHttpRequests() { ...@@ -604,7 +683,7 @@ std::string TestURLLoader::TestUntrustedHttpRequests() {
pp::URLRequestInfo request(instance_); pp::URLRequestInfo request(instance_);
request.SetCustomReferrerURL("http://www.google.com/"); request.SetCustomReferrerURL("http://www.google.com/");
int32_t rv = OpenUntrusted(request); int32_t rv = OpenUntrusted(request, NULL);
if (rv != PP_ERROR_NOACCESS) if (rv != PP_ERROR_NOACCESS)
return ReportError( return ReportError(
"Untrusted request with custom referrer restriction", rv); "Untrusted request with custom referrer restriction", rv);
...@@ -614,7 +693,7 @@ std::string TestURLLoader::TestUntrustedHttpRequests() { ...@@ -614,7 +693,7 @@ std::string TestURLLoader::TestUntrustedHttpRequests() {
pp::URLRequestInfo request(instance_); pp::URLRequestInfo request(instance_);
request.SetCustomContentTransferEncoding("foo"); request.SetCustomContentTransferEncoding("foo");
int32_t rv = OpenUntrusted(request); int32_t rv = OpenUntrusted(request, NULL);
if (rv != PP_ERROR_NOACCESS) if (rv != PP_ERROR_NOACCESS)
return ReportError( return ReportError(
"Untrusted request with content-transfer-encoding restriction", rv); "Untrusted request with content-transfer-encoding restriction", rv);
...@@ -660,7 +739,7 @@ std::string TestURLLoader::TestTrustedHttpRequests() { ...@@ -660,7 +739,7 @@ std::string TestURLLoader::TestTrustedHttpRequests() {
pp::URLRequestInfo request(instance_); pp::URLRequestInfo request(instance_);
request.SetCustomReferrerURL("http://www.google.com/"); request.SetCustomReferrerURL("http://www.google.com/");
int32_t rv = OpenTrusted(request); int32_t rv = OpenTrusted(request, NULL);
if (rv != PP_OK) if (rv != PP_OK)
return ReportError("Trusted request with custom referrer", rv); return ReportError("Trusted request with custom referrer", rv);
} }
...@@ -669,7 +748,7 @@ std::string TestURLLoader::TestTrustedHttpRequests() { ...@@ -669,7 +748,7 @@ std::string TestURLLoader::TestTrustedHttpRequests() {
pp::URLRequestInfo request(instance_); pp::URLRequestInfo request(instance_);
request.SetCustomContentTransferEncoding("foo"); request.SetCustomContentTransferEncoding("foo");
int32_t rv = OpenTrusted(request); int32_t rv = OpenTrusted(request, NULL);
if (rv != PP_OK) if (rv != PP_OK)
return ReportError( return ReportError(
"Trusted request with content-transfer-encoding failed", rv); "Trusted request with content-transfer-encoding failed", rv);
...@@ -824,7 +903,7 @@ int32_t TestURLLoader::OpenWithPrefetchBufferThreshold(int32_t lower, ...@@ -824,7 +903,7 @@ int32_t TestURLLoader::OpenWithPrefetchBufferThreshold(int32_t lower,
request.SetPrefetchBufferLowerThreshold(lower); request.SetPrefetchBufferLowerThreshold(lower);
request.SetPrefetchBufferUpperThreshold(upper); request.SetPrefetchBufferUpperThreshold(upper);
return OpenUntrusted(request); return OpenUntrusted(request, NULL);
} }
std::string TestURLLoader::TestPrefetchBufferThreshold() { std::string TestURLLoader::TestPrefetchBufferThreshold() {
......
...@@ -41,14 +41,17 @@ class TestURLLoader : public TestCase { ...@@ -41,14 +41,17 @@ class TestURLLoader : public TestCase {
std::string* message); std::string* message);
std::string GetReachableAbsoluteURL(const std::string& file_name); std::string GetReachableAbsoluteURL(const std::string& file_name);
std::string GetReachableCrossOriginURL(const std::string& file_name); std::string GetReachableCrossOriginURL(const std::string& file_name);
int32_t OpenUntrusted(const pp::URLRequestInfo& request); int32_t OpenUntrusted(const pp::URLRequestInfo& request,
int32_t OpenTrusted(const pp::URLRequestInfo& request); std::string* response_body);
int32_t OpenTrusted(const pp::URLRequestInfo& request,
std::string* response_body);
int32_t OpenUntrusted(const std::string& method, int32_t OpenUntrusted(const std::string& method,
const std::string& header); const std::string& header);
int32_t OpenTrusted(const std::string& method, int32_t OpenTrusted(const std::string& method,
const std::string& header); const std::string& header);
int32_t Open(const pp::URLRequestInfo& request, int32_t Open(const pp::URLRequestInfo& request,
bool with_universal_access); bool with_universal_access,
std::string* response_body);
int32_t OpenWithPrefetchBufferThreshold(int32_t lower, int32_t upper); int32_t OpenWithPrefetchBufferThreshold(int32_t lower, int32_t upper);
std::string TestBasicGET(); std::string TestBasicGET();
...@@ -65,6 +68,8 @@ class TestURLLoader : public TestCase { ...@@ -65,6 +68,8 @@ class TestURLLoader : public TestCase {
std::string TestTrustedSameOriginRestriction(); std::string TestTrustedSameOriginRestriction();
std::string TestUntrustedCrossOriginRequest(); std::string TestUntrustedCrossOriginRequest();
std::string TestTrustedCrossOriginRequest(); std::string TestTrustedCrossOriginRequest();
std::string TestUntrustedCorbEligibleRequest();
std::string TestTrustedCorbEligibleRequest();
std::string TestUntrustedJavascriptURLRestriction(); std::string TestUntrustedJavascriptURLRestriction();
std::string TestTrustedJavascriptURLRestriction(); std::string TestTrustedJavascriptURLRestriction();
std::string TestUntrustedHttpRequests(); std::string TestUntrustedHttpRequests();
......
...@@ -409,6 +409,9 @@ ...@@ -409,6 +409,9 @@
-FetchBehaviorTests/ContentVerifierHashTest.TamperRequestedResourceTamperComputedHashes/0 -FetchBehaviorTests/ContentVerifierHashTest.TamperRequestedResourceTamperComputedHashes/0
-FetchBehaviorTests/ContentVerifierHashTest.TamperRequestedResourceTamperComputedHashes/1 -FetchBehaviorTests/ContentVerifierHashTest.TamperRequestedResourceTamperComputedHashes/1
# https://crbug.com/846339: CORB: Don't block requests from plugins.
-OutOfProcessPPAPITest.URLLoaderTrusted
# NOTE: if adding an exclusion for an existing failure (e.g. additional test for # NOTE: if adding an exclusion for an existing failure (e.g. additional test for
# feature X that is already not working), please add it beside the existing # feature X that is already not working), please add it beside the existing
# failures. Otherwise please reach out to network-service-dev@. # failures. Otherwise please reach out to network-service-dev@.
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment