mirror of
https://github.com/renorris/openfsd
synced 2026-08-11 20:15:33 +08:00
fix: address review feedback for API versioning foundation
Registry invariant tests, public-route no-version-reject coverage, single major-version constant, OpenAPI BadAPIVersion responses, slog on pin reject, append-style Vary, and small dead-code cleanups.
This commit is contained in:
@@ -12,11 +12,11 @@ type APIV1Response struct {
|
||||
Data any `json:"data"`
|
||||
}
|
||||
|
||||
const v1Version = "v1"
|
||||
// Envelope body major version uses apiMajorVersion (api_version.go) as single source of truth.
|
||||
|
||||
func newAPIV1Success(data any) APIV1Response {
|
||||
return APIV1Response{
|
||||
Version: v1Version,
|
||||
Version: apiMajorVersion,
|
||||
Err: nil,
|
||||
Data: data,
|
||||
}
|
||||
@@ -24,7 +24,7 @@ func newAPIV1Success(data any) APIV1Response {
|
||||
|
||||
func newAPIV1Failure(err string) APIV1Response {
|
||||
return APIV1Response{
|
||||
Version: v1Version,
|
||||
Version: apiMajorVersion,
|
||||
Err: &err,
|
||||
Data: nil,
|
||||
}
|
||||
|
||||
@@ -5,6 +5,7 @@ import (
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"net/http"
|
||||
"regexp"
|
||||
"strings"
|
||||
@@ -37,8 +38,7 @@ var knownMicroversions = []string{
|
||||
}
|
||||
|
||||
var (
|
||||
errAPIVersionInvalid = errors.New("invalid OpenFSD-API-Version")
|
||||
errAPIVersionUnsupported = errors.New("unsupported OpenFSD-API-Version")
|
||||
errAPIVersionInvalid = errors.New("invalid OpenFSD-API-Version")
|
||||
|
||||
reAPIVersionDate = regexp.MustCompile(`^\d{4}-\d{2}-\d{2}$`)
|
||||
reAPIVersionAlias = regexp.MustCompile(`^1\.(\d{8})$`)
|
||||
@@ -146,7 +146,12 @@ func (s *Server) apiVersionMiddleware(c *gin.Context) {
|
||||
|
||||
canonical, err := normalizeAPIVersion(trimmed, apiMicroMax)
|
||||
if err != nil {
|
||||
setAPIVersionHeadersExplicit(c, apiMicroMax, true /* still show registry */)
|
||||
setAPIVersionHeadersExplicit(c, apiMicroMax)
|
||||
slog.Warn("invalid OpenFSD-API-Version",
|
||||
"path", c.Request.URL.Path,
|
||||
"raw", trimmed,
|
||||
"cid", apiVersionLogCID(c),
|
||||
)
|
||||
res := newAPIV1Failure("invalid OpenFSD-API-Version")
|
||||
writeAPIV1Response(c, http.StatusBadRequest, &res)
|
||||
c.Abort()
|
||||
@@ -154,7 +159,13 @@ func (s *Server) apiVersionMiddleware(c *gin.Context) {
|
||||
}
|
||||
|
||||
if !isKnownMicroversion(canonical) {
|
||||
setAPIVersionHeadersExplicit(c, apiMicroMax, true)
|
||||
setAPIVersionHeadersExplicit(c, apiMicroMax)
|
||||
slog.Warn("unsupported OpenFSD-API-Version",
|
||||
"path", c.Request.URL.Path,
|
||||
"raw", trimmed,
|
||||
"canonical", canonical,
|
||||
"cid", apiVersionLogCID(c),
|
||||
)
|
||||
msg := fmt.Sprintf("unsupported OpenFSD-API-Version %q; min=%s max=%s",
|
||||
canonical, apiMicroMin, apiMicroMax)
|
||||
res := newAPIV1Failure(msg)
|
||||
@@ -201,7 +212,7 @@ func setAPIVersionHeaders(c *gin.Context) {
|
||||
|
||||
// setAPIVersionHeadersExplicit sets registry headers with a chosen effective pin
|
||||
// (used when rejecting a bad pin before context is stored).
|
||||
func setAPIVersionHeadersExplicit(c *gin.Context, effective string, _ bool) {
|
||||
func setAPIVersionHeadersExplicit(c *gin.Context, effective string) {
|
||||
setAPIVersionHeadersValues(c, effective, false)
|
||||
}
|
||||
|
||||
@@ -213,13 +224,37 @@ func setAPIVersionHeadersValues(c *gin.Context, effective string, defaulted bool
|
||||
h.Set(headerAPIVersion, effective)
|
||||
h.Set(headerAPIMinVersion, apiMicroMin)
|
||||
h.Set(headerAPIMaxVersion, apiMicroMax)
|
||||
// Caches must vary on the client pin.
|
||||
h.Set("Vary", headerAPIVersion)
|
||||
// Caches must vary on the client pin; merge if other middleware already set Vary.
|
||||
appendVary(h, headerAPIVersion)
|
||||
if defaulted {
|
||||
h.Set(headerAPIVersionDefaulted, "true")
|
||||
}
|
||||
}
|
||||
|
||||
// appendVary adds value to the Vary header without duplicating an existing token.
|
||||
func appendVary(h http.Header, value string) {
|
||||
existing := h.Get("Vary")
|
||||
if existing == "" {
|
||||
h.Set("Vary", value)
|
||||
return
|
||||
}
|
||||
for _, part := range strings.Split(existing, ",") {
|
||||
if strings.EqualFold(strings.TrimSpace(part), value) {
|
||||
return
|
||||
}
|
||||
}
|
||||
h.Set("Vary", existing+", "+value)
|
||||
}
|
||||
|
||||
// apiVersionLogCID returns the authenticated CID when jwt middleware already ran, else 0.
|
||||
// Never logs tokens.
|
||||
func apiVersionLogCID(c *gin.Context) int {
|
||||
if claims := getJwtContext(c); claims != nil {
|
||||
return claims.CID
|
||||
}
|
||||
return 0
|
||||
}
|
||||
|
||||
// discoveryData is the public version-discovery payload (KD-20).
|
||||
type discoveryData struct {
|
||||
Major string `json:"major"`
|
||||
|
||||
@@ -3,6 +3,7 @@ package web
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
@@ -15,6 +16,30 @@ import (
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestAPIVersionRegistryInvariants(t *testing.T) {
|
||||
require.NotEmpty(t, knownMicroversions, "knownMicroversions must not be empty")
|
||||
assert.Equal(t, knownMicroversions[0], apiMicroMin, "apiMicroMin must equal first known pin")
|
||||
assert.Equal(t, knownMicroversions[len(knownMicroversions)-1], apiMicroMax, "apiMicroMax must equal last known pin")
|
||||
|
||||
for i, pin := range knownMicroversions {
|
||||
// Valid calendar YYYY-MM-DD after normalize (identity).
|
||||
canonical, err := normalizeAPIVersion(pin, apiMicroMax)
|
||||
require.NoError(t, err, "known pin %q must normalize", pin)
|
||||
assert.Equal(t, pin, canonical)
|
||||
assert.True(t, reAPIVersionDate.MatchString(pin), "pin %q must be YYYY-MM-DD", pin)
|
||||
|
||||
if i > 0 {
|
||||
assert.True(t, knownMicroversions[i-1] < pin,
|
||||
"knownMicroversions must be ascending ISO: %q then %q", knownMicroversions[i-1], pin)
|
||||
}
|
||||
}
|
||||
|
||||
// Constants must themselves be known pins.
|
||||
assert.True(t, isKnownMicroversion(apiMicroMin))
|
||||
assert.True(t, isKnownMicroversion(apiMicroMax))
|
||||
assert.Equal(t, "v1", apiMajorVersion)
|
||||
}
|
||||
|
||||
func TestNormalizeAPIVersion(t *testing.T) {
|
||||
const max = "2026-07-28"
|
||||
|
||||
@@ -193,11 +218,10 @@ func TestAPIVersionMiddleware_InvalidPin400(t *testing.T) {
|
||||
func TestAPIDiscovery_VersionAgnostic(t *testing.T) {
|
||||
env := setupTestAPI(t)
|
||||
|
||||
paths := []string{"/api/v1", "/api/v1/", "/api/v1/versions"}
|
||||
// Gin may normalize trailing slash — hit both discovery routes + bad pin.
|
||||
// Bad pin would 400 on resource groups; discovery must ignore it.
|
||||
for _, path := range []string{"/api/v1", "/api/v1/versions"} {
|
||||
req := httptest.NewRequest(http.MethodGet, path, nil)
|
||||
req.Header.Set(headerAPIVersion, "2020-01-01") // would 400 on resource groups
|
||||
req.Header.Set(headerAPIVersion, "2020-01-01")
|
||||
w := httptest.NewRecorder()
|
||||
env.router.ServeHTTP(w, req)
|
||||
|
||||
@@ -206,14 +230,76 @@ func TestAPIDiscovery_VersionAgnostic(t *testing.T) {
|
||||
require.Nil(t, res.Err)
|
||||
assert.Equal(t, "v1", res.Version)
|
||||
|
||||
// Never 400 for pin on discovery.
|
||||
assert.Equal(t, apiMicroMax, w.Header().Get(headerAPIVersion))
|
||||
assert.Equal(t, apiMicroMin, w.Header().Get(headerAPIMinVersion))
|
||||
assert.Equal(t, apiMicroMax, w.Header().Get(headerAPIMaxVersion))
|
||||
_ = paths
|
||||
}
|
||||
}
|
||||
|
||||
// TestPublicRoutes_NoVersionReject covers KD-3: fsd-jwt, auth login/refresh never
|
||||
// hard-reject on OpenFSD-API-Version (unlike dual-accept resource groups).
|
||||
func TestPublicRoutes_NoVersionReject(t *testing.T) {
|
||||
env := setupTestAPI(t)
|
||||
badPins := []string{"2020-01-01", "not-a-version", "1.2026-07-28", "2026-02-30"}
|
||||
|
||||
t.Run("fsd-jwt", func(t *testing.T) {
|
||||
for _, pin := range badPins {
|
||||
body := fmt.Sprintf(`{"cid":"%d","password":%q}`, env.admin.CID, env.adminPass)
|
||||
req := httptest.NewRequest(http.MethodPost, "/api/v1/fsd-jwt", bytes.NewReader([]byte(body)))
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
req.Header.Set(headerAPIVersion, pin)
|
||||
w := httptest.NewRecorder()
|
||||
env.router.ServeHTTP(w, req)
|
||||
// Success path — never version 400.
|
||||
require.Equal(t, http.StatusOK, w.Code, "pin=%s body=%s", pin, w.Body.String())
|
||||
assert.NotContains(t, w.Body.String(), "OpenFSD-API-Version")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("auth_login", func(t *testing.T) {
|
||||
for _, pin := range badPins {
|
||||
body := fmt.Sprintf(`{"cid":%d,"password":%q}`, env.admin.CID, env.adminPass)
|
||||
req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/login", bytes.NewReader([]byte(body)))
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
req.Header.Set(headerAPIVersion, pin)
|
||||
w := httptest.NewRecorder()
|
||||
env.router.ServeHTTP(w, req)
|
||||
require.Equal(t, http.StatusOK, w.Code, "pin=%s body=%s", pin, w.Body.String())
|
||||
res := decodeAPIV1(t, w)
|
||||
require.Nil(t, res.Err, "pin must not produce version err; got %v", res.Err)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("auth_refresh", func(t *testing.T) {
|
||||
_, refresh := env.login(t, env.observer.CID, env.observerPass)
|
||||
for _, pin := range badPins {
|
||||
body, err := json.Marshal(map[string]string{"refresh_token": refresh})
|
||||
require.NoError(t, err)
|
||||
req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/refresh", bytes.NewReader(body))
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
req.Header.Set(headerAPIVersion, pin)
|
||||
w := httptest.NewRecorder()
|
||||
env.router.ServeHTTP(w, req)
|
||||
require.Equal(t, http.StatusOK, w.Code, "pin=%s body=%s", pin, w.Body.String())
|
||||
res := decodeAPIV1(t, w)
|
||||
require.Nil(t, res.Err)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestAppendVary(t *testing.T) {
|
||||
h := http.Header{}
|
||||
appendVary(h, headerAPIVersion)
|
||||
assert.Equal(t, headerAPIVersion, h.Get("Vary"))
|
||||
// Idempotent.
|
||||
appendVary(h, headerAPIVersion)
|
||||
assert.Equal(t, headerAPIVersion, h.Get("Vary"))
|
||||
// Merge with existing.
|
||||
h.Set("Vary", "Accept-Encoding")
|
||||
appendVary(h, headerAPIVersion)
|
||||
assert.Equal(t, "Accept-Encoding, "+headerAPIVersion, h.Get("Vary"))
|
||||
}
|
||||
|
||||
func TestAPIDiscovery_PayloadGolden(t *testing.T) {
|
||||
env := setupTestAPI(t)
|
||||
req := httptest.NewRequest(http.MethodGet, "/api/v1/versions", nil)
|
||||
|
||||
@@ -39,8 +39,8 @@ tags:
|
||||
description: APT/AIR validate (Stable; Instructor1+)
|
||||
- name: Sweatbox
|
||||
description: Read-only PE proxies (raw; outside envelope goldens)
|
||||
- name: Data
|
||||
description: Public data feeds (outside microversion reject)
|
||||
# Data feeds (/data/*) are public and outside microversion reject; documented in
|
||||
# internal/web/README.md. Not listed here until a full inventory is needed.
|
||||
|
||||
paths:
|
||||
/:
|
||||
@@ -198,6 +198,8 @@ paths:
|
||||
application/json:
|
||||
schema:
|
||||
$ref: "#/components/schemas/APIV1Envelope"
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
/user/update:
|
||||
patch:
|
||||
@@ -217,6 +219,8 @@ paths:
|
||||
application/json:
|
||||
schema:
|
||||
$ref: "#/components/schemas/APIV1Envelope"
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
/user/create:
|
||||
post:
|
||||
@@ -236,6 +240,8 @@ paths:
|
||||
application/json:
|
||||
schema:
|
||||
$ref: "#/components/schemas/APIV1Envelope"
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
/config/load:
|
||||
get:
|
||||
@@ -255,6 +261,8 @@ paths:
|
||||
application/json:
|
||||
schema:
|
||||
$ref: "#/components/schemas/APIV1Envelope"
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
/config/update:
|
||||
post:
|
||||
@@ -274,6 +282,8 @@ paths:
|
||||
application/json:
|
||||
schema:
|
||||
$ref: "#/components/schemas/APIV1Envelope"
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
/config/resetsecretkey:
|
||||
post:
|
||||
@@ -293,6 +303,8 @@ paths:
|
||||
application/json:
|
||||
schema:
|
||||
$ref: "#/components/schemas/APIV1Envelope"
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
/config/createtoken:
|
||||
post:
|
||||
@@ -328,6 +340,8 @@ paths:
|
||||
application/json:
|
||||
schema:
|
||||
$ref: "#/components/schemas/APIV1EnvelopeCreateToken"
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
/fsdconn/kickuser:
|
||||
post:
|
||||
@@ -356,6 +370,8 @@ paths:
|
||||
application/json:
|
||||
schema:
|
||||
$ref: "#/components/schemas/APIV1Envelope"
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
/editor/validate-apt:
|
||||
post:
|
||||
@@ -384,6 +400,8 @@ paths:
|
||||
application/json:
|
||||
schema:
|
||||
$ref: "#/components/schemas/APIV1Envelope"
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
/editor/validate-air:
|
||||
post:
|
||||
@@ -412,6 +430,8 @@ paths:
|
||||
application/json:
|
||||
schema:
|
||||
$ref: "#/components/schemas/APIV1Envelope"
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
/sweatbox/state:
|
||||
get:
|
||||
@@ -427,6 +447,8 @@ paths:
|
||||
responses:
|
||||
"200":
|
||||
description: Raw FSD JSON body
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
/sweatbox/ops:
|
||||
get:
|
||||
@@ -442,8 +464,45 @@ paths:
|
||||
responses:
|
||||
"200":
|
||||
description: Raw FSD JSON body
|
||||
"400":
|
||||
$ref: "#/components/responses/BadAPIVersion"
|
||||
|
||||
components:
|
||||
responses:
|
||||
BadAPIVersion:
|
||||
description: |
|
||||
Invalid or unsupported `OpenFSD-API-Version` on a dual-accept resource group
|
||||
(HTTP 400 envelope). Also used when the request body is invalid JSON on
|
||||
some routes — check `err` text.
|
||||
|
||||
Recovery: call version-agnostic `GET /api/v1/versions` (never rejects on pin)
|
||||
and upgrade the client pin.
|
||||
headers:
|
||||
OpenFSD-API-Version:
|
||||
schema: { type: string }
|
||||
description: Echo of current max (or registry effective)
|
||||
OpenFSD-API-Min-Version:
|
||||
schema: { type: string }
|
||||
OpenFSD-API-Max-Version:
|
||||
schema: { type: string }
|
||||
content:
|
||||
application/json:
|
||||
schema:
|
||||
$ref: "#/components/schemas/APIV1Envelope"
|
||||
examples:
|
||||
invalid:
|
||||
summary: Malformed pin
|
||||
value:
|
||||
version: v1
|
||||
err: invalid OpenFSD-API-Version
|
||||
data: null
|
||||
unsupported:
|
||||
summary: Unknown or sunset pin
|
||||
value:
|
||||
version: v1
|
||||
err: 'unsupported OpenFSD-API-Version "2020-01-01"; min=2026-07-28 max=2026-07-28'
|
||||
data: null
|
||||
|
||||
securitySchemes:
|
||||
bearerAuth:
|
||||
type: http
|
||||
|
||||
Reference in New Issue
Block a user