6ba4e724f84f86544c11a90f3802fd6318cd154f
angie
  Fri Dec 21 16:28:42 2018 -0800
Watch out for invalid data and unencoded newlines when reading in file uploaded as a saved session.  refs #22638, #22637

cartLoadSettings did absolutely no checking that its input looked anything like cart settings, resulting in giant useless carts in which custom track files, hgTracks HTML and other arbitrary files apparently were parsed as cart settings.  Now we do quite a bit of checking in cartLoadSettingsFromUserInput, and can warn the user if it looks like they are attempting to upload a custom track as a saved session.

The unencoded newlines are our fault -- since 2010, we have been writing out saved session files without encoding newlines in setting values (pasted in by the user).  Then those newlines can be mistaken for the newlines that we use to separate settings.  Oops!  Use a heuristic that holds up so far to determine whether the newlines should have been encoded: they seem to occur in specific variables (like hgta_pastedIdentifiers), and those variables always appear in the middle of an alphabetically sorted cluster of variables with the same prefix (like hgta_).

And of course, we now write out saved session files with encoded newlines, which we decode when reading back in.

We even have to deal with carts that have mixed types of newlines from various garbage sources, using lineFileCarefulNewlines().

diff --git src/hg/lib/cart.c src/hg/lib/cart.c
index 27faf09..7fb781e 100644
--- src/hg/lib/cart.c
+++ src/hg/lib/cart.c
@@ -18,30 +18,31 @@
 #include "jksql.h"
 #include "jsHelper.h"
 #include "trashDir.h"
 #ifndef GBROWSE
 #include "customFactory.h"
 #include "googleAnalytics.h"
 #include "wikiLink.h"
 #endif /* GBROWSE */
 #include "hgMaf.h"
 #include "hui.h"
 #include "geoMirror.h"
 #include "hubConnect.h"
 #include "trackHub.h"
 #include "cgiApoptosis.h"
 #include "customComposite.h"
+#include "regexHelper.h"
 
 static char *sessionVar = "hgsid";	/* Name of cgi variable session is stored in. */
 static char *positionCgiName = "position";
 
 DbConnector cartDefaultConnector = hConnectCart;
 DbDisconnect cartDefaultDisconnector = hDisconnectCart;
 
 static void hashUpdateDynamicVal(struct hash *hash, char *name, void *val)
 /* Val is a dynamically allocated (freeMem-able) entity to put
  * in hash.  Override existing hash item with that name if any.
  * Otherwise make new hash item. */
 {
 struct hashEl *hel = hashLookup(hash, name);
 if (hel == NULL)
     hashAdd(hash, name, val);
@@ -599,85 +600,605 @@
 	    cartRemove(cart, actionVar);
 	hDisconnectCentral(&conn2);
 	}
     else
 	errAbort("Sharing has not been enabled for user %s's session %s.",
 		 sessionOwner, sessionName);
     }
 else
     errAbort("Could not find session %s for user %s.",
 	     sessionName, sessionOwner);
 sqlFreeResult(&sr);
 freeMem(encSessionName);
 }
 #endif /* GBROWSE */
 
-void cartLoadSettings(struct lineFile *lf, struct cart *cart,
-		      struct hash *oldVars, char *actionVar)
-/* Load settings (cartDump output) into current session, and then
- * reload the CGI settings (to support override of session settings).
+boolean containsNonPrintable(char *string)
+/* Return TRUE if string contains non-ascii printable character(s). */
+{
+if (isEmpty(string))
+    return FALSE;
+boolean hasNonPrintable = FALSE;
+int i;
+for (i = 0;  string[i] != '\0';  i++)
+    {
+    if ((string[i] < 32 || string[i] > 126) && string[i] != '\t')
+        {
+        hasNonPrintable = TRUE;
+        break;
+        }
+    }
+return hasNonPrintable;
+}
+
+enum vsErrorType { vsNone=0, vsValid, vsBinary, vsWeird, vsData, vsVarLong, vsValLong };
+
+struct validityStats
+/* Watch out for incoming garbage data loaded as if it were a saved session file.
+ * This helps decide whether to just bail or simply remove some garbage and alert the user. */
+{
+    char *weirdCharsExample;    // First "cart variable" with unexpected punct/space/etc
+    char *dataExample;          // First "cart variable" that looks like custom track data
+    char *valTooLongExample;    // First cart variable whose value is too long to include.
+    uint validCount;            // Number of cart variables with no red flags, so *probably* ok.
+    uint binaryCount;           // Number of "cart variables" with binary data
+    uint weirdCharsCount;       // Number of "cart variables" with unexpected punct/space/etc
+    uint dataCount;             // Number of "cart variables" that look like custom track data
+    uint varTooLongCount;       // Number of "cart variables" whose length is too long.
+    uint varTooLongLength;      // Longest too-long cart var found.
+    uint valTooLongCount;       // Number of cart variables whose values are too long.
+    uint valTooLongLength;      // Longest too-long cart var value found.
+    enum vsErrorType lastType;  // The latest type of error, so we can roll back the previous call.
+};
+
+static void vsInit(struct validityStats *stats)
+/* Set all counts to 0 and pointers to NULL. */
+{
+ZeroVar(stats);
+}
+
+static void vsFreeMembers(struct validityStats *stats)
+/* Free all allocated members of stats (not stats itself, it may be a stack var). */
+{
+freeMem(stats->weirdCharsExample);
+freeMem(stats->dataExample);
+freeMem(stats->valTooLongExample);
+}
+
+static void vsGotValid(struct validityStats *stats)
+/* Update validity stats after finding a cart var with no obvious red flags.  (No guarantee
+ * it's actually a real cart variable, but at worst it's just bloating the cart a bit.) */
+{
+if (stats)
+    {
+    stats->validCount++;
+    stats->lastType = vsValid;
+    }
+}
+
+static void vsGotBinary(struct validityStats *stats)
+/* Update validity stats after finding unprintable characters in "cart var". */
+{
+if (stats)
+    {
+    stats->binaryCount++;
+    stats->lastType = vsBinary;
+    }
+}
+
+static void vsGotWeirdChar(struct validityStats *stats, char *var)
+/* Update validity stats after finding unexpected but printable characters in "cart var". */
+{
+if (stats)
+    {
+    stats->weirdCharsCount++;
+    if (stats->weirdCharsExample == NULL)
+        stats->weirdCharsExample = cloneString(var);
+    stats->lastType = vsWeird;
+    }
+}
+
+static void vsGotData(struct validityStats *stats, char *var)
+/* Update validity stats after finding apparent custom track data in "cart var". */
+{
+if (stats)
+    {
+    stats->dataCount++;
+    if (stats->dataExample == NULL)
+        stats->dataExample = cloneString(var);
+    stats->lastType = vsData;
+    }
+}
+
+static void vsGotVarTooLong(struct validityStats *stats, size_t varLen)
+/* Update validity stats after finding suspiciously lengthy "cart var". */
+{
+if (stats)
+    {
+    stats->varTooLongCount++;
+    stats->varTooLongLength = max(stats->varTooLongLength, varLen);
+    stats->lastType = vsVarLong;
+    }
+}
+
+static void vsGotValTooLong(struct validityStats *stats, char *var, size_t valLen)
+/* Update validity stats after finding cart var whose value is too long. */
+{
+if (stats)
+    {
+    stats->valTooLongCount++;
+    stats->valTooLongLength = max (stats->valTooLongLength, valLen);
+    if (stats->valTooLongExample == NULL)
+        stats->valTooLongExample = cloneString(var);
+    stats->lastType = vsValLong;
+    }
+}
+
+static void vsUndo(struct validityStats *stats)
+/* Roll back the latest increment to stats after finding an exceptional case.
+   Only one level of undo is supported.  Not supported for vs{Binary,VarLong,ValLong}. */
+{
+if (stats)
+    {
+    switch (stats->lastType)
+        {
+        case vsNone:
+            errAbort("vsUndo: nothing to undo (only one level of undo is possible)");
+            break;
+        case vsValid:
+            stats->validCount--;
+            stats->lastType = vsNone;
+            break;
+        case vsWeird:
+            stats->weirdCharsCount--;
+            if (stats->weirdCharsCount < 1)
+                freez(&stats->weirdCharsExample);
+            stats->lastType = vsNone;
+            break;
+        case vsData:
+            stats->dataCount--;
+            if (stats->dataCount < 1)
+                freez(&stats->dataExample);
+            stats->lastType = vsNone;
+            break;
+        case vsBinary:
+        case vsVarLong:
+        case vsValLong:
+            errAbort("vsUndo: not supported for lastType vsBinary, vsVarLong or vsValLong (%d)",
+                     stats->lastType);
+            break;
+        default:
+            errAbort("vsUndo: invalid lastType %d", stats->lastType);
+        }
+    }
+}
+
+#define CART_LOAD_TOO_MANY_ERRORS 100
+#define CART_LOAD_ENOUGH_VALID 20
+#define CART_LOAD_WAY_TOO_MANY_ERRORS 1000
+
+static boolean vsTooManyErrors(struct validityStats *stats)
+/* Return TRUE if the input seems to be completely invalid. */
+{
+if (stats)
+    {
+    uint errorSum = (stats->binaryCount + stats->weirdCharsCount + stats->dataCount +
+                     stats->varTooLongCount);
+    uint total = errorSum + stats->validCount;
+    return ((total > (CART_LOAD_TOO_MANY_ERRORS + CART_LOAD_ENOUGH_VALID) &&
+             errorSum > CART_LOAD_TOO_MANY_ERRORS &&
+             stats->validCount < CART_LOAD_ENOUGH_VALID) ||
+            errorSum > CART_LOAD_WAY_TOO_MANY_ERRORS);
+    }
+return FALSE;
+}
+
+#define CART_VAR_MAX_LENGTH 1024
+#define CART_VAL_MAX_LENGTH (64 * 1024)
+
+static void vsReport(struct validityStats *stats, struct dyString *dyMessage)
+/* Append summary/explanation to dyMessage.   */
+{
+if (stats && dyMessage)
+    {
+    dyStringPrintf(dyMessage, "<br>%d valid settings found.  ", stats->validCount);
+    if (stats->binaryCount || stats->weirdCharsCount || stats->dataCount ||
+        stats->varTooLongCount || stats->valTooLongCount)
+        dyStringPrintf(dyMessage, "<b>Note: invalid settings were found and omitted.</b>  ");
+    if (stats->binaryCount)
+        dyStringPrintf(dyMessage, "%d setting names contained binary data.  ",
+                       stats->binaryCount);
+    if (stats->weirdCharsCount)
+        dyStringPrintf(dyMessage,
+                       "%d setting names contained unexpected characters, for example '%s'.  ",
+                       stats->weirdCharsCount, stats->weirdCharsExample);
+    if (stats->dataCount)
+        dyStringPrintf(dyMessage, "%d lines appeared to be custom track data, for example "
+                       "a line begins with '%s'.  ",
+                       stats->dataCount, stats->dataExample);
+    if (stats->varTooLongCount)
+        dyStringPrintf(dyMessage, "%d setting names were too long (up to %d).  ",
+                       stats->varTooLongCount, stats->varTooLongLength);
+    if (stats->valTooLongCount)
+        dyStringPrintf(dyMessage, "%d setting values were too long (up to %d).  ",
+                       stats->valTooLongCount, stats->valTooLongLength);
+    }
+if (vsTooManyErrors(stats))
+    dyStringPrintf(dyMessage, "Encountered too many errors -- quitting.  ");
+}
+
+// Our timestamp vars (_, hgt_) are an exception to the usual cart var naming patterns:
+#define CART_VAR_TIMESTAMP "^([a-z]+)?_$"
+// Legitimate cart vars look like this (but so do some not-vars, so we filter further below):
+#define CART_VAR_VALID_CHARACTERS "^[A-Za-z]([A-Za-z0-9._:-]*[A-Za-z0-9]+)?$"
+
+// These are "cart variables" that are actually custom track data:
+static char *cartVarBlackList[] = { "X",
+                                    "Y",
+                                    "MT",
+                                    "fixedStep",
+                                    "variableStep",
+                                   };
+
+// Prefixes of "cart variables" from data files that have caused trouble in the past:
+static char *cartVarBlackListPrefix[] = { "ENS",                // Giant Ensembl gene info dump
+                                          "RRBS",               // Some other big tab-sep dump
+                                          "VGXS",               // Genotypes
+                                          NULL };
+
+// More complicated patterns of custom track or genotype data:
+static char *cartVarBlackListRegex[] = { "^chr[0-9XYMTUnLR]+(_[a-zA-Z0-9_]+)?$",
+                                         "^(chr)?[A-Z]{2}[0-9]{5}[0-9]+",
+                                         "^rs[0-9]+$",          // Genotypes
+                                         "^i[0-9]{5}[0-9]*$)",  // Genotypes
+                                         NULL };
+
+static boolean isValidCartVar(char *var, struct validityStats *stats)
+/* Return TRUE if var looks like a plausible cart variable name (as opposed to other stuff
+ * that users try to load in as saved-to-file session data).  If var doesn't look right,
+ * return FALSE and if stats is not NULL, record the problem. */
+{
+boolean isValid = TRUE;
+size_t varLen = strlen(var);
+if (containsNonPrintable(var))
+    {
+    vsGotBinary(stats);
+    isValid = FALSE;
+    }
+else if (varLen > CART_VAR_MAX_LENGTH)
+    {
+    vsGotVarTooLong(stats, varLen);
+    isValid = FALSE;
+    }
+else if (!regexMatch(var, CART_VAR_TIMESTAMP) &&
+         !regexMatch(var, CART_VAR_VALID_CHARACTERS))
+    {
+    vsGotWeirdChar(stats, var);
+    isValid = FALSE;
+    }
+else
+    {
+    if (stringArrayIx(var, cartVarBlackList, ArraySize(cartVarBlackList)) >= 0)
+        {
+        vsGotData(stats, var);
+        isValid = FALSE;
+        }
+    else
+        {
+        int i;
+        for (i = 0;  cartVarBlackListPrefix[i] != NULL;  i++)
+            {
+            if (startsWith(cartVarBlackListPrefix[i], var))
+                {
+                vsGotData(stats, var);
+                isValid = FALSE;
+                break;
+                }
+            }
+        if (isValid)
+            for (i = 0;  cartVarBlackListRegex[i] != NULL;  i++)
+                {
+                if (regexMatchNoCase(var, cartVarBlackListRegex[i]))
+                    {
+                    vsGotData(stats, var);
+                    isValid = FALSE;
+                    break;
+                    }
+                }
+        }
+    }
+if (isValid)
+    vsGotValid(stats);
+return isValid;
+}
+
+static char *encodeForHgSession(char *string)
+/* Allocate and return a new string with \-escaped '\\' and '\n' so that newline characters
+ * don't cause bogus cart variables to appear (while truncating the original value) in files
+ * downloaded from hgSession.  Convert "\r\n" and lone '\r' to '\n'. */
+{
+if (string == NULL)
+    return NULL;
+int inLen = strlen(string);
+char outBuf[2*inLen + 1];
+char *pIn, *pOut;
+for (pIn = string, pOut = outBuf;  *pIn != '\0';  pIn++)
+    {
+    if (*pIn == '\\')
+        {
+        *pOut++ = '\\';
+        *pOut++ = '\\';
+        }
+    else if (*pIn == '\r')
+        {
+        if (*(pIn+1) != '\n')
+            {
+            *pOut++ = '\\';
+            *pOut++ = 'n';
+            }
+        }
+    else if (*pIn == '\n')
+        {
+        *pOut++ = '\\';
+        *pOut++ = 'n';
+        }
+    else
+        *pOut++ = *pIn;
+    }
+*pOut = '\0';
+return cloneString(outBuf);
+}
+
+static void decodeForHgSession(char *string)
+/* Decode in place \-escaped '\\' and '\n' in string.  Note: some older files have only
+ * \n escaped, not backslashes -- so watch out for those. */
+{
+if (string == NULL)
+    return;
+char *pIn, *pOut;
+for (pIn = string, pOut = string;  *pIn != '\0';  pIn++, pOut++)
+    {
+    if (*pIn == '\\')
+        {
+        char *pNext = pIn + 1;
+        if (*pNext == 'n')
+            {
+            pIn++;
+            *pOut = '\n';
+            }
+        else if (*pNext == '\\')
+            {
+            pIn++;
+            *pOut = '\\';
+            }
+        else
+            // '\\' followed by anything other than '\\' or 'n' means we're reading in
+            // an older file in which '\\' was not escaped; ignore.
+            *pOut = *pIn;
+        }
+    else
+        *pOut = *pIn;
+    }
+*pOut = '\0';
+}
+
+// DEVELOPER NOTE: If you add anything to this list, verify that the specific multiline
+// input variable occurs in the middle of an alphabetically ordered cluster of variables
+// with the same CGI prefix.  If so, hasMultilineCgiPrefix needs to include the prefix.
+// If not then we need a new approach to detecting unencoded newlines.
+static char *multilineVars[] = { "hgS_newSessionDescription",
+                                 "hgta_enteredUserRegionFile",
+                                 "hgta_enteredUserRegions",
+                                 "hgta_pastedIdentifiers",
+                                 "hgva_hgvs",
+                                 "hgva_variantIds",
+                                 "phyloGif_tree",
+                                 "phyloPng_tree",
+                                 "suggestDetails" };
+
+static boolean isMultilineVar(char *var)
+/* Return TRUE if var may contain newlines that we forgot to encode for many years. */
+{
+return (var && stringArrayIx(var, multilineVars, ArraySize(multilineVars)) >= 0);
+}
+
+static boolean hasMultilineCgiPrefix(char *var)
+/* Return TRUE if var seems to come from the same CGI as a multiline var. */
+{
+boolean matches = FALSE;
+if (isNotEmpty(var))
+    {
+    if (startsWith("hgS_", var) ||
+        startsWith("hgta_", var) ||
+        startsWith("hgva_", var) ||
+        startsWith("phyloGif_", var) ||
+        startsWith("phyloPng_", var) ||
+        startsWith("suggest", var))
+        matches = TRUE;
+    }
+return matches;
+}
+
+static void extendPrevCartVar(struct cart *cart, char *prevVar, char *var, char *val)
+/* Concatenate newline and var/val onto previous variable's value. */
+{
+char *prevVal = cartString(cart, prevVar);
+struct dyString *dy = dyStringCreate("%s\n%s", prevVal, var);
+if (isNotEmpty(val))
+    dyStringPrintf(dy, " %s", val);
+cartSetString(cart, prevVar, dy->string);
+dyStringFree(&dy);
+}
+
+static void updatePrevVar(char **pPrevVar, char *var)
+/* If pPrevVar is not NULL, free the old value of *pPrevVar and set it to a clone of var. */
+{
+if (pPrevVar)
+    {
+    freeMem(*pPrevVar);
+    *pPrevVar = cloneString(var);
+    }
+}
+
+static boolean cartAddSettingIfValid(struct cart *cart, char *var, char *val,
+                                     struct validityStats *stats, char **pPrevVar,
+                                     boolean decodeVal)
+/* If var and val raise no red flags then add the setting to cart and return TRUE.
+ * Use *pPrevVar to detect and fix unencoded newlines.  Update *pPrevVar if setting is valid.
+ * If decodeVal, then call decodeForHgSession on val (from hgSession saved settings file). */
+{
+char *prevVar = pPrevVar ? *pPrevVar : NULL;
+boolean addToCart = TRUE;
+if (! isValidCartVar(var, stats))
+    {
+    // This might be a sign of garbage being uploaded as a session -- or it might
+    // be a case of unencoded newlines causing the appearance of bogus cart variables.
+    addToCart = FALSE;
+    if (isMultilineVar(prevVar) &&
+        (stats->lastType == vsWeird || stats->lastType == vsData))
+        {
+        // It's our fault with the unencoded newlines, don't count it as invalid cart var.
+        vsUndo(stats);
+        extendPrevCartVar(cart, prevVar, var, val);
+        }
+    }
+else if (isMultilineVar(prevVar))
+    {
+    // We need to watch out for "vars" that make it past the validity patterns
+    // but are part of unencoded multiline input.
+    // A bit dicey, but all of the multi-line variables that I know of occur
+    // in the middle of an alphabetized cluster of vars with the same prefix.
+    // DEVELOPER NOTE: check that assumption when changing multilineVars/hasMultilineCgiPrefix.
+    if (! hasMultilineCgiPrefix(var))
+        {
+        extendPrevCartVar(cart, prevVar, var, val);
+        addToCart = FALSE;
+        }
+    else
+        {
+        // Check cart var length post-extension.
+        char *extendedVal = cartOptionalString(cart, prevVar);
+        size_t extendedValLen = strlen(extendedVal);
+        if (extendedValLen > CART_VAL_MAX_LENGTH)
+            {
+            vsGotValTooLong(stats, prevVar, extendedValLen);
+            cartRemove(cart, prevVar);
+            }
+        }
+    }
+if (addToCart)
+    {
+    if (val != NULL)
+        {
+        size_t valLen = strlen(val);
+        if (valLen > CART_VAL_MAX_LENGTH)
+            {
+            addToCart = FALSE;
+            vsGotValTooLong(stats, var, valLen);
+            }
+        else
+            {
+            if (decodeVal)
+                decodeForHgSession(val);
+            cartAddString(cart, var, val);
+            updatePrevVar(pPrevVar, var);
+            }
+        }
+    else if (var != NULL)
+        {
+        cartSetString(cart, var, "");
+        updatePrevVar(pPrevVar, var);
+        }
+    }
+return addToCart;
+}
+
+boolean cartLoadSettingsFromUserInput(struct lineFile *lf, struct cart *cart, struct hash *oldVars,
+                                      char *actionVar, struct dyString *dyMessage)
+/* Verify that the user data in lf looks like valid settings (hgSession saved file;
+ * like cartDump output, but values may or may not be htmlEncoded).
+ * Older session files may have unencoded newlines, causing bogus variables;
+ * watch out for those after pasted input variables like hgta_pastedIdentifiers.
+ * Users have uploaded custom tracks, DTC genotypes, hgTracks HTML, even
+ * binary data files.  Look for problematic patterns observed in the past.
+ * Load settings into current session, and then reload the CGI settings
+ * (to support override of session settings).
  * If non-NULL, oldVars will contain values overloaded when reloading CGI.
  * If non-NULL, actionVar is a cartRemove wildcard string specifying the
- * CGI action variable that sent us here. */
+ * CGI action variable that sent us here.
+ * If input contains suspect data, then add diagnostics to dyMessage.  If input
+ * contains so much garbage that we shouldn't even try to load what passes the filters,
+ * return FALSE. */
 {
+boolean isValidEnough = TRUE;
 char *line = NULL;
 int size = 0;
 char *sessionVar = cartSessionVarName();
 char *hgsid = cartSessionId(cart);
 char *sessionTableString = cartOptionalString(cart, hgSessionTableState);
 sessionTableString = cloneString(sessionTableString);
 char *pubSessionsTableString = cartOptionalString(cart, hgPublicSessionsTableState);
 pubSessionsTableString = cloneString(pubSessionsTableString);
 cartRemoveLike(cart, "*");
 cartSetString(cart, sessionVar, hgsid);
 if (sessionTableString != NULL)
     cartSetString(cart, hgSessionTableState, sessionTableString);
 if (pubSessionsTableString != NULL)
     cartSetString(cart, hgPublicSessionsTableState, pubSessionsTableString);
 
+char *prevVar = NULL;
+struct validityStats stats;
+vsInit(&stats);
 while (lineFileNext(lf, &line, &size))
     {
-    char *var = nextWord(&line);
-    if (isEmpty(var))
-        // blank line
+    char *var = line;
+    // We actually want to keep leading spaces intact... they're a sign of var not being a real var.
+    char *p = skipLeadingSpaces(var);
+    if (!p)
+        p = var;
+    char *val = skipToSpaces(p);
+    if (val)
+        *val++ = '\0';
+    if (isEmpty(var) || var[0] == '#')
+        // Ignore blank line / comment
         continue;
-    char *val = line;
-
-    if (sameString(var, sessionVar))
+    else if (sameString(var, sessionVar))
+        // Ignore old sessionVar (already set above)
 	continue;
-    else
+    else if (! cartAddSettingIfValid(cart, var, val, &stats, &prevVar, TRUE))
         {
-	if (val != NULL)
+        if (vsTooManyErrors(&stats))
             {
-	    struct dyString *dy = dyStringSub(val, "\\n", "\n");
-	    cartAddString(cart, var, dy->string);
-	    dyStringFree(&dy);
+            isValidEnough = FALSE;
+            break;
+            }
         }
-	else if (var != NULL)
-	    {
-	    cartSetString(cart, var, "");
     }
-	} /* not hgsid */
-    } /* each line */
+freeMem(prevVar);
+if (isValidEnough)
+    {
     if (oldVars)
         hashEmpty(oldVars);
     /* Overload settings explicitly passed in via CGI (except for the
      * command that sent us here): */
     loadCgiOverHash(cart, oldVars);
-
+    }
 if (isNotEmpty(actionVar))
     cartRemove(cart, actionVar);
+vsReport(&stats, dyMessage);
+vsFreeMembers(&stats);
+return isValidEnough;
 }
 
 static char *now()
 /* Return a mysql-formatted time like "2008-05-19 15:33:34". */
 {
 char nowBuf[256];
 time_t seconds = clock1();
 struct tm *theTime = localtime(&seconds);
 strftime(nowBuf, sizeof nowBuf, "%Y-%m-%d %H:%M:%S", theTime);
 return cloneString(nowBuf);
 }
 
 static struct cartDb *emptyCartDb()
 /* Create a new empty placeholder cartDb. */
 {
@@ -825,34 +1346,40 @@
     if (cartVarExists(cart, hgsDoOtherUser))
 	{
 	char *otherUser = cartString(cart, hgsOtherUserName);
 	char *sessionName = cartString(cart, hgsOtherUserSessionName);
 	struct sqlConnection *conn2 = hConnectCentral();
 	cartLoadUserSession(conn2, otherUser, sessionName, cart,
 			    oldVars, hgsDoOtherUser);
 	hDisconnectCentral(&conn2);
 	cartTrace(cart, "after cartLUS", conn);
 	didSessionLoad = TRUE;
 	}
     else if (cartVarExists(cart, hgsDoLoadUrl))
 	{
 	char *url = cartString(cart, hgsLoadUrlName);
 	struct lineFile *lf = netLineFileOpen(url);
-	cartLoadSettings(lf, cart, oldVars, hgsDoLoadUrl);
+        struct dyString *dyMessage = dyStringNew(0);
+	boolean ok = cartLoadSettingsFromUserInput(lf, cart, oldVars, hgsDoLoadUrl, dyMessage);
 	lineFileClose(&lf);
 	cartTrace(cart, "after cartLS", conn);
-	didSessionLoad = TRUE;
+        if (! ok)
+            {
+            warn("Unable to load session file: %s", dyMessage->string);
+            }
+	didSessionLoad = ok;
+        dyStringFree(&dyMessage);
 	}
     }
 #endif /* GBROWSE */
 
 /* wire up the assembly hubs so we can operate without sql */
 setUdcTimeout(cart);
 if (cartVarExists(cart, hgHubDoDisconnect))
     doDisconnectHub(cart);
 
 if (didSessionLoad)
     cartCopyCustomComposites(cart);
 
 pushWarnHandler(cartHubWarn);
 char *newDatabase = hubConnectLoadHubs(cart);
 popWarnHandler();
@@ -1428,84 +1955,88 @@
 /* Make a radio button that is selected if cart variable exists and matches
  * value (or value matches default val if cart var doesn't exist). */
 {
 boolean matches = sameString(val, cartUsualString(cart, var, defaultVal));
 cgiMakeRadioButton(var, val, matches);
 }
 
 void cartSaveSession(struct cart *cart)
 /* Save session in a hidden variable. This needs to be called
  * somewhere inside of form or bad things will happen when user
  * has multiple windows open. */
 {
 cgiMakeHiddenVar(sessionVar, cartSessionId(cart));
 }
 
-static void cartDumpItem(struct hashEl *hel,boolean asTable, boolean doHtmlEncode)
-/* Dump one item in cart hash */
+static void cartDumpItem(struct hashEl *hel, boolean asTable, boolean encodeAsHtml)
+/* Dump one item in cart hash.  If encodeAsHtml, call htmlEncode on variable name and value;
+ * otherwise, call encodeForHgSession on value. */
 {
-char *var = hel->name;
-char *val = (char *)(hel->val);
-
-if (doHtmlEncode)
+char *var = hel->name, *val = NULL;
+if (encodeAsHtml)
     {
     var = htmlEncode(hel->name);
     val = htmlEncode((char *)(hel->val));
     }
+else
+    {
+    val = encodeForHgSession((char *)(hel->val));
+    }
 
 if (asTable)
     {
     printf("<TR><TD>%s</TD><TD>", var);
     int width=(strlen(val)+1)*8;
     if (width<100)
         width = 100;
     cgiMakeTextVarWithJs(hel->name, val, width,
                                 "change", "setCartVar(this.name,this.value);");
     printf("</TD></TR>\n");
     }
 else
     printf("%s %s\n", var, val);
-
-if (doHtmlEncode)
-    {
-    freeMem(var);
 freeMem(val);
-    }
+if (encodeAsHtml)
+    freeMem(var);
 }
 
-void cartDumpList(struct hashEl *elList,boolean asTable, boolean doHtmlEncode)
-/* Dump list of cart variables optionally as a table with ajax update support. */
+static void cartDumpList(struct hashEl *elList, boolean asTable, boolean encodeAsHtml)
+/* Dump list of cart variables optionally as a table with ajax update support.
+ * If encodeAsHtml, call htmlEncode on variable name and value; otherwise, call
+ * encodeForHgSession on value to prevent newlines from corrupting settings saved
+ * to a file. */
 {
 struct hashEl *el;
 
 if (elList == NULL)
     return;
 slSort(&elList, hashElCmp);
 if (asTable)
     printf("<table>\n");
 for (el = elList; el != NULL; el = el->next)
-    cartDumpItem(el, asTable, doHtmlEncode);
+    cartDumpItem(el, asTable, encodeAsHtml);
 if (asTable)
     {
     printf("<tr><td colspan=2>&nbsp;&nbsp;<em>count: %d</em></td></tr>\n",slCount(elList));
     printf("</table>\n");
     }
 hashElFreeList(&elList);
 }
 
-void cartDumpNoEncode(struct cart *cart)
-/* Dump contents of cart without HTML encoding. */
+void cartDumpHgSession(struct cart *cart)
+/* Dump contents of cart with escaped newlines for hgSession output files.
+ * Cart variable "cartDumpAsTable" is ignored. */
 {
 struct hashEl *elList = hashElListHash(cart->hash);
 cartDumpList(elList, FALSE, FALSE);
 }
 
 void cartDump(struct cart *cart)
 /* Dump contents of cart. */
 {
 struct hashEl *elList = hashElListHash(cart->hash);
 cartDumpList(elList,cartVarExists(cart,CART_DUMP_AS_TABLE), TRUE);
 }
 
 void cartDumpPrefix(struct cart *cart, char *prefix)
 /* Dump all cart variables with prefix */
 {