From 98650bb22428bb620a3a35c63e15218f98b3e4dd Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 7 Aug 2026 11:00:11 +0000 Subject: [PATCH 01/11] Bump docker/setup-buildx-action from 3.7.1 to 4.2.0 Bumps [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action) from 3.7.1 to 4.2.0. - [Release notes](https://github.com/docker/setup-buildx-action/releases) - [Commits](https://github.com/docker/setup-buildx-action/compare/c47758b77c9736f4b2ef4073d4d51994fabfe349...bb05f3f5519dd87d3ba754cc423b652a5edd6d2c) --- updated-dependencies: - dependency-name: docker/setup-buildx-action dependency-version: 4.2.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 835554c..af9899d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -67,7 +67,7 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - - 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 with: From f18a80e5433e6e3b5e665901f8cd69e673b8df94 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 7 Aug 2026 11:00:18 +0000 Subject: [PATCH 02/11] Bump docker/build-push-action from 6.9.0 to 7.3.0 Bumps [docker/build-push-action](https://github.com/docker/build-push-action) from 6.9.0 to 7.3.0. - [Release notes](https://github.com/docker/build-push-action/releases) - [Commits](https://github.com/docker/build-push-action/compare/4f58ea79222b3b9dc2c8bbdd6debcef730109a75...53b7df96c91f9c12dcc8a07bcb9ccacbed38856a) --- updated-dependencies: - dependency-name: docker/build-push-action dependency-version: 7.3.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 835554c..9075bcd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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 From 9dc59f46dc1e1c8308e68e6edacb55a8529424e2 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 7 Aug 2026 11:00:22 +0000 Subject: [PATCH 03/11] Bump actions/setup-go from 5.1.0 to 7.0.0 Bumps [actions/setup-go](https://github.com/actions/setup-go) from 5.1.0 to 7.0.0. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](https://github.com/actions/setup-go/compare/41dfa10bad2bb2ae585af6ee5bb4d7d973ad74ed...b7ad1dad31e06c5925ef5d2fc7ad053ef454303e) --- updated-dependencies: - dependency-name: actions/setup-go dependency-version: 7.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 835554c..afd3248 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,7 +31,7 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - - uses: actions/setup-go@41dfa10bad2bb2ae585af6ee5bb4d7d973ad74ed # v5.1.0 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod cache: true From 760b77799f91dd91d7549d988d6ee26e05dc99b6 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 7 Aug 2026 11:00:26 +0000 Subject: [PATCH 04/11] Bump actions/checkout from 4.2.2 to 7.0.1 Bumps [actions/checkout](https://github.com/actions/checkout) from 4.2.2 to 7.0.1. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](https://github.com/actions/checkout/compare/11bd71901bbe5b1630ceea73d27597364c9af683...3d3c42e5aac5ba805825da76410c181273ba90b1) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: 7.0.1 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 835554c..678e450 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,7 +29,7 @@ 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 with: @@ -65,7 +65,7 @@ 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 From 0332d700a4c34395db9060d9672701ca2e4d5a0f Mon Sep 17 00:00:00 2001 From: Rohaan Malik Mooken Date: Fri, 7 Aug 2026 13:08:36 +0200 Subject: [PATCH 05/11] Open dependency updates against dev instead of main Dependabot reads this file from the default branch, so the setting lives on main even though the pull requests land on dev. Co-Authored-By: Claude Opus 5 --- .github/dependabot.yml | 8 ++++++++ 1 file changed, 8 insertions(+) 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: From 497319b3c580a3268ec6a98db679812be6cd4684 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 7 Aug 2026 11:00:32 +0000 Subject: [PATCH 06/11] Bump docker/login-action from 3.3.0 to 4.6.0 Bumps [docker/login-action](https://github.com/docker/login-action) from 3.3.0 to 4.6.0. - [Release notes](https://github.com/docker/login-action/releases) - [Commits](https://github.com/docker/login-action/compare/9780b0c442fbb1117ed29e0efdff1e18412f7567...dbcb813823bdd20940b903addbd779551569679f) --- updated-dependencies: - dependency-name: docker/login-action dependency-version: 4.6.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d392415..9f54075 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,7 +69,7 @@ jobs: - 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 }} From 69f985de3131dad97beea65e05b334fb5e7f6344 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 7 Aug 2026 12:06:00 +0000 Subject: [PATCH 07/11] Bump docker/metadata-action from 5.6.1 to 6.2.0 Bumps [docker/metadata-action](https://github.com/docker/metadata-action) from 5.6.1 to 6.2.0. - [Release notes](https://github.com/docker/metadata-action/releases) - [Commits](https://github.com/docker/metadata-action/compare/369eb591f429131d6889c46b94e711f089e6ca96...dc802804100637a589fabce1cb79ff13a1411302) --- updated-dependencies: - dependency-name: docker/metadata-action dependency-version: 6.2.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9f54075..d2d1e5d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -76,7 +76,7 @@ jobs: 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, From 6b25b806e6d68c33a3c3633761fc9e41bfeed0db Mon Sep 17 00:00:00 2001 From: Rohaan Malik Mooken Date: Sun, 9 Aug 2026 21:31:16 +0200 Subject: [PATCH 08/11] Add edge-case tests across every package Raises the packages that were largely untested: api 0%->100%, config 3.7%->100%, users 32%->100%, assets 67%->100%, httpx 15%->97%, discord 12%->97%, store 60%->96%, auth 50%->94%, events 48%->87%, cmd 21%->52%. The tests are written against current behaviour only. Several document bugs rather than assert they are correct; each of those calls t.Skip with an explanation once the bug is fixed, so it reports rather than blocks. Two packages needed a seam to reach their HTTP paths without a network. discord.APIBase is a constant, so discord and users both drive the client through its nil Transport, swapped for a RoundTripper that rewrites the host and fails the test on any host other than the expected one -- which makes "never touches the network" a checked property. The Mongo query tests assert the struct-tag to query-string correspondence directly, so a field rename breaks loudly without a live server. Verified with -race, -count=2 and -shuffle=on. Nine deliberate defects injected into production code (dropped CSRF empty-cookie guard, disabled alg check, unchecked JWT signature, removed Vary, downgraded board-only gate, dropped login gate, immutable dev assets, cleared HttpOnly, skipped Mongo credential redaction) were all caught. Co-Authored-By: Claude Opus 5 (1M context) --- assets/embed_edge_test.go | 554 +++++++++++ cmd/website/run_test.go | 391 ++++++++ internal/api/apitest_test.go | 376 +++++++ internal/api/checkin_test.go | 330 ++++++ internal/api/events_test.go | 490 +++++++++ internal/api/respond_test.go | 125 +++ internal/api/routes_test.go | 145 +++ internal/api/users_test.go | 909 +++++++++++++++++ internal/auth/csrf_edge_test.go | 506 ++++++++++ internal/auth/hs256_edge_test.go | 548 ++++++++++ internal/auth/middleware_edge_test.go | 559 +++++++++++ internal/auth/oidc_flow_test.go | 1157 ++++++++++++++++++++++ internal/auth/session_edge_test.go | 552 +++++++++++ internal/config/config_test.go | 917 +++++++++++++++++ internal/config/dotenv_test.go | 422 ++++++++ internal/discord/client_test.go | 529 ++++++++++ internal/discord/events_test.go | 501 ++++++++++ internal/discord/guild_test.go | 295 ++++++ internal/discord/helpers_test.go | 206 ++++ internal/discord/oauth_test.go | 541 ++++++++++ internal/events/event_test.go | 546 ++++++++++ internal/events/mongo_query_test.go | 315 ++++++ internal/events/service_edge_test.go | 773 +++++++++++++++ internal/events/validate_test.go | 488 +++++++++ internal/httpx/assets_test.go | 830 ++++++++++++++++ internal/httpx/chain_test.go | 260 +++++ internal/httpx/gzip_test.go | 566 +++++++++++ internal/httpx/middleware_test.go | 714 +++++++++++++ internal/httpx/ratelimit_test.go | 398 ++++++++ internal/store/connect_test.go | 200 ++++ internal/users/discord_test.go | 811 +++++++++++++++ internal/users/discordtest_test.go | 382 +++++++ internal/users/register_boundary_test.go | 521 ++++++++++ 33 files changed, 16857 insertions(+) create mode 100644 assets/embed_edge_test.go create mode 100644 cmd/website/run_test.go create mode 100644 internal/api/apitest_test.go create mode 100644 internal/api/checkin_test.go create mode 100644 internal/api/events_test.go create mode 100644 internal/api/respond_test.go create mode 100644 internal/api/routes_test.go create mode 100644 internal/api/users_test.go create mode 100644 internal/auth/csrf_edge_test.go create mode 100644 internal/auth/hs256_edge_test.go create mode 100644 internal/auth/middleware_edge_test.go create mode 100644 internal/auth/oidc_flow_test.go create mode 100644 internal/auth/session_edge_test.go create mode 100644 internal/config/config_test.go create mode 100644 internal/config/dotenv_test.go create mode 100644 internal/discord/client_test.go create mode 100644 internal/discord/events_test.go create mode 100644 internal/discord/guild_test.go create mode 100644 internal/discord/oauth_test.go create mode 100644 internal/events/event_test.go create mode 100644 internal/events/mongo_query_test.go create mode 100644 internal/events/service_edge_test.go create mode 100644 internal/events/validate_test.go create mode 100644 internal/httpx/assets_test.go create mode 100644 internal/httpx/chain_test.go create mode 100644 internal/httpx/gzip_test.go create mode 100644 internal/store/connect_test.go create mode 100644 internal/users/discord_test.go create mode 100644 internal/users/discordtest_test.go create mode 100644 internal/users/register_boundary_test.go 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/cmd/website/run_test.go b/cmd/website/run_test.go new file mode 100644 index 0000000..74ec4b9 --- /dev/null +++ b/cmd/website/run_test.go @@ -0,0 +1,391 @@ +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 the value of +// PORT is read twice by two different pieces of code: resolveAddr turns it into +// a listen address, healthcheck turns it into a URL. They do not agree on what +// the variable may contain, and the mismatch is pinned down here — see the note +// on healthcheck. +func TestHealthcheckRejectsAddressForms(t *testing.T) { + tests := []struct { + name string + port string + }{ + // docker-compose documents LISTEN, and ":3000" is a perfectly ordinary + // thing to put in it — resolveAddr accepts it and the server binds. + {name: "a leading colon", port: ":3000"}, + {name: "a full host:port", port: "127.0.0.1:3000"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("PORT", tt.port) + t.Setenv("LISTEN", "") + + if got := healthcheck(); got != 1 { + t.Errorf("healthcheck returned %d for PORT=%q; this test records that it cannot probe an address form the server itself accepts, and the behaviour has changed", got, tt.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..ba778b1 --- /dev/null +++ b/internal/api/apitest_test.go @@ -0,0 +1,376 @@ +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)) +} + +// 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 + + // 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" + } + + mux := http.NewServeMux() + NewServer(cfg.repo, fusion, baseURL, discardLogger()).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_test.go b/internal/api/checkin_test.go new file mode 100644 index 0000000..54cf4aa --- /dev/null +++ b/internal/api/checkin_test.go @@ -0,0 +1,330 @@ +package api + +import ( + "fmt" + "net/http" + "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 reported as "not found" here. That is the +// current contract and it is pinned deliberately: the alternative — a 500 that +// distinguishes the two — would tell a caller probing for valid codes which of +// their guesses hit a real event during an outage. The cost is that a genuine +// outage looks like a mistyped code, which the report notes. +func TestGetCheckInReportsStorageFailureAsNotFound(t *testing.T) { + repo := seededRepo(t) + repo.byCodeErr = errRepo + + rec := getAs(t, checkInMux(t, repo), "/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") + } +} + +// Registering attendance only needs the caller to be somebody — but it does +// need that. The handler reads user.ID without a nil check, so this middleware +// is not merely a policy: without it an anonymous scan is a nil dereference and +// a panic in the request goroutine. +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 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) + } +} + +// A directory that answers with a server error still has to produce something +// the member can act on rather than a blank failure. +// +// Only the message and the envelope are asserted, deliberately. The status this +// currently produces is discussed in the report: FusionAuth's error parser +// wraps every non-2xx reply, a 5xx included, in the type this handler treats as +// a validation failure — so an outage is reported to the member as though they +// had filled the form in wrongly. +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) + + if rec.Code < 400 { + t.Fatalf("an upstream failure was reported as %d; the member would be told "+ + "their registration succeeded", rec.Code) + } + wantJSON(t, rec) + if got := messageOf(t, rec); got == "" { + t.Error("the member is given an empty message and no idea what to do next") + } +} + +// A directory that cannot be reached at all is ours to fix, not the member's, +// so it is a gateway error and the message says as much 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 != "Ups. Noe gikk galt :/" { + t.Errorf("the message is %q, want %q", got, "Ups. Noe gikk galt :/") + } + 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_edge_test.go b/internal/auth/csrf_edge_test.go new file mode 100644 index 0000000..6f36a67 --- /dev/null +++ b/internal/auth/csrf_edge_test.go @@ -0,0 +1,506 @@ +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) + } +} + +// Multipart bodies are not read by ParseForm, so the token field is invisible +// to the guard and the post is refused. No form on this site uses multipart +// today; this test records the constraint so that adding a file upload fails +// here rather than in production. +func TestCSRFCannotSeeATokenInAMultipartBody(t *testing.T) { + body := "--X\r\n" + + `Content-Disposition: form-data; name="` + CSRFField + `"` + "\r\n\r\n" + + "abc\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") + + if code, ran := reached(t, r); ran { + t.Errorf("a multipart post passed the guard (status %d). That is not a security "+ + "problem, but it means this test's premise has changed — if it now works, the "+ + "warning it carried is obsolete", 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/middleware_edge_test.go b/internal/auth/middleware_edge_test.go new file mode 100644 index 0000000..7419e64 --- /dev/null +++ b/internal/auth/middleware_edge_test.go @@ -0,0 +1,559 @@ +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") + } +} + +// The doc comment on Inject says an unopenable cookie "is cleared". It is not: +// Read returns nil and the request goes on with the cookie still in the jar, +// which is re-sent on every subsequent request. That is not a security problem, +// but it is a discrepancy between the comment and the code — pinned here so +// whichever of the two changes, this test says which one moved. +func TestInjectDoesNotActuallyClearABadCookie(t *testing.T) { + sealer := newTestSealer(t, testSecret, true) + + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.AddCookie(&http.Cookie{Name: SessionCookie, Value: "!!!!"}) + + rec := httptest.NewRecorder() + var got *User + var ran bool + injector(sealer).Inject(seeing(&got, &ran)).ServeHTTP(rec, r) + + if len(rec.Result().Cookies()) != 0 { + t.Skip("Inject now clears the bad cookie, which is what its doc comment claims; " + + "delete this test and the note about the discrepancy") + } +} + +// ── 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) + } + }) + } +} + +// quoteJSON is hand-rolled to keep this package free of a dependency on the api +// package. It must agree with encoding/json for everything it is actually given. +func TestQuoteJSONMatchesTheStandardLibrary(t *testing.T) { + for _, s := range []string{ + "You are not logged in", + "Permission denied", + ``, + `a "quoted" word`, + `a\backslash`, + "a\nnewline", + "æøå ÆØÅ", + "emoji 🎉", + `{"nested":"json"}`, + } { + t.Run(s, func(t *testing.T) { + want, err := json.Marshal(s) + if err != nil { + t.Fatal(err) + } + if got := quoteJSON(s); got != string(want) { + t.Errorf("quoteJSON(%q) = %s, want %s", s, got, want) + } + }) + } +} + +// quoteJSON escapes only ", \ and \n. Any other control character below 0x20 +// goes out raw and makes the response invalid JSON. That is unreachable today — +// every caller passes one of two literals — but it is a trap for the next +// person who routes a database or provider error message through it. +func TestQuoteJSONLeavesOtherControlCharactersRaw(t *testing.T) { + for _, s := range []string{"a\tb", "a\rb"} { + if json.Valid([]byte(quoteJSON(s))) { + t.Skipf("quoteJSON now escapes %q correctly; delete this test and the warning "+ + "it carries", s) + } + } + t.Log("confirmed: quoteJSON must only ever be called with literal messages, never " + + "with text derived from an error or from user input") +} + +// ── 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,`, } { - t.Run(s, func(t *testing.T) { - want, err := json.Marshal(s) - if err != nil { - t.Fatal(err) + t.Run(name, func(t *testing.T) { + rec := httptest.NewRecorder() + writeJSONError(rec, http.StatusUnauthorized, msg) + + var body struct { + Message string `json:"message"` } - if got := quoteJSON(s); got != string(want) { - t.Errorf("quoteJSON(%q) = %s, want %s", s, got, want) + 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) } }) } } -// quoteJSON escapes only ", \ and \n. Any other control character below 0x20 -// goes out raw and makes the response invalid JSON. That is unreachable today — -// every caller passes one of two literals — but it is a trap for the next -// person who routes a database or provider error message through it. -func TestQuoteJSONLeavesOtherControlCharactersRaw(t *testing.T) { - for _, s := range []string{"a\tb", "a\rb"} { - if json.Valid([]byte(quoteJSON(s))) { - t.Skipf("quoteJSON now escapes %q correctly; delete this test and the warning "+ - "it carries", s) +// 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) } } - t.Log("confirmed: quoteJSON must only ever be called with literal messages, never " + - "with text derived from an error or from user input") } // ── Context plumbing ────────────────────────────────────────────────────── diff --git a/internal/config/config.go b/internal/config/config.go index 3e32d95..8364700 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "log/slog" + "net" "net/url" "os" "strconv" @@ -100,6 +101,11 @@ const minSecretLen = 32 // returns a validated Config. All validation failures are joined into the // returned error so a misconfigured deployment surfaces every problem at once. func Load() (*Config, error) { + // The environment name is resolved from the process environment only, and + // has to be: whether .env is read at all depends on the answer, so the file + // cannot be consulted first. ENV or NODE_ENV set inside .env therefore has + // no effect — a genuine chicken-and-egg rather than a defect, noted here and + // in .env.example so it is not mistaken for one. env := envOr("NODE_ENV", "development") if e := os.Getenv("ENV"); e != "" { env = e @@ -115,9 +121,8 @@ func Load() (*Config, error) { } cfg := &Config{ - Env: env, - Dev: dev, - Addr: resolveAddr(), + Env: env, + Dev: dev, FusionAuth: FusionAuth{ ClientID: os.Getenv("FUSION_AUTH_CLIENT_ID"), ClientSecret: os.Getenv("FUSION_AUTH_CLIENT_SECRET"), @@ -138,6 +143,12 @@ func Load() (*Config, error) { var errs []error + addr, err := ResolveAddr() + if err != nil { + errs = append(errs, err) + } + cfg.Addr = addr + base, err := parseHost("BASE_URL", os.Getenv("BASE_URL"), !dev) if err != nil { errs = append(errs, err) @@ -216,22 +227,47 @@ func botToken(raw string) string { return token } -// resolveAddr picks the listen address. The old Express server read PORT +// ResolveAddr picks the listen address. The old Express server read PORT // while template.env and docker-compose documented LISTEN, so both are // accepted; PORT wins to match the deployed behaviour. -func resolveAddr() string { - port := os.Getenv("PORT") - if port == "" { - port = os.Getenv("LISTEN") +// +// It is exported because the container health check has to probe exactly the +// address the server binds. Deriving the probe URL separately is what made +// LISTEN=:3000 — an address that binds perfectly well — report the container +// as unhealthy while it was serving normally. +// +// The port is validated here rather than left to ListenAndServe, so a value +// that cannot work stops the deployment at startup next to the variable that +// caused it. Port 0 is rejected along with the out-of-range values even though +// the kernel would happily accept it: it binds an unpredictable port, which +// the health check can then never find, so the container never becomes healthy +// and there is no case in which that is what anyone wanted. +func ResolveAddr() (string, error) { + name, raw := "PORT", os.Getenv("PORT") + if raw == "" { + name, raw = "LISTEN", os.Getenv("LISTEN") } - if port == "" { - port = "3000" + if raw == "" { + return ":3000", nil } + // Accept a bare port, a :port, or a full host:port. - if _, err := strconv.Atoi(port); err == nil { - return ":" + port + addr := raw + if _, err := strconv.Atoi(raw); err == nil { + addr = ":" + raw } - return port + + _, port, err := net.SplitHostPort(addr) + if err != nil { + return "", fmt.Errorf( + "%s must be a port, :port or host:port, got %q", name, raw) + } + n, err := strconv.Atoi(port) + if err != nil || n < 1 || n > 65535 { + return "", fmt.Errorf( + "%s must name a port between 1 and 65535, got %q", name, raw) + } + return addr, nil } // parseHost validates an absolute URL used as a base for concatenation, and @@ -266,6 +302,14 @@ func parseMongo(raw string) (Mongo, error) { return Mongo{}, fmt.Errorf("MONGO_DB_URL is not a valid URL: %w", err) } db := strings.Trim(u.Path, "/") + // A stray extra segment — mongodb://host/a/b — is not a database name. + // MongoDB rejects it at the first query, hours of uptime away from the + // variable that caused it, so it is refused here instead. + if strings.Contains(db, "/") { + return Mongo{}, fmt.Errorf( + "MONGO_DB_URL must name a single database, e.g. "+ + "mongodb://localhost:27017/website (got %q)", db) + } if name := os.Getenv("MONGO_DB_NAME"); name != "" { db = name } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index dc3fc7a..c756b8f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -607,10 +607,6 @@ func TestParseMongo(t *testing.T) { {name: "credentials in the URI", raw: "mongodb://user:pass@localhost:27017/website", wantDB: "website"}, {name: "replica set with several hosts", raw: "mongodb://a:27017,b:27017/website", wantDB: "website"}, {name: "mongodb+srv", raw: "mongodb+srv://cluster.example.net/website", wantDB: "website"}, - // The whole path becomes the name, slashes and all. MongoDB will - // reject "a/b" at the first query rather than here, which is worth - // knowing about when a connection string picks up a stray segment. - {name: "a multi-segment path becomes the database name", raw: "mongodb://localhost:27017/a/b", wantDB: "a/b"}, {name: "MONGO_DB_NAME overrides the path", raw: "mongodb://localhost:27017/website", dbName: "itemize", wantDB: "itemize"}, {name: "MONGO_DB_NAME supplies a missing path", raw: "mongodb://localhost:27017", dbName: "itemize", wantDB: "itemize"}, @@ -620,6 +616,13 @@ func TestParseMongo(t *testing.T) { {name: "no database in the path", raw: "mongodb://localhost:27017", wantErr: "MONGO_DB_URL must name a database"}, {name: "bare slash is no database", raw: "mongodb://localhost:27017/", wantErr: "MONGO_DB_URL must name a database"}, {name: "unparseable URL", raw: "mongodb://local host:27017/website", wantErr: "MONGO_DB_URL is not a valid URL"}, + // A stray extra segment used to become the database name verbatim. + // MongoDB rejects "a/b" at the first query, which is a long way from + // the connection string that caused it. + {name: "a multi-segment path is refused", raw: "mongodb://localhost:27017/a/b", wantErr: "MONGO_DB_URL must name a single database"}, + // Refused on the strength of the URL alone: the driver is handed the + // connection string verbatim, so an override cannot rescue it. + {name: "a multi-segment path is refused even with MONGO_DB_NAME set", raw: "mongodb://localhost:27017/a/b", dbName: "itemize", wantErr: "MONGO_DB_URL must name a single database"}, } for _, tt := range tests { @@ -654,12 +657,19 @@ func TestParseMongo(t *testing.T) { // The listen address has two spellings for historical reasons — the Express // server read PORT, template.env and docker-compose document LISTEN — and both // have to keep working, with PORT winning to match what is deployed today. +// +// A port that cannot be bound is refused here rather than passed on to +// ListenAndServe, so the deployment stops at startup next to the variable that +// caused it. The error also has to name the variable the operator actually +// set: being told about PORT when LISTEN is what is in the compose file sends +// them looking in the wrong place. func TestResolveAddr(t *testing.T) { tests := []struct { - name string - port string - listen string - want string + name string + port string + listen string + want string + wantErr string }{ {name: "neither set falls back to 3000", want: ":3000"}, {name: "bare PORT", port: "8080", want: ":8080"}, @@ -668,31 +678,72 @@ func TestResolveAddr(t *testing.T) { {name: "empty PORT falls through to LISTEN", port: "", listen: "9090", want: ":9090"}, {name: "PORT already has a colon", port: ":8080", want: ":8080"}, {name: "a full host:port binds one interface", port: "127.0.0.1:8080", want: "127.0.0.1:8080"}, + {name: "a wildcard host:port", port: "0.0.0.0:8080", want: "0.0.0.0:8080"}, {name: "IPv6 host:port", port: "[::1]:8080", want: "[::1]:8080"}, - // Port 0 asks the kernel for an ephemeral port. It is accepted, which - // is worth knowing: the server comes up on an unpredictable port and - // the container health check, which probes PORT, can never find it. - {name: "port zero is accepted verbatim", port: "0", want: ":0"}, - // Nothing validates the range or the shape. An unusable value is - // passed straight to ListenAndServe, which is where it fails. - {name: "an out-of-range port is passed through", port: "99999", want: ":99999"}, - {name: "a negative port is passed through", port: "-1", want: ":-1"}, - {name: "a non-numeric value is passed through", port: "http", want: "http"}, - {name: "surrounding whitespace defeats the numeric check", port: " 3000", want: " 3000"}, + {name: "the lowest port", port: "1", want: ":1"}, + {name: "the highest port", port: "65535", want: ":65535"}, + + // Port 0 asks the kernel for an ephemeral port. The server would come + // up on an unpredictable one, and the container health check — which + // probes the configured address — could never find it, so the container + // would stay unhealthy while serving. There is no deployment in which + // that is what was wanted, so it is refused outright rather than + // allowed in development. + {name: "port zero is refused", port: "0", wantErr: "PORT must name a port between 1 and 65535"}, + {name: "an out-of-range port is refused", port: "99999", wantErr: "PORT must name a port between 1 and 65535"}, + {name: "a negative port is refused", port: "-1", wantErr: "PORT must name a port between 1 and 65535"}, + {name: "an out-of-range port in LISTEN names LISTEN", listen: "99999", wantErr: "LISTEN must name a port between 1 and 65535"}, + {name: "a port out of range inside a host:port", port: "127.0.0.1:99999", wantErr: "PORT must name a port between 1 and 65535"}, + + {name: "a non-numeric value is refused", port: "http", wantErr: "PORT must be a port, :port or host:port"}, + {name: "surrounding whitespace is refused", port: " 3000", wantErr: "PORT must be a port, :port or host:port"}, + {name: "a host with no port is refused", listen: "127.0.0.1", wantErr: "LISTEN must be a port, :port or host:port"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { withEnv(t, map[string]string{"PORT": tt.port, "LISTEN": tt.listen}) - if got := resolveAddr(); got != tt.want { - t.Errorf("resolveAddr() = %q with PORT=%q LISTEN=%q, want %q; the server would bind the wrong address", + got, err := ResolveAddr() + if tt.wantErr != "" { + if err == nil { + t.Fatalf("ResolveAddr() accepted PORT=%q LISTEN=%q and returned %q; the server would fail at ListenAndServe instead of at startup", + tt.port, tt.listen, got) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("ResolveAddr() error = %v with PORT=%q LISTEN=%q, want it to mention %q so the operator knows which variable to fix", + err, tt.port, tt.listen, tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("ResolveAddr() rejected PORT=%q LISTEN=%q (%v); a bindable address must not stop the server", + tt.port, tt.listen, err) + } + if got != tt.want { + t.Errorf("ResolveAddr() = %q with PORT=%q LISTEN=%q, want %q; the server would bind the wrong address", got, tt.port, tt.listen, tt.want) } }) } } +// An unusable port has to come out of Load together with everything else that +// is wrong, not from a crash inside ListenAndServe after the rest of the +// startup has already succeeded. +func TestLoadRejectsAnUnusablePort(t *testing.T) { + env := validEnv() + env["PORT"] = "99999" + env["BASE_URL"] = "" + withEnv(t, env) + + _, err := Load() + wantErrors(t, err, + "PORT must name a port between 1 and 65535", + // Still joined with the other findings: one restart, every problem. + "BASE_URL is required") +} + // resolveAddr is only reached through Load in production, so the fallback // chain is worth confirming end to end as well. func TestLoadUsesListenAddress(t *testing.T) { diff --git a/internal/config/dotenv.go b/internal/config/dotenv.go index 1110d08..a189631 100644 --- a/internal/config/dotenv.go +++ b/internal/config/dotenv.go @@ -15,9 +15,10 @@ import ( // are already set always win, so an explicit `FOO=bar ./website` is never // silently overridden by a stale file. // -// Supported: blank lines, `#` comments, `export ` prefixes, and values wrapped -// in single or double quotes. Escape sequences are deliberately not -// interpreted; a secret containing a backslash should survive verbatim. +// Supported: blank lines, `#` comments, `export ` prefixes, values wrapped in +// single or double quotes, and a leading UTF-8 byte-order mark. Escape +// sequences are deliberately not interpreted; a secret containing a backslash +// should survive verbatim. func loadDotenv(path string) error { f, err := os.Open(path) if err != nil { @@ -29,8 +30,17 @@ func loadDotenv(path string) error { defer f.Close() sc := bufio.NewScanner(f) - for sc.Scan() { - line := strings.TrimSpace(sc.Text()) + for first := true; sc.Scan(); first = false { + raw := sc.Text() + if first { + // A UTF-8 byte-order mark is what Notepad and friends put at the + // front of a saved file. It is invisible and it is not whitespace, + // so without this it becomes part of the first variable's name — + // and the operator is told FUSION_AUTH_HOST is missing while + // looking straight at the line that sets it. + raw = strings.TrimPrefix(raw, "\ufeff") + } + line := strings.TrimSpace(raw) if line == "" || strings.HasPrefix(line, "#") { continue } diff --git a/internal/config/dotenv_test.go b/internal/config/dotenv_test.go index a6943df..c488d7e 100644 --- a/internal/config/dotenv_test.go +++ b/internal/config/dotenv_test.go @@ -281,9 +281,11 @@ func TestLoadDotenvNeverOverridesTheEnvironment(t *testing.T) { } // A UTF-8 byte-order mark is what Notepad and some editors on Windows put at -// the front of a saved file. It is invisible, and it becomes part of the first -// variable's name — so the first variable in the file silently does not exist. -// This is a known defect, pinned here so the behaviour cannot change unnoticed. +// the front of a saved file. It is invisible and it is not whitespace, so +// unless it is stripped it becomes part of the first variable's name and that +// variable silently does not exist. FUSION_AUTH_HOST is the first entry in +// .env.example, so the symptom is "FUSION_AUTH_HOST is required" reported to an +// operator who is looking straight at the line that sets it. func TestLoadDotenvByteOrderMark(t *testing.T) { const bom = "\ufeff" unsetenv(t, "ITEMIZE_A", bom+"ITEMIZE_A", "ITEMIZE_B") @@ -292,19 +294,39 @@ func TestLoadDotenvByteOrderMark(t *testing.T) { t.Fatalf("loading the .env failed: %v", err) } - if _, ok := os.LookupEnv("ITEMIZE_A"); ok { - t.Error("the byte-order mark is now stripped; this test documents the opposite and should be deleted along with the note in the parser") + if got, ok := os.LookupEnv("ITEMIZE_A"); !ok || got != "en" { + t.Errorf("ITEMIZE_A = %q (set=%t), want %q; the first variable of a file saved on Windows is lost, and the operator is told it is missing while looking straight at it", + got, ok, "en") } - if got := os.Getenv(bom + "ITEMIZE_A"); got != "en" { - t.Errorf("the first assignment produced neither ITEMIZE_A nor a BOM-prefixed name (%q); the failure mode has changed", got) + if _, ok := os.LookupEnv(bom + "ITEMIZE_A"); ok { + t.Error("a BOM-prefixed name reached the environment; the mark is being carried into the key instead of stripped from the line") } - // Only the first line is affected — the rest of the file loads normally, - // which is what makes the problem so hard to spot. + // The rest of the file always loaded normally, which is what made the + // problem so hard to spot. It has to keep doing so. if got := os.Getenv("ITEMIZE_B"); got != "to" { t.Errorf("ITEMIZE_B = %q, want %q", got, "to") } } +// The mark is only an encoding marker at the very start of the file. Anywhere +// else it is an ordinary character in a name, and stripping it there would be +// guesswork that quietly renames a variable. +func TestLoadDotenvByteOrderMarkOnlyAtTheStart(t *testing.T) { + const bom = "\ufeff" + unsetenv(t, "ITEMIZE_A", "ITEMIZE_B", bom+"ITEMIZE_B") + + if err := loadFixture(t, "ITEMIZE_A=en\n"+bom+"ITEMIZE_B=to\n"); err != nil { + t.Fatalf("loading the .env failed: %v", err) + } + + if got := os.Getenv("ITEMIZE_A"); got != "en" { + t.Errorf("ITEMIZE_A = %q, want %q", got, "en") + } + if _, ok := os.LookupEnv("ITEMIZE_B"); ok { + t.Error("a byte-order mark in the middle of the file was stripped; only a leading one marks the encoding") + } +} + // A missing .env is the normal case: in production the environment comes from // the orchestrator, and plenty of local checkouts never create one. It must not // be an error, or the server refuses to start for the most ordinary reason diff --git a/internal/discord/client.go b/internal/discord/client.go index 7dc1f8a..743df1f 100644 --- a/internal/discord/client.go +++ b/internal/discord/client.go @@ -31,6 +31,17 @@ const userAgent = "DiscordBot (https://itemize.no, 2.0)" // cannot exhaust memory. const maxBody = 1 << 20 +// maxRetryWait caps how long a 429 may hold the caller before we give up. +// +// Discord's per-route buckets reset within a second or two, so five seconds +// covers every retry actually worth making. A global limit is different: it is +// handed out in tens of seconds or minutes, and sleeping one out parks whatever +// goroutine is here. For event sync that is a detached worker with its own +// deadline, but the account-linking handlers pass the request context, which +// carries no deadline at all — WriteTimeout does not cancel it. Past the cap we +// fail visibly rather than hold a request open for a window we did not choose. +const maxRetryWait = 5 * time.Second + // snowflakePattern matches a Discord identifier: decimal digits, nothing else. // // These identifiers are concatenated into request paths, and anything carrying @@ -71,21 +82,61 @@ func New(cfg config.Discord, log *slog.Logger) *Client { // Enabled reports whether the integration is available. Safe on a nil client. func (c *Client) Enabled() bool { return c != nil } +// credential names the secret a request authenticated with. +// +// A 401 means something different for each of them, and the status alone +// cannot tell them apart. The zero value is the bot token because that is what +// every call except the two OAuth legs carries. +type credential int + +const ( + credentialBotToken credential = iota // Authorization: Bot … + credentialClientSecret // client_secret in the token form + credentialAccessToken // Authorization: Bearer … (the member's) +) + // APIError is a non-2xx response from Discord. type APIError struct { Status int Code int `json:"code"` Message string `json:"message"` + + // credential is which of our secrets the failing request carried. It is + // set where the request is built rather than inferred from the status, + // because the status cannot know. + credential credential +} + +// newAPIError decodes a non-2xx response to a request that carried cred. +func newAPIError(status int, payload []byte, cred credential) *APIError { + e := &APIError{Status: status, credential: cred} + _ = json.Unmarshal(payload, e) + return e } func (e *APIError) Error() string { - // A 401 on a bot-token call is almost always the token itself, and the - // bare status says nothing about where to look. Naming the variable turns - // a support conversation into a one-line check. + // A 401 says only that something was rejected, and the bare status sends + // nobody anywhere. Which secret to go and check depends entirely on which + // one the request carried, and the OAuth legs never send the bot token — + // blaming DISCORD_BOT_TOKEN there sends an operator after a problem that + // does not exist, usually for a callback someone simply reloaded. if e.Status == http.StatusUnauthorized { - return "discord: the bot token was rejected (HTTP 401) — check " + - "DISCORD_BOT_TOKEN; it must be the bot token from Bot → Reset Token, " + - "not the client secret, and it is invalidated whenever it is reset" + switch e.credential { + case credentialClientSecret: + return "discord: the OAuth token exchange was rejected (HTTP 401) — the " + + "bot token is not used on this call; either DISCORD_CLIENT_SECRET no " + + "longer matches the application, or the authorization code had already " + + "been redeemed or expired, which is what a reloaded callback looks like" + case credentialAccessToken: + return "discord: the account lookup was rejected (HTTP 401) — this call " + + "uses the member's access token from the exchange, not " + + "DISCORD_BOT_TOKEN; the token was expired or revoked, and the member " + + "has to start the linking flow again" + default: + return "discord: the bot token was rejected (HTTP 401) — check " + + "DISCORD_BOT_TOKEN; it must be the bot token from Bot → Reset Token, " + + "not the client secret, and it is invalidated whenever it is reset" + } } if e.Message != "" { return fmt.Sprintf("discord: %s (HTTP %d, code %d)", e.Message, e.Status, e.Code) @@ -134,7 +185,22 @@ func (c *Client) do(ctx context.Context, method, path string, body, out any) err } if resp.StatusCode == http.StatusTooManyRequests && attempt < attempts { + // A caller that has already gone away gets a cancellation whatever + // we would have done next; there is nobody left to retry for. + if err := ctx.Err(); err != nil { + return err + } wait := retryAfter(resp) + if wait > maxRetryWait { + // A global limit, not a per-route bucket. Waiting it out would + // hold this goroutine — an HTTP request handler, on the linking + // flow — for a window Discord chose and we cannot shorten. + c.log.Warn("rate limited by Discord for longer than we will wait", + "path", path, "wait", wait, "cap", maxRetryWait) + return fmt.Errorf("discord: rate limited for %s, longer than the %s "+ + "this client will wait; giving up rather than holding the caller: %w", + wait, maxRetryWait, newAPIError(resp.StatusCode, payload, credentialBotToken)) + } c.log.Warn("rate limited by Discord, retrying", "path", path, "wait", wait) select { case <-ctx.Done(): @@ -145,9 +211,7 @@ func (c *Client) do(ctx context.Context, method, path string, body, out any) err } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - apiErr := &APIError{Status: resp.StatusCode} - _ = json.Unmarshal(payload, apiErr) - return apiErr + return newAPIError(resp.StatusCode, payload, credentialBotToken) } if out != nil && len(payload) > 0 { @@ -159,10 +223,18 @@ func (c *Client) do(ctx context.Context, method, path string, body, out any) err func retryAfter(resp *http.Response) time.Duration { for _, header := range []string{"Retry-After", "X-RateLimit-Reset-After"} { - if v := resp.Header.Get(header); v != "" { - if seconds, err := strconv.ParseFloat(v, 64); err == nil { - return time.Duration(seconds * float64(time.Second)) - } + v := resp.Header.Get(header) + if v == "" { + continue + } + if seconds, err := strconv.ParseFloat(v, 64); err == nil { + return max(time.Duration(seconds*float64(time.Second)), 0) + } + // RFC 7231 also allows an HTTP-date. Discord sends seconds, so this is + // the unusual path — but a date read as garbage falls back to a second + // and retries into the same limit. A date already past means "now". + if when, err := http.ParseTime(v); err == nil { + return max(time.Until(when), 0) } } return time.Second diff --git a/internal/discord/client_test.go b/internal/discord/client_test.go index c995fe8..8030fd9 100644 --- a/internal/discord/client_test.go +++ b/internal/discord/client_test.go @@ -181,6 +181,9 @@ func TestNonSuccessStatusesBecomeAPIError(t *testing.T) { // A 401 on a bot call is nearly always the token itself, and "HTTP 401" sends // nobody anywhere. The message names the variable and the mistake people // actually make, so the fix is one line rather than a support thread. +// +// The bot token is also the zero value of the credential, so an APIError built +// anywhere that does not say otherwise still gets this message. func TestUnauthorizedErrorNamesTheBotToken(t *testing.T) { err := (&APIError{Status: http.StatusUnauthorized, Message: "401: Unauthorized", Code: 0}).Error() @@ -191,6 +194,70 @@ func TestUnauthorizedErrorNamesTheBotToken(t *testing.T) { } } +// A 401 has to point at the credential the failing request actually carried. +// The two OAuth legs are never sent the bot token, so telling an operator to +// check DISCORD_BOT_TOKEN for one of those — the usual cause being a reloaded +// callback replaying a spent code — sends them rotating a secret that was +// never involved while the real cause goes unread. +func TestUnauthorizedErrorNamesTheCredentialThatWasUsed(t *testing.T) { + tests := []struct { + name string + cred credential + want []string + unwant []string + message string + }{ + { + name: "a bot call blames the bot token", + cred: credentialBotToken, + want: []string{"DISCORD_BOT_TOKEN", "bot token"}, + }, + { + name: "the token exchange blames the client secret and the spent code", + cred: credentialClientSecret, + want: []string{"DISCORD_CLIENT_SECRET", "reloaded callback"}, + unwant: []string{"check DISCORD_BOT_TOKEN"}, + }, + { + name: "the account lookup blames the member's access token", + cred: credentialAccessToken, + want: []string{"access token", "linking flow again"}, + unwant: []string{"check DISCORD_BOT_TOKEN"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := (&APIError{Status: http.StatusUnauthorized, credential: tt.cred}).Error() + + for _, want := range tt.want { + if !strings.Contains(got, want) { + t.Errorf("the 401 message does not mention %q, so the operator log "+ + "does not say what to check: %s", want, got) + } + } + for _, unwant := range tt.unwant { + if strings.Contains(got, unwant) { + t.Errorf("the 401 message says %q for a call that never carried the "+ + "bot token; it sends an operator after the wrong secret: %s", unwant, got) + } + } + }) + } +} + +// Only a 401 is credential-specific. Every other status keeps Discord's own +// message and code, which is what makes a 403 diagnosable as "missing +// permissions" rather than "something went wrong". +func TestNonUnauthorizedErrorsIgnoreTheCredential(t *testing.T) { + for _, cred := range []credential{credentialBotToken, credentialClientSecret, credentialAccessToken} { + err := &APIError{Status: 403, Code: 50013, Message: "Missing Permissions", credential: cred} + if got, want := err.Error(), "discord: Missing Permissions (HTTP 403, code 50013)"; got != want { + t.Errorf("Error() = %q, want %q", got, want) + } + } +} + // A body that is not JSON at all must not mask the status. The status is the // only thing GuildMember and RemoveMemberRole branch on, so losing it turns // "not in the guild" into a hard failure. @@ -357,9 +424,11 @@ func TestRateLimitGivesUpAfterOneRetry(t *testing.T) { } } -// A long Retry-After is real: Discord hands out multi-second waits under a -// global limit. The wait has to be abandonable, or a cancelled HTTP request -// would still hold its goroutine for the whole window. +// A caller that has gone away gets a cancellation, whatever the retry logic +// would otherwise have done with the 429. Cancellation is checked before the +// wait is even measured, so this stays true for the long Retry-After that the +// client now refuses outright: "the caller left" and "Discord asked for ten +// minutes" are different problems, and only the first is worth logging as one. func TestRateLimitWaitHonoursCancellation(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -386,6 +455,72 @@ func TestRateLimitWaitHonoursCancellation(t *testing.T) { } } +// Under a global limit Discord hands out waits measured in tens of seconds or +// minutes. Sleeping one out parks whatever goroutine is here, and on the +// account-linking flow that is an HTTP handler whose request context carries no +// deadline of its own — nothing would cut the wait short. Past the cap the call +// has to come back promptly with an error saying why, rather than holding the +// member's request open for a window Discord chose. +func TestRateLimitLongerThanTheCapIsRefusedRatherThanSlept(t *testing.T) { + c, fake := newFakeDiscord(t, func(w http.ResponseWriter, r *http.Request) { + // What a global limit looks like. + w.Header().Set("Retry-After", "600") + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = io.WriteString(w, `{"message":"You are being rate limited.","code":0}`) + }) + + // Run off the test goroutine so a regression fails in seconds instead of + // sleeping out the ten minutes it was told to. + done := make(chan error, 1) + go func() { done <- c.AddMemberRole(context.Background(), testSnowflake) }() + + select { + case err := <-done: + if err == nil { + t.Fatal("a ten-minute rate limit was reported as success") + } + // Still recognisably a 429: callers branch on the status, and the cap + // is our decision rather than a different answer from Discord. + var apiErr *APIError + if !errors.As(err, &apiErr) || apiErr.Status != http.StatusTooManyRequests { + t.Fatalf("got %v, want an error unwrapping to an APIError carrying 429", err) + } + // The operator log has to say we declined to wait, or this reads as an + // ordinary rate limit and nobody knows the retry never happened. + if !strings.Contains(err.Error(), "rate limited for") { + t.Errorf("error = %q, want it to say the wait was longer than we will hold", err) + } + case <-time.After(maxRetryWait + 2*time.Second): + t.Fatal("the call was still waiting out the Retry-After; a request handler " + + "would be parked for the whole rate-limit window") + } + + if n := fake.count(); n != 1 { + t.Errorf("Discord saw %d requests, want 1 — the retry was abandoned, not made", n) + } +} + +// A wait the client is willing to sit through is still retried. The cap must +// bound the damage without turning the ordinary per-route bucket — which +// resets in well under a second — into a failed call. +func TestRateLimitWithinTheCapIsStillRetried(t *testing.T) { + c, fake := newFakeDiscord(t, sequence( + func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Retry-After", "0.05") + w.WriteHeader(http.StatusTooManyRequests) + }, + jsonReply(200, `{"id":"`+testSnowflake+`"}`), + )) + + if _, err := c.GetUser(context.Background(), testSnowflake); err != nil { + t.Fatalf("a short rate limit was refused instead of retried: %v", err) + } + if n := fake.count(); n != 2 { + t.Errorf("Discord saw %d requests, want 2 (the original and one retry)", n) + } +} + // Discord reports the wait in two different headers depending on the endpoint // and whether the limit is global. Reading neither means always waiting the // one-second fallback, which either retries too early (and gets another 429) @@ -405,12 +540,16 @@ func TestRetryAfterHeaderParsing(t *testing.T) { {"no headers at all", http.Header{}, time.Second}, {"an empty Retry-After falls through", http.Header{"Retry-After": {""}, "X-Ratelimit-Reset-After": {"4"}}, 4 * time.Second}, - // RFC 7231 allows an HTTP-date here. Discord sends seconds, so this is - // a documented gap rather than a bug: an unparseable value falls back - // to a second, which retries early but never hangs. - {"an HTTP-date falls back rather than failing", - http.Header{"Retry-After": {"Wed, 21 Oct 2015 07:28:00 GMT"}}, time.Second}, + // RFC 7231 allows an HTTP-date as well as a number. Discord sends + // seconds, so this is the unusual form — but read as garbage it falls + // back to a second and retries straight back into the same limit. + {"an HTTP-date already past means retry now", + http.Header{"Retry-After": {"Wed, 21 Oct 2015 07:28:00 GMT"}}, 0}, {"garbage falls back", http.Header{"Retry-After": {"soon"}}, time.Second}, + // A window that has already elapsed, or a clock that disagrees. A + // negative duration would make time.After fire at once anyway, but it + // reaches the log first and "wait=-5s" reads as a bug in us. + {"a negative wait is clamped to now", http.Header{"Retry-After": {"-5"}}, 0}, } for _, tt := range tests { @@ -422,6 +561,23 @@ func TestRetryAfterHeaderParsing(t *testing.T) { } } +// The HTTP-date form has to produce a wait measured from now. An exact +// duration would be a race against the clock, so this pins the window: the +// point is that a date is read as a date rather than falling through to the +// one-second default and retrying into a limit that has not lifted. +func TestRetryAfterReadsAnHTTPDate(t *testing.T) { + when := time.Now().Add(2 * time.Second).UTC().Format(http.TimeFormat) + + got := retryAfter(&http.Response{Header: http.Header{"Retry-After": {when}}}) + // http.TimeFormat has second granularity, so anything from just over a + // second to the full two is the date being read correctly. + if got <= 0 || got > 2*time.Second { + t.Errorf("waiting %s for a date two seconds out, want a wait just under two "+ + "seconds; a date read as garbage falls back to one second and retries "+ + "before the limit has lifted", got) + } +} + // A context that is already dead must stop the call before it leaves the // process, and the error must still be recognisable as a cancellation so the // caller does not log a request timeout as a Discord outage. diff --git a/internal/discord/oauth.go b/internal/discord/oauth.go index 4835cd3..98689c6 100644 --- a/internal/discord/oauth.go +++ b/internal/discord/oauth.go @@ -76,9 +76,9 @@ func (c *Client) Exchange(ctx context.Context, code, redirectURI string) (*User, return nil, err } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - apiErr := &APIError{Status: resp.StatusCode} - _ = json.Unmarshal(payload, apiErr) - return nil, apiErr + // The client secret and the code are what this request carried, so a + // 401 here is about one of those and never about the bot token. + return nil, newAPIError(resp.StatusCode, payload, credentialClientSecret) } var token tokenResponse @@ -113,9 +113,8 @@ func (c *Client) currentUser(ctx context.Context, accessToken string) (*User, er return nil, err } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - apiErr := &APIError{Status: resp.StatusCode} - _ = json.Unmarshal(payload, apiErr) - return nil, apiErr + // This leg carries the member's bearer token, not the bot token. + return nil, newAPIError(resp.StatusCode, payload, credentialAccessToken) } var u User @@ -123,11 +122,17 @@ func (c *Client) currentUser(ctx context.Context, accessToken string) (*User, er return nil, err } if u.ID == "" { - return nil, errors.New("discord: the account response carried no id") + return nil, errNoAccountID } return &u, nil } +// errNoAccountID is returned when Discord answers an account lookup without an +// identifier — a 204, or a 200 carrying `{}`. The id is the only field the link +// cannot do without: stored blank, it makes every later guild and role call +// fail on an empty identifier. +var errNoAccountID = errors.New("discord: the account response carried no id") + // GetUser reads an account by identifier, using the bot's own credentials. func (c *Client) GetUser(ctx context.Context, id string) (*User, error) { if !ValidID(id) { @@ -137,5 +142,11 @@ func (c *Client) GetUser(ctx context.Context, id string) (*User, error) { if err := c.do(ctx, http.MethodGet, "/users/"+id, nil, &u); err != nil { return nil, err } + // The same guard currentUser applies. do leaves the destination untouched + // on an empty 2xx body, so without this a 204 would come back as a + // perfectly valid-looking account with no id in it. + if u.ID == "" { + return nil, errNoAccountID + } return &u, nil } diff --git a/internal/discord/oauth_test.go b/internal/discord/oauth_test.go index 317c940..e0ba33f 100644 --- a/internal/discord/oauth_test.go +++ b/internal/discord/oauth_test.go @@ -204,7 +204,9 @@ func TestExchangeTokenEndpointFailures(t *testing.T) { body: `{"error":"invalid_grant","error_description":"Invalid \"code\" in request."}`, wantStatus: 400, }, - {"a rejected client secret", 401, `{"error":"invalid_client"}`, 401, ""}, + // The bot token is not sent to this endpoint, so a 401 here is the + // client secret or the code — never the bot token. + {"a rejected client secret", 401, `{"error":"invalid_client"}`, 401, "DISCORD_CLIENT_SECRET"}, {"the app was disabled", 403, `{"message":"Missing Access","code":50001}`, 403, "Missing Access"}, {"Discord is down", 503, ``, 503, ""}, {"rate limited", 429, `{"message":"You are being rate limited.","code":0}`, 429, ""}, @@ -421,6 +423,97 @@ func TestExchangeNamesTheFailingLeg(t *testing.T) { }) } +// A 401 on either OAuth leg must not blame the bot token, because neither +// request is sent it. This message reaches the operator log verbatim from the +// callback handler, and the everyday cause is a reloaded callback replaying a +// spent code — pointing at DISCORD_BOT_TOKEN sends someone rotating a token +// that works, while the actual cause goes unmentioned. +func TestUnauthorizedOnTheOAuthLegsDoesNotBlameTheBotToken(t *testing.T) { + t.Run("the token exchange", func(t *testing.T) { + c, _ := newFakeDiscord(t, jsonReply(401, `{"error":"invalid_client"}`)) + + _, err := c.Exchange(context.Background(), "the-code", "https://itemize.no/cb") + assertUnauthorizedBlames(t, err, "DISCORD_CLIENT_SECRET") + }) + + t.Run("the account lookup", func(t *testing.T) { + c, _ := newFakeDiscord(t, sequence( + jsonReply(200, `{"access_token":"tok","token_type":"Bearer"}`), + jsonReply(401, `{"message":"401: Unauthorized","code":0}`), + )) + + _, err := c.Exchange(context.Background(), "the-code", "https://itemize.no/cb") + assertUnauthorizedBlames(t, err, "access token") + }) + + // The bot calls are the ones the old message was written for, and they must + // keep it: a rejected bot token really is a DISCORD_BOT_TOKEN problem. + t.Run("a bot call still names the bot token", func(t *testing.T) { + c, _ := newFakeDiscord(t, jsonReply(401, `{"message":"401: Unauthorized","code":0}`)) + + _, err := c.GetUser(context.Background(), testSnowflake) + if err == nil || !strings.Contains(err.Error(), "DISCORD_BOT_TOKEN") { + t.Fatalf("error = %v, want it to name DISCORD_BOT_TOKEN", err) + } + }) +} + +// assertUnauthorizedBlames checks that a 401 from an OAuth leg names want and +// nothing about the bot token. +func assertUnauthorizedBlames(t *testing.T, err error, want string) { + t.Helper() + + var apiErr *APIError + if !errors.As(err, &apiErr) || apiErr.Status != http.StatusUnauthorized { + t.Fatalf("got %v, want an APIError carrying 401", err) + } + if !strings.Contains(err.Error(), want) { + t.Errorf("the 401 message does not mention %q, so the log does not say what "+ + "was actually rejected: %s", want, err) + } + if strings.Contains(err.Error(), "check DISCORD_BOT_TOKEN") { + t.Errorf("the 401 message tells the operator to check DISCORD_BOT_TOKEN for a "+ + "call that never carried it: %s", err) + } +} + +// GetUser applies the same id guard as the OAuth account lookup, so the two +// agree about what a usable account is. Without it a 204 or an empty object +// decodes into an account with no identifier: the refresh stores a blank link, +// GuildMember is then handed "" and answers ErrInvalidID, and the member +// silently loses role management with "could not check guild membership" as the +// only trace. +func TestGetUserRejectsAnAccountWithoutAnID(t *testing.T) { + tests := []struct { + name string + status int + body string + }{ + {"an empty object", 200, `{}`}, + {"a username but no id", 200, `{"username":"kari","global_name":"Kari"}`}, + {"an explicitly empty id", 200, `{"id":"","username":"kari"}`}, + {"no content at all", 204, ``}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c, _ := newFakeDiscord(t, jsonReply(tt.status, tt.body)) + + u, err := c.GetUser(context.Background(), testSnowflake) + if err == nil { + t.Fatal("an account with no id was accepted; the link would be stored " + + "blank and every later role call would fail on an empty identifier") + } + if u != nil { + t.Errorf("got %+v alongside the error", u) + } + if !strings.Contains(err.Error(), "id") { + t.Errorf("error = %q, want it to say the id was missing", err) + } + }) + } +} + // A linked account is rendered from whatever Discord returns, including names // in scripts the site never anticipated. Anything mangled here is written // verbatim into the member's profile. diff --git a/internal/events/validate_test.go b/internal/events/validate_test.go index 1814d79..7688076 100644 --- a/internal/events/validate_test.go +++ b/internal/events/validate_test.go @@ -172,8 +172,16 @@ func TestDurationBoundaries(t *testing.T) { "scientific notation is parsed and then rejected by the ceiling", "1e3", "Varighet kan ikke være større enn 168.", 1000, }, - {"infinity is above the ceiling", "Inf", "Varighet kan ikke være større enn 168.", math.Inf(1)}, - {"negative infinity is below the floor", "-Inf", "Varighet kan ikke være mindre enn 0.", math.Inf(-1)}, + // ParseFloat accepts these spellings, but no range check can hold them: + // NaN compares false against both bounds and infinity is not a length of + // time. They are refused as "not a number" rather than as out of range, + // and the returned duration falls back to zero like any other value that + // could not be read. + {"infinity is not a number", "Inf", "Varighet må være et tall.", 0}, + {"negative infinity is not a number either", "-Inf", "Varighet må være et tall.", 0}, + {"the long spelling of infinity", "infinity", "Varighet må være et tall.", 0}, + {"NaN is not a number", "NaN", "Varighet må være et tall.", 0}, + {"the spelling of NaN is not significant", "nan", "Varighet må være et tall.", 0}, } for _, tt := range tests { @@ -189,24 +197,37 @@ func TestDurationBoundaries(t *testing.T) { } } -// "NaN" parses as a float, and NaN compares false against both bounds, so it -// slips through untouched. ComputeEnd then produces an end time in the -// eighteenth century, which reads as an event that finished long ago. +// "NaN" and "Inf" parse as floats, and the range check cannot catch them — NaN +// compares false against both bounds, and neither is a length of time. The +// browser's number input will not submit them, but the endpoint accepts any +// form body, so the guard has to be in the validation rather than the markup. // -// This pins the current behaviour rather than endorsing it: it is a real hole -// in the validation, reported alongside these tests. If the range check ever -// learns about NaN, this test is the one to delete. -func TestDurationNaNIsCurrentlyAccepted(t *testing.T) { - ev, verr := FromForm(formWith(t, "duration", "NaN")) - - if msg, ok := verr["duration"]; ok { - t.Skipf("NaN is now rejected with %q — the hole this test documented has been closed", msg) - } - if !math.IsNaN(ev.Duration) { - t.Fatalf("duration = %v, want NaN", ev.Duration) - } - if end := ev.ComputeEnd(); !end.Before(ev.Date) { - t.Errorf("end = %v, expected the nonsensical value NaN produces; the consequence of accepting it is what makes this a bug", end) +// The consequence of letting one through is what this test really protects +// against: ComputeEnd multiplies the duration by an hour, and for NaN that is +// the minimum int64, which puts the end of the event in 1733. Past() then +// reports it finished, and the event disappears from the listing the moment the +// board saves it — with no error to explain where it went. +func TestNonFiniteDurationsAreRejectedBeforeTheyReachTheEndTime(t *testing.T) { + for _, value := range []string{"NaN", "nan", "Inf", "+Inf", "-Inf", "infinity"} { + t.Run(value, func(t *testing.T) { + ev, verr := FromForm(formWith(t, "duration", value)) + + if verr["duration"] == "" { + t.Fatalf("duration %q was accepted; the saved event's end time would "+ + "be nonsense and the board would never see it in the listing", value) + } + if math.IsNaN(ev.Duration) || math.IsInf(ev.Duration, 0) { + t.Fatalf("duration %q came back as %v; a rejected value must not be "+ + "carried into the event at all", value, ev.Duration) + } + // The end time is derived, so a refused duration has to leave it at the + // start rather than somewhere in the eighteenth century. + if end := ev.ComputeEnd(); !end.Equal(ev.Date) { + t.Errorf("end = %v, want the unchanged start %v — an end time before "+ + "the start reads as an event that has already finished", + end, ev.Date) + } + }) } } diff --git a/internal/httpx/assets_test.go b/internal/httpx/assets_test.go index 0015d8f..9bb62ec 100644 --- a/internal/httpx/assets_test.go +++ b/internal/httpx/assets_test.go @@ -801,6 +801,35 @@ func TestAssetsAreNotDoubleCompressed(t *testing.T) { } } +// A compressible asset small enough that gzip made it larger has no stored +// compressed copy, so Assets serves it as identity — including for a range +// request, where ServeContent computes Content-Range over those identity +// bytes. The gzip middleware wraps the mux in production, so it sees that 206 +// on its way out; if it compressed the body, the offsets it is labelled with +// would no longer describe what the client receives and a resumed download +// would be stitched back together wrongly. +func TestAssetsRangeIsNotCompressedDownstream(t *testing.T) { + tiny := testAssetFS() + tiny["static/robots.txt"] = &fstest.MapFile{Data: []byte("User-agent: *\nAllow: /\n")} + h := Gzip(newTestAssets(t, tiny, false)) + + rec := httptest.NewRecorder() + req := wireRequest(t, "/robots.txt") + req.Header.Set("Accept-Encoding", "gzip") + req.Header.Set("Range", "bytes=0-4") + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusPartialContent { + t.Fatalf("got %d, want 206 — the fixture no longer exercises the range path", rec.Code) + } + if enc := rec.Header().Get("Content-Encoding"); enc != "" { + t.Errorf("Content-Encoding = %q on a 206; the range offsets no longer describe the bytes on the wire", enc) + } + if got := rec.Body.String(); got != "User-" { + t.Errorf("body = %q, want the first five bytes the Content-Range promises", got) + } +} + // Dev mode rebuilds the table inside ServeHTTP while templates concurrently // call URL for the same table. Without the lock this is a straight data race, // and the symptom in production would be an intermittent panic under load. diff --git a/internal/httpx/gzip.go b/internal/httpx/gzip.go index 0273c3a..22e5fce 100644 --- a/internal/httpx/gzip.go +++ b/internal/httpx/gzip.go @@ -3,6 +3,7 @@ package httpx import ( "compress/gzip" "net/http" + "strconv" "strings" "sync" ) @@ -38,10 +39,38 @@ func Gzip(next http.Handler) http.Handler { }) } +// acceptsGzip reports whether the client offered gzip and did not then refuse +// it with a zero q-value. +// +// A bare "*" does not count as an offer, so "*;q=0" needs no special handling: +// without an explicit gzip token the answer is already no, and with one the +// explicit entry wins over the wildcard under RFC 9110 anyway. func acceptsGzip(r *http.Request) bool { for _, enc := range strings.Split(r.Header.Get("Accept-Encoding"), ",") { - name, _, _ := strings.Cut(strings.TrimSpace(enc), ";") - if strings.EqualFold(name, "gzip") { + // The whitespace has to come off after the cut as well as before it: + // RFC 9110 allows it on both sides of the ";", so "gzip ; q=1.0" is a + // perfectly legal way to ask for gzip. + name, params, _ := strings.Cut(enc, ";") + if strings.EqualFold(strings.TrimSpace(name), "gzip") { + return !refusedByQValue(params) + } + } + return false +} + +// refusedByQValue reports whether a coding's parameters carry q=0, which +// RFC 9110 defines as "not acceptable" — compressing for such a client is +// doing the one thing it explicitly asked us not to do. Ranking the codings +// against each other is not worth it here: gzip is the only encoding we can +// produce, so the only question is whether it is allowed at all. +func refusedByQValue(params string) bool { + for _, p := range strings.Split(params, ";") { + v, ok := strings.CutPrefix(strings.ToLower(strings.TrimSpace(p)), "q=") + if !ok { + continue + } + // Any legal spelling of zero: "0", "0.0", "0.000". + if q, err := strconv.ParseFloat(strings.TrimSpace(v), 64); err == nil && q <= 0 { return true } } @@ -89,7 +118,13 @@ func (w *gzipResponseWriter) WriteHeader(status int) { h := w.Header() // 204/304 have no body, and an existing Content-Encoding means the // handler already compressed (or otherwise encoded) the payload. + // + // A 206 is excluded because its Content-Range counts offsets into the body + // the handler chose to send. Compressing it here would leave those offsets + // describing bytes that are no longer on the wire, and a client resuming a + // download would stitch the pieces back together wrongly. if status != http.StatusNoContent && status != http.StatusNotModified && + status != http.StatusPartialContent && h.Get("Content-Encoding") == "" && compressible(h.Get("Content-Type")) { // The compressed length is unknown until the body is written, and an diff --git a/internal/httpx/gzip_test.go b/internal/httpx/gzip_test.go index ca63ba0..a26f3af 100644 --- a/internal/httpx/gzip_test.go +++ b/internal/httpx/gzip_test.go @@ -59,11 +59,12 @@ func TestAcceptsGzip(t *testing.T) { {"gzip, deflate, br", true}, {"br;q=1.0, gzip;q=0.8", true}, {" gzip,deflate ", true}, - // RFC 9110 permits whitespace around the ";" that introduces the - // q-value. Only the space before the whole coding is trimmed here, so - // a client writing it this way is served uncompressed. Pinned so the - // deviation is visible rather than accidental. - {"gzip ; q=1.0", false}, + // RFC 9110 permits whitespace on both sides of the ";" that introduces + // the q-value, so the coding name has to be trimmed after the cut as + // well as before it. Trimming only the whole coding leaves the name as + // "gzip " and this client is silently served uncompressed. + {"gzip ; q=1.0", true}, + {"gzip\t;q=1.0", true}, {"identity", false}, {"deflate", false}, {"br", false}, @@ -72,10 +73,26 @@ func TestAcceptsGzip(t *testing.T) { {"gzipper", false}, {",,,", false}, {";q=1", false}, - // RFC 9110 gives q=0 the meaning "not acceptable". The parser here - // ignores q-values entirely, so this client is sent gzip anyway. Pinned - // so the deviation is visible rather than accidental. - {"gzip;q=0", true}, + // RFC 9110 gives q=0 the meaning "not acceptable", in any of its legal + // spellings. Ignoring the q-value sends a gzipped body to a client that + // just said in as many words that it cannot read one. + {"gzip;q=0", false}, + {"gzip;q=0.0", false}, + {"gzip;q=0.000", false}, + {"gzip ; q=0", false}, + {"gzip;Q=0", false}, + {"deflate, gzip;q=0", false}, + // Anything above zero is acceptance: we do not rank codings, because + // gzip is the only one this server can produce. + {"gzip;q=0.001", true}, + {"gzip;q=1", true}, + // A wildcard refusal does not override an explicit offer of gzip: the + // specific entry wins, and without one the answer is already no. + {"gzip, *;q=0", true}, + {"*;q=0", false}, + // A malformed q-value is not a refusal; the offer still stands. + {"gzip;q=nonsense", true}, + {"gzip;level=1", true}, } for _, tt := range tests { @@ -218,6 +235,17 @@ func TestGzipMiddleware(t *testing.T) { status: http.StatusNotModified, wantVary: true, }, + { + // A 206 body is a slice the handler picked, and its Content-Range + // counts offsets into the bytes it sent. Compressing it out here + // leaves those offsets describing a payload that is no longer on + // the wire, and a client resuming a download reassembles garbage. + name: "206 is a range the handler already measured", + accept: "gzip", + contentType: "text/html; charset=utf-8", + status: http.StatusPartialContent, + wantVary: true, + }, { name: "error pages are compressed like any other page", accept: "gzip", diff --git a/internal/httpx/ratelimit.go b/internal/httpx/ratelimit.go index 9b4bbc3..129a779 100644 --- a/internal/httpx/ratelimit.go +++ b/internal/httpx/ratelimit.go @@ -1,6 +1,8 @@ package httpx import ( + "fmt" + "math" "net" "net/http" "sync" @@ -27,6 +29,9 @@ type RateLimiter struct { burst int window time.Duration + + stopOnce sync.Once + stop chan struct{} } type bucket struct { @@ -34,17 +39,51 @@ type bucket struct { seen time.Time } +const ( + // How often idle buckets are swept, and how long a bucket has to go + // untouched before it is swept. + reapInterval = 10 * time.Minute + idleTTL = time.Hour +) + // NewRateLimiter allows burst requests, refilled over window. +// +// It panics if either is not positive. Both call sites pass constants, so a +// bad value is a programmer error that a panic surfaces at startup — before +// the process serves anything — rather than a misconfiguration arriving from +// the outside world that we should tolerate at runtime. Silently substituting +// a default would be worse than either: a window of zero used to divide by +// zero in Allow and store a NaN in the bucket, after which every comparison +// against it was false and that client was never refused again. A limiter that +// looks installed but never limits is the failure mode this whole type exists +// to avoid, so it must not be reachable at all. func NewRateLimiter(burst int, window time.Duration) *RateLimiter { + if burst <= 0 { + panic(fmt.Sprintf("httpx.NewRateLimiter: burst must be positive, got %d", burst)) + } + if window <= 0 { + panic(fmt.Sprintf("httpx.NewRateLimiter: window must be positive, got %v", window)) + } l := &RateLimiter{ buckets: map[string]*bucket{}, burst: burst, window: window, + stop: make(chan struct{}), } - go l.reap() + go l.reap(reapInterval) return l } +// Stop ends the reaper goroutine and releases its ticker. It is safe to call +// more than once and safe never to call: the two limiters in this program live +// for the lifetime of the process. It exists so that a limiter created per test +// or per request does not leak a goroutine and a ticker for each one. +// +// A stopped limiter still limits; only the eviction of idle buckets stops. +func (l *RateLimiter) Stop() { + l.stopOnce.Do(func() { close(l.stop) }) +} + // Allow reports whether a request from key may proceed. func (l *RateLimiter) Allow(key string) bool { now := time.Now() @@ -58,8 +97,15 @@ func (l *RateLimiter) Allow(key string) bool { return true } - // Refill in proportion to the time that has passed. + // Refill in proportion to the time that has passed. The window is positive + // by construction so this cannot divide by zero; the guard is belt and + // braces, because a NaN stored here makes every later "b.tokens < 1" + // comparison false and that bucket is never refused again. A clock that + // jumped backwards is treated as no time passing rather than as a debit. refill := now.Sub(b.seen).Seconds() / l.window.Seconds() * float64(l.burst) + if math.IsNaN(refill) || refill < 0 { + refill = 0 + } b.tokens = min(b.tokens+refill, float64(l.burst)) b.seen = now @@ -73,17 +119,30 @@ func (l *RateLimiter) Allow(key string) bool { // reap discards idle buckets so the map cannot grow without bound — otherwise // the limiter itself becomes the memory-exhaustion vector it was added to // prevent. -func (l *RateLimiter) reap() { - for range time.Tick(10 * time.Minute) { - cutoff := time.Now().Add(-time.Hour) - - l.mu.Lock() - for key, b := range l.buckets { - if b.seen.Before(cutoff) { - delete(l.buckets, key) - } +// It runs until Stop. The interval is a parameter rather than a constant so a +// test can drive it without waiting ten minutes for the first tick. +func (l *RateLimiter) reap(every time.Duration) { + t := time.NewTicker(every) + defer t.Stop() + + for { + select { + case <-l.stop: + return + case now := <-t.C: + l.evictIdle(now.Add(-idleTTL)) + } + } +} + +// evictIdle drops every bucket last seen before cutoff. +func (l *RateLimiter) evictIdle(cutoff time.Time) { + l.mu.Lock() + defer l.mu.Unlock() + for key, b := range l.buckets { + if b.seen.Before(cutoff) { + delete(l.buckets, key) } - l.mu.Unlock() } } diff --git a/internal/httpx/ratelimit_test.go b/internal/httpx/ratelimit_test.go index 9792849..34f073b 100644 --- a/internal/httpx/ratelimit_test.go +++ b/internal/httpx/ratelimit_test.go @@ -1,6 +1,7 @@ package httpx import ( + "math" "net/http" "net/http/httptest" "strconv" @@ -10,8 +11,19 @@ import ( "time" ) +// newTestLimiter builds a limiter and stops its reaper when the test ends. A +// test binary that creates one per case would otherwise accumulate a goroutine +// and a ticker for every one of them, which is exactly the leak Stop exists to +// close. +func newTestLimiter(t *testing.T, burst int, window time.Duration) *RateLimiter { + t.Helper() + l := NewRateLimiter(burst, window) + t.Cleanup(l.Stop) + return l +} + func TestRateLimiterAllowsBurstThenRefuses(t *testing.T) { - l := NewRateLimiter(3, time.Hour) + l := newTestLimiter(t, 3, time.Hour) for i := 1; i <= 3; i++ { if !l.Allow("1.2.3.4") { @@ -29,7 +41,7 @@ func TestRateLimiterAllowsBurstThenRefuses(t *testing.T) { func TestRateLimiterRefills(t *testing.T) { // A short window so the refill is observable. - l := NewRateLimiter(2, 100*time.Millisecond) + l := newTestLimiter(t, 2, 100*time.Millisecond) l.Allow("1.2.3.4") l.Allow("1.2.3.4") @@ -46,7 +58,7 @@ func TestRateLimiterRefills(t *testing.T) { // Reads must not be throttled. A university network shares one address, so // limiting ordinary browsing would take the site down for a lecture hall. func TestRateLimiterIgnoresSafeMethods(t *testing.T) { - l := NewRateLimiter(1, time.Hour) + l := newTestLimiter(t, 1, time.Hour) h := l.Limit(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) })) @@ -63,7 +75,7 @@ func TestRateLimiterIgnoresSafeMethods(t *testing.T) { } func TestLimitRejectsWithRetryAfter(t *testing.T) { - l := NewRateLimiter(1, time.Hour) + l := newTestLimiter(t, 1, time.Hour) h := l.Limit(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) })) @@ -148,7 +160,7 @@ func TestRateLimiterRefillsProportionally(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - l := NewRateLimiter(10, time.Hour) + l := newTestLimiter(t, 10, time.Hour) if got := countAllowed(l, "1.2.3.4", 10); got != 10 { t.Fatalf("the initial burst allowed %d of 10", got) } @@ -168,7 +180,7 @@ func TestRateLimiterRefillsProportionally(t *testing.T) { // A shared address is one bucket, but distinct addresses must never interfere. // If they did, one abusive client would lock out everyone else on the site. func TestRateLimiterIsolatesKeys(t *testing.T) { - l := NewRateLimiter(2, time.Hour) + l := newTestLimiter(t, 2, time.Hour) keys := []string{"1.2.3.4", "5.6.7.8", "2001:db8::1", "", "10.0.0.1"} for _, key := range keys { @@ -195,7 +207,7 @@ func TestRateLimiterIsolatesKeys(t *testing.T) { // A burst of one is the tightest useful setting and the one most likely to // expose an off-by-one: the first request must succeed and the second must not. func TestRateLimiterBurstOfOne(t *testing.T) { - l := NewRateLimiter(1, time.Hour) + l := newTestLimiter(t, 1, time.Hour) if !l.Allow("1.2.3.4") { t.Fatal("the very first request was refused") } @@ -208,16 +220,60 @@ func TestRateLimiterBurstOfOne(t *testing.T) { } } -// A burst of zero does not deny everything: the first request from an address -// creates its bucket and is allowed unconditionally, and only the second is -// refused. Pinned because "burst 0" reads like "block everything" and is not. -func TestRateLimiterBurstOfZeroStillAllowsTheFirstRequest(t *testing.T) { - l := NewRateLimiter(0, time.Hour) - if !l.Allow("1.2.3.4") { - t.Error("behaviour changed: a burst of zero now refuses the first request too") +// A limiter that cannot limit must not be constructible. A window of zero used +// to divide by zero in the refill, storing a NaN that made every later +// comparison false — the bucket was never refused again, so the endpoint that +// mails whatever address is submitted was left unprotected for the life of the +// process. Both call sites pass constants, so failing at construction turns a +// silent security hole into a crash on the first run. +func TestNewRateLimiterRejectsSettingsThatCannotLimit(t *testing.T) { + tests := []struct { + name string + burst int + window time.Duration + }{ + {name: "zero window", burst: 5, window: 0}, + {name: "negative window", burst: 5, window: -time.Hour}, + {name: "zero burst", burst: 0, window: time.Hour}, + {name: "negative burst", burst: -1, window: time.Hour}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + defer func() { + if recover() == nil { + t.Errorf("NewRateLimiter(%d, %v) returned a limiter; a misconfiguration that never refuses anything must not start", + tt.burst, tt.window) + } + }() + l := NewRateLimiter(tt.burst, tt.window) + l.Stop() // unreachable unless the guard is gone, but do not leak if it is + }) + } +} + +// Belt and braces for the same failure: even if a NaN reached a bucket by some +// other route, one must never be stored, because "NaN < 1" is false and that +// client would then be allowed through forever. +func TestRateLimiterNeverStoresNaNTokens(t *testing.T) { + l := newTestLimiter(t, 3, time.Hour) + for range 3 { + l.Allow("1.2.3.4") } + + // A clock that jumped backwards produces a negative elapsed time, which is + // the other way the refill arithmetic can misbehave. It must not hand out + // credit and must not corrupt the bucket. + rewind(t, l, "1.2.3.4", -time.Hour) if l.Allow("1.2.3.4") { - t.Error("a second request was allowed with a burst of zero") + t.Error("an exhausted client got through after the clock jumped backwards") + } + + l.mu.Lock() + tokens := l.buckets["1.2.3.4"].tokens + l.mu.Unlock() + if math.IsNaN(tokens) { + t.Error("the bucket holds NaN; every later comparison is false, so this client is never refused again") } } @@ -232,7 +288,7 @@ func TestRateLimiterIsSafeUnderConcurrency(t *testing.T) { ) // A window this long makes refill during the test immeasurably small, so // the total is exact rather than approximately right. - l := NewRateLimiter(burst, 24*time.Hour) + l := newTestLimiter(t, burst, 24*time.Hour) var ( mu sync.Mutex @@ -265,7 +321,7 @@ func TestRateLimiterIsSafeUnderConcurrency(t *testing.T) { // Separate clients hammering at once must each get their full allowance, and // the map they all write to must survive it. func TestRateLimiterConcurrentDistinctKeys(t *testing.T) { - l := NewRateLimiter(3, 24*time.Hour) + l := newTestLimiter(t, 3, 24*time.Hour) var wg sync.WaitGroup for i := range 50 { @@ -326,7 +382,7 @@ func TestClientIP(t *testing.T) { // full RemoteAddr would give each connection its own allowance, which is the // same as having no limiter for anyone willing to reconnect. func TestLimitSharesABucketAcrossPorts(t *testing.T) { - l := NewRateLimiter(2, time.Hour) + l := newTestLimiter(t, 2, time.Hour) h := l.Limit(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) })) @@ -370,7 +426,7 @@ func TestLimitCountsOnlyUnsafeMethods(t *testing.T) { for _, tt := range tests { t.Run(tt.method, func(t *testing.T) { - l := NewRateLimiter(1, time.Hour) + l := newTestLimiter(t, 1, time.Hour) h := l.Limit(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) })) @@ -398,7 +454,7 @@ func TestLimitCountsOnlyUnsafeMethods(t *testing.T) { // A throttled request must not reach the handler at all — the handler is what // sends the mail the limit exists to prevent. func TestLimitStopsTheRequestReachingTheHandler(t *testing.T) { - l := NewRateLimiter(1, time.Hour) + l := newTestLimiter(t, 1, time.Hour) reached := 0 h := l.Limit(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { reached++ @@ -420,7 +476,7 @@ func TestLimitStopsTheRequestReachingTheHandler(t *testing.T) { // The 429 is shown to a person who has just filled in a form, so it has to say // what happened in the site's language rather than showing Go's default text. func TestLimitExplainsItselfInNorwegian(t *testing.T) { - l := NewRateLimiter(1, time.Hour) + l := newTestLimiter(t, 1, time.Hour) h := l.Limit(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) var rec *httptest.ResponseRecorder @@ -445,7 +501,7 @@ func TestLimitExplainsItselfInNorwegian(t *testing.T) { // Refilling has to work through the middleware too, not just through Allow: // a visitor locked out by a typo must eventually be able to submit again. func TestLimitRecoversAfterTheWindow(t *testing.T) { - l := NewRateLimiter(1, time.Hour) + l := newTestLimiter(t, 1, time.Hour) h := l.Limit(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) })) @@ -470,21 +526,15 @@ func TestLimitRecoversAfterTheWindow(t *testing.T) { // Idle buckets are deleted so the map cannot grow without bound — otherwise // the limiter is itself the memory-exhaustion vector it was added to prevent. -// The reaper's ticker is fixed at ten minutes and takes no clock, so this -// exercises the eviction rule directly rather than waiting for it to fire. func TestRateLimiterEvictionRule(t *testing.T) { - l := NewRateLimiter(5, time.Hour) + l := newTestLimiter(t, 5, time.Hour) l.Allow("recent") l.Allow("idle") - rewind(t, l, "idle", 2*time.Hour) + rewind(t, l, "idle", 2*idleTTL) + + l.evictIdle(time.Now().Add(-idleTTL)) - cutoff := time.Now().Add(-time.Hour) l.mu.Lock() - for key, b := range l.buckets { - if b.seen.Before(cutoff) { - delete(l.buckets, key) - } - } _, keptRecent := l.buckets["recent"] _, keptIdle := l.buckets["idle"] l.mu.Unlock() @@ -496,3 +546,61 @@ func TestRateLimiterEvictionRule(t *testing.T) { t.Error("a bucket untouched for two hours survived; the map would grow with every address ever seen") } } + +// The reaper itself, not just the rule it applies: with the ten-minute +// interval hardcoded this loop never ran in a test at all, so nothing would +// have caught a reaper that ticked but swept nothing. +func TestRateLimiterReaperEvictsOnTick(t *testing.T) { + l := newTestLimiter(t, 5, time.Hour) + l.Allow("recent") + l.Allow("idle") + rewind(t, l, "idle", 2*idleTTL) + + go l.reap(time.Millisecond) + + deadline := time.Now().Add(2 * time.Second) + for { + l.mu.Lock() + _, keptIdle := l.buckets["idle"] + _, keptRecent := l.buckets["recent"] + l.mu.Unlock() + + if !keptIdle { + if !keptRecent { + t.Error("the reaper also evicted an active client, which hands them a fresh allowance") + } + return + } + if time.Now().After(deadline) { + t.Fatal("the reaper never swept an idle bucket; the map grows with every address the site ever sees") + } + time.Sleep(time.Millisecond) + } +} + +// Every limiter used to run a ticker and a goroutine for the life of the +// process with no way to stop them. That is survivable for the two limiters +// this program builds at startup, but it makes the type unusable anywhere one +// is created per request or per test. +func TestRateLimiterStopEndsTheReaper(t *testing.T) { + l := NewRateLimiter(5, time.Hour) + + done := make(chan struct{}) + go func() { + l.reap(time.Millisecond) + close(done) + }() + + l.Stop() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("the reaper is still running after Stop; every limiter ever created leaks a goroutine and a ticker") + } + + // Stop is safe to call twice, and a stopped limiter still limits. + l.Stop() + if !l.Allow("1.2.3.4") { + t.Error("a stopped limiter refuses everything; stopping the reaper must not take the endpoint offline") + } +} diff --git a/internal/users/discord.go b/internal/users/discord.go index 99329ff..5024778 100644 --- a/internal/users/discord.go +++ b/internal/users/discord.go @@ -66,6 +66,16 @@ func (s *DiscordService) Complete(ctx context.Context, userID, code, redirectURI if err != nil { return nil, err } + // Nothing checks that this Discord account is not already linked to another + // member, and store writes without reading what is there first. The stored + // links do not collide — each patch touches only its own FusionAuth record, + // so no other member's link is detached — but the guild role is granted and + // withdrawn by Discord id alone, guild-wide. Two members sharing one Discord + // account therefore share one role: when either presses "fjern kobling", + // Unlink calls RemoveMemberRole on that id and the other member loses their + // access while their own profile still shows the account as linked and them + // as a guild member. Enforcing uniqueness needs a FusionAuth user search on + // data.discord.id, so it is left as a product decision rather than done here. return s.store(ctx, userID, account) } @@ -122,15 +132,23 @@ func (s *DiscordService) store(ctx context.Context, userID string, account *disc MembershipUnknown: unknown, } + stored := map[string]any{ + "id": link.ID, + "username": link.Username, + "avatar": link.Avatar, + } + // Only a real answer is written. A failed check says nothing about this + // member, and storing it as isMember:false would turn our own outage into a + // permanent "has not joined": the next page load reads the record back + // through CurrentLink, which has no notion of the check having failed, and + // tells them to join a server they may already be in. Leaving the key out + // of the merge patch keeps whatever was last known in place instead. + if !unknown { + stored["isMember"] = link.IsMember + } + changes := map[string]any{ - "data": map[string]any{ - "discord": map[string]any{ - "id": link.ID, - "username": link.Username, - "avatar": link.Avatar, - "isMember": link.IsMember, - }, - }, + "data": map[string]any{"discord": stored}, "imageUrl": link.Avatar, } if _, err := s.fusion.PatchUser(ctx, userID, changes); err != nil { diff --git a/internal/users/discord_test.go b/internal/users/discord_test.go index f027718..b7333f6 100644 --- a/internal/users/discord_test.go +++ b/internal/users/discord_test.go @@ -159,18 +159,22 @@ func TestCompleteRecordsMembershipTruthfully(t *testing.T) { if block["id"] != testDiscordID { t.Errorf("stored discord id = %v, want %q", block["id"], testDiscordID) } - // Note what this pins in the two "unknown" cases: isMember is - // written as false, and MembershipUnknown is not written at all. - // The distinction the Link carries therefore survives exactly one - // response — the next page load reads the record back through - // CurrentLink and tells the member they have not joined. - if block["isMember"] != tt.wantMember { + // A real answer is stored; a failed check writes no isMember at + // all. Omitting the key from the merge patch is what keeps our own + // outage from being recorded as a fact about the member: writing + // false would outlive the one response that carries + // MembershipUnknown, because the next page load reads the record + // back through CurrentLink, which has no such field. + value, present := block["isMember"] + if tt.wantUnknown { + if present { + t.Errorf("stored isMember = %v after a failed membership check; "+ + "a guess written here is read back as settled on every "+ + "later page load", value) + } + } else if value != tt.wantMember { t.Errorf("stored isMember = %v, want %v — the profile reads this "+ - "back on every page load", block["isMember"], tt.wantMember) - } - if _, stored := block["membershipUnknown"]; stored { - t.Error("membershipUnknown reached storage; if that is now " + - "deliberate, CurrentLink has to read it back") + "back on every page load", value, tt.wantMember) } }) } @@ -513,6 +517,52 @@ func TestRefreshReconcilesTheStoredLink(t *testing.T) { } } +// A membership check that could not be made is not an answer, and it must not +// be allowed to demote a member who is known to be in the guild. +// +// This is the case the whole distinction exists for: the member is in the +// server, presses «Oppdater» while our bot token is rejected, and the record +// says isMember:true. Writing false would replace that with a guess which +// CurrentLink reads back as settled on every later page load, so the profile +// would tell them to go and join a server they never left — over an outage +// that is ours and that they cannot do anything about. +func TestAFailedMembershipCheckDoesNotDemoteAKnownMember(t *testing.T) { + log := &callLog{} + api := newDiscordAPI(log) + api.inGuild = true + api.memberStatus = http.StatusForbidden // the bot is missing the intent + fusion := newFusionAPI(log).withLink(testDiscordID, "Kari N", "an-avatar", true) + svc := linkService(t, api, fusion) + + link, err := svc.Refresh(context.Background(), testUserID) + if err != nil { + t.Fatalf("refreshing failed: %v — a failed membership check must not cost "+ + "the member their link", err) + } + if !link.MembershipUnknown { + t.Error("MembershipUnknown = false after the check could not be made; the " + + "profile would blame the member for an outage of ours") + } + + block, ok := discordBlock(t, fusion.lastPatch(t)) + if !ok { + t.Fatal("data.discord was not written as an object") + } + if value, present := block["isMember"]; present { + t.Fatalf("isMember was written as %v from a check that never got an answer; "+ + "the stored true is what keeps the member out of the \"go and join\" "+ + "message on the next page load", value) + } + + // What the profile reads on the next page load is the record as it stands + // after the patch. With isMember absent from a merge patch, that is still + // the true FusionAuth already held. + if current := CurrentLink(&fusion.user); current == nil || !current.IsMember { + t.Errorf("the stored link reads back as %+v, want a member — this is the "+ + "value the profile page renders from", current) + } +} + // When the upstreams fail, a refresh must leave the stored link alone. Losing // a link because Discord had a bad minute would mean re-authorising. func TestRefreshUpstreamFailuresKeepTheLink(t *testing.T) { diff --git a/internal/users/register.go b/internal/users/register.go index 607c0e4..167102f 100644 --- a/internal/users/register.go +++ b/internal/users/register.go @@ -6,6 +6,7 @@ import ( "net/url" "strings" "time" + "unicode" "github.com/ItemizeNTNU/website/internal/fusionauth" "github.com/ItemizeNTNU/website/internal/validate" @@ -39,6 +40,12 @@ const studEmailSuffix = "@stud.ntnu.no" const studEmailMessage = "Vennligst ikke bruk din stud e-post adresse, " + "da du mister tilgang til denne etter fullført utdannelse." +// emailMax is the longest address that can actually be delivered: RFC 5321 +// caps the whole envelope path at 254 characters. Every other field here has a +// ceiling; without one the address is forwarded verbatim to FusionAuth, which +// sends a password-setting email to it. +const emailMax = 254 + // Registration is a validated signup, ready to send to FusionAuth. type Registration struct { FullName string @@ -107,9 +114,13 @@ func validateEmail(e *validate.Errors, raw string) string { e.Add("email", "E-postadresse må fylles ut.") return addr } + if len([]rune(addr)) > emailMax { + e.Add("email", fmt.Sprintf("E-postadresse kan ikke være lengre enn %d tegn.", emailMax)) + return addr + } local, domain, ok := strings.Cut(addr, "@") if !ok || local == "" || domain == "" || !strings.Contains(domain, ".") || - strings.ContainsAny(addr, " \t") { + strings.IndexFunc(addr, notAllowedInEmail) >= 0 { e.Add("email", "E-postadressen ser ikke gyldig ut.") return addr } @@ -121,6 +132,19 @@ func validateEmail(e *validate.Errors, raw string) string { return addr } +// notAllowedInEmail reports the characters an address must never contain. +// +// The check used to name a space and a tab, which left \n, \r and NUL to pass +// straight through. That matters more here than in an ordinary field: the +// address is handed to FusionAuth, which builds an email from it, so a line +// break is not merely malformed — it is a character with meaning to everything +// downstream. IsControl covers U+0000–U+001F and U+007F; IsSpace covers the +// separators TrimSpace leaves behind in the middle of the string, including +// the non-breaking space a paste from a PDF carries. +func notAllowedInEmail(r rune) bool { + return unicode.IsControl(r) || unicode.IsSpace(r) +} + // ToFusionAuth builds the user record. // // The data map contains only the keys for the chosen membership type. The diff --git a/internal/users/register_boundary_test.go b/internal/users/register_boundary_test.go index 1811636..03293b7 100644 --- a/internal/users/register_boundary_test.go +++ b/internal/users/register_boundary_test.go @@ -248,6 +248,20 @@ func TestEmailShapes(t *testing.T) { "kari nordmann@example.no", // a space, most often a stray paste "kari@exa mple.no", "kari\t@example.no", + // Control characters. The address is handed to FusionAuth, which builds + // a password-setting email from it, so a line break in the middle of one + // is not merely a malformed address — it is a character with meaning to + // everything downstream. + "kari@exa\nmple.no", + "kari\r@example.no", + "kari@example.no\x00x", + "kari\x1b[31m@example.no", + "kari@example.no\x7f", + "kari\vnordmann@example.no", + // A separator that is not U+0020. TrimSpace removes these at the ends, + // so only one in the middle survives to be rejected here. + "kari nordmann@example.no", + "kari@eksem
pel.no", } for _, addr := range rejected { t.Run("rejected/"+addr, func(t *testing.T) { @@ -283,6 +297,57 @@ func TestEmailShapes(t *testing.T) { } } +// Every other field on this form has a ceiling; the address needs one too, +// because it is forwarded verbatim to FusionAuth, which sends a +// password-setting email to whatever it is given. 254 characters is as long as +// a deliverable address can be — RFC 5321's limit on the envelope path — so +// nothing a real member could use is refused. +func TestEmailLengthCeiling(t *testing.T) { + const domain = "@example.no" + fill := func(runes int) string { + return strings.Repeat("a", runes-len(domain)) + domain + } + + tests := []struct { + name string + addr string + valid bool + }{ + {"an address at the maximum", fill(emailMax), true}, + {"one character past the maximum", fill(emailMax + 1), false}, + {"the kind of length only a crafted request carries", fill(5011), false}, + // Counted in runes like every other field: an address made of Norwegian + // letters must not be refused at half the documented limit. + {"Norwegian letters are counted as runes, not bytes", + strings.Repeat("ø", emailMax-len(domain)) + domain, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := employeeForm() + f.Set("email", tt.addr) + + r, verr := FromForm(f, now) + msg := verr["email"] + if accepted := msg == ""; accepted != tt.valid { + t.Fatalf("an address of %d runes: accepted=%v (%q), want %v — an "+ + "unbounded address is forwarded to the service that mails it", + len([]rune(tt.addr)), accepted, msg, tt.valid) + } + if tt.valid { + return + } + if !strings.Contains(msg, "lengre enn 254") { + t.Errorf("message %q does not tell the member the limit", msg) + } + if r.Email != tt.addr { + t.Error("the rejected address was not echoed back unchanged; a " + + "truncated echo would silently rewrite what the member typed") + } + }) + } +} + // The address is stored exactly as typed. FusionAuth decides what counts as // the same account, and lowercasing here would mean the address in a member's // welcome email differs from the one they entered. diff --git a/internal/validate/validate.go b/internal/validate/validate.go index 08a0f29..758ff2a 100644 --- a/internal/validate/validate.go +++ b/internal/validate/validate.go @@ -18,6 +18,7 @@ package validate import ( "fmt" + "math" "sort" "strconv" "strings" @@ -120,7 +121,14 @@ func (e *Errors) Number(field, label, v string, min, max float64) float64 { // Accept the comma decimal separator; a Norwegian keyboard produces it and // rejecting it is a needless papercut. n, err := strconv.ParseFloat(strings.Replace(v, ",", ".", 1), 64) - if err != nil { + // "NaN", "Inf" and "-Inf" all parse, and the range check below cannot hold + // them: NaN compares false against both bounds, so it passes through + // untouched however narrow the range is. The only caller is the event + // duration, where NaN becomes an end time in the eighteenth century — the + // event reads as already finished and vanishes from the listing the moment + // it is saved. They are not numbers a form can mean, so they are refused + // with the same message as any other unparseable value. + if err != nil || math.IsNaN(n) || math.IsInf(n, 0) { e.Add(field, label+" må være et tall.") return 0 } diff --git a/internal/validate/validate_test.go b/internal/validate/validate_test.go index 236897b..66ce065 100644 --- a/internal/validate/validate_test.go +++ b/internal/validate/validate_test.go @@ -300,6 +300,43 @@ func TestNumber(t *testing.T) { " 3 ", 0, 100, 3, "", }, + { + // ParseFloat accepts "NaN", and NaN compares false against both + // bounds — so without an explicit check it passes every range, + // however narrow, and reaches whatever the caller does with it. + "NaN parses but is not a number the range check can hold", + "NaN", 0, 100, + 0, "Pris må være et tall.", + }, + { + "the spelling of NaN is not significant", + "nan", 0, 100, + 0, "Pris må være et tall.", + }, + { + // Infinity is caught by a ceiling but not by its absence, and it is + // not a quantity anybody can have typed on purpose. + "positive infinity is refused as not a number", + "Inf", 0, 100, + 0, "Pris må være et tall.", + }, + { + "negative infinity too", + "-Inf", 0, 100, + 0, "Pris må være et tall.", + }, + { + "and the long spelling", + "infinity", 0, 100, + 0, "Pris må være et tall.", + }, + { + // A number this large is finite, so it is a range problem rather than + // a parse one — the distinction is what keeps the message truthful. + "a huge but finite number is out of range, not unparseable", + "1e300", 0, 100, + 1e300, "Pris kan ikke være større enn 100.", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/web/discord.go b/internal/web/discord.go index 83ca191..50ffc98 100644 --- a/internal/web/discord.go +++ b/internal/web/discord.go @@ -1,10 +1,12 @@ package web import ( + "context" "crypto/rand" "encoding/base64" "errors" "net/http" + "time" "github.com/ItemizeNTNU/website/internal/auth" "github.com/ItemizeNTNU/website/internal/discord" @@ -14,6 +16,17 @@ import ( // discordStateCookie carries the OAuth state across the round trip to Discord. const discordStateCookie = "itemize_discord_state" +// discordTimeout bounds one trip through the linking flow. +// +// r.Context() carries no deadline of its own — http.Server's WriteTimeout does +// not cancel it — so without this the handler waits for however long Discord +// and FusionAuth between them decide to take. Each of these calls touches +// Discord two or three times plus FusionAuth twice, and thirty seconds is +// comfortably more than that needs while staying inside the server's +// sixty-second WriteTimeout. Past it the member sees an error and can press the +// button again, which is better than a goroutine held open indefinitely. +const discordTimeout = 30 * time.Second + // discordCallbackPath is registered with Discord as the redirect URI and must // match byte for byte. const discordCallbackPath = "/api/discord/callback" @@ -79,7 +92,10 @@ func (s *Server) discordCallback(w http.ResponseWriter, r *http.Request) { return } - link, err := s.discordSvc.Complete(r.Context(), user.ID, + ctx, cancel := context.WithTimeout(r.Context(), discordTimeout) + defer cancel() + + link, err := s.discordSvc.Complete(ctx, user.ID, r.URL.Query().Get("code"), s.discordRedirectURI()) switch { case errors.Is(err, discord.ErrDenied): @@ -109,7 +125,10 @@ func (s *Server) discordCallback(w http.ResponseWriter, r *http.Request) { func (s *Server) discordRefresh(w http.ResponseWriter, r *http.Request) { user := auth.FromRequest(r) - link, err := s.discordSvc.Refresh(r.Context(), user.ID) + ctx, cancel := context.WithTimeout(r.Context(), discordTimeout) + defer cancel() + + link, err := s.discordSvc.Refresh(ctx, user.ID) switch { case errors.Is(err, users.ErrNotLinked): SetFlash(w, "error", "Du har ingen Discord-konto koblet.") @@ -136,7 +155,10 @@ func (s *Server) discordRefresh(w http.ResponseWriter, r *http.Request) { func (s *Server) discordUnlink(w http.ResponseWriter, r *http.Request) { user := auth.FromRequest(r) - switch err := s.discordSvc.Unlink(r.Context(), user.ID); { + ctx, cancel := context.WithTimeout(r.Context(), discordTimeout) + defer cancel() + + switch err := s.discordSvc.Unlink(ctx, user.ID); { case errors.Is(err, users.ErrNotLinked): SetFlash(w, "error", "Du har ingen Discord-konto koblet.") case err != nil: diff --git a/internal/web/register.go b/internal/web/register.go index 6d4dbed..b4a6974 100644 --- a/internal/web/register.go +++ b/internal/web/register.go @@ -11,6 +11,13 @@ import ( "github.com/ItemizeNTNU/website/internal/validate" ) +// upstreamDownMessage is what somebody sees when the registration failed on +// our side of the form: no service address, no status code, and advice worth +// following — what they typed was fine. Kept word for word in step with the +// JSON path in internal/api/users.go so the same outage reads the same way +// whichever entry point a member came through. +const upstreamDownMessage = "Innloggingstjenesten svarer ikke akkurat nå. Prøv igjen om litt." + type registerView struct { Page Form map[string]string @@ -80,18 +87,22 @@ func (s *Server) submitRegistration(w http.ResponseWriter, r *http.Request) { _, err := s.fusion.CreateUser(r.Context(), reg.ToFusionAuth()) if err != nil { + // FusionAuth's parser wraps every non-2xx reply in *APIError, a 5xx + // included, so the status has to be checked as well. Without that an + // outage upstream came back as a 422 on the form, telling somebody whose + // details were fine to correct them — and hiding the outage from + // anything watching for 5xx. Same split as internal/api/users.go. var apiErr *fusionauth.APIError - switch { - case errors.As(err, &apiErr): + if errors.As(err, &apiErr) && apiErr.Status < http.StatusInternalServerError { // FusionAuth's own message is the useful one here — "email already // in use" is something the person can act on. view.Errors = validate.Errors{"": apiErr.UserMessage()} s.render(w, r, http.StatusUnprocessableEntity, "registrer", view) - default: - s.log.Error("creating the user failed", "err", err) - view.Errors = validate.Errors{"": "Ups. Noe gikk galt :/"} - s.render(w, r, http.StatusInternalServerError, "registrer", view) + return } + s.log.Error("creating the user failed", "err", err) + view.Errors = validate.Errors{"": upstreamDownMessage} + s.render(w, r, http.StatusInternalServerError, "registrer", view) return } diff --git a/internal/web/register_flow_test.go b/internal/web/register_flow_test.go index 2c0315b..f4e7893 100644 --- a/internal/web/register_flow_test.go +++ b/internal/web/register_flow_test.go @@ -108,26 +108,110 @@ func TestRegistrationSurfacesDuplicateEmail(t *testing.T) { } } -// Subtlety: an HTTP 500 with an empty body from FusionAuth still parses into -// an *APIError, so it takes the errors.As branch and renders as 422 with the -// UserMessage fallback — not the generic 500 page. -func TestRegistrationUpstream500RendersFallbackMessage(t *testing.T) { - fusion := fakeFusion(t, func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusInternalServerError) - }) - mux := newSite(t, siteConfig{fusion: fusion}) +// wantUpstreamDown is what the form says when the failure is ours rather than +// the visitor's: no service address, no status code, and advice worth +// following — what they typed was fine. Word for word the message the JSON +// path returns (internal/api/users.go), so one outage reads the same way +// whichever entry point a member came through. +const wantUpstreamDown = "Innloggingstjenesten svarer ikke akkurat nå. Prøv igjen om litt." - rec := postForm(t, mux, "/registrer", validRegistrationForm(), nil) - if rec.Code != http.StatusUnprocessableEntity { - t.Fatalf("got %d, want 422 — a non-2xx response is an APIError whatever its status", rec.Code) - } - if !contains(rec.Body.String(), "Uventet svar fra innloggingstjenesten (HTTP 500).") { - t.Error("the fallback message naming the upstream status is missing") +// 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 visitor whose registration was +// perfectly valid that it was not — a 422 on a form with nothing wrong with +// it, sometimes quoting an HTTP status at them — and hid the outage from +// anything watching for 5xx. +// +// Each case gets its own mux: five POSTs to /registrer per Server is the whole +// rate-limit allowance (webtest_test.go). +func TestRegistrationSeparatesOurFailuresFromTheVisitors(t *testing.T) { + const duplicate = `{"fieldErrors":{"user.email":[{"code":"[duplicate]","message":"E-posten er allerede i bruk."}]}}` + + for _, tc := range []struct { + name string + status int // what FusionAuth answers + body string + wantStatus int + wantMsg string + wantEcho bool // the typed values must come back to be corrected + }{ + { + name: "a rejection the visitor can act on", + status: http.StatusBadRequest, + body: duplicate, + wantStatus: http.StatusUnprocessableEntity, + wantMsg: "E-posten er allerede i bruk.", + wantEcho: true, + }, + { + name: "a conflict is still the visitor's to resolve", + status: http.StatusConflict, + body: duplicate, + wantStatus: http.StatusUnprocessableEntity, + wantMsg: "E-posten er allerede i bruk.", + wantEcho: true, + }, + { + // Before the fix this rendered as a 422 carrying "Uventet svar fra + // innloggingstjenesten (HTTP 500)." — a validation error that + // quotes an HTTP status at somebody who typed nothing wrong. + name: "the directory is broken", + status: http.StatusInternalServerError, + body: "", + wantStatus: http.StatusInternalServerError, + wantMsg: wantUpstreamDown, + }, + { + name: "the directory is restarting", + status: http.StatusServiceUnavailable, + body: "", + wantStatus: http.StatusInternalServerError, + wantMsg: wantUpstreamDown, + }, + { + name: "something in front of the directory is broken", + status: http.StatusBadGateway, + body: "", + wantStatus: http.StatusInternalServerError, + wantMsg: wantUpstreamDown, + }, + } { + t.Run(tc.name, func(t *testing.T) { + fusion := fakeFusion(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tc.status) + _, _ = w.Write([]byte(tc.body)) + }) + mux := newSite(t, siteConfig{fusion: fusion}) + + rec := postForm(t, mux, "/registrer", validRegistrationForm(), nil) + + if rec.Code != tc.wantStatus { + t.Fatalf("FusionAuth answered %d and the page came back %d, want %d", + tc.status, rec.Code, tc.wantStatus) + } + body := rec.Body.String() + if !contains(body, tc.wantMsg) { + t.Errorf("FusionAuth answered %d and the visitor is not told %q", + tc.status, tc.wantMsg) + } + if !tc.wantEcho { + // Nothing they typed was at fault, so nothing may suggest it was. + if contains(body, "Uventet svar fra innloggingstjenesten") { + t.Error("the outage is reported as an upstream HTTP status, which the visitor can do nothing with") + } + return + } + if !contains(body, `value="kari@example.no"`) { + t.Error("the rejected form came back empty, so the visitor retypes everything to fix one field") + } + }) } } -// A transport failure — connection refused, DNS, timeout — is our problem, -// and renders the generic 500 branch. +// A transport failure — connection refused, DNS, timeout — is upstream being +// unreachable rather than anything the visitor did, so it is reported the same +// way as a 5xx from FusionAuth: a 500, and an invitation to try again. func TestRegistrationTransportFailureIs500(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) fusion := fusionauth.New(srv.URL, "test-api-key") @@ -138,8 +222,8 @@ func TestRegistrationTransportFailureIs500(t *testing.T) { if rec.Code != http.StatusInternalServerError { t.Fatalf("got %d, want 500", rec.Code) } - if !contains(rec.Body.String(), "Ups. Noe gikk galt :/") { - t.Error("the visitor is not told something went wrong on our side") + if !contains(rec.Body.String(), wantUpstreamDown) { + t.Error("the visitor is not told the fault is ours and that retrying is worth it") } } From d797670c84238b74924b480e2a67ebc53843ecfe Mon Sep 17 00:00:00 2001 From: Rohaan Malik Mooken Date: Tue, 11 Aug 2026 17:06:45 +0200 Subject: [PATCH 10/11] =?UTF-8?q?Show=20the=20start=E2=80=93end=20span=20w?= =?UTF-8?q?hen=20an=20event=20has=20a=20duration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Events without a duration still render just the start time under "Når". Co-Authored-By: Claude Fable 5 --- internal/events/event.go | 10 ++++++++-- internal/events/event_test.go | 37 +++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/internal/events/event.go b/internal/events/event.go index 48eac95..f8424c3 100644 --- a/internal/events/event.go +++ b/internal/events/event.go @@ -95,8 +95,14 @@ func (e Event) HexID() string { // Past reports whether the event has finished. func (e Event) Past() bool { return !e.End.IsZero() && e.End.Before(time.Now()) } -// When renders the start time the way the site displays it. -func (e Event) When() string { return timefmt.Smart(e.Date) } +// When renders the time the way the site displays it: just the start when no +// duration is set, the start–end span when one is. +func (e Event) When() string { + if e.Duration > 0 { + return timefmt.Range(e.Date, e.ComputeEnd()) + } + return timefmt.Smart(e.Date) +} // WhenISO renders the start time for a machine — the datetime attribute on a //