feat(middlewares): warn when a local preset file shadows a bundled name (#679) (#696)

## Summary

\`PresetLoader.Load\` resolves bundled presets first and never falls
through, so a file at \`\$LOCAL_DIR/json-post.yaml\` placed hoping to
override the bundled \`json-post\` is silently ignored at attach time.
Same applies to every bundled name: \`slack\`, \`discord\`, \`teams\`,
\`matrix\`, \`ntfy\`, \`ntfy-token\`, \`gotify\`, \`pushover\`,
\`pagerduty\`.

\`AddLocalPresetDir\` now scans the registered directory for \`*.yaml\`
/ \`*.yml\` files whose stem collides with a bundled preset name and
emits a startup \`slog.Warn\` per match. Missing or unreadable
directories stay silent (operators commonly register a dir before
populating it).

Closes [#679](#679).

## Why not invert the lookup order

The issue body called this out explicitly: a local typo accidentally
shadowing \`slack.yaml\` would silently break Slack delivery for *every*
webhook on the host. Operators trust the bundled set to behave
consistently across hosts; warn-and-keep-bundled preserves that contract
while surfacing the override gap.

## Tests

Two new tests in \`middlewares/preset_test.go\` (reusing the existing
\`captureSlog\` helper from \`webhook_security_warn_test.go\`):

1. **\`TestPresetLoader_AddLocalPresetDir_WarnsOnBundledShadow\`** —
temp dir with \`json-post.yaml\` + \`slack.yml\` + \`my-custom.yaml\` +
\`json-post.txt\`, asserts warnings for the two bundled collisions but
NOT for the non-colliding file or the non-yaml file. Covers both
\`.yaml\` and \`.yml\` extensions.
2. **\`TestPresetLoader_AddLocalPresetDir_NoWarnOnMissingDir\`** —
registering a directory that doesn'"'"'t exist is silent, preserving the
existing \`/tmp/presets\` fixture behavior.

## Docs

New "**Preset Lookup Order**" subsection in \`docs/webhooks.md\`
documents the bundled-first precedence and rationale.

## CHANGELOG

Entry under \`[Unreleased]\` → \`### Added\`.

## Test plan

- [x] \`go test ./...\` passes (full repo, 14 packages, ~58s)
- [x] \`golangci-lint run\` clean
- [x] \`go vet ./...\` clean
- [ ] CI green

## References

- Surfaced in: [#677](#677)
(parallel-reviewer pass)

Author: CybotTM

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- `PresetLoader.AddLocalPresetDir` now scans the registered directory for `*.yaml` files whose stem collides with a bundled preset name (`slack`, `discord`, `teams`, `matrix`, `ntfy`, `ntfy-token`, `pushover`, `pagerduty`, `gotify`, `json-post`) and emits a startup `slog.Warn` per collision. Pre-fix, `PresetLoader.Load` resolved bundled presets first and never fell through, so a file at `$LOCAL_DIR/json-post.yaml` placed hoping to override the bundled `json-post` was silently ignored at attach time. The warning matches `Load`'s `.yaml`-only resolution path so a `.yml` rename suggestion never misleads operators. The lookup order is documented in `docs/webhooks.md` under "Preset Lookup Order"; inverting the order to prefer local files is deliberately rejected (a local typo shadowing `slack.yaml` would silently break Slack delivery host-wide). Closes [#679](https://github.com/netresearch/ofelia/issues/679).
+
 ### Removed
 
 - **BREAKING (source-only, pre-1.0):** Removed unused `core/adapters/docker.ClientConfig.HTTPClient` field that was declared in [#681](https://github.com/netresearch/ofelia/pull/681) but never read — a caller setting `cfg.HTTPClient = someClient` silently got the auto-constructed transport instead of theirs. Downstream Go consumers that referenced the field in named struct literals or assignments will see a compile-time error after upgrade (semantically a no-op since the field was already ignored at runtime); permitted under SemVer for the current 0.y.z line (cf. [SemVer §4](https://semver.org/#spec-item-4)). Removing the field turns the silent footgun into a loud compile-time error rather than preserving it as a deprecated no-op. If you need a transport-level injection seam, file a feature request with the use case so the suppression of `disableHTTP2AutoConfig` on caller-supplied transports (the [#668](https://github.com/netresearch/ofelia/issues/668) invariant) can be wired in correctly. ([#693](https://github.com/netresearch/ofelia/pull/693), closes [#684](https://github.com/netresearch/ofelia/issues/684))

docs/webhooks.md

@@ -618,6 +618,18 @@ Templates support these helper functions:
 
 ## Creating Custom Presets
 
+### Preset Lookup Order
+
+`PresetLoader.Load` resolves a preset name against these sources, in order, and stops at the first hit:
+
+1. **Bundled presets** compiled into the binary (`slack`, `discord`, `teams`, `matrix`, `ntfy`, `ntfy-token`, `pushover`, `pagerduty`, `gotify`, `json-post`).
+2. **Path-prefixed local files** — preset names starting with `/`, `./`, or `../` are read from disk directly.
+3. **Local preset directories** — registered via `(*PresetLoader).AddLocalPresetDir(dir)`; `<dir>/<name>.yaml` lookup.
+4. **GitHub shorthand** — `gh:owner/repo/path/file.yaml@ref`.
+5. **Remote URLs** — `https://...`/`http://...` (requires `webhook-allow-remote-presets = true` AND a matching entry in `webhook-trusted-preset-sources`).
+
+> **Bundled wins:** a file at `<local-preset-dir>/json-post.yaml` will **not** override the bundled `json-post` preset — the bundled lookup at step 1 fires first and never falls through. To make collisions visible, Ofelia emits a startup `slog.Warn` per local `.yaml` file whose stem matches a bundled preset name. Rename the local file (e.g. `my-json-post.yaml`) to use it via step 3. This precedence is intentional: inverting it would let a local typo (e.g. an accidentally-saved `slack.yaml`) silently break Slack delivery for every webhook on the host. See [#679](https://github.com/netresearch/ofelia/issues/679).
+
 Custom presets use YAML format:
 
 ```yaml

middlewares/preset.go

@@ -8,7 +8,10 @@ import (
 	"context"
 	"embed"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"io/fs"
+	"log/slog"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -140,9 +143,71 @@ func (l *PresetLoader) loadBundledPresets() error {
 	return nil
 }
 
-// AddLocalPresetDir adds a directory to search for local preset files
+// AddLocalPresetDir adds a directory to search for local preset files. It
+// also scans the directory for files whose names collide with a bundled
+// preset (e.g. "json-post.yaml" shadowing the bundled "json-post") and emits
+// a startup-time slog.Warn per collision. Load() resolves bundled presets
+// first and never falls through, so a local file at $dir/<name>.yaml meant
+// to override the bundled preset would silently be ignored — surfacing the
+// shadow at AddLocalPresetDir time turns a silent no-op into an actionable
+// warning. See https://github.com/netresearch/ofelia/issues/679.
+//
+// Inverting the lookup order to prefer local files is deliberately rejected:
+// a local typo that accidentally shadows "slack" would silently break Slack
+// delivery for every webhook on the host. Operators trust the bundled set
+// to behave consistently across hosts; warn-and-keep-bundled preserves that.
 func (l *PresetLoader) AddLocalPresetDir(dir string) {
 	l.localPresetDirs = append(l.localPresetDirs, dir)
+	l.warnOnBundledShadow(dir)
+}
+
+// warnOnBundledShadow scans dir for *.yaml files whose stem matches a
+// bundled preset name and emits one slog.Warn per collision. Restricted to
+// the *.yaml extension on purpose: Load() only probes "<name>.yaml" in
+// localPresetDirs, so a *.yml file would not have been loaded anyway and
+// warning on it would mislead operators into thinking the rename to .yaml
+// is sufficient.
+//
+// A non-existent directory (operator registered before populating) is
+// silent. Other read failures are debug-logged so a permission-denied or
+// I/O error is still discoverable when an operator turns up the log level,
+// without becoming a startup-noise floor on a clean install.
+func (l *PresetLoader) warnOnBundledShadow(dir string) {
+	if len(l.bundledPresets) == 0 {
+		return
+	}
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		if !errors.Is(err, fs.ErrNotExist) {
+			slog.Default().Debug(
+				"PresetLoader: could not scan local preset dir for bundled-name shadows",
+				"dir", dir,
+				"error", err,
+			)
+		}
+		return
+	}
+	for _, entry := range entries {
+		if entry.IsDir() {
+			continue
+		}
+		name := entry.Name()
+		if filepath.Ext(name) != ".yaml" {
+			continue
+		}
+		stem := strings.TrimSuffix(name, ".yaml")
+		if _, shadowed := l.bundledPresets[stem]; !shadowed {
+			continue
+		}
+		fullPath := filepath.Join(dir, name)
+		slog.Default().Warn(
+			fmt.Sprintf("Local preset file %q shadows bundled preset %q; the bundled preset wins "+
+				"and the local file will not be loaded. Rename the file or remove it to silence this warning.",
+				fullPath, stem),
+			"file", fullPath,
+			"bundled", stem,
+		)
+	}
 }
 
 // DefaultPreset returns the effective global default preset name —

middlewares/preset_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -324,6 +325,98 @@ func TestPresetLoader_AddLocalPresetDir(t *testing.T) {
 	assert.Len(t, loader.localPresetDirs, 2)
 }
 
+// TestPresetLoader_AddLocalPresetDir_WarnsOnBundledShadow pins the fix for
+// https://github.com/netresearch/ofelia/issues/679. PresetLoader.Load resolves
+// bundled presets first and never falls through, so a local file at
+// $dir/<bundled-name>.yaml is a silent no-op. AddLocalPresetDir now scans the
+// directory and emits a slog.Warn per collision so operators learn at startup
+// that their override won't take effect.
+//
+// Scope is restricted to *.yaml to mirror Load's resolution path: Load only
+// probes "<name>.yaml" in localPresetDirs, so *.yml files would not have
+// been loaded anyway and warning on them would mislead operators into
+// thinking a .yml -> .yaml rename is the fix. The .yml anti-warning case
+// pins that contract.
+//
+// Not parallel: shares slog.Default with other captureSlog tests in the
+// package (see the helper's comment).
+func TestPresetLoader_AddLocalPresetDir_WarnsOnBundledShadow(t *testing.T) {
+	dir := t.TempDir()
+
+	// json-post is a bundled preset (since #676) — placing a local file
+	// with the same stem and .yaml extension must trigger the shadow
+	// warning.
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "json-post.yaml"), []byte("name: json-post\n"), 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "slack.yaml"), []byte("name: slack\n"), 0o644))
+	// Non-colliding file must NOT trigger a warning.
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "my-custom.yaml"), []byte("name: my-custom\n"), 0o644))
+	// .yml extension: Load only probes .yaml, so warning on .yml would
+	// mislead operators. This file's bundled stem must NOT trigger a warning.
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "discord.yml"), []byte("name: discord\n"), 0o644))
+	// Non-yaml file must be ignored entirely.
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "json-post.txt"), []byte("not yaml"), 0o644))
+
+	buf := captureSlog(t)
+	loader := NewPresetLoader(nil)
+	loader.AddLocalPresetDir(dir)
+
+	logs := buf.String()
+	assert.Contains(t, logs, "shadows bundled preset \\\"json-post\\\"",
+		"should warn that json-post.yaml shadows the bundled json-post preset")
+	assert.Contains(t, logs, "shadows bundled preset \\\"slack\\\"",
+		"should warn that slack.yaml shadows the bundled slack preset")
+	assert.Contains(t, logs, `"level":"WARN"`,
+		"shadow notices must use WARN level so operators don't have to raise log verbosity to see them")
+	assert.NotContains(t, logs, "my-custom",
+		"should NOT warn about non-colliding local presets")
+	assert.NotContains(t, logs, "discord",
+		"should NOT warn about .yml files — Load only probes .yaml, so .yml is not a shadow case")
+	assert.NotContains(t, logs, "json-post.txt",
+		"should NOT scan non-yaml files")
+}
+
+// TestPresetLoader_AddLocalPresetDir_NoWarnOnMissingDir verifies that
+// registering a directory that does not exist (yet) is silent. Operators
+// frequently configure the directory before populating it, and the existing
+// /tmp/presets fixture above relies on this behavior.
+func TestPresetLoader_AddLocalPresetDir_NoWarnOnMissingDir(t *testing.T) {
+	buf := captureSlog(t)
+	loader := NewPresetLoader(nil)
+	loader.AddLocalPresetDir(filepath.Join(t.TempDir(), "does-not-exist"))
+	assert.NotContains(t, buf.String(), "shadows bundled preset",
+		"missing directory must not produce a shadow warning")
+	assert.NotContains(t, buf.String(), "could not scan local preset dir",
+		"fs.ErrNotExist must stay silent (no debug log either) — operators register dirs before populating")
+}
+
+// TestPresetLoader_AddLocalPresetDir_DebugLogsNonENOENTError pins the debug-
+// log branch added in response to PR #696 review: read failures other than
+// "directory does not exist" (e.g. operator pointed at a file, permission
+// denied) are debug-logged so they surface when log level is raised, instead
+// of being silently swallowed alongside the legitimate not-yet-populated
+// case.
+//
+// We trigger ENOTDIR by registering a path that points at a regular file
+// rather than a directory — os.ReadDir then returns a non-ENOENT error,
+// which is exactly the branch we want to exercise.
+func TestPresetLoader_AddLocalPresetDir_DebugLogsNonENOENTError(t *testing.T) {
+	dir := t.TempDir()
+	notADir := filepath.Join(dir, "regular-file")
+	require.NoError(t, os.WriteFile(notADir, []byte("just a file"), 0o644))
+
+	buf := captureSlog(t)
+	loader := NewPresetLoader(nil)
+	loader.AddLocalPresetDir(notADir)
+
+	logs := buf.String()
+	assert.Contains(t, logs, "could not scan local preset dir",
+		"non-ENOENT errors must surface as a debug log (e.g. ENOTDIR when path is a regular file)")
+	assert.Contains(t, logs, `"level":"DEBUG"`,
+		"non-ENOENT errors are debug-level, not warn — they're operator-recoverable misconfig, not a startup gate")
+	assert.NotContains(t, logs, "shadows bundled preset",
+		"failure to scan must not produce a shadow warning")
+}
+
 func TestPresetLoader_LoadFromFile(t *testing.T) {
 	t.Parallel()