From 3c383cc371e7ad69c533e629c6997f325aa3752d Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 30 Jul 2023 13:36:41 +0000 Subject: [PATCH] CacheManager: require /squid-internal-mgr/ URL path prefix (#1426) ERROR: Squid BUG: assurance failed: tok.skip(internalMagicPrefix) exception location: cache_manager.cc(173) ParseUrl Due to poor code duplication, commit 92a5adb accidentally classified URLs without a trailing slash in the magical prefix as valid cache manager URLs, triggering the above ERRORs. We were denying such "slashless" cache manager URLs (as invalid internal URLs) prior to that commit. Since that commit, the ERRORs triggered by that commit effectively denied them as well. Denying them properly results in simpler/smaller code (than allowing them would), so we should avoid a UI change and continue to deny them, at least for now. This change also reduces duplication of magic prefix definitions. Other pending work will completely eliminate that duplication in src/ code. --- src/CacheManager.h | 3 + src/Makefile.am | 4 +- src/cache_manager.cc | 12 ++- src/internal.cc | 3 +- src/tests/testCacheManager.cc | 133 ++++++++++++++++++++-------------- 5 files changed, 92 insertions(+), 63 deletions(-) diff --git a/src/CacheManager.h b/src/CacheManager.h index ad6aabaf6b..54b2fb8a54 100644 --- a/src/CacheManager.h +++ b/src/CacheManager.h @@ -34,6 +34,9 @@ class CacheManager public: typedef std::vector Menu; + /// initial URL path characters that identify cache manager requests + static const SBuf &WellKnownUrlPathPrefix(); + void registerProfile(char const * action, char const * desc, OBJH * handler, int pw_req_flag, int atomic); diff --git a/src/Makefile.am b/src/Makefile.am index ea57ec7bbf..a29a69bf7b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -2587,8 +2587,7 @@ tests_testCacheManager_SOURCES = \ wordlist.cc \ wordlist.h nodist_tests_testCacheManager_SOURCES = \ - $(BUILT_SOURCES) \ - tests/stub_libtime.cc + $(BUILT_SOURCES) # comm.cc only requires comm/libcomm.la until fdc_table is dead. tests_testCacheManager_LDADD = \ libsquid.la \ @@ -2623,6 +2622,7 @@ tests_testCacheManager_LDADD = \ mem/libmem.la \ store/libstore.la \ sbuf/libsbuf.la \ + time/libtime.la \ debug/libdebug.la \ $(top_builddir)/lib/libmisccontainers.la \ $(top_builddir)/lib/libmiscencoding.la \ diff --git a/src/cache_manager.cc b/src/cache_manager.cc index 828b95dde5..b5a9cbecd3 100644 --- a/src/cache_manager.cc +++ b/src/cache_manager.cc @@ -152,6 +152,13 @@ CacheManager::createRequestedAction(const Mgr::ActionParams ¶ms) return cmd->profile->creator->create(cmd); } +const SBuf & +CacheManager::WellKnownUrlPathPrefix() +{ + static const SBuf prefix("/squid-internal-mgr/"); + return prefix; +} + /** * Parses the action requested by the user and checks via * CacheManager::ActionProtection() that the item is accessible by the user. @@ -169,8 +176,7 @@ CacheManager::ParseUrl(const AnyP::Uri &uri) { Parser::Tokenizer tok(uri.path()); - static const SBuf internalMagicPrefix("/squid-internal-mgr/"); - Assure(tok.skip(internalMagicPrefix)); + Assure(tok.skip(WellKnownUrlPathPrefix())); Mgr::Command::Pointer cmd = new Mgr::Command(); cmd->params.httpUri = SBufToString(uri.absolute()); @@ -367,7 +373,7 @@ CacheManager::start(const Comm::ConnectionPointer &client, HttpRequest *request, client << " requesting '" << actionName << "'" ); - // special case: /squid-internal-mgr/ index page + // special case: an index page if (!strcmp(cmd->profile->name, "index")) { ErrorState err(MGR_INDEX, Http::scOkay, request, ale); err.url = xstrdup(entry->url()); diff --git a/src/internal.cc b/src/internal.cc index be5d61c29b..6766f3bd23 100644 --- a/src/internal.cc +++ b/src/internal.cc @@ -86,8 +86,7 @@ internalStaticCheck(const SBuf &urlPath) bool ForSomeCacheManager(const SBuf &urlPath) { - static const SBuf mgrPfx("/squid-internal-mgr"); - return urlPath.startsWith(mgrPfx); + return urlPath.startsWith(CacheManager::WellKnownUrlPathPrefix()); } /* diff --git a/src/tests/testCacheManager.cc b/src/tests/testCacheManager.cc index aa2aa7450f..a23aee7419 100644 --- a/src/tests/testCacheManager.cc +++ b/src/tests/testCacheManager.cc @@ -39,9 +39,39 @@ CPPUNIT_TEST_SUITE_REGISTRATION( TestCacheManager ); class CacheManagerInternals : public CacheManager { public: - void ParseUrl(const AnyP::Uri &u) { CacheManager::ParseUrl(u); } + /// checks CacheManager parsing of the given valid URL + void testValidUrl(const AnyP::Uri &); + + /// checks CacheManager parsing of the given invalid URL + /// \param problem a bad part of the URL or its description + void testInvalidUrl(const AnyP::Uri &, const char *problem); }; +void +CacheManagerInternals::testValidUrl(const AnyP::Uri &url) +{ + try { + (void)ParseUrl(url); + } catch (...) { + std::cerr << "\nFAIL: " << url << + Debug::Extra << "error: " << CurrentException << "\n"; + CPPUNIT_FAIL("rejected a valid URL"); + } +} + +void +CacheManagerInternals::testInvalidUrl(const AnyP::Uri &url, const char *const problem) +{ + try { + (void)ParseUrl(url); + std::cerr << "\nFAIL: " << url << + Debug::Extra << "error: should be rejected due to '" << problem << "'\n"; + } catch (const TextException &) { + return; // success -- the parser signaled bad input + } + CPPUNIT_FAIL("failed to reject an invalid URL"); +} + /// customizes our test setup class MyTestProgram: public TestProgram { @@ -115,11 +145,6 @@ TestCacheManager::testParseUrl() mgrUrl.host("localhost"); mgrUrl.port(3128); - const std::vector magicPrefixes = { - "/", - "/squid-internal-mgr/" - }; - const std::vector validActions = { "", "menu" @@ -180,62 +205,58 @@ TestCacheManager::testParseUrl() "#fragment" }; + const auto &prefix = CacheManager::WellKnownUrlPathPrefix(); + + assert(prefix.length()); + const auto insufficientPrefix = prefix.substr(0, prefix.length()-1); + for (const auto &scheme : validSchemes) { mgrUrl.setScheme(scheme); - for (const auto *magic : magicPrefixes) { - - // all schemes require magic path prefix bytes - if (strlen(magic) <= 2) - continue; - - /* Check the parser accepts all the valid cases */ - - for (const auto *action : validActions) { - for (const auto *param : validParams) { - for (const auto *frag : validFragments) { - try { - SBuf bits; - bits.append(magic); - bits.append(action); - bits.append(param); - bits.append(frag); - mgrUrl.path(bits); - - (void)mgr->ParseUrl(mgrUrl); - } catch (...) { - std::cerr << std::endl - << "FAIL: " << mgrUrl - << Debug::Extra << "error: " << CurrentException << std::endl; - CPPUNIT_FAIL("rejected a valid URL"); - } - } + // Check that the parser rejects URLs that lack the full prefix prefix. + // These negative tests log "Squid BUG: assurance failed" ERRORs because + // they violate CacheManager::ParseUrl()'s ForSomeCacheManager() + // precondition. + for (const auto *action : validActions) { + for (const auto *param : validParams) { + for (const auto *frag : validFragments) { + SBuf bits; + bits.append(insufficientPrefix); + bits.append(action); + bits.append(param); + bits.append(frag); + mgrUrl.path(bits); + mgr->testInvalidUrl(mgrUrl, "insufficient prefix"); } } + } + + // Check that the parser accepts valid URLs. + for (const auto action: validActions) { + for (const auto param: validParams) { + for (const auto frag: validFragments) { + SBuf bits; + bits.append(prefix); + bits.append(action); + bits.append(param); + bits.append(frag); + mgrUrl.path(bits); + mgr->testValidUrl(mgrUrl); + } + } + } - /* Check that invalid parameters are rejected */ - - for (const auto *action : validActions) { - for (const auto *param : invalidParams) { - for (const auto *frag : validFragments) { - try { - SBuf bits; - bits.append(magic); - bits.append(action); - bits.append(param); - bits.append(frag); - mgrUrl.path(bits); - - (void)mgr->ParseUrl(mgrUrl); - - std::cerr << std::endl - << "FAIL: " << mgrUrl - << Debug::Extra << "error: should be rejected due to '" << param << "'" << std::endl; - } catch (const TextException &e) { - continue; // success. caught bad input - } - CPPUNIT_FAIL("failed to reject an invalid URL"); - } + // Check that the parser rejects URLs with invalid parameters. + for (const auto action: validActions) { + for (const auto invalidParam: invalidParams) { + for (const auto frag: validFragments) { + SBuf bits; + bits.append(prefix); + bits.append(action); + bits.append(invalidParam); + bits.append(frag); + mgrUrl.path(bits); + mgr->testInvalidUrl(mgrUrl, invalidParam); } } } -- 2.47.2