diff --git a/AGENTS.md b/AGENTS.md index 4ff678904..929feec75 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -195,6 +195,39 @@ Do **not** apply this mechanically to auth paths. Turning a lookup failure into Detailed rules load via skills (see below) — don't restate them here. +## Security Advisory Triage + +`SECURITY.md` promises a triage decision within 7 days. That promise was broken once, +badly: twelve advisories sat in `triage` from April to August 2026, **four of which +described live, exploitable vulnerabilities** — including token theft that had been +reported twice and flagged as an incomplete fix of an already-published CVE. Nineteen +release candidates shipped with it. + +The cost of triage is low; the cost of skipping it was that. Working the queue: + +```bash +gh api --paginate /repos/authorizerdev/authorizer/security-advisories \ + --jq '.[]|select(.state=="triage")|"\(.ghsa_id) \(.severity) \(.summary)"' +``` + +For each report, **write a probe before forming an opinion.** A ten-line `_test.go` +against current `main` settles almost every one, and a reported vulnerability is not +fixed until a test fails without the fix. Then: + +- **Confirmed** → fix it, ship the failing-test-turned-regression-test with the patch, + and only publish the advisory once a release actually contains the fix. An advisory + naming an unreleased commit tells people they are vulnerable with nowhere to go. +- **Invalid** → close it with a written reason and keep the original report under a + `
` fold. Say what would change your mind. +- Confirm the reporter's affected/patched range yourself with `git log -S ` and + `git tag --contains`. Submitted ranges are frequently wrong — three of the six + published on 2026-08-13 had a stale or missing patched version. + +**Fix a live, high-severity, unpatched issue in a private fork** (GitHub creates one +from the draft advisory), not a public PR. A public fix PR discloses the bug the moment +it is pushed, which is fine for already-public or low-severity issues and wrong for the +rest. + ## AI Agents | Agent | Model | Focus | diff --git a/Makefile b/Makefile index 17fcf62e0..0a81c5c7f 100644 --- a/Makefile +++ b/Makefile @@ -96,6 +96,7 @@ dev: --client-id=kbyuFDidLLm280LIwVFiazOqjO3ty8KH \ --client-secret=60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa \ --allowed-origins=localhost:8080,localhost:8090,localhost:9091,localhost:5173,localhost:5174 \ + --url=http://localhost:8080 \ $(DEV_FLAGS) test: diff --git a/SECURITY.md b/SECURITY.md index 120f0e3af..63fc27de9 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -28,6 +28,15 @@ If GitHub Security Advisories is unavailable, email **lakhan.m.samani@gmail.com* | Fix released for accepted critical/high-severity issues | within **30 days** of triage | | Public disclosure | coordinated with reporter, typically **90 days** after report or sooner if fix is released | +Every report reaches one of three outcomes, and none of them is silence: + +- **Confirmed** — we reproduce it with a failing test before writing a fix, and the test ships with the patch. +- **Not accepted** — we close the advisory with a written explanation of *why*, and your original report is preserved in full. If our reasoning is wrong, reopen it or file a new advisory; we would rather re-examine a close than miss a real issue. +- **Needs info** — we tell you exactly what would let us reproduce it. + +A report we cannot reproduce is not the same as a report we think is wrong, and we will say which one we mean. + + We follow [Coordinated Vulnerability Disclosure](https://en.wikipedia.org/wiki/Coordinated_vulnerability_disclosure). We will: - Confirm receipt of your report. diff --git a/cmd/authorizer_url_config_test.go b/cmd/authorizer_url_config_test.go new file mode 100644 index 000000000..4cc7be815 --- /dev/null +++ b/cmd/authorizer_url_config_test.go @@ -0,0 +1,99 @@ +package cmd + +import ( + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/config" + "github.com/authorizerdev/authorizer/internal/parsers" +) + +// TestValidateAuthorizerURLRequiresAUsableValue pins that the shipped binary +// cannot start in the header-derived-host configuration. +// +// Without --url the password-reset link, the email-verification link, the magic +// link and the JWT `iss` claim all come from request headers, so an +// unauthenticated attacker can have a victim emailed a genuine reset link +// pointing at a domain the attacker controls, then redeem the harvested token by +// replaying the same spoofed Host (CWE-640). +func TestValidateAuthorizerURLRequiresAUsableValue(t *testing.T) { + t.Run("empty is refused", func(t *testing.T) { + err := validateAuthorizerURL(&config.Config{AuthorizerURL: ""}) + require.Error(t, err) + assert.Contains(t, err.Error(), "--url is required") + }) + + t.Run("whitespace-only is refused", func(t *testing.T) { + require.Error(t, validateAuthorizerURL(&config.Config{AuthorizerURL: " "})) + }) + + // The subtle half. SetTrustedURL treats anything it cannot normalize as + // UNSET and silently resumes header derivation, so a value that merely looks + // configured would start the server in the vulnerable state. Rejecting only + // the empty string would leave that door open. + t.Run("unusable values are refused, not silently downgraded", func(t *testing.T) { + for _, bad := range []string{ + "auth.example.com", // no scheme + "//auth.example.com", // scheme-relative + "ftp://auth.example.com", // wrong scheme + "javascript:alert(1)", // not a location at all + "https://", // no host + "https://user:pw@auth.test", // user info + "not a url", // + } { + t.Run(bad, func(t *testing.T) { + require.Equal(t, "", parsers.SanitizeAuthorizerURL(bad), + "precondition: this value must be one SetTrustedURL would discard") + err := validateAuthorizerURL(&config.Config{AuthorizerURL: bad}) + require.Error(t, err, "an unusable --url must not be accepted") + assert.Contains(t, err.Error(), "not a usable canonical URL") + }) + } + }) + + t.Run("usable values are accepted", func(t *testing.T) { + for _, ok := range []string{ + "https://auth.example.com", + "http://localhost:8080", + "https://auth.example.com:8443", + "https://auth.example.com/", // trailing slash normalises away + " https://auth.example.com ", + } { + t.Run(ok, func(t *testing.T) { + assert.NoError(t, validateAuthorizerURL(&config.Config{AuthorizerURL: ok})) + }) + } + }) +} + +// TestTrustedURLBeatsEveryHeader is the property the requirement buys: once a +// usable --url is set, no request header can influence the host this server +// considers its own. Without this, requiring --url would be bookkeeping. +func TestTrustedURLBeatsEveryHeader(t *testing.T) { + parsers.SetTrustedURL("https://auth.example.com") + t.Cleanup(func() { parsers.SetTrustedURL("") }) + + req := httptest.NewRequest("GET", "http://ignored.test/forgot_password", nil) + req.Host = "evil.example" + req.Header.Set("X-Forwarded-Host", "evil.example") + req.Header.Set("X-Forwarded-Proto", "https") + req.Header.Set("X-Authorizer-URL", "https://evil.example") + + assert.Equal(t, "https://auth.example.com", parsers.GetHostFromRequest(req), + "a configured canonical URL must win over X-Authorizer-URL, X-Forwarded-Host and Host") +} + +// TestHeaderDerivationStillWorksWhenUnset guards the fallback that library +// embedders and the test suite still rely on. The shipped binary can no longer +// reach it, but it must not rot: silently returning "" here would break callers +// that construct a Config directly instead of going through cobra. +func TestHeaderDerivationStillWorksWhenUnset(t *testing.T) { + parsers.SetTrustedURL("") + + req := httptest.NewRequest("GET", "http://ignored.test/", nil) + req.Host = "embedded.test" + assert.Equal(t, "http://embedded.test", parsers.GetHostFromRequest(req)) +} diff --git a/cmd/root.go b/cmd/root.go index 87a4ebed9..7e9e3f16f 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -427,6 +427,11 @@ func runRoot(c *cobra.Command, args []string) { } } + if err := validateAuthorizerURL(&rootArgs.config); err != nil { + fmt.Fprintln(os.Stderr, err.Error()) + os.Exit(1) + } + if err := validateMCPConfig(&rootArgs.config); err != nil { fmt.Fprintln(os.Stderr, err.Error()) os.Exit(1) @@ -856,6 +861,52 @@ func runRoot(c *cobra.Command, args []string) { log.Info().Msg("Application terminated") } +// validateAuthorizerURL refuses to start without a usable canonical base URL. +// +// Without --url, every self-referential URL this server emits — the password +// reset link, the email verification link, the magic link, the JWT `iss` claim, +// the OIDC discovery and JWKS URLs — is derived from REQUEST HEADERS +// (X-Authorizer-URL, then X-Forwarded-Host, then Host). An unauthenticated +// attacker can therefore send a forgot-password request carrying their own +// Host, and the victim receives a genuine reset link pointing at the attacker's +// domain. When the victim clicks it the token is handed over, and because the +// token's `iss` is validated against that same header-derived host, the +// attacker redeems it by replaying the same spoofed Host. Full account takeover, +// no prior access, no mailbox compromise (CWE-640). +// +// Making --url mandatory is the only fix that closes the class. Validating the +// derived host against the origin allowlist would help only deployments that +// configured an explicit list, and would do nothing on the default "*" — which +// is the configuration the attack targets. +// +// This costs no supported capability. Setting --url ALREADY collapses an +// instance to a single canonical host (GetHostFromRequest returns it and ignores +// every header), so multi-host operation only ever worked on the vulnerable +// path. Verified org domains are email-domain-to-organization routing for home +// realm discovery, not HTTP virtual hosting, and are unaffected. +// +// An unusable value is rejected, not just an empty one: SetTrustedURL treats +// anything sanitizeAuthorizerURL cannot normalize as UNSET and falls back to +// headers, so "--url=auth.example.com" or "--url=https://user:pw@host" would +// otherwise start in the vulnerable configuration while looking configured. +func validateAuthorizerURL(cfg *config.Config) error { + if strings.TrimSpace(cfg.AuthorizerURL) == "" { + return fmt.Errorf("--url is required (e.g. --url=https://auth.example.com)\n\n" + + " Why: without it the password-reset, email-verification and magic-link URLs, and the\n" + + " JWT `iss` claim, are derived from request headers — so an attacker can have a victim\n" + + " emailed a genuine reset link pointing at a domain the attacker controls.\n\n" + + " Note --url is NOT --allowed-origins; you need both:\n" + + " --url this server's own address, e.g. https://auth.example.com\n" + + " --allowed-origins your apps this server may redirect to, e.g. https://app.example.com") + } + if parsers.SanitizeAuthorizerURL(cfg.AuthorizerURL) == "" { + return fmt.Errorf("--url=%q is not a usable canonical URL: it must be an absolute http(s) URL "+ + "with a host and no user info (e.g. https://auth.example.com). An unusable value is "+ + "treated as unset, which silently restores header-derived URLs", cfg.AuthorizerURL) + } + return nil +} + // validateMCPConfig refuses a configuration that would enable MCP without a // usable canonical URL. // diff --git a/internal/config/config.go b/internal/config/config.go index 3fb42b7a1..6dc5ad1a6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -21,7 +21,30 @@ type Config struct { OrganizationName string // AdminSecret is the secret for the admin AdminSecret string - // AllowedOrigins is the list of allowed origins + // AllowedOrigins is the list of origins this server is willing to send a + // user (and their tokens) BACK to, and which browser origins may call it + // cross-site. It gates redirect_uri on /authorize, password-reset and + // magic-link redirects, and CORS. + // + // Not to be confused with AuthorizerURL, which they are routinely mixed up + // with. The two answer different questions and neither substitutes for the + // other: + // + // --url what THIS server calls itself. One value, required. + // Used for the links it emails, its `iss` claim and its + // OIDC discovery document. Inbound identity. + // --allowed-origins where it may send users and tokens. A list, wildcards + // allowed. Outbound destinations. + // + // Typically --url is the Authorizer deployment (https://auth.example.com) + // and --allowed-origins are the applications (https://app.example.com). + // They usually differ; adding your app here does NOT satisfy --url, and + // setting --url does NOT let you omit this. + // + // One real interaction: when this list is empty or exactly ["*"], + // IsValidRedirectURI does not accept arbitrary destinations — it restricts + // redirects to the server's OWN host, which is derived from AuthorizerURL. + // So --url also decides where the wildcard default is allowed to redirect. AllowedOrigins []string // EnableLoginPage is the flag to enable login page @@ -259,10 +282,17 @@ type Config struct { // reset / magic-link email URLs, the JWT `iss` claim, and the OIDC // discovery/JWKS document URLs — and ALL request headers // (X-Authorizer-URL, X-Forwarded-Host, Host) are ignored for that purpose. - // Leaving it empty preserves the legacy header-based derivation for - // reverse-proxy / multi-tenant setups, but that is exactly what exposes the - // host-header-injection account-takeover class (CWE-640); operators SHOULD - // set --url in production. + // + // REQUIRED. Startup refuses an empty or unusable value + // (cmd.validateAuthorizerURL). It was optional up to 2.4.0-rc.19, and + // leaving it empty fell back to header-based derivation — which is the + // host-header-injection account-takeover class (CWE-640): an attacker sends + // a forgot-password request with their own Host and the victim receives a + // genuine reset link pointing at the attacker's domain. + // + // The header-derived fallback in parsers.GetHostFromRequest still exists for + // library embedders and tests, so it must stay correct — but no configuration + // of the shipped binary can reach it. AuthorizerURL string // ResetPasswordURL is the URL for reset password ResetPasswordURL string diff --git a/internal/e2e/org_admin_smoke_test.go b/internal/e2e/org_admin_smoke_test.go index 5e28cc447..8b806f6ef 100644 --- a/internal/e2e/org_admin_smoke_test.go +++ b/internal/e2e/org_admin_smoke_test.go @@ -46,6 +46,10 @@ func TestReleaseSmokeOrgAdmin(t *testing.T) { startServer(t, bin, []string{ "--database-type=sqlite", "--database-url=" + dbPath, + // Required since 2.4.0-rc.20: the binary refuses to start without a + // canonical URL, because deriving it from request headers is what makes + // password-reset links forgeable (CWE-640). + "--url=" + baseURL, "--jwt-type=HS256", "--jwt-secret=" + smokeJWTSecret, "--admin-secret=" + smokeAdminSecret, "--client-id=" + smokeClientID, "--client-secret=" + smokeClientSecret, diff --git a/internal/parsers/url.go b/internal/parsers/url.go index ba30639f2..fc8019c72 100644 --- a/internal/parsers/url.go +++ b/internal/parsers/url.go @@ -21,9 +21,13 @@ func GetHost(c *gin.Context) string { // accepts a connection, so no lock is needed: the write happens-before every // concurrent read. When set, ALL request headers are ignored for host // derivation, closing the host-header-injection account-takeover class -// (CWE-640). When empty (default) the legacy header-based derivation below is -// used, preserving reverse-proxy / multi-tenant deployments — operators SHOULD -// set --url; omitting it is what leaves this attack surface open. +// (CWE-640). +// +// As of 2.4.0-rc.20 the shipped binary REFUSES TO START without a usable --url +// (cmd.validateAuthorizerURL), so this is always set in a real deployment. The +// header-derived fallback below survives only for library embedders and tests, +// which construct a Config directly instead of going through cobra — it must +// stay correct, but no configuration of the binary can reach it. // ponytail: set-once-at-startup global; a mutex/atomic would only matter if we // ever reconfigured this at runtime, which we don't. var trustedURL string @@ -85,8 +89,20 @@ func GetHostFromRequest(r *http.Request) string { return scheme + "://" + host } -// sanitizeAuthorizerURL validates and sanitizes the X-Authorizer-URL header. -// Returns empty string if the URL is invalid or contains suspicious components. +// SanitizeAuthorizerURL normalizes an operator-supplied canonical URL to +// scheme+host, returning "" when the value is unusable. +// +// Exported so startup can refuse an unusable --url: SetTrustedURL treats an +// unusable value as UNSET and silently falls back to header-derived hosts, so +// "--url=auth.example.com" (no scheme) would look configured while leaving the +// deployment on exactly the path --url exists to close. +func SanitizeAuthorizerURL(raw string) string { + return sanitizeAuthorizerURL(strings.TrimSpace(raw)) +} + +// sanitizeAuthorizerURL validates and sanitizes a candidate authorizer URL +// (the --url value, or the X-Authorizer-URL header). Returns empty string if the +// URL is invalid or contains suspicious components. func sanitizeAuthorizerURL(raw string) string { u, err := url.Parse(raw) if err != nil {