fix(audit): harden local browser boundaries
- commit
- a19ed710b519d90cc4dc909419bbe66b7f77da8d
- author
- brice · · 2026-06-11 04:05 UTC
- parent
- 5eba8fd
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
13 files · +675 -64
M .github/workflows/ci.yml +5 -2
| @@ -65,9 +65,10 @@ jobs: | |||
| 65 | 65 | ${{ runner.os }}-go-build- | |
| 66 | 66 | - name: Build application and systemviews assets | |
| 67 | 67 | run: make build | |
| 68 | + | - name: Verify generated systemviews artifacts | |
| 69 | + | run: make verify-generated | |
| 68 | 70 | - name: Install golangci-lint | |
| 69 | - | run: | | |
| 70 | - | curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b /usr/local/bin latest | |
| 71 | + | run: make install-golangci-lint TOOL_BIN_DIR=/usr/local/bin | |
| 71 | 72 | - name: Run golangci-lint | |
| 72 | 73 | run: | | |
| 73 | 74 | git config --global --add safe.directory "$GITHUB_WORKSPACE" | |
| @@ -101,6 +102,8 @@ jobs: | |||
| 101 | 102 | ${{ runner.os }}-go-build- | |
| 102 | 103 | - name: Build application and systemviews assets | |
| 103 | 104 | run: make build | |
| 105 | + | - name: Verify generated systemviews artifacts | |
| 106 | + | run: make verify-generated | |
| 104 | 107 | - name: Run tests | |
| 105 | 108 | env: | |
| 106 | 109 | GDK_BACKEND: wayland | |
| @@ -65,9 +65,10 @@ jobs: | |||||
| 65 | ${{ runner.os }}-go-build- | 65 | ${{ runner.os }}-go-build- | ||
| 66 | - name: Build application and systemviews assets | 66 | - name: Build application and systemviews assets | ||
| 67 | run: make build | 67 | run: make build | ||
| 68 | + | - name: Verify generated systemviews artifacts | |||
| 69 | + | run: make verify-generated | |||
| 68 | - name: Install golangci-lint | 70 | - name: Install golangci-lint | ||
| 69 | - | run: | | 71 | + | run: make install-golangci-lint TOOL_BIN_DIR=/usr/local/bin |
| 70 | - | curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b /usr/local/bin latest | |||
| 71 | - name: Run golangci-lint | 72 | - name: Run golangci-lint | ||
| 72 | run: | | 73 | run: | | ||
| 73 | git config --global --add safe.directory "$GITHUB_WORKSPACE" | 74 | git config --global --add safe.directory "$GITHUB_WORKSPACE" | ||
| @@ -101,6 +102,8 @@ jobs: | |||||
| 101 | ${{ runner.os }}-go-build- | 102 | ${{ runner.os }}-go-build- | ||
| 102 | - name: Build application and systemviews assets | 103 | - name: Build application and systemviews assets | ||
| 103 | run: make build | 104 | run: make build | ||
| 105 | + | - name: Verify generated systemviews artifacts | |||
| 106 | + | run: make verify-generated | |||
| 104 | - name: Run tests | 107 | - name: Run tests | ||
| 105 | env: | 108 | env: | ||
| 106 | GDK_BACKEND: wayland | 109 | GDK_BACKEND: wayland | ||
M Makefile +25 -3
| @@ -1,6 +1,6 @@ | |||
| 1 | 1 | # Makefile for dumber (Clean Architecture - puregotk) | |
| 2 | 2 | ||
| 3 | - | .PHONY: build build-systemviews generate-systemviews build-quick install-local test lint clean install-tools dev generate help init man flatpak-deps flatpak-build flatpak-install flatpak-run flatpak-clean stress-omnibox-callbacks verify-purego check | |
| 3 | + | .PHONY: build build-systemviews generate-systemviews build-quick install-local test lint staticcheck clean install-tools install-golangci-lint install-staticcheck dev generate help init man flatpak-deps flatpak-build flatpak-install flatpak-run flatpak-clean stress-omnibox-callbacks verify-purego verify-generated check | |
| 4 | 4 | ||
| 5 | 5 | # Load local overrides from .env.local if present (Makefile syntax) | |
| 6 | 6 | ifneq (,$(wildcard .env.local)) | |
| @@ -13,6 +13,9 @@ BINARY_NAME=dumber | |||
| 13 | 13 | MAIN_PATH=./cmd/dumber | |
| 14 | 14 | DIST_DIR=dist | |
| 15 | 15 | LOCAL_BIN_DIR?=$(HOME)/.local/bin | |
| 16 | + | TOOL_BIN_DIR?=$(shell go env GOPATH)/bin | |
| 17 | + | GOLANGCI_LINT_VERSION?=v2.12.2 | |
| 18 | + | STATICCHECK_VERSION?=v0.7.0 | |
| 16 | 19 | ||
| 17 | 20 | # Detect number of CPU cores for parallel compilation | |
| 18 | 21 | NPROCS?=$(shell nproc 2>/dev/null || echo 1) | |
| @@ -126,6 +129,13 @@ stress-omnibox-callbacks: ## Run placeholder omnibox callback stress harness | |||
| 126 | 129 | verify-purego: ## Ensure callback path stays cgo/export free | |
| 127 | 130 | bash ./scripts/verify_purego_only.sh | |
| 128 | 131 | ||
| 132 | + | verify-generated: ## Verify generated systemviews artifacts are committed | |
| 133 | + | @echo "Verifying generated systemviews artifacts..." | |
| 134 | + | @git diff --exit-code -- assets/systemviews internal/ui/systemviews || { \ | |
| 135 | + | echo "Generated systemviews artifacts are out of date. Run 'make build-systemviews' and commit the result."; \ | |
| 136 | + | exit 1; \ | |
| 137 | + | } | |
| 138 | + | ||
| 129 | 139 | # Linting | |
| 130 | 140 | lint: ## Run golangci-lint | |
| 131 | 141 | @echo "Running golangci-lint..." | |
| @@ -135,16 +145,28 @@ lint-fix: ## Run golangci-lint with --fix | |||
| 135 | 145 | @echo "Running golangci-lint with --fix..." | |
| 136 | 146 | golangci-lint run --fix | |
| 137 | 147 | ||
| 148 | + | staticcheck: ## Run Staticcheck | |
| 149 | + | @echo "Running staticcheck..." | |
| 150 | + | staticcheck ./... | |
| 151 | + | ||
| 138 | 152 | # Format code | |
| 139 | 153 | fmt: ## Format Go code with gofmt | |
| 140 | 154 | @echo "Formatting Go code..." | |
| 141 | 155 | go fmt ./... | |
| 142 | 156 | ||
| 143 | 157 | # Tools installation | |
| 144 | - | install-tools: ## Install development tools | |
| 158 | + | install-golangci-lint: ## Install pinned golangci-lint | |
| 159 | + | @echo "Installing golangci-lint $(GOLANGCI_LINT_VERSION) to $(TOOL_BIN_DIR)..." | |
| 160 | + | @mkdir -p $(TOOL_BIN_DIR) | |
| 161 | + | curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/$(GOLANGCI_LINT_VERSION)/install.sh | sh -s -- -b $(TOOL_BIN_DIR) $(GOLANGCI_LINT_VERSION) | |
| 162 | + | ||
| 163 | + | install-staticcheck: ## Install pinned Staticcheck with the active Go toolchain | |
| 164 | + | @echo "Installing staticcheck $(STATICCHECK_VERSION)..." | |
| 165 | + | go install honnef.co/go/tools/cmd/staticcheck@$(STATICCHECK_VERSION) | |
| 166 | + | ||
| 167 | + | install-tools: install-golangci-lint install-staticcheck ## Install development tools | |
| 145 | 168 | @echo "Installing development tools..." | |
| 146 | 169 | go install github.com/sqlc-dev/sqlc/cmd/sqlc@latest | |
| 147 | - | go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest | |
| 148 | 170 | @echo "Tools installed successfully" | |
| 149 | 171 | ||
| 150 | 172 | # Cleanup | |
| @@ -1,6 +1,6 @@ | |||||
| 1 | # Makefile for dumber (Clean Architecture - puregotk) | 1 | # Makefile for dumber (Clean Architecture - puregotk) | ||
| 2 | 2 | ||||
| 3 | - | .PHONY: build build-systemviews generate-systemviews build-quick install-local test lint clean install-tools dev generate help init man flatpak-deps flatpak-build flatpak-install flatpak-run flatpak-clean stress-omnibox-callbacks verify-purego check | 3 | + | .PHONY: build build-systemviews generate-systemviews build-quick install-local test lint staticcheck clean install-tools install-golangci-lint install-staticcheck dev generate help init man flatpak-deps flatpak-build flatpak-install flatpak-run flatpak-clean stress-omnibox-callbacks verify-purego verify-generated check |
| 4 | 4 | ||||
| 5 | # Load local overrides from .env.local if present (Makefile syntax) | 5 | # Load local overrides from .env.local if present (Makefile syntax) | ||
| 6 | ifneq (,$(wildcard .env.local)) | 6 | ifneq (,$(wildcard .env.local)) | ||
| @@ -13,6 +13,9 @@ BINARY_NAME=dumber | |||||
| 13 | MAIN_PATH=./cmd/dumber | 13 | MAIN_PATH=./cmd/dumber | ||
| 14 | DIST_DIR=dist | 14 | DIST_DIR=dist | ||
| 15 | LOCAL_BIN_DIR?=$(HOME)/.local/bin | 15 | LOCAL_BIN_DIR?=$(HOME)/.local/bin | ||
| 16 | + | TOOL_BIN_DIR?=$(shell go env GOPATH)/bin | |||
| 17 | + | GOLANGCI_LINT_VERSION?=v2.12.2 | |||
| 18 | + | STATICCHECK_VERSION?=v0.7.0 | |||
| 16 | 19 | ||||
| 17 | # Detect number of CPU cores for parallel compilation | 20 | # Detect number of CPU cores for parallel compilation | ||
| 18 | NPROCS?=$(shell nproc 2>/dev/null || echo 1) | 21 | NPROCS?=$(shell nproc 2>/dev/null || echo 1) | ||
| @@ -126,6 +129,13 @@ stress-omnibox-callbacks: ## Run placeholder omnibox callback stress harness | |||||
| 126 | verify-purego: ## Ensure callback path stays cgo/export free | 129 | verify-purego: ## Ensure callback path stays cgo/export free | ||
| 127 | bash ./scripts/verify_purego_only.sh | 130 | bash ./scripts/verify_purego_only.sh | ||
| 128 | 131 | ||||
| 132 | + | verify-generated: ## Verify generated systemviews artifacts are committed | |||
| 133 | + | @echo "Verifying generated systemviews artifacts..." | |||
| 134 | + | @git diff --exit-code -- assets/systemviews internal/ui/systemviews || { \ | |||
| 135 | + | echo "Generated systemviews artifacts are out of date. Run 'make build-systemviews' and commit the result."; \ | |||
| 136 | + | exit 1; \ | |||
| 137 | + | } | |||
| 138 | + | ||||
| 129 | # Linting | 139 | # Linting | ||
| 130 | lint: ## Run golangci-lint | 140 | lint: ## Run golangci-lint | ||
| 131 | @echo "Running golangci-lint..." | 141 | @echo "Running golangci-lint..." | ||
| @@ -135,16 +145,28 @@ lint-fix: ## Run golangci-lint with --fix | |||||
| 135 | @echo "Running golangci-lint with --fix..." | 145 | @echo "Running golangci-lint with --fix..." | ||
| 136 | golangci-lint run --fix | 146 | golangci-lint run --fix | ||
| 137 | 147 | ||||
| 148 | + | staticcheck: ## Run Staticcheck | |||
| 149 | + | @echo "Running staticcheck..." | |||
| 150 | + | staticcheck ./... | |||
| 151 | + | ||||
| 138 | # Format code | 152 | # Format code | ||
| 139 | fmt: ## Format Go code with gofmt | 153 | fmt: ## Format Go code with gofmt | ||
| 140 | @echo "Formatting Go code..." | 154 | @echo "Formatting Go code..." | ||
| 141 | go fmt ./... | 155 | go fmt ./... | ||
| 142 | 156 | ||||
| 143 | # Tools installation | 157 | # Tools installation | ||
| 144 | - | install-tools: ## Install development tools | 158 | + | install-golangci-lint: ## Install pinned golangci-lint |
| 159 | + | @echo "Installing golangci-lint $(GOLANGCI_LINT_VERSION) to $(TOOL_BIN_DIR)..." | |||
| 160 | + | @mkdir -p $(TOOL_BIN_DIR) | |||
| 161 | + | curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/$(GOLANGCI_LINT_VERSION)/install.sh | sh -s -- -b $(TOOL_BIN_DIR) $(GOLANGCI_LINT_VERSION) | |||
| 162 | + | ||||
| 163 | + | install-staticcheck: ## Install pinned Staticcheck with the active Go toolchain | |||
| 164 | + | @echo "Installing staticcheck $(STATICCHECK_VERSION)..." | |||
| 165 | + | go install honnef.co/go/tools/cmd/staticcheck@$(STATICCHECK_VERSION) | |||
| 166 | + | ||||
| 167 | + | install-tools: install-golangci-lint install-staticcheck ## Install development tools | |||
| 145 | @echo "Installing development tools..." | 168 | @echo "Installing development tools..." | ||
| 146 | go install github.com/sqlc-dev/sqlc/cmd/sqlc@latest | 169 | go install github.com/sqlc-dev/sqlc/cmd/sqlc@latest | ||
| 147 | - | go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest | |||
| 148 | @echo "Tools installed successfully" | 170 | @echo "Tools installed successfully" | ||
| 149 | 171 | ||||
| 150 | # Cleanup | 172 | # Cleanup | ||
M README.md +11 -5
| @@ -208,9 +208,11 @@ Set `ENV=dev` to use `.dev/dumber/` for config and data instead of XDG paths. | |||
| 208 | 208 | ||
| 209 | 209 | **Prerequisites:** | |
| 210 | 210 | ||
| 211 | - | - Go 1.25+ | |
| 212 | - | - Node.js 20+ for frontend assets | |
| 211 | + | - Go 1.26+ | |
| 213 | 212 | - WebKitGTK 6.0 and GTK4 development packages | |
| 213 | + | - Brotli for compressed systemviews assets | |
| 214 | + | ||
| 215 | + | Systemviews assets are generated with `go tool templ` and Go's `js/wasm` toolchain; no root Node toolchain is required. | |
| 214 | 216 | ||
| 215 | 217 | ```bash | |
| 216 | 218 | git clone https://github.com/bnema/dumber | |
| @@ -223,13 +225,17 @@ make build | |||
| 223 | 225 | ||
| 224 | 226 | | Target | Description | | |
| 225 | 227 | |--------|-------------| | |
| 226 | - | | `make build` | Build frontend and binary | | |
| 227 | - | | `make build-quick` | Build binary only, skipping frontend | | |
| 228 | + | | `make build` | Build systemviews assets and binary | | |
| 229 | + | | `make build-quick` | Build binary only, skipping systemviews assets | | |
| 228 | 230 | | `make dev` | Run with `go run` | | |
| 229 | 231 | | `make test` | Run tests | | |
| 230 | - | | `make lint` | Run golangci-lint | | |
| 232 | + | | `make lint` | Run the pinned golangci-lint version | | |
| 233 | + | | `make staticcheck` | Run Staticcheck with the pinned tool version | | |
| 234 | + | | `make verify-generated` | Verify generated systemviews artifacts are committed | | |
| 231 | 235 | | `make flatpak-build` | Build Flatpak bundle | | |
| 232 | 236 | ||
| 237 | + | Development tool versions are pinned in `Makefile` (`GOLANGCI_LINT_VERSION`, `STATICCHECK_VERSION`). Bump those values intentionally when refreshing lint/static analysis tooling. | |
| 238 | + | ||
| 233 | 239 | ## Contributing | |
| 234 | 240 | ||
| 235 | 241 | Dumber is currently focused on stability, performance, browser-engine behavior, and core UI/UX polish. | |
| @@ -208,9 +208,11 @@ Set `ENV=dev` to use `.dev/dumber/` for config and data instead of XDG paths. | |||||
| 208 | 208 | ||||
| 209 | **Prerequisites:** | 209 | **Prerequisites:** | ||
| 210 | 210 | ||||
| 211 | - | - Go 1.25+ | 211 | + | - Go 1.26+ |
| 212 | - | - Node.js 20+ for frontend assets | |||
| 213 | - WebKitGTK 6.0 and GTK4 development packages | 212 | - WebKitGTK 6.0 and GTK4 development packages | ||
| 213 | + | - Brotli for compressed systemviews assets | |||
| 214 | + | ||||
| 215 | + | Systemviews assets are generated with `go tool templ` and Go's `js/wasm` toolchain; no root Node toolchain is required. | |||
| 214 | 216 | ||||
| 215 | ```bash | 217 | ```bash | ||
| 216 | git clone https://github.com/bnema/dumber | 218 | git clone https://github.com/bnema/dumber | ||
| @@ -223,13 +225,17 @@ make build | |||||
| 223 | 225 | ||||
| 224 | | Target | Description | | 226 | | Target | Description | | ||
| 225 | |--------|-------------| | 227 | |--------|-------------| | ||
| 226 | - | | `make build` | Build frontend and binary | | 228 | + | | `make build` | Build systemviews assets and binary | |
| 227 | - | | `make build-quick` | Build binary only, skipping frontend | | 229 | + | | `make build-quick` | Build binary only, skipping systemviews assets | |
| 228 | | `make dev` | Run with `go run` | | 230 | | `make dev` | Run with `go run` | | ||
| 229 | | `make test` | Run tests | | 231 | | `make test` | Run tests | | ||
| 230 | - | | `make lint` | Run golangci-lint | | 232 | + | | `make lint` | Run the pinned golangci-lint version | |
| 233 | + | | `make staticcheck` | Run Staticcheck with the pinned tool version | | |||
| 234 | + | | `make verify-generated` | Verify generated systemviews artifacts are committed | | |||
| 231 | | `make flatpak-build` | Build Flatpak bundle | | 235 | | `make flatpak-build` | Build Flatpak bundle | | ||
| 232 | 236 | ||||
| 237 | + | Development tool versions are pinned in `Makefile` (`GOLANGCI_LINT_VERSION`, `STATICCHECK_VERSION`). Bump those values intentionally when refreshing lint/static analysis tooling. | |||
| 238 | + | ||||
| 233 | ## Contributing | 239 | ## Contributing | ||
| 234 | 240 | ||||
| 235 | Dumber is currently focused on stability, performance, browser-engine behavior, and core UI/UX polish. | 241 | Dumber is currently focused on stability, performance, browser-engine behavior, and core UI/UX polish. | ||
M docs/installation.md +7 -3
| @@ -43,15 +43,19 @@ flatpak install dumber.flatpak | |||
| 43 | 43 | ```bash | |
| 44 | 44 | git clone https://github.com/bnema/dumber | |
| 45 | 45 | cd dumber | |
| 46 | - | go build -o dumber ./cmd/dumber | |
| 46 | + | make build | |
| 47 | + | ./dist/dumber browse | |
| 47 | 48 | ``` | |
| 48 | 49 | ||
| 49 | 50 | ### Build Dependencies | |
| 50 | 51 | ||
| 51 | - | - Go 1.25+ | |
| 52 | + | - Go 1.26+ | |
| 52 | 53 | - GTK4 development libraries | |
| 53 | - | - WebKitGTK 2.42+ development libraries | |
| 54 | + | - WebKitGTK 6.0 development libraries | |
| 54 | 55 | - GStreamer development libraries | |
| 56 | + | - Brotli for compressed systemviews assets | |
| 57 | + | ||
| 58 | + | Systemviews assets are generated with `go tool templ` and Go's `js/wasm` toolchain; no root Node toolchain is required. | |
| 55 | 59 | ||
| 56 | 60 | ## Post-Install | |
| 57 | 61 | ||
| @@ -43,15 +43,19 @@ flatpak install dumber.flatpak | |||||
| 43 | ```bash | 43 | ```bash | ||
| 44 | git clone https://github.com/bnema/dumber | 44 | git clone https://github.com/bnema/dumber | ||
| 45 | cd dumber | 45 | cd dumber | ||
| 46 | - | go build -o dumber ./cmd/dumber | 46 | + | make build |
| 47 | + | ./dist/dumber browse | |||
| 47 | ``` | 48 | ``` | ||
| 48 | 49 | ||||
| 49 | ### Build Dependencies | 50 | ### Build Dependencies | ||
| 50 | 51 | ||||
| 51 | - | - Go 1.25+ | 52 | + | - Go 1.26+ |
| 52 | - GTK4 development libraries | 53 | - GTK4 development libraries | ||
| 53 | - | - WebKitGTK 2.42+ development libraries | 54 | + | - WebKitGTK 6.0 development libraries |
| 54 | - GStreamer development libraries | 55 | - GStreamer development libraries | ||
| 56 | + | - Brotli for compressed systemviews assets | |||
| 57 | + | ||||
| 58 | + | Systemviews assets are generated with `go tool templ` and Go's `js/wasm` toolchain; no root Node toolchain is required. | |||
| 55 | 59 | ||||
| 56 | ## Post-Install | 60 | ## Post-Install | ||
| 57 | 61 | ||||
M internal/application/usecase/search_history.go +38 -16
| @@ -18,7 +18,16 @@ import ( | |||
| 18 | 18 | // Compile-time check: SearchHistoryUseCase must satisfy port.HomepageHistory. | |
| 19 | 19 | var _ port.HomepageHistory = (*SearchHistoryUseCase)(nil) | |
| 20 | 20 | ||
| 21 | - | const historyWindowDuration = 24 * time.Hour | |
| 21 | + | const ( | |
| 22 | + | historyWindowDuration = 24 * time.Hour | |
| 23 | + | ||
| 24 | + | defaultHistoryPageLimit = 50 | |
| 25 | + | maxHistoryPageLimit = 500 | |
| 26 | + | defaultHistorySearchLimit = 20 | |
| 27 | + | maxHistorySearchLimit = 100 | |
| 28 | + | defaultDomainStatsLimit = 20 | |
| 29 | + | maxDomainStatsLimit = 100 | |
| 30 | + | ) | |
| 22 | 31 | ||
| 23 | 32 | // SearchHistoryUseCase handles history search and retrieval operations. | |
| 24 | 33 | type SearchHistoryUseCase struct { | |
| @@ -47,10 +56,7 @@ func (uc *SearchHistoryUseCase) Search(ctx context.Context, input SearchInput) ( | |||
| 47 | 56 | return &SearchOutput{Matches: []entity.HistoryMatch{}}, nil | |
| 48 | 57 | } | |
| 49 | 58 | ||
| 50 | - | limit := input.Limit | |
| 51 | - | if limit <= 0 { | |
| 52 | - | limit = 20 // Default limit | |
| 53 | - | } | |
| 59 | + | limit := clampPositiveLimit(input.Limit, defaultHistorySearchLimit, maxHistorySearchLimit) | |
| 54 | 60 | ||
| 55 | 61 | // Use repository's FTS5 search | |
| 56 | 62 | matches, err := uc.historyRepo.Search(ctx, input.Query, limit) | |
| @@ -67,11 +73,10 @@ func (uc *SearchHistoryUseCase) Search(ctx context.Context, input SearchInput) ( | |||
| 67 | 73 | } | |
| 68 | 74 | ||
| 69 | 75 | // GetRecent retrieves recent history entries. A zero limit means all entries; | |
| 70 | - | // negative limits retain the historical default page size. | |
| 76 | + | // negative limits retain the historical default page size. Positive limits are | |
| 77 | + | // capped to keep WebUI-originated queries bounded. | |
| 71 | 78 | func (uc *SearchHistoryUseCase) GetRecent(ctx context.Context, limit, offset int) ([]*entity.HistoryEntry, error) { | |
| 72 | - | if limit < 0 { | |
| 73 | - | limit = 50 // Default limit for invalid negative values. | |
| 74 | - | } | |
| 79 | + | limit = clampOptionalLimit(limit, defaultHistoryPageLimit, maxHistoryPageLimit) | |
| 75 | 80 | ||
| 76 | 81 | entries, err := uc.historyRepo.GetRecent(ctx, limit, offset) | |
| 77 | 82 | if err != nil { | |
| @@ -82,11 +87,10 @@ func (uc *SearchHistoryUseCase) GetRecent(ctx context.Context, limit, offset int | |||
| 82 | 87 | } | |
| 83 | 88 | ||
| 84 | 89 | // GetRecentByDomain retrieves recent history entries for a canonical domain. A | |
| 85 | - | // zero limit means all matching entries; negative limits use the default page size. | |
| 90 | + | // zero limit means all matching entries; negative limits use the default page | |
| 91 | + | // size. Positive limits are capped to keep WebUI-originated queries bounded. | |
| 86 | 92 | func (uc *SearchHistoryUseCase) GetRecentByDomain(ctx context.Context, domain string, limit, offset int) ([]*entity.HistoryEntry, error) { | |
| 87 | - | if limit < 0 { | |
| 88 | - | limit = 50 | |
| 89 | - | } | |
| 93 | + | limit = clampOptionalLimit(limit, defaultHistoryPageLimit, maxHistoryPageLimit) | |
| 90 | 94 | domain, err := canonicalHistoryDomain(domain) | |
| 91 | 95 | if err != nil { | |
| 92 | 96 | return nil, err | |
| @@ -298,13 +302,31 @@ func (uc *SearchHistoryUseCase) GetDomainStats(ctx context.Context, limit int) ( | |||
| 298 | 302 | log := logging.FromContext(ctx) | |
| 299 | 303 | log.Debug().Int("limit", limit).Msg("getting domain stats") | |
| 300 | 304 | ||
| 301 | - | if limit <= 0 { | |
| 302 | - | limit = 20 | |
| 303 | - | } | |
| 305 | + | limit = clampPositiveLimit(limit, defaultDomainStatsLimit, maxDomainStatsLimit) | |
| 304 | 306 | ||
| 305 | 307 | return uc.historyRepo.GetDomainStats(ctx, limit) | |
| 306 | 308 | } | |
| 307 | 309 | ||
| 310 | + | func clampOptionalLimit(limit, defaultLimit, maxLimit int) int { | |
| 311 | + | if limit < 0 { | |
| 312 | + | return defaultLimit | |
| 313 | + | } | |
| 314 | + | if limit > maxLimit { | |
| 315 | + | return maxLimit | |
| 316 | + | } | |
| 317 | + | return limit | |
| 318 | + | } | |
| 319 | + | ||
| 320 | + | func clampPositiveLimit(limit, defaultLimit, maxLimit int) int { | |
| 321 | + | if limit <= 0 { | |
| 322 | + | return defaultLimit | |
| 323 | + | } | |
| 324 | + | if limit > maxLimit { | |
| 325 | + | return maxLimit | |
| 326 | + | } | |
| 327 | + | return limit | |
| 328 | + | } | |
| 329 | + | ||
| 308 | 330 | // GetStats retrieves lightweight aggregate history statistics. | |
| 309 | 331 | func (uc *SearchHistoryUseCase) GetStats(ctx context.Context) (*entity.HistoryStats, error) { | |
| 310 | 332 | stats, err := uc.historyRepo.GetStats(ctx) | |
| @@ -18,7 +18,16 @@ import ( | |||||
| 18 | // Compile-time check: SearchHistoryUseCase must satisfy port.HomepageHistory. | 18 | // Compile-time check: SearchHistoryUseCase must satisfy port.HomepageHistory. | ||
| 19 | var _ port.HomepageHistory = (*SearchHistoryUseCase)(nil) | 19 | var _ port.HomepageHistory = (*SearchHistoryUseCase)(nil) | ||
| 20 | 20 | ||||
| 21 | - | const historyWindowDuration = 24 * time.Hour | 21 | + | const ( |
| 22 | + | historyWindowDuration = 24 * time.Hour | |||
| 23 | + | ||||
| 24 | + | defaultHistoryPageLimit = 50 | |||
| 25 | + | maxHistoryPageLimit = 500 | |||
| 26 | + | defaultHistorySearchLimit = 20 | |||
| 27 | + | maxHistorySearchLimit = 100 | |||
| 28 | + | defaultDomainStatsLimit = 20 | |||
| 29 | + | maxDomainStatsLimit = 100 | |||
| 30 | + | ) | |||
| 22 | 31 | ||||
| 23 | // SearchHistoryUseCase handles history search and retrieval operations. | 32 | // SearchHistoryUseCase handles history search and retrieval operations. | ||
| 24 | type SearchHistoryUseCase struct { | 33 | type SearchHistoryUseCase struct { | ||
| @@ -47,10 +56,7 @@ func (uc *SearchHistoryUseCase) Search(ctx context.Context, input SearchInput) ( | |||||
| 47 | return &SearchOutput{Matches: []entity.HistoryMatch{}}, nil | 56 | return &SearchOutput{Matches: []entity.HistoryMatch{}}, nil | ||
| 48 | } | 57 | } | ||
| 49 | 58 | ||||
| 50 | - | limit := input.Limit | 59 | + | limit := clampPositiveLimit(input.Limit, defaultHistorySearchLimit, maxHistorySearchLimit) |
| 51 | - | if limit <= 0 { | |||
| 52 | - | limit = 20 // Default limit | |||
| 53 | - | } | |||
| 54 | 60 | ||||
| 55 | // Use repository's FTS5 search | 61 | // Use repository's FTS5 search | ||
| 56 | matches, err := uc.historyRepo.Search(ctx, input.Query, limit) | 62 | matches, err := uc.historyRepo.Search(ctx, input.Query, limit) | ||
| @@ -67,11 +73,10 @@ func (uc *SearchHistoryUseCase) Search(ctx context.Context, input SearchInput) ( | |||||
| 67 | } | 73 | } | ||
| 68 | 74 | ||||
| 69 | // GetRecent retrieves recent history entries. A zero limit means all entries; | 75 | // GetRecent retrieves recent history entries. A zero limit means all entries; | ||
| 70 | - | // negative limits retain the historical default page size. | 76 | + | // negative limits retain the historical default page size. Positive limits are |
| 77 | + | // capped to keep WebUI-originated queries bounded. | |||
| 71 | func (uc *SearchHistoryUseCase) GetRecent(ctx context.Context, limit, offset int) ([]*entity.HistoryEntry, error) { | 78 | func (uc *SearchHistoryUseCase) GetRecent(ctx context.Context, limit, offset int) ([]*entity.HistoryEntry, error) { | ||
| 72 | - | if limit < 0 { | 79 | + | limit = clampOptionalLimit(limit, defaultHistoryPageLimit, maxHistoryPageLimit) |
| 73 | - | limit = 50 // Default limit for invalid negative values. | |||
| 74 | - | } | |||
| 75 | 80 | ||||
| 76 | entries, err := uc.historyRepo.GetRecent(ctx, limit, offset) | 81 | entries, err := uc.historyRepo.GetRecent(ctx, limit, offset) | ||
| 77 | if err != nil { | 82 | if err != nil { | ||
| @@ -82,11 +87,10 @@ func (uc *SearchHistoryUseCase) GetRecent(ctx context.Context, limit, offset int | |||||
| 82 | } | 87 | } | ||
| 83 | 88 | ||||
| 84 | // GetRecentByDomain retrieves recent history entries for a canonical domain. A | 89 | // GetRecentByDomain retrieves recent history entries for a canonical domain. A | ||
| 85 | - | // zero limit means all matching entries; negative limits use the default page size. | 90 | + | // zero limit means all matching entries; negative limits use the default page |
| 91 | + | // size. Positive limits are capped to keep WebUI-originated queries bounded. | |||
| 86 | func (uc *SearchHistoryUseCase) GetRecentByDomain(ctx context.Context, domain string, limit, offset int) ([]*entity.HistoryEntry, error) { | 92 | func (uc *SearchHistoryUseCase) GetRecentByDomain(ctx context.Context, domain string, limit, offset int) ([]*entity.HistoryEntry, error) { | ||
| 87 | - | if limit < 0 { | 93 | + | limit = clampOptionalLimit(limit, defaultHistoryPageLimit, maxHistoryPageLimit) |
| 88 | - | limit = 50 | |||
| 89 | - | } | |||
| 90 | domain, err := canonicalHistoryDomain(domain) | 94 | domain, err := canonicalHistoryDomain(domain) | ||
| 91 | if err != nil { | 95 | if err != nil { | ||
| 92 | return nil, err | 96 | return nil, err | ||
| @@ -298,13 +302,31 @@ func (uc *SearchHistoryUseCase) GetDomainStats(ctx context.Context, limit int) ( | |||||
| 298 | log := logging.FromContext(ctx) | 302 | log := logging.FromContext(ctx) | ||
| 299 | log.Debug().Int("limit", limit).Msg("getting domain stats") | 303 | log.Debug().Int("limit", limit).Msg("getting domain stats") | ||
| 300 | 304 | ||||
| 301 | - | if limit <= 0 { | 305 | + | limit = clampPositiveLimit(limit, defaultDomainStatsLimit, maxDomainStatsLimit) |
| 302 | - | limit = 20 | |||
| 303 | - | } | |||
| 304 | 306 | ||||
| 305 | return uc.historyRepo.GetDomainStats(ctx, limit) | 307 | return uc.historyRepo.GetDomainStats(ctx, limit) | ||
| 306 | } | 308 | } | ||
| 307 | 309 | ||||
| 310 | + | func clampOptionalLimit(limit, defaultLimit, maxLimit int) int { | |||
| 311 | + | if limit < 0 { | |||
| 312 | + | return defaultLimit | |||
| 313 | + | } | |||
| 314 | + | if limit > maxLimit { | |||
| 315 | + | return maxLimit | |||
| 316 | + | } | |||
| 317 | + | return limit | |||
| 318 | + | } | |||
| 319 | + | ||||
| 320 | + | func clampPositiveLimit(limit, defaultLimit, maxLimit int) int { | |||
| 321 | + | if limit <= 0 { | |||
| 322 | + | return defaultLimit | |||
| 323 | + | } | |||
| 324 | + | if limit > maxLimit { | |||
| 325 | + | return maxLimit | |||
| 326 | + | } | |||
| 327 | + | return limit | |||
| 328 | + | } | |||
| 329 | + | ||||
| 308 | // GetStats retrieves lightweight aggregate history statistics. | 330 | // GetStats retrieves lightweight aggregate history statistics. | ||
| 309 | func (uc *SearchHistoryUseCase) GetStats(ctx context.Context) (*entity.HistoryStats, error) { | 331 | func (uc *SearchHistoryUseCase) GetStats(ctx context.Context) (*entity.HistoryStats, error) { | ||
| 310 | stats, err := uc.historyRepo.GetStats(ctx) | 332 | stats, err := uc.historyRepo.GetStats(ctx) | ||
M internal/application/usecase/search_history_test.go +49 -0
| @@ -38,6 +38,31 @@ func TestSearchHistoryUseCase_GetRecent_NegativeLimitDefaultsToPageSize(t *testi | |||
| 38 | 38 | assert.Empty(t, result) | |
| 39 | 39 | } | |
| 40 | 40 | ||
| 41 | + | func TestSearchHistoryUseCase_GetRecent_ClampsOversizedLimit(t *testing.T) { | |
| 42 | + | ctx := testContext() | |
| 43 | + | historyRepo := repomocks.NewMockHistoryRepository(t) | |
| 44 | + | historyRepo.EXPECT().GetRecent(mock.Anything, 500, 10).Return([]*entity.HistoryEntry{}, nil).Once() | |
| 45 | + | ||
| 46 | + | uc := usecase.NewSearchHistoryUseCase(historyRepo) | |
| 47 | + | result, err := uc.GetRecent(ctx, 10_000, 10) | |
| 48 | + | ||
| 49 | + | require.NoError(t, err) | |
| 50 | + | assert.Empty(t, result) | |
| 51 | + | } | |
| 52 | + | ||
| 53 | + | func TestSearchHistoryUseCase_Search_ClampsOversizedLimit(t *testing.T) { | |
| 54 | + | ctx := testContext() | |
| 55 | + | historyRepo := repomocks.NewMockHistoryRepository(t) | |
| 56 | + | historyRepo.EXPECT().Search(mock.Anything, "dumber", 100).Return([]entity.HistoryMatch{}, nil).Once() | |
| 57 | + | ||
| 58 | + | uc := usecase.NewSearchHistoryUseCase(historyRepo) | |
| 59 | + | result, err := uc.Search(ctx, usecase.SearchInput{Query: "dumber", Limit: 10_000}) | |
| 60 | + | ||
| 61 | + | require.NoError(t, err) | |
| 62 | + | require.NotNil(t, result) | |
| 63 | + | assert.Empty(t, result.Matches) | |
| 64 | + | } | |
| 65 | + | ||
| 41 | 66 | func TestSearchHistoryUseCase_GetRecentWindow_InvalidDomainReturnsValidationError(t *testing.T) { | |
| 42 | 67 | ctx := testContext() | |
| 43 | 68 | historyRepo := repomocks.NewMockHistoryRepository(t) | |
| @@ -189,6 +214,18 @@ func TestSearchHistoryUseCase_GetMostVisited_ReturnsEmptyWhenNoHistory(t *testin | |||
| 189 | 214 | assert.Empty(t, result) | |
| 190 | 215 | } | |
| 191 | 216 | ||
| 217 | + | func TestSearchHistoryUseCase_GetDomainStats_ClampsOversizedLimit(t *testing.T) { | |
| 218 | + | ctx := testContext() | |
| 219 | + | historyRepo := repomocks.NewMockHistoryRepository(t) | |
| 220 | + | historyRepo.EXPECT().GetDomainStats(mock.Anything, 100).Return([]*entity.DomainStat{}, nil).Once() | |
| 221 | + | ||
| 222 | + | uc := usecase.NewSearchHistoryUseCase(historyRepo) | |
| 223 | + | result, err := uc.GetDomainStats(ctx, 10_000) | |
| 224 | + | ||
| 225 | + | require.NoError(t, err) | |
| 226 | + | assert.Empty(t, result) | |
| 227 | + | } | |
| 228 | + | ||
| 192 | 229 | func TestSearchHistoryUseCase_ClearRange_RecentRangeDeletesSinceCutoff(t *testing.T) { | |
| 193 | 230 | ctx := testContext() | |
| 194 | 231 | historyRepo := repomocks.NewMockHistoryRepository(t) | |
| @@ -257,6 +294,18 @@ func TestSearchHistoryUseCase_GetRecentByDomainAllowsUnderscoreDomains(t *testin | |||
| 257 | 294 | assert.Equal(t, want, result) | |
| 258 | 295 | } | |
| 259 | 296 | ||
| 297 | + | func TestSearchHistoryUseCase_GetRecentByDomain_ClampsOversizedLimit(t *testing.T) { | |
| 298 | + | ctx := testContext() | |
| 299 | + | historyRepo := repomocks.NewMockHistoryRepository(t) | |
| 300 | + | historyRepo.EXPECT().GetRecentByDomain(mock.Anything, "example.com", 500, 25).Return([]*entity.HistoryEntry{}, nil).Once() | |
| 301 | + | uc := usecase.NewSearchHistoryUseCase(historyRepo) | |
| 302 | + | ||
| 303 | + | result, err := uc.GetRecentByDomain(ctx, "example.com", 10_000, 25) | |
| 304 | + | ||
| 305 | + | require.NoError(t, err) | |
| 306 | + | assert.Empty(t, result) | |
| 307 | + | } | |
| 308 | + | ||
| 260 | 309 | func TestSearchHistoryUseCase_DeleteByDomainAllowsUnderscoreDomains(t *testing.T) { | |
| 261 | 310 | ctx := testContext() | |
| 262 | 311 | historyRepo := repomocks.NewMockHistoryRepository(t) | |
| @@ -38,6 +38,31 @@ func TestSearchHistoryUseCase_GetRecent_NegativeLimitDefaultsToPageSize(t *testi | |||||
| 38 | assert.Empty(t, result) | 38 | assert.Empty(t, result) | ||
| 39 | } | 39 | } | ||
| 40 | 40 | ||||
| 41 | + | func TestSearchHistoryUseCase_GetRecent_ClampsOversizedLimit(t *testing.T) { | |||
| 42 | + | ctx := testContext() | |||
| 43 | + | historyRepo := repomocks.NewMockHistoryRepository(t) | |||
| 44 | + | historyRepo.EXPECT().GetRecent(mock.Anything, 500, 10).Return([]*entity.HistoryEntry{}, nil).Once() | |||
| 45 | + | ||||
| 46 | + | uc := usecase.NewSearchHistoryUseCase(historyRepo) | |||
| 47 | + | result, err := uc.GetRecent(ctx, 10_000, 10) | |||
| 48 | + | ||||
| 49 | + | require.NoError(t, err) | |||
| 50 | + | assert.Empty(t, result) | |||
| 51 | + | } | |||
| 52 | + | ||||
| 53 | + | func TestSearchHistoryUseCase_Search_ClampsOversizedLimit(t *testing.T) { | |||
| 54 | + | ctx := testContext() | |||
| 55 | + | historyRepo := repomocks.NewMockHistoryRepository(t) | |||
| 56 | + | historyRepo.EXPECT().Search(mock.Anything, "dumber", 100).Return([]entity.HistoryMatch{}, nil).Once() | |||
| 57 | + | ||||
| 58 | + | uc := usecase.NewSearchHistoryUseCase(historyRepo) | |||
| 59 | + | result, err := uc.Search(ctx, usecase.SearchInput{Query: "dumber", Limit: 10_000}) | |||
| 60 | + | ||||
| 61 | + | require.NoError(t, err) | |||
| 62 | + | require.NotNil(t, result) | |||
| 63 | + | assert.Empty(t, result.Matches) | |||
| 64 | + | } | |||
| 65 | + | ||||
| 41 | func TestSearchHistoryUseCase_GetRecentWindow_InvalidDomainReturnsValidationError(t *testing.T) { | 66 | func TestSearchHistoryUseCase_GetRecentWindow_InvalidDomainReturnsValidationError(t *testing.T) { | ||
| 42 | ctx := testContext() | 67 | ctx := testContext() | ||
| 43 | historyRepo := repomocks.NewMockHistoryRepository(t) | 68 | historyRepo := repomocks.NewMockHistoryRepository(t) | ||
| @@ -189,6 +214,18 @@ func TestSearchHistoryUseCase_GetMostVisited_ReturnsEmptyWhenNoHistory(t *testin | |||||
| 189 | assert.Empty(t, result) | 214 | assert.Empty(t, result) | ||
| 190 | } | 215 | } | ||
| 191 | 216 | ||||
| 217 | + | func TestSearchHistoryUseCase_GetDomainStats_ClampsOversizedLimit(t *testing.T) { | |||
| 218 | + | ctx := testContext() | |||
| 219 | + | historyRepo := repomocks.NewMockHistoryRepository(t) | |||
| 220 | + | historyRepo.EXPECT().GetDomainStats(mock.Anything, 100).Return([]*entity.DomainStat{}, nil).Once() | |||
| 221 | + | ||||
| 222 | + | uc := usecase.NewSearchHistoryUseCase(historyRepo) | |||
| 223 | + | result, err := uc.GetDomainStats(ctx, 10_000) | |||
| 224 | + | ||||
| 225 | + | require.NoError(t, err) | |||
| 226 | + | assert.Empty(t, result) | |||
| 227 | + | } | |||
| 228 | + | ||||
| 192 | func TestSearchHistoryUseCase_ClearRange_RecentRangeDeletesSinceCutoff(t *testing.T) { | 229 | func TestSearchHistoryUseCase_ClearRange_RecentRangeDeletesSinceCutoff(t *testing.T) { | ||
| 193 | ctx := testContext() | 230 | ctx := testContext() | ||
| 194 | historyRepo := repomocks.NewMockHistoryRepository(t) | 231 | historyRepo := repomocks.NewMockHistoryRepository(t) | ||
| @@ -257,6 +294,18 @@ func TestSearchHistoryUseCase_GetRecentByDomainAllowsUnderscoreDomains(t *testin | |||||
| 257 | assert.Equal(t, want, result) | 294 | assert.Equal(t, want, result) | ||
| 258 | } | 295 | } | ||
| 259 | 296 | ||||
| 297 | + | func TestSearchHistoryUseCase_GetRecentByDomain_ClampsOversizedLimit(t *testing.T) { | |||
| 298 | + | ctx := testContext() | |||
| 299 | + | historyRepo := repomocks.NewMockHistoryRepository(t) | |||
| 300 | + | historyRepo.EXPECT().GetRecentByDomain(mock.Anything, "example.com", 500, 25).Return([]*entity.HistoryEntry{}, nil).Once() | |||
| 301 | + | uc := usecase.NewSearchHistoryUseCase(historyRepo) | |||
| 302 | + | ||||
| 303 | + | result, err := uc.GetRecentByDomain(ctx, "example.com", 10_000, 25) | |||
| 304 | + | ||||
| 305 | + | require.NoError(t, err) | |||
| 306 | + | assert.Empty(t, result) | |||
| 307 | + | } | |||
| 308 | + | ||||
| 260 | func TestSearchHistoryUseCase_DeleteByDomainAllowsUnderscoreDomains(t *testing.T) { | 309 | func TestSearchHistoryUseCase_DeleteByDomainAllowsUnderscoreDomains(t *testing.T) { | ||
| 261 | ctx := testContext() | 310 | ctx := testContext() | ||
| 262 | historyRepo := repomocks.NewMockHistoryRepository(t) | 311 | historyRepo := repomocks.NewMockHistoryRepository(t) | ||
M internal/infrastructure/favicon/cache.go +17 -1
| @@ -30,6 +30,7 @@ type Cache struct { | |||
| 30 | 30 | diskDir string | |
| 31 | 31 | writeChan chan diskWrite | |
| 32 | 32 | closeOnce sync.Once | |
| 33 | + | closed bool | |
| 33 | 34 | mu sync.RWMutex | |
| 34 | 35 | } | |
| 35 | 36 | ||
| @@ -100,8 +101,12 @@ func (c *Cache) Set(ctx context.Context, domain string, data []byte) { | |||
| 100 | 101 | Int("bytes", len(data)). | |
| 101 | 102 | Msg("favicon: Cache.Set") | |
| 102 | 103 | ||
| 103 | - | // Write to memory cache | |
| 104 | + | // Write to memory cache unless the cache is already shutting down. | |
| 104 | 105 | c.mu.Lock() | |
| 106 | + | if c.closed { | |
| 107 | + | c.mu.Unlock() | |
| 108 | + | return | |
| 109 | + | } | |
| 105 | 110 | c.memCache[domain] = data | |
| 106 | 111 | c.mu.Unlock() | |
| 107 | 112 | ||
| @@ -228,6 +233,10 @@ func (c *Cache) HasOnDisk(domain string) bool { | |||
| 228 | 233 | // Close shuts down the background writer goroutine. | |
| 229 | 234 | func (c *Cache) Close() { | |
| 230 | 235 | c.closeOnce.Do(func() { | |
| 236 | + | c.mu.Lock() | |
| 237 | + | defer c.mu.Unlock() | |
| 238 | + | ||
| 239 | + | c.closed = true | |
| 231 | 240 | if c.writeChan != nil { | |
| 232 | 241 | close(c.writeChan) | |
| 233 | 242 | } | |
| @@ -308,6 +317,13 @@ func (c *Cache) queueDiskWrite(domain string, data []byte) { | |||
| 308 | 317 | if c.diskDir == "" { | |
| 309 | 318 | return | |
| 310 | 319 | } | |
| 320 | + | ||
| 321 | + | c.mu.RLock() | |
| 322 | + | defer c.mu.RUnlock() | |
| 323 | + | if c.closed { | |
| 324 | + | return | |
| 325 | + | } | |
| 326 | + | ||
| 311 | 327 | select { | |
| 312 | 328 | case c.writeChan <- diskWrite{domain: domain, data: data}: | |
| 313 | 329 | // queued successfully | |
| @@ -30,6 +30,7 @@ type Cache struct { | |||||
| 30 | diskDir string | 30 | diskDir string | ||
| 31 | writeChan chan diskWrite | 31 | writeChan chan diskWrite | ||
| 32 | closeOnce sync.Once | 32 | closeOnce sync.Once | ||
| 33 | + | closed bool | |||
| 33 | mu sync.RWMutex | 34 | mu sync.RWMutex | ||
| 34 | } | 35 | } | ||
| 35 | 36 | ||||
| @@ -100,8 +101,12 @@ func (c *Cache) Set(ctx context.Context, domain string, data []byte) { | |||||
| 100 | Int("bytes", len(data)). | 101 | Int("bytes", len(data)). | ||
| 101 | Msg("favicon: Cache.Set") | 102 | Msg("favicon: Cache.Set") | ||
| 102 | 103 | ||||
| 103 | - | // Write to memory cache | 104 | + | // Write to memory cache unless the cache is already shutting down. |
| 104 | c.mu.Lock() | 105 | c.mu.Lock() | ||
| 106 | + | if c.closed { | |||
| 107 | + | c.mu.Unlock() | |||
| 108 | + | return | |||
| 109 | + | } | |||
| 105 | c.memCache[domain] = data | 110 | c.memCache[domain] = data | ||
| 106 | c.mu.Unlock() | 111 | c.mu.Unlock() | ||
| 107 | 112 | ||||
| @@ -228,6 +233,10 @@ func (c *Cache) HasOnDisk(domain string) bool { | |||||
| 228 | // Close shuts down the background writer goroutine. | 233 | // Close shuts down the background writer goroutine. | ||
| 229 | func (c *Cache) Close() { | 234 | func (c *Cache) Close() { | ||
| 230 | c.closeOnce.Do(func() { | 235 | c.closeOnce.Do(func() { | ||
| 236 | + | c.mu.Lock() | |||
| 237 | + | defer c.mu.Unlock() | |||
| 238 | + | ||||
| 239 | + | c.closed = true | |||
| 231 | if c.writeChan != nil { | 240 | if c.writeChan != nil { | ||
| 232 | close(c.writeChan) | 241 | close(c.writeChan) | ||
| 233 | } | 242 | } | ||
| @@ -308,6 +317,13 @@ func (c *Cache) queueDiskWrite(domain string, data []byte) { | |||||
| 308 | if c.diskDir == "" { | 317 | if c.diskDir == "" { | ||
| 309 | return | 318 | return | ||
| 310 | } | 319 | } | ||
| 320 | + | ||||
| 321 | + | c.mu.RLock() | |||
| 322 | + | defer c.mu.RUnlock() | |||
| 323 | + | if c.closed { | |||
| 324 | + | return | |||
| 325 | + | } | |||
| 326 | + | ||||
| 311 | select { | 327 | select { | ||
| 312 | case c.writeChan <- diskWrite{domain: domain, data: data}: | 328 | case c.writeChan <- diskWrite{domain: domain, data: data}: | ||
| 313 | // queued successfully | 329 | // queued successfully | ||
A internal/infrastructure/favicon/cache_test.go +21 -0
| @@ -0,0 +1,21 @@ | |||
| 1 | + | package favicon | |
| 2 | + | ||
| 3 | + | import ( | |
| 4 | + | "context" | |
| 5 | + | "testing" | |
| 6 | + | ||
| 7 | + | "github.com/stretchr/testify/require" | |
| 8 | + | ) | |
| 9 | + | ||
| 10 | + | func TestCache_SetAfterCloseIsNoOp(t *testing.T) { | |
| 11 | + | ctx := context.Background() | |
| 12 | + | cache := NewCache(t.TempDir()) | |
| 13 | + | cache.Close() | |
| 14 | + | ||
| 15 | + | require.NotPanics(t, func() { | |
| 16 | + | cache.Set(ctx, "example.com", []byte("favicon")) | |
| 17 | + | }) | |
| 18 | + | ||
| 19 | + | _, ok := cache.Get(ctx, "example.com") | |
| 20 | + | require.False(t, ok) | |
| 21 | + | } | |
| @@ -0,0 +1,21 @@ | |||||
| 1 | + | package favicon | |||
| 2 | + | ||||
| 3 | + | import ( | |||
| 4 | + | "context" | |||
| 5 | + | "testing" | |||
| 6 | + | ||||
| 7 | + | "github.com/stretchr/testify/require" | |||
| 8 | + | ) | |||
| 9 | + | ||||
| 10 | + | func TestCache_SetAfterCloseIsNoOp(t *testing.T) { | |||
| 11 | + | ctx := context.Background() | |||
| 12 | + | cache := NewCache(t.TempDir()) | |||
| 13 | + | cache.Close() | |||
| 14 | + | ||||
| 15 | + | require.NotPanics(t, func() { | |||
| 16 | + | cache.Set(ctx, "example.com", []byte("favicon")) | |||
| 17 | + | }) | |||
| 18 | + | ||||
| 19 | + | _, ok := cache.Get(ctx, "example.com") | |||
| 20 | + | require.False(t, ok) | |||
| 21 | + | } | |||
M internal/infrastructure/filtering/downloader.go +230 -18
| @@ -7,7 +7,9 @@ import ( | |||
| 7 | 7 | "io" | |
| 8 | 8 | "net/http" | |
| 9 | 9 | "os" | |
| 10 | + | "path" | |
| 10 | 11 | "path/filepath" | |
| 12 | + | "strings" | |
| 11 | 13 | "time" | |
| 12 | 14 | ||
| 13 | 15 | "github.com/bnema/dumber/internal/logging" | |
| @@ -16,13 +18,34 @@ import ( | |||
| 16 | 18 | const ( | |
| 17 | 19 | cacheDirPerm = 0o755 | |
| 18 | 20 | manifestFilePerm = 0o644 | |
| 21 | + | ||
| 22 | + | defaultMaxManifestBytes int64 = 1 << 20 // 1 MiB | |
| 23 | + | defaultMaxFilterFiles = 128 | |
| 24 | + | defaultMaxFilterFileBytes int64 = 64 << 20 // 64 MiB | |
| 25 | + | defaultMaxTotalFilterBytes int64 = 512 << 20 // 512 MiB | |
| 19 | 26 | ) | |
| 20 | 27 | ||
| 28 | + | // downloadLimits bounds remote filter metadata and payloads. | |
| 29 | + | type downloadLimits struct { | |
| 30 | + | maxManifestBytes int64 | |
| 31 | + | maxFilterFiles int | |
| 32 | + | maxFilterFileBytes int64 | |
| 33 | + | maxTotalFilterBytes int64 | |
| 34 | + | } | |
| 35 | + | ||
| 36 | + | var defaultDownloadLimits = downloadLimits{ | |
| 37 | + | maxManifestBytes: defaultMaxManifestBytes, | |
| 38 | + | maxFilterFiles: defaultMaxFilterFiles, | |
| 39 | + | maxFilterFileBytes: defaultMaxFilterFileBytes, | |
| 40 | + | maxTotalFilterBytes: defaultMaxTotalFilterBytes, | |
| 41 | + | } | |
| 42 | + | ||
| 21 | 43 | // Downloader handles downloading filter files from GitHub releases. | |
| 22 | 44 | type Downloader struct { | |
| 23 | 45 | baseURL string | |
| 24 | 46 | cacheDir string | |
| 25 | 47 | httpClient *http.Client | |
| 48 | + | limits downloadLimits | |
| 26 | 49 | } | |
| 27 | 50 | ||
| 28 | 51 | // NewDownloader creates a new Downloader. | |
| @@ -34,14 +57,15 @@ func NewDownloader(cacheDir string) *Downloader { | |||
| 34 | 57 | httpClient: &http.Client{ | |
| 35 | 58 | Timeout: 60 * time.Second, | |
| 36 | 59 | }, | |
| 60 | + | limits: defaultDownloadLimits, | |
| 37 | 61 | } | |
| 38 | 62 | } | |
| 39 | 63 | ||
| 40 | 64 | // GetCachedManifest reads the locally cached manifest.json. | |
| 41 | 65 | // Returns nil if no cached manifest exists. | |
| 42 | 66 | func (d *Downloader) GetCachedManifest() (*Manifest, error) { | |
| 43 | - | path := filepath.Join(d.cacheDir, FilterFiles.Manifest) | |
| 44 | - | data, err := os.ReadFile(path) | |
| 67 | + | manifestPath := filepath.Join(d.cacheDir, FilterFiles.Manifest) | |
| 68 | + | data, err := readLimitedFile(manifestPath, d.limits.maxManifestBytes, "cached manifest") | |
| 45 | 69 | if err != nil { | |
| 46 | 70 | if os.IsNotExist(err) { | |
| 47 | 71 | return nil, nil | |
| @@ -53,6 +77,9 @@ func (d *Downloader) GetCachedManifest() (*Manifest, error) { | |||
| 53 | 77 | if err := json.Unmarshal(data, &manifest); err != nil { | |
| 54 | 78 | return nil, fmt.Errorf("failed to parse cached manifest: %w", err) | |
| 55 | 79 | } | |
| 80 | + | if err := d.validateManifest(&manifest); err != nil { | |
| 81 | + | return nil, fmt.Errorf("invalid cached manifest: %w", err) | |
| 82 | + | } | |
| 56 | 83 | return &manifest, nil | |
| 57 | 84 | } | |
| 58 | 85 | ||
| @@ -84,7 +111,7 @@ func (d *Downloader) FetchManifest(ctx context.Context) (*Manifest, error) { | |||
| 84 | 111 | return nil, fmt.Errorf("manifest fetch failed with status %d", resp.StatusCode) | |
| 85 | 112 | } | |
| 86 | 113 | ||
| 87 | - | data, err := io.ReadAll(resp.Body) | |
| 114 | + | data, err := readLimitedResponseBody(resp, d.limits.maxManifestBytes, "manifest") | |
| 88 | 115 | if err != nil { | |
| 89 | 116 | return nil, fmt.Errorf("failed to read manifest body: %w", err) | |
| 90 | 117 | } | |
| @@ -93,6 +120,9 @@ func (d *Downloader) FetchManifest(ctx context.Context) (*Manifest, error) { | |||
| 93 | 120 | if err := json.Unmarshal(data, &manifest); err != nil { | |
| 94 | 121 | return nil, fmt.Errorf("failed to parse manifest: %w", err) | |
| 95 | 122 | } | |
| 123 | + | if err := d.validateManifest(&manifest); err != nil { | |
| 124 | + | return nil, fmt.Errorf("invalid manifest: %w", err) | |
| 125 | + | } | |
| 96 | 126 | ||
| 97 | 127 | log.Debug().Str("version", manifest.Version).Msg("fetched manifest") | |
| 98 | 128 | return &manifest, nil | |
| @@ -153,7 +183,12 @@ func (d *Downloader) DownloadFilters(ctx context.Context, onProgress func(Downlo | |||
| 153 | 183 | }) | |
| 154 | 184 | } | |
| 155 | 185 | ||
| 156 | - | path, bytesDownloaded, err := d.downloadFile(ctx, filename) | |
| 186 | + | remainingBytes := d.limits.maxTotalFilterBytes - totalBytes | |
| 187 | + | if remainingBytes <= 0 { | |
| 188 | + | return nil, fmt.Errorf("filter downloads exceed total size limit of %d bytes", d.limits.maxTotalFilterBytes) | |
| 189 | + | } | |
| 190 | + | ||
| 191 | + | path, bytesDownloaded, err := d.downloadFile(ctx, filename, minInt64(d.limits.maxFilterFileBytes, remainingBytes)) | |
| 157 | 192 | if err != nil { | |
| 158 | 193 | log.Error().Err(err).Str("file", filename).Msg("failed to download filter file") | |
| 159 | 194 | return nil, err | |
| @@ -174,9 +209,13 @@ func (d *Downloader) DownloadFilters(ctx context.Context, onProgress func(Downlo | |||
| 174 | 209 | } | |
| 175 | 210 | ||
| 176 | 211 | // downloadFile downloads a single file and returns its local path. | |
| 177 | - | func (d *Downloader) downloadFile(ctx context.Context, filename string) (string, int64, error) { | |
| 178 | - | url := fmt.Sprintf("%s/%s", d.baseURL, filename) | |
| 179 | - | localPath := filepath.Join(d.cacheDir, filename) | |
| 212 | + | func (d *Downloader) downloadFile(ctx context.Context, filename string, maxBytes int64) (string, int64, error) { | |
| 213 | + | localPath, safeFilename, err := d.cachePathForFilterFile(filename) | |
| 214 | + | if err != nil { | |
| 215 | + | return "", 0, err | |
| 216 | + | } | |
| 217 | + | ||
| 218 | + | url := fmt.Sprintf("%s/%s", d.baseURL, safeFilename) | |
| 180 | 219 | ||
| 181 | 220 | req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) | |
| 182 | 221 | if err != nil { | |
| @@ -197,25 +236,29 @@ func (d *Downloader) downloadFile(ctx context.Context, filename string) (string, | |||
| 197 | 236 | return "", 0, fmt.Errorf("download %s failed with status %d", filename, resp.StatusCode) | |
| 198 | 237 | } | |
| 199 | 238 | ||
| 239 | + | if resp.ContentLength > maxBytes { | |
| 240 | + | return "", 0, fmt.Errorf("filter file %q too large: content length %d exceeds limit %d", safeFilename, resp.ContentLength, maxBytes) | |
| 241 | + | } | |
| 242 | + | ||
| 200 | 243 | // Write to temp file in cacheDir, then atomic rename into destination. | |
| 201 | - | tmpFile, err := os.CreateTemp(d.cacheDir, filepath.Base(filename)+".*.tmp") | |
| 244 | + | tmpFile, err := os.CreateTemp(d.cacheDir, filepath.Base(safeFilename)+".*.tmp") | |
| 202 | 245 | if err != nil { | |
| 203 | 246 | return "", 0, fmt.Errorf("failed to create temp file: %w", err) | |
| 204 | 247 | } | |
| 205 | 248 | tmpPath := tmpFile.Name() | |
| 206 | 249 | ||
| 207 | - | bytesWritten, err := io.Copy(tmpFile, resp.Body) | |
| 250 | + | bytesWritten, err := copyLimited(tmpFile, resp.Body, maxBytes, fmt.Sprintf("filter file %q", safeFilename)) | |
| 208 | 251 | if err != nil { | |
| 209 | 252 | _ = tmpFile.Close() | |
| 210 | 253 | _ = os.Remove(tmpPath) | |
| 211 | - | return "", 0, fmt.Errorf("failed to write %s: %w", filename, err) | |
| 254 | + | return "", 0, fmt.Errorf("failed to write %s: %w", safeFilename, err) | |
| 212 | 255 | } | |
| 213 | 256 | if err := tmpFile.Close(); err != nil { | |
| 214 | 257 | _ = os.Remove(tmpPath) | |
| 215 | 258 | return "", 0, fmt.Errorf("failed to close temp file: %w", err) | |
| 216 | 259 | } | |
| 217 | 260 | ||
| 218 | - | if err := d.renameTempFile(tmpPath, localPath); err != nil { | |
| 261 | + | if err := d.renameTempFile(tmpPath, safeFilename); err != nil { | |
| 219 | 262 | _ = os.Remove(tmpPath) | |
| 220 | 263 | return "", 0, err | |
| 221 | 264 | } | |
| @@ -223,14 +266,88 @@ func (d *Downloader) downloadFile(ctx context.Context, filename string) (string, | |||
| 223 | 266 | return localPath, bytesWritten, nil | |
| 224 | 267 | } | |
| 225 | 268 | ||
| 226 | - | func (*Downloader) renameTempFile(tmpPath, localPath string) error { | |
| 227 | - | parentDir := filepath.Dir(localPath) | |
| 269 | + | func (d *Downloader) cachePathForFilterFile(filename string) (string, string, error) { | |
| 270 | + | safeFilename, err := validateFilterFilename(filename) | |
| 271 | + | if err != nil { | |
| 272 | + | return "", "", err | |
| 273 | + | } | |
| 274 | + | ||
| 275 | + | cacheDirAbs, err := filepath.Abs(d.cacheDir) | |
| 276 | + | if err != nil { | |
| 277 | + | return "", "", fmt.Errorf("failed to resolve cache dir: %w", err) | |
| 278 | + | } | |
| 279 | + | localPath := filepath.Join(d.cacheDir, filepath.FromSlash(safeFilename)) | |
| 280 | + | targetAbs, err := filepath.Abs(localPath) | |
| 281 | + | if err != nil { | |
| 282 | + | return "", "", fmt.Errorf("failed to resolve cache path: %w", err) | |
| 283 | + | } | |
| 284 | + | if err := ensurePathInside(cacheDirAbs, targetAbs); err != nil { | |
| 285 | + | return "", "", fmt.Errorf("invalid filter filename %q: %w", filename, err) | |
| 286 | + | } | |
| 287 | + | ||
| 288 | + | return localPath, safeFilename, nil | |
| 289 | + | } | |
| 290 | + | ||
| 291 | + | func validateFilterFilename(filename string) (string, error) { | |
| 292 | + | if filename == "" { | |
| 293 | + | return "", fmt.Errorf("invalid filter filename: empty") | |
| 294 | + | } | |
| 295 | + | if strings.TrimSpace(filename) != filename { | |
| 296 | + | return "", fmt.Errorf("invalid filter filename %q: surrounding whitespace is not allowed", filename) | |
| 297 | + | } | |
| 298 | + | if filepath.IsAbs(filename) || strings.HasPrefix(filename, "/") { | |
| 299 | + | return "", fmt.Errorf("invalid filter filename %q: absolute paths are not allowed", filename) | |
| 300 | + | } | |
| 301 | + | if strings.Contains(filename, "\\") { | |
| 302 | + | return "", fmt.Errorf("invalid filter filename %q: backslashes are not allowed", filename) | |
| 303 | + | } | |
| 304 | + | if strings.ContainsAny(filename, "?#") { | |
| 305 | + | return "", fmt.Errorf("invalid filter filename %q: URL control characters are not allowed", filename) | |
| 306 | + | } | |
| 228 | 307 | ||
| 229 | - | if err := os.MkdirAll(parentDir, cacheDirPerm); err != nil { | |
| 230 | - | return fmt.Errorf("failed to create target dir: %w", err) | |
| 308 | + | parts := strings.Split(filename, "/") | |
| 309 | + | for _, part := range parts { | |
| 310 | + | if part == "" || part == "." || part == ".." { | |
| 311 | + | return "", fmt.Errorf("invalid filter filename %q: path traversal components are not allowed", filename) | |
| 312 | + | } | |
| 313 | + | } | |
| 314 | + | ||
| 315 | + | clean := path.Clean(filename) | |
| 316 | + | if path.Ext(clean) != ".json" { | |
| 317 | + | return "", fmt.Errorf("invalid filter filename %q: expected .json extension", filename) | |
| 318 | + | } | |
| 319 | + | ||
| 320 | + | return clean, nil | |
| 321 | + | } | |
| 322 | + | ||
| 323 | + | func ensurePathInside(baseAbs, targetAbs string) error { | |
| 324 | + | rel, err := filepath.Rel(baseAbs, targetAbs) | |
| 325 | + | if err != nil { | |
| 326 | + | return fmt.Errorf("failed to compare with cache dir: %w", err) | |
| 327 | + | } | |
| 328 | + | if rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { | |
| 329 | + | return fmt.Errorf("resolved path escapes cache dir") | |
| 330 | + | } | |
| 331 | + | return nil | |
| 332 | + | } | |
| 333 | + | ||
| 334 | + | func (d *Downloader) renameTempFile(tmpPath, safeFilename string) error { | |
| 335 | + | relTarget := filepath.FromSlash(safeFilename) | |
| 336 | + | parentDir := filepath.Dir(relTarget) | |
| 337 | + | ||
| 338 | + | root, err := os.OpenRoot(d.cacheDir) | |
| 339 | + | if err != nil { | |
| 340 | + | return fmt.Errorf("failed to open cache root: %w", err) | |
| 341 | + | } | |
| 342 | + | defer func() { _ = root.Close() }() | |
| 343 | + | ||
| 344 | + | if parentDir != "." { | |
| 345 | + | if err := root.MkdirAll(parentDir, cacheDirPerm); err != nil { | |
| 346 | + | return fmt.Errorf("failed to create target dir: %w", err) | |
| 347 | + | } | |
| 231 | 348 | } | |
| 232 | 349 | ||
| 233 | - | if err := os.Rename(tmpPath, localPath); err != nil { | |
| 350 | + | if err := root.Rename(filepath.Base(tmpPath), relTarget); err != nil { | |
| 234 | 351 | return fmt.Errorf("failed to rename temp file: %w", err) | |
| 235 | 352 | } | |
| 236 | 353 | ||
| @@ -239,6 +356,10 @@ func (*Downloader) renameTempFile(tmpPath, localPath string) error { | |||
| 239 | 356 | ||
| 240 | 357 | // cacheManifest writes an already-fetched manifest to disk. | |
| 241 | 358 | func (d *Downloader) cacheManifest(manifest *Manifest) error { | |
| 359 | + | if err := d.validateManifest(manifest); err != nil { | |
| 360 | + | return fmt.Errorf("invalid manifest: %w", err) | |
| 361 | + | } | |
| 362 | + | ||
| 242 | 363 | data, err := json.MarshalIndent(manifest, "", " ") | |
| 243 | 364 | if err != nil { | |
| 244 | 365 | return fmt.Errorf("failed to marshal manifest: %w", err) | |
| @@ -262,8 +383,11 @@ func (d *Downloader) GetCachedFilterPaths() []string { | |||
| 262 | 383 | } | |
| 263 | 384 | paths := make([]string, 0, len(manifest.Combined.Files)) | |
| 264 | 385 | for _, filename := range manifest.Combined.Files { | |
| 265 | - | path := filepath.Join(d.cacheDir, filename) | |
| 266 | - | if _, err := os.Stat(path); err != nil { | |
| 386 | + | path, safeFilename, err := d.cachePathForFilterFile(filename) | |
| 387 | + | if err != nil { | |
| 388 | + | return nil | |
| 389 | + | } | |
| 390 | + | if !d.hasCachedFilterFile(safeFilename) { | |
| 267 | 391 | return nil | |
| 268 | 392 | } | |
| 269 | 393 | paths = append(paths, path) | |
| @@ -314,6 +438,94 @@ func (d *Downloader) ClearCache() error { | |||
| 314 | 438 | return os.RemoveAll(d.cacheDir) | |
| 315 | 439 | } | |
| 316 | 440 | ||
| 441 | + | func (d *Downloader) hasCachedFilterFile(safeFilename string) bool { | |
| 442 | + | root, err := os.OpenRoot(d.cacheDir) | |
| 443 | + | if err != nil { | |
| 444 | + | return false | |
| 445 | + | } | |
| 446 | + | defer func() { _ = root.Close() }() | |
| 447 | + | ||
| 448 | + | info, err := root.Stat(filepath.FromSlash(safeFilename)) | |
| 449 | + | return err == nil && !info.IsDir() | |
| 450 | + | } | |
| 451 | + | ||
| 452 | + | func (d *Downloader) validateManifest(manifest *Manifest) error { | |
| 453 | + | if manifest == nil { | |
| 454 | + | return fmt.Errorf("manifest is nil") | |
| 455 | + | } | |
| 456 | + | if len(manifest.Combined.Files) > d.limits.maxFilterFiles { | |
| 457 | + | return fmt.Errorf("manifest lists %d combined filter files, limit is %d", len(manifest.Combined.Files), d.limits.maxFilterFiles) | |
| 458 | + | } | |
| 459 | + | for _, filename := range manifest.Combined.Files { | |
| 460 | + | if _, _, err := d.cachePathForFilterFile(filename); err != nil { | |
| 461 | + | return err | |
| 462 | + | } | |
| 463 | + | } | |
| 464 | + | return nil | |
| 465 | + | } | |
| 466 | + | ||
| 467 | + | func readLimitedFile(filename string, limit int64, label string) ([]byte, error) { | |
| 468 | + | file, err := os.Open(filename) | |
| 469 | + | if err != nil { | |
| 470 | + | return nil, err | |
| 471 | + | } | |
| 472 | + | defer func() { _ = file.Close() }() | |
| 473 | + | ||
| 474 | + | info, err := file.Stat() | |
| 475 | + | if err != nil { | |
| 476 | + | return nil, err | |
| 477 | + | } | |
| 478 | + | if info.Size() > limit { | |
| 479 | + | return nil, fmt.Errorf("%s too large: size %d exceeds limit %d", label, info.Size(), limit) | |
| 480 | + | } | |
| 481 | + | ||
| 482 | + | return readLimitedReader(file, limit, label) | |
| 483 | + | } | |
| 484 | + | ||
| 485 | + | func readLimitedResponseBody(resp *http.Response, limit int64, label string) ([]byte, error) { | |
| 486 | + | if resp.ContentLength > limit { | |
| 487 | + | return nil, fmt.Errorf("%s too large: content length %d exceeds limit %d", label, resp.ContentLength, limit) | |
| 488 | + | } | |
| 489 | + | return readLimitedReader(resp.Body, limit, label) | |
| 490 | + | } | |
| 491 | + | ||
| 492 | + | func readLimitedReader(r io.Reader, limit int64, label string) ([]byte, error) { | |
| 493 | + | if limit <= 0 { | |
| 494 | + | return nil, fmt.Errorf("%s size limit must be positive", label) | |
| 495 | + | } | |
| 496 | + | ||
| 497 | + | data, err := io.ReadAll(io.LimitReader(r, limit+1)) | |
| 498 | + | if err != nil { | |
| 499 | + | return nil, err | |
| 500 | + | } | |
| 501 | + | if int64(len(data)) > limit { | |
| 502 | + | return nil, fmt.Errorf("%s too large: exceeds limit %d bytes", label, limit) | |
| 503 | + | } | |
| 504 | + | return data, nil | |
| 505 | + | } | |
| 506 | + | ||
| 507 | + | func copyLimited(dst io.Writer, src io.Reader, limit int64, label string) (int64, error) { | |
| 508 | + | if limit <= 0 { | |
| 509 | + | return 0, fmt.Errorf("%s size limit must be positive", label) | |
| 510 | + | } | |
| 511 | + | ||
| 512 | + | written, err := io.Copy(dst, io.LimitReader(src, limit+1)) | |
| 513 | + | if err != nil { | |
| 514 | + | return written, err | |
| 515 | + | } | |
| 516 | + | if written > limit { | |
| 517 | + | return written, fmt.Errorf("%s too large: exceeds limit %d bytes", label, limit) | |
| 518 | + | } | |
| 519 | + | return written, nil | |
| 520 | + | } | |
| 521 | + | ||
| 522 | + | func minInt64(a, b int64) int64 { | |
| 523 | + | if a < b { | |
| 524 | + | return a | |
| 525 | + | } | |
| 526 | + | return b | |
| 527 | + | } | |
| 528 | + | ||
| 317 | 529 | // IsCacheStale checks if the cached manifest is older than maxAge. | |
| 318 | 530 | // Returns true if cache is stale or doesn't exist. | |
| 319 | 531 | func (d *Downloader) IsCacheStale(maxAge time.Duration) bool { | |
| @@ -7,7 +7,9 @@ import ( | |||||
| 7 | "io" | 7 | "io" | ||
| 8 | "net/http" | 8 | "net/http" | ||
| 9 | "os" | 9 | "os" | ||
| 10 | + | "path" | |||
| 10 | "path/filepath" | 11 | "path/filepath" | ||
| 12 | + | "strings" | |||
| 11 | "time" | 13 | "time" | ||
| 12 | 14 | ||||
| 13 | "github.com/bnema/dumber/internal/logging" | 15 | "github.com/bnema/dumber/internal/logging" | ||
| @@ -16,13 +18,34 @@ import ( | |||||
| 16 | const ( | 18 | const ( | ||
| 17 | cacheDirPerm = 0o755 | 19 | cacheDirPerm = 0o755 | ||
| 18 | manifestFilePerm = 0o644 | 20 | manifestFilePerm = 0o644 | ||
| 21 | + | ||||
| 22 | + | defaultMaxManifestBytes int64 = 1 << 20 // 1 MiB | |||
| 23 | + | defaultMaxFilterFiles = 128 | |||
| 24 | + | defaultMaxFilterFileBytes int64 = 64 << 20 // 64 MiB | |||
| 25 | + | defaultMaxTotalFilterBytes int64 = 512 << 20 // 512 MiB | |||
| 19 | ) | 26 | ) | ||
| 20 | 27 | ||||
| 28 | + | // downloadLimits bounds remote filter metadata and payloads. | |||
| 29 | + | type downloadLimits struct { | |||
| 30 | + | maxManifestBytes int64 | |||
| 31 | + | maxFilterFiles int | |||
| 32 | + | maxFilterFileBytes int64 | |||
| 33 | + | maxTotalFilterBytes int64 | |||
| 34 | + | } | |||
| 35 | + | ||||
| 36 | + | var defaultDownloadLimits = downloadLimits{ | |||
| 37 | + | maxManifestBytes: defaultMaxManifestBytes, | |||
| 38 | + | maxFilterFiles: defaultMaxFilterFiles, | |||
| 39 | + | maxFilterFileBytes: defaultMaxFilterFileBytes, | |||
| 40 | + | maxTotalFilterBytes: defaultMaxTotalFilterBytes, | |||
| 41 | + | } | |||
| 42 | + | ||||
| 21 | // Downloader handles downloading filter files from GitHub releases. | 43 | // Downloader handles downloading filter files from GitHub releases. | ||
| 22 | type Downloader struct { | 44 | type Downloader struct { | ||
| 23 | baseURL string | 45 | baseURL string | ||
| 24 | cacheDir string | 46 | cacheDir string | ||
| 25 | httpClient *http.Client | 47 | httpClient *http.Client | ||
| 48 | + | limits downloadLimits | |||
| 26 | } | 49 | } | ||
| 27 | 50 | ||||
| 28 | // NewDownloader creates a new Downloader. | 51 | // NewDownloader creates a new Downloader. | ||
| @@ -34,14 +57,15 @@ func NewDownloader(cacheDir string) *Downloader { | |||||
| 34 | httpClient: &http.Client{ | 57 | httpClient: &http.Client{ | ||
| 35 | Timeout: 60 * time.Second, | 58 | Timeout: 60 * time.Second, | ||
| 36 | }, | 59 | }, | ||
| 60 | + | limits: defaultDownloadLimits, | |||
| 37 | } | 61 | } | ||
| 38 | } | 62 | } | ||
| 39 | 63 | ||||
| 40 | // GetCachedManifest reads the locally cached manifest.json. | 64 | // GetCachedManifest reads the locally cached manifest.json. | ||
| 41 | // Returns nil if no cached manifest exists. | 65 | // Returns nil if no cached manifest exists. | ||
| 42 | func (d *Downloader) GetCachedManifest() (*Manifest, error) { | 66 | func (d *Downloader) GetCachedManifest() (*Manifest, error) { | ||
| 43 | - | path := filepath.Join(d.cacheDir, FilterFiles.Manifest) | 67 | + | manifestPath := filepath.Join(d.cacheDir, FilterFiles.Manifest) |
| 44 | - | data, err := os.ReadFile(path) | 68 | + | data, err := readLimitedFile(manifestPath, d.limits.maxManifestBytes, "cached manifest") |
| 45 | if err != nil { | 69 | if err != nil { | ||
| 46 | if os.IsNotExist(err) { | 70 | if os.IsNotExist(err) { | ||
| 47 | return nil, nil | 71 | return nil, nil | ||
| @@ -53,6 +77,9 @@ func (d *Downloader) GetCachedManifest() (*Manifest, error) { | |||||
| 53 | if err := json.Unmarshal(data, &manifest); err != nil { | 77 | if err := json.Unmarshal(data, &manifest); err != nil { | ||
| 54 | return nil, fmt.Errorf("failed to parse cached manifest: %w", err) | 78 | return nil, fmt.Errorf("failed to parse cached manifest: %w", err) | ||
| 55 | } | 79 | } | ||
| 80 | + | if err := d.validateManifest(&manifest); err != nil { | |||
| 81 | + | return nil, fmt.Errorf("invalid cached manifest: %w", err) | |||
| 82 | + | } | |||
| 56 | return &manifest, nil | 83 | return &manifest, nil | ||
| 57 | } | 84 | } | ||
| 58 | 85 | ||||
| @@ -84,7 +111,7 @@ func (d *Downloader) FetchManifest(ctx context.Context) (*Manifest, error) { | |||||
| 84 | return nil, fmt.Errorf("manifest fetch failed with status %d", resp.StatusCode) | 111 | return nil, fmt.Errorf("manifest fetch failed with status %d", resp.StatusCode) | ||
| 85 | } | 112 | } | ||
| 86 | 113 | ||||
| 87 | - | data, err := io.ReadAll(resp.Body) | 114 | + | data, err := readLimitedResponseBody(resp, d.limits.maxManifestBytes, "manifest") |
| 88 | if err != nil { | 115 | if err != nil { | ||
| 89 | return nil, fmt.Errorf("failed to read manifest body: %w", err) | 116 | return nil, fmt.Errorf("failed to read manifest body: %w", err) | ||
| 90 | } | 117 | } | ||
| @@ -93,6 +120,9 @@ func (d *Downloader) FetchManifest(ctx context.Context) (*Manifest, error) { | |||||
| 93 | if err := json.Unmarshal(data, &manifest); err != nil { | 120 | if err := json.Unmarshal(data, &manifest); err != nil { | ||
| 94 | return nil, fmt.Errorf("failed to parse manifest: %w", err) | 121 | return nil, fmt.Errorf("failed to parse manifest: %w", err) | ||
| 95 | } | 122 | } | ||
| 123 | + | if err := d.validateManifest(&manifest); err != nil { | |||
| 124 | + | return nil, fmt.Errorf("invalid manifest: %w", err) | |||
| 125 | + | } | |||
| 96 | 126 | ||||
| 97 | log.Debug().Str("version", manifest.Version).Msg("fetched manifest") | 127 | log.Debug().Str("version", manifest.Version).Msg("fetched manifest") | ||
| 98 | return &manifest, nil | 128 | return &manifest, nil | ||
| @@ -153,7 +183,12 @@ func (d *Downloader) DownloadFilters(ctx context.Context, onProgress func(Downlo | |||||
| 153 | }) | 183 | }) | ||
| 154 | } | 184 | } | ||
| 155 | 185 | ||||
| 156 | - | path, bytesDownloaded, err := d.downloadFile(ctx, filename) | 186 | + | remainingBytes := d.limits.maxTotalFilterBytes - totalBytes |
| 187 | + | if remainingBytes <= 0 { | |||
| 188 | + | return nil, fmt.Errorf("filter downloads exceed total size limit of %d bytes", d.limits.maxTotalFilterBytes) | |||
| 189 | + | } | |||
| 190 | + | ||||
| 191 | + | path, bytesDownloaded, err := d.downloadFile(ctx, filename, minInt64(d.limits.maxFilterFileBytes, remainingBytes)) | |||
| 157 | if err != nil { | 192 | if err != nil { | ||
| 158 | log.Error().Err(err).Str("file", filename).Msg("failed to download filter file") | 193 | log.Error().Err(err).Str("file", filename).Msg("failed to download filter file") | ||
| 159 | return nil, err | 194 | return nil, err | ||
| @@ -174,9 +209,13 @@ func (d *Downloader) DownloadFilters(ctx context.Context, onProgress func(Downlo | |||||
| 174 | } | 209 | } | ||
| 175 | 210 | ||||
| 176 | // downloadFile downloads a single file and returns its local path. | 211 | // downloadFile downloads a single file and returns its local path. | ||
| 177 | - | func (d *Downloader) downloadFile(ctx context.Context, filename string) (string, int64, error) { | 212 | + | func (d *Downloader) downloadFile(ctx context.Context, filename string, maxBytes int64) (string, int64, error) { |
| 178 | - | url := fmt.Sprintf("%s/%s", d.baseURL, filename) | 213 | + | localPath, safeFilename, err := d.cachePathForFilterFile(filename) |
| 179 | - | localPath := filepath.Join(d.cacheDir, filename) | 214 | + | if err != nil { |
| 215 | + | return "", 0, err | |||
| 216 | + | } | |||
| 217 | + | ||||
| 218 | + | url := fmt.Sprintf("%s/%s", d.baseURL, safeFilename) | |||
| 180 | 219 | ||||
| 181 | req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) | 220 | req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) | ||
| 182 | if err != nil { | 221 | if err != nil { | ||
| @@ -197,25 +236,29 @@ func (d *Downloader) downloadFile(ctx context.Context, filename string) (string, | |||||
| 197 | return "", 0, fmt.Errorf("download %s failed with status %d", filename, resp.StatusCode) | 236 | return "", 0, fmt.Errorf("download %s failed with status %d", filename, resp.StatusCode) | ||
| 198 | } | 237 | } | ||
| 199 | 238 | ||||
| 239 | + | if resp.ContentLength > maxBytes { | |||
| 240 | + | return "", 0, fmt.Errorf("filter file %q too large: content length %d exceeds limit %d", safeFilename, resp.ContentLength, maxBytes) | |||
| 241 | + | } | |||
| 242 | + | ||||
| 200 | // Write to temp file in cacheDir, then atomic rename into destination. | 243 | // Write to temp file in cacheDir, then atomic rename into destination. | ||
| 201 | - | tmpFile, err := os.CreateTemp(d.cacheDir, filepath.Base(filename)+".*.tmp") | 244 | + | tmpFile, err := os.CreateTemp(d.cacheDir, filepath.Base(safeFilename)+".*.tmp") |
| 202 | if err != nil { | 245 | if err != nil { | ||
| 203 | return "", 0, fmt.Errorf("failed to create temp file: %w", err) | 246 | return "", 0, fmt.Errorf("failed to create temp file: %w", err) | ||
| 204 | } | 247 | } | ||
| 205 | tmpPath := tmpFile.Name() | 248 | tmpPath := tmpFile.Name() | ||
| 206 | 249 | ||||
| 207 | - | bytesWritten, err := io.Copy(tmpFile, resp.Body) | 250 | + | bytesWritten, err := copyLimited(tmpFile, resp.Body, maxBytes, fmt.Sprintf("filter file %q", safeFilename)) |
| 208 | if err != nil { | 251 | if err != nil { | ||
| 209 | _ = tmpFile.Close() | 252 | _ = tmpFile.Close() | ||
| 210 | _ = os.Remove(tmpPath) | 253 | _ = os.Remove(tmpPath) | ||
| 211 | - | return "", 0, fmt.Errorf("failed to write %s: %w", filename, err) | 254 | + | return "", 0, fmt.Errorf("failed to write %s: %w", safeFilename, err) |
| 212 | } | 255 | } | ||
| 213 | if err := tmpFile.Close(); err != nil { | 256 | if err := tmpFile.Close(); err != nil { | ||
| 214 | _ = os.Remove(tmpPath) | 257 | _ = os.Remove(tmpPath) | ||
| 215 | return "", 0, fmt.Errorf("failed to close temp file: %w", err) | 258 | return "", 0, fmt.Errorf("failed to close temp file: %w", err) | ||
| 216 | } | 259 | } | ||
| 217 | 260 | ||||
| 218 | - | if err := d.renameTempFile(tmpPath, localPath); err != nil { | 261 | + | if err := d.renameTempFile(tmpPath, safeFilename); err != nil { |
| 219 | _ = os.Remove(tmpPath) | 262 | _ = os.Remove(tmpPath) | ||
| 220 | return "", 0, err | 263 | return "", 0, err | ||
| 221 | } | 264 | } | ||
| @@ -223,14 +266,88 @@ func (d *Downloader) downloadFile(ctx context.Context, filename string) (string, | |||||
| 223 | return localPath, bytesWritten, nil | 266 | return localPath, bytesWritten, nil | ||
| 224 | } | 267 | } | ||
| 225 | 268 | ||||
| 226 | - | func (*Downloader) renameTempFile(tmpPath, localPath string) error { | 269 | + | func (d *Downloader) cachePathForFilterFile(filename string) (string, string, error) { |
| 227 | - | parentDir := filepath.Dir(localPath) | 270 | + | safeFilename, err := validateFilterFilename(filename) |
| 271 | + | if err != nil { | |||
| 272 | + | return "", "", err | |||
| 273 | + | } | |||
| 274 | + | ||||
| 275 | + | cacheDirAbs, err := filepath.Abs(d.cacheDir) | |||
| 276 | + | if err != nil { | |||
| 277 | + | return "", "", fmt.Errorf("failed to resolve cache dir: %w", err) | |||
| 278 | + | } | |||
| 279 | + | localPath := filepath.Join(d.cacheDir, filepath.FromSlash(safeFilename)) | |||
| 280 | + | targetAbs, err := filepath.Abs(localPath) | |||
| 281 | + | if err != nil { | |||
| 282 | + | return "", "", fmt.Errorf("failed to resolve cache path: %w", err) | |||
| 283 | + | } | |||
| 284 | + | if err := ensurePathInside(cacheDirAbs, targetAbs); err != nil { | |||
| 285 | + | return "", "", fmt.Errorf("invalid filter filename %q: %w", filename, err) | |||
| 286 | + | } | |||
| 287 | + | ||||
| 288 | + | return localPath, safeFilename, nil | |||
| 289 | + | } | |||
| 290 | + | ||||
| 291 | + | func validateFilterFilename(filename string) (string, error) { | |||
| 292 | + | if filename == "" { | |||
| 293 | + | return "", fmt.Errorf("invalid filter filename: empty") | |||
| 294 | + | } | |||
| 295 | + | if strings.TrimSpace(filename) != filename { | |||
| 296 | + | return "", fmt.Errorf("invalid filter filename %q: surrounding whitespace is not allowed", filename) | |||
| 297 | + | } | |||
| 298 | + | if filepath.IsAbs(filename) || strings.HasPrefix(filename, "/") { | |||
| 299 | + | return "", fmt.Errorf("invalid filter filename %q: absolute paths are not allowed", filename) | |||
| 300 | + | } | |||
| 301 | + | if strings.Contains(filename, "\\") { | |||
| 302 | + | return "", fmt.Errorf("invalid filter filename %q: backslashes are not allowed", filename) | |||
| 303 | + | } | |||
| 304 | + | if strings.ContainsAny(filename, "?#") { | |||
| 305 | + | return "", fmt.Errorf("invalid filter filename %q: URL control characters are not allowed", filename) | |||
| 306 | + | } | |||
| 228 | 307 | ||||
| 229 | - | if err := os.MkdirAll(parentDir, cacheDirPerm); err != nil { | 308 | + | parts := strings.Split(filename, "/") |
| 230 | - | return fmt.Errorf("failed to create target dir: %w", err) | 309 | + | for _, part := range parts { |
| 310 | + | if part == "" || part == "." || part == ".." { | |||
| 311 | + | return "", fmt.Errorf("invalid filter filename %q: path traversal components are not allowed", filename) | |||
| 312 | + | } | |||
| 313 | + | } | |||
| 314 | + | ||||
| 315 | + | clean := path.Clean(filename) | |||
| 316 | + | if path.Ext(clean) != ".json" { | |||
| 317 | + | return "", fmt.Errorf("invalid filter filename %q: expected .json extension", filename) | |||
| 318 | + | } | |||
| 319 | + | ||||
| 320 | + | return clean, nil | |||
| 321 | + | } | |||
| 322 | + | ||||
| 323 | + | func ensurePathInside(baseAbs, targetAbs string) error { | |||
| 324 | + | rel, err := filepath.Rel(baseAbs, targetAbs) | |||
| 325 | + | if err != nil { | |||
| 326 | + | return fmt.Errorf("failed to compare with cache dir: %w", err) | |||
| 327 | + | } | |||
| 328 | + | if rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { | |||
| 329 | + | return fmt.Errorf("resolved path escapes cache dir") | |||
| 330 | + | } | |||
| 331 | + | return nil | |||
| 332 | + | } | |||
| 333 | + | ||||
| 334 | + | func (d *Downloader) renameTempFile(tmpPath, safeFilename string) error { | |||
| 335 | + | relTarget := filepath.FromSlash(safeFilename) | |||
| 336 | + | parentDir := filepath.Dir(relTarget) | |||
| 337 | + | ||||
| 338 | + | root, err := os.OpenRoot(d.cacheDir) | |||
| 339 | + | if err != nil { | |||
| 340 | + | return fmt.Errorf("failed to open cache root: %w", err) | |||
| 341 | + | } | |||
| 342 | + | defer func() { _ = root.Close() }() | |||
| 343 | + | ||||
| 344 | + | if parentDir != "." { | |||
| 345 | + | if err := root.MkdirAll(parentDir, cacheDirPerm); err != nil { | |||
| 346 | + | return fmt.Errorf("failed to create target dir: %w", err) | |||
| 347 | + | } | |||
| 231 | } | 348 | } | ||
| 232 | 349 | ||||
| 233 | - | if err := os.Rename(tmpPath, localPath); err != nil { | 350 | + | if err := root.Rename(filepath.Base(tmpPath), relTarget); err != nil { |
| 234 | return fmt.Errorf("failed to rename temp file: %w", err) | 351 | return fmt.Errorf("failed to rename temp file: %w", err) | ||
| 235 | } | 352 | } | ||
| 236 | 353 | ||||
| @@ -239,6 +356,10 @@ func (*Downloader) renameTempFile(tmpPath, localPath string) error { | |||||
| 239 | 356 | ||||
| 240 | // cacheManifest writes an already-fetched manifest to disk. | 357 | // cacheManifest writes an already-fetched manifest to disk. | ||
| 241 | func (d *Downloader) cacheManifest(manifest *Manifest) error { | 358 | func (d *Downloader) cacheManifest(manifest *Manifest) error { | ||
| 359 | + | if err := d.validateManifest(manifest); err != nil { | |||
| 360 | + | return fmt.Errorf("invalid manifest: %w", err) | |||
| 361 | + | } | |||
| 362 | + | ||||
| 242 | data, err := json.MarshalIndent(manifest, "", " ") | 363 | data, err := json.MarshalIndent(manifest, "", " ") | ||
| 243 | if err != nil { | 364 | if err != nil { | ||
| 244 | return fmt.Errorf("failed to marshal manifest: %w", err) | 365 | return fmt.Errorf("failed to marshal manifest: %w", err) | ||
| @@ -262,8 +383,11 @@ func (d *Downloader) GetCachedFilterPaths() []string { | |||||
| 262 | } | 383 | } | ||
| 263 | paths := make([]string, 0, len(manifest.Combined.Files)) | 384 | paths := make([]string, 0, len(manifest.Combined.Files)) | ||
| 264 | for _, filename := range manifest.Combined.Files { | 385 | for _, filename := range manifest.Combined.Files { | ||
| 265 | - | path := filepath.Join(d.cacheDir, filename) | 386 | + | path, safeFilename, err := d.cachePathForFilterFile(filename) |
| 266 | - | if _, err := os.Stat(path); err != nil { | 387 | + | if err != nil { |
| 388 | + | return nil | |||
| 389 | + | } | |||
| 390 | + | if !d.hasCachedFilterFile(safeFilename) { | |||
| 267 | return nil | 391 | return nil | ||
| 268 | } | 392 | } | ||
| 269 | paths = append(paths, path) | 393 | paths = append(paths, path) | ||
| @@ -314,6 +438,94 @@ func (d *Downloader) ClearCache() error { | |||||
| 314 | return os.RemoveAll(d.cacheDir) | 438 | return os.RemoveAll(d.cacheDir) | ||
| 315 | } | 439 | } | ||
| 316 | 440 | ||||
| 441 | + | func (d *Downloader) hasCachedFilterFile(safeFilename string) bool { | |||
| 442 | + | root, err := os.OpenRoot(d.cacheDir) | |||
| 443 | + | if err != nil { | |||
| 444 | + | return false | |||
| 445 | + | } | |||
| 446 | + | defer func() { _ = root.Close() }() | |||
| 447 | + | ||||
| 448 | + | info, err := root.Stat(filepath.FromSlash(safeFilename)) | |||
| 449 | + | return err == nil && !info.IsDir() | |||
| 450 | + | } | |||
| 451 | + | ||||
| 452 | + | func (d *Downloader) validateManifest(manifest *Manifest) error { | |||
| 453 | + | if manifest == nil { | |||
| 454 | + | return fmt.Errorf("manifest is nil") | |||
| 455 | + | } | |||
| 456 | + | if len(manifest.Combined.Files) > d.limits.maxFilterFiles { | |||
| 457 | + | return fmt.Errorf("manifest lists %d combined filter files, limit is %d", len(manifest.Combined.Files), d.limits.maxFilterFiles) | |||
| 458 | + | } | |||
| 459 | + | for _, filename := range manifest.Combined.Files { | |||
| 460 | + | if _, _, err := d.cachePathForFilterFile(filename); err != nil { | |||
| 461 | + | return err | |||
| 462 | + | } | |||
| 463 | + | } | |||
| 464 | + | return nil | |||
| 465 | + | } | |||
| 466 | + | ||||
| 467 | + | func readLimitedFile(filename string, limit int64, label string) ([]byte, error) { | |||
| 468 | + | file, err := os.Open(filename) | |||
| 469 | + | if err != nil { | |||
| 470 | + | return nil, err | |||
| 471 | + | } | |||
| 472 | + | defer func() { _ = file.Close() }() | |||
| 473 | + | ||||
| 474 | + | info, err := file.Stat() | |||
| 475 | + | if err != nil { | |||
| 476 | + | return nil, err | |||
| 477 | + | } | |||
| 478 | + | if info.Size() > limit { | |||
| 479 | + | return nil, fmt.Errorf("%s too large: size %d exceeds limit %d", label, info.Size(), limit) | |||
| 480 | + | } | |||
| 481 | + | ||||
| 482 | + | return readLimitedReader(file, limit, label) | |||
| 483 | + | } | |||
| 484 | + | ||||
| 485 | + | func readLimitedResponseBody(resp *http.Response, limit int64, label string) ([]byte, error) { | |||
| 486 | + | if resp.ContentLength > limit { | |||
| 487 | + | return nil, fmt.Errorf("%s too large: content length %d exceeds limit %d", label, resp.ContentLength, limit) | |||
| 488 | + | } | |||
| 489 | + | return readLimitedReader(resp.Body, limit, label) | |||
| 490 | + | } | |||
| 491 | + | ||||
| 492 | + | func readLimitedReader(r io.Reader, limit int64, label string) ([]byte, error) { | |||
| 493 | + | if limit <= 0 { | |||
| 494 | + | return nil, fmt.Errorf("%s size limit must be positive", label) | |||
| 495 | + | } | |||
| 496 | + | ||||
| 497 | + | data, err := io.ReadAll(io.LimitReader(r, limit+1)) | |||
| 498 | + | if err != nil { | |||
| 499 | + | return nil, err | |||
| 500 | + | } | |||
| 501 | + | if int64(len(data)) > limit { | |||
| 502 | + | return nil, fmt.Errorf("%s too large: exceeds limit %d bytes", label, limit) | |||
| 503 | + | } | |||
| 504 | + | return data, nil | |||
| 505 | + | } | |||
| 506 | + | ||||
| 507 | + | func copyLimited(dst io.Writer, src io.Reader, limit int64, label string) (int64, error) { | |||
| 508 | + | if limit <= 0 { | |||
| 509 | + | return 0, fmt.Errorf("%s size limit must be positive", label) | |||
| 510 | + | } | |||
| 511 | + | ||||
| 512 | + | written, err := io.Copy(dst, io.LimitReader(src, limit+1)) | |||
| 513 | + | if err != nil { | |||
| 514 | + | return written, err | |||
| 515 | + | } | |||
| 516 | + | if written > limit { | |||
| 517 | + | return written, fmt.Errorf("%s too large: exceeds limit %d bytes", label, limit) | |||
| 518 | + | } | |||
| 519 | + | return written, nil | |||
| 520 | + | } | |||
| 521 | + | ||||
| 522 | + | func minInt64(a, b int64) int64 { | |||
| 523 | + | if a < b { | |||
| 524 | + | return a | |||
| 525 | + | } | |||
| 526 | + | return b | |||
| 527 | + | } | |||
| 528 | + | ||||
| 317 | // IsCacheStale checks if the cached manifest is older than maxAge. | 529 | // IsCacheStale checks if the cached manifest is older than maxAge. | ||
| 318 | // Returns true if cache is stale or doesn't exist. | 530 | // Returns true if cache is stale or doesn't exist. | ||
| 319 | func (d *Downloader) IsCacheStale(maxAge time.Duration) bool { | 531 | func (d *Downloader) IsCacheStale(maxAge time.Duration) bool { | ||
M internal/infrastructure/filtering/downloader_test.go +202 -3
| @@ -2,11 +2,13 @@ package filtering | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | + | "encoding/json" | |
| 5 | 6 | "io" | |
| 6 | 7 | "net/http" | |
| 7 | 8 | "net/http/httptest" | |
| 8 | 9 | "os" | |
| 9 | 10 | "path/filepath" | |
| 11 | + | "strings" | |
| 10 | 12 | "testing" | |
| 11 | 13 | ||
| 12 | 14 | "github.com/stretchr/testify/require" | |
| @@ -46,7 +48,7 @@ func TestDownloader_DownloadFile_RecoversWhenTargetDirDeletedConcurrently(t *tes | |||
| 46 | 48 | } | |
| 47 | 49 | resultCh := make(chan result, 1) | |
| 48 | 50 | go func() { | |
| 49 | - | path, n, err := d.downloadFile(context.Background(), filename) | |
| 51 | + | path, n, err := d.downloadFile(context.Background(), filename, d.limits.maxFilterFileBytes) | |
| 50 | 52 | resultCh <- result{path: path, n: n, err: err} | |
| 51 | 53 | }() | |
| 52 | 54 | ||
| @@ -64,6 +66,203 @@ func TestDownloader_DownloadFile_RecoversWhenTargetDirDeletedConcurrently(t *tes | |||
| 64 | 66 | require.Contains(t, string(content), "url-filter") | |
| 65 | 67 | } | |
| 66 | 68 | ||
| 69 | + | func TestDownloader_DownloadFilters_RejectsUnsafeManifestFilenames(t *testing.T) { | |
| 70 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |
| 71 | + | requested := make([]string, 0, 1) | |
| 72 | + | ||
| 73 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 74 | + | requested = append(requested, r.URL.Path) | |
| 75 | + | if r.URL.Path != "/"+FilterFiles.Manifest { | |
| 76 | + | http.NotFound(w, r) | |
| 77 | + | return | |
| 78 | + | } | |
| 79 | + | _, _ = io.WriteString(w, `{"version":"test","combined":{"total_rules":1,"files":["../escape.json"]}}`) | |
| 80 | + | })) | |
| 81 | + | defer server.Close() | |
| 82 | + | ||
| 83 | + | d := NewDownloader(cacheDir) | |
| 84 | + | d.baseURL = server.URL | |
| 85 | + | ||
| 86 | + | paths, err := d.DownloadFilters(context.Background(), nil) | |
| 87 | + | require.Error(t, err) | |
| 88 | + | require.Nil(t, paths) | |
| 89 | + | require.Contains(t, err.Error(), "invalid manifest") | |
| 90 | + | require.Equal(t, []string{"/" + FilterFiles.Manifest}, requested) | |
| 91 | + | } | |
| 92 | + | ||
| 93 | + | func TestDownloader_DownloadFilters_AllowsSafeNestedManifestFilenames(t *testing.T) { | |
| 94 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |
| 95 | + | filename := "nested/combined-part1.json" | |
| 96 | + | body := `[{"trigger":{"url-filter":"test"},"action":{"type":"block"}}]` | |
| 97 | + | ||
| 98 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 99 | + | switch r.URL.Path { | |
| 100 | + | case "/" + FilterFiles.Manifest: | |
| 101 | + | _, _ = io.WriteString(w, `{"version":"test","combined":{"total_rules":1,"files":["`+filename+`"]}}`) | |
| 102 | + | case "/" + filename: | |
| 103 | + | _, _ = io.WriteString(w, body) | |
| 104 | + | default: | |
| 105 | + | http.NotFound(w, r) | |
| 106 | + | } | |
| 107 | + | })) | |
| 108 | + | defer server.Close() | |
| 109 | + | ||
| 110 | + | d := NewDownloader(cacheDir) | |
| 111 | + | d.baseURL = server.URL | |
| 112 | + | ||
| 113 | + | paths, err := d.DownloadFilters(context.Background(), nil) | |
| 114 | + | require.NoError(t, err) | |
| 115 | + | ||
| 116 | + | wantPath := filepath.Join(cacheDir, "nested", "combined-part1.json") | |
| 117 | + | require.Equal(t, []string{wantPath}, paths) | |
| 118 | + | content, err := os.ReadFile(wantPath) | |
| 119 | + | require.NoError(t, err) | |
| 120 | + | require.Equal(t, body, string(content)) | |
| 121 | + | } | |
| 122 | + | ||
| 123 | + | func TestDownloader_FetchManifest_RejectsOversizedManifest(t *testing.T) { | |
| 124 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 125 | + | require.Equal(t, "/"+FilterFiles.Manifest, r.URL.Path) | |
| 126 | + | _, _ = io.WriteString(w, strings.Repeat("x", 17)) | |
| 127 | + | })) | |
| 128 | + | defer server.Close() | |
| 129 | + | ||
| 130 | + | d := NewDownloader(t.TempDir()) | |
| 131 | + | d.baseURL = server.URL | |
| 132 | + | d.limits.maxManifestBytes = 16 | |
| 133 | + | ||
| 134 | + | manifest, err := d.FetchManifest(context.Background()) | |
| 135 | + | require.Error(t, err) | |
| 136 | + | require.Nil(t, manifest) | |
| 137 | + | require.Contains(t, err.Error(), "manifest too large") | |
| 138 | + | } | |
| 139 | + | ||
| 140 | + | func TestDownloader_FetchManifest_RejectsTooManyFiles(t *testing.T) { | |
| 141 | + | payload, err := json.Marshal(Manifest{ | |
| 142 | + | Version: "test", | |
| 143 | + | Combined: CombinedInfo{ | |
| 144 | + | TotalRules: 3, | |
| 145 | + | Files: []string{"part1.json", "part2.json", "part3.json"}, | |
| 146 | + | }, | |
| 147 | + | }) | |
| 148 | + | require.NoError(t, err) | |
| 149 | + | ||
| 150 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 151 | + | require.Equal(t, "/"+FilterFiles.Manifest, r.URL.Path) | |
| 152 | + | _, _ = w.Write(payload) | |
| 153 | + | })) | |
| 154 | + | defer server.Close() | |
| 155 | + | ||
| 156 | + | d := NewDownloader(t.TempDir()) | |
| 157 | + | d.baseURL = server.URL | |
| 158 | + | d.limits.maxFilterFiles = 2 | |
| 159 | + | ||
| 160 | + | manifest, err := d.FetchManifest(context.Background()) | |
| 161 | + | require.Error(t, err) | |
| 162 | + | require.Nil(t, manifest) | |
| 163 | + | require.Contains(t, err.Error(), "limit is 2") | |
| 164 | + | } | |
| 165 | + | ||
| 166 | + | func TestDownloader_DownloadFilters_RejectsOversizedFilterFile(t *testing.T) { | |
| 167 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |
| 168 | + | filename := "combined.json" | |
| 169 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 170 | + | switch r.URL.Path { | |
| 171 | + | case "/" + FilterFiles.Manifest: | |
| 172 | + | _, _ = io.WriteString(w, `{"version":"test","combined":{"total_rules":1,"files":["combined.json"]}}`) | |
| 173 | + | case "/" + filename: | |
| 174 | + | _, _ = io.WriteString(w, "12345") | |
| 175 | + | default: | |
| 176 | + | http.NotFound(w, r) | |
| 177 | + | } | |
| 178 | + | })) | |
| 179 | + | defer server.Close() | |
| 180 | + | ||
| 181 | + | d := NewDownloader(cacheDir) | |
| 182 | + | d.baseURL = server.URL | |
| 183 | + | d.limits.maxFilterFileBytes = 4 | |
| 184 | + | d.limits.maxTotalFilterBytes = 32 | |
| 185 | + | ||
| 186 | + | paths, err := d.DownloadFilters(context.Background(), nil) | |
| 187 | + | require.Error(t, err) | |
| 188 | + | require.Nil(t, paths) | |
| 189 | + | require.Contains(t, err.Error(), "too large") | |
| 190 | + | require.NoFileExists(t, filepath.Join(cacheDir, filename)) | |
| 191 | + | } | |
| 192 | + | ||
| 193 | + | func TestDownloader_DownloadFilters_EnforcesTotalDownloadLimit(t *testing.T) { | |
| 194 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |
| 195 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 196 | + | switch r.URL.Path { | |
| 197 | + | case "/" + FilterFiles.Manifest: | |
| 198 | + | _, _ = io.WriteString(w, `{"version":"test","combined":{"total_rules":2,"files":["part1.json","part2.json"]}}`) | |
| 199 | + | case "/part1.json", "/part2.json": | |
| 200 | + | _, _ = io.WriteString(w, "12345") | |
| 201 | + | default: | |
| 202 | + | http.NotFound(w, r) | |
| 203 | + | } | |
| 204 | + | })) | |
| 205 | + | defer server.Close() | |
| 206 | + | ||
| 207 | + | d := NewDownloader(cacheDir) | |
| 208 | + | d.baseURL = server.URL | |
| 209 | + | d.limits.maxFilterFileBytes = 8 | |
| 210 | + | d.limits.maxTotalFilterBytes = 8 | |
| 211 | + | ||
| 212 | + | paths, err := d.DownloadFilters(context.Background(), nil) | |
| 213 | + | require.Error(t, err) | |
| 214 | + | require.Nil(t, paths) | |
| 215 | + | require.Contains(t, err.Error(), "too large") | |
| 216 | + | require.FileExists(t, filepath.Join(cacheDir, "part1.json")) | |
| 217 | + | require.NoFileExists(t, filepath.Join(cacheDir, "part2.json")) | |
| 218 | + | } | |
| 219 | + | ||
| 220 | + | func TestDownloader_DownloadFilters_RejectsSymlinkEscape(t *testing.T) { | |
| 221 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |
| 222 | + | outsideDir := filepath.Join(t.TempDir(), "outside") | |
| 223 | + | require.NoError(t, os.MkdirAll(cacheDir, cacheDirPerm)) | |
| 224 | + | require.NoError(t, os.MkdirAll(outsideDir, cacheDirPerm)) | |
| 225 | + | require.NoError(t, os.Symlink(outsideDir, filepath.Join(cacheDir, "nested"))) | |
| 226 | + | ||
| 227 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 228 | + | switch r.URL.Path { | |
| 229 | + | case "/" + FilterFiles.Manifest: | |
| 230 | + | _, _ = io.WriteString(w, `{"version":"test","combined":{"total_rules":1,"files":["nested/combined.json"]}}`) | |
| 231 | + | case "/nested/combined.json": | |
| 232 | + | _, _ = io.WriteString(w, `[{"trigger":{"url-filter":"test"},"action":{"type":"block"}}]`) | |
| 233 | + | default: | |
| 234 | + | http.NotFound(w, r) | |
| 235 | + | } | |
| 236 | + | })) | |
| 237 | + | defer server.Close() | |
| 238 | + | ||
| 239 | + | d := NewDownloader(cacheDir) | |
| 240 | + | d.baseURL = server.URL | |
| 241 | + | ||
| 242 | + | paths, err := d.DownloadFilters(context.Background(), nil) | |
| 243 | + | require.Error(t, err) | |
| 244 | + | require.Nil(t, paths) | |
| 245 | + | require.Contains(t, err.Error(), "failed to create target dir") | |
| 246 | + | require.NoFileExists(t, filepath.Join(outsideDir, "combined.json")) | |
| 247 | + | } | |
| 248 | + | ||
| 249 | + | func TestDownloader_GetCachedFilterPaths_RejectsSymlinkEscape(t *testing.T) { | |
| 250 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |
| 251 | + | outsideDir := filepath.Join(t.TempDir(), "outside") | |
| 252 | + | require.NoError(t, os.MkdirAll(cacheDir, cacheDirPerm)) | |
| 253 | + | require.NoError(t, os.MkdirAll(outsideDir, cacheDirPerm)) | |
| 254 | + | require.NoError(t, os.Symlink(outsideDir, filepath.Join(cacheDir, "nested"))) | |
| 255 | + | require.NoError(t, os.WriteFile(filepath.Join(outsideDir, "combined.json"), []byte("[]"), manifestFilePerm)) | |
| 256 | + | require.NoError(t, os.WriteFile( | |
| 257 | + | filepath.Join(cacheDir, FilterFiles.Manifest), | |
| 258 | + | []byte(`{"version":"test","combined":{"total_rules":1,"files":["nested/combined.json"]}}`), | |
| 259 | + | manifestFilePerm, | |
| 260 | + | )) | |
| 261 | + | ||
| 262 | + | d := NewDownloader(cacheDir) | |
| 263 | + | require.Nil(t, d.GetCachedFilterPaths()) | |
| 264 | + | } | |
| 265 | + | ||
| 67 | 266 | func TestDownloader_RenameTempFile_RequiresDirectoryParent(t *testing.T) { | |
| 68 | 267 | t.Parallel() | |
| 69 | 268 | ||
| @@ -78,7 +277,7 @@ func TestDownloader_RenameTempFile_RequiresDirectoryParent(t *testing.T) { | |||
| 78 | 277 | ||
| 79 | 278 | d := NewDownloader(cacheDir) | |
| 80 | 279 | ||
| 81 | - | err := d.renameTempFile(tmpFile, filepath.Join(parentAsFile, "filter.json")) | |
| 280 | + | err := d.renameTempFile(tmpFile, "target/filter.json") | |
| 82 | 281 | require.Error(t, err) | |
| 83 | - | require.Contains(t, err.Error(), "not a directory") | |
| 282 | + | require.Contains(t, err.Error(), "failed to create target dir") | |
| 84 | 283 | } | |
| @@ -2,11 +2,13 @@ package filtering | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | + | "encoding/json" | |||
| 5 | "io" | 6 | "io" | ||
| 6 | "net/http" | 7 | "net/http" | ||
| 7 | "net/http/httptest" | 8 | "net/http/httptest" | ||
| 8 | "os" | 9 | "os" | ||
| 9 | "path/filepath" | 10 | "path/filepath" | ||
| 11 | + | "strings" | |||
| 10 | "testing" | 12 | "testing" | ||
| 11 | 13 | ||||
| 12 | "github.com/stretchr/testify/require" | 14 | "github.com/stretchr/testify/require" | ||
| @@ -46,7 +48,7 @@ func TestDownloader_DownloadFile_RecoversWhenTargetDirDeletedConcurrently(t *tes | |||||
| 46 | } | 48 | } | ||
| 47 | resultCh := make(chan result, 1) | 49 | resultCh := make(chan result, 1) | ||
| 48 | go func() { | 50 | go func() { | ||
| 49 | - | path, n, err := d.downloadFile(context.Background(), filename) | 51 | + | path, n, err := d.downloadFile(context.Background(), filename, d.limits.maxFilterFileBytes) |
| 50 | resultCh <- result{path: path, n: n, err: err} | 52 | resultCh <- result{path: path, n: n, err: err} | ||
| 51 | }() | 53 | }() | ||
| 52 | 54 | ||||
| @@ -64,6 +66,203 @@ func TestDownloader_DownloadFile_RecoversWhenTargetDirDeletedConcurrently(t *tes | |||||
| 64 | require.Contains(t, string(content), "url-filter") | 66 | require.Contains(t, string(content), "url-filter") | ||
| 65 | } | 67 | } | ||
| 66 | 68 | ||||
| 69 | + | func TestDownloader_DownloadFilters_RejectsUnsafeManifestFilenames(t *testing.T) { | |||
| 70 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |||
| 71 | + | requested := make([]string, 0, 1) | |||
| 72 | + | ||||
| 73 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 74 | + | requested = append(requested, r.URL.Path) | |||
| 75 | + | if r.URL.Path != "/"+FilterFiles.Manifest { | |||
| 76 | + | http.NotFound(w, r) | |||
| 77 | + | return | |||
| 78 | + | } | |||
| 79 | + | _, _ = io.WriteString(w, `{"version":"test","combined":{"total_rules":1,"files":["../escape.json"]}}`) | |||
| 80 | + | })) | |||
| 81 | + | defer server.Close() | |||
| 82 | + | ||||
| 83 | + | d := NewDownloader(cacheDir) | |||
| 84 | + | d.baseURL = server.URL | |||
| 85 | + | ||||
| 86 | + | paths, err := d.DownloadFilters(context.Background(), nil) | |||
| 87 | + | require.Error(t, err) | |||
| 88 | + | require.Nil(t, paths) | |||
| 89 | + | require.Contains(t, err.Error(), "invalid manifest") | |||
| 90 | + | require.Equal(t, []string{"/" + FilterFiles.Manifest}, requested) | |||
| 91 | + | } | |||
| 92 | + | ||||
| 93 | + | func TestDownloader_DownloadFilters_AllowsSafeNestedManifestFilenames(t *testing.T) { | |||
| 94 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |||
| 95 | + | filename := "nested/combined-part1.json" | |||
| 96 | + | body := `[{"trigger":{"url-filter":"test"},"action":{"type":"block"}}]` | |||
| 97 | + | ||||
| 98 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 99 | + | switch r.URL.Path { | |||
| 100 | + | case "/" + FilterFiles.Manifest: | |||
| 101 | + | _, _ = io.WriteString(w, `{"version":"test","combined":{"total_rules":1,"files":["`+filename+`"]}}`) | |||
| 102 | + | case "/" + filename: | |||
| 103 | + | _, _ = io.WriteString(w, body) | |||
| 104 | + | default: | |||
| 105 | + | http.NotFound(w, r) | |||
| 106 | + | } | |||
| 107 | + | })) | |||
| 108 | + | defer server.Close() | |||
| 109 | + | ||||
| 110 | + | d := NewDownloader(cacheDir) | |||
| 111 | + | d.baseURL = server.URL | |||
| 112 | + | ||||
| 113 | + | paths, err := d.DownloadFilters(context.Background(), nil) | |||
| 114 | + | require.NoError(t, err) | |||
| 115 | + | ||||
| 116 | + | wantPath := filepath.Join(cacheDir, "nested", "combined-part1.json") | |||
| 117 | + | require.Equal(t, []string{wantPath}, paths) | |||
| 118 | + | content, err := os.ReadFile(wantPath) | |||
| 119 | + | require.NoError(t, err) | |||
| 120 | + | require.Equal(t, body, string(content)) | |||
| 121 | + | } | |||
| 122 | + | ||||
| 123 | + | func TestDownloader_FetchManifest_RejectsOversizedManifest(t *testing.T) { | |||
| 124 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 125 | + | require.Equal(t, "/"+FilterFiles.Manifest, r.URL.Path) | |||
| 126 | + | _, _ = io.WriteString(w, strings.Repeat("x", 17)) | |||
| 127 | + | })) | |||
| 128 | + | defer server.Close() | |||
| 129 | + | ||||
| 130 | + | d := NewDownloader(t.TempDir()) | |||
| 131 | + | d.baseURL = server.URL | |||
| 132 | + | d.limits.maxManifestBytes = 16 | |||
| 133 | + | ||||
| 134 | + | manifest, err := d.FetchManifest(context.Background()) | |||
| 135 | + | require.Error(t, err) | |||
| 136 | + | require.Nil(t, manifest) | |||
| 137 | + | require.Contains(t, err.Error(), "manifest too large") | |||
| 138 | + | } | |||
| 139 | + | ||||
| 140 | + | func TestDownloader_FetchManifest_RejectsTooManyFiles(t *testing.T) { | |||
| 141 | + | payload, err := json.Marshal(Manifest{ | |||
| 142 | + | Version: "test", | |||
| 143 | + | Combined: CombinedInfo{ | |||
| 144 | + | TotalRules: 3, | |||
| 145 | + | Files: []string{"part1.json", "part2.json", "part3.json"}, | |||
| 146 | + | }, | |||
| 147 | + | }) | |||
| 148 | + | require.NoError(t, err) | |||
| 149 | + | ||||
| 150 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 151 | + | require.Equal(t, "/"+FilterFiles.Manifest, r.URL.Path) | |||
| 152 | + | _, _ = w.Write(payload) | |||
| 153 | + | })) | |||
| 154 | + | defer server.Close() | |||
| 155 | + | ||||
| 156 | + | d := NewDownloader(t.TempDir()) | |||
| 157 | + | d.baseURL = server.URL | |||
| 158 | + | d.limits.maxFilterFiles = 2 | |||
| 159 | + | ||||
| 160 | + | manifest, err := d.FetchManifest(context.Background()) | |||
| 161 | + | require.Error(t, err) | |||
| 162 | + | require.Nil(t, manifest) | |||
| 163 | + | require.Contains(t, err.Error(), "limit is 2") | |||
| 164 | + | } | |||
| 165 | + | ||||
| 166 | + | func TestDownloader_DownloadFilters_RejectsOversizedFilterFile(t *testing.T) { | |||
| 167 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |||
| 168 | + | filename := "combined.json" | |||
| 169 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 170 | + | switch r.URL.Path { | |||
| 171 | + | case "/" + FilterFiles.Manifest: | |||
| 172 | + | _, _ = io.WriteString(w, `{"version":"test","combined":{"total_rules":1,"files":["combined.json"]}}`) | |||
| 173 | + | case "/" + filename: | |||
| 174 | + | _, _ = io.WriteString(w, "12345") | |||
| 175 | + | default: | |||
| 176 | + | http.NotFound(w, r) | |||
| 177 | + | } | |||
| 178 | + | })) | |||
| 179 | + | defer server.Close() | |||
| 180 | + | ||||
| 181 | + | d := NewDownloader(cacheDir) | |||
| 182 | + | d.baseURL = server.URL | |||
| 183 | + | d.limits.maxFilterFileBytes = 4 | |||
| 184 | + | d.limits.maxTotalFilterBytes = 32 | |||
| 185 | + | ||||
| 186 | + | paths, err := d.DownloadFilters(context.Background(), nil) | |||
| 187 | + | require.Error(t, err) | |||
| 188 | + | require.Nil(t, paths) | |||
| 189 | + | require.Contains(t, err.Error(), "too large") | |||
| 190 | + | require.NoFileExists(t, filepath.Join(cacheDir, filename)) | |||
| 191 | + | } | |||
| 192 | + | ||||
| 193 | + | func TestDownloader_DownloadFilters_EnforcesTotalDownloadLimit(t *testing.T) { | |||
| 194 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |||
| 195 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 196 | + | switch r.URL.Path { | |||
| 197 | + | case "/" + FilterFiles.Manifest: | |||
| 198 | + | _, _ = io.WriteString(w, `{"version":"test","combined":{"total_rules":2,"files":["part1.json","part2.json"]}}`) | |||
| 199 | + | case "/part1.json", "/part2.json": | |||
| 200 | + | _, _ = io.WriteString(w, "12345") | |||
| 201 | + | default: | |||
| 202 | + | http.NotFound(w, r) | |||
| 203 | + | } | |||
| 204 | + | })) | |||
| 205 | + | defer server.Close() | |||
| 206 | + | ||||
| 207 | + | d := NewDownloader(cacheDir) | |||
| 208 | + | d.baseURL = server.URL | |||
| 209 | + | d.limits.maxFilterFileBytes = 8 | |||
| 210 | + | d.limits.maxTotalFilterBytes = 8 | |||
| 211 | + | ||||
| 212 | + | paths, err := d.DownloadFilters(context.Background(), nil) | |||
| 213 | + | require.Error(t, err) | |||
| 214 | + | require.Nil(t, paths) | |||
| 215 | + | require.Contains(t, err.Error(), "too large") | |||
| 216 | + | require.FileExists(t, filepath.Join(cacheDir, "part1.json")) | |||
| 217 | + | require.NoFileExists(t, filepath.Join(cacheDir, "part2.json")) | |||
| 218 | + | } | |||
| 219 | + | ||||
| 220 | + | func TestDownloader_DownloadFilters_RejectsSymlinkEscape(t *testing.T) { | |||
| 221 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |||
| 222 | + | outsideDir := filepath.Join(t.TempDir(), "outside") | |||
| 223 | + | require.NoError(t, os.MkdirAll(cacheDir, cacheDirPerm)) | |||
| 224 | + | require.NoError(t, os.MkdirAll(outsideDir, cacheDirPerm)) | |||
| 225 | + | require.NoError(t, os.Symlink(outsideDir, filepath.Join(cacheDir, "nested"))) | |||
| 226 | + | ||||
| 227 | + | server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 228 | + | switch r.URL.Path { | |||
| 229 | + | case "/" + FilterFiles.Manifest: | |||
| 230 | + | _, _ = io.WriteString(w, `{"version":"test","combined":{"total_rules":1,"files":["nested/combined.json"]}}`) | |||
| 231 | + | case "/nested/combined.json": | |||
| 232 | + | _, _ = io.WriteString(w, `[{"trigger":{"url-filter":"test"},"action":{"type":"block"}}]`) | |||
| 233 | + | default: | |||
| 234 | + | http.NotFound(w, r) | |||
| 235 | + | } | |||
| 236 | + | })) | |||
| 237 | + | defer server.Close() | |||
| 238 | + | ||||
| 239 | + | d := NewDownloader(cacheDir) | |||
| 240 | + | d.baseURL = server.URL | |||
| 241 | + | ||||
| 242 | + | paths, err := d.DownloadFilters(context.Background(), nil) | |||
| 243 | + | require.Error(t, err) | |||
| 244 | + | require.Nil(t, paths) | |||
| 245 | + | require.Contains(t, err.Error(), "failed to create target dir") | |||
| 246 | + | require.NoFileExists(t, filepath.Join(outsideDir, "combined.json")) | |||
| 247 | + | } | |||
| 248 | + | ||||
| 249 | + | func TestDownloader_GetCachedFilterPaths_RejectsSymlinkEscape(t *testing.T) { | |||
| 250 | + | cacheDir := filepath.Join(t.TempDir(), "cache") | |||
| 251 | + | outsideDir := filepath.Join(t.TempDir(), "outside") | |||
| 252 | + | require.NoError(t, os.MkdirAll(cacheDir, cacheDirPerm)) | |||
| 253 | + | require.NoError(t, os.MkdirAll(outsideDir, cacheDirPerm)) | |||
| 254 | + | require.NoError(t, os.Symlink(outsideDir, filepath.Join(cacheDir, "nested"))) | |||
| 255 | + | require.NoError(t, os.WriteFile(filepath.Join(outsideDir, "combined.json"), []byte("[]"), manifestFilePerm)) | |||
| 256 | + | require.NoError(t, os.WriteFile( | |||
| 257 | + | filepath.Join(cacheDir, FilterFiles.Manifest), | |||
| 258 | + | []byte(`{"version":"test","combined":{"total_rules":1,"files":["nested/combined.json"]}}`), | |||
| 259 | + | manifestFilePerm, | |||
| 260 | + | )) | |||
| 261 | + | ||||
| 262 | + | d := NewDownloader(cacheDir) | |||
| 263 | + | require.Nil(t, d.GetCachedFilterPaths()) | |||
| 264 | + | } | |||
| 265 | + | ||||
| 67 | func TestDownloader_RenameTempFile_RequiresDirectoryParent(t *testing.T) { | 266 | func TestDownloader_RenameTempFile_RequiresDirectoryParent(t *testing.T) { | ||
| 68 | t.Parallel() | 267 | t.Parallel() | ||
| 69 | 268 | ||||
| @@ -78,7 +277,7 @@ func TestDownloader_RenameTempFile_RequiresDirectoryParent(t *testing.T) { | |||||
| 78 | 277 | ||||
| 79 | d := NewDownloader(cacheDir) | 278 | d := NewDownloader(cacheDir) | ||
| 80 | 279 | ||||
| 81 | - | err := d.renameTempFile(tmpFile, filepath.Join(parentAsFile, "filter.json")) | 280 | + | err := d.renameTempFile(tmpFile, "target/filter.json") |
| 82 | require.Error(t, err) | 281 | require.Error(t, err) | ||
| 83 | - | require.Contains(t, err.Error(), "not a directory") | 282 | + | require.Contains(t, err.Error(), "failed to create target dir") |
| 84 | } | 283 | } | ||
M internal/infrastructure/webkit/messaging.go +28 -7
| @@ -198,16 +198,26 @@ func (r *MessageRouter) handleScriptMessage(senderUCM webkit.UserContentManager, | |||
| 198 | 198 | log.Warn().Err(err).Str("json", rawJSON).Msg("failed to unmarshal script message") | |
| 199 | 199 | return | |
| 200 | 200 | } | |
| 201 | - | if msg.WebViewID == 0 && msg.WebViewIDAlt != 0 { | |
| 202 | - | msg.WebViewID = msg.WebViewIDAlt | |
| 201 | + | ||
| 202 | + | senderWV := lookupSenderWebView(senderUCM) | |
| 203 | + | if senderWV == nil { | |
| 204 | + | log.Warn().Msg("rejecting script message from unknown sender") | |
| 205 | + | return | |
| 203 | 206 | } | |
| 204 | - | if msg.WebViewID == 0 { | |
| 205 | - | if senderWV := lookupSenderWebView(senderUCM); senderWV != nil { | |
| 206 | - | msg.WebViewID = uint64(senderWV.ID()) | |
| 207 | - | r.syncWebViewID(senderWV) | |
| 208 | - | } | |
| 207 | + | if !isTrustedBridgeWebView(senderWV) { | |
| 208 | + | log.Warn(). | |
| 209 | + | Str("type", msg.Type). | |
| 210 | + | Uint64("sender_webview_id", uint64(senderWV.ID())). | |
| 211 | + | Str("uri", senderWV.URI()). | |
| 212 | + | Msg("rejecting script message from untrusted page") | |
| 213 | + | return | |
| 209 | 214 | } | |
| 210 | 215 | ||
| 216 | + | // Trust the sender WebView resolved from WebKit's UserContentManager, not a | |
| 217 | + | // caller-provided webViewId field in page-controlled JSON. | |
| 218 | + | msg.WebViewID = uint64(senderWV.ID()) | |
| 219 | + | r.syncWebViewID(senderWV) | |
| 220 | + | ||
| 211 | 221 | if msg.Type == "" { | |
| 212 | 222 | log.Warn().Msg("script message missing type") | |
| 213 | 223 | return | |
| @@ -262,6 +272,17 @@ func lookupSenderWebView(senderUCM webkit.UserContentManager) *WebView { | |||
| 262 | 272 | return LookupWebViewByUCMPointer(ptr) | |
| 263 | 273 | } | |
| 264 | 274 | ||
| 275 | + | func isTrustedBridgeWebView(wv *WebView) bool { | |
| 276 | + | if wv == nil { | |
| 277 | + | return false | |
| 278 | + | } | |
| 279 | + | return isTrustedBridgeURI(wv.URI()) | |
| 280 | + | } | |
| 281 | + | ||
| 282 | + | func isTrustedBridgeURI(raw string) bool { | |
| 283 | + | return isTrustedSystemviewURL(raw) | |
| 284 | + | } | |
| 285 | + | ||
| 265 | 286 | func (r *MessageRouter) syncWebViewID(wv *WebView) { | |
| 266 | 287 | if wv == nil { | |
| 267 | 288 | return | |
| @@ -198,16 +198,26 @@ func (r *MessageRouter) handleScriptMessage(senderUCM webkit.UserContentManager, | |||||
| 198 | log.Warn().Err(err).Str("json", rawJSON).Msg("failed to unmarshal script message") | 198 | log.Warn().Err(err).Str("json", rawJSON).Msg("failed to unmarshal script message") | ||
| 199 | return | 199 | return | ||
| 200 | } | 200 | } | ||
| 201 | - | if msg.WebViewID == 0 && msg.WebViewIDAlt != 0 { | 201 | + | |
| 202 | - | msg.WebViewID = msg.WebViewIDAlt | 202 | + | senderWV := lookupSenderWebView(senderUCM) |
| 203 | + | if senderWV == nil { | |||
| 204 | + | log.Warn().Msg("rejecting script message from unknown sender") | |||
| 205 | + | return | |||
| 203 | } | 206 | } | ||
| 204 | - | if msg.WebViewID == 0 { | 207 | + | if !isTrustedBridgeWebView(senderWV) { |
| 205 | - | if senderWV := lookupSenderWebView(senderUCM); senderWV != nil { | 208 | + | log.Warn(). |
| 206 | - | msg.WebViewID = uint64(senderWV.ID()) | 209 | + | Str("type", msg.Type). |
| 207 | - | r.syncWebViewID(senderWV) | 210 | + | Uint64("sender_webview_id", uint64(senderWV.ID())). |
| 208 | - | } | 211 | + | Str("uri", senderWV.URI()). |
| 212 | + | Msg("rejecting script message from untrusted page") | |||
| 213 | + | return | |||
| 209 | } | 214 | } | ||
| 210 | 215 | ||||
| 216 | + | // Trust the sender WebView resolved from WebKit's UserContentManager, not a | |||
| 217 | + | // caller-provided webViewId field in page-controlled JSON. | |||
| 218 | + | msg.WebViewID = uint64(senderWV.ID()) | |||
| 219 | + | r.syncWebViewID(senderWV) | |||
| 220 | + | ||||
| 211 | if msg.Type == "" { | 221 | if msg.Type == "" { | ||
| 212 | log.Warn().Msg("script message missing type") | 222 | log.Warn().Msg("script message missing type") | ||
| 213 | return | 223 | return | ||
| @@ -262,6 +272,17 @@ func lookupSenderWebView(senderUCM webkit.UserContentManager) *WebView { | |||||
| 262 | return LookupWebViewByUCMPointer(ptr) | 272 | return LookupWebViewByUCMPointer(ptr) | ||
| 263 | } | 273 | } | ||
| 264 | 274 | ||||
| 275 | + | func isTrustedBridgeWebView(wv *WebView) bool { | |||
| 276 | + | if wv == nil { | |||
| 277 | + | return false | |||
| 278 | + | } | |||
| 279 | + | return isTrustedBridgeURI(wv.URI()) | |||
| 280 | + | } | |||
| 281 | + | ||||
| 282 | + | func isTrustedBridgeURI(raw string) bool { | |||
| 283 | + | return isTrustedSystemviewURL(raw) | |||
| 284 | + | } | |||
| 285 | + | ||||
| 265 | func (r *MessageRouter) syncWebViewID(wv *WebView) { | 286 | func (r *MessageRouter) syncWebViewID(wv *WebView) { | ||
| 266 | if wv == nil { | 287 | if wv == nil { | ||
| 267 | return | 288 | return | ||
A internal/infrastructure/webkit/messaging_test.go +42 -0
| @@ -0,0 +1,42 @@ | |||
| 1 | + | package webkit | |
| 2 | + | ||
| 3 | + | import ( | |
| 4 | + | "testing" | |
| 5 | + | ||
| 6 | + | "github.com/stretchr/testify/require" | |
| 7 | + | ) | |
| 8 | + | ||
| 9 | + | func TestIsTrustedBridgeURI(t *testing.T) { | |
| 10 | + | t.Parallel() | |
| 11 | + | ||
| 12 | + | trusted := []string{ | |
| 13 | + | "dumb://history", | |
| 14 | + | "dumb://favorites", | |
| 15 | + | "dumb://config", | |
| 16 | + | "dumb://error", | |
| 17 | + | "dumb://crash", | |
| 18 | + | "dumb://history/path?cursor=1", | |
| 19 | + | } | |
| 20 | + | for _, raw := range trusted { | |
| 21 | + | t.Run(raw, func(t *testing.T) { | |
| 22 | + | t.Parallel() | |
| 23 | + | require.True(t, isTrustedBridgeURI(raw)) | |
| 24 | + | }) | |
| 25 | + | } | |
| 26 | + | ||
| 27 | + | untrusted := []string{ | |
| 28 | + | "", | |
| 29 | + | "https://example.com", | |
| 30 | + | "http://localhost", | |
| 31 | + | "file:///tmp/history.html", | |
| 32 | + | "dumb://evil/history", | |
| 33 | + | "dumb://example.com", | |
| 34 | + | "javascript:alert(1)", | |
| 35 | + | } | |
| 36 | + | for _, raw := range untrusted { | |
| 37 | + | t.Run(raw, func(t *testing.T) { | |
| 38 | + | t.Parallel() | |
| 39 | + | require.False(t, isTrustedBridgeURI(raw)) | |
| 40 | + | }) | |
| 41 | + | } | |
| 42 | + | } | |
| @@ -0,0 +1,42 @@ | |||||
| 1 | + | package webkit | |||
| 2 | + | ||||
| 3 | + | import ( | |||
| 4 | + | "testing" | |||
| 5 | + | ||||
| 6 | + | "github.com/stretchr/testify/require" | |||
| 7 | + | ) | |||
| 8 | + | ||||
| 9 | + | func TestIsTrustedBridgeURI(t *testing.T) { | |||
| 10 | + | t.Parallel() | |||
| 11 | + | ||||
| 12 | + | trusted := []string{ | |||
| 13 | + | "dumb://history", | |||
| 14 | + | "dumb://favorites", | |||
| 15 | + | "dumb://config", | |||
| 16 | + | "dumb://error", | |||
| 17 | + | "dumb://crash", | |||
| 18 | + | "dumb://history/path?cursor=1", | |||
| 19 | + | } | |||
| 20 | + | for _, raw := range trusted { | |||
| 21 | + | t.Run(raw, func(t *testing.T) { | |||
| 22 | + | t.Parallel() | |||
| 23 | + | require.True(t, isTrustedBridgeURI(raw)) | |||
| 24 | + | }) | |||
| 25 | + | } | |||
| 26 | + | ||||
| 27 | + | untrusted := []string{ | |||
| 28 | + | "", | |||
| 29 | + | "https://example.com", | |||
| 30 | + | "http://localhost", | |||
| 31 | + | "file:///tmp/history.html", | |||
| 32 | + | "dumb://evil/history", | |||
| 33 | + | "dumb://example.com", | |||
| 34 | + | "javascript:alert(1)", | |||
| 35 | + | } | |||
| 36 | + | for _, raw := range untrusted { | |||
| 37 | + | t.Run(raw, func(t *testing.T) { | |||
| 38 | + | t.Parallel() | |||
| 39 | + | require.False(t, isTrustedBridgeURI(raw)) | |||
| 40 | + | }) | |||
| 41 | + | } | |||
| 42 | + | } | |||