Bug 1961 part 2: redesign of URL handling.

Move the HttpRequest::login detail into class URL. Renaming to userInfo
as per the RFC 3986 defined name for this URI piece.

Convert the details to SBuf gaining several minor str*() removals in
the process and a simpler FTP login parser based on SBuf capabilities.

Also, updated the base64 encoder API used for converting between URI
userInfo and Basic authentication header token. Gaining better control
over the length of maximum token size and a small speedup from pre-known
input length.

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index f6714a8943d5416a9d7da192400c95de8a52e547..d80a498811be3d9f5b80644e5c1c974bfccab613 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -69,7 +69,6 @@ HttpRequest::init()
     method = Http::METHOD_NONE;
     url.clear();
     urlpath = NULL;
-    login[0] = '\0';
     host[0] = '\0';
     host_is_numeric = -1;
 #if USE_AUTH
@@ -184,7 +183,7 @@ HttpRequest::clone() const
     copy->pstate = pstate; // TODO: should we assert a specific state here?
     copy->body_pipe = body_pipe;
 
-    strncpy(copy->login, login, sizeof(login)); // MAX_LOGIN_SZ
+    copy->url.userInfo(url.userInfo());
     strncpy(copy->host, host, sizeof(host)); // SQUIDHOSTNAMELEN
     copy->host_addr = host_addr;
 

diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index 5f50fbe70303a97a9ef1d5b08bb17db18af1b354..8c4716b78b1ef05f1032599ca5958664fe7ba354 100644 (file)
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -116,9 +116,7 @@ public:
     HttpRequestMethod method;
 
     // TODO expand to include all URI parts
-    URL url; ///< the request URI (scheme only)
-
-    char login[MAX_LOGIN_SZ];
+    URL url; ///< the request URI (scheme and userinfo only)
 
 private:
     char host[SQUIDHOSTNAMELEN];

diff --git a/src/URL.h b/src/URL.h
index d10672b696387aafaa34bce5faff00bfee833292..e1300da253d6a771091d9cfa2b851f7f4b0f1213 100644 (file)
--- a/src/URL.h
+++ b/src/URL.h
@@ -11,6 +11,7 @@
 
 #include "anyp/UriScheme.h"
 #include "MemPool.h"
+#include "SBuf.h"
 
 /**
  * The URL class represents a Uniform Resource Location
@@ -32,6 +33,9 @@ public:
     /// convert the URL scheme to that given
     void setScheme(const AnyP::ProtocolType &p) {scheme_=p;}
 
+    void userInfo(const SBuf &s) {userInfo_=s;}
+    const SBuf &userInfo() const {return userInfo_;}
+
 private:
     /**
      \par
@@ -54,6 +58,8 @@ private:
      * and immutable, only settable at construction time,
      */
     AnyP::UriScheme scheme_;
+
+    SBuf userInfo_; // aka 'URL-login'
 };
 
 class HttpRequest;

diff --git a/src/acl/UrlLogin.cc b/src/acl/UrlLogin.cc
index 657e84debf4a6bf4aed2196873998691e23c948d..5ba3c5a5104c9895a8ea975506423d95d48191e1 100644 (file)
--- a/src/acl/UrlLogin.cc
+++ b/src/acl/UrlLogin.cc
@@ -16,13 +16,19 @@
 #include "rfc1738.h"
 
 int
-ACLUrlLoginStrategy::match (ACLData<char const *> * &data, ACLFilledChecklist *checklist, ACLFlags &)
+ACLUrlLoginStrategy::match(ACLData<char const *> * &data, ACLFilledChecklist *checklist, ACLFlags &)
 {
-    char *esc_buf = xstrdup(checklist->request->login);
-    rfc1738_unescape(esc_buf);
-    int result = data->match(esc_buf);
-    safe_free(esc_buf);
-    return result;
+    if (checklist->request->url.userInfo().isEmpty()) {
+        debugs(28, 5, "URL has no user-info details. cannot match");
+        return 0; // nothing can match
+    }
+
+    static char str[MAX_URL]; // should be big enough for a single URI segment
+
+    const SBuf::size_type len = checklist->request->url.userInfo().copy(str, sizeof(str)-1);
+    str[len] = '\0';
+    rfc1738_unescape(str);
+    return data->match(str);
 }
 
 ACLUrlLoginStrategy *

diff --git a/src/client_side.cc b/src/client_side.cc
index f2ddf2b25eface1c36848ca90b0bdb9f2c89f3b7..22a08b5fdcb5a825b21065854c2464123a6b6b11 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2682,9 +2682,6 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c
                    ':' << request->port << " (not this proxy)");
     }
 
-    if (http->flags.internal)
-        request->login[0] = '\0';
-
     request->flags.internal = http->flags.internal;
     setLogUri (http, urlCanonicalClean(request.getRaw()));
     request->client_addr = conn->clientConnection->remote; // XXX: remove reuest->client_addr member.

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 0f0ccb231172b2da5fa07e1871859b2ad3ed25c0..d88c83a3c63a6fe1cd3576d42c0a5b0fe4dc7b77 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1129,7 +1129,7 @@ clientInterpretRequestHeaders(ClientHttpRequest * http)
 
     clientCheckPinning(http);
 
-    if (request->login[0] != '\0')
+    if (!request->url.userInfo().isEmpty())
         request->flags.auth = true;
 
     if (req_hdr->has(HDR_VIA)) {

diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc
index 7e605b0df4b0054308770b3358112aa53df3f440..9409ea186548447d32914eb5f3e0786a4afea606 100644 (file)
--- a/src/clients/FtpGateway.cc
+++ b/src/clients/FtpGateway.cc
@@ -126,7 +126,6 @@ public:
     // these should all be private
     virtual void start();
     virtual Http::StatusCode failedHttpStatus(err_type &error);
-    void loginParser(const char *, int escaped);
     int restartable();
     void appendSuccessHeader();
     void hackShortcut(StateMethod *nextState);
@@ -171,6 +170,8 @@ private:
     virtual bool mayReadVirginReplyBody() const;
     // BodyConsumer for HTTP: consume request body.
     virtual void handleRequestBodyProducerAborted();
+
+    void loginParser(const SBuf &login, bool escaped);
 };
 
 } // namespace Ftp
@@ -188,10 +189,6 @@ typedef struct {
     char *link;
 } ftpListParts;
 
-#define FTP_LOGIN_ESCAPED      1
-
-#define FTP_LOGIN_NOT_ESCAPED  0
-
 #define CTRL_BUFLEN 1024
 static char cbuf[CTRL_BUFLEN];
 
@@ -393,58 +390,54 @@ Ftp::Gateway::~Gateway()
 /**
  * Parse a possible login username:password pair.
  * Produces filled member variables user, password, password_url if anything found.
+ *
+ * \param login    a decoded Basic authentication credential token or URI user-info token
+ * \param escaped  whether to URL-decode the token after extracting user and password
  */
 void
-Ftp::Gateway::loginParser(const char *login, int escaped)
+Ftp::Gateway::loginParser(const SBuf &login, bool escaped)
 {
-    const char *u = NULL; // end of the username sub-string
-    int len;              // length of the current sub-string to handle.
+    debugs(9, 4, "login=" << login << ", escaped=" << escaped);
+    debugs(9, 9, "IN : login=" << login << ", escaped=" << escaped << ", user=" << user << ", password=" << password);
 
-    int total_len = strlen(login);
-
-    debugs(9, 4, HERE << ": login='" << login << "', escaped=" << escaped);
-    debugs(9, 9, HERE << ": IN : login='" << login << "', escaped=" << escaped << ", user=" << user << ", password=" << password);
-
-    if ((u = strchr(login, ':'))) {
+    if (login.isEmpty())
+        return;
 
-        /* if there was a username part */
-        if (u > login) {
-            len = u - login;
-            ++u; // jump off the delimiter.
-            if (len > MAX_URL)
-                len = MAX_URL-1;
-            xstrncpy(user, login, len +1);
-            debugs(9, 9, HERE << ": found user='" << user << "'(" << len <<"), escaped=" << escaped);
-            if (escaped)
-                rfc1738_unescape(user);
-            debugs(9, 9, HERE << ": found user='" << user << "'(" << len <<") unescaped.");
-        }
+    const SBuf::size_type colonPos = login.find(':');
 
-        /* if there was a password part */
-        len = login + total_len - u;
-        if ( len > 0) {
-            if (len > MAX_URL)
-                len = MAX_URL -1;
-            xstrncpy(password, u, len +1);
-            debugs(9, 9, HERE << ": found password='" << password << "'(" << len <<"), escaped=" << escaped);
-            if (escaped) {
-                rfc1738_unescape(password);
-                password_url = 1;
-            }
-            debugs(9, 9, HERE << ": found password='" << password << "'(" << len <<") unescaped.");
-        }
-    } else if (login[0]) {
-        /* no password, just username */
-        if (total_len > MAX_URL)
-            total_len = MAX_URL -1;
-        xstrncpy(user, login, total_len +1);
-        debugs(9, 9, HERE << ": found user='" << user << "'(" << total_len <<"), escaped=" << escaped);
+    /* If there was a username part with at least one character use it.
+     * Ignore 0-length username portion, retain what we have already.
+     */
+    if (colonPos == SBuf::npos || colonPos > 0) {
+        const SBuf userName = login.substr(0, colonPos);
+        SBuf::size_type upto = userName.copy(user, sizeof(user)-1);
+        user[upto]='\0';
+        debugs(9, 9, "found user=" << userName << ' ' <<
+               (upto != userName.length() ? ", truncated-to=" : ", length=") << upto <<
+               ", escaped=" << escaped);
         if (escaped)
             rfc1738_unescape(user);
-        debugs(9, 9, HERE << ": found user='" << user << "'(" << total_len <<") unescaped.");
+        debugs(9, 9, "found user=" << user << " (" << strlen(user) << ") unescaped.");
+    }
+
+    /* If there was a password part.
+     * For 0-length password clobber what we have already, this means explicitly none
+     */
+    if (colonPos != SBuf::npos) {
+        const SBuf pass = login.substr(colonPos+1, SBuf::npos);
+        SBuf::size_type upto = pass.copy(password, sizeof(password)-1);
+        password[upto]='\0';
+        debugs(9, 9, "found password=" << pass << " " <<
+               (upto != pass.length() ? ", truncated-to=" : ", length=") << upto <<
+               ", escaped=" << escaped);
+        if (escaped) {
+            rfc1738_unescape(password);
+            password_url = 1;
+        }
+        debugs(9, 9, "found password=" << password << " (" << strlen(password) << ") unescaped.");
     }
 
-    debugs(9, 9, HERE << ": OUT: login='" << login << "', escaped=" << escaped << ", user=" << user << ", password=" << password);
+    debugs(9, 9, "OUT: login=" << login << ", escaped=" << escaped << ", user=" << user << ", password=" << password);
 }
 
 void
@@ -1052,16 +1045,16 @@ Ftp::Gateway::checkAuth(const HttpHeader * req_hdr)
 
 #if HAVE_AUTH_MODULE_BASIC
     /* Check HTTP Authorization: headers (better than defaults, but less than URL) */
-    const char *auth;
-    if ( (auth = req_hdr->getAuth(HDR_AUTHORIZATION, "Basic")) ) {
+    const SBuf auth(req_hdr->getAuth(HDR_AUTHORIZATION, "Basic"));
+    if (!auth.isEmpty()) {
         flags.authenticated = 1;
-        loginParser(auth, FTP_LOGIN_NOT_ESCAPED);
+        loginParser(auth, false);
     }
     /* we fail with authorization-required error later IFF the FTP server requests it */
 #endif
 
     /* Test URL login syntax. Overrides any headers received. */
-    loginParser(request->login, FTP_LOGIN_ESCAPED);
+    loginParser(request->url.userInfo(), true);
 
     /* name is missing. thats fatal. */
     if (!user[0])

diff --git a/src/http.cc b/src/http.cc
index 0fb48186f584802f91f23638f90cd83ddb34f1c3..3426a15f3c659bbd1493c4d62f66663e36102e89 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1790,9 +1790,10 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
 
     /* append Authorization if known in URL, not in header and going direct */
     if (!hdr_out->has(HDR_AUTHORIZATION)) {
-        if (!request->flags.proxying && request->login[0] != '\0') {
-            httpHeaderPutStrf(hdr_out, HDR_AUTHORIZATION, "Basic %s",
-                              old_base64_encode(request->login));
+        if (!request->flags.proxying && !request->url.userInfo().isEmpty()) {
+            static char result[MAX_URL*2]; // should be big enough for a single URI segment
+            if (base64_encode_str(result, sizeof(result)-1, request->url.userInfo().rawContent(), request->url.userInfo().length()) < static_cast<int>(sizeof(result)-1))
+                httpHeaderPutStrf(hdr_out, HDR_AUTHORIZATION, "Basic %s", result);
         }
     }
 

diff --git a/src/icmp/net_db.cc b/src/icmp/net_db.cc
index d13a633c520173084e74a8842ed3857464b50676..550a304bfdc813a3017d22447dea711d4d576eb5 100644 (file)
--- a/src/icmp/net_db.cc
+++ b/src/icmp/net_db.cc
@@ -1307,8 +1307,9 @@ netdbExchangeStart(void *data)
                     netdbExchangeHandleReply, ex);
     ex->r->flags.loopDetected = true;  /* cheat! -- force direct */
 
+    // XXX: send as Proxy-Authenticate instead
     if (p->login)
-        xstrncpy(ex->r->login, p->login, MAX_LOGIN_SZ);
+        ex->r->url.userInfo(SBuf(p->login));
 
     urlCanonical(ex->r);
 

diff --git a/src/url.cc b/src/url.cc
index c72bdac001c3442888e19e6f9e700df9a6c164fa..0a858eef26d32912ee2b0e32fc52f118dc6a318c 100644 (file)
--- a/src/url.cc
+++ b/src/url.cc
@@ -20,7 +20,7 @@ static HttpRequest *urlParseFinish(const HttpRequestMethod& method,
                                    const AnyP::ProtocolType protocol,
                                    const char *const urlpath,
                                    const char *const host,
-                                   const char *const login,
+                                   const SBuf &login,
                                    const int port,
                                    HttpRequest *request);
 static HttpRequest *urnParse(const HttpRequestMethod& method, char *urn, HttpRequest *request);
@@ -218,7 +218,7 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
                strcmp(url, "*") == 0) {
         protocol = AnyP::PROTO_HTTP;
         port = urlDefaultPort(protocol);
-        return urlParseFinish(method, protocol, url, host, login, port, request);
+        return urlParseFinish(method, protocol, url, host, SBuf(), port, request);
     } else if (!strncmp(url, "urn:", 4)) {
         return urnParse(method, url, request);
     } else {
@@ -423,7 +423,7 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request)
         }
     }
 
-    return urlParseFinish(method, protocol, urlpath, host, login, port, request);
+    return urlParseFinish(method, protocol, urlpath, host, SBuf(login), port, request);
 }
 
 /**
@@ -436,7 +436,7 @@ urlParseFinish(const HttpRequestMethod& method,
                const AnyP::ProtocolType protocol,
                const char *const urlpath,
                const char *const host,
-               const char *const login,
+               const SBuf &login,
                const int port,
                HttpRequest *request)
 {
@@ -448,7 +448,7 @@ urlParseFinish(const HttpRequestMethod& method,
     }
 
     request->SetHost(host);
-    xstrncpy(request->login, login, MAX_LOGIN_SZ);
+    request->url.userInfo(login);
     request->port = (unsigned short) port;
     return request;
 }
@@ -491,10 +491,10 @@ urlCanonical(HttpRequest * request)
             if (request->port != urlDefaultPort(request->url.getScheme()))
                 snprintf(portbuf, 32, ":%d", request->port);
 
-            snprintf(urlbuf, MAX_URL, "%s://%s%s%s%s" SQUIDSTRINGPH,
+            snprintf(urlbuf, MAX_URL, "%s://" SQUIDSBUFPH "%s%s%s" SQUIDSTRINGPH,
                      request->url.getScheme().c_str(),
-                     request->login,
-                     *request->login ? "@" : null_string,
+                     SQUIDSBUFPRINT(request->url.userInfo()),
+                     !request->url.userInfo().isEmpty() ? "@" : "",
                      request->GetHost(),
                      portbuf,
                      SQUIDSTRINGPRINT(request->urlpath));
@@ -514,7 +514,6 @@ urlCanonicalClean(const HttpRequest * request)
 {
     LOCAL_ARRAY(char, buf, MAX_URL);
     LOCAL_ARRAY(char, portbuf, 32);
-    LOCAL_ARRAY(char, loginbuf, MAX_LOGIN_SZ + 1);
     char *t;
 
     if (request->url.getScheme() == AnyP::PROTO_URN) {
@@ -533,20 +532,10 @@ urlCanonicalClean(const HttpRequest * request)
             if (request->port != urlDefaultPort(request->url.getScheme()))
                 snprintf(portbuf, 32, ":%d", request->port);
 
-            loginbuf[0] = '\0';
-
-            if ((int) strlen(request->login) > 0) {
-                strcpy(loginbuf, request->login);
-
-                if ((t = strchr(loginbuf, ':')))
-                    *t = '\0';
-
-                strcat(loginbuf, "@");
-            }
-
-            snprintf(buf, MAX_URL, "%s://%s%s%s" SQUIDSTRINGPH,
+            snprintf(buf, MAX_URL, "%s://" SQUIDSBUFPH "%s%s%s" SQUIDSTRINGPH,
                      request->url.getScheme().c_str(),
-                     loginbuf,
+                     SQUIDSBUFPRINT(request->url.userInfo()),
+                     (request->url.userInfo().isEmpty() ? "" : "@"),
                      request->GetHost(),
                      portbuf,
                      SQUIDSTRINGPRINT(request->urlpath));
@@ -556,7 +545,7 @@ urlCanonicalClean(const HttpRequest * request)
                 if ((t = strchr(buf, '?')))
                     *(++t) = '\0';
         }
-        }
+        } // switch
     }
 
     if (stringHasCntl(buf))
@@ -644,18 +633,18 @@ urlMakeAbsolute(const HttpRequest * req, const char *relUrl)
     size_t urllen;
 
     if (req->port != urlDefaultPort(req->url.getScheme())) {
-        urllen = snprintf(urlbuf, MAX_URL, "%s://%s%s%s:%d",
+        urllen = snprintf(urlbuf, MAX_URL, "%s://" SQUIDSBUFPH "%s%s:%d",
                           req->url.getScheme().c_str(),
-                          req->login,
-                          *req->login ? "@" : null_string,
+                          SQUIDSBUFPRINT(req->url.userInfo()),
+                          !req->url.userInfo().isEmpty() ? "@" : "",
                           req->GetHost(),
                           req->port
                          );
     } else {
-        urllen = snprintf(urlbuf, MAX_URL, "%s://%s%s%s",
+        urllen = snprintf(urlbuf, MAX_URL, "%s://" SQUIDSBUFPH "%s%s",
                           req->url.getScheme().c_str(),
-                          req->login,
-                          *req->login ? "@" : null_string,
+                          SQUIDSBUFPRINT(req->url.userInfo()),
+                          !req->url.userInfo().isEmpty() ? "@" : "",
                           req->GetHost()
                          );
     }