From e963a67b6325b7b6a34747ac248aa1f4cd95cd32 Mon Sep 17 00:00:00 2001 From: Sylvester Damgaard Date: Thu, 27 Aug 2026 11:08:59 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20three=20fixes=20that=20only=20appeared?= =?UTF-8?q?=20to=20work=20=E2=80=94=20redaction,=20timezone,=20--watch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each of these shipped with a green test that asserted the wrong thing. The API redacted secrets by name, and the exemption for frontend-published variables (VITE_, NEXT_PUBLIC_, MIX_, REACT_APP_, …) returned BEFORE the secret-word check — the opposite of what the comment directly above it claimed. GET /api/v1/processes/{name} therefore served VITE_DB_PASSWORD, NEXT_PUBLIC_API_SECRET and MIX_PUSHER_APP_SECRET in cleartext. Naming a password VITE_DB_PASSWORD is a mistake; publishing its value is not the way to point that out. The exemption now yields to an unambiguous secret word — password, secret, token, private_key, passphrase, salt — while still ignoring the merely key-ish ones a genuinely publishable key carries, so NEXT_PUBLIC_API_KEY, VITE_API_URL and STRIPE_PUBLISHABLE_KEY stay readable. schedule_timezone never took effect. NewScheduledJobWithOptions built the CRON_TZ-prefixed spec correctly and stored the parsed schedule on the job — and then the scheduler registered the RAW expression with cron.AddJob, which re-parsed it in the cron instance's own default location. The parsed schedule was computed, stored, and used nowhere. A job configured for America/New_York fired at 03:00 UTC instead of 08:00. The existing test was green because it asserted on that dead field; the new one goes through the Scheduler and reads back the entry the cron library will actually fire. --watch was a no-op for a symlinked config on Linux. filepath.Abs does not resolve symlinks, so the directory watch landed on the LINK's directory, where nothing ever changes — the shape you get from /etc/cbox-init/cbox-init.yaml pointing into a mounted volume, or from a Kubernetes ConfigMap's ..data indirection. Both directories are now watched, since which one sees the event depends on the shape: a plain symlink changes at the target, while a ConfigMap update swaps the ..data link beside the link itself. macOS's kqueue backend follows the symlink and hid this entirely, so the test asserts the watch list structurally as well as behaviourally. --- internal/api/server.go | 22 +++++- internal/api/server_test.go | 12 ++++ internal/schedule/job.go | 7 ++ internal/schedule/scheduler.go | 14 ++-- internal/schedule/timezone_test.go | 57 ++++++++++++++++ internal/watcher/symlink_test.go | 106 +++++++++++++++++++++++++++++ internal/watcher/watcher.go | 60 +++++++++++++--- 7 files changed, 261 insertions(+), 17 deletions(-) create mode 100644 internal/watcher/symlink_test.go diff --git a/internal/api/server.go b/internal/api/server.go index c6d54f1..b0da785 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -1193,6 +1193,14 @@ const RedactedValue = "***REDACTED***" var secretEnvKeyPattern = regexp.MustCompile( `(?i)(password|passwd|secret|secret[_-]?key|token|credentials?|apikey|api[_-]?key|access[_-]?key|private[_-]?key|auth[_-]?config|erlang[_-]?cookie|webhook|dsn|salt|passphrase)([^a-z]|$)`) +// strongSecretKeyPattern is the subset of secret words that mean "secret" no +// matter what prefix the name carries. It deliberately omits the key-ish +// alternatives (apikey, api_key, access_key) that a genuinely publishable key +// legitimately contains, so NEXT_PUBLIC_API_KEY stays readable while +// NEXT_PUBLIC_API_SECRET does not. +var strongSecretKeyPattern = regexp.MustCompile( + `(?i)(password|passwd|secret|token|credentials?|private[_-]?key|passphrase|salt|erlang[_-]?cookie|dsn)([^a-z]|$)`) + // shortSecretKeyPattern covers the abbreviated forms (DB_PASS, MYSQL_PWD). // These need a boundary on BOTH sides, or COMPASS_DIR and PASSENGER_ROOT would // be masked. @@ -1245,9 +1253,19 @@ func shouldRedactEnv(key, value string) bool { if credentialURLKeyPattern.MatchString(key) && urlHasCredentials(value) { return true } - // Framework-published variables can't hold a server-side secret. + // A framework-published variable is compiled into the browser bundle, so + // NEXT_PUBLIC_API_KEY and VITE_API_URL are public by construction and stay + // readable — that is what this exemption is for. + // + // But it used to exempt the name unconditionally, ahead of the secret-word + // check below, so VITE_DB_PASSWORD and NEXT_PUBLIC_API_SECRET were served in + // cleartext. Naming a password VITE_DB_PASSWORD is a mistake; publishing its + // value is not the way to point that out. So the exemption now yields to an + // unambiguous secret word — "password", "secret", "token", "private_key" — + // while still ignoring the merely key-ish ones a public key legitimately + // carries. if frontendPublicPattern.MatchString(key) { - return false + return strongSecretKeyPattern.MatchString(key) || shortSecretKeyPattern.MatchString(key) } // An explicit secret word always wins, even alongside "public"/"site" — // RECAPTCHA_SITE_SECRET and SITE_PRIVATE_KEY are secrets. diff --git a/internal/api/server_test.go b/internal/api/server_test.go index 5d8537d..64f4ac8 100644 --- a/internal/api/server_test.go +++ b/internal/api/server_test.go @@ -4916,6 +4916,13 @@ func TestShouldRedactEnv(t *testing.T) { "RABBITMQ_ERLANG_COOKIE": "x", "ERLANG_COOKIE": "x", // "public"/"site" must not exempt a name that also says secret. "RECAPTCHA_SITE_SECRET": "x", "SITE_PRIVATE_KEY": "x", "PUBLIC_SECRET_KEY": "x", + // Nor must a frontend prefix. The exemption for VITE_/NEXT_PUBLIC_/MIX_ + // used to return BEFORE the secret-word check, so these were served in + // cleartext. Naming a password VITE_DB_PASSWORD is a mistake; publishing + // its value is not the way to point that out. + "VITE_DB_PASSWORD": "x", "NEXT_PUBLIC_API_SECRET": "x", + "MIX_PUSHER_APP_SECRET": "x", "VITE_JWT_SECRET": "x", + "REACT_APP_PRIVATE_KEY": "x", "NUXT_PUBLIC_DB_PASS": "x", // Compact forms, and credential-bearing URLs whose name would otherwise // be exempted as public/frontend. "DBPASS": "x", "SMTPPASS": "x", @@ -4939,6 +4946,11 @@ func TestShouldRedactEnv(t *testing.T) { // Explicitly public values must stay visible even though they contain "key". "PUBLIC_KEY": "pk", "VAPID_PUBLIC_KEY": "pk", "STRIPE_PUBLISHABLE_KEY": "pk_live_x", "RECAPTCHA_SITE_KEY": "6Lx", "MIX_PUSHER_APP_KEY": "abc", "NEXT_PUBLIC_API_KEY": "abc", + // Frontend-published variables are compiled into the browser bundle, so + // the key-ish words a publishable key legitimately carries must not + // trigger redaction — only an unambiguous secret word does. + "VITE_API_URL": "https://api.example.com", "NEXT_PUBLIC_STRIPE_KEY": "pk_live_x", + "VITE_APP_NAME": "shop", "REACT_APP_API_KEY": "abc", "NUXT_PUBLIC_SITE_URL": "https://x.dev", } for k, v := range secret { if !shouldRedactEnv(k, v) { diff --git a/internal/schedule/job.go b/internal/schedule/job.go index af8bd14..dfd2b29 100644 --- a/internal/schedule/job.go +++ b/internal/schedule/job.go @@ -186,6 +186,13 @@ func NewScheduledJobWithOptions(name, scheduleExpr, timezone string, historySize }, nil } +// CronSchedule returns the parsed schedule, which carries the job's configured +// timezone. The scheduler must register THIS rather than re-parsing the raw +// expression, or the timezone is silently dropped. +func (j *ScheduledJob) CronSchedule() cron.Schedule { + return j.schedule +} + // GetState returns the current job state (thread-safe) func (j *ScheduledJob) GetState() JobState { j.mu.Lock() diff --git a/internal/schedule/scheduler.go b/internal/schedule/scheduler.go index ae28c86..0d9fd30 100644 --- a/internal/schedule/scheduler.go +++ b/internal/schedule/scheduler.go @@ -81,11 +81,15 @@ func (s *Scheduler) AddJobWithOptions(name, scheduleExpr, timezone string, opts return fmt.Errorf("failed to create job: %w", err) } - // Add to cron scheduler - entryID, err := s.cron.AddJob(scheduleExpr, job) - if err != nil { - return fmt.Errorf("failed to add job to cron: %w", err) - } + // Register the job's ALREADY-PARSED schedule, not the raw expression. + // + // cron.AddJob re-parses the string with the cron instance's own default + // location, which threw away the CRON_TZ binding that + // NewScheduledJobWithOptions had just built — so schedule_timezone had no + // effect whatever and every nightly job still fired at the container's local + // time. The parsed schedule was computed, stored on the job, and used + // nowhere. + entryID := s.cron.Schedule(job.CronSchedule(), job) job.SetCronID(entryID) // If the scheduler is already running, hand the new job the live run context diff --git a/internal/schedule/timezone_test.go b/internal/schedule/timezone_test.go index 36de4e3..ce2630b 100644 --- a/internal/schedule/timezone_test.go +++ b/internal/schedule/timezone_test.go @@ -47,3 +47,60 @@ func TestScheduledJob_RejectsUnknownTimezone(t *testing.T) { t.Error("an unknown timezone must be rejected") } } + +// TestSchedulerRegistersTheTimezoneAwareSchedule is the test the one above +// should have been. +// +// TestScheduledJob_HonorsTimezone asserts on job.schedule — the parsed, +// CRON_TZ-bound schedule that NewScheduledJobWithOptions builds. But the +// Scheduler registered the RAW expression with cron.AddJob, which re-parsed it +// in the cron instance's own default location, so job.schedule was dead code and +// schedule_timezone had no effect at all. The test was green against a field +// nothing used. This one goes through the Scheduler and reads back the entry the +// cron library will actually fire. +func TestSchedulerRegistersTheTimezoneAwareSchedule(t *testing.T) { + lg := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + + // 1 March 2026 is inside EST (UTC-5), so 03:00 New York is 08:00 UTC. + base := time.Date(2026, 3, 1, 0, 0, 0, 0, time.UTC) + + for _, c := range []struct { + timezone string + wantUTC int + }{ + {"UTC", 3}, + {"America/New_York", 8}, + } { + t.Run(c.timezone, func(t *testing.T) { + s := NewScheduler(nil, 10, lg) + if err := s.AddJob("nightly", "0 3 * * *", c.timezone); err != nil { + t.Fatalf("AddJob: %v", err) + } + + job, ok := s.GetJob("nightly") + if !ok { + t.Fatal("job not registered") + } + + entries := s.cron.Entries() + if len(entries) != 1 { + t.Fatalf("expected 1 cron entry, got %d", len(entries)) + } + + // This is the schedule the cron library will actually use. + next := entries[0].Schedule.Next(base).UTC() + t.Logf("timezone %s -> cron entry fires at %s", c.timezone, next.Format(time.RFC3339)) + + if next.Hour() != c.wantUTC { + t.Errorf("the registered cron entry fires at %02d:00 UTC, want %02d:00; "+ + "schedule_timezone %q never reached the scheduler", + next.Hour(), c.wantUTC, c.timezone) + } + + // And the entry agrees with the job's own parsed schedule. + if want := job.CronSchedule().Next(base).UTC(); !next.Equal(want) { + t.Errorf("cron entry (%s) and job schedule (%s) disagree", next, want) + } + }) + } +} diff --git a/internal/watcher/symlink_test.go b/internal/watcher/symlink_test.go new file mode 100644 index 0000000..77b42b3 --- /dev/null +++ b/internal/watcher/symlink_test.go @@ -0,0 +1,106 @@ +package watcher + +import ( + "context" + "log/slog" + "os" + "path/filepath" + "testing" + "time" +) + +// TestWatchFollowsSymlinkedConfig: filepath.Abs does not resolve symlinks, so a +// config reached through a link — /etc/cbox-init/cbox-init.yaml pointing at +// /data/config/app.yaml, or a Kubernetes ConfigMap's ..data indirection — put +// the directory watch on the LINK's directory, where nothing ever changes. +// --watch became a silent no-op on Linux. (On macOS the file watch happened to +// survive, which is why it never showed up locally.) +func TestWatchFollowsSymlinkedConfig(t *testing.T) { + root := t.TempDir() + + // The real file lives in one directory... + dataDir := filepath.Join(root, "data") + if err := os.Mkdir(dataDir, 0o755); err != nil { + t.Fatal(err) + } + target := filepath.Join(dataDir, "app.yaml") + if err := os.WriteFile(target, []byte("version: \"1.0\"\n"), 0o600); err != nil { + t.Fatal(err) + } + + // ...and is reached through a link in another. + linkDir := filepath.Join(root, "etc") + if err := os.Mkdir(linkDir, 0o755); err != nil { + t.Fatal(err) + } + link := filepath.Join(linkDir, "cbox-init.yaml") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + reloaded := make(chan struct{}, 4) + w, err := New(Config{ + ConfigPath: link, + Debounce: 30 * time.Millisecond, + Logger: slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), + Handler: func() error { + select { + case reloaded <- struct{}{}: + default: + } + return nil + }, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if err := w.Start(ctx); err != nil { + t.Fatalf("Start: %v", err) + } + defer func() { _ = w.Stop() }() + + // Give the watch a moment to be established. + time.Sleep(100 * time.Millisecond) + + // Write through the TARGET, as an editor or a config generator would. + if err := os.WriteFile(target, []byte("version: \"1.0\"\n# edited\n"), 0o600); err != nil { + t.Fatal(err) + } + + select { + case <-reloaded: + case <-time.After(3 * time.Second): + t.Fatal("editing the symlink's target triggered no reload; " + + "--watch is a no-op for a linked config") + } + + // The behavioural check above only fails on Linux: macOS's kqueue backend + // follows the symlink when it registers the directory's entries, so the bug + // is invisible there. Assert the structure directly as well, so this test + // means something on every platform. + watched := w.watcher.WatchList() + if !containsPath(watched, dataDir) { + t.Errorf("the resolved target's directory %s is not watched (watching %v); "+ + "on Linux inotify this makes --watch a no-op", dataDir, watched) + } + if !containsPath(watched, linkDir) { + t.Errorf("the link's own directory %s is not watched (watching %v); "+ + "a Kubernetes ConfigMap update swaps the ..data link there", linkDir, watched) + } +} + +func containsPath(list []string, want string) bool { + want, err := filepath.EvalSymlinks(want) + if err != nil { + return false + } + for _, got := range list { + if resolved, err := filepath.EvalSymlinks(got); err == nil && resolved == want { + return true + } + } + return false +} diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 9e3e057..478bcfc 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "runtime/debug" + "strings" "sync" "time" @@ -18,7 +19,10 @@ type ReloadHandler func() error // Watcher watches configuration files for changes and triggers reload type Watcher struct { - configPath string + configPath string + // resolvedPath is configPath with symlinks resolved; the two differ for a + // linked config, and both directories have to be watched. + resolvedPath string debounceTimer *time.Timer handler ReloadHandler logger *slog.Logger @@ -70,12 +74,26 @@ func New(cfg Config) (*Watcher, error) { return nil, fmt.Errorf("failed to get absolute path: %w", err) } + // Resolve symlinks. filepath.Abs does not, so a config reached through a + // link — /etc/cbox-init/cbox-init.yaml -> /data/config/app.yaml, or the + // ..data indirection a Kubernetes ConfigMap mount uses — put the directory + // watch on the LINK's directory, where nothing ever changes. --watch then + // silently did nothing on Linux. Both directories are watched below, since + // which one sees the event depends on which shape it is: a plain symlink + // changes at the target, while a ConfigMap update swaps the ..data link + // beside the link itself. + resolvedPath := absPath + if resolved, rerr := filepath.EvalSymlinks(absPath); rerr == nil { + resolvedPath = filepath.Clean(resolved) + } + w := &Watcher{ - configPath: absPath, - handler: cfg.Handler, - logger: cfg.Logger, - watcher: fsWatcher, - debounce: cfg.Debounce, + configPath: absPath, + resolvedPath: resolvedPath, + handler: cfg.Handler, + logger: cfg.Logger, + watcher: fsWatcher, + debounce: cfg.Debounce, } return w, nil @@ -98,8 +116,15 @@ func (w *Watcher) Start(ctx context.Context) error { if _, err := os.Stat(w.configPath); err != nil { return fmt.Errorf("failed to watch config file: %w", err) } - if err := w.watcher.Add(filepath.Dir(w.configPath)); err != nil { - return fmt.Errorf("failed to watch config directory: %w", err) + watched := map[string]bool{} + for _, dir := range []string{filepath.Dir(w.configPath), filepath.Dir(w.resolvedPath)} { + if watched[dir] { + continue + } + if err := w.watcher.Add(dir); err != nil { + return fmt.Errorf("failed to watch config directory %s: %w", dir, err) + } + watched[dir] = true } w.logger.Info("Config watcher started", @@ -125,8 +150,8 @@ func (w *Watcher) watchLoop(ctx context.Context) { return } - // The watch is on the directory, so filter to our own file. - if filepath.Clean(event.Name) != w.configPath { + // The watch is on the directory (or two), so filter to our own file. + if !w.isOurs(event.Name) { continue } @@ -147,6 +172,21 @@ func (w *Watcher) watchLoop(ctx context.Context) { } } +// isOurs reports whether a directory event concerns the config we watch. +// +// It matches the configured path, the symlink-resolved path, and the "..data" +// indirection a Kubernetes ConfigMap mount swaps on update — that rename is the +// only event a ConfigMap change produces in the mount directory, and matching +// only the file names would miss it entirely. +func (w *Watcher) isOurs(name string) bool { + clean := filepath.Clean(name) + if clean == w.configPath || clean == w.resolvedPath { + return true + } + + return strings.HasPrefix(filepath.Base(clean), "..") +} + // handleFileChange processes a file change event with trailing-edge debouncing. // // Trailing edge matters: reloading on the FIRST event of a burst and ignoring