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