Add UI for RuntimeErrors in the ErrorConsole

chrome/browser/ui/webui/extensions/extension_error_handler.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/extensions/extension_error_handler.h"
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/extension.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_data_source.h"
 #include "extensions/browser/extension_error.h"
-#include "extensions/browser/manifest_highlighter.h"
+#include "extensions/browser/file_highlighter.h"
+#include "extensions/common/constants.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace extensions {
 
 namespace {
 
 // Keys for objects passed to and from extension error UI.
 const char kFileTypeKey[] = "fileType";
 const char kManifestFileType[] = "manifest";
 const char kPathSuffixKey[] = "pathSuffix";
+const char kSourceFileType[] = "source";
 const char kTitleKey[] = "title";
 
-const char kBeforeHighlightKey[] = "beforeHighlight";
-const char kHighlightKey[] = "highlight";
-const char kAfterHighlightKey[] = "afterHighlight";
-
-// Populate a DictionaryValue with the highlighted portions for the callback to
-// ExtensionErrorOverlay, given the components.
-void HighlightDictionary(base::DictionaryValue* dict,
-                         const ManifestHighlighter& highlighter) {
-  std::string before_feature = highlighter.GetBeforeFeature();
-  if (!before_feature.empty())
-    dict->SetString(kBeforeHighlightKey, base::UTF8ToUTF16(before_feature));
-
-  std::string feature = highlighter.GetFeature();
-  if (!feature.empty())
-    dict->SetString(kHighlightKey, base::UTF8ToUTF16(feature));
-
-  std::string after_feature = highlighter.GetAfterFeature();
-  if (!after_feature.empty())
-    dict->SetString(kAfterHighlightKey, base::UTF8ToUTF16(after_feature));
-}
-
 }  // namespace
 
 ExtensionErrorHandler::ExtensionErrorHandler(Profile* profile)
     : profile_(profile) {
 }
 
 ExtensionErrorHandler::~ExtensionErrorHandler() {
 }
 
 void ExtensionErrorHandler::GetLocalizedValues(
     content::WebUIDataSource* source) {
   source->AddString(
       "extensionErrorsManifestErrors",
       l10n_util::GetStringUTF16(IDS_EXTENSIONS_ERRORS_MANIFEST_ERRORS));
   source->AddString(
+      "extensionErrorsRuntimeErrors",
+      l10n_util::GetStringUTF16(IDS_EXTENSIONS_ERRORS_RUNTIME_ERRORS));
+  source->AddString(
       "extensionErrorsShowMore",
       l10n_util::GetStringUTF16(IDS_EXTENSIONS_ERRORS_SHOW_MORE));
   source->AddString(
       "extensionErrorsShowFewer",
       l10n_util::GetStringUTF16(IDS_EXTENSIONS_ERRORS_SHOW_FEWER));
   source->AddString(
       "extensionErrorViewSource",
       l10n_util::GetStringUTF16(IDS_EXTENSIONS_ERROR_VIEW_SOURCE));
+  source->AddString(
+      "extensionErrorContext",
+      l10n_util::GetStringUTF16(IDS_EXTENSIONS_ERROR_CONTEXT));
+  source->AddString(
+      "extensionErrorStackTrace",
+      l10n_util::GetStringUTF16(IDS_EXTENSIONS_ERROR_STACK_TRACE));
+  source->AddString(
+      "extensionErrorAnonymousFunction",
+      l10n_util::GetStringUTF16(IDS_EXTENSIONS_ERROR_ANONYMOUS_FUNCTION));
 }
 
 void ExtensionErrorHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "extensionErrorRequestFileSource",
       base::Bind(&ExtensionErrorHandler::HandleRequestFileSource,
                  base::Unretained(this)));
 }
 
 void ExtensionErrorHandler::HandleRequestFileSource(
     const base::ListValue* args) {
   // There should only be one argument, a dictionary. Use this instead of a list
   // because it's more descriptive, harder to accidentally break with minor
   // modifications, and supports optional arguments more easily.
   CHECK_EQ(1u, args->GetSize());
 
   const base::DictionaryValue* dict = NULL;
 
-  // Four required arguments: extension_id, path_suffix, error_message, and
-  // file_type.
+  // Three required arguments: extension_id, path_suffix, and error_message.
   std::string extension_id;
   base::FilePath::StringType path_suffix;
   base::string16 error_message;
-  std::string file_type;
 
   if (!args->GetDictionary(0, &dict) ||
-      !dict->GetString(kFileTypeKey, &file_type) ||
       !dict->GetString(kPathSuffixKey, &path_suffix) ||
       !dict->GetString(ExtensionError::kExtensionIdKey, &extension_id) ||
       !dict->GetString(ExtensionError::kMessageKey, &error_message)) {
     NOTREACHED();
     return;
   }
 
   const Extension* extension =
       ExtensionSystem::Get(Profile::FromWebUI(web_ui()))->
           extension_service()->GetExtensionById(extension_id,
                                                 true /* include disabled */ );
   base::FilePath path = extension->path().Append(path_suffix);

[Finnur, 2013/09/09 09:15:56]

In the event of a compromised caller (sending in values of |args| we don't
normally expect), can this be used to read any manifest.json on disk)? For
example, if I pass in a suffix of:

../<other extension id>/manifest.json

Will I be able to read the contents of another extension's json file?

In other words, don't you need to check that extension->path().IsParent(path) or
use FilePath::AppendRelativePath() instead of Append?

[Devlin, 2013/09/09 16:56:03]

It's possible that, if the caller is compromised, you could see any other file,
yes.  But, is this really a concern?  This won't expose anything that isn't
already in plaintext on the machine -
$ vim
~/.config/google-chrome/<profile>/Extensions/<extension_id>/<version>/manifest.json
will give you the same information, significantly easier...

[Finnur, 2013/09/09 18:05:43]

The person controlling a compromised renderer does not have access to vim as
he/she is not the same person as the one sitting by the keyboard. So I'm not
sure what point you are trying to make here.
Compromised callers are a concern and we can't always anticipate how they will
use the small cracks they find. They've shown great ability to jump through
hoops in the past. As for being able to read arbitrary manifest.json files
(which is what you were referring to)... probably not a huge deal. Looks pretty
innocent, right?

Although there might be potential value in being able to detect which extensions
the user has installed (which you can do if you can request arbitrary
manifests). Also, people have on occasion distributed their private key with
their extension, so I wouldn't put it past them to store something sensitive in
the manifest. Yes, the extension author would look pretty dumb if it gets
exploited, but we'd also look a little dumb for enabling it. And then there's
the question whether any |other| random app out there has ever stored something
sensitive in a file called manifest.json?

Is it likely this would be exploited? No. This is very low on the list of what
would scare a security person. So low it feels a bit silly to be spending any
time debating the merits of this because a) it is super easy to fix and b) that
seems like the right thing to do.

Why not fix it? Do we lose anything?

[Devlin, 2013/09/09 18:26:14]

Thanks for the explanation :)  Fixed in 23875013.

 
   // Setting the title and the error message is the same for all file types.
   scoped_ptr<base::DictionaryValue> results(new base::DictionaryValue);
   results->SetString(kTitleKey,
                      base::UTF8ToUTF16(extension->name()) +
                          base::ASCIIToUTF16(": ") +
                          path.BaseName().LossyDisplayName());
   results->SetString(ExtensionError::kMessageKey, error_message);
 
   base::Closure closure;
   std::string* contents = NULL;
 
-  if (file_type == kManifestFileType) {
+  if (path_suffix == kManifestFilename) {
     std::string manifest_key;
     if (!dict->GetString(ManifestError::kManifestKeyKey, &manifest_key)) {
       NOTREACHED();
       return;
     }
 
     // A "specific" location is optional.
     std::string specific;
     dict->GetString(ManifestError::kManifestSpecificKey, &specific);
 
     contents = new std::string;  // Owned by GetManifestFileCallback(    )

[Finnur, 2013/09/09 09:15:56]

It looks like this leaks as I don't really see this owned and deleted by
GetManifestFileCallback (it is passed in to the highlighter as a const ref where
it is copied but nobody cleans it up).

[Devlin, 2013/09/09 16:56:03]

|contents| is base::Owned by the callback.  This means it will be deleted once
all references to the closure are deleted.  Documentation here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....

[Finnur, 2013/09/09 18:05:43]

Thanks, I overlooked that.

     closure = base::Bind(&ExtensionErrorHandler::GetManifestFileCallback,
                          base::Unretained(this),
                          base::Owned(results.release()),
                          manifest_key,
                          specific,
                          base::Owned(contents));
   } else {
-    // currently, only manifest file types supported.
-    NOTREACHED();
-    return;
+    int line_number = 0;
+    dict->GetInteger(RuntimeError::kLineNumberKey, &line_number);
+
+    contents = new std::string;  // Owned by GetSourceFileCallback()

[Finnur, 2013/09/09 09:15:56]

Same problem here: GetSourceFileCallback passes this to the highlighter as a
const ref but does nothing more with it.

[Devlin, 2013/09/09 16:56:03]

See above :)

+    closure = base::Bind(&ExtensionErrorHandler::GetSourceFileCallback,
+                         base::Unretained(this),
+                         base::Owned(results.release()),
+                         line_number,
+                         base::Owned(contents));
   }
 
   content::BrowserThread::PostBlockingPoolTaskAndReply(
       FROM_HERE,
       base::Bind(base::IgnoreResult(&base::ReadFileToString),
                  path,
                  contents),
       closure);
 }
 
 void ExtensionErrorHandler::GetManifestFileCallback(
     base::DictionaryValue* results,
     const std::string& key,
     const std::string& specific,
     std::string* contents) {
   ManifestHighlighter highlighter(*contents, key, specific);
-  HighlightDictionary(results, highlighter);
+  highlighter.SetHighlightedRegions(results);
   web_ui()->CallJavascriptFunction(
       "extensions.ExtensionErrorOverlay.requestFileSourceResponse", *results);
 }
+
+void ExtensionErrorHandler::GetSourceFileCallback(
+    base::DictionaryValue* results,
+    int line_number,
+    std::string* contents) {
+  SourceHighlighter highlighter(*contents, line_number);
+  highlighter.SetHighlightedRegions(results);
+  web_ui()->CallJavascriptFunction(
+      "extensions.ExtensionErrorOverlay.requestFileSourceResponse", *results);
+}
 
 }  // namespace extensions