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 ea57ec7bbf640f09a3f3f128810c8d49e00c72c0..a29a69bf7b0b4e6aa83b4cacff282ac6c686c397 100644 (file)
--- 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 828b95dde52fd832203b35fd1b6928c18ad0384d..b5a9cbecd337477f39708fba65cb1b976c16ce69 100644 (file)
--- a/src/cache_manager.cc
+++ b/src/cache_manager.cc
@@ -152,6 +152,13 @@ CacheManager::createRequestedAction(const Mgr::ActionParams &params)
     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 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 aa2aa7450f8cd955ddac1aafb8e804bdb53133c7..a23aee741956c6cadfc30da1dfc00e3d696f69ab 100644 (file)
--- 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<const char *> magicPrefixes = {
-        "/",
-        "/squid-internal-mgr/"
-    };
-
     const std::vector<const char *> 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);
                 }
             }
         }