chore: remove resolved plan for JSON attribute escaping
This commit is contained in:
@@ -20,13 +20,28 @@ func (fxLogger) LogEvent(event fxevent.Event) {
|
||||
}
|
||||
|
||||
func describeFXEventError(event fxevent.Event) (string, map[string]any, error) {
|
||||
if eventName, fields, ok, err := describeFXBuildEventError(event); ok {
|
||||
return eventName, fields, err
|
||||
}
|
||||
|
||||
return describeFXLifecycleEventError(event)
|
||||
}
|
||||
|
||||
func describeFXBuildEventError(event fxevent.Event) (string, map[string]any, bool, error) {
|
||||
switch e := event.(type) {
|
||||
case *fxevent.Provided:
|
||||
return "fx_provide_failed", map[string]any{"constructor": e.ConstructorName}, e.Err
|
||||
return "fx_provide_failed", map[string]any{"constructor": e.ConstructorName}, true, e.Err
|
||||
case *fxevent.Invoked:
|
||||
return "fx_invoke_failed", map[string]any{"function": e.FunctionName}, e.Err
|
||||
return "fx_invoke_failed", map[string]any{"function": e.FunctionName}, true, e.Err
|
||||
case *fxevent.Run:
|
||||
return "fx_run_failed", map[string]any{"function": e.Name, "kind": e.Kind}, e.Err
|
||||
return "fx_run_failed", map[string]any{"function": e.Name, "kind": e.Kind}, true, e.Err
|
||||
default:
|
||||
return "", nil, false, nil
|
||||
}
|
||||
}
|
||||
|
||||
func describeFXLifecycleEventError(event fxevent.Event) (string, map[string]any, error) {
|
||||
switch e := event.(type) {
|
||||
case *fxevent.OnStartExecuted:
|
||||
return "fx_on_start_failed", map[string]any{"caller": e.CallerName, "function": e.FunctionName, "runtime": e.Runtime}, e.Err
|
||||
case *fxevent.OnStopExecuted:
|
||||
|
||||
119
internal/observability/fx_test.go
Normal file
119
internal/observability/fx_test.go
Normal file
@@ -0,0 +1,119 @@
|
||||
package observability
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"reflect"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"go.uber.org/fx/fxevent"
|
||||
)
|
||||
|
||||
func TestDescribeFXEventErrorDescribesFailedBuildEvents(t *testing.T) {
|
||||
err := errors.New("fx failed")
|
||||
|
||||
tests := map[string]struct {
|
||||
event fxevent.Event
|
||||
wantName string
|
||||
wantFields map[string]any
|
||||
}{
|
||||
"provided": {
|
||||
event: &fxevent.Provided{ConstructorName: "newServer", Err: err},
|
||||
wantName: "fx_provide_failed",
|
||||
wantFields: map[string]any{"constructor": "newServer"},
|
||||
},
|
||||
"invoked": {
|
||||
event: &fxevent.Invoked{FunctionName: "startServer", Err: err},
|
||||
wantName: "fx_invoke_failed",
|
||||
wantFields: map[string]any{"function": "startServer"},
|
||||
},
|
||||
"run": {
|
||||
event: &fxevent.Run{Name: "newRepo", Kind: "provide", Err: err},
|
||||
wantName: "fx_run_failed",
|
||||
wantFields: map[string]any{"function": "newRepo", "kind": "provide"},
|
||||
},
|
||||
}
|
||||
|
||||
for name, tt := range tests {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
assertFXEventError(t, tt.event, tt.wantName, tt.wantFields, err)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestDescribeFXEventErrorDescribesFailedLifecycleEvents(t *testing.T) {
|
||||
err := errors.New("fx failed")
|
||||
runtime := 12 * time.Millisecond
|
||||
|
||||
tests := map[string]struct {
|
||||
event fxevent.Event
|
||||
wantName string
|
||||
wantFields map[string]any
|
||||
}{
|
||||
"on start executed": {
|
||||
event: &fxevent.OnStartExecuted{CallerName: "server", FunctionName: "listen", Runtime: runtime, Err: err},
|
||||
wantName: "fx_on_start_failed",
|
||||
wantFields: map[string]any{"caller": "server", "function": "listen", "runtime": runtime},
|
||||
},
|
||||
"on stop executed": {
|
||||
event: &fxevent.OnStopExecuted{CallerName: "server", FunctionName: "close", Runtime: runtime, Err: err},
|
||||
wantName: "fx_on_stop_failed",
|
||||
wantFields: map[string]any{"caller": "server", "function": "close", "runtime": runtime},
|
||||
},
|
||||
"started": {
|
||||
event: &fxevent.Started{Err: err},
|
||||
wantName: "fx_start_failed",
|
||||
},
|
||||
"stopped": {
|
||||
event: &fxevent.Stopped{Err: err},
|
||||
wantName: "fx_stop_failed",
|
||||
},
|
||||
"rolling back": {
|
||||
event: &fxevent.RollingBack{StartErr: err},
|
||||
wantName: "fx_rollback_start",
|
||||
},
|
||||
"rolled back": {
|
||||
event: &fxevent.RolledBack{Err: err},
|
||||
wantName: "fx_rollback_failed",
|
||||
},
|
||||
}
|
||||
|
||||
for name, tt := range tests {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
assertFXEventError(t, tt.event, tt.wantName, tt.wantFields, err)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func assertFXEventError(t *testing.T, event fxevent.Event, wantName string, wantFields map[string]any, wantErr error) {
|
||||
t.Helper()
|
||||
|
||||
gotName, gotFields, gotErr := describeFXEventError(event)
|
||||
|
||||
if gotName != wantName {
|
||||
t.Fatalf("event name = %q, want %q", gotName, wantName)
|
||||
}
|
||||
if !reflect.DeepEqual(gotFields, wantFields) {
|
||||
t.Fatalf("fields = %#v, want %#v", gotFields, wantFields)
|
||||
}
|
||||
if !errors.Is(gotErr, wantErr) {
|
||||
t.Fatalf("error = %v, want %v", gotErr, wantErr)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDescribeFXEventErrorIgnoresEventsWithoutErrors(t *testing.T) {
|
||||
tests := map[string]fxevent.Event{
|
||||
"provided": &fxevent.Provided{ConstructorName: "newServer"},
|
||||
"unknown": &fxevent.Supplied{},
|
||||
}
|
||||
|
||||
for name, event := range tests {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
_, _, gotErr := describeFXEventError(event)
|
||||
|
||||
if gotErr != nil {
|
||||
t.Fatalf("error = %v, want nil", gotErr)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -1,32 +0,0 @@
|
||||
---
|
||||
finding: "Fix broken Go lint baseline"
|
||||
catalog: "DX / Tooling"
|
||||
impact: "The documented full verification gate cannot pass because Go lint currently fails."
|
||||
base_commit: "0d31d46"
|
||||
---
|
||||
|
||||
## Effort
|
||||
|
||||
S - one focused refactor plus verification.
|
||||
|
||||
## Risk
|
||||
|
||||
LOW - the change should preserve behavior and only reduce function complexity.
|
||||
|
||||
## Confidence
|
||||
|
||||
HIGH - `bun run lint:go` fails reproducibly on one function.
|
||||
|
||||
## Evidence
|
||||
|
||||
- `internal/observability/fx.go:22` defines `describeFXEventError` with a large type switch.
|
||||
- `bun run lint:go` reports `cyclop`: calculated cyclomatic complexity is 11, max is 10.
|
||||
- `justfile:40` defines `check` as `lint test typecheck build`, so this failure breaks `just check`.
|
||||
|
||||
## Resolution Approach
|
||||
|
||||
Refactor `describeFXEventError` without changing the log events it emits. Keep the same public behavior: `LogEvent` should call `describeFXEventError`, ignore events with no error, and log failed Fx lifecycle events with the same event names and field maps.
|
||||
|
||||
A small, safe approach is to split the switch into narrower helpers, for example one helper for constructor/invoke/run failures and one helper for lifecycle/start/stop/rollback failures. Another acceptable approach is to use small typed helper functions for repeated return shapes. Do not suppress the linter unless a refactor proves impossible.
|
||||
|
||||
Add or update tests if existing tests cover Fx event descriptions. Verify with `bun run lint:go`, then run `go test ./...`. After this lands, `just check` should be able to proceed past Go lint.
|
||||
@@ -1,35 +0,0 @@
|
||||
---
|
||||
finding: "Stop marking JSON attributes as trusted HTML"
|
||||
catalog: "Security"
|
||||
impact: "Provider-controlled strings can break quoted data attributes on the watch page, creating an HTML injection risk."
|
||||
base_commit: "0d31d46"
|
||||
---
|
||||
|
||||
## Effort
|
||||
|
||||
S - localized template helper and tests.
|
||||
|
||||
## Risk
|
||||
|
||||
LOW - the intended behavior is still to serialize data for client-side parsing, but escaping must be handled by `html/template`.
|
||||
|
||||
## Confidence
|
||||
|
||||
HIGH - the current helper explicitly bypasses contextual escaping.
|
||||
|
||||
## Evidence
|
||||
|
||||
- `templates/funcs.go:37-42` marshals JSON and returns `template.HTMLAttr`.
|
||||
- `templates/renderer.go:46-50` registers the helper as `json`.
|
||||
- `templates/components/video_player.gohtml:10-12` embeds JSON inside single-quoted `data-*` attributes.
|
||||
- `internal/playback/watch_data.go:181-188` copies provider subtitle labels into that serialized payload.
|
||||
|
||||
## Resolution Approach
|
||||
|
||||
Change the JSON serialization path so marshaled JSON is not marked as trusted `template.HTMLAttr`. Let `html/template` perform contextual escaping, or move larger JSON payloads into `<script type="application/json">` blocks where the template engine can safely escape script content.
|
||||
|
||||
The executor should preserve the client contract used by `static/player/state.ts`: the player still needs to read mode sources, available modes, and skip segments from the rendered page. If the storage location changes from `data-*` attributes to JSON script tags, update the parser in one place and add tests around that parser.
|
||||
|
||||
Add a template or renderer test that renders a watch-page-like payload containing apostrophes, double quotes, angle brackets, and ampersands in provider-controlled fields such as subtitle labels. The test should assert the rendered HTML does not create new attributes or tags and that the browser-side parser can recover the original data.
|
||||
|
||||
Verify with `go test ./templates ./internal/playback/...`, `bunx tsc -p tsconfig.json --noEmit`, and `bun test`.
|
||||
Reference in New Issue
Block a user