From 6695897990071a277aad797b9ed30a4746f5c4c0 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 | 13 +++- src/internal.cc | 3 +- src/tests/testCacheManager.cc | 133 +++++++++++++++++++--------------- 5 files changed, 91 insertions(+), 65 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 dcbb71e35c..0c4c4a0282 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -2608,8 +2608,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 \ @@ -2644,6 +2643,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 3173860144..bb70216c1a 100644 --- a/src/cache_manager.cc +++ b/src/cache_manager.cc @@ -165,6 +165,13 @@ MgrFieldChars(const AnyP::ProtocolType &protocol) return actionChars; } +const SBuf & +CacheManager::WellKnownUrlPathPrefix() +{ + static const SBuf prefix("/squid-internal-mgr/"); + return prefix; +} + /** * define whether the URL is a cache-manager URL and parse the action * requested by the user. Checks via CacheManager::ActionProtection() that the @@ -183,9 +190,7 @@ CacheManager::ParseUrl(const AnyP::Uri &uri) { Parser::Tokenizer tok(uri.path()); - static const SBuf internalMagicPrefix("/squid-internal-mgr/"); - if (!tok.skip(internalMagicPrefix) && !tok.skip('/')) - throw TextException("invalid URL path", Here()); + Assure(tok.skip(WellKnownUrlPathPrefix())); Mgr::Command::Pointer cmd = new Mgr::Command(); cmd->params.httpUri = SBufToString(uri.absolute()); @@ -394,7 +399,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 6503f4aff9..424aabf860 100644 --- a/src/tests/testCacheManager.cc +++ b/src/tests/testCacheManager.cc @@ -22,10 +22,38 @@ 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); }; -/* init memory pools */ +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"); +} void testCacheManager::setUp() { @@ -92,11 +120,6 @@ testCacheManager::testParseUrl() mgrUrl.host("localhost"); mgrUrl.port(3128); - const std::vector magicPrefixes = { - "/", - "/squid-internal-mgr/" - }; - const std::vector validActions = { "", "menu" @@ -157,62 +180,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 except cache_object require magic path prefix bytes - if (scheme != AnyP::PROTO_CACHE_OBJECT && 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