diff --git a/.env.example b/.env.example index c5a5fb3..1cf1054 100644 --- a/.env.example +++ b/.env.example @@ -3,6 +3,10 @@ # In production these are injected by docker-compose via env_file; the .env # loader only runs outside production and never overrides an already-set # variable. +# +# ENV and NODE_ENV cannot be set here. Whether this file is read at all depends +# on the environment name, so it is resolved before the file is opened — set +# them in the real environment instead (docker-compose sets ENV). # ── FusionAuth (identity) ────────────────────────────────────────────────── FUSION_AUTH_HOST=https://auth.itemize.no @@ -40,7 +44,17 @@ DISCORD_SERVER_MEMBER_ROLE_ID= # ── HTTP ─────────────────────────────────────────────────────────────────── # Port to bind. PORT wins if both are set; LISTEN is kept for compatibility -# with the old deployment and with docker-compose. +# with the old deployment and with docker-compose. A bare port, :port or +# host:port are all accepted; the port must be between 1 and 65535, and 0 is +# refused because the kernel would pick a port the health check cannot find. +# +# Under docker-compose, keep this a bare port. The compose file spends LISTEN +# twice — as the host side of '${LISTEN:-3000}:3000', and, via env_file, as the +# port the process inside the container binds. The container side of that +# mapping is hardcoded to 3000, so the two only agree when LISTEN is 3000. +# Anything else publishes a port nothing is listening on, and :port or +# host:port is not a shape the ports: mapping accepts at all. Running the +# binary directly, every form above works. LISTEN=3000 # No trailing slash — OAuth redirect URIs are built by concatenation. BASE_URL=https://itemize.no diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 4f2ac9f..108d3f8 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -8,11 +8,17 @@ # the repository's history is otherwise dominated by one-at-a-time # "Bump X from A to B". Major versions still arrive on their own, so somebody # actually reads them. +# +# Updates open against dev rather than main, so they land where the rest of the +# work does and reach main through the usual pull request. Dependabot reads this +# file from the default branch regardless, so target-branch has to be set here +# on main to take effect. Security updates are exempt and still arrive on main. version: 2 updates: - package-ecosystem: gomod directory: / + target-branch: dev schedule: interval: weekly open-pull-requests-limit: 5 @@ -24,11 +30,13 @@ updates: - package-ecosystem: docker directory: / + target-branch: dev schedule: interval: weekly - package-ecosystem: github-actions directory: / + target-branch: dev schedule: interval: weekly groups: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 835554c..d2d1e5d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,9 +29,9 @@ jobs: --health-retries 10 steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - uses: actions/setup-go@41dfa10bad2bb2ae585af6ee5bb4d7d973ad74ed # v5.1.0 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod cache: true @@ -65,18 +65,18 @@ jobs: packages: write steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - uses: docker/setup-buildx-action@c47758b77c9736f4b2ef4073d4d51994fabfe349 # v3.7.1 + - uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4.2.0 - - uses: docker/login-action@9780b0c442fbb1117ed29e0efdff1e18412f7567 # v3.3.0 + - uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4.6.0 with: registry: ghcr.io username: ${{ github.actor }} password: ${{ secrets.GITHUB_TOKEN }} - id: meta - uses: docker/metadata-action@369eb591f429131d6889c46b94e711f089e6ca96 # v5.6.1 + uses: docker/metadata-action@dc802804100637a589fabce1cb79ff13a1411302 # v6.2.0 with: images: ghcr.io/${{ github.repository }} # latest on main, dev on dev — unchanged from the previous pipeline, @@ -88,7 +88,7 @@ jobs: type=raw,value=dev,enable=${{ github.ref == 'refs/heads/dev' }} type=sha,prefix=sha-,format=short - - uses: docker/build-push-action@4f58ea79222b3b9dc2c8bbdd6debcef730109a75 # v6.9.0 + - uses: docker/build-push-action@53b7df96c91f9c12dcc8a07bcb9ccacbed38856a # v7.3.0 with: context: . push: true diff --git a/assets/embed_edge_test.go b/assets/embed_edge_test.go new file mode 100644 index 0000000..c76cea2 --- /dev/null +++ b/assets/embed_edge_test.go @@ -0,0 +1,554 @@ +package assets + +// What the two filesystems contain, and what happens at their edges. +// +// The embedded tree is fixed at build time and nothing about it can be checked +// by the compiler: a file that go:embed quietly skipped, a stylesheet that was +// renamed out from under a template, or a stray editor backup that got shipped +// inside the binary all build perfectly well and only show up in production. +// Everything here is about catching that at test time instead. + +import ( + "html/template" + "io/fs" + "os" + "path" + "regexp" + "slices" + "strings" + "testing" +) + +// walkFiles lists every file (not directory) under the given roots, sorted. +func walkFiles(t *testing.T, fsys fs.FS, roots ...string) []string { + t.Helper() + + var out []string + for _, root := range roots { + err := fs.WalkDir(fsys, root, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if !d.IsDir() { + out = append(out, p) + } + return nil + }) + if err != nil { + t.Fatalf("walking %q failed: %v", root, err) + } + } + slices.Sort(out) + return out +} + +// The embedded tree must be exactly the tree on disk. +// +// go:embed silently skips files whose names begin with a dot or an underscore. +// A contributor adding _draft.html or .htaccess under assets/ sees it work in +// -dev mode, where the files are read from disk, and never learn that it is +// missing from the binary everyone else runs. Comparing the two filesystems is +// the only way that difference ever becomes visible. +func TestEmbeddedTreeMatchesTheRepository(t *testing.T) { + // The test binary runs in its own package directory, so the repository + // root — which is what the -dev filesystem is relative to — is one level + // up. t.Chdir restores the previous directory when the test ends. + t.Chdir("..") + + embeddedFiles := walkFiles(t, FS(false), "templates", "static") + diskFiles := walkFiles(t, os.DirFS("assets"), "templates", "static") + + if slices.Equal(embeddedFiles, diskFiles) { + return + } + + for _, name := range diskFiles { + if !slices.Contains(embeddedFiles, name) { + t.Errorf("%s exists on disk but is not in the binary. go:embed skips "+ + "names beginning with a dot or an underscore, so this file works "+ + "in -dev mode and is missing everywhere else.", name) + } + } + for _, name := range embeddedFiles { + if !slices.Contains(diskFiles, name) { + t.Errorf("%s is embedded but no longer exists on disk; the binary is "+ + "serving something no one can edit", name) + } + } +} + +// The embed patterns name two directories, and nothing else may come along. +// A widened pattern would pull this test file, embed.go and anything else in +// the package into the shipped binary. +func TestEmbeddedRootHoldsOnlyTheAssetDirectories(t *testing.T) { + entries, err := fs.ReadDir(FS(false), ".") + if err != nil { + t.Fatalf("reading the embed root failed: %v", err) + } + + var got []string + for _, e := range entries { + if !e.IsDir() { + t.Errorf("%s is embedded at the top level; only the templates and "+ + "static directories belong in the binary", e.Name()) + } + got = append(got, e.Name()) + } + slices.Sort(got) + + if want := []string{"static", "templates"}; !slices.Equal(got, want) { + t.Errorf("embed root = %v, want %v", got, want) + } +} + +// Every directory the server reaches for by name. An absent one is not a build +// error — fs.Glob simply returns nothing and ReadDir is treated as optional by +// the asset builder — so the site comes up unstyled or without images instead +// of failing loudly. +func TestExpectedDirectoriesArePresent(t *testing.T) { + dirs := map[string]string{ + "templates/layout": "every page render starts from the layout", + "templates/pages": "the renderer refuses to start without page templates", + "templates/partials": "pages that include partials would fail to parse", + "static/css": "the site would render unstyled", + "static/js": "client-side behaviour would be missing", + "static/img": "the logo and favicon would 404", + "static/fonts": "the page would fall back to system fonts", + } + + for dir, why := range dirs { + t.Run(dir, func(t *testing.T) { + entries, err := fs.ReadDir(FS(false), dir) + if err != nil { + t.Fatalf("%s is not embedded: %v. %s.", dir, err, why) + } + if len(entries) == 0 { + t.Errorf("%s is embedded but empty. %s.", dir, why) + } + }) + } +} + +// Nothing that is not an asset may be shipped inside the binary. Editor +// backups, macOS metadata and source maps are all things that arrive by +// accident, and once embedded they are served to anyone who guesses the path. +func TestNoJunkIsEmbedded(t *testing.T) { + // One mebibyte. The largest asset today is a 320 KB SVG; anything past + // this is a file that was committed by mistake, and every byte of it is + // carried by every deployment. + const maxFileSize = 1 << 20 + + badNames := []string{".DS_Store", "Thumbs.db", "desktop.ini"} + badSuffixes := []string{ + ".map", // a source map exposes the unminified original + ".bak", ".orig", ".rej", ".swp", ".swo", "~", // editor and merge leftovers + ".go", // no source belongs in the asset tree + ".psd", ".ai", ".sketch", // design sources; large and useless at runtime + } + + fsys := FS(false) + err := fs.WalkDir(fsys, ".", func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + return nil + } + + name := path.Base(p) + if slices.Contains(badNames, name) { + t.Errorf("%s is embedded; it is metadata, not an asset", p) + } + for _, suffix := range badSuffixes { + if strings.HasSuffix(name, suffix) { + t.Errorf("%s is embedded and ends in %q, which is not something "+ + "the site serves on purpose", p, suffix) + } + } + if strings.HasPrefix(name, "#") { + t.Errorf("%s looks like an Emacs autosave file", p) + } + + info, err := d.Info() + if err != nil { + return err + } + if info.Size() == 0 { + t.Errorf("%s is embedded but empty; a truncated asset is served as a "+ + "blank page rather than a 404, which is far harder to notice", p) + } + if info.Size() > maxFileSize { + t.Errorf("%s is %d bytes, over the %d byte limit this test pins. Every "+ + "deployment carries it; if it really belongs in the binary, raise "+ + "the limit deliberately.", p, info.Size(), maxFileSize) + } + return nil + }) + if err != nil { + t.Fatalf("walking the embedded filesystem failed: %v", err) + } +} + +// The production filesystem must not depend on where the process was started +// from, and the development one must. That difference is the entire point of +// the flag: a deployment that suddenly needed a working directory would fail +// only once it was running somewhere other than the build machine. +func TestFSSelection(t *testing.T) { + const probe = "templates/layout/base.html" + + t.Run("embedded is independent of the working directory", func(t *testing.T) { + t.Chdir(t.TempDir()) // nothing resembling the repository is here + + if _, err := fs.ReadFile(FS(false), probe); err != nil { + t.Errorf("the embedded filesystem could not read %s from an unrelated "+ + "directory: %v. Production would depend on where it was started.", + probe, err) + } + if _, err := fs.ReadFile(FS(true), probe); err == nil { + t.Error("the -dev filesystem found the templates in a directory that " + + "has none, so it is not reading from disk at all") + } + }) + + t.Run("dev reads the working copy", func(t *testing.T) { + t.Chdir("..") // the repository root, which is where -dev must be run + + disk, err := fs.ReadFile(FS(true), probe) + if err != nil { + t.Fatalf("the -dev filesystem could not read %s from the repository "+ + "root: %v. Template edits would not show up on reload.", probe, err) + } + embedded, err := fs.ReadFile(FS(false), probe) + if err != nil { + t.Fatalf("reading %s from the embed failed: %v", probe, err) + } + if string(disk) != string(embedded) { + t.Error("the -dev and embedded copies of the layout differ, so the " + + "two modes are reading different trees") + } + }) +} + +// Paths that are not in the form io/fs defines must be refused rather than +// quietly normalised. The asset server builds lookups from request paths, and +// an fs that resolved "../" or a leading slash would widen what a crafted +// request can reach beyond the two embedded directories. +func TestEmbeddedRefusesUnnormalisedPaths(t *testing.T) { + const real = "templates/layout/base.html" + fsys := FS(false) + + if _, err := fs.ReadFile(fsys, real); err != nil { + t.Fatalf("the control case failed: %v", err) + } + + for _, p := range []string{ + "/templates/layout/base.html", // absolute + "./templates/layout/base.html", // a dot segment + "templates//layout/base.html", // an empty segment + "templates/../templates/layout/base.html", // a traversal that resolves inside + "static/../../assets/embed.go", // one that resolves outside + "templates/layout/../layout/base.html", // and one in the middle + "templates/layout/base.html/", // a trailing slash + } { + t.Run(p, func(t *testing.T) { + if _, err := fs.ReadFile(fsys, p); err == nil { + t.Errorf("%q resolved to a file; io/fs paths are unrooted and "+ + "already clean, and an fs that normalises them makes every "+ + "caller responsible for doing so first", p) + } + }) + } + + // A directory is not a file, however it is asked for. + for _, p := range []string{"templates", "static/css", "."} { + if _, err := fs.ReadFile(fsys, p); err == nil { + t.Errorf("reading the directory %q as a file succeeded", p) + } + } +} + +// fs.Sub is how a caller narrows the filesystem to one subtree. embed.FS does +// not implement it natively, so the wrapper is lazy: an invalid path fails at +// once, a merely absent one fails only when read. Both have to be errors and +// neither may panic. +func TestSubFilesystems(t *testing.T) { + fsys := FS(false) + + t.Run("a subtree serves its own paths", func(t *testing.T) { + static, err := fs.Sub(fsys, "static") + if err != nil { + t.Fatalf("fs.Sub(static) failed: %v", err) + } + if _, err := fs.ReadFile(static, "img/icon.svg"); err != nil { + t.Errorf("img/icon.svg is not readable under the static subtree: %v", err) + } + // The prefix is gone, not optional. + if _, err := fs.ReadFile(static, "static/img/icon.svg"); err == nil { + t.Error("the full path still resolves under the subtree, so fs.Sub " + + "did not narrow anything") + } + // And the subtree cannot see its siblings. + if _, err := fs.ReadFile(static, "templates/layout/base.html"); err == nil { + t.Error("the templates are reachable from the static subtree") + } + }) + + t.Run("the root subtree is the whole filesystem", func(t *testing.T) { + same, err := fs.Sub(fsys, ".") + if err != nil { + t.Fatalf("fs.Sub(.) failed: %v", err) + } + if _, err := fs.ReadFile(same, "templates/layout/base.html"); err != nil { + t.Errorf("the layout is unreadable through fs.Sub(.): %v", err) + } + }) + + t.Run("invalid subtree paths are refused", func(t *testing.T) { + for _, p := range []string{"/static", "static/", "./static", "../assets", ""} { + if _, err := fs.Sub(fsys, p); err == nil { + t.Errorf("fs.Sub(%q) was accepted; only unrooted, clean paths are "+ + "filesystem paths", p) + } + } + }) + + t.Run("an absent subtree fails on read, not on Sub", func(t *testing.T) { + missing, err := fs.Sub(fsys, "static/does-not-exist") + if err != nil { + t.Skipf("fs.Sub now validates existence (%v), which is stricter than "+ + "this test assumed", err) + } + if _, err := fs.ReadFile(missing, "anything.css"); err == nil { + t.Error("reading through a subtree that does not exist succeeded") + } + }) +} + +// templateFuncs pins the helper names the templates are allowed to call. The +// real map is built in internal/web; naming them here keeps this package from +// depending on the server, and a template calling something new fails to parse +// with a message pointing straight at this list. +var templateFuncs = []string{ + "asset", "hasAsset", "eml", "emlfallback", "csrf", + "smartTime", "dict", "list", "hasRole", +} + +func stubFuncs() template.FuncMap { + funcs := template.FuncMap{} + for _, name := range templateFuncs { + funcs[name] = func(...any) any { return nil } + } + return funcs +} + +// Every page must parse together with the layout and all the partials, the +// same combination the renderer builds. A template that does not parse is a +// 500 on that page and nothing else — the rest of the site keeps working, so +// nobody notices until someone visits it. +func TestEveryPageParses(t *testing.T) { + fsys := FS(false) + + pages, err := fs.Glob(fsys, "templates/pages/*.html") + if err != nil { + t.Fatalf("globbing pages failed: %v", err) + } + if len(pages) == 0 { + t.Fatal("no page templates are embedded; the renderer refuses to start") + } + + for _, page := range pages { + t.Run(path.Base(page), func(t *testing.T) { + _, err := template.New("base.html"). + Funcs(stubFuncs()). + ParseFS(fsys, "templates/layout/*.html", "templates/partials/*.html", page) + if err != nil { + t.Errorf("%s does not parse: %v. If the failure names an undefined "+ + "function, add it to templateFuncs here and to web.Funcs.", page, err) + } + }) + } +} + +var templateCall = regexp.MustCompile(`\{\{-?\s*template\s+"([^"]+)"`) + +// Every {{ template }} invocation has to name something that exists. Missing +// ones are only found at execution time, and only on the branch that reaches +// them — so a partial dropped from a conditional survives every render until +// the day the condition is true. +func TestEveryTemplateInvocationIsDefined(t *testing.T) { + fsys := FS(false) + + set, err := template.New("base.html"). + Funcs(stubFuncs()). + ParseFS(fsys, + "templates/layout/*.html", + "templates/partials/*.html", + "templates/pages/*.html") + if err != nil { + t.Fatalf("parsing the whole template tree failed: %v", err) + } + + var checked int + for _, name := range walkFiles(t, fsys, "templates") { + body, err := fs.ReadFile(fsys, name) + if err != nil { + t.Fatalf("reading %s failed: %v", name, err) + } + for _, match := range templateCall.FindAllStringSubmatch(string(body), -1) { + checked++ + called := match[1] + if defined := set.Lookup(called); defined == nil || defined.Tree == nil { + t.Errorf("%s invokes the template %q, which nothing defines; the "+ + "page renders until that branch is taken and then 500s", + name, called) + } + } + } + // The layout alone pulls in several partials, so finding none means the + // pattern stopped matching rather than the invocations going away. + if checked == 0 { + t.Error("no {{ template }} invocations were found in the whole tree, so " + + "this test checked nothing") + } +} + +var ( + assetCall = regexp.MustCompile(`\{\{-?\s*(?:hasAsset|asset)\s+"([^"]+)"`) + // Only literal paths: anything holding a template action is resolved at + // render time and cannot be checked here. + markupRef = regexp.MustCompile(`(?:href|src)="(/[^"{]+)"`) + manifestRef = regexp.MustCompile(`"src"\s*:\s*"([^"]+)"`) +) + +// rootFileSources mirrors the table in internal/httpx: files served from / +// because their URLs are referenced from outside our own HTML. +var rootFileSources = map[string]string{ + "/icon.svg": "static/img/icon.svg", + "/logo.png": "static/img/logo.png", + "/logo-192.png": "static/img/logo-192.png", + "/logo-512.png": "static/img/logo-512.png", + "/manifest.json": "static/manifest.json", + "/robots.txt": "static/robots.txt", + "/service-worker.js": "static/service-worker.js", +} + +// Every file the markup points at must exist. +// +// An unknown logical name does not fail: the asset resolver returns "/" + the +// name so the mistake becomes a 404 rather than a crash. That is the right +// runtime behaviour and a terrible way to find out, because the page still +// renders — unstyled, or without its logo, with nothing in the server log. +func TestReferencedFilesExist(t *testing.T) { + fsys := FS(false) + + // Logical names, as passed to the asset helper. Stylesheets and scripts + // are bundles built from a whole directory; everything else is one file + // under static/. + resolve := func(name string) (string, bool) { + switch name { + case "app.css": + matches, _ := fs.Glob(fsys, "static/css/*.css") + return "static/css/*.css", len(matches) > 0 + case "app.js": + matches, _ := fs.Glob(fsys, "static/js/*.js") + return "static/js/*.js", len(matches) > 0 + case "boot.js": + _, err := fs.Stat(fsys, "static/boot.js") + return "static/boot.js", err == nil + default: + source := "static/" + name + _, err := fs.Stat(fsys, source) + return source, err == nil + } + } + + var checked int + for _, name := range walkFiles(t, fsys, "templates") { + body, err := fs.ReadFile(fsys, name) + if err != nil { + t.Fatalf("reading %s failed: %v", name, err) + } + text := string(body) + + for _, match := range assetCall.FindAllStringSubmatch(text, -1) { + checked++ + source, ok := resolve(match[1]) + if !ok { + t.Errorf("%s asks for the asset %q, which nothing under %s "+ + "provides; the page would link a path that 404s", + name, match[1], source) + } + } + + for _, match := range markupRef.FindAllStringSubmatch(text, -1) { + ref := match[1] + if path.Ext(ref) == "" { + continue // a route, not a file + } + checked++ + source, served := rootFileSources[ref] + if !served { + t.Errorf("%s links %s, which is not one of the files served from "+ + "the site root; either add it to internal/httpx or fix the link", + name, ref) + continue + } + if _, err := fs.Stat(fsys, source); err != nil { + t.Errorf("%s links %s, which is served from %s — but that file is "+ + "not embedded: %v", name, ref, source, err) + } + } + } + + // The layout alone links the stylesheet, two scripts and the favicon, so + // an empty run means the patterns stopped matching the markup. + if checked == 0 { + t.Error("no asset references were found in any template, so this test " + + "checked nothing") + } + + // The manifest is not markup, but browsers fetch what it points at and a + // missing icon there is an install prompt with a blank square. + manifest, err := fs.ReadFile(fsys, "static/manifest.json") + if err != nil { + t.Fatalf("reading the web app manifest failed: %v", err) + } + for _, match := range manifestRef.FindAllStringSubmatch(string(manifest), -1) { + source, served := rootFileSources[match[1]] + if !served { + t.Errorf("the manifest references %s, which is not served from the "+ + "site root", match[1]) + continue + } + if _, err := fs.Stat(fsys, source); err != nil { + t.Errorf("the manifest references %s, but %s is not embedded: %v", + match[1], source, err) + } + } +} + +// The two fonts the layout preloads. The preload is guarded by hasAsset, so a +// renamed font file does not break the page — it silently stops being +// preloaded, and the first paint waits for the font instead. +func TestPreloadedFontsExist(t *testing.T) { + fsys := FS(false) + + layout, err := fs.ReadFile(fsys, "templates/layout/base.html") + if err != nil { + t.Fatalf("reading the layout failed: %v", err) + } + + fonts := regexp.MustCompile(`"(fonts/[^"]+\.woff2)"`). + FindAllStringSubmatch(string(layout), -1) + if len(fonts) == 0 { + t.Fatal("the layout preloads no fonts at all; if that is deliberate, " + + "this test should go") + } + for _, match := range fonts { + if _, err := fs.Stat(fsys, "static/"+match[1]); err != nil { + t.Errorf("the layout preloads %s, which is not embedded: %v. The "+ + "hasAsset guard means the page still renders, so this would only "+ + "show up as a slower first paint.", match[1], err) + } + } +} diff --git a/assets/templates/partials/event-form.html b/assets/templates/partials/event-form.html index b1a57da..f6119d0 100644 --- a/assets/templates/partials/event-form.html +++ b/assets/templates/partials/event-form.html @@ -68,9 +68,9 @@
-

Antall timer.

+

Antall timer. Kan stå tomt.

{{- with index .Errors "duration" }}

{{ . }}

{{ end }}
diff --git a/cmd/website/main.go b/cmd/website/main.go index a516daf..fb33ab6 100644 --- a/cmd/website/main.go +++ b/cmd/website/main.go @@ -11,6 +11,7 @@ import ( "flag" "fmt" "log/slog" + "net" "net/http" "os" "os/signal" @@ -237,16 +238,21 @@ func openEvents(cfg *config.Config, log *slog.Logger, dev bool) (events.Reposito // The container image has no shell and no curl — that is the point of a // distroless base — so the binary doubles as its own health probe. func healthcheck() int { - port := os.Getenv("PORT") - if port == "" { - port = os.Getenv("LISTEN") + // The same call the server makes, so the probe can never disagree with + // what was bound. + addr, err := config.ResolveAddr() + if err != nil { + fmt.Fprintln(os.Stderr, "healthcheck:", err) + return 1 } - if port == "" { - port = "3000" + target, err := probeURL(addr) + if err != nil { + fmt.Fprintln(os.Stderr, "healthcheck:", err) + return 1 } client := &http.Client{Timeout: 3 * time.Second} - resp, err := client.Get("http://127.0.0.1:" + port + "/healthz") + resp, err := client.Get(target) if err != nil { fmt.Fprintln(os.Stderr, "healthcheck:", err) return 1 @@ -260,6 +266,29 @@ func healthcheck() int { return 0 } +// probeURL turns a listen address into a URL that reaches the server from +// inside its own container. +// +// The address is split rather than concatenated: LISTEN=:3000 and +// PORT=0.0.0.0:3000 both bind, and pasting either after "http://127.0.0.1:" +// produced a URL that will not even parse, so -healthcheck exited 1 forever +// and the container was reported unhealthy while serving normally. +func probeURL(addr string) (string, error) { + host, port, err := net.SplitHostPort(addr) + if err != nil { + return "", fmt.Errorf("cannot probe listen address %q: %w", addr, err) + } + // A wildcard bind is not an address to connect to. It answers on loopback, + // which is the only interface the probe shares with the server for certain. + switch host { + case "", "0.0.0.0": + host = "127.0.0.1" + case "::": + host = "::1" + } + return "http://" + net.JoinHostPort(host, port) + "/healthz", nil +} + func healthz(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.Header().Set("Cache-Control", "no-store") diff --git a/cmd/website/run_test.go b/cmd/website/run_test.go new file mode 100644 index 0000000..ab2b660 --- /dev/null +++ b/cmd/website/run_test.go @@ -0,0 +1,479 @@ +package main + +// No test in this file may call t.Parallel: they use t.Setenv and t.Chdir, +// both of which panic in a parallel test, and TestServeShutsDownOnSIGTERM +// signals the whole process. + +import ( + "bytes" + "io" + "log/slog" + "net" + "net/http" + "os" + "os/exec" + "os/signal" + "strings" + "syscall" + "testing" + "time" + + "github.com/ItemizeNTNU/website/internal/config" +) + +// serverEnv is every variable config.Load consults. Clearing all of them is +// what stops a contributor's own exported BASE_URL or MONGO_DB_URL from +// deciding whether a case here passes. +var serverEnv = []string{ + "NODE_ENV", "ENV", + "PORT", "LISTEN", + "BASE_URL", + "FUSION_AUTH_HOST", + "FUSION_AUTH_CLIENT_ID", + "FUSION_AUTH_CLIENT_SECRET", + "FUSION_AUTH_SECRET", + "FUSION_AUTH_API_TOKEN", + "FUSION_AUTH_ID_TOKEN_ALG", + "FUSION_AUTH_ID_TOKEN_HMAC_SECRET", + "MONGO_DB_URL", + "MONGO_DB_NAME", + "DISCORD_CLIENT_ID", + "DISCORD_CLIENT_SECRET", + "DISCORD_BOT_TOKEN", + "DISCORD_SERVER_ID", + "DISCORD_SERVER_MEMBER_ROLE_ID", +} + +// withServerEnv installs env as the entire server environment and moves the +// test into an empty directory, so the ./.env that config.Load reads outside +// production is guaranteed not to exist. +func withServerEnv(t *testing.T, env map[string]string) { + t.Helper() + t.Chdir(t.TempDir()) + for _, key := range serverEnv { + t.Setenv(key, "") + } + for key, value := range env { + t.Setenv(key, value) + } +} + +// discardLogger keeps the server's own log output out of the test run. +func discardLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +// keepDefaultLogger restores the process-wide default logger afterwards. run +// calls slog.SetDefault, and a test that leaves the default pointing at its own +// handler would silently change what every later test logs. +func keepDefaultLogger(t *testing.T) { + t.Helper() + previous := slog.Default() + t.Cleanup(func() { slog.SetDefault(previous) }) +} + +// A server that cannot be configured must never reach the point of listening. +// Half-starting — binding the port, then failing on the first request because +// there is no identity provider — is what makes a bad rollout look healthy. +func TestRunRefusesToStartWithoutConfiguration(t *testing.T) { + keepDefaultLogger(t) + withServerEnv(t, nil) + + err := run(false) + if err == nil { + t.Fatal("run started the server with no configuration at all") + } + for _, want := range []string{ + "BASE_URL is required", + "FUSION_AUTH_HOST is required", + "FUSION_AUTH_CLIENT_ID is required", + "FUSION_AUTH_SECRET is required", + "MONGO_DB_URL is required", + } { + if !strings.Contains(err.Error(), want) { + t.Errorf("run's error does not mention %q, so an operator fixing the deployment has to restart to find that problem.\nfull error: %v", want, err) + } + } +} + +// The calendar is half the reason the site exists, so a database that will not +// answer is fatal in production rather than a warning — a deployment that comes +// up with no events must not be mistaken for a healthy one. +// +// The connection string here is rejected by the driver before any socket is +// opened, which is what keeps this test off the network. +func TestRunFailsOnAnUnusableDatabaseInProduction(t *testing.T) { + keepDefaultLogger(t) + withServerEnv(t, map[string]string{ + "ENV": "production", + "BASE_URL": "https://itemize.no", + "FUSION_AUTH_HOST": "https://auth.itemize.no", + "FUSION_AUTH_CLIENT_ID": "5c1b8e2a-0000-4000-8000-000000000001", + "FUSION_AUTH_CLIENT_SECRET": "client-secret", + "FUSION_AUTH_SECRET": strings.Repeat("a", 32), + // Valid enough for config.Load — it names a database — and rejected by + // the driver's own URI parser the moment it is applied. + "MONGO_DB_URL": "mongodb://localhost:27017/website?connectTimeoutMS=ikke-et-tall", + }) + + err := run(false) + if err == nil { + t.Fatal("run started in production without a database; the events page would be empty and the rollout would look successful") + } + if !strings.Contains(err.Error(), "MongoDB") && !strings.Contains(err.Error(), "mongo") { + t.Errorf("run's error does not point at the database (%v); the operator has nothing to act on", err) + } +} + +// The same unreachable database is fatal in production and a warning in +// development: a contributor has to be able to work on the content pages +// without running MongoDB at all, and the warning is the only thing telling +// them why the calendar is empty. +func TestOpenEvents(t *testing.T) { + // Rejected by the driver's URI parser, so no socket is opened either way. + cfg := &config.Config{Mongo: config.Mongo{ + URI: "mongodb://localhost:27017/website?connectTimeoutMS=ikke-et-tall", + Database: "website", + }} + + t.Run("development carries on without a database", func(t *testing.T) { + var logged bytes.Buffer + log := slog.New(slog.NewTextHandler(&logged, &slog.HandlerOptions{Level: slog.LevelDebug})) + + repo, disconnect, err := openEvents(cfg, log, true) + if err != nil { + t.Fatalf("a missing database stopped a development server from starting: %v", err) + } + if repo != nil { + t.Error("openEvents returned a repository backed by a connection that was never established") + } + if disconnect == nil { + t.Fatal("openEvents returned a nil disconnect function; run defers it unconditionally and would panic") + } + // Safe to call even though nothing was opened — run defers it before + // it knows whether there is a connection. + disconnect() + + if !strings.Contains(logged.String(), "no database") { + t.Errorf("nothing was logged about the missing database, so an empty calendar looks like a bug in the site.\nlog: %s", logged.String()) + } + }) + + t.Run("production refuses to start", func(t *testing.T) { + _, disconnect, err := openEvents(cfg, discardLogger(), false) + if err == nil { + t.Fatal("a production server started without a database; the deployment would look healthy with no events on it") + } + if disconnect == nil { + t.Fatal("openEvents returned a nil disconnect function alongside its error; run defers it before checking the error") + } + disconnect() + }) +} + +// A listen address that cannot be bound has to come back out of serve. The +// goroutine that calls ListenAndServe is the only thing that sees the error, so +// a broken hand-off here is a process that exits zero without ever serving. +func TestServeReturnsListenErrors(t *testing.T) { + // Held open for the duration so the "address already in use" case has + // something to collide with. + taken, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("taking an ephemeral port failed: %v", err) + } + t.Cleanup(func() { _ = taken.Close() }) + + tests := []struct { + name string + addr string + }{ + {name: "port out of range", addr: "127.0.0.1:99999"}, + {name: "not an address at all", addr: "ikke-en-adresse"}, + {name: "address already in use", addr: taken.Addr().String()}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := &http.Server{Addr: tt.addr, Handler: http.NewServeMux()} + t.Cleanup(func() { _ = srv.Close() }) + + done := make(chan error, 1) + go func() { done <- serve(srv, discardLogger()) }() + + select { + case err := <-done: + if err == nil { + t.Errorf("serve(%q) returned no error; the process would exit successfully having never listened", tt.addr) + } + case <-time.After(30 * time.Second): + t.Fatalf("serve(%q) never returned; a bind failure would hang the process instead of reporting it", tt.addr) + } + }) + } +} + +// The whole point of the shutdown path is that a redeploy does not cut off a +// member mid-request: SIGTERM stops the listener and lets what is in flight +// finish. If the signal is not wired up, the container is killed after the +// orchestrator's grace period instead. +func TestServeShutsDownOnSIGTERM(t *testing.T) { + // Keep a handler registered for the whole test. Without one, a SIGTERM + // that lands before serve has installed its own would be delivered with + // its default disposition and kill the test binary outright. + guard := make(chan os.Signal, 1) + signal.Notify(guard, syscall.SIGTERM) + t.Cleanup(func() { signal.Stop(guard) }) + + // Port zero: the kernel picks a free port, so this can never fail because + // something else on the machine holds a fixed one. + srv := &http.Server{Addr: "127.0.0.1:0", Handler: http.NewServeMux()} + t.Cleanup(func() { _ = srv.Close() }) + + done := make(chan error, 1) + go func() { done <- serve(srv, discardLogger()) }() + + // Signalling repeatedly rather than once removes the race with serve + // installing its handler: an early SIGTERM is absorbed by the guard above + // and simply retried. Extra signals after serve returns are harmless. + ticker := time.NewTicker(20 * time.Millisecond) + defer ticker.Stop() + deadline := time.After(30 * time.Second) + + for { + if err := syscall.Kill(syscall.Getpid(), syscall.SIGTERM); err != nil { + t.Fatalf("signalling the test process failed: %v", err) + } + select { + case err := <-done: + if err != nil { + t.Fatalf("serve returned %v on SIGTERM; a clean redeploy would be reported as a crash", err) + } + // The listener must actually be closed, not merely reported as + // shut down — otherwise the port stays held by a process that + // believes it has stopped. + if err := srv.ListenAndServe(); err != http.ErrServerClosed { + t.Errorf("the server was not left in a shut-down state (ListenAndServe returned %v)", err) + } + return + case <-ticker.C: + case <-deadline: + t.Fatal("serve did not return after SIGTERM; the orchestrator would have to kill the container") + } + } +} + +// The container health check runs the binary against itself, so PORT is read +// twice by two different pieces of code: config.ResolveAddr turns it into a +// listen address, healthcheck turns it into a URL. Every form the server binds +// has to be probeable, or the container is reported unhealthy — and eventually +// restarted — while it is serving perfectly well. +// +// The server here listens on 127.0.0.1, which every one of these forms reaches. +func TestHealthcheckProbesEveryAddressForm(t *testing.T) { + port, _ := startHealthServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + tests := []struct { + name string + port string + }{ + {name: "a bare port", port: port}, + // docker-compose and .env.example both document LISTEN, and ":3000" is + // a perfectly ordinary thing to put in it. + {name: "a leading colon", port: ":" + port}, + {name: "a full host:port", port: "127.0.0.1:" + port}, + // A wildcard bind is not an address to connect to; the probe has to + // fall back to loopback rather than dial 0.0.0.0. + {name: "a wildcard host:port", port: "0.0.0.0:" + port}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("PORT", tt.port) + t.Setenv("LISTEN", "") + + if got := healthcheck(); got != 0 { + t.Errorf("healthcheck returned %d for PORT=%q, but the server is answering; the container would be marked unhealthy and restarted while serving normally", got, tt.port) + } + }) + } +} + +// IPv6 is split out because the loopback interface is not guaranteed to exist +// on every build machine. The bracketed form is the one thing a naive +// "host:" + port concatenation gets wrong even when the host is right. +func TestHealthcheckProbesAnIPv6Address(t *testing.T) { + ln, err := net.Listen("tcp", "[::1]:0") + if err != nil { + t.Skipf("no IPv6 loopback on this machine: %v", err) + } + srv := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })} + go func() { _ = srv.Serve(ln) }() + t.Cleanup(func() { _ = srv.Close() }) + + t.Setenv("PORT", ln.Addr().String()) + t.Setenv("LISTEN", "") + + if got := healthcheck(); got != 0 { + t.Errorf("healthcheck returned %d for PORT=%q, but the server is answering there", got, ln.Addr()) + } +} + +// probeURL is what turns the bound address into something dialable. The +// wildcard cases are the ones that matter in the container: the server binds +// every interface, and the probe has to pick the one it shares with it. +func TestProbeURL(t *testing.T) { + tests := []struct { + name string + addr string + want string + wantErr bool + }{ + {name: "a bare port from PORT=3000", addr: ":3000", want: "http://127.0.0.1:3000/healthz"}, + {name: "an explicit loopback host", addr: "127.0.0.1:3000", want: "http://127.0.0.1:3000/healthz"}, + {name: "an IPv4 wildcard probes loopback", addr: "0.0.0.0:3000", want: "http://127.0.0.1:3000/healthz"}, + {name: "an IPv6 wildcard probes IPv6 loopback", addr: "[::]:3000", want: "http://[::1]:3000/healthz"}, + {name: "an IPv6 host keeps its brackets", addr: "[::1]:3000", want: "http://[::1]:3000/healthz"}, + {name: "a named host", addr: "localhost:3000", want: "http://localhost:3000/healthz"}, + // Not reachable through config.ResolveAddr any more, but probeURL must + // still refuse rather than build a URL that will not parse. + {name: "no port at all", addr: "3000", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := probeURL(tt.addr) + if tt.wantErr { + if err == nil { + t.Fatalf("probeURL(%q) = %q; an address that cannot be split must be reported, not turned into a malformed URL", tt.addr, got) + } + return + } + if err != nil { + t.Fatalf("probeURL(%q) failed: %v; the health check cannot probe an address the server binds", tt.addr, err) + } + if got != tt.want { + t.Errorf("probeURL(%q) = %q, want %q; the probe would dial the wrong place and report a serving container as unhealthy", tt.addr, got, tt.want) + } + }) + } +} + +// An unusable PORT stops the server from starting, so -healthcheck must not +// answer 0 for it either — a probe that succeeds against a configuration the +// server refuses would mark a container healthy that can never come up. +func TestHealthcheckRejectsAnUnusablePort(t *testing.T) { + for _, port := range []string{"0", "99999", "ikke-et-tall"} { + t.Run(port, func(t *testing.T) { + t.Setenv("PORT", port) + t.Setenv("LISTEN", "") + + if got := healthcheck(); got != 1 { + t.Errorf("healthcheck returned %d for PORT=%q, which the server itself refuses to bind", got, port) + } + }) + } +} + +// mainArgsEnv both marks the re-executed child process and carries the command +// line main should see. Its presence is the only thing that distinguishes the +// child from an ordinary test run. +const mainArgsEnv = "ITEMIZE_TEST_MAIN_ARGS" + +// TestMainHelperProcess is not a test. It is the entry point of the child +// process spawned by TestMainDispatch: main calls os.Exit, so the only way to +// observe what it does with a flag is from outside the process. +// +// os.Args is rewritten rather than passed on the child's command line because +// the testing package parses the real command line first, and would reject +// -healthcheck as an unknown flag before main ever registers it. +func TestMainHelperProcess(t *testing.T) { + args, ok := os.LookupEnv(mainArgsEnv) + if !ok { + t.Skip("not the re-executed child process") + } + os.Args = append([]string{"website"}, strings.Fields(args)...) + main() +} + +// main is a flag switch and an exit code, and both matter operationally: the +// container health check depends on -healthcheck exiting 0 or 1, and the +// orchestrator's restart loop depends on a failed start exiting non-zero. +func TestMainDispatch(t *testing.T) { + // A server the child can probe, and a port that is guaranteed to have + // nothing on it once the second server is shut down. + livePort, _ := startHealthServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + deadPort, shutdown := startHealthServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + shutdown() + + tests := []struct { + name string + args string + env []string + wantExit int + wantOut string + }{ + { + name: "-healthcheck reports a serving instance", + args: "-healthcheck", + env: []string{"PORT=" + livePort}, + wantExit: 0, + }, + { + name: "-healthcheck reports an instance that is not answering", + args: "-healthcheck", + env: []string{"PORT=" + deadPort}, + wantExit: 1, + wantOut: "healthcheck:", + }, + { + // No flags: main goes on to start the server, which refuses an + // empty environment. Exiting non-zero is what makes the + // orchestrator retry rather than mark the deployment healthy. + name: "a failed start exits non-zero and says why", + args: "", + wantExit: 1, + wantOut: "BASE_URL is required", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := exec.Command(os.Args[0], "-test.run=^TestMainHelperProcess$") + // A deliberately minimal environment: nothing the parent happens + // to export can configure the child by accident. + cmd.Env = append([]string{ + mainArgsEnv + "=" + tt.args, + "PATH=" + os.Getenv("PATH"), + "HOME=" + os.Getenv("HOME"), + }, tt.env...) + // An empty directory, so the .env that config.Load reads outside + // production cannot exist. + cmd.Dir = t.TempDir() + + out, err := cmd.CombinedOutput() + exit := cmd.ProcessState.ExitCode() + if exit == -1 { + t.Fatalf("the child process did not exit normally: %v\noutput:\n%s", err, out) + } + if exit != tt.wantExit { + t.Errorf("`website %s` exited %d, want %d\noutput:\n%s", tt.args, exit, tt.wantExit, out) + } + if tt.wantOut != "" && !strings.Contains(string(out), tt.wantOut) { + t.Errorf("`website %s` printed nothing about %q, so the failure is invisible in the container log\noutput:\n%s", tt.args, tt.wantOut, out) + } + // main always ends in os.Exit, so the testing package never gets + // to print its verdict. Seeing it means the helper returned + // without main taking over, and the exit code above proves + // nothing. + if strings.Contains(string(out), "PASS") { + t.Errorf("the child finished as an ordinary test run rather than through main\noutput:\n%s", out) + } + }) + } +} diff --git a/internal/api/apitest_test.go b/internal/api/apitest_test.go new file mode 100644 index 0000000..4ba3913 --- /dev/null +++ b/internal/api/apitest_test.go @@ -0,0 +1,414 @@ +package api + +// Shared fixtures and fakes for the API handler tests. Helpers only — the +// tests themselves live in events_test.go, checkin_test.go, users_test.go, +// respond_test.go and routes_test.go. +// +// These are in-package rather than in an api_test package because the wire +// contract lives as much in the unexported helpers (toDTO, truthy, num, +// yearOf) as in the handlers, and those are worth testing directly instead of +// only through whatever combination of requests happens to reach them. + +import ( + "context" + "encoding/json" + "errors" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/ItemizeNTNU/website/internal/auth" + "github.com/ItemizeNTNU/website/internal/events" + "github.com/ItemizeNTNU/website/internal/fusionauth" +) + +func discardLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +// logCapture collects what a handler logged. Some of these handlers answer a +// caller with deliberately less than they know — a storage failure served as +// "not found", so that somebody working through guessed codes cannot tell which +// of them hit — and the log line is then the only place the real failure +// appears. A test that only reads the response cannot tell that apart from +// swallowing the error entirely. +type logCapture struct { + mu sync.Mutex + buf strings.Builder +} + +func (c *logCapture) Write(p []byte) (int, error) { + c.mu.Lock() + defer c.mu.Unlock() + return c.buf.Write(p) +} + +func (c *logCapture) String() string { + c.mu.Lock() + defer c.mu.Unlock() + return c.buf.String() +} + +// captureLogger is a logger whose output a test can read back. +func captureLogger() (*slog.Logger, *logCapture) { + capture := &logCapture{} + return slog.New(slog.NewTextHandler(capture, &slog.HandlerOptions{Level: slog.LevelDebug})), capture +} + +// errRepo is a generic infrastructure failure — anything that is not one of +// the sentinel errors the handlers branch on. +var errRepo = errors.New("the database is on fire") + +// stubRepo is an in-memory events.Repository. The embedded interface supplies +// the methods no API handler should ever reach: a call to one of them is a nil +// dereference, which is a louder failure than quietly returning a zero value +// and is exactly what a handler reaching for storage it has no business +// touching deserves. +type stubRepo struct { + events.Repository + + // list is returned verbatim by List. The handler's job is deriving the + // filter and mapping the result, not filtering — so the fake deliberately + // does no filtering of its own, and the filter it was handed is recorded + // instead. + list []events.Event + public []events.Event + byCode map[string]events.Event + + listErr error + publicErr error + byCodeErr error + addErr error + + gotFilter events.Filter + gotCode string + gotAttendance events.Attendance + adds int +} + +func (s *stubRepo) List(_ context.Context, f events.Filter) ([]events.Event, error) { + s.gotFilter = f + if s.listErr != nil { + return nil, s.listErr + } + return s.list, nil +} + +func (s *stubRepo) Public(context.Context) ([]events.Event, error) { + if s.publicErr != nil { + return nil, s.publicErr + } + return s.public, nil +} + +func (s *stubRepo) ByCheckInCode(_ context.Context, code string) (*events.Event, error) { + s.gotCode = code + if s.byCodeErr != nil { + return nil, s.byCodeErr + } + e, ok := s.byCode[code] + if !ok { + return nil, events.ErrNotFound + } + return &e, nil +} + +func (s *stubRepo) AddAttendance(_ context.Context, code string, a events.Attendance) error { + s.gotCode = code + s.gotAttendance = a + s.adds++ + return s.addErr +} + +// apiConfig is what a test can vary about the API under test. The zero value +// is "like a deployment with no FusionAuth key": every user call answers 503. +type apiConfig struct { + repo events.Repository + fusion *fusionauth.Client + baseURL string + + // log defaults to a discarding one. Set it from captureLogger to assert on + // what a handler recorded. + log *slog.Logger + + // nilFusion passes a literal nil client, which is what a caller that + // forgot to wire FusionAuth would produce. Configured() has a nil check + // for exactly this, and the handlers rely on it. + nilFusion bool +} + +// newAPI builds the real routing table over injectable dependencies. +// +// Every call builds a fresh Server, and with it a fresh signupLimit rate +// limiter holding five tokens. The 429 test in users_test.go burns the whole +// allowance on its own mux; every other test must stay at five or fewer PUTs +// to /api/user per mux, or it starts seeing 429s that have nothing to do with +// what it is testing. +func newAPI(t *testing.T, cfg apiConfig) *http.ServeMux { + t.Helper() + + fusion := cfg.fusion + if fusion == nil && !cfg.nilFusion { + fusion = fusionauth.New("https://auth.example", "") + } + baseURL := cfg.baseURL + if baseURL == "" { + baseURL = "https://itemize.no" + } + + log := cfg.log + if log == nil { + log = discardLogger() + } + + mux := http.NewServeMux() + NewServer(cfg.repo, fusion, baseURL, log).Routes(mux) + return mux +} + +// fusionSpy records what reached the fake FusionAuth. Everything is behind a +// mutex because the handler runs on the server's goroutine while the test reads +// it from its own. +type fusionSpy struct { + mu sync.Mutex + calls int + path string + method string + auth string + body string +} + +func (f *fusionSpy) snapshot() fusionSpy { + f.mu.Lock() + defer f.mu.Unlock() + return fusionSpy{calls: f.calls, path: f.path, method: f.method, auth: f.auth, body: f.body} +} + +// fakeFusion points a configured FusionAuth client at handler and records what +// was asked of it — which is how the identifier-validation tests tell "refused +// here" from "refused by FusionAuth". +func fakeFusion(t *testing.T, handler http.HandlerFunc) (*fusionauth.Client, *fusionSpy) { + t.Helper() + spy := &fusionSpy{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + + spy.mu.Lock() + spy.calls++ + spy.path = r.URL.Path + spy.method = r.Method + spy.auth = r.Header.Get("Authorization") + spy.body = string(body) + spy.mu.Unlock() + + handler(w, r) + })) + t.Cleanup(srv.Close) + return fusionauth.New(srv.URL, "test-api-key"), spy +} + +// deadFusion is a client pointed at a server that is already gone: the call +// fails at the transport rather than with a status code, which is the branch a +// FusionAuth outage or a DNS failure takes. +func deadFusion(t *testing.T) *fusionauth.Client { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + url := srv.URL + srv.Close() + return fusionauth.New(url, "test-api-key") +} + +// The user fixtures. The identifiers are canonical UUIDs because +// fusionauth.ValidID refuses anything else before a request is made — a +// placeholder like "user-1" would turn every FusionAuth-backed test into an +// ErrInvalidID test by accident. +var styret = &auth.User{ + ID: "11111111-2222-4333-8444-999999999999", + Name: "Styremedlem", + FullName: "Åse Øverland", + Roles: []string{auth.RoleStyret}, +} + +var member = &auth.User{ + ID: "22222222-3333-4444-8555-666666666666", + Name: "Kari", + FullName: "Kari Nordmann", + Email: "medlem@example.no", +} + +// asUser attaches u to the request context the way the authn middleware would. +// The test mux carries no Inject middleware, so this is the only way a request +// is ever signed in. A nil user is an anonymous visitor. +func asUser(r *http.Request, u *auth.User) *http.Request { + if u == nil { + return r + } + return r.WithContext(auth.WithUser(r.Context(), u)) +} + +// do serves a request through the mux as u. +// +// Requests go through the mux rather than at a handler directly because the +// authorization middleware and the path wildcards are part of what is being +// tested: a handler reached without them sees an empty PathValue and an +// unchecked role. +func do(t *testing.T, mux *http.ServeMux, method, path string, body io.Reader, u *auth.User) *httptest.ResponseRecorder { + t.Helper() + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, asUser(httptest.NewRequest(method, path, body), u)) + return rec +} + +// getAs is do for the read endpoints, which never carry a body. +func getAs(t *testing.T, mux *http.ServeMux, path string, u *auth.User) *httptest.ResponseRecorder { + t.Helper() + return do(t, mux, http.MethodGet, path, nil, u) +} + +// putJSON is do for the registration endpoint, whose body is JSON. +func putJSON(t *testing.T, mux *http.ServeMux, path, body string, u *auth.User) *httptest.ResponseRecorder { + t.Helper() + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPut, path, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + mux.ServeHTTP(rec, asUser(req, u)) + return rec +} + +// wantStatus fails when the response carries a status other than want. The +// body is included because an unexpected status is nearly always explained by +// the message the handler put in it. +func wantStatus(t *testing.T, rec *httptest.ResponseRecorder, want int) { + t.Helper() + if rec.Code != want { + t.Fatalf("got %d, want %d; body was %s", rec.Code, want, strings.TrimSpace(rec.Body.String())) + } +} + +// wantJSON checks the Content-Type. Clients parse the body without sniffing +// it, so a response served as anything else is one they will not read at all +// — which is a different and much more confusing failure than an error status. +func wantJSON(t *testing.T, rec *httptest.ResponseRecorder) { + t.Helper() + const want = "application/json; charset=utf-8" + if got := rec.Header().Get("Content-Type"); got != want { + t.Errorf("Content-Type is %q, want %q; a JSON client will not parse this", got, want) + } +} + +// messageOf decodes the {"message": ...} envelope every status and error +// response uses. Clients key off that field, so a response that has lost the +// shape is a break even when the status code is right. +func messageOf(t *testing.T, rec *httptest.ResponseRecorder) string { + t.Helper() + var body message + decodeBody(t, rec, &body) + return body.Message +} + +func decodeBody(t *testing.T, rec *httptest.ResponseRecorder, dst any) { + t.Helper() + if err := json.Unmarshal(rec.Body.Bytes(), dst); err != nil { + t.Fatalf("the response is not JSON the client can decode: %v; body was %s", + err, strings.TrimSpace(rec.Body.String())) + } +} + +// The event fixtures. Every timestamp is fixed and in UTC so the encoded shape +// can be compared literally rather than field by field. +var ( + fixedCreated = time.Date(2098, 3, 1, 9, 0, 0, 0, time.UTC) + fixedEdited = time.Date(2098, 3, 2, 10, 30, 0, 0, time.UTC) + fixedStart = time.Date(2098, 9, 1, 17, 15, 0, 0, time.UTC) + fixedEnd = time.Date(2098, 9, 1, 20, 15, 0, 0, time.UTC) + fixedCheckIn = time.Date(2098, 9, 1, 17, 22, 0, 0, time.UTC) +) + +// testCode is the check-in credential. It is UUID-shaped because the real ones +// are, and the tests that put it in a URL want a realistic path segment. +const testCode = "3fa85f64-5717-4562-b3fc-2c963f66afa6" + +const testHexID = "507f1f77bcf86cd799439011" + +func mustObjectID(t *testing.T, hex string) bson.ObjectID { + t.Helper() + id, err := bson.ObjectIDFromHex(hex) + if err != nil { + t.Fatalf("the fixture identifier %q is not a valid ObjectID: %v", hex, err) + } + return id +} + +// pizzakveld is a fully populated event: every optional field set, so a test +// that pins the encoded shape sees all of them, and Norwegian text in the +// places a board member would actually type it. +func pizzakveld(t *testing.T) events.Event { + t.Helper() + edited := fixedEdited + return events.Event{ + ID: mustObjectID(t, testHexID), + Name: "Pizza og CTF", + Location: events.Place{Name: "Savannen", URL: "https://itemize.no/savannen"}, + RegisterURL: "https://itemize.no/pamelding", + Date: fixedStart, + Duration: 3, + End: fixedEnd, + CTF: events.Place{Name: "ItemizeCTF", URL: "https://ctf.itemize.no"}, + Info: "Ta med laptop.", + Hidden: false, + Discord: true, + DiscordEventID: "1234567890", + Created: fixedCreated, + Edited: &edited, + CheckIn: events.CheckIn{ + Code: testCode, + Attendances: []events.Attendance{{ + ID: mustObjectID(t, "507f191e810c19729de860ea"), + Name: "Kari Nordmann", + UserID: member.ID, + Registered: fixedCheckIn, + }}, + }, + } +} + +// pizzakveldFields is the encoded form of the fixture, closing brace omitted +// so the check-in block can be appended. +// +// It is written out in full rather than asserted field by field because it is +// a published contract: this JSON and the iCal feed are what things outside +// this repository depend on, and a renamed, reordered or newly-omitempty field +// breaks a consumer nobody here can see. A diff on this string is the warning. +const pizzakveldFields = `{"_id":"507f1f77bcf86cd799439011",` + + `"name":"Pizza og CTF",` + + `"location":{"name":"Savannen","url":"https://itemize.no/savannen"},` + + `"register_url":"https://itemize.no/pamelding",` + + `"date":"2098-09-01T17:15:00Z",` + + `"duration":3,` + + `"end":"2098-09-01T20:15:00Z",` + + `"ctf":{"name":"ItemizeCTF","url":"https://ctf.itemize.no"},` + + `"info":"Ta med laptop.",` + + `"hidden":false,` + + `"discord":true,` + + `"discordEventId":"1234567890",` + + `"created":"2098-03-01T09:00:00Z",` + + `"edited":"2098-03-02T10:30:00Z"` + +// pizzakveldJSON is what a caller who is not on the board gets: no check-in +// block at all. +const pizzakveldJSON = pizzakveldFields + `}` + +// pizzakveldStyretJSON is the same event for the board, carrying the check-in +// code and the attendance register. +const pizzakveldStyretJSON = pizzakveldFields + + `,"check_in":{"code":"` + testCode + `",` + + `"attendances":[{"name":"Kari Nordmann",` + + `"user_id":"22222222-3333-4444-8555-666666666666",` + + `"registered":"2098-09-01T17:22:00Z"}]}}` diff --git a/internal/api/checkin.go b/internal/api/checkin.go index c1b24b9..0f1c398 100644 --- a/internal/api/checkin.go +++ b/internal/api/checkin.go @@ -17,8 +17,17 @@ import ( // Its only caller was a page already restricted to the board, so nothing is // lost by enforcing that on the server too. func (s *Server) getCheckIn(w http.ResponseWriter, r *http.Request) { - event, err := s.events.ByCheckInCode(r.Context(), r.PathValue("code")) + code := r.PathValue("code") + event, err := s.events.ByCheckInCode(r.Context(), code) if err != nil { + // A storage failure is still answered as "not found", on purpose: + // telling the two apart would confirm to somebody working through + // guesses which of them hit a real code. It does have to be logged + // though — swallowing it made an outage on this path look to everyone, + // operators included, like a mistyped code. + if !errors.Is(err, events.ErrNotFound) { + s.log.Error("looking up the check-in code failed", "code", code, "err", err) + } writeJSON(w, http.StatusNotFound, message{"Event not found"}) return } @@ -28,6 +37,15 @@ func (s *Server) getCheckIn(w http.ResponseWriter, r *http.Request) { // postCheckIn registers the caller's attendance. func (s *Server) postCheckIn(w http.ResponseWriter, r *http.Request) { user := auth.FromRequest(r) + if user == nil { + // Unreachable through Routes, which mounts this behind + // RequireLoginAPI. It is here so the handler is safe wherever it is + // mounted: user.ID below is a nil dereference, so a future route + // registration that forgets the wrapper would turn a missing gate into + // a panic in the request goroutine rather than a 401. + writeJSON(w, http.StatusUnauthorized, message{"You are not logged in"}) + return + } code := r.PathValue("code") err := s.events.AddAttendance(r.Context(), code, events.Attendance{ diff --git a/internal/api/checkin_test.go b/internal/api/checkin_test.go new file mode 100644 index 0000000..5320a07 --- /dev/null +++ b/internal/api/checkin_test.go @@ -0,0 +1,388 @@ +package api + +import ( + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/ItemizeNTNU/website/internal/auth" + "github.com/ItemizeNTNU/website/internal/events" +) + +func checkInMux(t *testing.T, repo *stubRepo) *http.ServeMux { + t.Helper() + return newAPI(t, apiConfig{repo: repo}) +} + +func seededRepo(t *testing.T) *stubRepo { + t.Helper() + return &stubRepo{byCode: map[string]events.Event{testCode: pizzakveld(t)}} +} + +// Reading the register hands out the check-in code itself — the credential that +// registers attendance — together with every attendee's name and FusionAuth +// identifier. The previous version served that to anyone who knew a code. The +// board gate is the fix, and these are the two ways past it that must not work. +// +// The status is 401 for both, including for a signed-in member without the +// role, because clients key off the message and the previous API answered that +// way. It reads oddly next to 403 but changing it is a silent break. +func TestGetCheckInRequiresTheBoard(t *testing.T) { + for _, tc := range []struct { + name string + user *auth.User + want string + }{ + {"an anonymous caller", nil, "You are not logged in"}, + {"a member without the board role", member, "Permission denied"}, + } { + t.Run(tc.name, func(t *testing.T) { + repo := seededRepo(t) + rec := getAs(t, checkInMux(t, repo), "/api/checkin/"+testCode, tc.user) + + wantStatus(t, rec, http.StatusUnauthorized) + wantJSON(t, rec) + if got := messageOf(t, rec); got != tc.want { + t.Errorf("the refusal reads %q, want %q", got, tc.want) + } + if strings.Contains(rec.Body.String(), member.ID) { + t.Error("a refused request still leaked an attendee identifier") + } + if repo.gotCode != "" { + t.Error("storage was queried before the caller was refused; the gate " + + "must come first, not merely hide the answer") + } + }) + } +} + +// What the board actually gets: the event plus the register, in the shape the +// check-in page reads. +func TestGetCheckInReturnsTheRegister(t *testing.T) { + repo := seededRepo(t) + rec := getAs(t, checkInMux(t, repo), "/api/checkin/"+testCode, styret) + + wantStatus(t, rec, http.StatusOK) + wantJSON(t, rec) + if got := strings.TrimSuffix(rec.Body.String(), "\n"); got != pizzakveldStyretJSON { + t.Errorf("the register does not match the shape the check-in page reads.\n got: %s\nwant: %s", + got, pizzakveldStyretJSON) + } + if repo.gotCode != testCode { + t.Errorf("storage was asked for code %q, want %q", repo.gotCode, testCode) + } +} + +// An event whose register is still empty must come back with the code and +// without an attendances key — the page shows "nobody yet" from the absence, +// and an event with no code at all still has to render rather than 500. +func TestGetCheckInWithAnEmptyRegister(t *testing.T) { + repo := &stubRepo{byCode: map[string]events.Event{ + testCode: {Name: "Nytt arrangement", CheckIn: events.CheckIn{Code: testCode}}, + }} + + rec := getAs(t, checkInMux(t, repo), "/api/checkin/"+testCode, styret) + + wantStatus(t, rec, http.StatusOK) + body := strings.TrimSpace(rec.Body.String()) + if !strings.Contains(body, `"check_in":{"code":"`+testCode+`"}`) { + t.Errorf("an unused register did not encode as a bare code: %s", body) + } + if strings.Contains(body, "attendances") { + t.Errorf("an empty register emitted an attendances key: %s", body) + } +} + +// A code that matches nothing is a 404 with the JSON envelope, not the site's +// HTML error page — the check-in screen fetches this and parses the answer. +func TestGetCheckInUnknownCode(t *testing.T) { + for _, tc := range []struct { + name string + path string + }{ + {"a code that matches no event", "/api/checkin/ingen-slik-kode"}, + {"a Norwegian code", "/api/checkin/" + url.PathEscape("blåbærsyltetøy")}, + {"a single character", "/api/checkin/x"}, + {"a code that looks like a path traversal", "/api/checkin/" + url.PathEscape("../../etc/passwd")}, + } { + t.Run(tc.name, func(t *testing.T) { + rec := getAs(t, checkInMux(t, seededRepo(t)), tc.path, styret) + + wantStatus(t, rec, http.StatusNotFound) + wantJSON(t, rec) + if got := messageOf(t, rec); got != "Event not found" { + t.Errorf("the message is %q, want %q", got, "Event not found") + } + }) + } +} + +// A database that will not answer is still reported as "not found" to the +// caller: a 500 that distinguishes the two would tell somebody working through +// guessed codes which of them hit a real event during an outage. +// +// The operator is a different audience, and used not to be told at all. Every +// error became a bare 404 with no log line, so an outage on this path looked to +// everyone — the board member at the door and whoever they asked for help — +// like a mistyped code. The 404 stays; the log line is what makes it +// diagnosable. +func TestGetCheckInReportsStorageFailureAsNotFoundButLogsIt(t *testing.T) { + repo := seededRepo(t) + repo.byCodeErr = errRepo + log, capture := captureLogger() + + rec := getAs(t, newAPI(t, apiConfig{repo: repo, log: log}), "/api/checkin/"+testCode, styret) + + wantStatus(t, rec, http.StatusNotFound) + if got := messageOf(t, rec); got != "Event not found" { + t.Errorf("the message is %q, want %q", got, "Event not found") + } + if strings.Contains(rec.Body.String(), errRepo.Error()) { + t.Error("the internal failure was echoed to the caller") + } + + logged := capture.String() + if !strings.Contains(logged, "level=ERROR") { + t.Errorf("a storage failure was not logged at error level, so nothing "+ + "watching the logs sees the outage; got %q", logged) + } + if !strings.Contains(logged, errRepo.Error()) { + t.Errorf("the log line does not carry the underlying failure, which is the "+ + "only place it survives; got %q", logged) + } +} + +// The ordinary miss must stay quiet. This endpoint is the one a scan hits, and +// logging every wrong code at error level would bury the storage failure above +// in noise generated by anybody who wants to. +func TestGetCheckInDoesNotLogAnOrdinaryMiss(t *testing.T) { + log, capture := captureLogger() + + rec := getAs(t, newAPI(t, apiConfig{repo: seededRepo(t), log: log}), "/api/checkin/ingen-slik-kode", styret) + + wantStatus(t, rec, http.StatusNotFound) + if logged := capture.String(); logged != "" { + t.Errorf("a wrong code was logged as %q; a scan would drown the log", logged) + } +} + +// Registering attendance only needs the caller to be somebody — but it does +// need that, or the register fills with rows attributed to nobody and the +// endpoint becomes a way to inflate an attendance list without turning up. +func TestPostCheckInRequiresALogin(t *testing.T) { + repo := seededRepo(t) + rec := do(t, checkInMux(t, repo), http.MethodPost, "/api/checkin/"+testCode, nil, nil) + + wantStatus(t, rec, http.StatusUnauthorized) + wantJSON(t, rec) + if got := messageOf(t, rec); got != "You are not logged in" { + t.Errorf("the refusal reads %q, want %q", got, "You are not logged in") + } + if repo.adds != 0 { + t.Error("an anonymous request reached storage") + } +} + +// The same request with the middleware taken away, which is what a future route +// registration that forgets RequireLoginAPI would produce. +// +// The handler dereferences the user for the attendance it writes, so without a +// check of its own a forgotten wrapper is not a missing gate — it is a panic in +// the request goroutine on an anonymous request, which is far worse than the +// 401 that was intended. Reaching the handler directly is the point of this +// test: through Routes it is unreachable, and that is exactly the assumption +// being removed. +func TestPostCheckInIsSafeWithoutItsMiddleware(t *testing.T) { + repo := seededRepo(t) + srv := NewServer(repo, nil, "https://itemize.no", discardLogger()) + + req := httptest.NewRequest(http.MethodPost, "/api/checkin/"+testCode, nil) + req.SetPathValue("code", testCode) + rec := httptest.NewRecorder() + + srv.postCheckIn(rec, req) + + wantStatus(t, rec, http.StatusUnauthorized) + wantJSON(t, rec) + if got := messageOf(t, rec); got != "You are not logged in" { + t.Errorf("the refusal reads %q, want %q", got, "You are not logged in") + } + if repo.adds != 0 { + t.Error("an anonymous request was written to the attendance register") + } +} + +// The happy path, and the one thing about it that matters afterwards: the +// register is read by people, so it wants the legal name rather than the +// identifier. +func TestPostCheckInRegistersAttendance(t *testing.T) { + repo := seededRepo(t) + rec := do(t, checkInMux(t, repo), http.MethodPost, "/api/checkin/"+testCode, nil, member) + + wantStatus(t, rec, http.StatusOK) + wantJSON(t, rec) + if got := messageOf(t, rec); got != "Success" { + t.Errorf("the confirmation reads %q, want %q", got, "Success") + } + if repo.adds != 1 { + t.Fatalf("storage recorded %d attendances, want 1", repo.adds) + } + if repo.gotCode != testCode { + t.Errorf("attendance was recorded against code %q, want %q", repo.gotCode, testCode) + } + if repo.gotAttendance.Name != member.FullName { + t.Errorf("the register would show %q, want the member's full name %q", + repo.gotAttendance.Name, member.FullName) + } + if repo.gotAttendance.UserID != member.ID { + t.Errorf("the attendance is attributed to %q, want %q", repo.gotAttendance.UserID, member.ID) + } +} + +// The name is what somebody reads off the list afterwards, so an incomplete +// token must not produce a blank row — which is what writing fullName +// unconditionally used to do, unnoticed until the list was read. +func TestNameFor(t *testing.T) { + for _, tc := range []struct { + name string + user *auth.User + want string + }{ + {"nil user", nil, ""}, + {"full name wins", &auth.User{FullName: "Kari Nordmann", Name: "Kari"}, "Kari Nordmann"}, + {"display name when the legal name is missing", &auth.User{Name: "Kari"}, "Kari"}, + {"email as a last resort", &auth.User{Email: "kari@example.no"}, "kari@example.no"}, + {"nothing at all", &auth.User{}, ""}, + {"Norwegian characters are untouched", &auth.User{FullName: "Åse Øverland-Æsberg"}, "Åse Øverland-Æsberg"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := nameFor(tc.user); got != tc.want { + t.Errorf("nameFor gave %q, want %q", got, tc.want) + } + }) + } +} + +// The same fallback, reached the way it actually happens: a member whose token +// carries no legal-name claim scans the code at the door. +func TestPostCheckInFallsBackToTheDisplayName(t *testing.T) { + repo := seededRepo(t) + user := &auth.User{ID: "33333333-4444-4555-8666-777777777777", Name: "Øyvind"} + + do(t, checkInMux(t, repo), http.MethodPost, "/api/checkin/"+testCode, nil, user) + + if repo.gotAttendance.Name != "Øyvind" { + t.Errorf("the register would show %q, want the display name; a blank row is "+ + "a person nobody can identify afterwards", repo.gotAttendance.Name) + } +} + +// Every branch the write path can take, and the status each one has to keep. A +// second scan is a conflict rather than an error, because scanning twice is +// what people do and the response is shown to them at the door. +func TestPostCheckInErrorMapping(t *testing.T) { + for _, tc := range []struct { + name string + addErr error + wantStatus int + wantMsg string + }{ + { + name: "a second scan of the same code", + addErr: events.ErrAlreadyCheckedIn, + wantStatus: http.StatusConflict, + wantMsg: "You have already registered your attendance for this event", + }, + { + name: "a code that matches no event", + addErr: events.ErrNotFound, + wantStatus: http.StatusNotFound, + wantMsg: `Event not found with check_in code "` + testCode + `"`, + }, + { + name: "storage is unavailable", + addErr: errRepo, + wantStatus: http.StatusInternalServerError, + wantMsg: "Something broke :/", + }, + } { + t.Run(tc.name, func(t *testing.T) { + repo := seededRepo(t) + repo.addErr = tc.addErr + + rec := do(t, checkInMux(t, repo), http.MethodPost, "/api/checkin/"+testCode, nil, member) + + wantStatus(t, rec, tc.wantStatus) + wantJSON(t, rec) + if got := messageOf(t, rec); got != tc.wantMsg { + t.Errorf("the message reads %q, want %q", got, tc.wantMsg) + } + }) + } +} + +// The sentinel errors are matched with errors.Is, so a repository that wraps +// them for context must still land on the right status. Unwrapping by equality +// would turn a duplicate scan into a 500 at the door. +func TestPostCheckInMatchesWrappedSentinels(t *testing.T) { + for _, tc := range []struct { + name string + addErr error + wantStatus int + }{ + { + "a wrapped duplicate", + fmt.Errorf("recording attendance: %w", events.ErrAlreadyCheckedIn), + http.StatusConflict, + }, + { + "a wrapped miss", + fmt.Errorf("looking up the code: %w", events.ErrNotFound), + http.StatusNotFound, + }, + } { + t.Run(tc.name, func(t *testing.T) { + repo := seededRepo(t) + repo.addErr = tc.addErr + + rec := do(t, checkInMux(t, repo), http.MethodPost, "/api/checkin/"+testCode, nil, member) + + wantStatus(t, rec, tc.wantStatus) + }) + } +} + +// The rejected code is echoed back in the message, so a code carrying a quote +// or a tag must not break out of the JSON string. This is the one place in the +// package where caller-controlled text reaches a response body, and the +// envelope is what keeps it inert. +func TestPostCheckInEscapesTheEchoedCode(t *testing.T) { + for _, code := range []string{ + `"; DROP TABLE`, + ``, + `æøå`, + `back\slash`, + } { + t.Run(code, func(t *testing.T) { + repo := seededRepo(t) + repo.addErr = events.ErrNotFound + + rec := do(t, checkInMux(t, repo), http.MethodPost, + "/api/checkin/"+url.PathEscape(code), nil, member) + + wantStatus(t, rec, http.StatusNotFound) + // Decoding is the assertion: a body that has broken out of the + // string would not parse, and the raw tag must not appear in it. + got := messageOf(t, rec) + if want := `Event not found with check_in code "` + code + `"`; got != want { + t.Errorf("the message decoded to %q, want %q", got, want) + } + if strings.Contains(rec.Body.String(), ""}, + {name: "an ampersand", in: "Kari & Ola"}, + {name: "an HTML comment", in: ""}, + // JSON requires escaping these two, so only the round trip is asserted. + {name: "a quote", in: `si "hei"`}, + {name: "a newline", in: "linje\nto"}, + {name: "Norwegian characters", in: "Ærlig øl på Åsen", literal: true}, + {name: "an emoji", in: "ferdig 🎉", literal: true}, + } { + t.Run(tc.name, func(t *testing.T) { + rec := httptest.NewRecorder() + + writeJSON(rec, http.StatusOK, message{tc.in}) + body := rec.Body.String() + + for _, char := range htmlSignificant { + if strings.Contains(tc.in, char) && strings.Contains(body, char) { + t.Errorf("%q reached the body unescaped: %s", char, body) + } + } + if tc.literal && !strings.Contains(body, tc.in) { + t.Errorf("%q was escaped rather than written literally: %s", tc.in, body) + } + + // Whatever the escaping, it has to decode back to the original. + var out message + if err := json.Unmarshal([]byte(body), &out); err != nil { + t.Fatalf("the encoded body no longer parses: %v", err) + } + if out.Message != tc.in { + t.Errorf("decoded to %q, want %q", out.Message, tc.in) + } + }) + } +} + +// A value that cannot be encoded arrives after the status line has been sent, +// so there is nothing to salvage — but it must not panic and take the +// connection with it, and it must not be silent either. +func TestWriteJSONWithAnUnencodableBody(t *testing.T) { + // The failure is logged through the package-level logger, which would + // otherwise print to stderr during the run. + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(io.Discard, nil))) + t.Cleanup(func() { slog.SetDefault(previous) }) + + rec := httptest.NewRecorder() + + writeJSON(rec, http.StatusOK, map[string]any{"ch": make(chan int)}) + + if rec.Code != http.StatusOK { + t.Errorf("the status is %d; it was already sent and cannot change", rec.Code) + } + // The truncated body is the honest outcome: the header promised JSON and the + // encoder produced none. + if body := strings.TrimSpace(rec.Body.String()); body != "" { + t.Errorf("an unencodable value produced the partial body %q", body) + } +} diff --git a/internal/api/routes_test.go b/internal/api/routes_test.go new file mode 100644 index 0000000..d250d2a --- /dev/null +++ b/internal/api/routes_test.go @@ -0,0 +1,145 @@ +package api + +import ( + "net/http" + "strings" + "testing" + + "github.com/ItemizeNTNU/website/internal/events" +) + +// Registering the API twice on one mux, or alongside a pattern it conflicts +// with, panics at registration — which is a crash at container start, in +// production, on a Friday. Building the table here turns that into a test +// failure instead. +func TestRoutesRegisterWithoutConflict(t *testing.T) { + newAPI(t, apiConfig{repo: &stubRepo{}}) +} + +// Everything under /api answers as JSON rather than falling through to the +// site's HTML error page. A script that fetches a mistyped path and gets a page +// of markup fails with a parse error that says nothing about what went wrong. +func TestUnknownAPIPathAnswersJSON(t *testing.T) { + mux := newAPI(t, apiConfig{repo: &stubRepo{}}) + + for _, path := range []string{ + "/api/", + "/api/nope", + "/api/events/nope", + "/api/checkin", + "/api/user", + "/api/arrangementer", + } { + t.Run(path, func(t *testing.T) { + rec := getAs(t, mux, path, styret) + + wantStatus(t, rec, http.StatusNotFound) + wantJSON(t, rec) + if got := messageOf(t, rec); got != "API endpoint not found" { + t.Errorf("the message is %q, want %q", got, "API endpoint not found") + } + if strings.Contains(rec.Body.String(), "maintenance`), + wantStatus: http.StatusBadGateway, + wantMsg: "Error fetching user", + }, + { + name: "a reply that is cut short", + handler: jsonUser(`{"user":{"id":`), + wantStatus: http.StatusBadGateway, + wantMsg: "Error fetching user", + }, + } { + t.Run(tc.name, func(t *testing.T) { + fusion, _ := fakeFusion(t, tc.handler) + mux := newAPI(t, apiConfig{fusion: fusion}) + + rec := getAs(t, mux, userPath, member) + + wantStatus(t, rec, tc.wantStatus) + wantJSON(t, rec) + if got := messageOf(t, rec); got != tc.wantMsg { + t.Errorf("the message is %q, want %q", got, tc.wantMsg) + } + }) + } +} + +// A directory that cannot be reached at all — the machine is down, DNS is +// wrong — is the same class of problem as one answering 500, and must not be +// reported as a missing member. +func TestGetUserWhenTheDirectoryIsUnreachable(t *testing.T) { + mux := newAPI(t, apiConfig{fusion: deadFusion(t)}) + + rec := getAs(t, mux, userPath, member) + + wantStatus(t, rec, http.StatusBadGateway) + if got := messageOf(t, rec); got != "Error fetching user" { + t.Errorf("the message is %q, want %q", got, "Error fetching user") + } + if strings.Contains(rec.Body.String(), "connection refused") { + t.Error("the transport error was echoed to the caller, which discloses the " + + "directory's address") + } +} + +// Registration exists to create an account, so a caller who already has one is +// almost certainly a confused client rather than a member — and letting it +// through would send a password-setting email to whatever address it named. +func TestRegisterUserRejectsSignedInCallers(t *testing.T) { + fusion, spy := fakeFusion(t, jsonUser(`{"user":{"id":"x"}}`)) + mux := newAPI(t, apiConfig{fusion: fusion}) + + rec := putJSON(t, mux, "/api/user", validRegistration(t, "student"), member) + + wantStatus(t, rec, http.StatusBadRequest) + wantJSON(t, rec) + if got := messageOf(t, rec); got != "You are already registered" { + t.Errorf("the message is %q, want %q", got, "You are already registered") + } + if spy.snapshot().calls != 0 { + t.Error("a signed-in caller still caused an upstream account creation") + } +} + +// Without an API key nothing can be created, and saying so is better than the +// generic failure a contributor would otherwise spend an evening on. +func TestRegisterUserWithoutAConfiguredDirectory(t *testing.T) { + for _, tc := range []struct { + name string + cfg apiConfig + }{ + {"no API token", apiConfig{}}, + {"no client at all", apiConfig{nilFusion: true}}, + } { + t.Run(tc.name, func(t *testing.T) { + rec := putJSON(t, newAPI(t, tc.cfg), "/api/user", validRegistration(t, "student"), nil) + + wantStatus(t, rec, http.StatusServiceUnavailable) + wantJSON(t, rec) + if got := messageOf(t, rec); got != "Registration is unavailable" { + t.Errorf("the message is %q, want %q", got, "Registration is unavailable") + } + }) + } +} + +// A body the decoder cannot read is answered before anything is created. The +// wrong content type is in the table because the handler never looks at the +// header — a form-encoded submission is refused by the decoder rather than by a +// content negotiation the endpoint does not do. +func TestRegisterUserRejectsUnreadableBodies(t *testing.T) { + for _, tc := range []struct { + name string + body string + }{ + {"an empty body", ""}, + {"whitespace", " "}, + {"truncated JSON", `{"email":`}, + {"not JSON at all", "hello"}, + {"a bare string", `"hello"`}, + {"an array where an object belongs", `[]`}, + {"a number", `42`}, + {"a form submission", "fullName=Kari&email=kari%40example.no"}, + {"a wrongly typed field", `{"fullName":123}`}, + {"a wrongly typed nested block", `{"data":"student"}`}, + } { + t.Run(tc.name, func(t *testing.T) { + fusion, spy := fakeFusion(t, jsonUser(`{"user":{"id":"x"}}`)) + mux := newAPI(t, apiConfig{fusion: fusion}) + + rec := putJSON(t, mux, "/api/user", tc.body, nil) + + wantStatus(t, rec, http.StatusBadRequest) + wantJSON(t, rec) + if got := messageOf(t, rec); got != "Invalid request body" { + t.Errorf("the message is %q, want %q", got, "Invalid request body") + } + if spy.snapshot().calls != 0 { + t.Error("an unreadable body still reached FusionAuth") + } + }) + } +} + +// The body is capped at a megabyte. Without the cap an unauthenticated caller +// could hold the process's memory open by streaming a body that never ends, +// which needs no credentials at all — the endpoint is public by necessity. +func TestRegisterUserRejectsAnOversizedBody(t *testing.T) { + fusion, spy := fakeFusion(t, jsonUser(`{"user":{"id":"x"}}`)) + mux := newAPI(t, apiConfig{fusion: fusion}) + + // Comfortably past 1<<20, in a field the decoder has to read through. + huge := `{"fullName":"` + strings.Repeat("a", 1<<20+64) + `"}` + + rec := putJSON(t, mux, "/api/user", huge, nil) + + wantStatus(t, rec, http.StatusBadRequest) + if got := messageOf(t, rec); got != "Invalid request body" { + t.Errorf("the message is %q, want %q", got, "Invalid request body") + } + if spy.snapshot().calls != 0 { + t.Error("an oversized body still reached FusionAuth") + } +} + +// A body that decodes but says nothing must fail validation rather than create +// an empty member. The message is the first field in sorted order, which is why +// it is the same one every time — a message that varied between identical +// requests would be untestable and bewildering to support. +func TestRegisterUserRejectsAnEmptyRegistration(t *testing.T) { + for _, tc := range []struct { + name string + body string + }{ + {"an empty object", `{}`}, + {"a JSON null", `null`}, + {"only unknown fields", `{"nickname":"kari","admin":true}`}, + } { + t.Run(tc.name, func(t *testing.T) { + fusion, spy := fakeFusion(t, jsonUser(`{"user":{"id":"x"}}`)) + mux := newAPI(t, apiConfig{fusion: fusion}) + + rec := putJSON(t, mux, "/api/user", tc.body, nil) + + wantStatus(t, rec, http.StatusBadRequest) + wantJSON(t, rec) + if got := messageOf(t, rec); got != "Visningsnavn må fylles ut." { + t.Errorf("the message is %q, want the first validation failure in "+ + "sorted field order", got) + } + if spy.snapshot().calls != 0 { + t.Error("an invalid registration still reached FusionAuth") + } + }) + } +} + +// The endpoint runs the same validation as the form, so the rules that matter +// to a member are enforced whichever entry point they arrive through — the +// whole reason the JSON body is flattened into form values first. +func TestRegisterUserValidation(t *testing.T) { + next := time.Now().Year() + 1 + + for _, tc := range []struct { + name string + body string + wantMsg string + }{ + { + name: "a student address is refused, with the reason", + body: registrationBody("Kari Nordmann", "kari@stud.ntnu.no", "Kari", "student", next), + wantMsg: "Vennligst ikke bruk din stud e-post adresse, da du mister tilgang til denne etter fullført utdannelse.", + }, + { + name: "an address that is not one", + body: registrationBody("Kari Nordmann", "kari-at-example", "Kari", "student", next), + wantMsg: "E-postadressen ser ikke gyldig ut.", + }, + { + name: "a missing address", + body: registrationBody("Kari Nordmann", "", "Kari", "student", next), + wantMsg: "E-postadresse må fylles ut.", + }, + { + name: "a display name of two characters", + body: registrationBody("Kari Nordmann", "kari@example.no", "Ka", "student", next), + wantMsg: "Visningsnavn må være minst 3 tegn.", + }, + { + name: "a display name past the limit", + body: registrationBody("Kari Nordmann", "kari@example.no", strings.Repeat("æ", 33), "student", next), + wantMsg: "Visningsnavn kan ikke være lengre enn 32 tegn.", + }, + { + name: "a membership type that is not one of the three", + body: registrationBody("Kari Nordmann", "kari@example.no", "Kari", "styremedlem", next), + wantMsg: "Medlemstype er ikke et gyldig valg.", + }, + { + name: "an expected finish year in the past", + body: registrationBody("Kari Nordmann", "kari@example.no", "Kari", "student", 2001), + wantMsg: fmt.Sprintf("Forventet ferdig år kan ikke være mindre enn %d.", time.Now().Year()), + }, + { + name: "an expected finish year beyond the horizon", + body: registrationBody("Kari Nordmann", "kari@example.no", "Kari", "student", time.Now().Year()+16), + wantMsg: fmt.Sprintf("Forventet ferdig år kan ikke være større enn %d.", time.Now().Year()+15), + }, + } { + t.Run(tc.name, func(t *testing.T) { + fusion, spy := fakeFusion(t, jsonUser(`{"user":{"id":"x"}}`)) + mux := newAPI(t, apiConfig{fusion: fusion}) + + rec := putJSON(t, mux, "/api/user", tc.body, nil) + + wantStatus(t, rec, http.StatusBadRequest) + wantJSON(t, rec) + if got := messageOf(t, rec); got != tc.wantMsg { + t.Errorf("the member would be told %q, want %q", got, tc.wantMsg) + } + if spy.snapshot().calls != 0 { + t.Error("an invalid registration still reached FusionAuth") + } + }) + } +} + +// The happy path for each membership type, checked at the upstream request +// rather than the response: what is created is a permanent record, and the +// fields belonging to the other two types must be absent rather than empty — +// an alumnus arriving with a blank study year would carry it for good. +func TestRegisterUserCreatesTheAccount(t *testing.T) { + next := time.Now().Year() + 1 + + for _, tc := range []struct { + name string + body string + want map[string]any + absent []string + present []string + }{ + { + name: "a student", + body: registrationBody("Kari Nordmann", "kari@example.no", "Kari", "student", next), + want: map[string]any{ + "displayName": "Kari", + "type": "student", + "study": map[string]any{ + "program": "Datateknologi", + "year": float64(2), + "expectedFinishYear": fmt.Sprintf("%04d-06-01T00:00:00Z", next), + }, + }, + absent: []string{"alumni", "employee"}, + }, + { + name: "an alumnus", + body: fmt.Sprintf(`{"fullName":"Ola Nordmann","email":"ola@example.no", + "data":{"displayName":"Ola","type":"alumni", + "study":{"program":"Datateknologi"}, + "alumni":{"joinYear":%d}}}`, time.Now().Year()), + want: map[string]any{ + "displayName": "Ola", + "type": "alumni", + "study": map[string]any{"program": "Datateknologi"}, + "alumni": map[string]any{"joinYear": float64(time.Now().Year())}, + }, + absent: []string{"employee"}, + }, + { + name: "an employee", + body: `{"fullName":"Åse Øverland","email":"aase@example.no", + "data":{"displayName":"Åse","type":"employee", + "employee":{"title":"Førsteamanuensis"}}}`, + want: map[string]any{ + "displayName": "Åse", + "type": "employee", + "employee": map[string]any{"title": "Førsteamanuensis"}, + }, + absent: []string{"study", "alumni"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + fusion, spy := fakeFusion(t, jsonUser(`{"user":{"id":"new-user"}}`)) + mux := newAPI(t, apiConfig{fusion: fusion}) + + rec := putJSON(t, mux, "/api/user", tc.body, nil) + + wantStatus(t, rec, http.StatusOK) + wantJSON(t, rec) + if got := messageOf(t, rec); got != "Success" { + t.Errorf("the confirmation reads %q, want %q", got, "Success") + } + + snap := spy.snapshot() + if snap.calls != 1 { + t.Fatalf("FusionAuth saw %d requests, want exactly 1", snap.calls) + } + if snap.method != http.MethodPost || snap.path != "/api/user" { + t.Errorf("upstream was called as %s %s, want POST /api/user", snap.method, snap.path) + } + + var sent struct { + SendSetPasswordEmail bool `json:"sendSetPasswordEmail"` + User struct { + Email string `json:"email"` + FullName string `json:"fullName"` + Data map[string]any `json:"data"` + } `json:"user"` + } + if err := json.Unmarshal([]byte(snap.body), &sent); err != nil { + t.Fatalf("the upstream request is not JSON: %v; body was %s", err, snap.body) + } + + // Without this the member is created but never receives the link + // that lets them set a password, and the account is unusable. + if !sent.SendSetPasswordEmail { + t.Error("the account was created without asking FusionAuth to send " + + "the password-setting email, so the member can never sign in") + } + for key, want := range tc.want { + if got := sent.User.Data[key]; !sameJSON(got, want) { + t.Errorf("data[%q] was sent as %#v, want %#v", key, got, want) + } + } + for _, key := range tc.absent { + if _, ok := sent.User.Data[key]; ok { + t.Errorf("data carries %q, which belongs to a different membership "+ + "type and would be stored permanently", key) + } + } + }) + } +} + +// The study year arrives as a JSON number from a modern client and as a string +// from whatever is still posting the previous API's shape. Both have to reach +// validation as the same digits, or one of the two callers is rejected for a +// field they filled in correctly. +func TestRegisterUserAcceptsNumbersAsStringsOrNumbers(t *testing.T) { + next := time.Now().Year() + 1 + + for _, tc := range []struct { + name string + year string + }{ + {"a JSON number", `2`}, + {"a JSON string", `"2"`}, + } { + t.Run(tc.name, func(t *testing.T) { + fusion, spy := fakeFusion(t, jsonUser(`{"user":{"id":"new-user"}}`)) + mux := newAPI(t, apiConfig{fusion: fusion}) + + body := fmt.Sprintf(`{"fullName":"Kari Nordmann","email":"kari@example.no", + "data":{"displayName":"Kari","type":"student", + "study":{"program":"Datateknologi","year":%s,"expectedFinishYear":"%d-06-01T00:00:00Z"}}}`, + tc.year, next) + + rec := putJSON(t, mux, "/api/user", body, nil) + + wantStatus(t, rec, http.StatusOK) + if !strings.Contains(spy.snapshot().body, `"year":2`) { + t.Errorf("the study year did not reach FusionAuth as 2: %s", spy.snapshot().body) + } + }) + } +} + +// FusionAuth's own rejections are passed through, because the one that actually +// happens is "email already in use" and the member needs to read it. Anything +// else it says is at least closer to the truth than a generic failure. +func TestRegisterUserSurfacesUpstreamRejections(t *testing.T) { + fusion, _ := fakeFusion(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadRequest) + _, _ = io.WriteString(w, `{"fieldErrors":{"user.email":[{"code":"[duplicate]", + "message":"A User with email 'kari@example.no' already exists."}]}}`) + }) + mux := newAPI(t, apiConfig{fusion: fusion}) + + rec := putJSON(t, mux, "/api/user", validRegistration(t, "student"), nil) + + wantStatus(t, rec, http.StatusBadRequest) + wantJSON(t, rec) + const want = "A User with email 'kari@example.no' already exists." + if got := messageOf(t, rec); got != want { + t.Errorf("the member would be told %q, want %q", got, want) + } +} + +// wantUpstreamDown is what a member sees when the failure is ours rather than +// theirs: no service address, no status code, and an instruction that is worth +// following — the form they filled in was fine. +const wantUpstreamDown = "Innloggingstjenesten svarer ikke akkurat nå. Prøv igjen om litt." + +// Who is at fault decides both the status and the wording, and FusionAuth's +// error parser makes that easy to get wrong: it wraps every non-2xx reply, a +// 5xx included, in the same *APIError the handler reads validation messages +// out of. Matching on the type alone told a member whose registration was +// perfectly valid that it was not, sent them back to correct a form with +// nothing wrong with it, and hid the outage from anything watching for 5xx. +func TestRegisterUserSeparatesItsOwnFailuresFromTheMembers(t *testing.T) { + for _, tc := range []struct { + name string + status int + body string + wantStatus int + wantMsg string + }{ + { + name: "a rejection the member can act on", + status: http.StatusBadRequest, + body: `{"generalErrors":[{"code":"[duplicate]","message":"E-posten er allerede i bruk."}]}`, + wantStatus: http.StatusBadRequest, + wantMsg: "E-posten er allerede i bruk.", + }, + { + name: "a conflict is still the member's to resolve", + status: http.StatusConflict, + body: `{"generalErrors":[{"code":"[duplicate]","message":"E-posten er allerede i bruk."}]}`, + wantStatus: http.StatusBadRequest, + wantMsg: "E-posten er allerede i bruk.", + }, + { + name: "the directory is broken", + status: http.StatusInternalServerError, + body: `{}`, + wantStatus: http.StatusBadGateway, + wantMsg: wantUpstreamDown, + }, + { + name: "the directory is restarting", + status: http.StatusServiceUnavailable, + body: `{}`, + wantStatus: http.StatusBadGateway, + wantMsg: wantUpstreamDown, + }, + { + // The worst of the lot before the fix: FusionAuth's fallback + // message is "Uventet svar fra innloggingstjenesten (HTTP 502)", + // which was handed to the member as a 400 — a validation error + // telling them about an HTTP status. + name: "something in front of the directory is broken", + status: http.StatusBadGateway, + body: `{}`, + wantStatus: http.StatusBadGateway, + wantMsg: wantUpstreamDown, + }, + } { + t.Run(tc.name, func(t *testing.T) { + fusion, _ := fakeFusion(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tc.status) + _, _ = io.WriteString(w, tc.body) + }) + mux := newAPI(t, apiConfig{fusion: fusion}) + + rec := putJSON(t, mux, "/api/user", validRegistration(t, "student"), nil) + + wantStatus(t, rec, tc.wantStatus) + wantJSON(t, rec) + if got := messageOf(t, rec); got != tc.wantMsg { + t.Errorf("FusionAuth answered %d and the member was told %q, want %q", + tc.status, got, tc.wantMsg) + } + }) + } +} + +// A directory that answers with a server error is ours to fix, not the +// member's: their registration is untouched and retrying it is the right +// advice, so the failure must not arrive as a 400 that blames their input. +func TestRegisterUserWhenTheDirectoryErrors(t *testing.T) { + fusion, _ := fakeFusion(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = io.WriteString(w, `{}`) + }) + mux := newAPI(t, apiConfig{fusion: fusion}) + + rec := putJSON(t, mux, "/api/user", validRegistration(t, "student"), nil) + + wantStatus(t, rec, http.StatusBadGateway) + wantJSON(t, rec) + if got := messageOf(t, rec); got != wantUpstreamDown { + t.Errorf("the member would be told %q, want %q", got, wantUpstreamDown) + } + // The fallback FusionAuth builds for a body it cannot read is a status code + // in Norwegian prose. It is a fine thing to log and a useless thing to show + // somebody trying to sign up. + if strings.Contains(rec.Body.String(), "HTTP 500") { + t.Errorf("the upstream status was shown to the member: %s", rec.Body.String()) + } +} + +// A directory that cannot be reached at all is the same class of problem as one +// answering 500 — ours, and probably temporary — so it gets the same status and +// the same wording, without naming the service or the address. +func TestRegisterUserWhenTheDirectoryIsUnreachable(t *testing.T) { + mux := newAPI(t, apiConfig{fusion: deadFusion(t)}) + + rec := putJSON(t, mux, "/api/user", validRegistration(t, "student"), nil) + + wantStatus(t, rec, http.StatusBadGateway) + wantJSON(t, rec) + if got := messageOf(t, rec); got != wantUpstreamDown { + t.Errorf("the message is %q, want %q", got, wantUpstreamDown) + } + if strings.Contains(rec.Body.String(), "connection refused") { + t.Error("the transport error was echoed to the caller") + } +} + +// Creating an account makes FusionAuth send mail to an address the caller +// chooses, so an unthrottled endpoint is a way to send mail from our domain to +// arbitrary people and to fill the directory with junk. Neither needs any +// access: the endpoint is public by necessity. +// +// This test burns its whole mux's allowance, which is why it builds its own. +func TestRegisterUserIsRateLimited(t *testing.T) { + mux := newAPI(t, apiConfig{}) + body := validRegistration(t, "student") + + // The limiter counts every attempt, not only the successful ones — an + // unconfigured directory still answers 503 rather than passing through. + for i := range 5 { + rec := putJSON(t, mux, "/api/user", body, nil) + if rec.Code == http.StatusTooManyRequests { + t.Fatalf("attempt %d was throttled; the allowance is five", i+1) + } + } + + rec := putJSON(t, mux, "/api/user", body, nil) + + wantStatus(t, rec, http.StatusTooManyRequests) + if got := strings.TrimSpace(rec.Body.String()); got != "For mange forsøk. Vent litt og prøv igjen." { + t.Errorf("the throttle message is %q, want the Norwegian one", got) + } + // Without this a client has no idea whether to retry in a second or an hour. + if got := rec.Header().Get("Retry-After"); got != "60" { + t.Errorf("Retry-After is %q, want %q", got, "60") + } +} + +// num renders the numbers that arrive as JSON, which decode as float64. A +// decimal tail would reach validation as "2.000000" and be rejected as not a +// whole number, so the member sees an error for a field they filled in. +func TestNum(t *testing.T) { + for _, tc := range []struct { + name string + in any + want string + }{ + {"a JSON number", float64(3), "3"}, + {"zero", float64(0), "0"}, + {"a fractional year is truncated", float64(3.7), "3"}, + {"a negative number", float64(-1), "-1"}, + {"an already-string number", "4", "4"}, + {"an empty string", "", ""}, + {"a non-numeric string is passed through for validation to reject", "fjerde", "fjerde"}, + {"absent", nil, ""}, + {"a boolean", true, ""}, + {"an object", map[string]any{"year": 2}, ""}, + {"an array", []any{2}, ""}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := num(tc.in); got != tc.want { + t.Errorf("num(%#v) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// The previous API sent the expected finish year as a full ISO date, and +// clients built against it still do. Both forms have to reduce to the year, or +// a returning client is told its date is not a number. +func TestYearOf(t *testing.T) { + for _, tc := range []struct { + in string + want string + }{ + {"", ""}, + {"2030", "2030"}, + {"2030-06-01T00:00:00Z", "2030"}, + {"2030-06-01", "2030"}, + // Too short to hold a year: passed through so validation rejects it + // rather than this helper inventing one. + {"203", "203"}, + {"20", "20"}, + {"tjuetretti", "tjue"}, + } { + t.Run(strconv.Quote(tc.in), func(t *testing.T) { + if got := yearOf(tc.in); got != tc.want { + t.Errorf("yearOf(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// str reads a value out of FusionAuth's free-form data block, which is not a +// shape this code controls. Anything that is not a string has to come back +// empty rather than panic. +func TestStr(t *testing.T) { + data := map[string]any{ + "displayName": "Kari", + "type": 42, + "empty": "", + "null": nil, + } + for _, tc := range []struct { + key string + want string + }{ + {"displayName", "Kari"}, + {"type", ""}, + {"empty", ""}, + {"null", ""}, + {"missing", ""}, + } { + if got := str(data, tc.key); got != tc.want { + t.Errorf("str(data, %q) = %q, want %q", tc.key, got, tc.want) + } + } + if got := str(nil, "displayName"); got != "" { + t.Errorf("str(nil, ...) = %q, want the empty string; a member with no data "+ + "block must not crash the handler", got) + } +} + +// registrationBody builds a student registration with the given values. +func registrationBody(fullName, email, displayName, memberType string, finishYear int) string { + body := map[string]any{ + "fullName": fullName, + "email": email, + "data": map[string]any{ + "displayName": displayName, + "type": memberType, + "study": map[string]any{ + "program": "Datateknologi", + "year": 2, + "expectedFinishYear": fmt.Sprintf("%04d-06-01T00:00:00Z", finishYear), + }, + }, + } + encoded, err := json.Marshal(body) + if err != nil { + panic(err) + } + return string(encoded) +} + +// validRegistration is a body that passes validation today and will keep doing +// so: the expected finish year is relative to the current year rather than a +// literal, so the tests do not start failing on New Year's Eve. +func validRegistration(t *testing.T, memberType string) string { + t.Helper() + return registrationBody("Kari Nordmann", "kari@example.no", "Kari", memberType, time.Now().Year()+1) +} + +// sameJSON compares decoded JSON values, which are maps and float64s rather +// than the types they were written as. +func sameJSON(got, want any) bool { + gotEncoded, err := json.Marshal(got) + if err != nil { + return false + } + wantEncoded, err := json.Marshal(want) + if err != nil { + return false + } + return string(gotEncoded) == string(wantEncoded) +} + +// The registration endpoint takes no notice of who the caller claims to be +// beyond "nobody", so an anonymous request is the only one that proceeds. This +// pins that the check is on presence rather than on a role — a board member +// creating a second account for themselves is refused just the same. +func TestRegisterUserRejectsAnySignedInCaller(t *testing.T) { + for _, u := range []*auth.User{member, styret} { + rec := putJSON(t, newAPI(t, apiConfig{}), "/api/user", validRegistration(t, "student"), u) + + wantStatus(t, rec, http.StatusBadRequest) + if got := messageOf(t, rec); got != "You are already registered" { + t.Errorf("%s was told %q, want %q", u.Name, got, "You are already registered") + } + } +} diff --git a/internal/auth/csrf.go b/internal/auth/csrf.go index 5389e0f..2cc8a46 100644 --- a/internal/auth/csrf.go +++ b/internal/auth/csrf.go @@ -3,6 +3,7 @@ package auth import ( "crypto/rand" "encoding/base64" + "errors" "html/template" "net/http" ) @@ -66,6 +67,22 @@ func CSRF(next http.Handler) http.Handler { http.Error(w, "Skjemaet kunne ikke leses.", http.StatusBadRequest) return } + // ParseForm does not read a multipart body. It leaves PostForm non-nil + // but empty, so PostFormValue below would read "" and every multipart + // post — a file upload, once the site has one — would be rejected as if + // the token had expired. Parse it explicitly instead. + // + // maxFormBytes is passed as the in-memory limit deliberately: the + // MaxBytesReader above already caps the whole body at that figure, so + // nothing can be large enough to spill into a temporary file, and the + // bound on what one client can occupy is unchanged. ErrNotMultipart is + // the ordinary answer for the urlencoded posts every form on the site + // actually sends; anything else is a body we could not read. + if err := r.ParseMultipartForm(maxFormBytes); err != nil && + !errors.Is(err, http.ErrNotMultipart) { + http.Error(w, "Skjemaet kunne ikke leses.", http.StatusBadRequest) + return + } if !constantTimeEqual(cookie.Value, r.PostFormValue(CSRFField)) { http.Error(w, "Skjemaet er utløpt. Last siden på nytt og prøv igjen.", http.StatusForbidden) return diff --git a/internal/auth/csrf_edge_test.go b/internal/auth/csrf_edge_test.go new file mode 100644 index 0000000..4ffca21 --- /dev/null +++ b/internal/auth/csrf_edge_test.go @@ -0,0 +1,599 @@ +package auth + +// Edge cases for the double-submit CSRF guard, the token it issues, and the +// constant-time comparison both it and the OIDC callback depend on. + +import ( + "encoding/base64" + "html/template" + "io" + "net/http" + "net/http/httptest" + "net/url" + "os" + "strings" + "testing" +) + +// withSecureCookies pins the package-level flag for one test and puts it back +// afterwards, so tests that assert on the CSRF cookie's Secure attribute do not +// depend on which sealer another test happened to build last. +func withSecureCookies(t *testing.T, secure bool) { + t.Helper() + previous := secureCookies + t.Cleanup(func() { SetSecureCookies(previous) }) + SetSecureCookies(secure) +} + +// formPost builds a state-changing request. Unlike the helper in csrf_test.go +// this one can set an *empty* cookie value, which is the case that matters +// most below. +func formPost(method string, cookie *string, fetchSite, body string) *http.Request { + r := httptest.NewRequest(method, "/arrangementer", strings.NewReader(body)) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if cookie != nil { + r.AddCookie(&http.Cookie{Name: csrfCookie, Value: *cookie}) + } + if fetchSite != "" { + r.Header.Set("Sec-Fetch-Site", fetchSite) + } + return r +} + +func encodedField(value string) string { + return url.Values{CSRFField: {value}}.Encode() +} + +// reached reports whether the guarded handler ran. A CSRF test that only checks +// the status code can pass while the handler has already had its side effect. +func reached(t *testing.T, r *http.Request) (int, bool) { + t.Helper() + var ran bool + rec := httptest.NewRecorder() + CSRF(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + ran = true + w.WriteHeader(http.StatusOK) + })).ServeHTTP(rec, r) + return rec.Code, ran +} + +// ── The empty-token bypass ──────────────────────────────────────────────── + +// This is the single most important case in this file. +// +// constantTimeEqual("", "") is true — subtle.ConstantTimeCompare returns 1 for +// two zero-length slices. So if the guard ever stops rejecting an empty cookie +// before it compares, every cross-site form post that simply omits the token +// and arrives with an empty cookie would pass the comparison. The emptiness +// check in csrf.go is the only thing standing between here and a universal +// bypass, and this test exists to make removing it fail loudly. +func TestCSRFRejectsAnEmptyCookieAgainstAnEmptyField(t *testing.T) { + if !constantTimeEqual("", "") { + t.Fatal("constantTimeEqual no longer treats two empty strings as equal; the reasoning " + + "in this test needs rewriting, but check first that the CSRF guard is still correct") + } + + empty := "" + code, ran := reached(t, formPost(http.MethodPost, &empty, "same-origin", encodedField(""))) + if ran || code != http.StatusForbidden { + t.Errorf("a post with an empty CSRF cookie and an empty token field was allowed "+ + "(status %d, handler ran %v); because an empty-vs-empty comparison succeeds, "+ + "this is a complete bypass of the double-submit check", code, ran) + } +} + +// The same hole from the other direction: a cookie that is present but empty +// must never match, whatever the body says. +func TestCSRFRejectsAnEmptyCookieAgainstAnyField(t *testing.T) { + empty := "" + for _, field := range []string{"", "abc", " "} { + code, ran := reached(t, formPost(http.MethodPost, &empty, "same-origin", encodedField(field))) + if ran || code != http.StatusForbidden { + t.Errorf("an empty CSRF cookie was accepted against field %q (status %d)", field, code) + } + } +} + +// ── Methods ─────────────────────────────────────────────────────────────── + +// Every method that can change state must be guarded, not only POST. A route +// added later with PUT or DELETE must not be unprotected by default. +func TestCSRFGuardsEveryUnsafeMethod(t *testing.T) { + for _, method := range []string{ + http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete, + http.MethodConnect, "PROPFIND", "FROBNICATE", + } { + t.Run(method, func(t *testing.T) { + code, ran := reached(t, formPost(method, nil, "same-origin", "")) + if ran || code == http.StatusOK { + t.Errorf("%s passed the CSRF guard without a token (status %d); any route "+ + "mounted on this method would be forgeable from another site", method, code) + } + }) + } +} + +// Reads must never be blocked, including the ones an older browser sends with +// no Sec-Fetch-Site header and no cookie at all. +func TestCSRFExemptsSafeMethods(t *testing.T) { + for _, method := range []string{ + http.MethodGet, http.MethodHead, http.MethodOptions, http.MethodTrace, + } { + t.Run(method, func(t *testing.T) { + r := httptest.NewRequest(method, "/", nil) + // Even a genuinely cross-site read must go through: a link from + // another site to an event page is normal traffic. + r.Header.Set("Sec-Fetch-Site", "cross-site") + if code, ran := reached(t, r); !ran || code != http.StatusOK { + t.Errorf("%s was blocked (status %d); ordinary browsing would break", method, code) + } + }) + } +} + +// ── Sec-Fetch-Site ──────────────────────────────────────────────────────── + +// Only "cross-site" is a refusal. "none" means the visitor typed the URL or +// used a bookmark, and "same-site" covers a subdomain — both are legitimate and +// blocking them would break real submissions. +func TestCSRFSecFetchSiteHandling(t *testing.T) { + tests := []struct { + site string + want int + why string + }{ + {"same-origin", http.StatusOK, "the normal case: a form on our own page"}, + {"same-site", http.StatusOK, "a subdomain of itemize.no is trusted"}, + {"none", http.StatusOK, "a bookmark or typed URL is not an attack"}, + {"", http.StatusOK, "a browser too old to send the header falls back to the token"}, + {"CROSS-SITE", http.StatusOK, "the header is lower-case by specification; an " + + "upper-case value is not something a browser sends, and the token still gates it"}, + {"cross-site", http.StatusForbidden, "the attack this header exists to stop"}, + } + + for _, tt := range tests { + t.Run(tt.site, func(t *testing.T) { + token := "matching-token" + code, _ := reached(t, formPost(http.MethodPost, &token, tt.site, encodedField(token))) + if code != tt.want { + t.Errorf("Sec-Fetch-Site: %q gave %d, want %d — %s", tt.site, code, tt.want, tt.why) + } + }) + } +} + +// A cross-site request must be refused before its body is even read, so a +// forged post cannot be used to make the server buffer a large body. +func TestCSRFRefusesCrossSiteBeforeReadingTheBody(t *testing.T) { + token := "abc" + body := &countingReader{inner: strings.NewReader(encodedField(token))} + + r := httptest.NewRequest(http.MethodPost, "/arrangementer", body) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + r.AddCookie(&http.Cookie{Name: csrfCookie, Value: token}) + r.Header.Set("Sec-Fetch-Site", "cross-site") + + rec := httptest.NewRecorder() + CSRF(okHandler()).ServeHTTP(rec, r) + + if rec.Code != http.StatusForbidden { + t.Fatalf("got %d, want 403", rec.Code) + } + if body.reads > 0 { + t.Error("the body of a cross-site request was read; it should be refused on the " + + "header alone so an attacker cannot make us buffer anything") + } +} + +type countingReader struct { + inner io.Reader + reads int +} + +func (c *countingReader) Read(p []byte) (int, error) { + c.reads++ + return c.inner.Read(p) +} + +// ── Bodies ──────────────────────────────────────────────────────────────── + +// The body is capped so one client cannot occupy Go's 10 MB default per +// request across many connections. Over the cap the request must fail rather +// than be truncated into a form that happens to parse. +func TestCSRFRejectsAnOversizedBody(t *testing.T) { + token := "abc" + // Valid form encoding, but far past maxFormBytes. + body := encodedField(token) + "&filler=" + strings.Repeat("x", maxFormBytes+1) + + code, ran := reached(t, formPost(http.MethodPost, &token, "same-origin", body)) + if ran { + t.Error("the handler ran on an over-sized body; the size cap is not being enforced") + } + if code != http.StatusBadRequest { + t.Errorf("got %d, want 400 for a body over the %d-byte cap", code, maxFormBytes) + } +} + +// A body just under the cap must still work, or the limit would be rejecting +// legitimate submissions. +func TestCSRFAcceptsABodyJustUnderTheCap(t *testing.T) { + token := "abc" + filler := strings.Repeat("x", maxFormBytes-len(encodedField(token))-len("&filler=")-1) + body := encodedField(token) + "&filler=" + filler + + if code, ran := reached(t, formPost(http.MethodPost, &token, "same-origin", body)); !ran { + t.Errorf("a body of %d bytes, inside the %d-byte cap, was rejected with %d", + len(body), maxFormBytes, code) + } +} + +// An unparseable body is a 400, not a 403: the difference matters because a 403 +// tells the visitor to reload the page, which would not help. +func TestCSRFReportsAnUnparseableBodyAsABadRequest(t *testing.T) { + token := "abc" + code, ran := reached(t, formPost(http.MethodPost, &token, "same-origin", "%zz=%zz")) + if ran { + t.Error("the handler ran on a body ParseForm could not read") + } + if code != http.StatusBadRequest { + t.Errorf("got %d, want 400", code) + } +} + +// multipartPost builds a multipart/form-data post carrying one token field, the +// shape a form with a file input would submit. +func multipartPost(cookie, field string) *http.Request { + body := "--X\r\n" + + `Content-Disposition: form-data; name="` + CSRFField + `"` + "\r\n\r\n" + + field + "\r\n--X--\r\n" + + r := httptest.NewRequest(http.MethodPost, "/arrangementer", strings.NewReader(body)) + r.Header.Set("Content-Type", "multipart/form-data; boundary=X") + r.AddCookie(&http.Cookie{Name: csrfCookie, Value: cookie}) + r.Header.Set("Sec-Fetch-Site", "same-origin") + return r +} + +// ParseForm does not read a multipart body — it leaves PostForm empty — so the +// guard has to parse one itself or the token would be invisible and every +// multipart post would be refused as expired. No form on the site uses +// multipart today; this test is what will keep the first file upload from +// failing with a message telling the visitor to reload a page that was fine. +func TestCSRFReadsATokenFromAMultipartBody(t *testing.T) { + code, ran := reached(t, multipartPost("abc", "abc")) + if !ran { + t.Errorf("a multipart post carrying the right token was rejected with %d; "+ + "the token in a multipart body must be found, or a form with a file input "+ + "can never be submitted", code) + } +} + +// The multipart path is not a way around the check: a body that carries the +// wrong token must be refused exactly as an urlencoded one is. If it were not, +// an attacker could bypass the guard by changing the form's enctype. +func TestCSRFRejectsAWrongTokenInAMultipartBody(t *testing.T) { + code, ran := reached(t, multipartPost("abc", "wrong")) + if ran { + t.Error("a multipart post with a token that did not match the cookie reached the " + + "handler; switching a form to enctype=multipart/form-data would then defeat " + + "the whole double-submit guard") + } + if code != http.StatusForbidden { + t.Errorf("got %d, want 403", code) + } +} + +// The guard parses the body before the handler does, so the handler has to +// still find its own fields there. If parsing consumed the body without +// populating PostForm, every field on a multipart form would arrive empty and +// the handler would silently save nothing. +func TestCSRFLeavesMultipartFieldsReadableByTheHandler(t *testing.T) { + body := "--X\r\n" + + `Content-Disposition: form-data; name="` + CSRFField + `"` + "\r\n\r\n" + + "abc\r\n--X\r\n" + + `Content-Disposition: form-data; name="tittel"` + "\r\n\r\n" + + "Julebord\r\n--X--\r\n" + + r := httptest.NewRequest(http.MethodPost, "/arrangementer", strings.NewReader(body)) + r.Header.Set("Content-Type", "multipart/form-data; boundary=X") + r.AddCookie(&http.Cookie{Name: csrfCookie, Value: "abc"}) + r.Header.Set("Sec-Fetch-Site", "same-origin") + + var got string + CSRF(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + got = r.FormValue("tittel") + })).ServeHTTP(httptest.NewRecorder(), r) + + if got != "Julebord" { + t.Errorf("the handler read %q for the tittel field, want %q; the guard consumed the "+ + "multipart body without leaving its values behind", got, "Julebord") + } +} + +// A multipart body is bounded by the same cap as any other. Parsing one must +// not be a way to make the server hold more than maxFormBytes. +func TestCSRFRejectsAnOversizedMultipartBody(t *testing.T) { + body := "--X\r\n" + + `Content-Disposition: form-data; name="` + CSRFField + `"` + "\r\n\r\n" + + "abc\r\n--X\r\n" + + `Content-Disposition: form-data; name="filler"; filename="f.bin"` + "\r\n\r\n" + + strings.Repeat("x", maxFormBytes+1) + "\r\n--X--\r\n" + + r := httptest.NewRequest(http.MethodPost, "/arrangementer", strings.NewReader(body)) + r.Header.Set("Content-Type", "multipart/form-data; boundary=X") + r.AddCookie(&http.Cookie{Name: csrfCookie, Value: "abc"}) + r.Header.Set("Sec-Fetch-Site", "same-origin") + + code, ran := reached(t, r) + if ran { + t.Error("the handler ran on an over-sized multipart body; the size cap is not " + + "applied to multipart, so one client could occupy far more memory than the " + + "urlencoded path allows") + } + if code != http.StatusBadRequest { + t.Errorf("got %d, want 400 for a body over the %d-byte cap", code, maxFormBytes) + } +} + +// A multipart header the parser cannot make sense of is a 400, not a 403 — the +// same distinction the urlencoded path makes, and for the same reason: telling +// the visitor to reload would not help. +func TestCSRFReportsAMalformedMultipartBodyAsABadRequest(t *testing.T) { + r := httptest.NewRequest(http.MethodPost, "/arrangementer", strings.NewReader("not multipart")) + // No boundary parameter, so there is nothing to split the body on. + r.Header.Set("Content-Type", "multipart/form-data") + r.AddCookie(&http.Cookie{Name: csrfCookie, Value: "abc"}) + r.Header.Set("Sec-Fetch-Site", "same-origin") + + code, ran := reached(t, r) + if ran { + t.Error("the handler ran on a multipart body that could not be parsed") + } + if code != http.StatusBadRequest { + t.Errorf("got %d, want 400", code) + } +} + +// The token may appear only in the body. A value in the query string must not +// satisfy the check, because a URL is exactly what an attacker controls when +// they get a browser to issue a request. +func TestCSRFIgnoresATokenInTheQueryString(t *testing.T) { + token := "abc" + r := httptest.NewRequest(http.MethodPost, "/arrangementer?"+encodedField(token), strings.NewReader("")) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + r.AddCookie(&http.Cookie{Name: csrfCookie, Value: token}) + r.Header.Set("Sec-Fetch-Site", "same-origin") + + if code, ran := reached(t, r); ran { + t.Errorf("a token supplied in the URL satisfied the guard (status %d); an attacker "+ + "controls the URL of a request they cause, so only the body may count", code) + } +} + +// ── Token issuance ──────────────────────────────────────────────────────── + +// The token has to be unguessable: the whole double-submit argument is that an +// attacker cannot learn the cookie's value, so a predictable one defeats it. +func TestCSRFTokensAreUnpredictableAndFullLength(t *testing.T) { + withSecureCookies(t, true) + + seen := make(map[string]bool, 200) + for i := 0; i < 200; i++ { + token := CSRFToken(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/", nil)) + if token == "" { + t.Fatal("CSRFToken returned an empty string; every form on the page would then " + + "carry an empty token and be rejected") + } + raw, err := base64.RawURLEncoding.DecodeString(token) + if err != nil { + t.Fatalf("token %q is not raw base64url and cannot go in a cookie: %v", token, err) + } + if len(raw) != 32 { + t.Fatalf("token carries %d bytes of entropy, want 32", len(raw)) + } + if seen[token] { + t.Fatal("CSRFToken returned a value it had already issued; the token generator " + + "is not random and the double-submit check is worthless") + } + seen[token] = true + } +} + +// The cookie's attributes are deliberate and each one is explained in csrf.go. +// HttpOnly in particular must stay off: the double-submit pattern needs the +// value readable by same-origin script, and the token is not a secret from a +// page that is already same-origin. +func TestCSRFCookieAttributes(t *testing.T) { + for _, secure := range []bool{true, false} { + name := "secure deployment" + if !secure { + name = "plain-HTTP development" + } + t.Run(name, func(t *testing.T) { + withSecureCookies(t, secure) + + rec := httptest.NewRecorder() + token := CSRFToken(rec, httptest.NewRequest(http.MethodGet, "/", nil)) + + c := cookieNamed(t, rec, csrfCookie) + if c.Value != token { + t.Errorf("the cookie carries %q but the form would carry %q, so every post "+ + "would be refused", c.Value, token) + } + if c.HttpOnly { + t.Error("the CSRF cookie is HttpOnly; the double-submit pattern requires " + + "same-origin script to be able to read it") + } + if c.Secure != secure { + t.Errorf("Secure = %v, want %v; a Secure cookie is discarded over plain HTTP "+ + "and every form post in development would then 403", c.Secure, secure) + } + if c.SameSite != http.SameSiteLaxMode { + t.Errorf("SameSite = %v, want Lax", c.SameSite) + } + if c.Path != "/" { + t.Errorf("Path = %q, want \"/\"; a form on another path would get a second, "+ + "different token", c.Path) + } + }) + } +} + +// Re-rendering a page must not mint a second token, or the two forms on it +// would disagree with the cookie. Nothing may be written to the response when +// the cookie is already there. +func TestCSRFTokenSetsNoCookieWhenOneExists(t *testing.T) { + withSecureCookies(t, true) + + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.AddCookie(&http.Cookie{Name: csrfCookie, Value: "already-issued"}) + + rec := httptest.NewRecorder() + if got := CSRFToken(rec, r); got != "already-issued" { + t.Errorf("CSRFToken returned %q rather than the value already in the cookie", got) + } + if len(rec.Result().Cookies()) != 0 { + t.Error("a second CSRF cookie was set on a request that already had one; the two " + + "forms on the page would end up carrying different tokens") + } +} + +// An empty cookie value is treated as "no token", so a browser holding a +// cleared cookie gets a fresh one rather than being wedged into permanent 403s. +func TestCSRFTokenReplacesAnEmptyCookie(t *testing.T) { + withSecureCookies(t, true) + + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.AddCookie(&http.Cookie{Name: csrfCookie, Value: ""}) + + rec := httptest.NewRecorder() + if got := CSRFToken(rec, r); got == "" { + t.Fatal("no token was issued to a request holding an empty CSRF cookie, so the " + + "visitor could never submit a form again") + } + if len(rec.Result().Cookies()) == 0 { + t.Error("no replacement cookie was set") + } +} + +// A token issued by CSRFToken must actually satisfy CSRF. This closes the loop +// between the two halves of the pattern, which are otherwise only tested apart. +func TestAnIssuedTokenSatisfiesTheGuard(t *testing.T) { + withSecureCookies(t, false) + + issuing := httptest.NewRecorder() + token := CSRFToken(issuing, httptest.NewRequest(http.MethodGet, "/", nil)) + + r := httptest.NewRequest(http.MethodPost, "/arrangementer", + strings.NewReader(encodedField(token))) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + r.Header.Set("Sec-Fetch-Site", "same-origin") + for _, c := range issuing.Result().Cookies() { + r.AddCookie(c) + } + + if code, ran := reached(t, r); !ran { + t.Errorf("a freshly issued token was refused by the guard with %d; no form on the "+ + "site could be submitted", code) + } +} + +// ── Rendering ───────────────────────────────────────────────────────────── + +// CSRFInput returns template.HTML, which the template engine trusts verbatim. +// Anything unescaped in it is stored XSS on every page with a form, so the +// escaping here is the only thing between a token value and script execution. +func TestCSRFInputEscapesItsToken(t *testing.T) { + tests := map[string]struct { + token string + absent []string + present []string + }{ + "an ordinary token": { + token: "abc-_123", + present: []string{`name="` + CSRFField + `"`, `value="abc-_123"`, `type="hidden"`}, + }, + "a token that tries to close the attribute": { + token: `">`, + absent: []string{`">`}, + }, + "a token containing a single quote and an angle bracket": { + token: `'`, + absent: []string{``}, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + got := string(CSRFInput(tt.token)) + for _, want := range tt.present { + if !strings.Contains(got, want) { + t.Errorf("rendered field %q is missing %q", got, want) + } + } + for _, bad := range tt.absent { + if strings.Contains(got, bad) { + t.Errorf("rendered field %q contains %q unescaped, which is script "+ + "execution on every page carrying a form", got, bad) + } + } + }) + } + + // The declared type matters as much as the content: a plain string would be + // escaped again by the template engine and render as visible markup. + var _ template.HTML = CSRFInput("x") +} + +// ── Constant-time comparison ────────────────────────────────────────────── + +// Token comparison must not stop at the first differing byte. It is used for +// the CSRF token and for the OIDC state parameter, and in both cases a timing +// oracle lets an attacker recover the value one byte at a time. +func TestConstantTimeEqual(t *testing.T) { + tests := []struct { + name string + a, b string + want bool + }{ + {"identical", "abc123", "abc123", true}, + {"different values", "abc123", "xyz789", false}, + {"differing only in the last byte", "abc123", "abc124", false}, + {"differing only in the first byte", "abc123", "bbc123", false}, + {"a prefix of the other", "abc", "abc123", false}, + {"differing case", "ABC", "abc", false}, + {"one empty", "", "abc", false}, + {"the other empty", "abc", "", false}, + // Surprising, and the reason csrf.go rejects an empty cookie before it + // ever reaches this function. Pinned so the assumption stays visible. + {"both empty", "", "", true}, + {"unicode, identical", "æøå", "æøå", true}, + {"unicode, different", "æøå", "æøa", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := constantTimeEqual(tt.a, tt.b); got != tt.want { + t.Errorf("constantTimeEqual(%q, %q) = %v, want %v", tt.a, tt.b, got, tt.want) + } + if got := ConstantTimeEqual(tt.a, tt.b); got != tt.want { + t.Errorf("the exported ConstantTimeEqual disagrees with the unexported one "+ + "for (%q, %q)", tt.a, tt.b) + } + }) + } +} + +// Timing cannot be measured reliably in a unit test, so this reads the source +// instead. Replacing subtle.ConstantTimeCompare with == would leave every test +// above passing while quietly reintroducing the oracle. +func TestTokenComparisonIsConstantTimeInSource(t *testing.T) { + src, err := os.ReadFile("middleware.go") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(src), "subtle.ConstantTimeCompare(") { + t.Error("constantTimeEqual no longer uses subtle.ConstantTimeCompare; CSRF tokens " + + "and the OIDC state parameter would become recoverable a byte at a time") + } +} diff --git a/internal/auth/hs256_edge_test.go b/internal/auth/hs256_edge_test.go new file mode 100644 index 0000000..da03d3c --- /dev/null +++ b/internal/auth/hs256_edge_test.go @@ -0,0 +1,548 @@ +package auth + +// Adversarial tests for the HS256 ID-token verifier and for the claim checks +// go-oidc layers on top of it. Everything here is offline: the verifier is +// constructed directly with oidc.NewVerifier, so no provider is contacted and +// no clock is real — oidc.Config.Now is frozen instead. + +import ( + "context" + "crypto/hmac" + "crypto/sha256" + "encoding/base64" + "os" + "strconv" + "strings" + "testing" + "time" + + "github.com/coreos/go-oidc/v3/oidc" +) + +// itoa renders a Unix timestamp for embedding in a hand-written claim set. +func itoa(n int64) string { return strconv.FormatInt(n, 10) } + +// hmacSecret is the shared secret these tests sign with. Distinct from the +// session-sealing secret in auth_test.go so that a mix-up between the two shows +// up as a failure rather than accidentally passing. +const hmacSecret = "id-token-hmac-secret-0123456789ab" + +func b64seg(s string) string { return base64.RawURLEncoding.EncodeToString([]byte(s)) } + +// mintJWT signs the given header and payload with secret. The declared alg and +// the algorithm actually used are deliberately decoupled: forging an +// "alg": "none" or RS256-labelled token that nonetheless carries a valid HMAC +// is exactly the attack the verifier has to refuse, and a helper that kept them +// in step could not express it. +func mintJWT(t *testing.T, secret, header, payload string) string { + t.Helper() + h, p := b64seg(header), b64seg(payload) + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write([]byte(h + "." + p)) + return h + "." + p + "." + base64.RawURLEncoding.EncodeToString(mac.Sum(nil)) +} + +// signOver produces the correct signature segment for two already-encoded +// segments, letting a test sign material that is not valid base64 or not JSON. +func signOver(secret, encodedHeader, encodedPayload string) string { + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write([]byte(encodedHeader + "." + encodedPayload)) + return base64.RawURLEncoding.EncodeToString(mac.Sum(nil)) +} + +func hs256Header(alg string) string { return `{"alg":"` + alg + `","typ":"JWT"}` } + +// ── Signature integrity ─────────────────────────────────────────────────── + +// A payload that has been edited after signing must not verify. If it did, +// anyone who could intercept a token could promote themselves to Styret by +// rewriting the roles claim. +func TestHS256RejectsTamperedPayload(t *testing.T) { + ks := hmacKeySet{secret: []byte(hmacSecret)} + honest := mintJWT(t, hmacSecret, hs256Header("HS256"), `{"sub":"fa-1","roles":["Medlem"]}`) + parts := strings.Split(honest, ".") + + forged := parts[0] + "." + b64seg(`{"sub":"fa-1","roles":["Styret"]}`) + "." + parts[2] + if _, err := ks.VerifySignature(context.Background(), forged); err == nil { + t.Error("a token whose payload was rewritten after signing verified; " + + "any member could grant themselves the board role") + } +} + +// Flipping bits in the signature must fail. This also covers the case of an +// attacker who knows the payload they want and is guessing at the MAC. +func TestHS256RejectsMutatedSignature(t *testing.T) { + ks := hmacKeySet{secret: []byte(hmacSecret)} + honest := mintJWT(t, hmacSecret, hs256Header("HS256"), `{"sub":"fa-1"}`) + parts := strings.Split(honest, ".") + + sig, err := base64.RawURLEncoding.DecodeString(parts[2]) + if err != nil { + t.Fatal(err) + } + + tests := map[string]func() string{ + "first byte flipped": func() string { + m := append([]byte(nil), sig...) + m[0] ^= 0xFF + return base64.RawURLEncoding.EncodeToString(m) + }, + "last byte flipped": func() string { + m := append([]byte(nil), sig...) + m[len(m)-1] ^= 0x01 + return base64.RawURLEncoding.EncodeToString(m) + }, + "truncated to 16 bytes": func() string { + return base64.RawURLEncoding.EncodeToString(sig[:16]) + }, + "extended with a trailing byte": func() string { + return base64.RawURLEncoding.EncodeToString(append(append([]byte(nil), sig...), 0x00)) + }, + "empty": func() string { return "" }, + "all zeroes": func() string { + return base64.RawURLEncoding.EncodeToString(make([]byte, sha256.Size)) + }, + } + + for name, mutate := range tests { + t.Run(name, func(t *testing.T) { + forged := parts[0] + "." + parts[1] + "." + mutate() + if _, err := ks.VerifySignature(context.Background(), forged); err == nil { + t.Errorf("a token with a %s signature was accepted, so the MAC is not "+ + "actually gating anything", name) + } + }) + } +} + +// Only the configured secret may verify. A key that is a prefix, a suffix, one +// byte different, or empty must all fail — otherwise rotating the shared secret +// would not actually invalidate anything. +func TestHS256RejectsEveryOtherKey(t *testing.T) { + honest := mintJWT(t, hmacSecret, hs256Header("HS256"), `{"sub":"fa-1"}`) + + wrong := map[string]string{ + "empty": "", + "one byte shorter": hmacSecret[:len(hmacSecret)-1], + "one byte longer": hmacSecret + "x", + "one character changed": strings.Replace(hmacSecret, "a", "b", 1), + "unrelated same length": strings.Repeat("f", len(hmacSecret)), + } + for name, secret := range wrong { + t.Run(name, func(t *testing.T) { + ks := hmacKeySet{secret: []byte(secret)} + if _, err := ks.VerifySignature(context.Background(), honest); err == nil { + t.Errorf("a token signed with the real secret verified under a %s key", name) + } + }) + } +} + +// An empty configured secret is a misconfiguration, but it must not degrade +// into "accept anything" — HMAC with an empty key is still a real MAC. +func TestHS256WithEmptySecretStillRequiresACorrectMAC(t *testing.T) { + ks := hmacKeySet{secret: nil} + if _, err := ks.VerifySignature(context.Background(), + mintJWT(t, hmacSecret, hs256Header("HS256"), `{"sub":"fa-1"}`)); err == nil { + t.Error("an empty verification key accepted a token signed with a different key") + } + if _, err := ks.VerifySignature(context.Background(), + mintJWT(t, "", hs256Header("HS256"), `{"sub":"fa-1"}`)); err != nil { + t.Errorf("HMAC with an empty key should still verify its own output: %v", err) + } +} + +// ── Algorithm confusion ─────────────────────────────────────────────────── + +// The verifier must assert HS256 rather than dispatch on the token's own +// header. Every entry here carries a *valid* HMAC over its segments, so the +// only thing that can reject them is the algorithm assertion — which is the +// point: a verifier that trusted the header would accept all of them. +func TestHS256RefusesAnyHeaderThatIsNotExactlyHS256(t *testing.T) { + ks := hmacKeySet{secret: []byte(hmacSecret)} + + headers := map[string]string{ + "alg none": `{"alg":"none"}`, + "alg None": `{"alg":"None"}`, + "alg NONE": `{"alg":"NONE"}`, + "alg empty string": `{"alg":""}`, + "alg absent": `{"typ":"JWT"}`, + "alg null": `{"alg":null}`, + "alg RS256": `{"alg":"RS256"}`, + "alg ES256": `{"alg":"ES256"}`, + "alg HS384": `{"alg":"HS384"}`, + "alg HS512": `{"alg":"HS512"}`, + "alg lowercase hs256": `{"alg":"hs256"}`, + "alg with trailing space": `{"alg":"HS256 "}`, + "alg with leading space": `{"alg":" HS256"}`, + // Not a string at all. A verifier that scanned the raw header bytes for + // the substring "HS256" rather than decoding it would be fooled. + "alg is an array containing HS256": `{"alg":["HS256"]}`, + "alg is a nested object": `{"alg":{"alg":"HS256"}}`, + // encoding/json keeps the last value for a duplicated key, so this + // header resolves to "none". A verifier that merely searched the raw + // header bytes for "HS256" would accept it. + "alg repeated, none second": `{"alg":"HS256","alg":"none"}`, + } + + for name, header := range headers { + t.Run(name, func(t *testing.T) { + token := mintJWT(t, hmacSecret, header, `{"sub":"attacker","roles":["Styret"]}`) + if _, err := ks.VerifySignature(context.Background(), token); err == nil { + t.Errorf("a correctly MACed token with header %s was accepted; the verifier "+ + "is trusting the token's own algorithm claim", header) + } + }) + } +} + +// The one header that must work, including when the provider adds fields we do +// not read. Refusing unknown header fields would break on any FusionAuth +// upgrade that starts emitting "kid". +func TestHS256AcceptsHS256WithUnknownHeaderFields(t *testing.T) { + ks := hmacKeySet{secret: []byte(hmacSecret)} + header := `{"typ":"JWT","kid":"abc123","alg":"HS256","cty":"JWT"}` + + payload, err := ks.VerifySignature(context.Background(), + mintJWT(t, hmacSecret, header, `{"sub":"fa-1"}`)) + if err != nil { + t.Fatalf("a valid HS256 token with extra header fields was rejected, which would "+ + "break login the moment FusionAuth adds a key id: %v", err) + } + if string(payload) != `{"sub":"fa-1"}` { + t.Errorf("payload came back altered: %s", payload) + } +} + +// ── Shape ───────────────────────────────────────────────────────────────── + +// Anything that is not exactly three dot-separated segments must be refused +// before any cryptography happens. A verifier that indexed into the parts +// without checking would panic on short input, turning a malformed cookie into +// a denial of service. +func TestHS256RejectsWrongSegmentCounts(t *testing.T) { + ks := hmacKeySet{secret: []byte(hmacSecret)} + + for _, token := range []string{ + "", + ".", + "..", + "...", + "....", + "a", + "a.b", + "a.b.c.d", + strings.Repeat(".", 100), + b64seg(hs256Header("HS256")) + "." + b64seg(`{"sub":"x"}`), // signature dropped + } { + t.Run("token "+token, func(t *testing.T) { + if _, err := ks.VerifySignature(context.Background(), token); err == nil { + t.Errorf("%q was accepted as a JWT", token) + } + }) + } +} + +// Each segment must be raw base64url. Standard-alphabet and padded encodings +// are not the same thing and must not be quietly tolerated. +func TestHS256RejectsInvalidBase64(t *testing.T) { + ks := hmacKeySet{secret: []byte(hmacSecret)} + goodHeader := b64seg(hs256Header("HS256")) + goodPayload := b64seg(`{"sub":"fa-1"}`) + + tests := map[string]string{ + "header is not base64": "!!!!." + goodPayload + "." + signOver(hmacSecret, "!!!!", goodPayload), + "header uses padding": "eyJhbGciOiJIUzI1NiJ9=." + goodPayload + ".sig", + "header uses the + and / alphabet": "ab+/cd." + goodPayload + "." + signOver(hmacSecret, "ab+/cd", goodPayload), + "signature is not base64": goodHeader + "." + goodPayload + ".!!!!", + "signature is padded": goodHeader + "." + goodPayload + ".YWJj=", + } + for name, token := range tests { + t.Run(name, func(t *testing.T) { + if _, err := ks.VerifySignature(context.Background(), token); err == nil { + t.Errorf("%s: token accepted", name) + } + }) + } +} + +// A payload segment that is correctly signed but not decodable must still fail. +// This pins the ordering inside the verifier — signature first, decode second — +// which is what keeps an attacker from using decode errors as an oracle. +func TestHS256ChecksTheSignatureBeforeDecodingThePayload(t *testing.T) { + ks := hmacKeySet{secret: []byte(hmacSecret)} + header := b64seg(hs256Header("HS256")) + badPayload := "!!!not-base64!!!" + + token := header + "." + badPayload + "." + signOver(hmacSecret, header, badPayload) + if _, err := ks.VerifySignature(context.Background(), token); err == nil { + t.Error("a token with an undecodable payload was accepted") + } else if !strings.Contains(err.Error(), "payload") { + t.Errorf("expected the failure to name the payload, got %v", err) + } +} + +func TestHS256RejectsUnreadableHeader(t *testing.T) { + ks := hmacKeySet{secret: []byte(hmacSecret)} + + for name, header := range map[string]string{ + "not JSON at all": `this is not json`, + "a JSON array": `["HS256"]`, + "a JSON string": `"HS256"`, + "alg is a number": `{"alg":256}`, + "empty": ``, + } { + t.Run(name, func(t *testing.T) { + token := mintJWT(t, hmacSecret, header, `{"sub":"fa-1"}`) + if _, err := ks.VerifySignature(context.Background(), token); err == nil { + t.Errorf("a token whose header was %s was accepted", name) + } + }) + } +} + +// The verifier's contract is signature checking only: it hands back the raw +// payload bytes and leaves claim parsing to go-oidc. A non-JSON payload +// therefore comes back without error. This is not a defect, but it is a +// boundary worth pinning — if this file ever grows a claim check, callers must +// not end up validating claims twice with different rules. +func TestHS256ReturnsThePayloadVerbatimWithoutParsingIt(t *testing.T) { + ks := hmacKeySet{secret: []byte(hmacSecret)} + + for _, payload := range []string{`not json`, `[]`, `null`, `{}`, ``} { + got, err := ks.VerifySignature(context.Background(), + mintJWT(t, hmacSecret, hs256Header("HS256"), payload)) + if err != nil { + t.Fatalf("a correctly signed token with payload %q was rejected here rather "+ + "than by the claim parser: %v", payload, err) + } + if string(got) != payload { + t.Errorf("payload %q came back as %q", payload, got) + } + } +} + +// ── Constant time ───────────────────────────────────────────────────────── + +// Signature comparison must not short-circuit on the first differing byte: the +// timing difference is enough to forge a MAC one byte at a time. Timing cannot +// be asserted reliably in a unit test, so this reads the source instead. A +// refactor that swaps hmac.Equal for bytes.Equal or == is a security +// regression, and it must fail here rather than pass silently. +func TestHS256UsesAConstantTimeComparison(t *testing.T) { + src, err := os.ReadFile("hs256.go") + if err != nil { + t.Fatal(err) + } + code := string(src) + if !strings.Contains(code, "hmac.Equal(") { + t.Error("hs256.go no longer calls hmac.Equal; signature comparison must be " + + "constant-time or the MAC can be forged a byte at a time") + } + for _, leaky := range []string{"bytes.Equal(", "string(mac.Sum"} { + if strings.Contains(code, leaky) { + t.Errorf("hs256.go contains %q, which compares in variable time", leaky) + } + } +} + +// ── Claim validation, as go-oidc performs it over this key set ──────────── + +// frozenVerifier builds the real ID-token verifier this package uses in +// production, with the clock pinned so expiry tests can never be flaky. +func frozenVerifier(issuer, clientID string, now time.Time) *oidc.IDTokenVerifier { + return oidc.NewVerifier(issuer, hmacKeySet{secret: []byte(hmacSecret)}, &oidc.Config{ + ClientID: clientID, + SupportedSigningAlgs: []string{"HS256"}, + Now: func() time.Time { return now }, + }) +} + +func TestIDTokenTimeClaims(t *testing.T) { + const issuer, clientID = "https://auth.example", "itemize-web" + now := time.Date(2026, 8, 9, 12, 0, 0, 0, time.UTC) + verifier := frozenVerifier(issuer, clientID, now) + + // claimsJSON builds a minimal-but-valid token body, letting each case + // override only the time claims it cares about. + body := func(extra string) string { + return `{"iss":"` + issuer + `","aud":"` + clientID + `","sub":"fa-1"` + extra + `}` + } + tests := []struct { + name string + extra string + wantErr bool + why string + }{ + { + name: "expires in an hour", + extra: `,"exp":` + itoa(now.Add(time.Hour).Unix()) + `,"iat":` + itoa(now.Unix()), + why: "an ordinary fresh token must log the member in", + }, + { + name: "expired a second ago", + extra: `,"exp":` + itoa(now.Add(-time.Second).Unix()), + wantErr: true, + why: "an expired ID token must not establish a session", + }, + { + name: "expires exactly now", + extra: `,"exp":` + itoa(now.Unix()), + // go-oidc uses Expiry.Before(now), so the boundary second is still + // valid. Pinned deliberately: if this ever flips, logins will start + // failing intermittently for tokens issued with a zero lifetime. + why: "the exp boundary is inclusive", + }, + { + name: "no exp claim at all", + extra: ``, + wantErr: true, + why: "a token without exp decodes to the zero time, which must read as " + + "expired rather than as never expiring", + }, + { + name: "exp far in the past", + extra: `,"exp":1`, + wantErr: true, + why: "a decade-old token must not be replayable", + }, + { + name: "nbf four minutes in the future", + extra: `,"exp":` + itoa(now.Add(time.Hour).Unix()) + `,"nbf":` + itoa(now.Add(4*time.Minute).Unix()), + why: "go-oidc allows five minutes of clock skew on nbf; a slightly fast provider must still work", + }, + { + name: "nbf six minutes in the future", + extra: `,"exp":` + itoa(now.Add(time.Hour).Unix()) + `,"nbf":` + itoa(now.Add(6*time.Minute).Unix()), + wantErr: true, + why: "beyond the skew allowance a not-yet-valid token must be refused", + }, + { + name: "nbf in the past", + extra: `,"exp":` + itoa(now.Add(time.Hour).Unix()) + `,"nbf":` + itoa(now.Add(-time.Hour).Unix()), + why: "a token that became valid an hour ago is fine", + }, + { + name: "iat in the future", + extra: `,"exp":` + itoa(now.Add(time.Hour).Unix()) + `,"iat":` + itoa(now.Add(time.Hour).Unix()), + // Characterisation, not endorsement: go-oidc does not check iat at + // all. Recorded so that anyone reasoning about replay windows knows + // iat is decorative here and only exp and nbf are enforced. + why: "iat is not validated by go-oidc", + }, + { + name: "iat missing", + extra: `,"exp":` + itoa(now.Add(time.Hour).Unix()), + why: "iat is optional in practice", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + token := mintJWT(t, hmacSecret, hs256Header("HS256"), body(tt.extra)) + _, err := verifier.Verify(context.Background(), token) + if tt.wantErr && err == nil { + t.Errorf("token was accepted but should not have been (%s)", tt.why) + } + if !tt.wantErr && err != nil { + t.Errorf("token was rejected but should have been accepted (%s): %v", tt.why, err) + } + }) + } +} + +func TestIDTokenIssuerAndAudience(t *testing.T) { + const issuer, clientID = "https://auth.example", "itemize-web" + now := time.Date(2026, 8, 9, 12, 0, 0, 0, time.UTC) + verifier := frozenVerifier(issuer, clientID, now) + exp := itoa(now.Add(time.Hour).Unix()) + + tests := []struct { + name string + body string + wantErr bool + why string + }{ + { + name: "correct issuer and audience", + body: `{"iss":"` + issuer + `","aud":"` + clientID + `","sub":"fa-1","exp":` + exp + `}`, + why: "our own provider's token must work", + }, + { + name: "issuer is a different provider", + body: `{"iss":"https://evil.example","aud":"` + clientID + `","sub":"fa-1","exp":` + exp + `}`, + wantErr: true, + why: "a token minted elsewhere must never be accepted, however well signed", + }, + { + name: "issuer differs only by a trailing slash", + body: `{"iss":"` + issuer + `/","aud":"` + clientID + `","sub":"fa-1","exp":` + exp + `}`, + wantErr: true, + why: "issuer matching is exact; this is the misconfiguration New's error message warns about", + }, + { + name: "issuer missing", + body: `{"aud":"` + clientID + `","sub":"fa-1","exp":` + exp + `}`, + wantErr: true, + why: "an absent issuer must not pass as a match", + }, + { + name: "audience is another application on the same tenant", + body: `{"iss":"` + issuer + `","aud":"the-wiki","sub":"fa-1","exp":` + exp + `}`, + wantErr: true, + why: "FusionAuth hosts the wiki on the same tenant; a token issued to it must not " + + "be replayable against the website", + }, + { + name: "audience is a list containing us", + body: `{"iss":"` + issuer + `","aud":["the-wiki","` + clientID + `"],"sub":"fa-1","exp":` + exp + `}`, + why: "a multi-audience token that names us is valid", + }, + { + name: "audience is a list not containing us", + body: `{"iss":"` + issuer + `","aud":["the-wiki","something-else"],"sub":"fa-1","exp":` + exp + `}`, + wantErr: true, + why: "membership in the audience list must be checked, not merely its presence", + }, + { + name: "audience missing", + body: `{"iss":"` + issuer + `","sub":"fa-1","exp":` + exp + `}`, + wantErr: true, + why: "a token with no audience is not addressed to us", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := verifier.Verify(context.Background(), + mintJWT(t, hmacSecret, hs256Header("HS256"), tt.body)) + if tt.wantErr && err == nil { + t.Errorf("token was accepted but should not have been (%s)", tt.why) + } + if !tt.wantErr && err != nil { + t.Errorf("token was rejected but should have been accepted (%s): %v", tt.why, err) + } + }) + } +} + +// go-oidc filters on the declared algorithm before the key set is consulted, so +// this is a second, independent barrier against algorithm confusion. Both must +// hold: SupportedSigningAlgs here, and the assertion inside hmacKeySet. +func TestVerifierRejectsUnsupportedAlgorithmsBeforeReachingTheKeySet(t *testing.T) { + const issuer, clientID = "https://auth.example", "itemize-web" + now := time.Date(2026, 8, 9, 12, 0, 0, 0, time.UTC) + verifier := frozenVerifier(issuer, clientID, now) + + body := `{"iss":"` + issuer + `","aud":"` + clientID + `","sub":"fa-1","exp":` + + itoa(now.Add(time.Hour).Unix()) + `}` + + for _, alg := range []string{"none", "RS256", "HS512", "ES256"} { + t.Run(alg, func(t *testing.T) { + if _, err := verifier.Verify(context.Background(), + mintJWT(t, hmacSecret, hs256Header(alg), body)); err == nil { + t.Errorf("the verifier accepted a token declaring alg=%q", alg) + } + }) + } +} diff --git a/internal/auth/login_stale_cookie_test.go b/internal/auth/login_stale_cookie_test.go new file mode 100644 index 0000000..4d90d7b --- /dev/null +++ b/internal/auth/login_stale_cookie_test.go @@ -0,0 +1,141 @@ +package auth + +// Inject clears a session cookie it cannot open (middleware.go:19). The login +// callback sets a new one. Both run on the same response for the same cookie +// name, because Inject wraps the whole mux — the callback route included +// (cmd/website/main.go:146-158). These tests are about that overlap. +// +// The case is not hypothetical: it is exactly what every signed-in member hits +// the first time they log in after the session key is rotated. Their old cookie +// no longer opens, so Inject clears it on the very request that is trying to +// establish the new session. If the clearing were to win, the rotation would +// lock out every member until they cleared their own cookies by hand — with no +// error anywhere, because each half is behaving as designed. + +import ( + "net/http" + "net/http/httptest" + "net/url" + "testing" +) + +// injectThenCallback runs the callback through Inject, the way the real chain +// does, rather than calling Callback on its own. +func injectThenCallback(t *testing.T, a *Authenticator, flow *http.Cookie, + query url.Values, stale string) *httptest.ResponseRecorder { + t.Helper() + + r := httptest.NewRequest(http.MethodGet, "/callback?"+query.Encode(), nil) + if flow != nil { + r.AddCookie(flow) + } + if stale != "" { + r.AddCookie(&http.Cookie{Name: SessionCookie, Value: stale}) + } + + rec := httptest.NewRecorder() + a.Inject(http.HandlerFunc(a.Callback)).ServeHTTP(rec, r) + return rec +} + +// A member whose old cookie cannot be opened must still be able to log in. The +// response carries two Set-Cookie headers for the same name — Inject's clear +// and the callback's new session — and the browser keeps the last one. +func TestLoginSucceedsDespiteAStaleSessionCookie(t *testing.T) { + idp := newFakeIDP(t) + a, sealer := newAuthenticator(t, idp, "HS256", "") + + authURL, flow := startLogin(t, a, "/profil") + idp.idToken = signClaims(t, testClientSecret, idp.claimsFor(authURL.Query().Get("nonce"))) + + // Not openable with the current key: what a cookie sealed with the previous + // key looks like after a rotation. + rec := injectThenCallback(t, a, flow, url.Values{ + "state": {authURL.Query().Get("state")}, + "code": {"the-authorization-code"}, + }, "not-a-cookie-this-key-can-open") + + if rec.Code != http.StatusFound { + t.Fatalf("the callback returned %d rather than a redirect: %s", rec.Code, rec.Body.String()) + } + + sess := sessionFrom(t, sealer, rec) + if sess == nil { + t.Fatal("no readable session survived the response, so a member logging in after a " + + "key rotation would be bounced straight back to the login page — and would stay " + + "stuck there, because every attempt carries the same stale cookie") + } + if sess.ID != "11111111-2222-4333-8444-999999999999" { + t.Errorf("the established session is not the one the provider just vouched for: %q", sess.ID) + } +} + +// The order matters, not just the presence of both headers. A browser applies +// Set-Cookie in the order it receives them, so the clear has to come first; if +// a refactor ever moved Inject's write after the handler's, the member would be +// logged straight back out and the test above would still pass whenever the +// recorder happened to hand back the working cookie. +func TestTheNewSessionCookieIsWrittenAfterTheClear(t *testing.T) { + idp := newFakeIDP(t) + a, _ := newAuthenticator(t, idp, "HS256", "") + + authURL, flow := startLogin(t, a, "") + idp.idToken = signClaims(t, testClientSecret, idp.claimsFor(authURL.Query().Get("nonce"))) + + rec := injectThenCallback(t, a, flow, url.Values{ + "state": {authURL.Query().Get("state")}, + "code": {"the-authorization-code"}, + }, "not-a-cookie-this-key-can-open") + + var cleared, established int + for i, c := range rec.Result().Cookies() { + if c.Name != SessionCookie { + continue + } + if c.Value == "" { + cleared = i + 1 + continue + } + established = i + 1 + } + + if cleared == 0 { + t.Fatal("Inject did not clear the unopenable cookie, so it would be re-sent on every " + + "later request for as long as the browser kept it") + } + if established == 0 { + t.Fatal("the callback established no session cookie") + } + if cleared > established { + t.Errorf("the clearing Set-Cookie is emitted after the new session (positions %d and %d); "+ + "a browser applies them in order, so the member would be logged out by the very "+ + "response that logged them in", cleared, established) + } +} + +// The clear must not fire for a visitor who sent no cookie at all: a first-time +// member logging in should receive exactly one session cookie, not a clear they +// never needed followed by the real one. +func TestAFirstLoginSetsOnlyOneSessionCookie(t *testing.T) { + idp := newFakeIDP(t) + a, _ := newAuthenticator(t, idp, "HS256", "") + + authURL, flow := startLogin(t, a, "") + idp.idToken = signClaims(t, testClientSecret, idp.claimsFor(authURL.Query().Get("nonce"))) + + rec := injectThenCallback(t, a, flow, url.Values{ + "state": {authURL.Query().Get("state")}, + "code": {"the-authorization-code"}, + }, "") + + var n int + for _, c := range rec.Result().Cookies() { + if c.Name == SessionCookie { + n++ + } + } + if n != 1 { + t.Errorf("a first-time login wrote %d %s cookies, want 1 — Inject is clearing a cookie "+ + "the visitor never sent", n, SessionCookie) + } +} diff --git a/internal/auth/middleware.go b/internal/auth/middleware.go index 54b4a1d..501a450 100644 --- a/internal/auth/middleware.go +++ b/internal/auth/middleware.go @@ -2,6 +2,7 @@ package auth import ( "crypto/subtle" + "encoding/json" "net/http" "net/url" ) @@ -15,6 +16,21 @@ func (a *Authenticator) Inject(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { sess := a.sealer.Read(r) if sess == nil { + // Clear it, so the claim above holds. A cookie that cannot be + // opened would otherwise be re-sent on every request for as long as + // the browser keeps it — the visitor is anonymous either way, but + // each request carries a kilobyte of dead weight forever. Only when + // one was actually sent: a first-time visitor has nothing to clear + // and must not be handed a Set-Cookie they never asked for. + // + // Written before the handler runs, deliberately. The login callback + // is behind this middleware too, and a member logging in after a key + // rotation arrives with a stale cookie on the very request that + // establishes the new one. A browser applies Set-Cookie in order, so + // the clear has to come first or it would undo the login. + if _, err := r.Cookie(SessionCookie); err == nil { + a.sealer.Clear(w) + } next.ServeHTTP(w, r) return } @@ -93,28 +109,26 @@ func RequireRoleAPI(role string) func(http.Handler) http.Handler { } } +// jsonError is the shape of an error body. Declared here rather than taken +// from the api package, which depends on this one. +type jsonError struct { + Message string `json:"message"` +} + func writeJSONError(w http.ResponseWriter, status int, msg string) { w.Header().Set("Content-Type", "application/json; charset=utf-8") w.WriteHeader(status) - // Small enough to hand-write, and doing so keeps this package free of a - // dependency on the api package, which depends on this one. - _, _ = w.Write([]byte(`{"message":` + quoteJSON(msg) + `}`)) -} - -func quoteJSON(s string) string { - out := make([]byte, 0, len(s)+2) - out = append(out, '"') - for i := 0; i < len(s); i++ { - switch c := s[i]; c { - case '"', '\\': - out = append(out, '\\', c) - case '\n': - out = append(out, '\\', 'n') - default: - out = append(out, c) - } - } - return string(append(out, '"')) + // encoding/json rather than hand-rolled quoting: the saving was one + // allocation on an error path, and the cost was a body that stopped being + // parseable the moment a message contained a tab, a carriage return or any + // other control character — which is exactly what happens the first time + // somebody passes a provider or database error through here. + // + // Marshalling a struct of one string cannot fail: invalid UTF-8 is replaced + // rather than rejected, so there is no error to report and nowhere to + // report it once the status has gone out. + body, _ := json.Marshal(jsonError{Message: msg}) + _, _ = w.Write(body) } func constantTimeEqual(a, b string) bool { diff --git a/internal/auth/middleware_edge_test.go b/internal/auth/middleware_edge_test.go new file mode 100644 index 0000000..32300a2 --- /dev/null +++ b/internal/auth/middleware_edge_test.go @@ -0,0 +1,643 @@ +package auth + +// Edge cases for context injection, the role gates, and the User predicates +// the gates are built on. + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" +) + +// injector builds the smallest Authenticator that Inject needs. Inject reads +// nothing but the sealer, so a full one — which would require reaching an +// identity provider — is not warranted here. +func injector(sealer *Sealer) *Authenticator { return &Authenticator{sealer: sealer} } + +// seeing records the user the wrapped handler was given, so a test can assert +// on what reached the handler rather than only on the status code. +func seeing(got **User, ran *bool) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + *got = FromRequest(r) + *ran = true + w.WriteHeader(http.StatusOK) + }) +} + +// ── Inject ──────────────────────────────────────────────────────────────── + +// Inject must never fail a request. Whatever the cookie jar contains, the worst +// outcome is an anonymous visitor — a broken cookie turning into a 500 would +// lock everybody out of the whole site until the secret was rolled back. +func TestInjectNeverFailsARequest(t *testing.T) { + sealer := newTestSealer(t, testSecret, true) + other := newTestSealer(t, "ffffffffffffffffffffffffffffffff", true) + + live, err := sealer.Seal(NewSession(User{ID: "fa-1", Name: "Kari", + Roles: []string{RoleStyret}}, time.Now().Add(time.Hour))) + if err != nil { + t.Fatal(err) + } + expired, err := sealer.Seal(&Session{User: User{ID: "fa-1"}, + Expires: time.Now().Add(-time.Minute)}) + if err != nil { + t.Fatal(err) + } + foreign, err := other.Seal(NewSession(User{ID: "attacker", Roles: []string{RoleStyret}}, + time.Now().Add(time.Hour))) + if err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + value *string + wantUser bool + why string + }{ + {"no cookie", nil, false, "an ordinary first-time visitor is anonymous"}, + {"a valid session", &live, true, "the normal signed-in case"}, + {"an empty cookie", ptr(""), false, "an emptied cookie is not a session"}, + {"garbage", ptr("!!!!"), false, "a corrupted cookie must not fail the request"}, + {"a truncated cookie", ptr(live[:len(live)/2]), false, + "a cookie clipped in transit must not decrypt into a partial user"}, + {"a session sealed under a rotated secret", &foreign, false, + "after a secret rotation every old cookie must read as anonymous, and a " + + "cookie sealed by anyone else must never grant a role"}, + {"an expired session", &expired, false, + "expiry must be enforced here, not only when the cookie was written"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/", nil) + if tt.value != nil { + r.AddCookie(&http.Cookie{Name: SessionCookie, Value: *tt.value}) + } + + var got *User + var ran bool + rec := httptest.NewRecorder() + injector(sealer).Inject(seeing(&got, &ran)).ServeHTTP(rec, r) + + if !ran { + t.Fatalf("Inject swallowed the request instead of passing it on (%s)", tt.why) + } + if rec.Code != http.StatusOK { + t.Errorf("status %d; Inject must never fail a request (%s)", rec.Code, tt.why) + } + if tt.wantUser && got == nil { + t.Errorf("no user reached the handler: %s", tt.why) + } + if !tt.wantUser && got != nil { + t.Errorf("the handler was handed %+v: %s", *got, tt.why) + } + }) + } +} + +func ptr(s string) *string { return &s } + +// Inject copies the whole user into the context, not just an identifier — every +// page renders the display name and avatar straight out of it. +func TestInjectCarriesTheWholeUser(t *testing.T) { + sealer := newTestSealer(t, testSecret, true) + + want := User{ + ID: "fa-1", Name: "Bjørn", FullName: "Bjørn Ærlig Ødegård", + Email: "bjorn@example.no", ImageURL: "https://itemize.no/b.png", + Roles: []string{"Medlem", RoleStyret}, + } + sealed, err := sealer.Seal(NewSession(want, time.Now().Add(time.Hour))) + if err != nil { + t.Fatal(err) + } + + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.AddCookie(&http.Cookie{Name: SessionCookie, Value: sealed}) + + var got *User + var ran bool + injector(sealer).Inject(seeing(&got, &ran)).ServeHTTP(httptest.NewRecorder(), r) + + if got == nil { + t.Fatal("no user in the context") + } + if got.ID != want.ID || got.Name != want.Name || got.FullName != want.FullName || + got.Email != want.Email || got.ImageURL != want.ImageURL { + t.Errorf("got %+v, want %+v", *got, want) + } + if !got.IsStyret() { + t.Error("roles did not survive injection, so a board member would see no admin links") + } +} + +// A cookie Inject cannot open must be cleared, which is what its doc comment +// promises. Left in place it is re-sent on every single request for as long as +// the browser keeps it — after a secret rotation that is every request from +// every returning visitor, indefinitely, for a value that can never be read +// again. Clearing must not fail the request or change what the handler sees: +// the visitor is anonymous either way. +func TestInjectClearsACookieItCannotOpen(t *testing.T) { + sealer := newTestSealer(t, testSecret, true) + other := newTestSealer(t, "ffffffffffffffffffffffffffffffff", true) + + foreign, err := other.Seal(NewSession(User{ID: "fa-1"}, time.Now().Add(time.Hour))) + if err != nil { + t.Fatal(err) + } + expired, err := sealer.Seal(&Session{User: User{ID: "fa-1"}, + Expires: time.Now().Add(-time.Minute)}) + if err != nil { + t.Fatal(err) + } + + for name, value := range map[string]string{ + "garbage": "!!!!", + "an empty value": "", + "a session sealed under a rotated secret": foreign, + "a session that has passed its expiry": expired, + } { + t.Run(name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.AddCookie(&http.Cookie{Name: SessionCookie, Value: value}) + + rec := httptest.NewRecorder() + var got *User + var ran bool + injector(sealer).Inject(seeing(&got, &ran)).ServeHTTP(rec, r) + + if !ran || got != nil { + t.Fatalf("handler ran %v with user %v; clearing must not change the "+ + "anonymous outcome", ran, got) + } + + c := cookieNamed(t, rec, SessionCookie) + if c.Value != "" || c.MaxAge >= 0 { + t.Errorf("the response carried %s; a cookie that can never be opened "+ + "again must be expired, not re-issued", c.String()) + } + }) + } +} + +// A visitor who sent no session cookie must not be handed one back. Setting an +// expiring cookie for a name that was never in the jar is pure noise on every +// anonymous request — which is most of the traffic to a public site. +func TestInjectDoesNotClearACookieThatWasNeverSent(t *testing.T) { + sealer := newTestSealer(t, testSecret, true) + + r := httptest.NewRequest(http.MethodGet, "/", nil) + rec := httptest.NewRecorder() + var got *User + var ran bool + injector(sealer).Inject(seeing(&got, &ran)).ServeHTTP(rec, r) + + if cookies := rec.Result().Cookies(); len(cookies) != 0 { + t.Errorf("an anonymous request with no cookies came back with %d Set-Cookie "+ + "headers, the first being %s", len(cookies), cookies[0].String()) + } +} + +// The ordinary signed-in case must be left completely alone: a valid session +// that got cleared here would log the member out on their next page view. +func TestInjectLeavesAValidSessionCookieAlone(t *testing.T) { + sealer := newTestSealer(t, testSecret, true) + + sealed, err := sealer.Seal(NewSession(User{ID: "fa-1", Name: "Kari"}, + time.Now().Add(time.Hour))) + if err != nil { + t.Fatal(err) + } + + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.AddCookie(&http.Cookie{Name: SessionCookie, Value: sealed}) + + rec := httptest.NewRecorder() + var got *User + var ran bool + injector(sealer).Inject(seeing(&got, &ran)).ServeHTTP(rec, r) + + if got == nil { + t.Fatal("no user in the context for a valid session") + } + if cookies := rec.Result().Cookies(); len(cookies) != 0 { + t.Errorf("a valid session came back with %d Set-Cookie headers, the first being "+ + "%s; a signed-in member would be logged out on their next request", + len(cookies), cookies[0].String()) + } +} + +// ── Nil users ───────────────────────────────────────────────────────────── + +// A typed nil *User in the context must read as anonymous, not as a signed-in +// visitor with no roles. The difference is a redirect to login versus a 403 — +// and, if HasRole did not guard against nil, a panic. +func TestATypedNilUserIsAnonymous(t *testing.T) { + deny := func(w http.ResponseWriter, _ *http.Request, status int) { w.WriteHeader(status) } + + handlers := map[string]struct { + h http.Handler + want int + }{ + "RequireLogin": {RequireLogin(okHandler()), http.StatusFound}, + "RequireRole": {RequireRole(RoleStyret, deny)(okHandler()), http.StatusFound}, + "RequireLoginAPI": {RequireLoginAPI(okHandler()), http.StatusUnauthorized}, + "RequireRoleAPI": {RequireRoleAPI(RoleStyret)(okHandler()), http.StatusUnauthorized}, + } + + for name, tt := range handlers { + t.Run(name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/skjult", nil) + r = r.WithContext(WithUser(r.Context(), (*User)(nil))) + + rec := httptest.NewRecorder() + tt.h.ServeHTTP(rec, r) + if rec.Code != tt.want { + t.Errorf("got %d, want %d; a nil user in the context must be treated as "+ + "anonymous rather than as a signed-in member", rec.Code, tt.want) + } + }) + } +} + +// ── Role names ──────────────────────────────────────────────────────────── + +// Role comparison is exact. FusionAuth role names are free text set in an admin +// interface, so "styret" and "Styret" are different roles and only the second +// one grants access. Anything looser here would let a differently-cased or +// similarly-named role inherit board privileges. +func TestRoleMatchingIsExact(t *testing.T) { + deny := func(w http.ResponseWriter, _ *http.Request, status int) { w.WriteHeader(status) } + + tests := []struct { + name string + roles []string + want int + }{ + {"the role itself", []string{RoleStyret}, http.StatusOK}, + {"among several roles", []string{"Medlem", "Infra", RoleStyret}, http.StatusOK}, + {"the same role twice", []string{RoleStyret, RoleStyret}, http.StatusOK}, + {"lower case", []string{"styret"}, http.StatusForbidden}, + {"upper case", []string{"STYRET"}, http.StatusForbidden}, + {"trailing space", []string{"Styret "}, http.StatusForbidden}, + {"leading space", []string{" Styret"}, http.StatusForbidden}, + {"a prefix", []string{"Sty"}, http.StatusForbidden}, + {"a superstring", []string{"Styretmedlem"}, http.StatusForbidden}, + {"a similar Norwegian word", []string{"Styrer"}, http.StatusForbidden}, + {"no roles at all", nil, http.StatusForbidden}, + {"an empty role name", []string{""}, http.StatusForbidden}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/arrangementer/ny", nil) + r = r.WithContext(WithUser(r.Context(), &User{ID: "fa-1", Roles: tt.roles})) + + rec := httptest.NewRecorder() + RequireRole(RoleStyret, deny)(okHandler()).ServeHTTP(rec, r) + + if rec.Code != tt.want { + t.Errorf("a member holding %v got %d, want %d — role names must match "+ + "character for character", tt.roles, rec.Code, tt.want) + } + }) + } +} + +// A gate on a role nobody holds must refuse everyone rather than fall open. +func TestGatingOnAnUnknownRoleRefusesEveryone(t *testing.T) { + deny := func(w http.ResponseWriter, _ *http.Request, status int) { w.WriteHeader(status) } + + for _, role := range []string{"Admin", "Kasserer", "", "Styret\n"} { + t.Run("role "+role, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/skjult", nil) + r = r.WithContext(WithUser(r.Context(), + &User{ID: "fa-1", Roles: []string{"Medlem", RoleStyret}})) + + rec := httptest.NewRecorder() + RequireRole(role, deny)(okHandler()).ServeHTTP(rec, r) + if rec.Code != http.StatusForbidden { + t.Errorf("a gate on the unheld role %q returned %d rather than 403", role, rec.Code) + } + }) + } +} + +// ── Redirect construction ───────────────────────────────────────────────── + +// The login redirect has to carry the visitor back to where they were, and the +// return path has to survive the trip intact — including a query string, which +// is what "?old=1" on the events page relies on. It must also come back out of +// safeReturnTo unchanged, or the visitor lands on the front page instead. +func TestLoginRedirectPreservesTheRequestedPath(t *testing.T) { + deny := func(w http.ResponseWriter, _ *http.Request, status int) { w.WriteHeader(status) } + + paths := []string{ + "/profil", + "/arrangementer?old=1", + "/arrangementer?q=pizza&old=1", + "/arrangementer/68f0b3c1a2b3c4d5e6f70819/rediger", + "/s%C3%B8k?q=%C3%A6%C3%B8%C3%A5", + } + + for _, name := range []string{"RequireLogin", "RequireRole"} { + for _, path := range paths { + t.Run(name+" "+path, func(t *testing.T) { + var h http.Handler = RequireLogin(okHandler()) + if name == "RequireRole" { + h = RequireRole(RoleStyret, deny)(okHandler()) + } + + r := httptest.NewRequest(http.MethodGet, path, nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, r) + + if rec.Code != http.StatusFound { + t.Fatalf("got %d, want 302", rec.Code) + } + location, err := url.Parse(rec.Header().Get("Location")) + if err != nil { + t.Fatalf("the Location header is not a URL: %v", err) + } + if location.Path != "/login" { + t.Errorf("redirected to %q rather than /login", location.Path) + } + if location.Host != "" || location.Scheme != "" { + t.Errorf("the login redirect points off-site, to %q", location) + } + + returnTo := location.Query().Get("return_to") + if returnTo != r.URL.RequestURI() { + t.Errorf("return_to = %q, want %q; the visitor would not land back "+ + "where they were", returnTo, r.URL.RequestURI()) + } + // The round trip that matters: what the redirect puts in the URL + // must be what safeReturnTo hands back after login. + if got := safeReturnTo(returnTo); got != r.URL.RequestURI() { + t.Errorf("safeReturnTo(%q) = %q; the escaping done here and the "+ + "validation done at callback time disagree", returnTo, got) + } + }) + } + } +} + +// The query string is escaped into return_to rather than concatenated raw, so a +// visitor cannot smuggle extra parameters into the login URL. +func TestLoginRedirectEscapesTheReturnPath(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/x?a=1&return_to=%2F%2Fevil.example", nil) + + rec := httptest.NewRecorder() + RequireLogin(okHandler()).ServeHTTP(rec, r) + + location, err := url.Parse(rec.Header().Get("Location")) + if err != nil { + t.Fatal(err) + } + if got := location.Query()["return_to"]; len(got) != 1 { + t.Fatalf("the login URL carries %d return_to parameters (%v); a second one injected "+ + "through the original query string could win", len(got), got) + } + if got := safeReturnTo(location.Query().Get("return_to")); strings.HasPrefix(got, "//") { + t.Errorf("the round trip produced %q, a protocol-relative URL browsers follow "+ + "off-site", got) + } +} + +// ── The JSON error contract ─────────────────────────────────────────────── + +// The previous API used 401 for a missing role, and its clients branch on the +// message text. Both are load-bearing compatibility, not style: changing either +// breaks callers silently. +func TestAPIErrorBodies(t *testing.T) { + tests := []struct { + name string + handler http.Handler + user *User + status int + message string + }{ + {"not logged in", RequireLoginAPI(okHandler()), nil, + http.StatusUnauthorized, "You are not logged in"}, + {"logged in without the role", RequireRoleAPI(RoleStyret)(okHandler()), + &User{ID: "fa-1", Roles: []string{"Medlem"}}, + http.StatusUnauthorized, "Permission denied"}, + {"anonymous at a role-gated endpoint", RequireRoleAPI(RoleStyret)(okHandler()), nil, + http.StatusUnauthorized, "You are not logged in"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/api/events", nil) + if tt.user != nil { + r = r.WithContext(WithUser(r.Context(), tt.user)) + } + rec := httptest.NewRecorder() + tt.handler.ServeHTTP(rec, r) + + if rec.Code != tt.status { + t.Errorf("status %d, want %d; API clients branch on this", rec.Code, tt.status) + } + if ct := rec.Header().Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { + t.Errorf("Content-Type %q; a client parsing this as JSON would fail", ct) + } + + var body struct { + Message string `json:"message"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("the error body %q is not valid JSON: %v", rec.Body.String(), err) + } + if body.Message != tt.message { + t.Errorf("message %q, want %q; clients key off this text", body.Message, tt.message) + } + }) + } +} + +// The error body has to survive any message, not only the two literals the +// package passes today. Hand-rolled quoting used to escape ", \ and \n and +// nothing else, so a tab, a carriage return or a NUL went out raw and the body +// stopped being parseable — the failure the next person to route a provider or +// database error through writeJSONError would have hit. The message must come +// back out of a decoder exactly as it went in. +func TestWriteJSONErrorSurvivesAwkwardMessages(t *testing.T) { + for name, msg := range map[string]string{ + "the literals the package actually sends": "You are not logged in", + "empty": "", + "a tab, a CR and a NUL": "a\tb\rc\x00d", + "a newline": "line\nline", + "a quote and a backslash": `he said "hi" \ then left`, + "non-ASCII": "æøå ÆØÅ 🎉", + "something that looks like JSON": `{"message":"nested"}`, + "HTML that must not break a client": ``, + } { + t.Run(name, func(t *testing.T) { + rec := httptest.NewRecorder() + writeJSONError(rec, http.StatusUnauthorized, msg) + + var body struct { + Message string `json:"message"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("the error body %q is not valid JSON: %v; a client would get a "+ + "parse failure instead of the reason it was refused", + rec.Body.String(), err) + } + if body.Message != msg { + t.Errorf("the message decoded as %q, want %q; the text a client keys off "+ + "was altered in transit", body.Message, msg) + } + }) + } +} + +// The exact bytes of the two messages the package sends are part of the API: +// the previous site's clients branch on this text, so a change here is a silent +// break for them. +func TestWriteJSONErrorBodyIsUnchangedForTheLiterals(t *testing.T) { + for _, msg := range []string{"You are not logged in", "Permission denied"} { + rec := httptest.NewRecorder() + writeJSONError(rec, http.StatusUnauthorized, msg) + + if want := `{"message":"` + msg + `"}`; rec.Body.String() != want { + t.Errorf("body %s, want %s", rec.Body.String(), want) + } + } +} + +// ── Context plumbing ────────────────────────────────────────────────────── + +func TestUserContextPlumbing(t *testing.T) { + t.Run("an empty context has no user", func(t *testing.T) { + if got := FromContext(context.Background()); got != nil { + t.Errorf("got %+v from a bare context", got) + } + }) + + t.Run("a request with no user is anonymous", func(t *testing.T) { + if got := FromRequest(httptest.NewRequest(http.MethodGet, "/", nil)); got != nil { + t.Errorf("got %+v from a plain request", got) + } + }) + + t.Run("the user survives a round trip", func(t *testing.T) { + want := &User{ID: "fa-1", Name: "Kari"} + if got := FromContext(WithUser(context.Background(), want)); got != want { + t.Errorf("got %v, want the same pointer back", got) + } + }) + + t.Run("the key is unexported and cannot collide", func(t *testing.T) { + // A context key of a named type declared in this package cannot be + // produced by another package, so nothing outside auth can plant a user. + // Planting an int 0 — the underlying value of userKey — must not work. + ctx := context.WithValue(context.Background(), 0, &User{ID: "attacker", Roles: []string{RoleStyret}}) //nolint:staticcheck + if got := FromContext(ctx); got != nil { + t.Errorf("a value stored under a bare int key was read back as %+v; anything "+ + "in the process could then forge a signed-in board member", got) + } + }) + + t.Run("a later WithUser wins", func(t *testing.T) { + first := &User{ID: "fa-1"} + second := &User{ID: "fa-2"} + ctx := WithUser(WithUser(context.Background(), first), second) + if got := FromContext(ctx); got != second { + t.Errorf("got %+v, want the most recently injected user", got) + } + }) +} + +// ── User predicates ─────────────────────────────────────────────────────── + +func TestHasRoleTable(t *testing.T) { + tests := []struct { + name string + user *User + role string + want bool + why string + }{ + {"nil user", nil, RoleStyret, false, + "callers rely on this so they do not need a separate nil check"}, + {"nil user, empty role", nil, "", false, "a nil user holds nothing at all"}, + {"no roles", &User{}, RoleStyret, false, "a member with no roles holds none"}, + {"empty role list", &User{Roles: []string{}}, RoleStyret, false, "same as nil"}, + {"holds it", &User{Roles: []string{RoleStyret}}, RoleStyret, true, "the ordinary case"}, + {"holds it last", &User{Roles: []string{"a", "b", RoleStyret}}, RoleStyret, true, + "the whole list must be searched"}, + {"asked for the empty role", &User{Roles: []string{"Medlem"}}, "", false, + "an empty role name must not match a real one"}, + {"holds the empty role", &User{Roles: []string{""}}, "", true, + "an empty entry matches an empty query; RequireRole(\"\") would therefore let " + + "such a member through, which is why no gate uses an empty role name"}, + {"unicode role", &User{Roles: []string{"Økonomi"}}, "Økonomi", true, + "Norwegian role names must work"}, + {"unicode role, different form", &User{Roles: []string{"Økonomi"}}, "Okonomi", false, + "comparison is byte-for-byte, not transliterating"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.user.HasRole(tt.role); got != tt.want { + t.Errorf("HasRole(%q) = %v, want %v — %s", tt.role, got, tt.want, tt.why) + } + }) + } +} + +func TestIsStyretIsHasRoleStyret(t *testing.T) { + for _, u := range []*User{ + nil, + {}, + {Roles: []string{RoleStyret}}, + {Roles: []string{"Medlem"}}, + {Roles: []string{"styret"}}, + } { + if u.IsStyret() != u.HasRole(RoleStyret) { + t.Errorf("IsStyret and HasRole(%q) disagree for %+v", RoleStyret, u) + } + } + if RoleStyret != "Styret" { + t.Errorf("RoleStyret is %q; it must match the role name configured in FusionAuth "+ + "exactly, or the board loses access to event administration", RoleStyret) + } +} + +// DisplayName falls back through the available fields so the interface never +// renders an empty name where a person should be. FusionAuth supplies "name" +// only when the lambda populates it, so the fallbacks are the normal path for +// some members, not an edge case. +func TestDisplayNameFallback(t *testing.T) { + tests := []struct { + name string + user *User + want string + }{ + {"nil user", nil, ""}, + {"name present", &User{Name: "Kari", FullName: "Kari Nordmann", + Email: "kari@example.no"}, "Kari"}, + {"no name, full name present", &User{FullName: "Kari Nordmann", + Email: "kari@example.no"}, "Kari Nordmann"}, + {"only an email", &User{Email: "kari@example.no"}, "kari@example.no"}, + {"nothing at all", &User{}, ""}, + {"only an ID", &User{ID: "fa-1"}, ""}, + {"unicode name", &User{Name: "Bjørn Ærlig Ødegård"}, "Bjørn Ærlig Ødegård"}, + {"a name that is only whitespace", &User{Name: " ", FullName: "Kari Nordmann"}, " "}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.user.DisplayName(); got != tt.want { + t.Errorf("DisplayName() = %q, want %q", got, tt.want) + } + }) + } +} diff --git a/internal/auth/oidc_flow_test.go b/internal/auth/oidc_flow_test.go new file mode 100644 index 0000000..58b8a70 --- /dev/null +++ b/internal/auth/oidc_flow_test.go @@ -0,0 +1,1157 @@ +package auth + +// End-to-end tests for the OpenID Connect login flow, driven against a fake +// provider on a local httptest server. Nothing here reaches FusionAuth or any +// other host: discovery, the token exchange and the JWKS endpoint are all +// served by newFakeIDP, so the suite runs offline and at full speed. + +import ( + "context" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync" + "testing" + "time" + + "github.com/coreos/go-oidc/v3/oidc" + + "github.com/ItemizeNTNU/website/internal/config" +) + +const ( + testClientID = "itemize-web" + testClientSecret = "client-secret-0123456789abcdefgh" + testBaseURL = "https://itemize.no" +) + +func nullLogger() *slog.Logger { return slog.New(slog.NewTextHandler(io.Discard, nil)) } + +// ── A fake identity provider ────────────────────────────────────────────── + +// fakeIDP serves just enough of FusionAuth for this package: the discovery +// document, an empty JWKS, and a token endpoint whose behaviour each test can +// bend. The authorization endpoint is never actually fetched — the browser step +// is simulated by reading the redirect and constructing the callback by hand. +type fakeIDP struct { + srv *httptest.Server + + // idToken is what the token endpoint returns as "id_token". Tests set it + // after starting the login, once the nonce is known. + idToken string + // omitIDToken makes the token response leave out id_token entirely, which + // is what a misconfigured application without the openid scope produces. + omitIDToken bool + // tokenStatus, when non-zero, replaces the whole token response with that + // status and an OAuth error body. + tokenStatus int + + mu sync.Mutex + tokenForm url.Values + tokenCalls int +} + +func newFakeIDP(t *testing.T) *fakeIDP { + t.Helper() + idp := &fakeIDP{} + + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": idp.srv.URL, + "authorization_endpoint": idp.srv.URL + "/oauth2/authorize", + "token_endpoint": idp.srv.URL + "/oauth2/token", + "jwks_uri": idp.srv.URL + "/.well-known/jwks.json", + "userinfo_endpoint": idp.srv.URL + "/oauth2/userinfo", + "id_token_signing_alg_values_supported": []string{"HS256", "RS256"}, + }) + }) + mux.HandleFunc("/.well-known/jwks.json", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + // Deliberately empty: under RS256 there is no key here that could + // verify an HS256 token, which is what the RS256 test relies on. + _, _ = w.Write([]byte(`{"keys":[]}`)) + }) + mux.HandleFunc("/oauth2/token", func(w http.ResponseWriter, r *http.Request) { + _ = r.ParseForm() + + idp.mu.Lock() + idp.tokenForm = r.PostForm + idp.tokenCalls++ + status := idp.tokenStatus + idp.mu.Unlock() + + w.Header().Set("Content-Type", "application/json") + if status != 0 { + w.WriteHeader(status) + _, _ = w.Write([]byte(`{"error":"invalid_grant"}`)) + return + } + + body := map[string]any{ + "access_token": "an-access-token", + "token_type": "Bearer", + "expires_in": 3600, + } + if !idp.omitIDToken { + body["id_token"] = idp.idToken + } + _ = json.NewEncoder(w).Encode(body) + }) + + idp.srv = httptest.NewServer(mux) + t.Cleanup(idp.srv.Close) + return idp +} + +func (f *fakeIDP) lastTokenRequest(t *testing.T) url.Values { + t.Helper() + f.mu.Lock() + defer f.mu.Unlock() + if f.tokenCalls == 0 { + t.Fatal("the token endpoint was never called, so no code was ever exchanged") + } + return f.tokenForm +} + +// claimsFor is a complete, valid claim set for the nonce a login just issued. +// Each test copies it and breaks exactly one thing. +func (f *fakeIDP) claimsFor(nonce string) map[string]any { + return map[string]any{ + "iss": f.srv.URL, + "aud": testClientID, + "sub": "11111111-2222-4333-8444-999999999999", + "exp": time.Now().Add(time.Hour).Unix(), + "iat": time.Now().Unix(), + "nonce": nonce, + "name": "Bjørn", + "fullName": "Bjørn Ærlig Ødegård", + "email": "bjorn@stud.ntnu.no", + "imageUrl": "https://auth.example/avatar.png", + "roles": []string{"Medlem", RoleStyret}, + } +} + +func signClaims(t *testing.T, secret string, claims map[string]any) string { + t.Helper() + payload, err := json.Marshal(claims) + if err != nil { + t.Fatal(err) + } + return mintJWT(t, secret, hs256Header("HS256"), string(payload)) +} + +// newAuthenticator wires an Authenticator to the fake provider, exercising New +// — including its discovery call and its choice of verifier — rather than +// building the struct by hand. +func newAuthenticator(t *testing.T, idp *fakeIDP, alg, idTokenHMACSecret string) (*Authenticator, *Sealer) { + t.Helper() + + host, err := url.Parse(idp.srv.URL) + if err != nil { + t.Fatal(err) + } + base, err := url.Parse(testBaseURL) + if err != nil { + t.Fatal(err) + } + + sealer := newTestSealer(t, testSecret, true) + a, err := New(context.Background(), &config.Config{ + BaseURL: base, + FusionAuth: config.FusionAuth{ + Host: host, + ClientID: testClientID, + ClientSecret: testClientSecret, + IDTokenAlg: alg, + IDTokenHMACSecret: idTokenHMACSecret, + }, + }, sealer, nullLogger()) + if err != nil { + t.Fatalf("building the authenticator against the fake provider: %v", err) + } + return a, sealer +} + +// startLogin runs Login and returns where the visitor was sent and the flow +// cookie their browser would now hold. +func startLogin(t *testing.T, a *Authenticator, returnTo string) (*url.URL, *http.Cookie) { + t.Helper() + + target := "/login" + if returnTo != "" { + target += "?return_to=" + url.QueryEscape(returnTo) + } + rec := httptest.NewRecorder() + a.Login(rec, httptest.NewRequest(http.MethodGet, target, nil)) + + if rec.Code != http.StatusFound { + t.Fatalf("Login returned %d rather than a redirect to the provider: %s", + rec.Code, rec.Body.String()) + } + authURL, err := url.Parse(rec.Header().Get("Location")) + if err != nil { + t.Fatalf("the authorization URL is not a URL: %v", err) + } + + for _, c := range rec.Result().Cookies() { + if c.Name == flowCookie { + return authURL, c + } + } + t.Fatal("Login set no flow cookie, so the callback could never be verified") + return nil, nil +} + +// callback runs the callback the way the browser would: with the flow cookie +// attached and the provider's parameters in the query string. +func callback(t *testing.T, a *Authenticator, flow *http.Cookie, query url.Values) *httptest.ResponseRecorder { + t.Helper() + r := httptest.NewRequest(http.MethodGet, "/callback?"+query.Encode(), nil) + if flow != nil { + r.AddCookie(flow) + } + rec := httptest.NewRecorder() + a.Callback(rec, r) + return rec +} + +// sessionFrom reads back whatever session the response established, or nil. +func sessionFrom(t *testing.T, sealer *Sealer, rec *httptest.ResponseRecorder) *Session { + t.Helper() + r := httptest.NewRequest(http.MethodGet, "/", nil) + for _, c := range rec.Result().Cookies() { + if c.Name == SessionCookie && c.Value != "" { + r.AddCookie(c) + } + } + return sealer.Read(r) +} + +// ── Login ───────────────────────────────────────────────────────────────── + +// The authorization request has to carry everything the provider needs, and +// three of these parameters are security-critical: state is the CSRF defence +// for the callback, nonce binds the ID token to this particular login, and the +// PKCE challenge stops an intercepted code from being redeemed by anyone else. +func TestLoginBuildsTheAuthorizationRequest(t *testing.T) { + idp := newFakeIDP(t) + a, sealer := newAuthenticator(t, idp, "HS256", "") + + authURL, flowCookie := startLogin(t, a, "/profil") + q := authURL.Query() + + if got := authURL.Scheme + "://" + authURL.Host + authURL.Path; got != idp.srv.URL+"/oauth2/authorize" { + t.Errorf("the visitor was sent to %q rather than the provider's authorization "+ + "endpoint", got) + } + if got := q.Get("client_id"); got != testClientID { + t.Errorf("client_id = %q, want %q", got, testClientID) + } + if got := q.Get("redirect_uri"); got != testBaseURL+"/callback" { + t.Errorf("redirect_uri = %q; it must match the callback route exactly or the "+ + "provider refuses the request", got) + } + if got := q.Get("response_type"); got != "code" { + t.Errorf("response_type = %q, want code; anything else is the implicit flow", got) + } + for _, scope := range []string{oidc.ScopeOpenID, "profile", "email"} { + if !strings.Contains(q.Get("scope"), scope) { + t.Errorf("scope %q is missing %q; without it the ID token arrives without the "+ + "claims the site renders", q.Get("scope"), scope) + } + } + if q.Get("state") == "" { + t.Error("no state parameter, so the callback has nothing to check against and " + + "becomes forgeable") + } + if q.Get("nonce") == "" { + t.Error("no nonce parameter, so an ID token from an unrelated login could be replayed") + } + if q.Get("code_challenge") == "" || q.Get("code_challenge_method") != "S256" { + t.Errorf("PKCE challenge is %q with method %q; without S256 an intercepted "+ + "authorization code can be redeemed by whoever intercepted it", + q.Get("code_challenge"), q.Get("code_challenge_method")) + } + + // The cookie is the only place state, nonce and verifier are kept — the + // flow is stateless by design — so it must agree with the URL. + var flow flowState + if err := sealer.Open(flowCookie.Value, &flow); err != nil { + t.Fatalf("the flow cookie could not be opened: %v", err) + } + if flow.State != q.Get("state") { + t.Error("the state in the cookie does not match the state sent to the provider, so " + + "every callback would be rejected") + } + if flow.Nonce != q.Get("nonce") { + t.Error("the nonce in the cookie does not match the one sent to the provider") + } + if flow.ReturnTo != "/profil" { + t.Errorf("ReturnTo = %q, want /profil", flow.ReturnTo) + } + if flow.Expires.IsZero() { + t.Error("the flow has no expiry, so an abandoned login attempt stays usable forever") + } +} + +// The flow cookie carries the state, nonce and PKCE verifier. It must be +// unreadable by script and short-lived: a leaked verifier plus an intercepted +// code is a complete account takeover. +func TestLoginFlowCookieAttributes(t *testing.T) { + idp := newFakeIDP(t) + a, _ := newAuthenticator(t, idp, "HS256", "") + + _, c := startLogin(t, a, "") + + if !c.HttpOnly { + t.Error("the flow cookie is not HttpOnly; script on the page could read the PKCE " + + "verifier and the state parameter") + } + if !c.Secure { + t.Error("the flow cookie is not Secure on a TLS deployment") + } + if c.SameSite != http.SameSiteLaxMode { + t.Errorf("SameSite = %v; Strict would drop the cookie on the redirect back from the "+ + "provider and no login could ever complete", c.SameSite) + } + if c.Path != "/" { + t.Errorf("Path = %q, want \"/\"", c.Path) + } + if c.MaxAge != int(flowTTL.Seconds()) { + t.Errorf("Max-Age = %d, want %d — a login attempt must not outlive its window", + c.MaxAge, int(flowTTL.Seconds())) + } +} + +// Every login attempt gets its own state, nonce and verifier. A value reused +// across logins would make the state parameter useless as a CSRF token. +func TestLoginIssuesFreshSecretsEveryTime(t *testing.T) { + idp := newFakeIDP(t) + a, sealer := newAuthenticator(t, idp, "HS256", "") + + states := map[string]bool{} + nonces := map[string]bool{} + verifiers := map[string]bool{} + + for i := 0; i < 20; i++ { + _, c := startLogin(t, a, "") + var flow flowState + if err := sealer.Open(c.Value, &flow); err != nil { + t.Fatal(err) + } + if states[flow.State] || nonces[flow.Nonce] || verifiers[flow.Verifier] { + t.Fatal("a login attempt reused the state, nonce or PKCE verifier of an earlier " + + "one; a predictable state parameter defeats the callback's CSRF check") + } + states[flow.State], nonces[flow.Nonce], verifiers[flow.Verifier] = true, true, true + + if len(flow.State) < 40 || len(flow.Nonce) < 40 { + t.Fatalf("state %q or nonce %q is short enough to guess", flow.State, flow.Nonce) + } + } +} + +// return_to is attacker-controlled — it is a query parameter on a URL anyone +// can send a member — so it is sanitised before it is sealed, not after it is +// read back. +func TestLoginSanitisesReturnToBeforeSealingIt(t *testing.T) { + idp := newFakeIDP(t) + a, sealer := newAuthenticator(t, idp, "HS256", "") + + tests := map[string]string{ + "/profil": "/profil", + "/arrangementer?old=1": "/arrangementer?old=1", + "": "/", + "//evil.example": "/", + "///evil.example": "/", + "https://evil.example/phish": "/", + "http://evil.example": "/", + "javascript:alert(1)": "/", + "//evil.example/%2e%2e": "/", + `\\evil.example`: "/", + "/\\evil.example": "/%5Cevil.example", + "data:text/html,