From 4e2fde5b8965e71b9c6692281a8b090dff9acb36 Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Sun, 19 Jul 2026 10:11:25 -0500 Subject: [PATCH 1/3] Fix duplicate extension config entries --- daemon/extension/ScriptConfig.cpp | 15 ++++-- tests/extension/ScriptConfig.cpp | 81 +++++++++++++++++++++++++++++++ tests/extension/extension.cmake | 1 + 3 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 tests/extension/ScriptConfig.cpp diff --git a/daemon/extension/ScriptConfig.cpp b/daemon/extension/ScriptConfig.cpp index 8aa1883df..c7e8b8d0e 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,7 @@ bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries) } std::vector config; - std::set writtenOptions; + std::set writtenOptions; // read config file into memory array int fileLen = (int)FileSystem::FileSize(g_Options->GetConfigFilename()) + 1; @@ -113,7 +121,7 @@ bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries) if (optEntry) { infile.Print("%s=%s\n", optEntry->GetName(), optEntry->GetValue()); - writtenOptions.insert(optEntry); + writtenOptions.insert(optEntry->GetName()); } } } @@ -126,10 +134,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()); + writtenOptions.insert(optEntry.GetName()); } } diff --git a/tests/extension/ScriptConfig.cpp b/tests/extension/ScriptConfig.cpp new file mode 100644 index 000000000..c8af84b32 --- /dev/null +++ b/tests/extension/ScriptConfig.cpp @@ -0,0 +1,81 @@ +/* + * 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(); + } + BOOST_CHECK_EQUAL(firstContents, "# existing config\nExtension.Option=first\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_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}) From 2da9d5a8bb43bec2b3949568af9a7b25cfd95f52 Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Thu, 30 Jul 2026 21:33:39 -0500 Subject: [PATCH 2/3] Remove existing duplicate config lines when saving config SaveConfig previously rewrote every config line whose option name matched a save request entry, so duplicate lines accumulated by earlier versions (issue #588) were preserved forever even though new appends were prevented. Write each option only once in the replacement path as well, so a single save converges a damaged config back to one line per option. Add a regression test covering exact and case-variant duplicate lines. --- daemon/extension/ScriptConfig.cpp | 4 +++- tests/extension/ScriptConfig.cpp | 38 +++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/daemon/extension/ScriptConfig.cpp b/daemon/extension/ScriptConfig.cpp index c7e8b8d0e..11b8e8bbd 100644 --- a/daemon/extension/ScriptConfig.cpp +++ b/daemon/extension/ScriptConfig.cpp @@ -118,7 +118,9 @@ 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) + if (optEntry && writtenOptions.find(optEntry->GetName()) == writtenOptions.end()) { infile.Print("%s=%s\n", optEntry->GetName(), optEntry->GetValue()); writtenOptions.insert(optEntry->GetName()); diff --git a/tests/extension/ScriptConfig.cpp b/tests/extension/ScriptConfig.cpp index c8af84b32..20c087ae1 100644 --- a/tests/extension/ScriptConfig.cpp +++ b/tests/extension/ScriptConfig.cpp @@ -78,4 +78,42 @@ BOOST_AUTO_TEST_CASE(SaveConfigDoesNotAppendDuplicateOptionNames) 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"); +} + BOOST_AUTO_TEST_SUITE_END() From 0309227b06b0bebbf618838964b3b618f4d85be6 Mon Sep 17 00:00:00 2001 From: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com> Date: Fri, 31 Jul 2026 00:03:50 -0500 Subject: [PATCH 3/3] Keep the last value when collapsing duplicate config lines SaveConfig collapsed duplicate lines to the first matching entry's value, but Options::SetOption overwrites the entry in place while parsing, so the last line for a name is the value nzbget actually runs on. Healing a damaged config therefore discarded the live value and fell back to the option default on the next start - a disabled news server came back enabled. Resolve duplicates the same way the loader does: keep the first occurrence's name and position, but the last occurrence's value. Applied to both the replace and append paths. Extend the regression tests to assert the surviving value, not just the line count. --- daemon/extension/ScriptConfig.cpp | 24 ++++++++++++++-- tests/extension/ScriptConfig.cpp | 46 ++++++++++++++++++++++++++++++- 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/daemon/extension/ScriptConfig.cpp b/daemon/extension/ScriptConfig.cpp index 11b8e8bbd..a76287247 100644 --- a/daemon/extension/ScriptConfig.cpp +++ b/daemon/extension/ScriptConfig.cpp @@ -93,6 +93,22 @@ bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries) std::vector config; 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; CString content; @@ -119,10 +135,12 @@ bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries) { Options::OptEntry* optEntry = optEntries->FindOption(optname); // write each option only once, dropping duplicate lines accumulated - // in the config file by earlier versions (issue #588) + // 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()); + infile.Print("%s=%s\n", optEntry->GetName(), findLastValue(optEntry->GetName())); writtenOptions.insert(optEntry->GetName()); } } @@ -139,7 +157,7 @@ bool ScriptConfig::SaveConfig(Options::OptEntries* optEntries) 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 index 20c087ae1..ce44330d3 100644 --- a/tests/extension/ScriptConfig.cpp +++ b/tests/extension/ScriptConfig.cpp @@ -65,7 +65,9 @@ BOOST_AUTO_TEST_CASE(SaveConfigDoesNotAppendDuplicateOptionNames) contents << firstFile.rdbuf(); firstContents = contents.str(); } - BOOST_CHECK_EQUAL(firstContents, "# existing config\nExtension.Option=first\n"); + // 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; @@ -116,4 +118,46 @@ BOOST_AUTO_TEST_CASE(SaveConfigRemovesDuplicateLinesFromDamagedConfig) "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()