Compare commits

..

No commits in common. "87c17d4854714d3f6c52ca6210ac5a368eb92f6b" and "051169a18a9efd2d74da53930fc7d266e4d4928e" have entirely different histories.

9 changed files with 10 additions and 347 deletions

View file

@ -1,36 +0,0 @@
name: CI
on:
push:
pull_request:
jobs:
ci:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version: '1.26'
- name: gofmt
run: test -z "$(gofmt -l .)"
- run: go build ./...
- run: go vet ./...
- run: go test ./...
- uses: actions/setup-node@v4
with:
node-version: '22'
- uses: pnpm/action-setup@v4
with:
version: '12'
- name: web check
working-directory: web
run: pnpm install --frozen-lockfile && pnpm check

View file

@ -88,7 +88,6 @@ func run(cfg core.Config, log *slog.Logger) error {
api := httpapi.New(db, log, version)
authenticator := auth.NewAuthenticator(db, cfg.CookieName, cfg.SessionTTL, cfg.CookieSecure, log)
authenticator.TrustedProxies = cfg.TrustedProxies
if !cfg.CookieSecure {
log.Warn("VELLUM_COOKIE_SECURE is false; session cookies will be sent over plain HTTP. " +
"Set it to true when serving over TLS.")

View file

@ -1,118 +0,0 @@
package auth
import (
"net"
"net/http"
"net/http/httptest"
"testing"
)
// clientIPFor builds an Authenticator trusting the given CIDRs and returns the
// address it derives from a request arriving from peer with header XFF.
func clientIPFor(t *testing.T, peer, xff string, trusted ...string) string {
t.Helper()
a := &Authenticator{}
for _, c := range trusted {
_, n, err := net.ParseCIDR(c)
if err != nil {
t.Fatalf("parse test CIDR %q: %v", c, err)
}
a.TrustedProxies = append(a.TrustedProxies, n)
}
r := httptest.NewRequest(http.MethodGet, "/", nil)
r.RemoteAddr = peer
if xff != "" {
r.Header.Set("X-Forwarded-For", xff)
}
return a.ClientIP(r)
}
func TestClientIPUntrustedPeerIgnoresForwardedFor(t *testing.T) {
// The security case: an untrusted peer sending X-Forwarded-For must not be
// believed, or anyone could evade (or poison) the login rate limiter.
got := clientIPFor(t, "203.0.113.9:44321", "1.2.3.4", "172.16.0.0/12")
if got != "203.0.113.9" {
t.Errorf("ClientIP = %q, want the peer 203.0.113.9", got)
}
}
func TestClientIPTrustedPeerUsesForwardedFor(t *testing.T) {
// The reverse-proxy case: every request arrives from the proxy, so without
// the header every client shares one limiter key.
got := clientIPFor(t, "172.20.0.5:51000", "1.2.3.4", "172.16.0.0/12")
if got != "1.2.3.4" {
t.Errorf("ClientIP = %q, want the client 1.2.3.4 from the header", got)
}
}
func TestClientIPChain(t *testing.T) {
tests := []struct {
name string
peer string
xff string
trusted []string
want string
}{
{
name: "no trusted proxies at all",
peer: "172.20.0.5:51000", xff: "1.2.3.4",
want: "172.20.0.5",
},
{
name: "trusted peer without header",
peer: "172.20.0.5:51000",
trusted: []string{"172.16.0.0/12"},
want: "172.20.0.5",
},
{
name: "spans proxies on the right of the client",
peer: "172.20.0.5:51000", xff: "1.2.3.4, 172.18.0.1",
trusted: []string{"172.16.0.0/12"},
want: "1.2.3.4",
},
{
// A client-supplied leftmost entry is attacker-controlled, so the
// walk must stop at the first untrusted hop counting from the right.
name: "ignores a spoofed leftmost entry",
peer: "172.20.0.5:51000", xff: "9.9.9.9, 1.2.3.4, 172.18.0.1",
trusted: []string{"172.16.0.0/12"},
want: "1.2.3.4",
},
{
name: "every hop trusted falls back to the leftmost",
peer: "172.20.0.5:51000", xff: "172.19.0.1, 172.18.0.1, 172.17.0.1",
trusted: []string{"172.16.0.0/12"},
want: "172.19.0.1",
},
{
name: "multiple trusted networks",
peer: "127.0.0.1:51000", xff: "1.2.3.4",
trusted: []string{"172.16.0.0/12", "127.0.0.1/32"},
want: "1.2.3.4",
},
{
name: "unparseable hop is not trusted",
peer: "172.20.0.5:51000", xff: "not-an-ip",
trusted: []string{"172.16.0.0/12"},
want: "not-an-ip",
},
{
name: "remote address without a port",
peer: "172.20.0.5", xff: "1.2.3.4",
trusted: []string{"172.16.0.0/12"},
want: "1.2.3.4",
},
{
name: "untrusted peer without a port",
peer: "203.0.113.9", xff: "1.2.3.4",
want: "203.0.113.9",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
if got := clientIPFor(t, tc.peer, tc.xff, tc.trusted...); got != tc.want {
t.Errorf("ClientIP = %q, want %q", got, tc.want)
}
})
}
}

View file

@ -83,7 +83,7 @@ func (h *Handlers) handleLogin(w http.ResponseWriter, r *http.Request) {
return
}
ip := h.auth.ClientIP(r)
ip := ClientIP(r)
if !h.limiter.allow(ip + "|" + req.Username) {
h.log.Warn("login rate limited", "ip", ip, "username", req.Username)
httpx.WriteError(w, http.StatusTooManyRequests, "too many login attempts, try again later")

View file

@ -6,7 +6,6 @@ import (
"log/slog"
"net"
"net/http"
"strings"
"time"
"github.com/mithral/vellum/internal/httpx"
@ -34,9 +33,6 @@ type Authenticator struct {
CookieTTL time.Duration
Secure bool
Log *slog.Logger
// TrustedProxies are the networks allowed to name the real client in
// X-Forwarded-For. Empty means trust nobody. Set from cfg.TrustedProxies.
TrustedProxies []*net.IPNet
// touchEvery is the minimum age of a session's last_seen before Middleware
// will write it again. A zero value means "touch on every request" (the
@ -186,49 +182,12 @@ func (a *Authenticator) TokenFromRequest(r *http.Request) (string, error) {
}
// ClientIP extracts a best-effort client address for logging and rate limiting.
//
// Behind a reverse proxy every request arrives from the proxy, so RemoteAddr
// alone keys the login limiter identically for every client on the internet:
// one attacker locks out a username for everyone, and sessions.ip records the
// proxy. X-Forwarded-For fixes that, but only for a peer we already trust —
// from an untrusted peer that header is attacker-controlled, so it is ignored
// outright and the connection's own address is used, as before.
//
// With a trusted peer the chain is walked right-to-left and the first address
// that is not itself a trusted proxy wins: everything to its right is a hop we
// control, and everything to its left was appended by a client we do not. If
// every hop is trusted the leftmost entry is the best available answer.
func (a *Authenticator) ClientIP(r *http.Request) string {
// It deliberately does NOT trust X-Forwarded-For: behind an untrusted network
// that header is attacker-controlled, and this server is LAN-facing.
func ClientIP(r *http.Request) string {
host, _, err := net.SplitHostPort(r.RemoteAddr)
if err != nil {
host = r.RemoteAddr
return r.RemoteAddr
}
if !a.trusts(host) {
return host
}
chain := r.Header.Get("X-Forwarded-For")
if chain == "" {
return host
}
hops := strings.Split(chain, ",")
for i := len(hops) - 1; i >= 0; i-- {
if hop := strings.TrimSpace(hops[i]); !a.trusts(hop) {
return hop
}
}
return strings.TrimSpace(hops[0])
}
// trusts reports whether ip falls inside one of the configured proxy networks.
func (a *Authenticator) trusts(ip string) bool {
parsed := net.ParseIP(ip)
if parsed == nil {
return false
}
for _, n := range a.TrustedProxies {
if n.Contains(parsed) {
return true
}
}
return false
return host
}

View file

@ -2,7 +2,6 @@ package core
import (
"fmt"
"net"
"os"
"path/filepath"
"strconv"
@ -45,15 +44,6 @@ type Config struct {
CookieName string
CookieSecure bool
// PublicURL is the external origin Vellum is reached at, e.g.
// "https://media.example.com". Empty means "not published", which is the
// case for plain-HTTP LAN use. It is the signal that the deployment sits
// behind a TLS-terminating proxy, so it also drives the CookieSecure default.
PublicURL string
// TrustedProxies are the networks allowed to speak for a client in
// X-Forwarded-For. Empty means trust nobody: only RemoteAddr is used.
TrustedProxies []*net.IPNet
SessionTTL time.Duration
ReadTimeout time.Duration
WriteTimeout time.Duration
@ -64,7 +54,6 @@ type Config struct {
// LoadConfig reads configuration from the environment and fills in defaults.
// It does not create directories; call Validate for that.
func LoadConfig() (Config, error) {
publicURL := env("VELLUM_PUBLIC_URL", "")
c := Config{
Listen: env("VELLUM_LISTEN", ":7788"),
DataDir: env("VELLUM_DATA_DIR", "./data"),
@ -80,11 +69,10 @@ func LoadConfig() (Config, error) {
IGDBClientID: env("VELLUM_IGDB_CLIENT_ID", ""),
IGDBClientSecret: env("VELLUM_IGDB_CLIENT_SECRET", ""),
CookieName: env("VELLUM_COOKIE_NAME", "vellum_session"),
// Secure defaults to true for an https:// PublicURL — a published origin
// is TLS-terminated, so plain-HTTP dev still gets false with it unset.
// VELLUM_COOKIE_SECURE overrides both ways; boot logs a warning when off.
CookieSecure: envBool("VELLUM_COOKIE_SECURE", strings.HasPrefix(publicURL, "https://")),
PublicURL: publicURL,
// Secure defaults to false so plain-HTTP development works. Production
// behind TLS must set VELLUM_COOKIE_SECURE=true; boot logs a warning
// when it is off.
CookieSecure: envBool("VELLUM_COOKIE_SECURE", false),
SessionTTL: envDuration("VELLUM_SESSION_TTL", 30*24*time.Hour),
ReadTimeout: envDuration("VELLUM_READ_TIMEOUT", 15*time.Second),
WriteTimeout: envDuration("VELLUM_WRITE_TIMEOUT", 0), // 0 = no limit; streaming needs this
@ -92,12 +80,6 @@ func LoadConfig() (Config, error) {
ShutdownGrace: envDuration("VELLUM_SHUTDOWN_GRACE", 15*time.Second),
}
proxies, err := envCIDRs("VELLUM_TRUSTED_PROXIES")
if err != nil {
return Config{}, err
}
c.TrustedProxies = proxies
abs, err := filepath.Abs(c.DataDir)
if err != nil {
return Config{}, fmt.Errorf("resolve data dir: %w", err)
@ -167,25 +149,6 @@ func envList(key string, def []string) []string {
return out
}
// envCIDRs parses a comma-separated CIDR list, e.g.
// "172.16.0.0/12,127.0.0.1/32". A malformed entry is an error rather than a
// silent skip: dropping a proxy network quietly widens the trust boundary,
// which is exactly the failure this setting exists to prevent.
func envCIDRs(key string) ([]*net.IPNet, error) {
var out []*net.IPNet
for _, part := range strings.Split(env(key, ""), ",") {
if part = strings.TrimSpace(part); part == "" {
continue
}
_, n, err := net.ParseCIDR(part)
if err != nil {
return nil, fmt.Errorf("%s: %w", key, err)
}
out = append(out, n)
}
return out, nil
}
func envDuration(key string, def time.Duration) time.Duration {
v, ok := os.LookupEnv(key)
if !ok || v == "" {

View file

@ -1,7 +1,6 @@
package core
import (
"net"
"path/filepath"
"strings"
"testing"
@ -51,72 +50,6 @@ func TestLoadConfigFromEnv(t *testing.T) {
}
}
func TestLoadConfigTrustedProxies(t *testing.T) {
t.Setenv("VELLUM_TRUSTED_PROXIES", "172.16.0.0/12, 127.0.0.1/32")
cfg, err := LoadConfig()
if err != nil {
t.Fatalf("LoadConfig: %v", err)
}
if len(cfg.TrustedProxies) != 2 {
t.Fatalf("TrustedProxies = %v, want 2 entries", cfg.TrustedProxies)
}
if !cfg.TrustedProxies[0].Contains(net.ParseIP("172.20.0.5")) {
t.Errorf("TrustedProxies[0] = %v, want it to contain 172.20.0.5", cfg.TrustedProxies[0])
}
if cfg.TrustedProxies[1].Contains(net.ParseIP("172.20.0.5")) {
t.Errorf("TrustedProxies[1] = %v, want it to contain only 127.0.0.1", cfg.TrustedProxies[1])
}
}
func TestLoadConfigRejectsBadTrustedProxy(t *testing.T) {
// A malformed entry must fail the boot: silently dropping it would quietly
// narrow the trust boundary and the operator would never know.
t.Setenv("VELLUM_TRUSTED_PROXIES", "172.16.0.0/12, not-a-cidr")
_, err := LoadConfig()
if err == nil {
t.Fatal("expected error for malformed VELLUM_TRUSTED_PROXIES")
}
if !strings.Contains(err.Error(), "not-a-cidr") {
t.Errorf("error %q should name the offending entry", err)
}
}
func TestCookieSecureDefaultsFromPublicURL(t *testing.T) {
tests := []struct {
name string
public string
cookie string // empty means unset
want bool
}{
{name: "https origin secures the cookie", public: "https://media.example.com", want: true},
{name: "http origin leaves it off", public: "http://media.example.com", want: false},
{name: "unset origin leaves it off", want: false},
{name: "explicit false overrides https", public: "https://media.example.com", cookie: "false", want: false},
{name: "explicit true overrides http", public: "http://media.example.com", cookie: "true", want: true},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Setenv("VELLUM_PUBLIC_URL", tc.public)
// An empty value reads as unset to the env helpers, and t.Setenv
// restores the ambient value either way.
t.Setenv("VELLUM_COOKIE_SECURE", tc.cookie)
cfg, err := LoadConfig()
if err != nil {
t.Fatalf("LoadConfig: %v", err)
}
if cfg.PublicURL != tc.public {
t.Errorf("PublicURL = %q, want %q", cfg.PublicURL, tc.public)
}
if cfg.CookieSecure != tc.want {
t.Errorf("CookieSecure = %v, want %v", cfg.CookieSecure, tc.want)
}
})
}
}
func TestValidateRejectsMissingMediaDir(t *testing.T) {
cfg := Config{
DataDir: t.TempDir(),

View file

@ -31,23 +31,6 @@ func Migrate(ctx context.Context, db *sql.DB, fsys fs.FS, log *slog.Logger) erro
}
log.Debug("schema version", "current", current, "available", len(names))
highest := 0
for _, name := range names {
n, err := migrationNumber(name)
if err != nil {
return err
}
if n > highest {
highest = n
}
}
// A version above every embedded migration means this binary is older than
// the database. Without this, the loop below skips everything and starts up
// against a schema it cannot read.
if current > highest {
return fmt.Errorf("database schema is version %d but this binary only knows %d: this binary is older than the database — upgrade the binary or restore a backup", current, highest)
}
for _, name := range names {
n, err := migrationNumber(name)
if err != nil {

View file

@ -3,7 +3,6 @@ package core
import (
"context"
"database/sql"
"fmt"
"io"
"io/fs"
"log/slog"
@ -105,25 +104,6 @@ func TestForeignKeyEnforcementIsOn(t *testing.T) {
}
}
func TestMigrateRefusesDatabaseNewerThanBinary(t *testing.T) {
ctx := context.Background()
db := openTempDB(t)
// A database migrated by a newer binary.
newer := highestMigrationNumber(t) + 1
if _, err := db.ExecContext(ctx, fmt.Sprintf("PRAGMA user_version = %d", newer)); err != nil {
t.Fatalf("set user_version: %v", err)
}
err := Migrate(ctx, db, migrations.FS, discardLogger())
if err == nil {
t.Fatal("expected error when user_version is above the highest embedded migration, got nil")
}
if !strings.Contains(err.Error(), fmt.Sprintf("version %d", newer)) {
t.Errorf("error should name the database version, got: %v", err)
}
}
func TestMigrationNumber(t *testing.T) {
cases := map[string]int{
"0001_core.sql": 1,