diff --git a/daemon/extension/ScriptConfig.cpp b/daemon/extension/ScriptConfig.cpp index 8aa1883df..a76287247 100644 --- a/daemon/extension/ScriptConfig.cpp +++ b/daemon/extension/ScriptConfig.cpp @@ -74,6 +74,14 @@ bool ScriptConfig::LoadConfig(Options::OptEntries* optEntries) bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries) { + struct CaseInsensitiveLess + { + bool operator()(const CString& left, const CString& right) const + { + return strcasecmp(*left, *right) < 0; + } + }; + // save to config file DiskFile infile; @@ -83,7 +91,23 @@ bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries) } std::vector config; - std::set writtenOptions; + std::set writtenOptions; + + // Options::SetOption overwrites the existing entry while parsing, so when a + // name occurs on several lines the last one is the value nzbget runs on. + // Collapsing duplicates must preserve that value, otherwise saving a damaged + // config silently changes settings. + auto findLastValue = [optEntries](const char* name) -> const char* + { + for (auto it = optEntries->rbegin(); it != optEntries->rend(); ++it) + { + if (!strcasecmp(it->GetName(), name)) + { + return it->GetValue(); + } + } + return nullptr; + }; // read config file into memory array int fileLen = (int)FileSystem::FileSize(g_Options->GetConfigFilename()) + 1; @@ -110,10 +134,14 @@ bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries) if (g_Options->SplitOptionString(buf, optname, optvalue)) { Options::OptEntry* optEntry = optEntries->FindOption(optname); - if (optEntry) + // write each option only once, dropping duplicate lines accumulated + // in the config file by earlier versions (issue #588); keep the + // first occurrence's name and position but the last occurrence's + // value, so the collapsed line is the one nzbget was running on + if (optEntry && writtenOptions.find(optEntry->GetName()) == writtenOptions.end()) { - infile.Print("%s=%s\n", optEntry->GetName(), optEntry->GetValue()); - writtenOptions.insert(optEntry); + infile.Print("%s=%s\n", optEntry->GetName(), findLastValue(optEntry->GetName())); + writtenOptions.insert(optEntry->GetName()); } } } @@ -126,10 +154,11 @@ bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries) // write new options for (Options::OptEntry& optEntry : *optEntries) { - std::set::iterator fit = writtenOptions.find(&optEntry); + std::set::iterator fit = writtenOptions.find(optEntry.GetName()); if (fit == writtenOptions.end()) { - infile.Print("%s=%s\n", optEntry.GetName(), optEntry.GetValue()); + infile.Print("%s=%s\n", optEntry.GetName(), findLastValue(optEntry.GetName())); + writtenOptions.insert(optEntry.GetName()); } } diff --git a/tests/extension/ScriptConfig.cpp b/tests/extension/ScriptConfig.cpp new file mode 100644 index 000000000..ce44330d3 --- /dev/null +++ b/tests/extension/ScriptConfig.cpp @@ -0,0 +1,163 @@ +/* + * This file is part of nzbget. See . + * + * Copyright (C) 2026 Denis + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include "nzbget.h" + +#include +#include +#include +#include "Options.h" +#include "ScriptConfig.h" + +BOOST_AUTO_TEST_SUITE(ExtensionTest) + +struct TempConfigFile +{ + fs::path path = fs::temp_directory_path() / fs::make_unique_filename(); + + ~TempConfigFile() + { + fs::error_code error; + fs::remove(path, error); + } +}; + +struct OptionsGuard +{ + Options* oldOptions = g_Options; + + ~OptionsGuard() + { + g_Options = oldOptions; + } +}; + +BOOST_AUTO_TEST_CASE(SaveConfigDoesNotAppendDuplicateOptionNames) +{ + TempConfigFile configFile; + { + std::ofstream output(configFile.path); + output << "# existing config\n"; + } + + OptionsGuard optionsGuard; + Options options("nzbget", configFile.path.string().c_str(), true, nullptr, nullptr); + g_Options = &options; + + Options::OptEntries optEntries; + optEntries.emplace_back("Extension.Option", "first"); + optEntries.emplace_back("extension.option", "second"); + ScriptConfig scriptConfig; + + BOOST_REQUIRE(scriptConfig.SaveConfig(&optEntries)); + std::string firstContents; + { + std::ifstream firstFile(configFile.path); + std::stringstream contents; + contents << firstFile.rdbuf(); + firstContents = contents.str(); + } + // one line only, carrying the last entry's value - Options::SetOption + // resolves duplicates the same way, so the file matches what nzbget loads + BOOST_CHECK_EQUAL(firstContents, "# existing config\nExtension.Option=second\n"); + + BOOST_REQUIRE(scriptConfig.SaveConfig(&optEntries)); + std::string secondContents; + { + std::ifstream secondFile(configFile.path); + std::stringstream contents; + contents << secondFile.rdbuf(); + secondContents = contents.str(); + } + BOOST_CHECK_EQUAL(secondContents, firstContents); +} + +BOOST_AUTO_TEST_CASE(SaveConfigRemovesDuplicateLinesFromDamagedConfig) +{ + TempConfigFile configFile; + { + std::ofstream output(configFile.path); + output << "# existing config\n" + << "Server1.Active=yes\n" + << "Server2.Active=yes\n" + << "Server2.Active=yes\n" + << "server2.active=yes\n" + << "Server2.Host=news.example.com\n"; + } + + OptionsGuard optionsGuard; + Options options("nzbget", configFile.path.string().c_str(), true, nullptr, nullptr); + g_Options = &options; + + Options::OptEntries optEntries; + optEntries.emplace_back("Server1.Active", "yes"); + optEntries.emplace_back("Server2.Active", "no"); + optEntries.emplace_back("Server2.Host", "news.example.com"); + ScriptConfig scriptConfig; + + BOOST_REQUIRE(scriptConfig.SaveConfig(&optEntries)); + std::string contents; + { + std::ifstream file(configFile.path); + std::stringstream buffer; + buffer << file.rdbuf(); + contents = buffer.str(); + } + BOOST_CHECK_EQUAL(contents, + "# existing config\n" + "Server1.Active=yes\n" + "Server2.Active=no\n" + "Server2.Host=news.example.com\n"); +} + +// When a config file carries duplicate lines for one option, Options::SetOption +// overwrites the same entry while parsing, so the LAST line is the value nzbget +// actually runs on. Collapsing the duplicates must preserve that value, +// otherwise saving a damaged config silently changes settings (issue #588). +BOOST_AUTO_TEST_CASE(SaveConfigKeepsLastValueWhenConfigHasDuplicateLines) +{ + TempConfigFile configFile; + { + std::ofstream output(configFile.path); + output << "# existing config\n" + << "Server2.Active=yes\n" + << "Server2.Name=xsusenet\n" + << "server2.active=no\n"; + } + + OptionsGuard optionsGuard; + Options options("nzbget", configFile.path.string().c_str(), true, nullptr, nullptr); + g_Options = &options; + + // as loadconfig reports it: raw file lines, duplicates included + Options::OptEntries optEntries; + optEntries.emplace_back("Server2.Active", "yes"); + optEntries.emplace_back("Server2.Name", "xsusenet"); + optEntries.emplace_back("server2.active", "no"); + ScriptConfig scriptConfig; + + BOOST_REQUIRE(scriptConfig.SaveConfig(&optEntries)); + std::string contents; + { + std::ifstream file(configFile.path); + std::stringstream buffer; + buffer << file.rdbuf(); + contents = buffer.str(); + } + // one line per option, keeping the first occurrence's name and position + // but the last occurrence's value - exactly what Options::SetOption does + BOOST_CHECK_EQUAL(contents, + "# existing config\n" + "Server2.Active=no\n" + "Server2.Name=xsusenet\n"); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/tests/extension/extension.cmake b/tests/extension/extension.cmake index 7bd242ce6..ca1391232 100644 --- a/tests/extension/extension.cmake +++ b/tests/extension/extension.cmake @@ -4,6 +4,7 @@ list(APPEND TESTS_SRC ${CMAKE_CURRENT_SOURCE_DIR}/extension/Extension.cpp ${CMAKE_CURRENT_SOURCE_DIR}/extension/ExtensionManager.cpp ${CMAKE_CURRENT_SOURCE_DIR}/extension/ScanScript.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/extension/ScriptConfig.cpp ) file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/testdata/extension/manifest DESTINATION ${CMAKE_CURRENT_BINARY_DIR})