docs: add security-focused improvement plans (XSS, pprof, proxy target validation)
This commit is contained in:
35
plans/002-stop-marking-json-attributes-trusted-html.md
Normal file
35
plans/002-stop-marking-json-attributes-trusted-html.md
Normal file
@@ -0,0 +1,35 @@
|
||||
---
|
||||
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`.
|
||||
34
plans/003-gate-or-remove-unauthenticated-pprof-routes.md
Normal file
34
plans/003-gate-or-remove-unauthenticated-pprof-routes.md
Normal file
@@ -0,0 +1,34 @@
|
||||
---
|
||||
finding: "Gate or remove unauthenticated pprof routes"
|
||||
catalog: "Security"
|
||||
impact: "Anyone who can reach the server can access debug profiling endpoints and runtime internals."
|
||||
base_commit: "0d31d46"
|
||||
---
|
||||
|
||||
## Effort
|
||||
|
||||
S - small routing/configuration change plus route tests.
|
||||
|
||||
## Risk
|
||||
|
||||
LOW - normal application routes should be unaffected.
|
||||
|
||||
## Confidence
|
||||
|
||||
HIGH - `pprof` routes are registered before authentication middleware is applied.
|
||||
|
||||
## Evidence
|
||||
|
||||
- `internal/server/server.go:10` imports `net/http/pprof`, registering handlers on `http.DefaultServeMux`.
|
||||
- `internal/server/server.go:42-43` mounts `/debug/pprof` and `/debug/pprof/*action` on the Gin router.
|
||||
- `internal/app.go:43-45` applies auth middleware later, before module routes are registered, so these pre-existing routes are not protected by that middleware.
|
||||
|
||||
## Resolution Approach
|
||||
|
||||
Choose one explicit policy and implement it consistently. The safest default is to remove pprof routes from the main application router. If local profiling is still desired, add an explicit configuration flag such as `MAL_ENABLE_PPROF=1` and register routes only when it is enabled.
|
||||
|
||||
If pprof remains available, it should be behind the same authentication middleware as protected pages or restricted to development-only usage. Avoid enabling it by default in release mode.
|
||||
|
||||
Add tests in `internal/server` or an app-level route test that assert `/debug/pprof` is not available by default. If a config flag is added, test both disabled and enabled behavior.
|
||||
|
||||
Verify with `go test ./internal/server ./...` and `bun run lint:go` after the lint baseline plan is complete.
|
||||
35
plans/004-validate-playback-proxy-targets.md
Normal file
35
plans/004-validate-playback-proxy-targets.md
Normal file
@@ -0,0 +1,35 @@
|
||||
---
|
||||
finding: "Validate playback proxy targets before signing/fetching"
|
||||
catalog: "Security"
|
||||
impact: "A compromised provider response can cause the server-side playback proxy to fetch arbitrary HTTP(S) targets."
|
||||
base_commit: "0d31d46"
|
||||
---
|
||||
|
||||
## Effort
|
||||
|
||||
M - validation needs careful tests around providers, redirects, and legitimate CDN URLs.
|
||||
|
||||
## Risk
|
||||
|
||||
MED - overly strict validation can block legitimate playback hosts or subtitle CDNs.
|
||||
|
||||
## Confidence
|
||||
|
||||
HIGH - provider URLs are accepted and later fetched server-side without target validation.
|
||||
|
||||
## Evidence
|
||||
|
||||
- `integrations/playback/allanime/sources.go:115-131` accepts any direct `http://` or `https://` source URL that is not detected as an embed.
|
||||
- `internal/playback/watch_data.go:181-191` signs subtitle and stream URLs returned by the provider.
|
||||
- `internal/playback/handler/proxy_request.go:13-24` creates a server-side GET request to the signed target URL.
|
||||
- `internal/playback/handler/proxy_stream.go:33` and `internal/playback/handler/proxy_subtitle.go:34` execute those requests.
|
||||
|
||||
## Resolution Approach
|
||||
|
||||
Introduce a proxy target validation boundary before issuing tokens and before fetching token targets. The validation should reject non-HTTP(S) schemes, empty hosts, loopback, private, link-local, and otherwise local network destinations. Because DNS and redirects can change the destination, validation should cover the final network target used by the HTTP client, not only the original string.
|
||||
|
||||
Keep legitimate provider/CDN behavior in mind. If a host allowlist is practical for AllAnime and known subtitle hosts, prefer it. If not, use network-range blocking with strong tests and conservative redirect handling.
|
||||
|
||||
Apply validation in two places: before signing tokens in playback service code, and again in the proxy handler before `Do` as defense in depth. Add tests for direct stream URLs, subtitle URLs, HLS playlist rewrites, and redirect behavior. Do not include runnable abuse payloads in tests or docs; keep cases generic and defensive.
|
||||
|
||||
Verify with `go test ./internal/playback/... ./integrations/playback/allanime/...` and `bun run lint:go` after the lint baseline is fixed.
|
||||
Reference in New Issue
Block a user