I0184 (CppMicroServices#1102) (CppMicroServices#1170)

* Issue CppMicroServices#184: added check for bundle name matching manifest value

Causes test failures... not exactly sure how to resolve them.

* Updated test cases to work properly with new restrictions

* added test for mismatched bundle name and bundle.symbolic_name

* responded to comments and refactored some code.

Refactored code is around the use of the option::Option array. Instead of a std::unique_ptr to an
allocated array of option::Option object, use a std::vector and pass a reference to the vector to
checkSanity(). This should avoid warnings about pointer arithmetic and is also more appropriate code
for the use case.

---------

Co-authored-by: carneyweb <mike@carneyweb.com>
Co-authored-by: Michael Carney <carney@macmini.mynetworksettings.com>

Author: 3 people

framework/test/gtest/ResourceCompilerTest.cpp

@@ -72,12 +72,6 @@ namespace
       protected:
         std::string rcbinpath;
         std::string tempdir;
-
-        const std::string manifest_json = R"({
-    "bundle.symbolic_name" : "main",
-    "bundle.version" : "0.1.0",
-    "bundle.activator" : true
-    })";
     };
 
     // A zip file containing only a manifest.
@@ -370,15 +364,26 @@ TEST_F(ResourceCompilerTest, testEscapePath)
 
 TEST_F(ResourceCompilerTest, testManifestAdd)
 {
+    const std::string manifest_json = R"({
+      "bundle.symbolic_name" : "mybundle",
+      "bundle.version" : "0.1.0",
+      "bundle.activator" : true
+    })";
     createDirHierarchy(tempdir, manifest_json);
 
-    std::ostringstream cmd;
-    cmd << rcbinpath;
-    cmd << " --bundle-name "
-        << "mybundle";
-    cmd << " --out-file \"" << tempdir << "Example.zip\"";
-    cmd << " --manifest-add \"" << tempdir << "manifest.json\"";
+    std::ostringstream badcmd;
+    badcmd << rcbinpath
+        << " --bundle-name mismatched_bundle_name"
+        << " --out-file \"" << tempdir << "Example.zip\""
+        << " --manifest-add \"" << tempdir << "manifest.json\"";
+    // Test that invoking command with --bundle-name different from that in the manifest fails.
+    ASSERT_NE(EXIT_SUCCESS, runExecutable(badcmd.str()));
 
+    std::ostringstream cmd;
+    cmd << rcbinpath
+        << " --bundle-name mybundle"
+        << " --out-file \"" << tempdir << "Example.zip\""
+        << " --manifest-add \"" << tempdir << "manifest.json\"";
     // Test that Cmdline invocation in testManifestAdd returns 0
     ASSERT_EQ(EXIT_SUCCESS, runExecutable(cmd.str()));
 
@@ -549,10 +554,16 @@ TEST_F(ResourceCompilerTest, testZipAddTwice)
  */
 TEST_F(ResourceCompilerTest, testBundleManifestZipAdd)
 {
+    const std::string manifest_json = R"({
+      "bundle.symbolic_name" : "anotherbundle",
+      "bundle.version" : "0.1.0",
+      "bundle.activator" : true
+    })";
+    createManifestFile(tempdir, manifest_json, "manifest2.json");
+
     std::ostringstream cmd;
     cmd << rcbinpath;
-    cmd << " --bundle-name anotherbundle ";
-    cmd << " --manifest-add \"" << tempdir << "manifest.json\" ";
+    cmd << " --manifest-add \"" << tempdir << "manifest2.json\" ";
     cmd << " --bundle-file \"" << tempdir << "sample1.dll\" ";
     cmd << " --zip-add \"" << tempdir << "tomerge.zip\" ";
     cmd << " --zip-add \"" << tempdir << "Example2.zip\"";
@@ -645,7 +656,7 @@ TEST_F(ResourceCompilerTest, testDuplicateManifestFileAdd)
 {
     std::ostringstream cmd;
     cmd << rcbinpath;
-    cmd << " --bundle-name multiple_dups ";
+    cmd << " --bundle-name mybundle ";
     cmd << " --manifest-add \"" << tempdir << "manifest.json\" ";
     cmd << " --manifest-add \"" << tempdir << "manifest.json\" ";
     cmd << " --out-file \"" << tempdir << "testDuplicateManifestFileAdd.zip\" ";
@@ -857,6 +868,7 @@ TEST_F(ResourceCompilerTest, testManifestAddWithInvalidJSON)
 
 TEST_F(ResourceCompilerTest, testUnicodeBundleFile)
 {
+
     std::ostringstream cmd;
     cmd << rcbinpath;
     cmd << " --bundle-file \"" << tempdir << u8"ｆｏｏｂａｒ/myArchive00.zip\"";
@@ -915,13 +927,20 @@ TEST_F(ResourceCompilerTest, testUnicodeZipAdd)
 
 TEST_F(ResourceCompilerTest, testUnicodeManifestAdd)
 {
+    const std::string manifest_json = u8R"({
+      "bundle.symbolic_name" : "myunicodebundle02-V",
+      "bundle.version" : "0.1.0",
+      "bundle.activator" : true
+    })";
+    createManifestFile(tempdir, manifest_json, "manifest3.json");
+
     auto origdir = util::GetCurrentWorkingDirectory();
     ChangeDirectory(tempdir);
     std::ostringstream cmd;
     cmd << rcbinpath;
     cmd << " --bundle-file " << u8"ｆｏｏｂａｒ/myArchive05.zip";
     cmd << " --out-file unicodeResAdd02.zip";
-    cmd << " --manifest-add " << u8"ｆｏｏｂａｒ/manifest.json";
+    cmd << " --manifest-add " << u8"manifest3.json";
     cmd << " --bundle-name myunicodebundle02";
     cmd << "-V";
 
@@ -941,21 +960,21 @@ TEST_F(ResourceCompilerTest, testManifestAddWithJSONComments)
     std::string jsonCommentSyntax(R"(
 		{ /* no, no, no. */
 		 "bundle.name": "foo",
+		 "bundle.symbolic_name": "mybundle",
 		 "bundle.version" : "1.0.2",
 		 "bundle.description" : "This bundle shouldn't have comments!",
 		 "authors" : ["John Doe", "Douglas Reynolds", "Daniel Cannady"],
 		 "rating" : 5 // no comments allowed
 		} 
     )");
 
-    createManifestFile(tempdir, jsonCommentSyntax);
+    createManifestFile(tempdir, jsonCommentSyntax, "manifest4.json");
 
     std::ostringstream cmd;
     cmd << rcbinpath;
-    cmd << " --bundle-name "
-        << "mybundle";
+    cmd << " --bundle-name mybundle";
     cmd << " --out-file \"" << tempdir << "json_comment_syntax.zip\"";
-    cmd << " --manifest-add \"" << tempdir << "manifest.json\"";
+    cmd << " --manifest-add \"" << tempdir << "manifest4.json\"";
     // Test embedding a manifest containing JSON comments.
     ASSERT_EQ(EXIT_SUCCESS, runExecutable(cmd.str()));
 }
@@ -1320,7 +1339,8 @@ TEST_F(ResourceCompilerTest, testMultipleManifestConcatenation)
 TEST_F(ResourceCompilerTest, testManifestWithNullTerminator)
 {
     const std::string manifest_json = R"({
-    "test" : { 
+    "bundle.symbolic_name" : "main",
+    "test" : {
         "bar" : "baz\\0bar",
         "foo\\0bar" : [1, 2, 5, 7]
     }})";

tools/rc/ResourceCompiler.cpp

@@ -29,7 +29,6 @@
 #include <fstream>
 #include <iostream>
 #include <list>
-#include <memory>
 #include <set>
 #include <stdexcept>
 #include <string>
@@ -416,6 +415,28 @@ ZipArchive::CheckAndAddToArchivedNames(std::string const& archiveEntry)
 void
 ZipArchive::AddManifestFile(Json::Value const& manifest)
 {
+    // Check to make sure that the bundleName passed on the command line and the
+    // bundle.symbolic_name in the manifest file match. If the bundleName is not passed in on the
+    // command line, use the name specified in the manifest. If there's a mismatch or if no
+    // bundleName is supplied in either location, it's an error.
+    auto bname = manifest.get("bundle.symbolic_name", "");
+    if (bname.isString()) {
+        auto bnameStr = bname.asString();
+        if (!bundleName.empty()) {
+            if (bnameStr != bundleName) {
+                throw std::runtime_error("Bundle name in manifest "
+                                         + bnameStr
+                                         + " does not match value supplied on command line "
+                                         + bundleName);
+            }
+        }
+        else {
+            bundleName = bnameStr;
+        }
+    }
+    if (bundleName.empty()) {
+      throw std::runtime_error("Bundle name is required. Make sure that \"bundle.symbolic_name\" is set in the manifest.json file.");
+    }
     std::string styledManifestJson(manifest.toStyledString());
     std::string archiveEntry(bundleName + "/manifest.json");
 
@@ -745,7 +766,7 @@ const option::Descriptor usage[] = {
 
 // Check invalid invocations and errors
 static int
-checkSanity(option::Parser& parse, option::Option* options)
+checkSanity(option::Parser& parse, std::vector<option::Option>& options)
 {
     int return_code = EXIT_SUCCESS;
 
@@ -792,17 +813,16 @@ checkSanity(option::Parser& parse, option::Option* options)
         return_code = EXIT_FAILURE;
     }
 
-    // If either --manifest-add or --res-add is given, --bundle-name must also be given.
-    if ((options[MANIFESTADD] || options[RESADD]) && !options[BUNDLENAME])
+    // If --res-add is given, --bundle-name must also be given.
+    if (options[RESADD] && !options[BUNDLENAME])
     {
-        std::cerr << "If either --manifest-add or --res-add is provided, "
-                     "--bundle-name must be provided."
+        std::cerr << "If --res-add is provided, --bundle-name must be provided."
                   << std::endl;
         return_code = EXIT_FAILURE;
     }
 
     // Generate a warning that --bundle-name is not necessary in following invocation.
-    if (options[BUNDLENAME] && !options[MANIFESTADD] && !options[RESADD] && return_code != EXIT_FAILURE)
+    if (options[BUNDLENAME] && !options[RESADD] && return_code != EXIT_FAILURE)
     {
         std::clog << "Warning: --bundle-name option is unnecessary here." << std::endl;
     }
@@ -835,17 +855,17 @@ main(int argc, char** argv)
     argc -= (argc > 0);
     argv += (argc > 0); // skip program name argv[0]
     option::Stats stats(usage, argc, argv);
-    std::unique_ptr<option::Option[]> options(new option::Option[stats.options_max]);
-    std::unique_ptr<option::Option[]> buffer(new option::Option[stats.buffer_max]);
-    option::Parser parse(true, usage, argc, argv, options.get(), buffer.get());
+    std::vector<option::Option> options { stats.options_max };
+    std::vector<option::Option> buffer { stats.buffer_max };
+    option::Parser parse(true, usage, argc, argv, options.data(), buffer.data());
 
     if (argc == 0 || options[HELP])
     {
         option::printUsage(std::clog, usage);
         return return_code;
     }
 
-    return_code = checkSanity(parse, options.get());
+    return_code = checkSanity(parse, options);
     if (return_code == EXIT_FAILURE)
     {
         return return_code;