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.

diff --git a/src/CacheManager.h b/src/CacheManager.h
index ad6aabaf6b4cd2fe46a02d6579ec3d770cb78f13..54b2fb8a54a79ee6e5e3163245d90ab1d5c9e25e 100644 (file)
--- a/src/CacheManager.h
+++ b/src/CacheManager.h
@@ -34,6 +34,9 @@ class CacheManager
 public:
     typedef std::vector<Mgr::ActionProfilePointer> 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 dcbb71e35cb9bdf53a3a068571a8eaf4d545aebf..0c4c4a0282747b1e0f1321eb9a0ab838b170ac42 100644 (file)
--- 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 317386014492515929d6e7a7368787f662a062ed..bb70216c1a2481dea2c9dceb2c53aaf5b4e554cb 100644 (file)
--- 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 be5d61c29b0098baa2c1b2694febc8a9c9aa86da..6766f3bd2365c69268890dce284bbed7ec173707 100644 (file)
--- 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 6503f4aff9866b1caba5bd8af355ad009b612bba..424aabf8601e124e8793a9a3e351dda6edbc2c52 100644 (file)
--- 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<const char *> magicPrefixes = {
-        "/",
-        "/squid-internal-mgr/"
-    };
-
     const std::vector<const char *> 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);
                 }
             }
         }