diff --git a/assets/assets.go b/assets/assets.go index 8f9ac3ed98448718b4f9c6b8f80b85443735cd06..844ffd2609404288c588df1f1d2c7bcf2e9fdd50 100644 --- a/assets/assets.go +++ b/assets/assets.go @@ -43,6 +43,7 @@ import ( "fmt" "io/fs" "net/http" + "os" "path" "regexp" "strings" @@ -313,3 +314,25 @@ // overwritten, for the reason Handler gives. w.Header().Set("Cache-Control", w.cacheControl) w.Header().Del("Vary") } + +// DirFS is os.DirFS for a configured directory, and an empty filesystem for an +// unconfigured one. +// +// os.DirFS("") does not mean "this build ships no assets". It resolves every +// name against the filesystem root, so one unset config key turns a static +// handler into a reader of the host — reachable, on this instance, by leaving +// a single line out of config.ini. The guard is four lines that nobody writes +// until they have seen it happen, which is the argument for it living here. +func DirFS(dir string) fs.FS { + if dir == "" { + return emptyFS{} + } + return os.DirFS(dir) +} + +// emptyFS is a filesystem in which nothing exists. +type emptyFS struct{} + +func (emptyFS) Open(name string) (fs.File, error) { + return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrNotExist} +} diff --git a/assets/assets_test.go b/assets/assets_test.go index 40afe93d16cbb93259425f89ab7957768d8fba46..e57d2b707e4f4b2a2abd9ac067632db5a13b95d5 100644 --- a/assets/assets_test.go +++ b/assets/assets_test.go @@ -290,3 +290,24 @@ } { assert.Equal(t, want, assets.NormalizePrefix(given), given) } } + +// TestDirFSRefusesToServeTheFilesystemRoot pins the one-key hole: os.DirFS("") +// resolves every name against /, so an unset static directory would turn the +// handler into a reader of the host. +func TestDirFSRefusesToServeTheFilesystemRoot(t *testing.T) { + empty := assets.DirFS("") + _, err := fs.ReadFile(empty, "etc/passwd") + require.Error(t, err) + assert.ErrorIs(t, err, fs.ErrNotExist) + + names, err := fs.Glob(empty, "*") + require.NoError(t, err) + assert.Empty(t, names, "nothing exists in it, so a glob finds nothing") + + // A configured directory behaves as os.DirFS does. + dir := t.TempDir() + require.NoError(t, os.WriteFile(dir+"/main.min.0badc0de.css", []byte("body{}"), 0o644)) + got, err := fs.ReadFile(assets.DirFS(dir), "main.min.0badc0de.css") + require.NoError(t, err) + assert.Equal(t, "body{}", string(got)) +} diff --git a/bearer/status.go b/bearer/status.go index f92a6782dd3e369cefa73a311d0ccf06a0fb821b..dca20fe981b8bbdef523ccd0b2a1201bbd2a21af 100644 --- a/bearer/status.go +++ b/bearer/status.go @@ -48,6 +48,35 @@ return http.StatusUnauthorized } } +// IsRefusal reports whether err is one this package decided — that is, whether +// StatusFor's answer means anything for it. +// +// A service's own resolver returns more than bearer's vocabulary: the Postgres +// lookup it had to make, a context that expired, a bug. Handing those to +// StatusFor would answer 401 through its default arm, which is right for a +// credential and wrong for a database that did not answer — a caller told its +// token is bad re-mints a token that was never the problem. So a service that +// wraps this table guards the delegation: +// +// if bearer.IsRefusal(err) { +// http.Error(w, msg, bearer.StatusFor(err)) +// return +// } +// // anything else is ours, not the caller's +// +// Without this, each service spells out the sentinel list again, which is the +// five-line copy this package exists to stop. +func IsRefusal(err error) bool { + for _, sentinel := range []error{ + ErrInvalid, ErrNotOurs, ErrForbidden, ErrRevoked, ErrUnavailable, + } { + if errors.Is(err, sentinel) { + return true + } + } + return false +} + // Challenge is the WWW-Authenticate value a 401 carries: the scheme, and the // service's own config section as the realm. // diff --git a/bearer/status_test.go b/bearer/status_test.go index f5adaaad42187df0475782e9b3762a539f4766a5..d45b14a61c502ee76b9c8f703a2d53185fd83fb4 100644 --- a/bearer/status_test.go +++ b/bearer/status_test.go @@ -1,6 +1,7 @@ package bearer import ( + "context" "errors" "fmt" "net/http" @@ -29,6 +30,20 @@ assert.Equal(t, http.StatusOK, StatusFor(nil)) assert.Equal(t, http.StatusUnauthorized, StatusFor(errors.New("something new")), "an unrecognised failure refuses the request rather than declaring the service unwell") +} + +// TestIsRefusalSeparatesOurVocabularyFromTheServices is the guard a resolver +// needs: its own failures must not be answered as a bad credential. +func TestIsRefusalSeparatesOurVocabularyFromTheServices(t *testing.T) { + for _, err := range []error{ErrInvalid, ErrNotOurs, ErrForbidden, ErrRevoked, ErrUnavailable} { + assert.True(t, IsRefusal(err), "%v", err) + assert.True(t, IsRefusal(fmt.Errorf("validate the token: %w", err)), "wrapped %v", err) + } + + assert.False(t, IsRefusal(nil)) + assert.False(t, IsRefusal(errors.New("dial tcp 127.0.0.1:5432: connection refused")), + "a database that did not answer is not a refused credential") + assert.False(t, IsRefusal(context.DeadlineExceeded)) } func TestChallengeNamesTheServiceAndQuotesIt(t *testing.T) { diff --git a/instconf/instconf.go b/instconf/instconf.go index 8a48f86a3ff5996f691f4c3de314256bb973e69a..549a25c70fec545f3aabb429845da2742538f0e7 100644 --- a/instconf/instconf.go +++ b/instconf/instconf.go @@ -178,11 +178,13 @@ // returns "". // // It is the other half of a disagreement between two copies in one repo: a // Host-header check wants the name alone, because it compares against a request -// Host whose port it has already stripped, while a JWT audience, a sealed URL -// or a synthesized email domain wants the authority that actually identifies -// the endpoint — https://x:8080 and https://x:9090 are two audiences. Neither -// is the general answer, so both are here and the caller names which one it -// means. +// Host whose port it has already stripped, while a JWT audience or a sealed URL +// wants the authority that actually identifies the endpoint — https://x:8080 +// and https://x:9090 are two audiences. Neither is the general answer, so both +// are here and the caller names which one it means. +// +// A synthesized email domain is NOT one of these: an address is built from +// [OriginHost], because agent@localhost:5091 is not a mailbox. func OriginAuthority(origin string) string { u, err := url.Parse(CanonicalOrigin(origin)) if err != nil { @@ -197,10 +199,15 @@ // Key is one configuration requirement: a section, and the key names that // satisfy it. More than one name means an alternation — any of them will do — // which is how the internal API-origin ladder is expressed. Build one with -// [Need] or [NeedAny]. +// [Need] or [NeedAny], and add the reason with [Key.Because]. type Key struct { Section string Names []string + // Why is what the operator is told the key is for, e.g. "crypto.InitCrypto + // exits without it". It is optional, and it is the difference between a + // list of names and a message somebody can act on: three services kept + // their own hand-written checker rather than lose this sentence. + Why string } // Need is a requirement for one named key in a section. @@ -214,18 +221,31 @@ func NeedAny(section string, names ...string) Key { return Key{Section: section, Names: names} } +// Because attaches the reason the key is required, for the operator reading the +// refusal: Need("sr.ht", "network-key").Because("crypto.InitCrypto exits +// without it"). +func (k Key) Because(why string) Key { + k.Why = why + return k +} + // String renders the requirement the way an operator has to read it back into // config.ini: "[git.sr.ht] origin", or "[git.sr.ht] one of api-internal-origin, // internal-origin, api-origin, origin". func (k Key) String() string { + var named string switch len(k.Names) { case 0: - return fmt.Sprintf("[%s] ", k.Section) + named = fmt.Sprintf("[%s] ", k.Section) case 1: - return fmt.Sprintf("[%s] %s", k.Section, k.Names[0]) + named = fmt.Sprintf("[%s] %s", k.Section, k.Names[0]) default: - return fmt.Sprintf("[%s] one of %s", k.Section, strings.Join(k.Names, ", ")) + named = fmt.Sprintf("[%s] one of %s", k.Section, strings.Join(k.Names, ", ")) + } + if k.Why != "" { + return named + " — " + k.Why } + return named } // satisfied reports whether the config has a non-blank value for any of the diff --git a/logging/logging.go b/logging/logging.go index a98249d368b820676f69df6582cd026236a3c10b..f866996277c2212a514312498f5d47c3b47b6baa 100644 --- a/logging/logging.go +++ b/logging/logging.go @@ -223,8 +223,25 @@ // an operator passes -d to watch. A service that installs its logger before // loading config calls Defaults(nil, "") — -d and $LOG_LEVEL still resolve, and // the config file has nothing to say yet. func Defaults(conf ini.File, section string) Options { + return defaults(resolveLevel(conf, section, true)) +} + +// DefaultsWithoutDebugFlag is Defaults for a binary whose -d is not the +// daemon's. +// +// A migration CLI on this instance passes -d to brant, where it means +// --dialect and takes a value: `coversrht-migrate -d postgres` would otherwise +// arrive here as a request for debug logging, silently, because the probe sees +// the flag and never the value. $LOG_LEVEL and the config key still resolve — +// only the argument scan is dropped, which is the one source that cannot tell +// the two meanings apart. +func DefaultsWithoutDebugFlag(conf ini.File, section string) Options { + return defaults(resolveLevel(conf, section, false)) +} + +func defaults(level slog.Level) Options { return Options{ - Level: resolveLevel(conf, section), + Level: level, AddSource: true, Color: ColorEnabled(os.Stderr), TimeFormat: TimeFormat, @@ -235,8 +252,9 @@ } } // resolveLevel walks the three sources of verbosity in order of authority. -func resolveLevel(conf ini.File, section string) slog.Level { - if DebugRequested(os.Args[1:]) { +// debugFlag is false for a binary whose -d belongs to something else. +func resolveLevel(conf ini.File, section string, debugFlag bool) slog.Level { + if debugFlag && DebugRequested(os.Args[1:]) { return slog.LevelDebug } if level, ok := ParseLevel(os.Getenv(LevelEnv)); ok { diff --git a/logging/logging_test.go b/logging/logging_test.go index 920e62c3d968c847dc8cff47cddd18609aada5b2..5f9623a9e3a454417908a443bebce3a3a5d91e3c 100644 --- a/logging/logging_test.go +++ b/logging/logging_test.go @@ -357,3 +357,22 @@ func TestInstallRejectsANilHandler(t *testing.T) { assert.Panics(t, func() { Install(nil) }) } + +// TestDefaultsWithoutDebugFlagIgnoresTheArgument covers the migration CLI whose +// -d is brant's --dialect and takes a value: `-d postgres` must not arrive here +// as a request for debug logging. +func TestDefaultsWithoutDebugFlagIgnoresTheArgument(t *testing.T) { + saved := os.Args + os.Args = []string{"coversrht-migrate", "-d", "postgres"} + t.Cleanup(func() { os.Args = saved }) + t.Setenv(LevelEnv, "") + + assert.Equal(t, slog.LevelDebug, Defaults(nil, "").Level, + "the daemon's probe still sees -d") + assert.Equal(t, slog.LevelInfo, DefaultsWithoutDebugFlag(nil, "").Level, + "the CLI's -d is not ours") + + // The other two sources keep working for the CLI. + t.Setenv(LevelEnv, "warn") + assert.Equal(t, slog.LevelWarn, DefaultsWithoutDebugFlag(nil, "").Level) +} diff --git a/pages/error.go b/pages/error.go index 1ed4808c8ed1c50edc161220b5636f6dc6d98fae..21392dcd82abbe879f08950ce1795c3fe44942f3 100644 --- a/pages/error.go +++ b/pages/error.go @@ -55,6 +55,46 @@ ) // Message is the standard message for a status, or "" for a status that has // none — a 400 above all, whose message is the caller's own text. +// The machine-facing halves of the same table. A REST surface answers a caller +// that parses, not a person that reads, and every service on the instance keeps +// its 404 body byte-identical on purpose: two spellings of "not found" are two +// facts a client can accidentally distinguish, which is exactly what the shared +// status was chosen to prevent. +// +// They live beside the page sentences rather than in a second package because +// they are one table read two ways — and because the service that tried to +// reuse RenderRefusals on its REST surface could not, for want of these. +const ( + APINotFoundMessage = "not found" + APIUnauthorizedMessage = "unauthorized" + APIForbiddenMessage = "forbidden" + APIMethodMessage = "method not allowed" + APIInternalMessage = "internal server error" + APIUnavailableMessage = "service unavailable" +) + +// APIMessage is Message for a machine-facing surface: the same statuses, in the +// register a JSON client expects. An unmapped status returns "", which the +// caller renders as it likes — usually http.StatusText. +func APIMessage(status int) string { + switch status { + case http.StatusUnauthorized: + return APIUnauthorizedMessage + case http.StatusForbidden: + return APIForbiddenMessage + case http.StatusNotFound: + return APINotFoundMessage + case http.StatusMethodNotAllowed: + return APIMethodMessage + case http.StatusInternalServerError: + return APIInternalMessage + case http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout: + return APIUnavailableMessage + default: + return "" + } +} + func Message(status int) string { switch status { case http.StatusUnauthorized: diff --git a/pages/error_test.go b/pages/error_test.go new file mode 100644 index 0000000000000000000000000000000000000000..fb48eb2cc641793071bbe222b2e51381c963a55e --- /dev/null +++ b/pages/error_test.go @@ -0,0 +1,27 @@ +package pages + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestAPIMessageMirrorsMessage keeps the two registers on one table: a status +// that has a sentence for a reader must have a word for a parser, or a REST +// surface silently falls back to its own spelling. +func TestAPIMessageMirrorsMessage(t *testing.T) { + for _, status := range []int{ + http.StatusUnauthorized, http.StatusForbidden, http.StatusNotFound, + http.StatusMethodNotAllowed, http.StatusInternalServerError, + http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout, + } { + assert.NotEmpty(t, Message(status), "page sentence for %d", status) + assert.NotEmpty(t, APIMessage(status), "machine word for %d", status) + assert.NotEqual(t, Message(status), APIMessage(status), + "%d: a parser and a reader are not the same audience", status) + } + + assert.Empty(t, APIMessage(http.StatusTeapot)) + assert.Equal(t, "not found", APIMessage(http.StatusNotFound)) +}