diff options
| -rw-r--r-- | base/file_path.cc | 69 | ||||
| -rw-r--r-- | base/file_path.h | 12 | ||||
| -rw-r--r-- | base/file_path_unittest.cc | 92 | ||||
| -rw-r--r-- | chrome/browser/extensions/extensions_service_unittest.cc | 16 | ||||
| -rw-r--r-- | chrome/common/extensions/extension_resource.cc | 28 | ||||
| -rw-r--r-- | chrome/common/extensions/extension_resource_unittest.cc | 7 |
6 files changed, 34 insertions, 190 deletions
diff --git a/base/file_path.cc b/base/file_path.cc index 2d9d26e..dde8ae1 100644 --- a/base/file_path.cc +++ b/base/file_path.cc @@ -171,75 +171,6 @@ bool FilePath::operator!=(const FilePath& that) const { #endif // defined(FILE_PATH_USES_DRIVE_LETTERS) } -bool FilePath::AppendAndResolveRelative(const FilePath& relative_path, - FilePath* path) const { - DCHECK(path); - if (!path || relative_path.IsAbsolute()) - return false; - - FilePath full_path = Append(relative_path); - // Is it worth looking for parent references? - if (!full_path.ReferencesParent()) { - *path = full_path; - return true; - } - - // If the parent has a drive letter, then we must not remove the first - // component, which is the drive letter. - bool drive_letter = (FindDriveLetter(full_path.path_) != - FilePath::StringType::npos); - - std::vector<FilePath::StringType> components; - full_path.GetComponents(&components); - std::vector<FilePath::StringType>::iterator it = components.begin(); - // Start by removing any kCurrentDirectory component, since they may - // fool us into not going back to the appropriate parent level. - for (; it != components.end(); ++it) { - if (*it == kCurrentDirectory) { - // erase returns an iterator to the next component. - it = components.erase(it); - // So now, go back to previous iterator, - // so that we can appropriately process the next one as we loop. - --it; - } - } - - // Now parse the component looking for kParentDirectory and remove them as - // well as the previous component. - it = components.begin(); - for (; it != components.end(); ++it) { - if (*it == kParentDirectory) { - // Did we reach the beginning? - if (it == components.begin() || - (drive_letter && (it - 1) == components.begin())) { - return false; - } - // Remove the previous component, as well as the current one. - std::vector<FilePath::StringType>::iterator previous = it - 1; - // Unless the previous is at the beginning. - if (previous == components.begin() || - (drive_letter && (previous - 1) == components.begin())) { - return false; - } - // vector::erase doesn't erase _Last, it erases [_First, _Last[, - // so we must increment current which we want erased. - it = components.erase(previous, it + 1); - // And go back to previous so that we can process the next one as we loop. - --it; - } - } - - // Now reconstruct the path with the components that were left in. - it = components.begin(); - // We start with the first component, in case it is absolute - // and absolute paths can't be appended. - *path = FilePath(*it); - for (++it; it != components.end(); ++it) - *path = path->Append(*it); - - return true; -} - bool FilePath::IsParent(const FilePath& child) const { return AppendRelativePath(child, NULL); } diff --git a/base/file_path.h b/base/file_path.h index 7900922..0d0bdad 100644 --- a/base/file_path.h +++ b/base/file_path.h @@ -178,18 +178,6 @@ class FilePath { // and BaseName().value() on each child component. void GetComponents(std::vector<FilePath::StringType>* components) const; - // Returns true, and sets *path to resulting full path, if relative_path can - // be applied to current path by resolving any '..' it may contain. Returns - // false otherwise, e.g., if relative path is absolute, or if it climbs back - // up the hierarchy too far (i.e., beyond the root of current path). - // - // Note that if the current path ends with a file name, we won't try to - // figure it out (so this method doesn't go to the disk) and we will blindly - // append relative_path at the end of the current path, including the file - // name in the current path (if any). - bool AppendAndResolveRelative(const FilePath& relative_path, - FilePath* path) const; - // Returns true if this FilePath is a strict parent of the |child|. Absolute // and relative paths are accepted i.e. is /foo parent to /foo/bar and // is foo parent to foo/bar. Does not convert paths to absolute, follow diff --git a/base/file_path_unittest.cc b/base/file_path_unittest.cc index 2673220..92659f1 100644 --- a/base/file_path_unittest.cc +++ b/base/file_path_unittest.cc @@ -502,98 +502,6 @@ TEST_F(FilePathTest, PathComponentsTest) { } } -TEST_F(FilePathTest, AppendAndResolveRelativeTest) { - const struct BinaryTestData cases[] = { -#if defined(FILE_PATH_USES_DRIVE_LETTERS) - { { FPL("c:/"), FPL("foo") }, FPL("c:/foo") }, - { { FPL("f:/foo/bar"), FPL("..") }, FPL("f:/foo") }, - { { FPL("f:/foo.bar"), FPL("..") }, FPL("f:/") }, - { { FPL("F:/foo/.."), FPL("./bar/.") }, FPL("F:/bar") }, - { { FPL("E:/Foo/bar"), FPL("../..") }, FPL("E:/") }, - { { FPL("E:/Foo/bar/."), FPL("../..") }, FPL("E:/") }, - { { FPL("e:/foo/.."), FPL("bar/..") }, FPL("e:/") }, - { { FPL("c:/foo/./bar/.."), FPL("../baz") }, FPL("c:/baz") }, - { { FPL("E:/./foo/bar/.."), FPL("../baz/..") }, FPL("E:/") }, - { { FPL("x:/foo/../bar/.."), FPL("baz/../boo") }, FPL("x:/boo") }, - { { FPL("E:/foo.bar/.."), FPL("../baz/..") }, FPL("") }, - { { FPL("Z:/foo"), FPL("../..") }, FPL("") }, - { { FPL("y:/"), FPL("..") }, FPL("") }, - { { FPL("B:/.."), FPL("bar/.") }, FPL("") }, - { { FPL("a:/foo/.."), FPL("..") }, FPL("") }, - { { FPL("r:/.."), FPL("..") }, FPL("") }, - { { FPL("F:/foo/.."), FPL("../..") }, FPL("") }, - { { FPL("O:/foo/bar/.."), FPL("../..") }, FPL("") }, -#endif // FILE_PATH_USES_DRIVE_LETTERS -#if defined(FILE_PATH_USES_WIN_SEPARATORS) - { { FPL("\\\\"), FPL("foo") }, FPL("\\\\foo") }, - { { FPL("\\\\foo"), FPL("bar") }, FPL("\\\\foo\\bar") }, - { { FPL("\\\\foo\\bar"), FPL("..") }, FPL("\\\\foo") }, - { { FPL("\\\\foo.bar"), FPL("..") }, FPL("\\\\") }, - { { FPL("\\\\Foo\\bar"), FPL("..\\..") }, FPL("\\\\") }, - { { FPL("\\\\Foo\\bar\\."), FPL("..\\..") }, FPL("\\\\") }, - { { FPL("\\\\foo\\bar"), FPL("foo\\..\\baz") }, FPL("\\\\foo\\bar\\baz") }, - { { FPL("\\\\foo\\.\\bar"), FPL("..\\baz\\.") }, FPL("\\\\foo\\baz") }, - { { FPL("\\\\.\\foo\\.."), FPL("bar") }, FPL("\\\\bar") }, - { { FPL("\\\\foo\\.."), FPL(".\\bar\\..") }, FPL("\\\\") }, - { { FPL("\\\\foo\\bar\\.."), FPL("..\\baz") }, FPL("\\\\baz") }, - { { FPL("\\\\foo\\bar\\.."), FPL("..\\baz\\..") }, FPL("\\\\") }, - { { FPL("\\\\foo\\..\\bar\\.."), FPL("baz\\..\\boo") }, FPL("\\\\boo"), }, - { { FPL("\\\\foo.bar\\.."), FPL("..\\baz\\..") }, FPL("") }, - { { FPL("\\\\foo"), FPL("..\\..") }, FPL("") }, - { { FPL("\\\\"), FPL("..") }, FPL("") }, - { { FPL("\\\\.."), FPL("bar\\.") }, FPL("") }, - { { FPL("\\\\foo\\.."), FPL("..") }, FPL("") }, - { { FPL("\\\\.."), FPL("..") }, FPL("") }, - { { FPL("\\\\foo\\.."), FPL("..\\..") }, FPL("") }, - { { FPL("\\\\foo\\bar\\.."), FPL("..\\..") }, FPL("") }, -#if defined(FILE_PATH_USES_DRIVE_LETTERS) - { { FPL("E:/foo"), FPL("bar") }, FPL("E:/foo\\bar") }, - { { FPL("C:/foo/bar"), FPL("foo/../baz") }, FPL("C:/foo\\bar\\baz") }, - { { FPL("e:/foo/bar"), FPL("../baz") }, FPL("e:/foo\\baz") }, -#endif -#else // FILE_PATH_USES_WIN_SEPARAORS - { { FPL("/"), FPL("foo") }, FPL("/foo") }, - { { FPL("/foo"), FPL("bar") }, FPL("/foo/bar") }, - { { FPL("/foo/bar/"), FPL("..") }, FPL("/foo") }, - { { FPL("/foo.bar"), FPL("..") }, FPL("/") }, - { { FPL("//foo"), FPL("..") }, FPL("//") }, - { { FPL("/foo/./bar"), FPL("../..") }, FPL("/") }, - { { FPL("/foo/bar/."), FPL("foo/../baz") }, FPL("/foo/bar/baz") }, - { { FPL("/./foo/bar"), FPL("../baz/.") }, FPL("/foo/baz") }, - { { FPL("/foo/.."), FPL("./bar") }, FPL("/bar") }, - { { FPL("/foo/.."), FPL("bar/..") }, FPL("/") }, - { { FPL("//foo/bar/.."), FPL("../baz") }, FPL("//baz") }, - { { FPL("/foo/bar/.."), FPL("../baz/..") }, FPL("/") }, - { { FPL("/foo/../bar/.."), FPL("baz/../boo") }, FPL("/boo") }, - { { FPL("//foo.bar/.."), FPL("../baz") }, FPL("") }, - { { FPL("/foo"), FPL("../..") }, FPL("") }, - { { FPL("//"), FPL("..") }, FPL("") }, - { { FPL("/.."), FPL("./bar") }, FPL("") }, - { { FPL("/foo/.."), FPL("..") }, FPL("") }, - { { FPL("/.."), FPL("..") }, FPL("") }, - { { FPL("/foo/.."), FPL("../..") }, FPL("") }, - { { FPL("/foo/bar/.."), FPL("../..") }, FPL("") }, -#if defined(FILE_PATH_USES_DRIVE_LETTERS) - { { FPL("E:/foo"), FPL("bar") }, FPL("E:/foo/bar") }, - { { FPL("C:/foo/bar"), FPL("foo/../baz") }, FPL("C:/foo/bar/baz") }, - { { FPL("e:/foo/bar"), FPL("../baz") }, FPL("e:/foo/baz") }, -#endif -#endif // FILE_PATH_USES_WIN_SEPARAORS - }; - - for (size_t i = 0; i < arraysize(cases); ++i) { - FilePath parent(cases[i].inputs[0]); - FilePath child(cases[i].inputs[1]); - - FilePath result; - EXPECT_EQ(cases[i].expected[0] != '\0', - parent.AppendAndResolveRelative(child, &result)) << - "i: " << i << ", parent: " << parent.value() << ", child: " << - child.value(); - EXPECT_STREQ(cases[i].expected, result.value().c_str()); - } -} - TEST_F(FilePathTest, IsParentTest) { const struct BinaryBooleanTestData cases[] = { { { FPL("/"), FPL("/foo/bar/baz") }, true}, diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc index 45cad68..e6ed987 100644 --- a/chrome/browser/extensions/extensions_service_unittest.cc +++ b/chrome/browser/extensions/extensions_service_unittest.cc @@ -542,19 +542,23 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) { EXPECT_EQ(2u, scripts[0].js_scripts().size()); ExtensionResource resource00(scripts[0].js_scripts()[0].extension_root(), scripts[0].js_scripts()[0].relative_path()); - EXPECT_TRUE(resource00.ComparePathWithDefault( - extension->path().AppendASCII("script1.js"))); + FilePath expected_path(extension->path().AppendASCII("script1.js")); + ASSERT_TRUE(file_util::AbsolutePath(&expected_path)); + EXPECT_TRUE(resource00.ComparePathWithDefault(expected_path)); ExtensionResource resource01(scripts[0].js_scripts()[1].extension_root(), scripts[0].js_scripts()[1].relative_path()); - EXPECT_TRUE(resource01.ComparePathWithDefault( - extension->path().AppendASCII("script2.js"))); + expected_path = extension->path().AppendASCII("script2.js"); + ASSERT_TRUE(file_util::AbsolutePath(&expected_path)); + EXPECT_TRUE(resource01.ComparePathWithDefault(expected_path)); EXPECT_TRUE(extension->plugins().empty()); EXPECT_EQ(1u, scripts[1].url_patterns().size()); EXPECT_EQ("http://*.news.com/*", scripts[1].url_patterns()[0].GetAsString()); ExtensionResource resource10(scripts[1].js_scripts()[0].extension_root(), scripts[1].js_scripts()[0].relative_path()); - EXPECT_TRUE(resource10.ComparePathWithDefault( - extension->path().AppendASCII("js_files").AppendASCII("script3.js"))); + expected_path = + extension->path().AppendASCII("js_files").AppendASCII("script3.js"); + ASSERT_TRUE(file_util::AbsolutePath(&expected_path)); + EXPECT_TRUE(resource10.ComparePathWithDefault(expected_path)); const std::vector<URLPattern> permissions = extension->host_permissions(); ASSERT_EQ(2u, permissions.size()); EXPECT_EQ("http://*.google.com/*", permissions[0].GetAsString()); diff --git a/chrome/common/extensions/extension_resource.cc b/chrome/common/extensions/extension_resource.cc index 7b2a833..6c1cff6 100644 --- a/chrome/common/extensions/extension_resource.cc +++ b/chrome/common/extensions/extension_resource.cc @@ -41,21 +41,35 @@ FilePath ExtensionResource::GetFilePath(const FilePath& extension_root, extension_l10n_util::GetL10nRelativePaths(relative_path, &l10n_relative_paths); + // We need to resolve the parent references in the extension_root + // path on its own because IsParent doesn't like parent references. + FilePath clean_extension_root(extension_root); + if (!file_util::AbsolutePath(&clean_extension_root)) + return FilePath(); + // Stat l10n file(s), and return new path if it exists. for (size_t i = 0; i < l10n_relative_paths.size(); ++i) { - FilePath full_path; - if (extension_root.AppendAndResolveRelative(l10n_relative_paths[i], - &full_path) && - extension_root.IsParent(full_path) && + FilePath full_path = clean_extension_root.Append(l10n_relative_paths[i]); + if (file_util::AbsolutePath(&full_path) && + clean_extension_root.IsParent(full_path) && file_util::PathExists(full_path)) { return full_path; } } // Fall back to root resource. - FilePath full_path; - if (extension_root.AppendAndResolveRelative(relative_path, &full_path) && - extension_root.IsParent(full_path)) { + FilePath full_path = clean_extension_root.Append(relative_path); + + // We must resolve the absolute path of the combined path when + // the relative path contains references to a parent folder (i.e., '..'). + // We also check if the path exists because the posix version of AbsolutePath + // will fail if the path doesn't exist, and we want the same behavior on + // Windows... So until the posix and Windows version of AbsolutePath are + // unified, we need an extra call to PathExists, unfortunately. + // TODO(mad): Fix this once AbsolutePath is unified. + if (file_util::AbsolutePath(&full_path) && + file_util::PathExists(full_path) && + clean_extension_root.IsParent(full_path)) { return full_path; } diff --git a/chrome/common/extensions/extension_resource_unittest.cc b/chrome/common/extensions/extension_resource_unittest.cc index f467e47..807452e 100644 --- a/chrome/common/extensions/extension_resource_unittest.cc +++ b/chrome/common/extensions/extension_resource_unittest.cc @@ -35,12 +35,10 @@ TEST(ExtensionResourceTest, CreateWithMissingResourceOnDisk) { relative_path = relative_path.AppendASCII("cira.js"); ExtensionResource resource(root_path, relative_path); + // The path doesn't exist on disk, we will be returned an empty path. EXPECT_EQ(root_path.value(), resource.extension_root().value()); EXPECT_EQ(relative_path.value(), resource.relative_path().value()); - EXPECT_EQ(ToLower(root_path.Append(relative_path).value()), - ToLower(resource.GetFilePath().value())); - - EXPECT_FALSE(resource.GetFilePath().empty()); + EXPECT_TRUE(resource.GetFilePath().empty()); } TEST(ExtensionResourceTest, CreateWithAllResourcesOnDisk) { @@ -76,6 +74,7 @@ TEST(ExtensionResourceTest, CreateWithAllResourcesOnDisk) { ASSERT_FALSE(locales.empty()); FilePath expected_path; expected_path = l10n_path.AppendASCII(locales[0]).AppendASCII(filename); + ASSERT_TRUE(file_util::AbsolutePath(&expected_path)); EXPECT_EQ(ToLower(expected_path.value()), ToLower(resolved_path.value())); EXPECT_EQ(ToLower(temp.path().value()), |