Improve console log message for CORS failure

third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp

 /*
  * Copyright (C) 2008 Apple Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 250 matching lines @@
             headerSet.add(header);
         return;
     }
     parseAccessControlExposeHeadersAllowList(response.httpHeaderField(HTTPNames::Access_Control_Expose_Headers), headerSet);
 }
 
 bool CrossOriginAccessControl::isLegalRedirectLocation(const KURL& requestURL, String& errorDescription)
 {
     // CORS restrictions imposed on Location: URL -- http://www.w3.org/TR/cors/#redirect-steps (steps 2 + 3.)
     if (!SchemeRegistry::shouldTreatURLSchemeAsCORSEnabled(requestURL.protocol())) {
-        errorDescription = "The request was redirected to a URL ('" + requestURL.getString() + "') which has a disallowed scheme for cross-origin requests.";
+        errorDescription = "Redirect location '" + requestURL.getString() + "' has a disallowed scheme for cross-origin requests.";
         return false;
     }
 
     if (!(requestURL.user().isEmpty() && requestURL.pass().isEmpty())) {
-        errorDescription = "The request was redirected to a URL ('" + requestURL.getString() + "') containing userinfo, which is disallowed for cross-origin requests.";
+        errorDescription = "Redirect location '" + requestURL.getString() + "' contains userinfo, which is disallowed for cross-origin requests.";
         return false;
     }
 
     return true;
 }
 
 bool CrossOriginAccessControl::handleRedirect(SecurityOrigin* securityOrigin, ResourceRequest& newRequest, const ResourceResponse& redirectResponse, StoredCredentials withCredentials, ResourceLoaderOptions& options, String& errorMessage)
 {
     // http://www.w3.org/TR/cors/#redirect-steps terminology:
     const KURL& originalURL = redirectResponse.url();
     const KURL& newURL = newRequest.url();
 
     bool redirectCrossOrigin = !securityOrigin->canRequest(newURL);
 
     // Same-origin request URLs that redirect are allowed without checking access.
     if (!securityOrigin->canRequest(originalURL)) {
         // Follow http://www.w3.org/TR/cors/#redirect-steps
         String errorDescription;
 
         // Steps 3 & 4 - check if scheme and other URL restrictions hold.
-        bool allowRedirect = isLegalRedirectLocation(newURL, errorDescription);
-        if (allowRedirect) {
-            // Step 5: perform resource sharing access check.
-            allowRedirect = passesAccessControlCheck(redirectResponse, withCredentials, securityOrigin, errorDescription, newRequest.requestContext());
-            if (allowRedirect) {
-                RefPtr<SecurityOrigin> originalOrigin = SecurityOrigin::create(originalURL);
-                // Step 6: if the request URL origin is not same origin as the original URL's,
-                // set the source origin to a globally unique identifier.
-                if (!originalOrigin->canRequest(newURL)) {
-                    options.securityOrigin = SecurityOrigin::createUnique();
-                    securityOrigin = options.securityOrigin.get();
-                }
-            }
+        if (!isLegalRedirectLocation(newURL, errorDescription)) {
+            errorMessage = "Redirect from '" + originalURL.getString() + "' has been blocked by CORS policy: " + errorDescription;
+            return false;
         }
-        if (!allowRedirect) {
-            const String& originalOrigin = SecurityOrigin::create(originalURL)->toString();
-            errorMessage = "Redirect at origin '" + originalOrigin + "' has been blocked from loading by Cross-Origin Resource Sharing policy: " + errorDescription;
+
+        // Step 5: perform resource sharing access check.
+        if (!passesAccessControlCheck(redirectResponse, withCredentials, securityOrigin, errorDescription, newRequest.requestContext())) {
+            errorMessage = "Redirect from '" + originalURL.getString() + "' has been blocked because the redirect response does not pass CORS check: '" + errorDescription;

[sof, 2016/07/24 07:48:00]

Would it be worth making the error message format be consistent with the above
(and ResourceFetcher's),

  "Redirect from <origin> has been blocked by CORS policy: <errorDescription>

?

[tyoshino (SeeGerritForStatus), 2016/07/26 09:13:00]

Good point. I initially changed this message to the following considering that
it might be good to clarify which of the redirect location or the redirect
response's CORS headers was bad.

"Redirect from <origin> has been blocked by CORS policy: Redirect response does
not pass CORS check: <errorDescription>"

And then, I noticed two CORS are redundant and consolidated the sentences.

I don't have strong opinion. Changed back to have better consistency.

             return false;
         }
+
+        RefPtr<SecurityOrigin> originalOrigin = SecurityOrigin::create(originalURL);
+        // Step 6: if the request URL origin is not same origin as the original URL's,
+        // set the source origin to a globally unique identifier.
+        if (!originalOrigin->canRequest(newURL)) {

[sof, 2016/07/24 07:48:00]

Some SecurityOrigin churn for this check, but i guess url security origins will
be cached.

[tyoshino (SeeGerritForStatus), 2016/07/26 09:13:00]

Could you please elaborate the comment? URL -> SecurityOrigin caching works only
for Blob URLs.

[sof, 2016/07/27 11:51:03]

You're right, I thought some wider class of URLs were being cached for their
origins.

It is not an oft-used code path or anything, but having to allocate a pair of
temporary SecurityOrigins here, feels unsatisfactory - that's just what the
drive-by comment was trying to express. But it is how such checks are done.

[tyoshino (SeeGerritForStatus), 2016/07/28 12:23:05]

We could have a variant of canRequest() which takes a SecurityOrigin. It looks
canAccess() is not such a thing.

+            options.securityOrigin = SecurityOrigin::createUnique();
+            securityOrigin = options.securityOrigin.get();
+        }
     }
     if (redirectCrossOrigin) {
         // If now to a different origin, update/set Origin:.
         newRequest.clearHTTPOrigin();
         newRequest.setHTTPOrigin(securityOrigin);
         // If the user didn't request credentials in the first place, update our
         // state so we neither request them nor expect they must be allowed.
         if (options.credentialsRequested == ClientDidNotRequestCredentials)
             options.allowCredentials = DoNotAllowStoredCredentials;
     }
     return true;
 }
 
 } // namespace blink