amadan

carlos / platform Public

console: pending-change refusals are sentinel errors, not substrings

refuseConfirm matched ConfirmChange's refusals by grepping their message
text ("no such pending change", "has expired", "already been executed"),
so rewording any of the three would silently degrade a 404, a 410 or a
409 into the 500 fallback — "could not record your confirmation, please
try again" told to somebody about a change that can never succeed, with
no test anywhere to notice.

The three refusals become exported sentinel values wrapped with the
change id at the raise site, and the mapping is errors.Is. Same statuses,
same wording on the wire; the mapping is now pinned to values a test can
hold, including one whose message is deliberately nothing like the
store's.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell da39bafc9829840da9bcda4ca9b011eab514bb53 parent ce6ca58
3 files changed, +97 −7
  • internal/console/pending.go +19 −3
  • internal/console/pending_test.go +69 −0
  • internal/console/pendinghandlers.go +9 −4
diff --git a/internal/console/pending.go b/internal/console/pending.go
index db10fd140..c0a60f3e1 100644
--- a/internal/console/pending.go
+++ b/internal/console/pending.go
@@ -19,6 +19,7 @@ import (
"crypto/rand"
"database/sql"
"encoding/hex"
+ "errors"
"fmt"
"time"
)
@@ -27,6 +28,21 @@ import (
// it can no longer be confirmed or executed.
const PendingChangeTTL = 7 * 24 * time.Hour
+// ConfirmChange's three refusals, as values. The console maps them onto the
+// wire (refuseConfirm: 404 / 410 / 409), and it does so with errors.Is — the
+// substring matching this replaced meant a reworded message silently became a
+// 500 telling a member to "try again" about a change that can never succeed.
+// A refusal that has to be recognised by another package's grep is not a
+// contract; these are.
+//
+// Each is wrapped with the change's id at the raise site, so the log and the
+// operator keep the detail while callers keep the value.
+var (
+ ErrNoPendingChange = errors.New("console: no such pending change")
+ ErrPendingChangeExpired = errors.New("console: pending change has expired")
+ ErrPendingChangeExecuted = errors.New("console: pending change has already been executed")
+)
+
// PendingChange is one proposed, quorum-gated action.
type PendingChange struct {
ID string
@@ -152,13 +168,13 @@ func (s *Store) ConfirmChange(id, address string, now time.Time) (*PendingChange
return nil, false, err
}
if pc == nil {
- return nil, false, fmt.Errorf("console: no such pending change %q", id)
+ return nil, false, fmt.Errorf("%w %q", ErrNoPendingChange, id)
}
if now.Unix() >= pc.ExpiresAt.Unix() {
- return nil, false, fmt.Errorf("console: pending change %q has expired", id)
+ return nil, false, fmt.Errorf("%w: %q", ErrPendingChangeExpired, id)
}
if !executedAtZero(tx, id) {
- return nil, false, fmt.Errorf("console: pending change %q has already been executed", id)
+ return nil, false, fmt.Errorf("%w: %q", ErrPendingChangeExecuted, id)
}
if _, err := tx.Exec(`INSERT OR IGNORE INTO pending_confirms (change_id, address, confirmed_at) VALUES (?, ?, ?)`,
id, address, now.Unix()); err != nil {
diff --git a/internal/console/pending_test.go b/internal/console/pending_test.go
index 5c5ddcb4d..ca29f6189 100644
--- a/internal/console/pending_test.go
+++ b/internal/console/pending_test.go
@@ -1,6 +1,10 @@
package console
import (
+ "errors"
+ "fmt"
+ "net/http"
+ "net/http/httptest"
"reflect"
"sort"
"strings"
@@ -183,3 +187,68 @@ func TestPendingChangeExpiryAndExecution(t *testing.T) {
t.Fatalf("PendingChange after DeleteChange = %v, %v, want nil, nil", got, err)
}
}
+
+// TestConfirmChangeRefusalsAreSentinels: ConfirmChange's three refusals are
+// values a caller can test for, not prose it has to grep. The HTTP mapping
+// (refuseConfirm) turns them into 404 / 410 / 409, and a reworded message
+// must not be able to silently downgrade any of those to a 500 that tells a
+// member to "try again" about something that can never succeed.
+func TestConfirmChangeRefusalsAreSentinels(t *testing.T) {
+ s := openTestStore(t)
+ now := time.Now()
+ accountID := testAccountID()
+
+ if _, _, err := s.ConfirmChange("nosuchid", "paul@keymail.dev", now); !errors.Is(err, ErrNoPendingChange) {
+ t.Errorf("ConfirmChange on an unknown id = %v, want ErrNoPendingChange", err)
+ }
+
+ expiring, err := s.ProposeChange(accountID, "kass", "promote", []byte(`{}`), 2, "paul@keymail.dev", true, now)
+ if err != nil {
+ t.Fatal(err)
+ }
+ _, _, err = s.ConfirmChange(expiring.ID, "bob@keymail.dev", expiring.ExpiresAt.Add(time.Second))
+ if !errors.Is(err, ErrPendingChangeExpired) {
+ t.Errorf("ConfirmChange past expiry = %v, want ErrPendingChangeExpired", err)
+ }
+ // The id stays in the message: a sentinel is for the caller, the text is
+ // for the log.
+ if err == nil || !strings.Contains(err.Error(), expiring.ID) {
+ t.Errorf("expiry error %v does not name the change", err)
+ }
+
+ done, err := s.ProposeChange(accountID, "kass", "promote", []byte(`{}`), 2, "paul@keymail.dev", true, now)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := s.MarkExecuted(done.ID, now); err != nil {
+ t.Fatal(err)
+ }
+ if _, _, err := s.ConfirmChange(done.ID, "bob@keymail.dev", now); !errors.Is(err, ErrPendingChangeExecuted) {
+ t.Errorf("ConfirmChange on an executed change = %v, want ErrPendingChangeExecuted", err)
+ }
+}
+
+// TestRefuseConfirmMapsSentinels: the status each sentinel earns, pinned
+// against errors.Is rather than the wording — the wording here is deliberately
+// nothing like the store's, which is exactly the case substring matching used
+// to get wrong.
+func TestRefuseConfirmMapsSentinels(t *testing.T) {
+ fx := newConsoleFixture(t)
+ for _, tc := range []struct {
+ name string
+ err error
+ want int
+ }{
+ {"unknown", fmt.Errorf("wrapped in something else entirely: %w", ErrNoPendingChange), http.StatusNotFound},
+ {"expired", fmt.Errorf("wrapped in something else entirely: %w", ErrPendingChangeExpired), http.StatusGone},
+ {"executed", fmt.Errorf("wrapped in something else entirely: %w", ErrPendingChangeExecuted), http.StatusConflict},
+ {"other", errors.New("database is locked"), http.StatusInternalServerError},
+ } {
+ rec := httptest.NewRecorder()
+ req := httptest.NewRequest(http.MethodPost, "/a/bab/apps/widget/pending/x/confirm", nil)
+ fx.srv.refuseConfirm(rec, req, tc.err)
+ if rec.Code != tc.want {
+ t.Errorf("refuseConfirm(%s) = %d, want %d", tc.name, rec.Code, tc.want)
+ }
+ }
+}
diff --git a/internal/console/pendinghandlers.go b/internal/console/pendinghandlers.go
index 69c56a180..bba238d84 100644
--- a/internal/console/pendinghandlers.go
+++ b/internal/console/pendinghandlers.go
@@ -636,15 +636,20 @@ func (s *Server) handlePendingConfirm(w http.ResponseWriter, r *http.Request) {
// refuseConfirm maps ConfirmChange's own refusals onto the wire. See
// handlePendingConfirm's doc comment for why this cannot be an oracle.
+//
+// errors.Is, never the message text: these are sentinel values (pending.go)
+// precisely so that rewording a refusal cannot silently turn a 410 or a 409
+// into the 500 fallback — "please try again" is the wrong thing to tell
+// somebody about a change that expired, and a wrong status is the kind of
+// regression no test catches unless the mapping is pinned to values.
func (s *Server) refuseConfirm(w http.ResponseWriter, r *http.Request, err error) {
- msg := err.Error()
switch {
- case strings.Contains(msg, "no such pending change"):
+ case errors.Is(err, ErrNoPendingChange):
http.NotFound(w, r)
- case strings.Contains(msg, "has expired"):
+ case errors.Is(err, ErrPendingChangeExpired):
s.renderMessage(w, r, http.StatusGone, "Expired",
"That change expired before it collected enough confirmations. Nothing has been changed — propose it again.")
- case strings.Contains(msg, "already been executed"):
+ case errors.Is(err, ErrPendingChangeExecuted):
s.renderMessage(w, r, http.StatusConflict, "Already done",
"That change was carried out while you were confirming it. Nothing else has been changed.")
default: