Fix Update endpoint (#260)

Multiple state transitions in the Update endpoint were broken. This
fixes those transitions and deprecates the Rejected status in favor of
Removed.

api/api_test.go

@@ -135,6 +135,7 @@ func TestAPI(t *testing.T) {
 		{Name: "removal-preloaded-bulk-eligible.test", Mode: preloadlist.ForceHTTPS, IncludeSubDomains: true, Policy: preloadlist.Bulk18Weeks},
 		{Name: "removal-preloaded-not-bulk-eligible.test", Mode: preloadlist.ForceHTTPS, IncludeSubDomains: true, Policy: preloadlist.Test},
 		{Name: "removal-preloaded-bulk-ineligible.test", Mode: preloadlist.ForceHTTPS, IncludeSubDomains: true, Policy: preloadlist.BulkLegacy},
+		{Name: "pending-automated-removal.test", Mode: preloadlist.ForceHTTPS, IncludeSubDomains: true, Policy: preloadlist.Test},
 		{Name: "godoc.og", Mode: "", IncludeSubDomains: true},
 		{Name: "dev", Mode: preloadlist.ForceHTTPS, IncludeSubDomains: true},
 	}}
@@ -221,7 +222,7 @@ func TestAPI(t *testing.T) {
 		{"update database failure", data1, failDatabase, api.Update, "GET", "",
 			500, textContentType, wantBody{text: "Internal error: could not retrieve domain names previously marked as preloaded. (forced failure)\n\n"}},
 		{"update success", data1, failNone, api.Update, "GET", "",
-			200, textContentType, wantBody{text: "The preload list has 7 entries.\n- # of preloaded HSTS entries: 6\n- # to be added in this update: 6\n- # to be removed this update: 0\n- # to be self-rejected this update: 0\nSuccess. 6 domain states updated.\n"}},
+			200, textContentType, wantBody{text: "The preload list has 8 entries.\n- # to be added in this update: 6\n- # to be updated in this update: 0\n- # to be removed this update: 0\nSuccess. 6 domain states updated.\n"}},
 		{"pending 3", data1, failNone, api.Pending, "GET", "",
 			200, jsonContentType, wantBody{text: "[\n]\n"}},
 
@@ -332,7 +333,7 @@ func TestAPI(t *testing.T) {
 
 		// update with removal
 		{"update with removal", data2, failNone, api.Update, "GET", "",
-			200, textContentType, wantBody{text: "The preload list has 2 entries.\n- # of preloaded HSTS entries: 1\n- # to be added in this update: 0\n- # to be removed this update: 5\n- # to be self-rejected this update: 2\nSuccess. 7 domain states updated.\n"}},
+			200, textContentType, wantBody{text: "The preload list has 2 entries.\n- # to be added in this update: 0\n- # to be updated in this update: 0\n- # to be removed this update: 7\nSuccess. 7 domain states updated.\n"}},
 		{"garron.net after update with removal", data2, failNone, api.Status, "GET", "?domain=garron.net",
 			200, jsonContentType, wantBody{state: &database.DomainState{
 				Name: "garron.net", Status: database.StatusRemoved}}},
@@ -371,7 +372,7 @@ func TestAPI(t *testing.T) {
 		if tt.wantBody.text != "" {
 			text := w.Body.String()
 			if text != tt.wantBody.text {
-				t.Errorf("[%s] Body text does not match wanted: %#v", tt.description, text)
+				t.Errorf("[%s] Wanted body text %q, got %q", tt.description, tt.wantBody.text, text)
 			}
 		}
 

api/update.go

@@ -8,21 +8,6 @@ import (
 	"github.com/chromium/hstspreload/chromium/preloadlist"
 )
 
-func difference(from []preloadlist.Entry, take []preloadlist.Entry) (diff []preloadlist.Entry) {
-	takeSet := make(map[string]bool)
-	for _, elem := range take {
-		takeSet[elem.Name] = true
-	}
-
-	for _, elem := range from {
-		if !takeSet[elem.Name] {
-			diff = append(diff, elem)
-		}
-	}
-
-	return diff
-}
-
 // Update tells the server to update pending/removed entries based
 // on the HSTS preload list source.
 //
@@ -40,73 +25,74 @@ func (api API) Update(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, msg, http.StatusInternalServerError)
 		return
 	}
-	var actualPreload []preloadlist.Entry
-	for _, entry := range preloadList.Entries {
-		if entry.Mode == preloadlist.ForceHTTPS {
-			actualPreload = append(actualPreload, entry)
-		}
-	}
 
-	// Get domains currently recorded as preloaded.
-	preloadedDomains, dbErr := api.database.StatesWithStatus(database.StatusPreloaded)
-	if dbErr != nil {
-		msg := fmt.Sprintf(
-			"Internal error: could not retrieve domain names previously marked as preloaded. (%s)\n",
-			dbErr,
-		)
-		http.Error(w, msg, http.StatusInternalServerError)
-		return
-	}
-	var databasePreload []preloadlist.Entry
-	for _, ds := range preloadedDomains {
-		databasePreload = append(databasePreload, ds.ToEntry())
+	domainStates := make(map[string]database.DomainState)
+	addDomainStatesWithStatus := func(status database.PreloadStatus) bool {
+		domains, err := api.database.StatesWithStatus(status)
+		if err != nil {
+			msg := fmt.Sprintf("Internal error: could not retrieve domain names previously marked as %s. (%s)\n", status, err)
+			http.Error(w, msg, http.StatusInternalServerError)
+			return false
+		}
+		for _, domain := range domains {
+			domainStates[domain.Name] = domain
+		}
+		return true
 	}
-
-	// Get domains currently recorded as pending removal.
-	pendingRemovalDomains, dbErr := api.database.StatesWithStatus(database.StatusPendingRemoval)
-	if dbErr != nil {
-		msg := fmt.Sprintf(
-			"Internal error: could not retrieve domain names previously marked as pending removal. (%s)\n",
-			dbErr,
-		)
-		http.Error(w, msg, http.StatusInternalServerError)
+	// Get domains currently recorded as preloaded, pending removal, or
+	// pending automated removal.
+	if !addDomainStatesWithStatus(database.StatusPreloaded) ||
+		!addDomainStatesWithStatus(database.StatusPendingRemoval) ||
+		!addDomainStatesWithStatus(database.StatusPendingAutomatedRemoval) {
 		return
 	}
-	var databasePendingRemoval []preloadlist.Entry
-	for _, ds := range pendingRemovalDomains {
-		databasePendingRemoval = append(databasePendingRemoval, ds.ToEntry())
-	}
 
-	// Calculate values that are out of date.
 	var updates []database.DomainState
-
-	added := difference(difference(actualPreload, databasePreload), databasePendingRemoval)
-	for _, entry := range added {
-		updates = append(updates, database.EntryToDomainState(entry, database.StatusPreloaded))
-	}
-
-	removed := difference(databasePreload, actualPreload)
-	for _, entry := range removed {
-		updates = append(updates, database.EntryToDomainState(entry, database.StatusRemoved))
+	added := 0
+	updated := 0
+	removed := 0
+	for _, entry := range preloadList.Entries {
+		if entry.Mode != preloadlist.ForceHTTPS {
+			continue
+		}
+		domainState, found := domainStates[entry.Name]
+		if !found {
+			// entry is on the preload list but not marked as
+			// preloaded, pending removal, or pending automated
+			// removal in the database. Mark it as preloaded in the
+			// database.
+			updates = append(updates, database.EntryToDomainState(entry, database.StatusPreloaded))
+			added++
+			continue
+		}
+		delete(domainStates, entry.Name)
+		// entry is in both the preload list and in one of the states of
+		// preloaded, pending removal, or pending automated removal. If
+		// the preload list entry differs from what's in the database,
+		// update the database to match.
+		if domainState.ToEntry().Equal(entry) {
+			continue
+		}
+		domainState.Policy = entry.Policy
+		domainState.IncludeSubDomains = entry.IncludeSubDomains
+		updates = append(updates, domainState)
+		updated++
 	}
-
-	selfRejected := difference(databasePendingRemoval, actualPreload)
-	for _, entry := range selfRejected {
-		updates = append(updates, database.EntryToDomainStateWithMessage(entry, database.StatusRejected, "Domain was added and removed without being preloaded."))
+	// domainStates now only contains domains that aren't on the preload
+	// list. Update their state in the database to mark them as removed.
+	for _, domainState := range domainStates {
+		domainState.Status = database.StatusRemoved
+		domainState.Policy = preloadlist.UnspecifiedPolicyType
+		updates = append(updates, domainState)
+		removed++
 	}
 
 	fmt.Fprintf(w, `The preload list has %d entries.
-- # of preloaded HSTS entries: %d
 - # to be added in this update: %d
+- # to be updated in this update: %d
 - # to be removed this update: %d
-- # to be self-rejected this update: %d
 `,
-		len(preloadList.Entries),
-		len(actualPreload),
-		len(added),
-		len(removed),
-		len(selfRejected),
-	)
+		len(preloadList.Entries), added, updated, removed)
 
 	// Create log function to show progress.
 	written := false

api/update_test.go

@@ -118,8 +118,6 @@ func TestUpdate(t *testing.T) {
 					Name: "preloaded-custom.test",
 					Status: database.StatusRemoved,
 					IncludeSubDomains: true,
-					// TODO: the policy field should get cleared for removed domains.
-					Policy: preloadlist.Custom,
 				},
 			},
 		},
@@ -136,9 +134,7 @@ func TestUpdate(t *testing.T) {
 			[]database.DomainState{
 				{
 					Name: "preloaded.test",
-					Status: database.StatusRejected,
-					// TODO: This state transition should not have this message.
-					Message: "Domain was added and removed without being preloaded.",
+					Status: database.StatusRemoved,
 					IncludeSubDomains: true,
 				},
 			},
@@ -156,8 +152,7 @@ func TestUpdate(t *testing.T) {
 			[]database.DomainState{
 				{
 					Name: "preloaded.test",
-					// TODO: This should be StatusRejected.
-					Status: database.StatusPendingAutomatedRemoval,
+					Status: database.StatusRemoved,
 					IncludeSubDomains: true,
 				},
 			},
@@ -201,8 +196,7 @@ func TestUpdate(t *testing.T) {
 				},
 				{
 					Name: "pending-automated-removal.test",
-					// TODO: This should be StatusPendingAutomatedRemoval
-					Status: database.StatusPreloaded,
+					Status: database.StatusPendingAutomatedRemoval,
 					IncludeSubDomains: true,
 					Policy: preloadlist.Bulk1Year,
 				},
@@ -232,9 +226,7 @@ func TestUpdate(t *testing.T) {
 					Name: "preloaded.test",
 					Status: database.StatusPreloaded,
 					IncludeSubDomains: true,
-					// TODO: Uncomment the following line when
-					// the functionality under test is fixed.
-					// Policy: preloadlist.Bulk1Year,
+					Policy: preloadlist.Bulk1Year,
 				},
 			},
 		},

database/domainstate.go

@@ -18,6 +18,7 @@ const (
 	StatusUnknown                 = "unknown"
 	StatusPending                 = "pending"
 	StatusPreloaded               = "preloaded"
+	// StatusRejected is deprecated. StatusRemoved should be used instead.
 	StatusRejected                = "rejected"
 	StatusRemoved                 = "removed"
 	StatusPendingRemoval          = "pending-removal"
@@ -83,7 +84,7 @@ func (s DomainState) Equal(s2 DomainState) bool {
 // Only the name, preload status, include subdomains boolean and policy is preserved during the conversion.
 func (s DomainState) ToEntry() preloadlist.Entry {
 	mode := preloadlist.ForceHTTPS
-	if s.Status != StatusPreloaded {
+	if s.Status != StatusPreloaded && s.Status != StatusPendingRemoval && s.Status != StatusPendingAutomatedRemoval {
 		mode = ""
 	}
 	return preloadlist.Entry{Name: s.Name, Mode: mode, IncludeSubDomains: s.IncludeSubDomains, Policy: s.Policy}

go.mod

@@ -4,7 +4,7 @@ go 1.15
 
 require (
 	cloud.google.com/go/datastore v1.15.0
-	github.com/chromium/hstspreload v0.0.0-20230717215815-2404b4b967f9
+	github.com/chromium/hstspreload v0.0.0-20240911231609-cc9e2f8f2623
 	golang.org/x/net v0.23.0
 	google.golang.org/api v0.149.0
 	google.golang.org/grpc v1.59.0

go.sum

@@ -949,6 +949,8 @@ github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XL
 github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
 github.com/chromium/hstspreload v0.0.0-20230717215815-2404b4b967f9 h1:lUjpcxlnalltzHmS0ljbLvTVNyn+D9SlaVkFzNKvE6o=
 github.com/chromium/hstspreload v0.0.0-20230717215815-2404b4b967f9/go.mod h1:wnaDPQdV/ehNVGxLu9m5/NWnVHEizalu4/Z47anr3mg=
+github.com/chromium/hstspreload v0.0.0-20240911231609-cc9e2f8f2623 h1:XEjUCFrFn3Xml+OHwg+rmRJ2C7PoBtT9BmvRpdiukpw=
+github.com/chromium/hstspreload v0.0.0-20240911231609-cc9e2f8f2623/go.mod h1:aL/chD5x+u+u7eA4Ql7XCp8K2bAoy4qEiR7pgdWSDJk=
 github.com/chzyer/logex v1.1.10/go.mod h1:+Ywpsq7O8HXn0nuIou7OrIPyXbp3wmkHB+jjWRnGsAI=
 github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e/go.mod h1:nSuG5e5PlCu98SY8svDHJxuZscDgtXS6KTTbou5AhLI=
 github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1/go.mod h1:Q3SI9o4m/ZMnBNeIyt5eFwwo7qiLfzFZmjNmxjkiQlU=