feat(config): flag + env parsing with validation (forgejo-mcp-broker-9nq)

Implements internal/config. Flags override env; empty env values are treated
as unset so exported-but-empty vars don't clobber defaults. Validation
aggregates every problem via errors.Join so operators see the full list
at once, not one at a time.

Notable decisions:
- Issuer URL validation requires http/https and rejects plain http on
  non-loopback hosts (classic OAuth misconfig). Loopback http allowed so
  local dev doesn't need TLS.
- Store-path writability probed via os.CreateTemp in the parent dir — a
  real write test, not a string check. No side effects on the store file
  itself (that's the storage layer's job).
- Public API: Load(args, out) returns *Config or error. flag.ErrHelp is
  passed through so callers can exit 0 on -h.

Tests: 94.1% line coverage, covers env/flag precedence, empty-as-unset,
every required-field message, each URL failure mode, numeric bounds,
env-parse errors, and -help output.

Closes forgejo-mcp-broker-9nq.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ole-Morten Duesund 2026-04-24 17:10:18 +02:00
commit 382356c61c
4 changed files with 593 additions and 6 deletions

257
internal/config/config.go Normal file
View file

@ -0,0 +1,257 @@
// Package config loads broker configuration from environment variables and
// command-line flags, applies defaults, and validates the result.
//
// Precedence: flags win over environment variables. Empty environment values
// are treated as unset, so operators can neutralise an exported variable
// without needing to unset it. Required fields have no default and must be
// supplied one way or the other. Validation is strict — the process must not
// start with invalid config.
package config
import (
"errors"
"flag"
"fmt"
"io"
"net/url"
"os"
"path/filepath"
"strconv"
"time"
)
// Default values for optional fields. Exposed so tests and callers can
// reference them without re-hardcoding.
const (
DefaultListen = ":8080"
DefaultForgejoOAuthScopes = "read:user write:repository write:issue write:notification read:organization"
DefaultForgejoMCPBinary = "forgejo-mcp"
DefaultStorePath = "/data/broker.db"
DefaultMaxSessions = 100
DefaultSessionIdleTimeout = 15 * time.Minute
)
// Config holds fully resolved and validated broker configuration.
type Config struct {
PublicURL string
Listen string
ForgejoURL string
ForgejoOAuthClientID string
ForgejoOAuthClientSecret string
ForgejoOAuthScopes string
ForgejoMCPBinary string
StorePath string
MaxSessions int
SessionIdleTimeout time.Duration
Debug bool
}
// Load resolves configuration from environment variables and the provided
// command-line arguments (typically os.Args[1:]), then validates the result.
// out receives flag-package output (usage text on -h / parse errors).
//
// Returns flag.ErrHelp when the user requested help — callers should treat
// that as a normal exit, not a configuration failure.
func Load(args []string, out io.Writer) (*Config, error) {
cfg, err := fromEnv()
if err != nil {
return nil, err
}
fs := flag.NewFlagSet("fjmcp-broker", flag.ContinueOnError)
fs.SetOutput(out)
cfg.bindFlags(fs)
if err := fs.Parse(args); err != nil {
return nil, err
}
if err := cfg.Validate(); err != nil {
return nil, err
}
return cfg, nil
}
func fromEnv() (*Config, error) {
cfg := &Config{
PublicURL: envOr("FJMCP_BROKER_PUBLIC_URL", ""),
Listen: envOr("FJMCP_BROKER_LISTEN", DefaultListen),
ForgejoURL: envOr("FORGEJO_URL", ""),
ForgejoOAuthClientID: envOr("FORGEJO_OAUTH_CLIENT_ID", ""),
ForgejoOAuthClientSecret: envOr("FORGEJO_OAUTH_CLIENT_SECRET", ""),
ForgejoOAuthScopes: envOr("FORGEJO_OAUTH_SCOPES", DefaultForgejoOAuthScopes),
ForgejoMCPBinary: envOr("FJMCP_BROKER_MCP_BINARY", DefaultForgejoMCPBinary),
StorePath: envOr("FJMCP_BROKER_STORE", DefaultStorePath),
MaxSessions: DefaultMaxSessions,
SessionIdleTimeout: DefaultSessionIdleTimeout,
}
if v, ok := lookupNonEmpty("FJMCP_BROKER_MAX_SESSIONS"); ok {
n, err := strconv.Atoi(v)
if err != nil {
return nil, fmt.Errorf("FJMCP_BROKER_MAX_SESSIONS: %w", err)
}
cfg.MaxSessions = n
}
if v, ok := lookupNonEmpty("FJMCP_BROKER_IDLE_TIMEOUT"); ok {
d, err := time.ParseDuration(v)
if err != nil {
return nil, fmt.Errorf("FJMCP_BROKER_IDLE_TIMEOUT: %w", err)
}
cfg.SessionIdleTimeout = d
}
if v, ok := lookupNonEmpty("FJMCP_BROKER_DEBUG"); ok {
b, err := strconv.ParseBool(v)
if err != nil {
return nil, fmt.Errorf("FJMCP_BROKER_DEBUG: %w", err)
}
cfg.Debug = b
}
return cfg, nil
}
func (c *Config) bindFlags(fs *flag.FlagSet) {
fs.StringVar(&c.PublicURL, "public-url", c.PublicURL,
"Public issuer URL, e.g. https://mcp.example.com (REQUIRED; env FJMCP_BROKER_PUBLIC_URL)")
fs.StringVar(&c.Listen, "listen", c.Listen,
"HTTP listen address (env FJMCP_BROKER_LISTEN)")
fs.StringVar(&c.ForgejoURL, "forgejo-url", c.ForgejoURL,
"Upstream Forgejo instance URL (REQUIRED; env FORGEJO_URL)")
fs.StringVar(&c.ForgejoOAuthClientID, "forgejo-oauth-client-id", c.ForgejoOAuthClientID,
"Forgejo OAuth client ID (REQUIRED; env FORGEJO_OAUTH_CLIENT_ID)")
fs.StringVar(&c.ForgejoOAuthClientSecret, "forgejo-oauth-client-secret", c.ForgejoOAuthClientSecret,
"Forgejo OAuth client secret (REQUIRED; env FORGEJO_OAUTH_CLIENT_SECRET)")
fs.StringVar(&c.ForgejoOAuthScopes, "forgejo-oauth-scopes", c.ForgejoOAuthScopes,
"Space-separated OAuth scopes requested from Forgejo (env FORGEJO_OAUTH_SCOPES)")
fs.StringVar(&c.ForgejoMCPBinary, "forgejo-mcp-binary", c.ForgejoMCPBinary,
"Path to the forgejo-mcp binary (env FJMCP_BROKER_MCP_BINARY)")
fs.StringVar(&c.StorePath, "store-path", c.StorePath,
"SQLite store file path (env FJMCP_BROKER_STORE)")
fs.IntVar(&c.MaxSessions, "max-sessions", c.MaxSessions,
"Maximum concurrent MCP sessions (env FJMCP_BROKER_MAX_SESSIONS)")
fs.DurationVar(&c.SessionIdleTimeout, "session-idle-timeout", c.SessionIdleTimeout,
"Idle timeout before a session subprocess is reaped (env FJMCP_BROKER_IDLE_TIMEOUT)")
fs.BoolVar(&c.Debug, "debug", c.Debug,
"Verbose logging (env FJMCP_BROKER_DEBUG)")
}
// Validate checks that required fields are present and all values are
// internally consistent. Errors are aggregated so the operator sees every
// problem at once.
func (c *Config) Validate() error {
var errs []error
if c.PublicURL == "" {
errs = append(errs, errors.New("public-url is required (--public-url or FJMCP_BROKER_PUBLIC_URL)"))
}
if c.ForgejoURL == "" {
errs = append(errs, errors.New("forgejo-url is required (--forgejo-url or FORGEJO_URL)"))
}
if c.ForgejoOAuthClientID == "" {
errs = append(errs, errors.New("forgejo-oauth-client-id is required (--forgejo-oauth-client-id or FORGEJO_OAUTH_CLIENT_ID)"))
}
if c.ForgejoOAuthClientSecret == "" {
errs = append(errs, errors.New("forgejo-oauth-client-secret is required (--forgejo-oauth-client-secret or FORGEJO_OAUTH_CLIENT_SECRET)"))
}
if c.PublicURL != "" {
if err := validateIssuerURL("public-url", c.PublicURL); err != nil {
errs = append(errs, err)
}
}
if c.ForgejoURL != "" {
if err := validateIssuerURL("forgejo-url", c.ForgejoURL); err != nil {
errs = append(errs, err)
}
}
if c.StorePath == "" {
errs = append(errs, errors.New("store-path must not be empty"))
} else if err := checkStorePathWritable(c.StorePath); err != nil {
errs = append(errs, fmt.Errorf("store-path: %w", err))
}
if c.Listen == "" {
errs = append(errs, errors.New("listen must not be empty"))
}
if c.MaxSessions <= 0 {
errs = append(errs, fmt.Errorf("max-sessions must be > 0, got %d", c.MaxSessions))
}
if c.SessionIdleTimeout <= 0 {
errs = append(errs, fmt.Errorf("session-idle-timeout must be > 0, got %s", c.SessionIdleTimeout))
}
return errors.Join(errs...)
}
// validateIssuerURL rejects malformed URLs, enforces http/https scheme, and
// disallows plain http for non-loopback hosts. Publishing an http issuer for
// a non-loopback host is a classic OAuth misconfiguration that undermines
// the whole point of TLS on the AS.
func validateIssuerURL(field, raw string) error {
u, err := url.Parse(raw)
if err != nil {
return fmt.Errorf("%s: %w", field, err)
}
if u.Scheme != "http" && u.Scheme != "https" {
return fmt.Errorf("%s: scheme must be http or https, got %q", field, u.Scheme)
}
if u.Host == "" {
return fmt.Errorf("%s: missing host in %q", field, raw)
}
if u.Scheme == "http" && !isLoopback(u.Hostname()) {
return fmt.Errorf("%s: http scheme only allowed for loopback hosts, got %q", field, u.Hostname())
}
return nil
}
func isLoopback(host string) bool {
switch host {
case "localhost", "127.0.0.1", "::1":
return true
}
return false
}
// checkStorePathWritable verifies the store's parent directory exists and
// the current process can create files in it. The store file itself is not
// created here — that is the storage layer's responsibility.
func checkStorePathWritable(path string) error {
abs, err := filepath.Abs(path)
if err != nil {
return fmt.Errorf("invalid path %q: %w", path, err)
}
dir := filepath.Dir(abs)
info, err := os.Stat(dir)
if err != nil {
return fmt.Errorf("parent directory %q: %w", dir, err)
}
if !info.IsDir() {
return fmt.Errorf("parent %q is not a directory", dir)
}
f, err := os.CreateTemp(dir, ".fjmcp-broker-probe-*")
if err != nil {
return fmt.Errorf("parent directory %q not writable: %w", dir, err)
}
name := f.Name()
_ = f.Close()
_ = os.Remove(name)
return nil
}
func envOr(key, fallback string) string {
if v, ok := lookupNonEmpty(key); ok {
return v
}
return fallback
}
// lookupNonEmpty returns the env value and true only when the variable is
// set AND non-empty. An exported-but-empty variable is treated as unset,
// matching common shell usage.
func lookupNonEmpty(key string) (string, bool) {
v, ok := os.LookupEnv(key)
if !ok || v == "" {
return "", false
}
return v, true
}

View file

@ -0,0 +1,335 @@
package config_test
import (
"bytes"
"errors"
"flag"
"io"
"path/filepath"
"strings"
"testing"
"time"
"kode.naiv.no/olemd/forgejo-mcp-broker/internal/config"
)
// envVars enumerates every environment variable read by the config package.
// Tests clear all of these to isolate from the ambient shell environment.
var envVars = []string{
"FJMCP_BROKER_PUBLIC_URL",
"FJMCP_BROKER_LISTEN",
"FORGEJO_URL",
"FORGEJO_OAUTH_CLIENT_ID",
"FORGEJO_OAUTH_CLIENT_SECRET",
"FORGEJO_OAUTH_SCOPES",
"FJMCP_BROKER_MCP_BINARY",
"FJMCP_BROKER_STORE",
"FJMCP_BROKER_MAX_SESSIONS",
"FJMCP_BROKER_IDLE_TIMEOUT",
"FJMCP_BROKER_DEBUG",
}
// clearEnv wipes every known env var for the test's lifetime. Setting to ""
// is sufficient because the config treats empty values as unset.
func clearEnv(t *testing.T) {
t.Helper()
for _, k := range envVars {
t.Setenv(k, "")
}
}
// validBaseArgs returns the minimum required flag set, plus a writable
// store path pointing inside a per-test TempDir.
func validBaseArgs(t *testing.T) []string {
t.Helper()
storePath := filepath.Join(t.TempDir(), "broker.db")
return []string{
"--public-url", "https://mcp.example.com",
"--forgejo-url", "https://forgejo.example.com",
"--forgejo-oauth-client-id", "client-id",
"--forgejo-oauth-client-secret", "client-secret",
"--store-path", storePath,
}
}
func TestLoad_HappyPath_Defaults(t *testing.T) {
clearEnv(t)
cfg, err := config.Load(validBaseArgs(t), io.Discard)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if cfg.Listen != config.DefaultListen {
t.Errorf("Listen = %q, want %q", cfg.Listen, config.DefaultListen)
}
if cfg.ForgejoOAuthScopes != config.DefaultForgejoOAuthScopes {
t.Errorf("ForgejoOAuthScopes = %q, want %q", cfg.ForgejoOAuthScopes, config.DefaultForgejoOAuthScopes)
}
if cfg.ForgejoMCPBinary != config.DefaultForgejoMCPBinary {
t.Errorf("ForgejoMCPBinary = %q, want %q", cfg.ForgejoMCPBinary, config.DefaultForgejoMCPBinary)
}
if cfg.MaxSessions != config.DefaultMaxSessions {
t.Errorf("MaxSessions = %d, want %d", cfg.MaxSessions, config.DefaultMaxSessions)
}
if cfg.SessionIdleTimeout != config.DefaultSessionIdleTimeout {
t.Errorf("SessionIdleTimeout = %s, want %s", cfg.SessionIdleTimeout, config.DefaultSessionIdleTimeout)
}
if cfg.Debug {
t.Error("Debug = true, want false by default")
}
}
func TestLoad_Env_PopulatesAllFields(t *testing.T) {
clearEnv(t)
storePath := filepath.Join(t.TempDir(), "broker.db")
t.Setenv("FJMCP_BROKER_PUBLIC_URL", "https://mcp.example.com")
t.Setenv("FJMCP_BROKER_LISTEN", ":9090")
t.Setenv("FORGEJO_URL", "https://forgejo.example.com")
t.Setenv("FORGEJO_OAUTH_CLIENT_ID", "envcid")
t.Setenv("FORGEJO_OAUTH_CLIENT_SECRET", "envcs")
t.Setenv("FORGEJO_OAUTH_SCOPES", "read:user")
t.Setenv("FJMCP_BROKER_MCP_BINARY", "/opt/forgejo-mcp")
t.Setenv("FJMCP_BROKER_STORE", storePath)
t.Setenv("FJMCP_BROKER_MAX_SESSIONS", "42")
t.Setenv("FJMCP_BROKER_IDLE_TIMEOUT", "5m")
t.Setenv("FJMCP_BROKER_DEBUG", "true")
cfg, err := config.Load(nil, io.Discard)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
want := &config.Config{
PublicURL: "https://mcp.example.com",
Listen: ":9090",
ForgejoURL: "https://forgejo.example.com",
ForgejoOAuthClientID: "envcid",
ForgejoOAuthClientSecret: "envcs",
ForgejoOAuthScopes: "read:user",
ForgejoMCPBinary: "/opt/forgejo-mcp",
StorePath: storePath,
MaxSessions: 42,
SessionIdleTimeout: 5 * time.Minute,
Debug: true,
}
if *cfg != *want {
t.Errorf("cfg mismatch\n got: %+v\nwant: %+v", *cfg, *want)
}
}
func TestLoad_FlagBeatsEnv(t *testing.T) {
clearEnv(t)
storePath := filepath.Join(t.TempDir(), "broker.db")
t.Setenv("FJMCP_BROKER_PUBLIC_URL", "https://env.example.com")
t.Setenv("FORGEJO_URL", "https://env-forgejo.example.com")
t.Setenv("FORGEJO_OAUTH_CLIENT_ID", "env-cid")
t.Setenv("FORGEJO_OAUTH_CLIENT_SECRET", "env-cs")
t.Setenv("FJMCP_BROKER_STORE", storePath)
t.Setenv("FJMCP_BROKER_MAX_SESSIONS", "10")
cfg, err := config.Load([]string{
"--public-url", "https://flag.example.com",
"--max-sessions", "77",
}, io.Discard)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if cfg.PublicURL != "https://flag.example.com" {
t.Errorf("PublicURL = %q, want flag value", cfg.PublicURL)
}
if cfg.MaxSessions != 77 {
t.Errorf("MaxSessions = %d, want 77", cfg.MaxSessions)
}
// Unoverridden fields should still come from env:
if cfg.ForgejoOAuthClientID != "env-cid" {
t.Errorf("ForgejoOAuthClientID = %q, want env-cid", cfg.ForgejoOAuthClientID)
}
}
func TestLoad_EmptyEnvTreatedAsUnset(t *testing.T) {
// An exported-but-empty variable must not clobber the default. This matches
// the shell idiom where `export FOO=` is effectively a reset.
clearEnv(t)
t.Setenv("FJMCP_BROKER_LISTEN", "") // empty => use default
cfg, err := config.Load(validBaseArgs(t), io.Discard)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if cfg.Listen != config.DefaultListen {
t.Errorf("Listen = %q, want default %q", cfg.Listen, config.DefaultListen)
}
}
func TestLoad_MissingRequired_ListsAllFields(t *testing.T) {
clearEnv(t)
_, err := config.Load(nil, io.Discard)
if err == nil {
t.Fatal("expected error, got nil")
}
msg := err.Error()
for _, want := range []string{
"public-url",
"forgejo-url",
"forgejo-oauth-client-id",
"forgejo-oauth-client-secret",
} {
if !strings.Contains(msg, want) {
t.Errorf("error message missing %q: %s", want, msg)
}
}
}
func TestLoad_InvalidIssuerURL(t *testing.T) {
cases := []struct {
name string
url string
want string
}{
{"bad_scheme", "ftp://example.com", "scheme must be http or https"},
{"http_non_loopback", "http://example.com", "http scheme only allowed for loopback"},
{"no_host", "http:///path", "missing host"},
{"no_scheme", "example.com", "scheme must be http or https"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
clearEnv(t)
args := validBaseArgs(t)
// Replace the public-url value in args:
for i, a := range args {
if a == "--public-url" && i+1 < len(args) {
args[i+1] = tc.url
}
}
_, err := config.Load(args, io.Discard)
if err == nil || !strings.Contains(err.Error(), tc.want) {
t.Errorf("want error containing %q, got %v", tc.want, err)
}
})
}
}
func TestLoad_IssuerURL_LoopbackHTTPAllowed(t *testing.T) {
// Local development should not require a TLS cert. Validate that the
// loopback escape hatch works for the three canonical forms.
for _, host := range []string{"http://localhost:8080", "http://127.0.0.1", "http://[::1]:9000"} {
t.Run(host, func(t *testing.T) {
clearEnv(t)
args := validBaseArgs(t)
for i, a := range args {
if a == "--public-url" && i+1 < len(args) {
args[i+1] = host
}
}
if _, err := config.Load(args, io.Discard); err != nil {
t.Errorf("loopback http should be allowed, got: %v", err)
}
})
}
}
func TestLoad_InvalidStorePath(t *testing.T) {
t.Run("empty", func(t *testing.T) {
clearEnv(t)
args := validBaseArgs(t)
for i, a := range args {
if a == "--store-path" && i+1 < len(args) {
args[i+1] = ""
}
}
_, err := config.Load(args, io.Discard)
if err == nil || !strings.Contains(err.Error(), "store-path") {
t.Errorf("want store-path error, got %v", err)
}
})
t.Run("nonexistent_parent", func(t *testing.T) {
clearEnv(t)
args := validBaseArgs(t)
bogus := filepath.Join(t.TempDir(), "definitely", "not", "here", "broker.db")
for i, a := range args {
if a == "--store-path" && i+1 < len(args) {
args[i+1] = bogus
}
}
_, err := config.Load(args, io.Discard)
if err == nil || !strings.Contains(err.Error(), "store-path") {
t.Errorf("want store-path error, got %v", err)
}
})
}
func TestLoad_NumericBounds(t *testing.T) {
cases := []struct {
name string
args []string
want string
}{
{"max_sessions_zero", []string{"--max-sessions", "0"}, "max-sessions must be > 0"},
{"max_sessions_negative", []string{"--max-sessions", "-5"}, "max-sessions must be > 0"},
{"idle_timeout_zero", []string{"--session-idle-timeout", "0"}, "session-idle-timeout must be > 0"},
{"idle_timeout_negative", []string{"--session-idle-timeout", "-10s"}, "session-idle-timeout must be > 0"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
clearEnv(t)
args := append(validBaseArgs(t), tc.args...)
_, err := config.Load(args, io.Discard)
if err == nil || !strings.Contains(err.Error(), tc.want) {
t.Errorf("want %q, got %v", tc.want, err)
}
})
}
}
func TestLoad_BadEnvParse(t *testing.T) {
cases := []struct {
envKey string
value string
want string
}{
{"FJMCP_BROKER_MAX_SESSIONS", "not-a-number", "FJMCP_BROKER_MAX_SESSIONS"},
{"FJMCP_BROKER_IDLE_TIMEOUT", "forever", "FJMCP_BROKER_IDLE_TIMEOUT"},
{"FJMCP_BROKER_DEBUG", "maybe", "FJMCP_BROKER_DEBUG"},
}
for _, tc := range cases {
t.Run(tc.envKey, func(t *testing.T) {
clearEnv(t)
t.Setenv(tc.envKey, tc.value)
_, err := config.Load(validBaseArgs(t), io.Discard)
if err == nil || !strings.Contains(err.Error(), tc.want) {
t.Errorf("want error containing %q, got %v", tc.want, err)
}
})
}
}
func TestLoad_Help_ListsAllFlagsAndEnvVars(t *testing.T) {
clearEnv(t)
var buf bytes.Buffer
_, err := config.Load([]string{"-help"}, &buf)
if !errors.Is(err, flag.ErrHelp) {
t.Fatalf("want flag.ErrHelp, got %v", err)
}
out := buf.String()
wantFlags := []string{
"-public-url", "-listen", "-forgejo-url",
"-forgejo-oauth-client-id", "-forgejo-oauth-client-secret", "-forgejo-oauth-scopes",
"-forgejo-mcp-binary", "-store-path",
"-max-sessions", "-session-idle-timeout", "-debug",
}
for _, f := range wantFlags {
if !strings.Contains(out, f) {
t.Errorf("help missing flag %q", f)
}
}
wantEnv := []string{
"FJMCP_BROKER_PUBLIC_URL", "FORGEJO_URL",
"FORGEJO_OAUTH_CLIENT_ID", "FORGEJO_OAUTH_CLIENT_SECRET",
"FJMCP_BROKER_STORE", "FJMCP_BROKER_IDLE_TIMEOUT",
}
for _, e := range wantEnv {
if !strings.Contains(out, e) {
t.Errorf("help missing env var %q", e)
}
}
}

View file

@ -1,5 +0,0 @@
// Package config loads broker configuration from flags and environment
// variables, applies defaults, and validates the result.
//
// Implementation lands in forgejo-mcp-broker-9nq.
package config