diff --git a/main.go b/main.go index c0d3a6a..982bbb4 100644 --- a/main.go +++ b/main.go @@ -23,6 +23,20 @@ import ( "gorm.io/gorm" ) +// corsSettings is a function so it can be tested. +// +// Inline, it could only be checked by starting the server and pointing a real +// browser at it — which is how the missing Authorization header reached +// production in the first place. +func corsSettings() cors.Config { + return cors.Config{ + AllowHeaders: "Origin,Content-Type,Accept,Content-Length,Accept-Language,Accept-Encoding,Connection,Authorization", + AllowOrigins: "*", + AllowCredentials: false, + AllowMethods: "GET,POST,HEAD,PUT,DELETE,PATCH,OPTIONS", + } +} + func main() { // Loads `.env.` (default `.env.local`) and `.env`, then checks // every required setting at once. Nothing below runs against a half @@ -31,12 +45,43 @@ func main() { app := fiber.New() - app.Use(cors.New(cors.Config{ - AllowHeaders: "Origin,Content-Type,Accept,Content-Length,Accept-Language,Accept-Encoding,Connection,Access-Control-Allow-Origin", - AllowOrigins: "*", - AllowCredentials: true, - AllowMethods: "GET,POST,HEAD,PUT,DELETE,PATCH,OPTIONS", - })) + // Cross-origin access. + // + // The console is served from app.nearledaily.com and calls this host + // directly, so every request it makes is cross-origin and the browser + // decides whether to allow it from the headers below. + // + // ── Authorization has to be listed ────────────────────────────────────── + // + // It was not, and adding the session token to the console broke every call + // the moment it shipped. A request carrying `Authorization` is no longer a + // "simple" request, so the browser stops and asks permission first — and the + // answer has to name that header explicitly. It was never needed before + // because the console sent nothing but `Accept` and `Content-Type`. + // + // The failure is worth recognising again: the preflight returns 204 and + // looks healthy in a terminal, the server logs nothing, and only the browser + // refuses. `curl` cannot reproduce it, because curl does not enforce CORS. + // + // ── Credentials off, wildcard on ──────────────────────────────────────── + // + // `AllowOrigins: "*"` with `AllowCredentials: true` is not a valid pair: a + // browser rejects a credentialed response that carries a wildcard origin. + // That combination was here already and was harmless only because nothing + // used credentials — it would have become a second, identical-looking bug + // the day anything did. + // + // Credentials means cookies and TLS client certs, and this backend uses + // neither: authentication is a Bearer token, which is an ordinary header and + // needs no credentialed mode. Nothing in the console, the app or the POS + // sets `credentials: 'include'`, so turning it off costs nothing and makes + // the pair legal. + // + // The wildcard itself is worth revisiting — it lets any site on the internet + // call this API from a browser, and the tenant guard is what stops that + // mattering. Narrowing it to the known console origins is a separate change, + // and one that breaks local development if the list is got wrong. + app.Use(cors.New(corsSettings())) fmt.Println("🌐 Connecting to databases...") db.Connect(cfg) diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..ceec87c --- /dev/null +++ b/main_test.go @@ -0,0 +1,113 @@ +package main + +import ( + "net/http/httptest" + "strings" + "testing" + + "github.com/gofiber/fiber/v2" + "github.com/gofiber/fiber/v2/middleware/cors" +) + +// Cross-origin access, checked the way a browser checks it. +// +// These exist because this went wrong in production and nothing caught it. +// Adding the session token to the console made every request non-simple, so +// browsers began asking permission first — and the answer did not name the +// `Authorization` header, so every call was blocked. +// +// The reason it reached production is worth keeping in mind while reading +// these: the preflight returns 204 and looks perfectly healthy from a terminal, +// the server logs nothing unusual, and `curl` cannot reproduce it because curl +// does not enforce CORS. The only thing that noticed was a browser. + +// preflight asks the question a browser asks before a cross-origin request. +func preflight(t *testing.T, requestHeaders string) map[string]string { + t.Helper() + + app := fiber.New() + app.Use(cors.New(corsSettings())) + app.Get("/probe", func(c *fiber.Ctx) error { return c.SendStatus(fiber.StatusOK) }) + + req := httptest.NewRequest("OPTIONS", "/probe", nil) + req.Header.Set("Origin", "https://app.nearledaily.com") + req.Header.Set("Access-Control-Request-Method", "GET") + if requestHeaders != "" { + req.Header.Set("Access-Control-Request-Headers", requestHeaders) + } + + resp, err := app.Test(req, -1) + if err != nil { + t.Fatalf("preflight: %v", err) + } + + out := map[string]string{} + for _, name := range []string{ + "Access-Control-Allow-Origin", + "Access-Control-Allow-Headers", + "Access-Control-Allow-Methods", + "Access-Control-Allow-Credentials", + } { + out[name] = resp.Header.Get(name) + } + return out +} + +func TestTheBrowserIsAllowedToSendTheSessionToken(t *testing.T) { + // The bug itself. Without `Authorization` in this list the console cannot + // make a single authenticated call, and the error surfaces only in a + // browser console as a CORS failure. + headers := preflight(t, "authorization")["Access-Control-Allow-Headers"] + + if !strings.Contains(strings.ToLower(headers), "authorization") { + t.Fatalf("the console may not send its session token: %q", headers) + } +} + +func TestTheHeadersTheConsoleAlreadySentStillWork(t *testing.T) { + // Adding one header must not quietly drop the others. + headers := strings.ToLower(preflight(t, "content-type")["Access-Control-Allow-Headers"]) + + for _, needed := range []string{"content-type", "accept", "origin"} { + if !strings.Contains(headers, needed) { + t.Fatalf("%q is no longer allowed: %q", needed, headers) + } + } +} + +func TestAWildcardOriginIsNotPairedWithCredentials(t *testing.T) { + // Not a valid combination: a browser rejects a credentialed response + // carrying a wildcard origin. It was here already and was harmless only + // because nothing used credentials — it would have become a second bug + // that looked exactly like the first, the day anything did. + got := preflight(t, "authorization") + + if got["Access-Control-Allow-Origin"] == "*" && + strings.EqualFold(got["Access-Control-Allow-Credentials"], "true") { + t.Fatal("wildcard origin with credentials allowed — browsers reject this pair") + } +} + +func TestEveryMethodTheConsoleUsesIsAllowed(t *testing.T) { + // The console writes with POST, PUT and DELETE. A missing one fails only + // on the screens that use it, which is the kind of gap that ships. + methods := strings.ToUpper(preflight(t, "authorization")["Access-Control-Allow-Methods"]) + + for _, method := range []string{"GET", "POST", "PUT", "DELETE", "OPTIONS"} { + if !strings.Contains(methods, method) { + t.Fatalf("%s is not allowed cross-origin: %q", method, methods) + } + } +} + +func TestAResponseHeaderIsNotListedAsAnAllowedRequestHeader(t *testing.T) { + // `Access-Control-Allow-Origin` was in the allowed REQUEST headers, which is + // a category error: it is something the server sends back, never something a + // browser asks to send. Harmless, but it reads as though somebody added + // names until the error went away. + headers := strings.ToLower(preflight(t, "authorization")["Access-Control-Allow-Headers"]) + + if strings.Contains(headers, "access-control-allow-origin") { + t.Fatalf("a response header is listed as an allowed request header: %q", headers) + } +} diff --git a/middleware/webauth.go b/middleware/webauth.go index 59e1077..6e718ab 100644 --- a/middleware/webauth.go +++ b/middleware/webauth.go @@ -65,21 +65,29 @@ const WebLocalsKey = "webclaims" // webAuthRequired reports whether a request without a valid token is refused. // -// Defaults to OFF, for the same reason POS enforcement does: the console is in -// use by real merchants right now, and its sign-in does not yet hand back a -// token. Switching enforcement on before the console sends one would lock every -// user out of a working product. +// Defaults to ON. It did not always: this shipped defaulting to off, because +// the console was live and its sign-in did not yet hand back a token, so +// enforcing first would have locked every merchant out of a working product. // -// So the order is: this middleware ships, sign-in starts issuing tokens, the -// console starts sending them, and `WEB_AUTH_REQUIRED=true` closes the door. -// While it is off a token is still VERIFIED when one is sent, and a request -// carrying a token for the wrong tenant is still refused — the flag only -// decides what happens to a request carrying none. +// That rollout is finished. Sign-in mints a token, the console sends it on +// every call, and it expires cleanly. Leaving the default off after that point +// was not caution, it was an open door nobody had got round to shutting — and +// it was measured wide open: a `getorders` with no credential at all returned a +// real merchant's orders to anyone on the internet. // -// This is a temporary state and should be short. An unauthenticated `/web` -// surface is the most serious thing in this codebase. +// ── The way out, if this goes wrong ───────────────────────────────────────── +// +// `WEB_AUTH_REQUIRED=false` restores the old behaviour, immediately and without +// a deploy. That is the escape hatch, and it exists because flipping a default +// that can lock people out should always be reversible by one person in one +// minute. A token that is SENT is still always verified either way — the flag +// only decides what happens to a request carrying none. func webAuthRequired() bool { - return strings.EqualFold(strings.TrimSpace(os.Getenv("WEB_AUTH_REQUIRED")), "true") + setting := strings.TrimSpace(os.Getenv("WEB_AUTH_REQUIRED")) + if setting == "" { + return true + } + return !strings.EqualFold(setting, "false") } // publicWebPaths are the endpoints that must work before anybody has a token. diff --git a/middleware/webauth_test.go b/middleware/webauth_test.go index 73aa604..bee23ce 100644 --- a/middleware/webauth_test.go +++ b/middleware/webauth_test.go @@ -271,3 +271,64 @@ func TestNoTokenIsNotAPlatformAccount(t *testing.T) { t.Fatal("claims were reported present on a request that carried none") } } + +/* ── The default, after the rollout ────────────────────────────────────── */ + +func TestEnforcementIsOnByDefault(t *testing.T) { + // It shipped defaulting to off so a live console could adopt tokens without + // its users being locked out. That finished, and the default was measured + // still open: a getorders with no credential returned a real merchant's + // orders to anyone. + t.Setenv("POS_TOKEN_SECRET", webTestSecret) + t.Setenv("WEB_AUTH_REQUIRED", "") + + got := call(t, fakeLocations{}, "", "GET", "/live/api/v1/web/orders/tenant/getorders?tenantid=916", "") + if got != fiber.StatusUnauthorized { + t.Fatalf("an untokened request was served with no setting present: %d", got) + } +} + +func TestEnforcementCanBeTurnedOffWithoutADeploy(t *testing.T) { + // The escape hatch. Flipping a default that can lock people out has to be + // reversible by one person in one minute. + t.Setenv("POS_TOKEN_SECRET", webTestSecret) + t.Setenv("WEB_AUTH_REQUIRED", "false") + + got := call(t, fakeLocations{}, "", "GET", "/live/api/v1/web/orders/tenant/getorders?tenantid=916", "") + if got != fiber.StatusOK { + t.Fatalf("the escape hatch does not work: %d", got) + } +} + +func TestOnlyTheWordFalseOpensTheDoor(t *testing.T) { + // A typo must fail closed. "no", "0" and "off" all look like they might + // disable it, and a deployment that meant to disable it and did not is far + // safer than one that meant to enable it and did not. + t.Setenv("POS_TOKEN_SECRET", webTestSecret) + for _, setting := range []string{"no", "0", "off", "FALSE ", "nope"} { + t.Setenv("WEB_AUTH_REQUIRED", setting) + got := call(t, fakeLocations{}, "", "GET", "/live/api/v1/web/orders/tenant/getorders?tenantid=916", "") + if setting == "FALSE " && got != fiber.StatusOK { + t.Fatalf("a trimmed, case-insensitive false was not honoured: %d", got) + } + if setting != "FALSE " && got != fiber.StatusUnauthorized { + t.Fatalf("%q opened the door: %d", setting, got) + } + } +} + +func TestSignInStillWorksWithTheNewDefault(t *testing.T) { + // The test that catches a locked-out deployment. Guarding the login route + // means nobody can ever obtain a token. + t.Setenv("POS_TOKEN_SECRET", webTestSecret) + t.Setenv("WEB_AUTH_REQUIRED", "") + + for _, path := range []string{ + "/live/api/v1/web/users/applogin", + "/live/api/v1/web/tenant/weblogin", + } { + if got := call(t, fakeLocations{}, "", "POST", path, `{"authname":"a@b.c"}`); got != fiber.StatusOK { + t.Fatalf("%s was locked behind a session: %d", path, got) + } + } +}