d338d5080783d2ac5828658fe17160668bf64cdb chmalee Thu May 7 15:05:24 2026 -0700 Fixes from code review, refs #37500 - Revalidate shared myVariants tracks against hgcentral on every read path (hgTracks, hgc, hgTables); cart-supplied owner/db/project no longer trusted. New myVariantsResolveSharedTrack helper. - Scope shared-track UPDATE statements by share->project/db so a recipient can't edit rows outside the granted scope. - Add hgsid CSRF check to myVariantsJsCommand; pass hgsid in the hgTracks.js highlight Add-Annotation POST. - HTML-escape owner-controlled fields in the canEdit branch of doMyVariantsDetails (Chromosome, Project, project select options, hidden text input). - Validate targetUser against gbMembers when creating a share; return a clear 400 on typos. - Replace the concat(id,' ',name)='%s' lookup with parsed-id + name verification. - Remove cgiMakeColorVar / cgiMakeColorVarWithLabel; the canEdit form uses spectrum.js (already loaded for the create dialog). - Strip _hidden_* columns from hgTables field lists for shared tracks, both the display path and the selected-fields read path. - Make the per-assembly invariant explicit: myVariantsLoadItems and doMyVariantsDetails bail out if share->db != current database. - Memoize myVariantsSharedScopeWhere to avoid per-region hgcentral round-trips on genome-wide hgTables queries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> diff --git src/hg/hgTracks/myVariantsTrack.c src/hg/hgTracks/myVariantsTrack.c index 2ffee273a38..31cf394a06a 100644 --- src/hg/hgTracks/myVariantsTrack.c +++ src/hg/hgTracks/myVariantsTrack.c @@ -57,30 +57,37 @@ return TRUE; } void myVariantsJsCommand(char *command, struct track *trackList, struct hash *trackHash) /* Execute some command sent to us from the javaScript. All we know for sure is that * the first word of the command is "myVariants." We expect it to be of format: * myVariants <trackName> <jsonData> * where jsonData is a JSON string containing the item details */ { if (!cfgOptionBooleanDefault("doMyVariants", FALSE)) return; char *userName = getUserName(); if (userName == NULL) return; +/* This command runs mysql commands so require the hgsids match to prevent CSRF. */ +char *suppliedHgsid = cgiOptionalString("hgsid"); +char *expectedHgsid = cartSessionId(cart); +if (isEmpty(suppliedHgsid) || isEmpty(expectedHgsid) + || !sameString(suppliedHgsid, expectedHgsid)) + errAbort("session token missing or invalid"); + /* Parse out command into local variables. */ char *words[3]; char *dupeCommand = cloneString(command); /* For parsing. */ int wordCount = chopByWhiteRespectDoubleQuotes(dupeCommand, words, ArraySize(words)); if (wordCount != 3) errAbort("Expecting %d words in jsCommand '%s'", wordCount, command); char *jsonData = words[2]; /* Parse JSON data */ struct jsonElement *json = jsonParse(jsonData); if (json == NULL) errAbort("Invalid JSON data in command: %s", jsonData); /* Handle hideField command: rename column with _hidden_ prefix */ char *hideFieldName = jsonOptionalStringField(json, "hideField", NULL); @@ -280,53 +287,63 @@ item->chrom, item->chromStart, item->chromEnd); jsInlineF("// END newItemPos\n"); hFreeConn(&conn); freez(&dupeCommand); } static int myVariantsExtraHeight(struct track *track) /* Return extra height of track. */ { return tl.fontHeight+2; } static void updateTextField(char *trackName, struct sqlConnection *conn, - char *tableName, char *fieldName, int id) -/* Update text valued field with new val. */ + char *tableName, char *fieldName, int id, char *scopeProject, char *scopeDb) +/* Update text valued field with new val. If scopeProject/scopeDb are non-NULL, + * include them in the WHERE clause so a recipient cannot edit rows outside + * the share's authorized project/db. scopeProject of "*" means all projects. */ { char varName[128]; safef(varName, sizeof(varName), "%s_%s", trackName, fieldName); char *newVal = cartOptionalString(cart, varName); if (newVal == NULL) return; if (endsWith(varName, "itemRgb")) { unsigned color; if (htmlColorForCode(newVal, &color)) { - char sql[256]; - sqlSafef(sql, sizeof(sql), "update %s set %s='%d' where id=%d", - tableName, fieldName, color, id); - sqlUpdate(conn, sql); + struct dyString *sql = sqlDyStringCreate( + "update %s set %s='%d' where id=%d", tableName, fieldName, color, id); + if (isNotEmpty(scopeDb)) + sqlDyStringPrintf(sql, " and db='%s'", scopeDb); + if (isNotEmpty(scopeProject) && !sameString(scopeProject, "*")) + sqlDyStringPrintf(sql, " and project='%s'", scopeProject); + sqlUpdate(conn, sql->string); + dyStringFree(&sql); cartRemove(cart, varName); } return; } struct dyString *sql = sqlDyStringCreate("update %s set %s='%s' where id=%d", tableName, fieldName, newVal, id); +if (isNotEmpty(scopeDb)) + sqlDyStringPrintf(sql, " and db='%s'", scopeDb); +if (isNotEmpty(scopeProject) && !sameString(scopeProject, "*")) + sqlDyStringPrintf(sql, " and project='%s'", scopeProject); sqlUpdate(conn, sql->string); dyStringFree(&sql); cartRemove(cart, varName); } static void myVariantsEditOrDelete(char *trackName, struct sqlConnection *conn, char *tableName) /* Troll through cart variables looking for things that indicate user edited item * or deleted it in hgc, and carry out edits. See hgc/myVariantsClick.c. */ { char varName[128]; char sql[256]; safef(varName, sizeof(varName), "%s_%s", trackName, "id"); char *idString = cartOptionalString(cart, varName); if (idString != NULL) { @@ -353,47 +370,47 @@ char *userName = getUserName(); if (userName) { char *ctFile = myVariantsWriteCtFile(userName, database, cart); if (isNotEmpty(ctFile)) { char ctVar[256]; safef(ctVar, sizeof ctVar, CT_FILE_VAR_PREFIX "%s", database); cartSetString(cart, ctVar, ctFile); freeMem(ctFile); } } return; } - /* Handle edits. */ - updateTextField(trackName, conn, tableName, "name", id); - updateTextField(trackName, conn, tableName, "description", id); - updateTextField(trackName, conn, tableName, "itemRgb", id); - updateTextField(trackName, conn, tableName, "chromStart", id); - updateTextField(trackName, conn, tableName, "chromEnd", id); - updateTextField(trackName, conn, tableName, "ref", id); - updateTextField(trackName, conn, tableName, "alt", id); - updateTextField(trackName, conn, tableName, "project", id); - updateTextField(trackName, conn, tableName, "mouseover", id); + /* Handle edits. Owner edits their own table, no project/db scope filter. */ + updateTextField(trackName, conn, tableName, "name", id, NULL, NULL); + updateTextField(trackName, conn, tableName, "description", id, NULL, NULL); + updateTextField(trackName, conn, tableName, "itemRgb", id, NULL, NULL); + updateTextField(trackName, conn, tableName, "chromStart", id, NULL, NULL); + updateTextField(trackName, conn, tableName, "chromEnd", id, NULL, NULL); + updateTextField(trackName, conn, tableName, "ref", id, NULL, NULL); + updateTextField(trackName, conn, tableName, "alt", id, NULL, NULL); + updateTextField(trackName, conn, tableName, "project", id, NULL, NULL); + updateTextField(trackName, conn, tableName, "mouseover", id, NULL, NULL); /* Update any custom fields */ { char *editUserName = getUserName(); struct slName *customCols = myVariantsGetCustomFields(editUserName); struct slName *col; for (col = customCols; col != NULL; col = col->next) - updateTextField(trackName, conn, tableName, col->name, id); + updateTextField(trackName, conn, tableName, col->name, id, NULL, NULL); slFreeList(&customCols); } /* Trigger CT refresh after edits */ { char *userName = getUserName(); if (userName) { char *ctFile = myVariantsWriteCtFile(userName, database, cart); if (isNotEmpty(ctFile)) { char ctVar[256]; safef(ctVar, sizeof ctVar, CT_FILE_VAR_PREFIX "%s", database); cartSetString(cart, ctVar, ctFile); freeMem(ctFile); } @@ -452,59 +469,55 @@ } sqlFreeResult(&sr); dyStringFree(&mouseover); return lfList; } void myVariantsLoadItems(struct track *track) /* Load up items in track already. Also make up a pseudo-item that is * where you drag to create an item. */ { struct linkedFeatures *lfList = NULL; boolean isShared = startsWith("myVariants_shared_", track->track); if (isShared) { - /* Extract the token and look up the share info from the cart */ - char *token = track->track + strlen("myVariants_shared_"); - char cartVar[256]; - safef(cartVar, sizeof(cartVar), MYVAR_SHARED_CART_PREFIX "%s", token); - char *cartVal = cartOptionalString(cart, cartVar); - if (isEmpty(cartVal)) + /* Resolve via hgcentral so revoked/downgraded shares stop returning data. */ + struct myVariantsShare *share = myVariantsResolveSharedTrack(track->track, cart); + if (share == NULL) return; - char *owner = NULL, *project = NULL, *db = NULL; - int permission = 0; - if (!myVariantsParseShareCartValue(cartVal, &owner, &project, &db, &permission, NULL)) + /* Shared tracks are per-assembly; only load when the browser's current + * db matches the share's db. */ + if (!sameString(share->db, database)) + { + myVariantsShareFree(&share); return; - char *tableName = myVariantsGetDbTable(owner); + } + char *tableName = myVariantsGetDbTable(share->ownerUser); if (isEmpty(tableName)) { - freeMem(owner); - freeMem(project); - freeMem(db); + myVariantsShareFree(&share); return; } struct sqlConnection *conn = hAllocConn(CUSTOM_TRASH); - struct dyString *whereClause = sqlDyStringCreate("db='%s'", database); - if (!sameString(project, "*")) - sqlDyStringPrintf(whereClause, " and project='%s'", project); + struct dyString *whereClause = sqlDyStringCreate("db='%s'", share->db); + if (!sameString(share->project, "*")) + sqlDyStringPrintf(whereClause, " and project='%s'", share->project); lfList = loadMyVariantsItems(conn, tableName, track->tdb, dyStringCannibalize(&whereClause)); hFreeConn(&conn); - freeMem(owner); - freeMem(project); - freeMem(db); + myVariantsShareFree(&share); } else { /* Load user's own items */ char *userName = getUserName(); char *db = myVariantsGetDatabaseForUser(userName); if (!db) return; char *tableName = myVariantsGetDbTable(userName); struct sqlConnection *conn = hAllocConn(CUSTOM_TRASH); myVariantsEditOrDelete(track->track, conn, tableName); struct dyString *whereClause = sqlDyStringCreate("db='%s'", database); lfList = loadMyVariantsItems(conn, tableName, track->tdb, dyStringCannibalize(&whereClause)); hFreeConn(&conn); @@ -665,30 +678,39 @@ if (strlen(project) > 200 || (targetUser && strlen(targetUser) > 200) || (label && strlen(label) > 200)) apiError(400, "project, targetUser, and label must each be <= 200 chars"); /* Project must be "*" (all) or one of the owner's existing project values. * Prevents creating misleading share URLs that filter on a non-existent * project name. */ if (!sameString(project, "*")) { struct slName *projects = myVariantsGetProjects(userName); boolean found = slNameInList(projects, project); slFreeList(&projects); if (!found) apiError(400, "project does not exist for current user"); } + if (isNotEmpty(targetUser)) + { + char gbq[512]; + sqlSafef(gbq, sizeof(gbq), + "select count(*) from gbMembers where userName='%s'", targetUser); + if (sqlQuickNum(conn, gbq) == 0) + apiError(400, "targetUser not found - check the spelling, or omit" + " targetUser to make an 'anyone with link' share"); + } struct myVariantsShare *share = myVariantsCreateShare(conn, userName, project, db, permission, targetUser, label); struct jsonWrite *jw = jsonWriteNew(); jsonWriteObjectStart(jw, NULL); jsonWriteString(jw, "status", "ok"); jsonWriteString(jw, "token", share->shareToken); jsonWriteNumber(jw, "id", share->id); jsonWriteObjectEnd(jw); myVariantsShareFree(&share); hDisconnectCentral(&conn); apiSuccess(jw); } else if (sameString(action, "getShares")) { struct myVariantsShare *shares = myVariantsGetSharesForOwner(conn, userName, db); @@ -813,89 +835,73 @@ struct hashEl *shareVars = cartFindPrefix(cart, MYVAR_SHARED_CART_PREFIX); struct hashEl *el; for (el = shareVars; el != NULL; el = el->next) { char *token = el->name + strlen(MYVAR_SHARED_CART_PREFIX); char trackName[512]; safef(trackName, sizeof(trackName), "myVariants_shared_%s", token); /* Check if there's a pending edit for this shared track */ char varName[256]; safef(varName, sizeof(varName), "%s_%s", trackName, "id"); char *idString = cartOptionalString(cart, varName); if (idString == NULL) continue; - /* Re-validate the share against hgcentral on every edit. The cart-cached - * permission is not authoritative: the owner may have revoked the share - * or downgraded it from read-write to read-only since the recipient's - * cart was last set, and we don't want stale cart contents to grant - * writes that the live share record no longer permits. */ - struct sqlConnection *centralConn = hConnectCentral(); - if (!sqlTableExists(centralConn, "myVariantsShares")) - { - hDisconnectCentral(¢ralConn); - continue; - } - struct myVariantsShare *liveShare = myVariantsGetShareByToken(centralConn, token); - hDisconnectCentral(¢ralConn); + /* Revalidate via hgcentral so revoked/downgraded shares can't write. */ + struct myVariantsShare *liveShare = myVariantsResolveSharedTrack(trackName, cart); if (liveShare == NULL) continue; if (liveShare->permission != MYVAR_PERM_READWRITE) { myVariantsShareFree(&liveShare); continue; } - if (isNotEmpty(liveShare->targetUser) && !sameString(liveShare->targetUser, userName)) - { - myVariantsShareFree(&liveShare); - continue; - } - char *owner = cloneString(liveShare->ownerUser); - myVariantsShareFree(&liveShare); - - char *tableName = myVariantsGetDbTable(owner); + char *tableName = myVariantsGetDbTable(liveShare->ownerUser); if (isEmpty(tableName)) { - freeMem(owner); + myVariantsShareFree(&liveShare); continue; } int id = sqlUnsigned(idString); cartRemove(cart, varName); /* Handle cancel */ safef(varName, sizeof(varName), "%s_%s", trackName, "cancel"); if (cartVarExists(cart, varName)) { cartRemove(cart, varName); - freeMem(owner); + myVariantsShareFree(&liveShare); continue; } - /* Apply field edits - no delete, no project change */ + /* Apply field edits, scoped to the share's project/db so a recipient + * cannot edit rows in projects/assemblies they were not granted. */ + char *sp = liveShare->project; + char *sd = liveShare->db; struct sqlConnection *conn = hAllocConn(CUSTOM_TRASH); - updateTextField(trackName, conn, tableName, "name", id); - updateTextField(trackName, conn, tableName, "description", id); - updateTextField(trackName, conn, tableName, "itemRgb", id); - updateTextField(trackName, conn, tableName, "chromStart", id); - updateTextField(trackName, conn, tableName, "chromEnd", id); - updateTextField(trackName, conn, tableName, "ref", id); - updateTextField(trackName, conn, tableName, "alt", id); - updateTextField(trackName, conn, tableName, "mouseover", id); - struct slName *customCols = myVariantsGetCustomFields(owner); + updateTextField(trackName, conn, tableName, "name", id, sp, sd); + updateTextField(trackName, conn, tableName, "description", id, sp, sd); + updateTextField(trackName, conn, tableName, "itemRgb", id, sp, sd); + updateTextField(trackName, conn, tableName, "chromStart", id, sp, sd); + updateTextField(trackName, conn, tableName, "chromEnd", id, sp, sd); + updateTextField(trackName, conn, tableName, "ref", id, sp, sd); + updateTextField(trackName, conn, tableName, "alt", id, sp, sd); + updateTextField(trackName, conn, tableName, "mouseover", id, sp, sd); + struct slName *customCols = myVariantsGetCustomFields(liveShare->ownerUser); struct slName *col; for (col = customCols; col != NULL; col = col->next) - updateTextField(trackName, conn, tableName, col->name, id); + updateTextField(trackName, conn, tableName, col->name, id, sp, sd); slFreeList(&customCols); hFreeConn(&conn); - freeMem(owner); + myVariantsShareFree(&liveShare); } hashElFreeList(&shareVars); } boolean myVariantsTrackEnabled() /* Return TRUE if the "My Variants" feature is enabled in hg.conf * and the current request comes from a valid user */ { return cfgOptionBooleanDefault("doMyVariants", FALSE) && (getUserName() != NULL); }