Condition grammar improvements.

Check for safe file paths when parsing, simplify grammar constructor to
avoid invalid arguments, simplify some code.

src/backend/metadata/condition_grammar.h

@@ -64,10 +64,8 @@ namespace loot {
     template<typename Iterator, typename Skipper>
     class ConditionGrammar : public qi::grammar < Iterator, bool(), Skipper > {
     public:
-        ConditionGrammar(Game * game, bool parseOnly) : ConditionGrammar::base_type(expression, "condition grammar"), _game(game), _parseOnly(parseOnly) {
-            if (!_parseOnly && _game == nullptr)
-                throw error(error::invalid_args, "A valid game pointer was not passed during a condition evaluation.");
-
+        ConditionGrammar() : ConditionGrammar(nullptr) {}
+        ConditionGrammar(Game * game) : ConditionGrammar::base_type(expression, "condition grammar"), _game(game) {
             expression =
                 qi::eps >
                 compound[qi::labels::_val = qi::labels::_1]
@@ -142,13 +140,9 @@ namespace loot {
         qi::rule<Iterator, char()> invalidPathChars;
 
         Game * _game;
-        bool _parseOnly;
 
         //Eval's exact paths. Check for files and ghosted plugins.
         void CheckFile(bool& result, const std::string& file) const {
-            if (_parseOnly)
-                return;
-
             BOOST_LOG_TRIVIAL(trace) << "Checking to see if the file \"" << file << "\" exists.";
 
             if (file == "LOOT") {
@@ -161,6 +155,9 @@ namespace loot {
                 throw loot::error(loot::error::invalid_args, boost::locale::translate("Invalid file path:").str() + " " + file);
             }
 
+            if (_game == nullptr)
+                return;
+
             if (boost::iends_with(file, ".esp") || boost::iends_with(file, ".esm"))
                 result = boost::filesystem::exists(_game->DataPath() / file) || boost::filesystem::exists(_game->DataPath() / (file + ".ghost"));
             else
@@ -191,27 +188,20 @@ namespace loot {
                 throw loot::error(loot::error::invalid_args, (boost::format(boost::locale::translate("Invalid regex string \"%1%\": %2%")) % regex % e.what()).str());
             }
 
-            std::regex sepReg("/|(\\\\\\\\)", std::regex::ECMAScript | std::regex::icase);
+            std::regex sepReg("/|(\\\\\\\\)", std::regex::ECMAScript);
 
-            std::vector<std::string> components;
             std::sregex_token_iterator it(regex.begin(), regex.end(), sepReg, -1);
-            std::sregex_token_iterator itend;
-            for (; it != itend; ++it) {
-                components.push_back(*it);
-            }
+            std::vector<std::string> components(it, std::sregex_token_iterator());
 
             std::string filename = components.back();
             components.pop_back();
 
             boost::filesystem::path parent;
-            for (std::vector<std::string>::const_iterator it = components.begin(), endIt = components.end()--; it != endIt; ++it) {
-                if (*it == ".")
-                    continue;
-
-                parent += *it;
+            for (const auto& component : components) {
+                parent /= component;
             }
 
-            if (!IsSafePath(parent.string())) {
+            if (!IsSafePath(parent)) {
                 BOOST_LOG_TRIVIAL(error) << "Invalid folder path: " << parent;
                 throw loot::error(loot::error::invalid_args, boost::locale::translate("Invalid folder path:").str() + " " + parent.string());
             }
@@ -235,7 +225,7 @@ namespace loot {
 
             std::pair<boost::filesystem::path, std::regex> pathRegex = SplitRegex(regexStr);
 
-            if (_parseOnly)
+            if (_game == nullptr)
                 return;
 
             //Now we have a valid parent path and a regex filename. Check that
@@ -263,7 +253,7 @@ namespace loot {
 
             std::pair<boost::filesystem::path, std::regex> pathRegex = SplitRegex(regexStr);
 
-            if (_parseOnly)
+            if (_game == nullptr)
                 return;
 
             //Now we have a valid parent path and a regex filename. Check that
@@ -287,7 +277,6 @@ namespace loot {
         }
 
         void CheckSum(bool& result, const std::string& file, const uint32_t checksum) {
-
             BOOST_LOG_TRIVIAL(trace) << "Checking the CRC of the file \"" << file << "\".";
 
             if (!IsSafePath(file)) {
@@ -295,7 +284,7 @@ namespace loot {
                 throw loot::error(loot::error::invalid_args, boost::locale::translate("Invalid file path:").str() + " " + file);
             }
 
-            if (_parseOnly)
+            if (_game == nullptr)
                 return;
 
             uint32_t crc;
@@ -322,12 +311,13 @@ namespace loot {
         }
 
         void CheckVersion(bool& result, const std::string&  file, const std::string& version, const std::string& comparator) const {
-            if (_parseOnly)
-                return;
-
             BOOST_LOG_TRIVIAL(trace) << "Checking version of file \"" << file << "\".";
 
             CheckFile(result, file);
+
+            if (_game == nullptr)
+                return;
+
             if (!result) {
                 if (comparator == "!=" || comparator == "<" || comparator == "<=")
                     result = true;
@@ -360,14 +350,14 @@ namespace loot {
         }
 
         void CheckActive(bool& result, const std::string& file) const {
-            if (_parseOnly)
-                return;
-
             if (!IsSafePath(file)) {
                 BOOST_LOG_TRIVIAL(error) << "Invalid file path: " << file;
                 throw loot::error(loot::error::invalid_args, boost::locale::translate("Invalid file path:").str() + " " + file);
             }
 
+            if (_game == nullptr)
+                return;
+
             if (file == "LOOT")
                 result = false;
             else
@@ -386,19 +376,21 @@ namespace loot {
         }
 
         //Checks that the path (not regex) doesn't go outside any game folders.
-        bool IsSafePath(const std::string& path) const {
+        bool IsSafePath(const boost::filesystem::path& path) const {
             BOOST_LOG_TRIVIAL(trace) << "Checking to see if the path \"" << path << "\" is safe.";
 
-            std::vector<std::string> components;
-            boost::split(components, path, boost::is_any_of("/\\"));
-            components.pop_back();
-            std::string parent_path;
-            for (auto it = components.cbegin(), endIt = components.cend()--; it != endIt; ++it) {
-                if (*it == ".")
+            boost::filesystem::path temp;
+            for (const auto& component : path) {
+                if (component == ".")
                     continue;
-                parent_path += *it + '/';
+
+                if (component == ".." && temp.filename() == "..")
+                    return false;
+
+                temp /= component;
             }
-            return !boost::contains(parent_path, "../../");
+
+            return true;
         }
     };
 }

src/backend/metadata/conditional_metadata.cpp

@@ -55,7 +55,7 @@ namespace loot {
         if (it != game.conditionCache.end())
             return it->second;
 
-        ConditionGrammar<std::string::const_iterator, boost::spirit::qi::space_type> grammar(&game, false);
+        ConditionGrammar<std::string::const_iterator, boost::spirit::qi::space_type> grammar(&game);
         boost::spirit::qi::space_type skipper;
         std::string::const_iterator begin, end;
         bool eval;
@@ -88,7 +88,7 @@ namespace loot {
 
         BOOST_LOG_TRIVIAL(trace) << "Testing condition syntax: " << _condition;
 
-        ConditionGrammar<std::string::const_iterator, boost::spirit::qi::space_type> grammar(nullptr, true);
+        ConditionGrammar<std::string::const_iterator, boost::spirit::qi::space_type> grammar(nullptr);
         boost::spirit::qi::space_type skipper;
         std::string::const_iterator begin, end;
 

src/tests/backend/metadata/test_condition_grammar.h

@@ -34,14 +34,12 @@ class ConditionGrammar : public SkyrimTest {};
 typedef loot::ConditionGrammar<std::string::const_iterator, boost::spirit::qi::space_type> Grammar;
 
 TEST_F(ConditionGrammar, Constructor) {
-    EXPECT_NO_THROW(Grammar cg(nullptr, true));
-    EXPECT_THROW(Grammar cg(nullptr, false), loot::error);
+    EXPECT_NO_THROW(Grammar cg(nullptr));
 
     loot::Game game(loot::Game::tes5);
     game.SetGamePath(dataPath.parent_path());
 
-    EXPECT_NO_THROW(Grammar cg(&game, true));
-    EXPECT_NO_THROW(Grammar cg(&game, false));
+    EXPECT_NO_THROW(Grammar cg(&game));
 }
 
 TEST_F(ConditionGrammar, InvalidSyntax) {
@@ -51,7 +49,7 @@ TEST_F(ConditionGrammar, InvalidSyntax) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("file(foo)");
     std::string::const_iterator begin = condition.begin();
@@ -67,7 +65,7 @@ TEST_F(ConditionGrammar, EmptyCondition) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("");
     std::string::const_iterator begin = condition.begin();
@@ -83,13 +81,13 @@ TEST_F(ConditionGrammar, FileConditionTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("file(\"Blank.esm\")");
     std::string::const_iterator begin = condition.begin();
     std::string::const_iterator end = condition.end();
 
-    EXPECT_NO_THROW(r = boost::spirit::qi::phrase_parse(begin, end, cg, skipper, eval));
+    r = boost::spirit::qi::phrase_parse(begin, end, cg, skipper, eval);
     EXPECT_TRUE(r);
     EXPECT_TRUE(eval);
 }
@@ -101,7 +99,7 @@ TEST_F(ConditionGrammar, FileConditionFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("file(\"Blank.missing.esm\")");
     std::string::const_iterator begin = condition.begin();
@@ -119,7 +117,7 @@ TEST_F(ConditionGrammar, UnsafePath) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("file(\"../../Blank.esm\")");
     std::string::const_iterator begin = condition.begin();
@@ -135,7 +133,7 @@ TEST_F(ConditionGrammar, RegexConditionTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("regex(\"Blank.+\\.esm\")");
     std::string::const_iterator begin = condition.begin();
@@ -153,7 +151,7 @@ TEST_F(ConditionGrammar, RegexConditionFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("regex(\"Blank\\.m.+\\.esm\")");
     std::string::const_iterator begin = condition.begin();
@@ -164,6 +162,24 @@ TEST_F(ConditionGrammar, RegexConditionFalse) {
     EXPECT_FALSE(eval);
 }
 
+TEST_F(ConditionGrammar, RegexCondition_Subfolder) {
+    loot::Game game(loot::Game::tes5);
+    game.SetGamePath(dataPath.parent_path());
+
+    boost::spirit::qi::space_type skipper;
+    bool eval = true;
+    bool r = false;
+    Grammar cg(&game);
+
+    std::string condition("regex(\"resource\\\\detail\\\\resource\\.txt\")");
+    std::string::const_iterator begin = condition.begin();
+    std::string::const_iterator end = condition.end();
+
+    EXPECT_NO_THROW(r = boost::spirit::qi::phrase_parse(begin, end, cg, skipper, eval));
+    EXPECT_TRUE(r);
+    EXPECT_TRUE(eval);
+}
+
 TEST_F(ConditionGrammar, ManyConditionTrue) {
     loot::Game game(loot::Game::tes5);
     game.SetGamePath(dataPath.parent_path());
@@ -171,7 +187,7 @@ TEST_F(ConditionGrammar, ManyConditionTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("many(\"Blank.+\\.esm\")");
     std::string::const_iterator begin = condition.begin();
@@ -189,7 +205,7 @@ TEST_F(ConditionGrammar, ManyConditionFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("many(\"Blank\\.esm\")");
     std::string::const_iterator begin = condition.begin();
@@ -208,7 +224,7 @@ TEST_F(ConditionGrammar, ChecksumConditionTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("checksum(\"Blank.esp\", E12EFAAA)");
     std::string::const_iterator begin = condition.begin();
@@ -227,7 +243,7 @@ TEST_F(ConditionGrammar, ChecksumConditionFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("checksum(\"Blank.esp\", DEADBEEF)");
     std::string::const_iterator begin = condition.begin();
@@ -247,7 +263,7 @@ TEST_F(ConditionGrammar, VersionConditionEqualTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"5.0\", ==)");
     std::string::const_iterator begin = condition.begin();
@@ -267,7 +283,7 @@ TEST_F(ConditionGrammar, VersionConditionEqualFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"6.0\", ==)");
     std::string::const_iterator begin = condition.begin();
@@ -287,7 +303,7 @@ TEST_F(ConditionGrammar, VersionConditionNotEqualTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"6.0\", !=)");
     std::string::const_iterator begin = condition.begin();
@@ -307,7 +323,7 @@ TEST_F(ConditionGrammar, VersionConditionNotEqualFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"5.0\", !=)");
     std::string::const_iterator begin = condition.begin();
@@ -327,7 +343,7 @@ TEST_F(ConditionGrammar, VersionConditionLessThanTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"6.0\", <)");
     std::string::const_iterator begin = condition.begin();
@@ -347,7 +363,7 @@ TEST_F(ConditionGrammar, VersionConditionLessThanFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"5.0\", <)");
     std::string::const_iterator begin = condition.begin();
@@ -367,7 +383,7 @@ TEST_F(ConditionGrammar, VersionConditionGreaterThanTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"4.0\", >)");
     std::string::const_iterator begin = condition.begin();
@@ -387,7 +403,7 @@ TEST_F(ConditionGrammar, VersionConditionGreaterThanFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"5.0\", >)");
     std::string::const_iterator begin = condition.begin();
@@ -407,7 +423,7 @@ TEST_F(ConditionGrammar, VersionConditionLETrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"5.0\", <=)");
     std::string::const_iterator begin = condition.begin();
@@ -427,7 +443,7 @@ TEST_F(ConditionGrammar, VersionConditionLEFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"4.0\", <=)");
     std::string::const_iterator begin = condition.begin();
@@ -447,7 +463,7 @@ TEST_F(ConditionGrammar, VersionConditionGETrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"5.0\", >=)");
     std::string::const_iterator begin = condition.begin();
@@ -466,7 +482,7 @@ TEST_F(ConditionGrammar, VersionConditionGEFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("version(\"Blank.esm\", \"6.0\", >=)");
     std::string::const_iterator begin = condition.begin();
@@ -485,7 +501,7 @@ TEST_F(ConditionGrammar, ActiveConditionTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("active(\"Blank.esm\")");
     std::string::const_iterator begin = condition.begin();
@@ -504,7 +520,7 @@ TEST_F(ConditionGrammar, ActiveConditionFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("active(\"Blank.esp\")");
     std::string::const_iterator begin = condition.begin();
@@ -522,7 +538,7 @@ TEST_F(ConditionGrammar, NegatorTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = false;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("not file(\"Blank.missing.esm\")");
     std::string::const_iterator begin = condition.begin();
@@ -540,7 +556,7 @@ TEST_F(ConditionGrammar, NegatorFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("not file(\"Blank.esm\")");
     std::string::const_iterator begin = condition.begin();
@@ -558,7 +574,7 @@ TEST_F(ConditionGrammar, CompoundAndTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("file(\"Blank.esm\") and file(\"Blank.esp\")");
     std::string::const_iterator begin = condition.begin();
@@ -576,7 +592,7 @@ TEST_F(ConditionGrammar, CompoundAndFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("file(\"Blank.esm\") and file(\"Blank.missing.esp\")");
     std::string::const_iterator begin = condition.begin();
@@ -594,7 +610,7 @@ TEST_F(ConditionGrammar, CompoundOrTrue) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("file(\"Blank.missing.esm\") or file(\"Blank.esp\")");
     std::string::const_iterator begin = condition.begin();
@@ -612,7 +628,7 @@ TEST_F(ConditionGrammar, CompoundOrFalse) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("file(\"Blank.missing.esm\") or file(\"Blank.missing.esp\")");
     std::string::const_iterator begin = condition.begin();
@@ -630,7 +646,7 @@ TEST_F(ConditionGrammar, OrderOfEvaluation) {
     boost::spirit::qi::space_type skipper;
     bool eval = true;
     bool r = false;
-    Grammar cg(&game, false);
+    Grammar cg(&game);
 
     std::string condition("file(\"Blank.esm\") and ( not file(\"Blank.esm\") or file(\"Blank.esp\") ) or file(\"Blank.missing.esp\")");
     std::string::const_iterator begin = condition.begin();

src/tests/fixtures.h

@@ -35,7 +35,13 @@ along with LOOT.  If not, see
 class GameTest : public ::testing::Test {
 protected:
     GameTest(const boost::filesystem::path& gameDataPath, const boost::filesystem::path& gameLocalPath)
-        : dataPath(gameDataPath), localPath(gameLocalPath), missingPath("./missing"), masterlistPath(localPath / "masterlist.yaml"), userlistPath(localPath / "userlist.yaml"), db(nullptr) {}
+        : dataPath(gameDataPath),
+        localPath(gameLocalPath),
+        missingPath("./missing"),
+        masterlistPath(localPath / "masterlist.yaml"),
+        userlistPath(localPath / "userlist.yaml"),
+        resourcePath(dataPath / "resource" / "detail" / "resource.txt"),
+        db(nullptr) {}
 
     inline virtual void SetUp() {
         ASSERT_NO_THROW(boost::filesystem::create_directories(localPath));
@@ -70,6 +76,12 @@ protected:
         out.close();
         ASSERT_TRUE(boost::filesystem::exists(dataPath / "EmptyFile.esm"));
 
+        // Write out an empty resource file.
+        ASSERT_NO_THROW(boost::filesystem::create_directories(resourcePath.parent_path()));
+        out.open(resourcePath);
+        out.close();
+        ASSERT_TRUE(boost::filesystem::exists(resourcePath));
+
         // Write out an non-empty, non-plugin file.
         out.open(dataPath / "NotAPlugin.esm");
         out << "This isn't a valid plugin file.";
@@ -85,8 +97,10 @@ protected:
 
         // Delete generated files.
         ASSERT_NO_THROW(boost::filesystem::remove(dataPath / "EmptyFile.esm"));
+        ASSERT_NO_THROW(boost::filesystem::remove(resourcePath));
         ASSERT_NO_THROW(boost::filesystem::remove(dataPath / "NotAPlugin.esm"));
         ASSERT_FALSE(boost::filesystem::exists(dataPath / "EmptyFile.esm"));
+        ASSERT_FALSE(boost::filesystem::exists(resourcePath));
         ASSERT_FALSE(boost::filesystem::exists(dataPath / "NotAPlugin.esm"));
 
         // Masterlist & userlist may have been created during test, so delete them.
@@ -106,6 +120,8 @@ protected:
     const boost::filesystem::path masterlistPath;
     const boost::filesystem::path userlistPath;
 
+    const boost::filesystem::path resourcePath;
+
     loot_db db;
 };