[refactor](alarm): make a readable message structural on every error

An error status with an empty body is a client-breaking bug that review does not reliably catch. Route every JSON and plain-text error through internal/apierr, which substitutes the code canonical message when a caller supplies none, and keep the frozen codes in one package shared by the action and WebSocket layers.
This commit is contained in:
2026-10-19 02:26:46 +01:00
parent 3ccea3e9f6
commit 028f9e7dae
5 changed files with 522 additions and 35 deletions
+14 -32
View File
@@ -31,7 +31,6 @@ import (
"time"
"git.dcntrlized.org/Dcntrlized/Alarm/db"
"git.dcntrlized.org/Dcntrlized/Alarm/internal/alertstate"
"git.dcntrlized.org/Dcntrlized/Alarm/internal/apierr"
"git.dcntrlized.org/dcntrlized/dcntrlized-auth/authclient"
"github.com/google/uuid"
@@ -40,15 +39,16 @@ import (
// Frozen v1 error codes, aliased from internal/apierr so the WebSocket layer
// (which cannot import this package) uses the same strings.
const (
CodeAlertNotFound = apierr.AlertNotFound
CodeAlertTokenInvalid = apierr.AlertTokenInvalid
CodeAlertNotAdmin = apierr.AlertNotAdmin
CodeAlertAlreadyJoined = apierr.AlertAlreadyJoined
CodeAlertNotSubscribed = apierr.AlertNotSubscribed
CodeAlertState = apierr.AlertState
CodeValidation = apierr.Validation
CodeUnauthorized = apierr.Unauthorized
CodeInternal = apierr.Internal
CodeAlertNotFound = apierr.AlertNotFound
CodeAlertTokenInvalid = apierr.AlertTokenInvalid
CodeAlertNotAdmin = apierr.AlertNotAdmin
CodeAlertAlreadyJoined = apierr.AlertAlreadyJoined
CodeAlertNotSubscribed = apierr.AlertNotSubscribed
CodeAlertState = apierr.AlertState
CodeAlarmTriggeredOffline = apierr.AlarmTriggeredOffline
CodeValidation = apierr.Validation
CodeUnauthorized = apierr.Unauthorized
CodeInternal = apierr.Internal
)
// maxMessageBytes / maxNameChars bound the free-text fields.
@@ -60,22 +60,11 @@ const (
maxPlatformLen = 32
)
// errorEnvelope is the platform error body.
type errorEnvelope struct {
Error struct {
Code string `json:"code"`
Message string `json:"message"`
} `json:"error"`
}
// writeError emits {"error":{"code","message"}}.
// writeError emits {"error":{"code","message"}}. It delegates to apierr.Write,
// which guarantees both fields are non-empty: this package can pass a literal
// message, but the invariant is enforced centrally (see internal/apierr).
func writeError(w http.ResponseWriter, status int, code, message string) {
var body errorEnvelope
body.Error.Code = code
body.Error.Message = message
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(status)
_ = json.NewEncoder(w).Encode(body)
apierr.Write(w, status, code, message)
}
// writeJSON emits a success body.
@@ -180,13 +169,6 @@ func validateMessage(raw string) (string, error) {
return raw, nil
}
func validateAutoStop(seconds int) error {
if seconds < 0 || seconds > alertstate.MaxAutoStopSeconds {
return fmt.Errorf("autoStopSeconds must be between 0 and %d", alertstate.MaxAutoStopSeconds)
}
return nil
}
func validateDeviceID(raw string) (string, error) {
id := strings.TrimSpace(raw)
if id == "" {
+191
View File
@@ -0,0 +1,191 @@
// Copyright (C) 2026 Dcntrlized
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public License as published
// by the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Affero General Public License for more details.
//
// You should have received a copy of the GNU Affero General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.
package actions
import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"
)
// The dispatch layer routes by verb and answers in plain text. This test walks
// every routed action and every dispatch-level rejection and asserts the
// invariant the user requires: an error is never a bare status line — every
// response carries a human-readable message, and every JSON error carries both
// "code" and "message".
//
// Requests carry a short deadline: the shared-auth proxy actions need a live
// dcntrlized-auth (which this gate does not assume), and the assertion is about
// the error body, not about the proxy hop. The deadline bounds the unreachable
// hop so the walk stays fast.
const dispatchTimeout = 600 * time.Millisecond
var (
alertActions = []string{
"createAlert", "getAlert", "joinAlert", "listSubscriptions",
"removeSubscription", "triggerAlert", "disarmAlert", "alarmStatus",
"ackAlarm", "deleteAlert", "qrImage",
}
authProxyActions = []string{"login", "register", "refresh", "delete", "logout"}
readOnlyActions = []string{"getAlert", "listSubscriptions", "alarmStatus", "qrImage", "logout", "me"}
deleteActions = []string{"deleteAlert", "removeSubscription"}
)
type verbCase struct {
method string
handler func(http.ResponseWriter, *http.Request, map[string]string)
names []string
body bool
}
func verbs() []verbCase {
return []verbCase{
{http.MethodGet, ActionActionNameGet, readOnlyActions, false},
{http.MethodPost, ActionActionNamePost, append(append([]string{}, alertActions...), authProxyActions...), true},
{http.MethodPut, ActionActionNamePut, nil, true},
{http.MethodDelete, ActionActionNameDelete, deleteActions, true},
}
}
// serve runs one dispatch call with the given action name and body.
func serve(t *testing.T, vc verbCase, name, body string) *httptest.ResponseRecorder {
t.Helper()
ctx, cancel := context.WithTimeout(context.Background(), dispatchTimeout)
defer cancel()
var req *http.Request
if vc.body {
req = httptest.NewRequest(vc.method, "/action/"+name, strings.NewReader(body))
} else {
req = httptest.NewRequest(vc.method, "/action/"+name, nil)
}
req = req.WithContext(ctx)
req.Header.Set("Content-Type", "application/json")
rec := httptest.NewRecorder()
vc.handler(rec, req, map[string]string{"actionName": name})
return rec
}
// assertReadableError is the invariant: non-empty body, and a complete JSON
// envelope when the response claims to be JSON.
func assertReadableError(t *testing.T, rec *httptest.ResponseRecorder, what string) {
t.Helper()
if rec.Code < 400 {
t.Fatalf("%s: expected an error status, got %d", what, rec.Code)
}
body := strings.TrimSpace(rec.Body.String())
if body == "" {
t.Fatalf("%s: status %d with an empty body — an error must carry a message", what, rec.Code)
}
if ct := rec.Header().Get("Content-Type"); strings.Contains(ct, "application/json") {
var envelope struct {
Error struct {
Code string `json:"code"`
Message string `json:"message"`
} `json:"error"`
}
if err := json.Unmarshal([]byte(body), &envelope); err != nil {
t.Fatalf("%s: JSON error body does not parse: %v (%s)", what, err, body)
}
if strings.TrimSpace(envelope.Error.Code) == "" {
t.Fatalf("%s: JSON error without a code: %s", what, body)
}
if strings.TrimSpace(envelope.Error.Message) == "" {
t.Fatalf("%s: JSON error without a message: %s", what, body)
}
}
}
// TestUnknownActionsAreReadablePlainText walks GET/POST/PUT/DELETE with names no
// switch routes. Every one is 501 and every one must read.
func TestUnknownActionsAreReadablePlainText(t *testing.T) {
for _, vc := range verbs() {
for _, name := range []string{"nope", "getAlerts", "triggeralert", ""} {
rec := serve(t, vc, name, "{}")
if rec.Code != http.StatusNotImplemented {
t.Fatalf("%s /action/%s = %d, want 501", vc.method, name, rec.Code)
}
if ct := rec.Header().Get("Content-Type"); !strings.Contains(ct, "text/plain") {
t.Fatalf("%s /action/%s Content-Type = %q, want text/plain", vc.method, name, ct)
}
if strings.TrimSpace(rec.Body.String()) == "" {
t.Fatalf("%s /action/%s: 501 with an empty body", vc.method, name)
}
}
}
}
// TestMalformedEnvelopesAreReadablePlainText covers the decoder failures that
// the dispatcher writes itself.
func TestMalformedEnvelopesAreReadablePlainText(t *testing.T) {
bodies := []string{
"not json at all",
`{"payload":{},"unknown":"top-level"}`,
`[1,2,3]`,
`"a string"`,
}
for _, vc := range verbs() {
if !vc.body {
continue
}
for _, body := range bodies {
rec := serve(t, vc, "getAlert", body)
if rec.Code != http.StatusBadRequest {
t.Fatalf("%s body %q = %d, want 400", vc.method, body, rec.Code)
}
if strings.TrimSpace(rec.Body.String()) == "" {
t.Fatalf("%s body %q: 400 with an empty body", vc.method, body)
}
}
}
}
// TestEveryRoutedActionHasReadableError is the route walk proper: every action
// the switches route, with a valid envelope and no access token, must answer
// with a readable error (401/403/404/409/500/502 depending on the action).
func TestEveryRoutedActionHasReadableError(t *testing.T) {
// Point the shared-auth proxy at a closed port so the proxy actions fail
// immediately (502 with a message) instead of depending on DNS or a live
// auth service. This keeps the walk hermetic and fast.
t.Setenv("AUTH_SERVICE_URL", "http://127.0.0.1:1")
for _, vc := range verbs() {
for _, name := range vc.names {
rec := serve(t, vc, name, "{}")
assertReadableError(t, rec, vc.method+" /action/"+name)
}
}
}
// TestPutIsReadableForKnownActions pins that PUT is routed but implements
// nothing: 501 for every name, known or not, with a message.
func TestPutIsReadableForKnownActions(t *testing.T) {
vc := verbCase{http.MethodPut, ActionActionNamePut, nil, true}
for _, name := range append(append([]string{}, alertActions...), "me", "logout") {
rec := serve(t, vc, name, "{}")
if rec.Code != http.StatusNotImplemented {
t.Fatalf("PUT /action/%s = %d, want 501", name, rec.Code)
}
if strings.TrimSpace(rec.Body.String()) == "" {
t.Fatalf("PUT /action/%s: 501 with an empty body", name)
}
}
}
+143
View File
@@ -0,0 +1,143 @@
// Copyright (C) 2026 Dcntrlized
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public License as published
// by the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Affero General Public License for more details.
//
// You should have received a copy of the GNU Affero General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.
// This file makes "every error response carries a human-readable message" a
// structural property instead of a convention: Write and WriteText are the only
// emitters of an error status in the service, and both substitute a non-empty
// message when handed an empty one.
//
// It exists because an error status with no readable body is a client-breaking
// bug that no amount of review reliably catches: http.Error(w, "", 500) and a
// JSON envelope built without a message both look harmless at the call site.
package apierr
import (
"encoding/json"
"fmt"
"net/http"
"strings"
)
// fallbackMessage is the last resort for a code nobody registered.
const fallbackMessage = "unexpected error"
// defaultMessages is the canonical human-readable text for each wire code. The
// handlers usually send a more specific message; this is the floor.
var defaultMessages = map[string]string{
AlertNotFound: "alert not found",
AlertTokenInvalid: "the provided token does not match this alert",
AlertNotAdmin: "caller is not the alert admin",
AlertAlreadyJoined: "this device is already subscribed to this alert",
AlertNotSubscribed: "caller is not subscribed to this alert",
AlertState: "the alert is not in a state that allows this action",
AlarmTriggeredOffline: "an alarm was triggered while this device was offline",
Validation: "the request payload is not valid",
Unauthorized: "missing or invalid access token",
Internal: "internal error",
MethodNotAllowed: "method not allowed",
Unavailable: "service is unavailable",
}
// Entry describes one documented wire code.
type Entry struct {
Code string
Message string
}
// All returns every documented wire code with its default message, in a stable
// order (the order of the const block in codes.go).
func All() []Entry {
return []Entry{
{AlertNotFound, defaultMessages[AlertNotFound]},
{AlertTokenInvalid, defaultMessages[AlertTokenInvalid]},
{AlertNotAdmin, defaultMessages[AlertNotAdmin]},
{AlertAlreadyJoined, defaultMessages[AlertAlreadyJoined]},
{AlertNotSubscribed, defaultMessages[AlertNotSubscribed]},
{AlertState, defaultMessages[AlertState]},
{AlarmTriggeredOffline, defaultMessages[AlarmTriggeredOffline]},
{Validation, defaultMessages[Validation]},
{Unauthorized, defaultMessages[Unauthorized]},
{Internal, defaultMessages[Internal]},
{MethodNotAllowed, defaultMessages[MethodNotAllowed]},
{Unavailable, defaultMessages[Unavailable]},
}
}
// Message returns the canonical message for code. It never returns an empty
// string: an unknown or empty code gets a generic message (and Write upgrades an
// empty code to Internal before asking).
func Message(code string) string {
if m, ok := defaultMessages[code]; ok && strings.TrimSpace(m) != "" {
return m
}
return fallbackMessage
}
// ErrorBody is the platform error envelope.
type ErrorBody struct {
Error struct {
Code string `json:"code"`
Message string `json:"message"`
} `json:"error"`
}
// Write emits {"error":{"code","message"}} and is the only way this service
// writes a JSON error. An empty code becomes ERR_INTERNAL and an empty message
// becomes the code's canonical message, so the body always carries both a code
// and a readable message.
func Write(w http.ResponseWriter, status int, code, message string) {
code = strings.TrimSpace(code)
if code == "" {
code = Internal
}
message = strings.TrimSpace(message)
if message == "" {
message = Message(code)
}
if status < 400 {
// Callers only reach this on an error path; keep a nonsensical status
// from producing an apparently successful error response.
status = http.StatusInternalServerError
}
var body ErrorBody
body.Error.Code = code
body.Error.Message = message
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(status)
_ = json.NewEncoder(w).Encode(body)
}
// WriteText emits a plain-text error body (the dispatch-level responses). An
// empty message falls back to the status text and then to a generic phrase, so
// it can never write a bare status line.
func WriteText(w http.ResponseWriter, status int, message string) {
message = strings.TrimSpace(message)
if message == "" {
message = strings.TrimSpace(http.StatusText(status))
}
if message == "" {
message = fallbackMessage
}
if status < 400 {
status = http.StatusInternalServerError
}
w.Header().Set("Content-Type", "text/plain; charset=utf-8")
w.Header().Set("X-Content-Type-Options", "nosniff")
w.WriteHeader(status)
_, _ = fmt.Fprintln(w, message)
}
+160
View File
@@ -0,0 +1,160 @@
// Copyright (C) 2026 Dcntrlized
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public License as published
// by the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Affero General Public License for more details.
//
// You should have received a copy of the GNU Affero General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.
package apierr
import (
"encoding/json"
"net/http"
"net/http/httptest"
"os"
"regexp"
"strings"
"testing"
)
// TestEveryCodeHasMessage walks the documented codes and fails if any of them
// would emit an empty message.
func TestEveryCodeHasMessage(t *testing.T) {
entries := All()
if len(entries) == 0 {
t.Fatal("All() returned no codes")
}
for _, e := range entries {
if strings.TrimSpace(e.Code) == "" {
t.Fatalf("code entry with empty code: %+v", e)
}
if strings.TrimSpace(e.Message) == "" {
t.Fatalf("%s has no default message", e.Code)
}
if got := Message(e.Code); got != e.Message {
t.Fatalf("Message(%s) = %q, want %q", e.Code, got, e.Message)
}
}
}
// TestAllCoversEveryDeclaredCode reads codes.go and checks that no wire code
// constant exists without a default message registered in All(). Adding a code
// to the const block without registering it fails here rather than at runtime
// with a bodyless error.
func TestAllCoversEveryDeclaredCode(t *testing.T) {
src, err := os.ReadFile("codes.go")
if err != nil {
t.Fatalf("read codes.go: %v", err)
}
declared := regexp.MustCompile(`=\s*"(ERR_[A-Z_]+)"`).FindAllStringSubmatch(string(src), -1)
if len(declared) == 0 {
t.Fatal("found no ERR_ constants in codes.go")
}
known := make(map[string]bool, len(All()))
for _, e := range All() {
known[e.Code] = true
}
for _, m := range declared {
if !known[m[1]] {
t.Fatalf("code %s is declared in codes.go but missing from apierr.All() — it would have no default message", m[1])
}
}
}
// TestWriteFillsEmptyMessageAndCode pins the structural guarantee: the emitter,
// not the caller, is responsible for a readable body.
func TestWriteFillsEmptyMessageAndCode(t *testing.T) {
rec := httptest.NewRecorder()
Write(rec, http.StatusInternalServerError, "", "")
assertJSONError(t, rec, Internal, "")
rec = httptest.NewRecorder()
Write(rec, http.StatusForbidden, AlertTokenInvalid, " ")
assertJSONError(t, rec, AlertTokenInvalid, Message(AlertTokenInvalid))
rec = httptest.NewRecorder()
Write(rec, http.StatusTeapot, "ERR_NOT_A_REAL_CODE", "")
assertJSONError(t, rec, "ERR_NOT_A_REAL_CODE", fallbackMessage)
}
// TestWriteTextNeverEmpty covers the plain-text dispatch path, including the
// statuses it actually uses. This is the http.Error(w, "", 500) case.
func TestWriteTextNeverEmpty(t *testing.T) {
for _, status := range []int{
http.StatusBadRequest,
http.StatusMethodNotAllowed,
http.StatusNotImplemented,
http.StatusServiceUnavailable,
} {
rec := httptest.NewRecorder()
WriteText(rec, status, "")
body := strings.TrimSpace(rec.Body.String())
if rec.Code != status {
t.Fatalf("status %d: got %d", status, rec.Code)
}
if body == "" {
t.Fatalf("status %d: empty plain-text body", status)
}
if !strings.Contains(rec.Header().Get("Content-Type"), "text/plain") {
t.Fatalf("status %d: Content-Type = %q", status, rec.Header().Get("Content-Type"))
}
}
rec := httptest.NewRecorder()
WriteText(rec, http.StatusBadRequest, "invalid request body: boom")
if got := strings.TrimSpace(rec.Body.String()); got != "invalid request body: boom" {
t.Fatalf("message not passed through: %q", got)
}
}
// TestWriteAlwaysHasBothJSONFields is the shape assertion: {code, message} even
// when the caller supplies neither.
func TestWriteAlwaysHasBothJSONFields(t *testing.T) {
rec := httptest.NewRecorder()
Write(rec, http.StatusConflict, AlertState, "")
var body struct {
Error struct {
Code string `json:"code"`
Message string `json:"message"`
} `json:"error"`
}
if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil {
t.Fatalf("body is not JSON: %v (%s)", err, rec.Body.String())
}
if body.Error.Code == "" || body.Error.Message == "" {
t.Fatalf("incomplete envelope: %s", rec.Body.String())
}
}
func assertJSONError(t *testing.T, rec *httptest.ResponseRecorder, wantCode, wantMessage string) {
t.Helper()
if !strings.Contains(rec.Header().Get("Content-Type"), "application/json") {
t.Fatalf("Content-Type = %q", rec.Header().Get("Content-Type"))
}
var body struct {
Error struct {
Code string `json:"code"`
Message string `json:"message"`
} `json:"error"`
}
if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil {
t.Fatalf("body is not JSON: %v (%s)", err, rec.Body.String())
}
if body.Error.Code != wantCode {
t.Fatalf("code = %q, want %q", body.Error.Code, wantCode)
}
if wantMessage != "" && body.Error.Message != wantMessage {
t.Fatalf("message = %q, want %q", body.Error.Message, wantMessage)
}
if strings.TrimSpace(body.Error.Message) == "" {
t.Fatal("empty message in the error envelope")
}
}
+14 -3
View File
@@ -28,7 +28,18 @@ const (
AlertAlreadyJoined = "ERR_ALERT_ALREADY_JOINED"
AlertNotSubscribed = "ERR_ALERT_NOT_SUBSCRIBED"
AlertState = "ERR_ALERT_STATE"
Validation = "ERR_VALIDATION"
Unauthorized = "ERR_UNAUTHORIZED"
Internal = "ERR_INTERNAL"
// AlarmTriggeredOffline is NOT an error: it is the alarmStatus check-in
// answering 409 because this device still owes a trigger it missed while
// offline. The body carries the alarm frame and the client must ring.
AlarmTriggeredOffline = "ERR_ALARM_TRIGGERED_OFFLINE"
Validation = "ERR_VALIDATION"
Unauthorized = "ERR_UNAUTHORIZED"
Internal = "ERR_INTERNAL"
// MethodNotAllowed and Unavailable are emitted on the /ws/alarm handshake
// only. They are JSON codes; the dispatch-level plain-text 405 carries no
// code (see API.md).
MethodNotAllowed = "ERR_METHOD_NOT_ALLOWED"
Unavailable = "ERR_UNAVAILABLE"
)