mirror of
https://github.com/lukaszraczylo/gohoarder.git
synced 2026-07-17 05:34:09 +00:00
feat: comprehensive audit + Tier 3 wiring (security/correctness/features)
Multi-agent audit + fix pass covering bugs, security, dead config wiring,
and missing features. All quality gates green: build/vet/test -race/govulncheck/
golangci-lint. Frontend tests 47/47.
SECURITY & CORRECTNESS
- Scanner pipeline fail-closed: cache.go scan timeout now 503 (was goto servePkg);
scanner.go all-scanners-fail saves ScanStatusError; CheckVulnerabilities blocks
on missing/error result instead of allowing.
- Scanner argv corrections: govulncheck source-mode + go.mod discovery;
npm-audit generates lockfile via npm install --package-lock-only --ignore-scripts;
pip-audit per-extension dispatch (wheel direct / sdist extract+pyproject).
- GHSA version-range filtering implemented (was always-include); url.QueryEscape
on package name.
- pypi SSRF closed via host allowlist on original_url; url.QueryEscape on
rewriteURL output.
- Path traversal: cache temp file uses os.CreateTemp; smb keyToPath sanitized;
filesystem keyToPath returns error + filepath.Abs prefix-verify.
- Goroutine leaks plugged: cache.cleanupWorker stop channel; auth.ValidationCache
Stop() with sync.Once; ws.unregister buffered with non-blocking send.
- WebSocket double-close panic fixed via sync.Once Client.closeSend().
- Race conditions: gormstore.registryCache sync.RWMutex (was concurrent map
panic); auth.LastUsedAt no longer mutated under RLock; analytics strict
lock-ordering invariant (statsMu before downloadsMu).
- Auth validator fails CLOSED on transport/unknown-status errors (was returning
true,err 'allow cache fallback').
- Credential cache hash full SHA256 (was 8-byte truncate; eliminates 2^32
collision risk).
- filesystem.fs.used incremented after successful rename (no quota inflation
race).
- gormstore aggregation events deleted in same tx as insert (no double-counting).
- gormstore.SavePackage uses Updates(map) so zero-value security flags
(RequiresAuth=false) can be cleared.
- partition_manager validates partition name regex before DROP TABLE.
- vcs/git: version regex validation rejects --option-injection; checkout uses
--detach -- separator; removed TrimPrefix(repo,'v') that corrupted vault/
vitess/vim-go.
- WebSocket CheckOrigin allowlist (was return true; CSWSH closed); same-origin
default + ServerConfig.AllowedOrigins.
- fiber CVE GO-2026-4543 patched (v2.52.10 -> v2.52.12).
FUNCTIONAL FEATURES
- API key DB persistence: APIKeyModel + migration 202604280002, full CRUD,
async LastUsedAt updates with WaitGroup-tracked goroutines drained on Close.
- Admin bootstrap via GOHOARDER_BOOTSTRAP_ADMIN_KEY env var; idempotent
(skipped when non-revoked admin already exists).
- Auth middleware factory in pkg/app: RequireAuth, RequireRole, RequirePermission,
OptionalAuth. Mounted on proxy + read APIs when Auth.Enabled=true.
DELETE /api/packages/* requires admin role. /health public for k8s probes.
- NFS storage backend: pkg/storage/nfs wraps filesystem with /proc/mounts
detection (Linux), post-write fsync, read-after-write health probe.
- WebSocket real-time pipeline: pkg/events Broadcaster interface; cache + scanner
managers emit EventPackageCached / EventPackageDownloaded / EventScanComplete;
app.go runs 30s EventStatsUpdate ticker. Frontend has full WS client (reconnect
with exponential backoff 1s->30s, 25s heartbeat, subscriber pattern), Pinia
realtime store, Dashboard live indicator + activity feed + reactive stats
override.
- TLS termination: fiber.ListenTLS when Server.TLS.Enabled.
- Pre-warming: Prewarming.Enabled flag wired (was hardcoded false); Interval,
MaxConcurrent, TopPackages plumbed.
- NetworkConfig wired (timeouts, retry, rate-limit, circuit-breaker).
- AuthConfig.BcryptCost wired.
- Security.Scanners.Static.MaxPackageSize plumbed to cache.io.LimitReader.
- Handlers.{Go,NPM,PyPI}.Enabled gates route mounting.
- Graceful shutdown: authManager.Close drains async writes.
LINT/HYGIENE
- 80 golangci-lint issues resolved: errcheck (Close/Write/RemoveAll wrapped),
gofmt, gosec G104/G306, govet shadow renames + fieldalignment reorders,
staticcheck ST1000 pkg comments + QF1003 tagged switches + QF1008 embedded
field selectors + QF1001 De Morgan's law.
BEHAVIOR CHANGES
- Scanner now fail-closed: deployments without scanner binaries
(trivy/govulncheck/npm-audit/pip-audit/grype) BLOCK packages instead of
serving unscanned. Add binaries or plan a future allow-on-scan-error flag.
- WS origin defaults to same-origin; cross-origin dashboards must set
server.allowed_origins.
- Validator no longer fails open; private packages won't serve from cache when
validation can't be performed.
- download_events retention dropped from 24h to ~5min (deleted in agg tx).
Migration 202604280001 purges pre-upgrade events.
- DELETE /api/packages/* requires admin role when auth enabled.
NEW PACKAGES
- pkg/events - Broadcaster interface
- pkg/storage/nfs - NFS-aware filesystem wrapper
DEFERRED (out of scope this pass)
- Static scanner implementation (config defines AllowedLicenses/MaxPackageSize/
BlockSuspicious but pkg/scanner/static/ does not exist).
- AuthConfig.AuditLog wiring (no audit log infra yet).
- CacheConfig.TTLOverrides passthrough (would touch cache.Config beyond
integrator scope).
This commit is contained in:
+80
-9
@@ -1,3 +1,5 @@
|
||||
// Package pypi implements the HTTP handler that proxies PyPI registry
|
||||
// requests through the GoHoarder cache.
|
||||
package pypi
|
||||
|
||||
import (
|
||||
@@ -6,6 +8,7 @@ import (
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"regexp"
|
||||
"strings"
|
||||
"time"
|
||||
@@ -17,6 +20,36 @@ import (
|
||||
"github.com/rs/zerolog/log"
|
||||
)
|
||||
|
||||
// defaultAllowedPyPIHosts is the hardcoded SSRF allowlist for hosts that
|
||||
// original_url query params may target. Subdomains of pythonhosted.org are
|
||||
// also accepted (handled in isAllowedPyPIHost).
|
||||
var defaultAllowedPyPIHosts = []string{
|
||||
"pypi.org",
|
||||
"files.pythonhosted.org",
|
||||
"pythonhosted.org",
|
||||
}
|
||||
|
||||
// isAllowedPyPIHost reports whether host is on the allowlist or a subdomain
|
||||
// of pythonhosted.org. Comparison is case-insensitive on host only.
|
||||
func isAllowedPyPIHost(host string, allowed []string) bool {
|
||||
host = strings.ToLower(host)
|
||||
// Strip optional port
|
||||
if i := strings.IndexByte(host, ':'); i >= 0 {
|
||||
host = host[:i]
|
||||
}
|
||||
for _, a := range allowed {
|
||||
a = strings.ToLower(a)
|
||||
if host == a {
|
||||
return true
|
||||
}
|
||||
}
|
||||
// Allow any subdomain of pythonhosted.org (e.g. files.pythonhosted.org)
|
||||
if strings.HasSuffix(host, ".pythonhosted.org") {
|
||||
return true
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// Handler implements the PyPI Simple API (PEP 503)
|
||||
type Handler struct {
|
||||
cache *cache.Manager
|
||||
@@ -26,11 +59,15 @@ type Handler struct {
|
||||
credValidator *auth.PyPIValidator
|
||||
validationCache *auth.ValidationCache
|
||||
upstream string
|
||||
allowedHosts []string
|
||||
}
|
||||
|
||||
// Config holds PyPI proxy configuration
|
||||
type Config struct {
|
||||
Upstream string // Upstream PyPI index (e.g., pypi.org/simple)
|
||||
// AllowedHosts is an SSRF allowlist for hosts that original_url query
|
||||
// params may target. If empty, defaultAllowedPyPIHosts is used.
|
||||
AllowedHosts []string
|
||||
}
|
||||
|
||||
// New creates a new PyPI proxy handler
|
||||
@@ -39,10 +76,16 @@ func New(cacheManager *cache.Manager, client *network.Client, config Config) *Ha
|
||||
config.Upstream = "https://pypi.org/simple"
|
||||
}
|
||||
|
||||
allowed := config.AllowedHosts
|
||||
if len(allowed) == 0 {
|
||||
allowed = defaultAllowedPyPIHosts
|
||||
}
|
||||
|
||||
return &Handler{
|
||||
cache: cacheManager,
|
||||
client: client,
|
||||
upstream: config.Upstream,
|
||||
allowedHosts: allowed,
|
||||
credExtractor: auth.NewCredentialExtractor(),
|
||||
credHasher: auth.NewCredentialHasher(),
|
||||
credValidator: auth.NewPyPIValidator(),
|
||||
@@ -87,7 +130,7 @@ func (h *Handler) handleIndex(ctx context.Context, w http.ResponseWriter, r *htt
|
||||
return nil, "", err
|
||||
}
|
||||
if statusCode != http.StatusOK {
|
||||
body.Close() // #nosec G104 -- Cleanup, error not critical
|
||||
_ = body.Close()
|
||||
return nil, "", fmt.Errorf("upstream returned status %d", statusCode)
|
||||
}
|
||||
return body, url, nil
|
||||
@@ -98,7 +141,11 @@ func (h *Handler) handleIndex(ctx context.Context, w http.ResponseWriter, r *htt
|
||||
http.Error(w, "Failed to fetch PyPI index", http.StatusBadGateway)
|
||||
return
|
||||
}
|
||||
defer entry.Data.Close() // #nosec G104 -- Cleanup, error not critical
|
||||
defer func() {
|
||||
if cerr := entry.Data.Close(); cerr != nil {
|
||||
log.Warn().Err(cerr).Msg("Failed to close PyPI index body")
|
||||
}
|
||||
}()
|
||||
|
||||
w.Header().Set("Content-Type", "text/html; charset=UTF-8")
|
||||
_, _ = io.Copy(w, entry.Data) // #nosec G104 -- HTTP response write
|
||||
@@ -115,7 +162,7 @@ func (h *Handler) handlePackagePage(ctx context.Context, w http.ResponseWriter,
|
||||
return nil, "", err
|
||||
}
|
||||
if statusCode != http.StatusOK {
|
||||
body.Close() // #nosec G104 -- Cleanup, error not critical
|
||||
_ = body.Close()
|
||||
return nil, "", fmt.Errorf("upstream returned status %d", statusCode)
|
||||
}
|
||||
return body, url, nil
|
||||
@@ -126,7 +173,11 @@ func (h *Handler) handlePackagePage(ctx context.Context, w http.ResponseWriter,
|
||||
http.Error(w, "Failed to fetch package page", http.StatusBadGateway)
|
||||
return
|
||||
}
|
||||
defer entry.Data.Close() // #nosec G104 -- Cleanup, error not critical
|
||||
defer func() {
|
||||
if cerr := entry.Data.Close(); cerr != nil {
|
||||
log.Warn().Err(cerr).Msg("Failed to close PyPI package page body")
|
||||
}
|
||||
}()
|
||||
|
||||
// Read page into memory for URL rewriting
|
||||
var buf bytes.Buffer
|
||||
@@ -175,6 +226,23 @@ func (h *Handler) handlePackageFile(ctx context.Context, w http.ResponseWriter,
|
||||
if !strings.HasPrefix(originalURL, "http://") && !strings.HasPrefix(originalURL, "https://") {
|
||||
originalURL = "https://pypi.org" + originalURL
|
||||
}
|
||||
|
||||
// SSRF protection: validate parsed host against allowlist before
|
||||
// fetching. Rejects 169.254.169.254, internal services, etc.
|
||||
parsed, parseErr := url.Parse(originalURL)
|
||||
if parseErr != nil || parsed.Host == "" || (parsed.Scheme != "http" && parsed.Scheme != "https") {
|
||||
log.Warn().Str("original_url", originalURL).Msg("Rejected invalid original_url")
|
||||
http.Error(w, "Invalid original_url", http.StatusBadRequest)
|
||||
return
|
||||
}
|
||||
if !isAllowedPyPIHost(parsed.Host, h.allowedHosts) {
|
||||
log.Warn().
|
||||
Str("original_url", originalURL).
|
||||
Str("host", parsed.Host).
|
||||
Msg("Rejected original_url host not on allowlist")
|
||||
http.Error(w, "original_url host not allowed", http.StatusBadRequest)
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
log.Debug().
|
||||
@@ -199,7 +267,7 @@ func (h *Handler) handlePackageFile(ctx context.Context, w http.ResponseWriter,
|
||||
return nil, "", err
|
||||
}
|
||||
if statusCode != http.StatusOK {
|
||||
body.Close() // #nosec G104 -- Cleanup, error not critical
|
||||
_ = body.Close()
|
||||
return nil, "", fmt.Errorf("upstream returned status %d", statusCode)
|
||||
}
|
||||
return body, originalURL, nil
|
||||
@@ -218,7 +286,11 @@ func (h *Handler) handlePackageFile(ctx context.Context, w http.ResponseWriter,
|
||||
http.Error(w, "Failed to fetch package file", http.StatusBadGateway)
|
||||
return
|
||||
}
|
||||
defer entry.Data.Close() // #nosec G104 -- Cleanup, error not critical
|
||||
defer func() {
|
||||
if cerr := entry.Data.Close(); cerr != nil {
|
||||
log.Warn().Err(cerr).Msg("Failed to close PyPI package file body")
|
||||
}
|
||||
}()
|
||||
|
||||
// CRITICAL SECURITY CHECK: If package requires auth, validate credentials
|
||||
if entry.Package != nil && entry.Package.RequiresAuth {
|
||||
@@ -396,9 +468,8 @@ func rewritePackagePageURLs(html, packageName, proxyBaseURL string) string {
|
||||
// This preserves the original CDN URL so we can fetch from the correct location
|
||||
baseURL := strings.TrimSuffix(proxyBaseURL, "/simple")
|
||||
|
||||
// URL encode the original URL
|
||||
encodedURL := strings.ReplaceAll(originalURL, "&", "%26")
|
||||
encodedURL = strings.ReplaceAll(encodedURL, "=", "%3D")
|
||||
// URL encode the original URL — covers &, =, ?, #, +, /, etc.
|
||||
encodedURL := url.QueryEscape(originalURL)
|
||||
|
||||
newURL := fmt.Sprintf(`href="%s/%s/%s?original_url=%s"`, baseURL, packageName, filenameMatch, encodedURL)
|
||||
return newURL
|
||||
|
||||
@@ -0,0 +1,139 @@
|
||||
package pypi
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"testing"
|
||||
|
||||
"github.com/lukaszraczylo/gohoarder/pkg/auth"
|
||||
)
|
||||
|
||||
func TestIsAllowedPyPIHost(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
host string
|
||||
allowed []string
|
||||
want bool
|
||||
}{
|
||||
{"pypi.org allowed", "pypi.org", defaultAllowedPyPIHosts, true},
|
||||
{"files.pythonhosted.org allowed", "files.pythonhosted.org", defaultAllowedPyPIHosts, true},
|
||||
{"subdomain of pythonhosted.org allowed", "cdn.pythonhosted.org", defaultAllowedPyPIHosts, true},
|
||||
{"case insensitive", "PyPI.ORG", defaultAllowedPyPIHosts, true},
|
||||
{"with port", "pypi.org:443", defaultAllowedPyPIHosts, true},
|
||||
{"AWS metadata blocked", "169.254.169.254", defaultAllowedPyPIHosts, false},
|
||||
{"GCP metadata blocked", "metadata.google.internal", defaultAllowedPyPIHosts, false},
|
||||
{"localhost blocked", "localhost", defaultAllowedPyPIHosts, false},
|
||||
{"loopback blocked", "127.0.0.1", defaultAllowedPyPIHosts, false},
|
||||
{"private RFC1918 blocked", "10.0.0.1", defaultAllowedPyPIHosts, false},
|
||||
{"attacker domain blocked", "evil.example.com", defaultAllowedPyPIHosts, false},
|
||||
{"empty host blocked", "", defaultAllowedPyPIHosts, false},
|
||||
{"trailing-substring attack blocked", "evilpythonhosted.org", defaultAllowedPyPIHosts, false},
|
||||
{"prefix attack blocked", "pypi.org.evil.com", defaultAllowedPyPIHosts, false},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
got := isAllowedPyPIHost(tt.host, tt.allowed)
|
||||
if got != tt.want {
|
||||
t.Errorf("isAllowedPyPIHost(%q) = %v, want %v", tt.host, got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// newHandlerForSSRFTest builds a Handler with only the fields needed to reach
|
||||
// the SSRF guard. cache is nil intentionally — the guard rejects before any
|
||||
// cache call.
|
||||
func newHandlerForSSRFTest() *Handler {
|
||||
return &Handler{
|
||||
credExtractor: auth.NewCredentialExtractor(),
|
||||
credHasher: auth.NewCredentialHasher(),
|
||||
upstream: "https://pypi.org/simple",
|
||||
allowedHosts: defaultAllowedPyPIHosts,
|
||||
}
|
||||
}
|
||||
|
||||
func TestHandlePackageFile_SSRFRejected(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
originalURL string
|
||||
}{
|
||||
{"AWS metadata IP", "http://169.254.169.254/"},
|
||||
{"GCP metadata host", "http://metadata.google.internal/computeMetadata/v1/"},
|
||||
{"localhost", "http://localhost:8080/secret"},
|
||||
{"private network", "http://10.0.0.5/"},
|
||||
{"file scheme", "file:///etc/passwd"},
|
||||
{"gopher scheme", "gopher://internal/"},
|
||||
{"unrelated public host", "https://evil.example.com/payload.whl"},
|
||||
}
|
||||
|
||||
h := newHandlerForSSRFTest()
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
path := "/example/example-1.0.0.whl"
|
||||
q := url.Values{}
|
||||
q.Set("original_url", tt.originalURL)
|
||||
req := httptest.NewRequest(http.MethodGet, path+"?"+q.Encode(), nil)
|
||||
w := httptest.NewRecorder()
|
||||
|
||||
h.handlePackageFile(req.Context(), w, req, path)
|
||||
|
||||
if w.Code != http.StatusBadRequest {
|
||||
t.Errorf("expected 400 for SSRF target %q, got %d (body=%q)", tt.originalURL, w.Code, w.Body.String())
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestHandlePackageFile_AllowedHostNotRejected(t *testing.T) {
|
||||
// Sanity check: an allowlisted host should NOT be rejected at the SSRF
|
||||
// guard. We don't have a real cache so the call will fail later with a
|
||||
// non-400 status — that's fine, we only assert the SSRF guard didn't
|
||||
// fire.
|
||||
h := newHandlerForSSRFTest()
|
||||
|
||||
path := "/example/example-1.0.0.whl"
|
||||
q := url.Values{}
|
||||
q.Set("original_url", "https://files.pythonhosted.org/packages/abc/example-1.0.0.whl")
|
||||
req := httptest.NewRequest(http.MethodGet, path+"?"+q.Encode(), nil)
|
||||
w := httptest.NewRecorder()
|
||||
|
||||
defer func() {
|
||||
// nil cache will panic when reached — recover and treat as "guard
|
||||
// did not block", which is the property we care about.
|
||||
_ = recover()
|
||||
}()
|
||||
|
||||
h.handlePackageFile(req.Context(), w, req, path)
|
||||
|
||||
if w.Code == http.StatusBadRequest {
|
||||
t.Errorf("allowlisted host wrongly rejected with 400: %s", w.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
func TestRewritePackagePageURLs_QueryEscape(t *testing.T) {
|
||||
// Original URL contains characters that strings.ReplaceAll(&,=) would miss:
|
||||
// '?', '#', '+'. Verify url.QueryEscape handles them.
|
||||
html := `<a href="https://files.pythonhosted.org/packages/x/y+z/foo-1.0.whl?token=abc#frag">link</a>`
|
||||
out := rewritePackagePageURLs(html, "foo", "http://proxy/pypi")
|
||||
|
||||
// '+' must be encoded (otherwise PyPI would interpret as space)
|
||||
if !contains(out, "original_url=https%3A%2F%2Ffiles.pythonhosted.org%2Fpackages%2Fx%2Fy%2Bz%2Ffoo-1.0.whl%3Ftoken%3Dabc%23frag") {
|
||||
t.Errorf("expected fully URL-encoded original_url, got: %s", out)
|
||||
}
|
||||
}
|
||||
|
||||
func contains(s, sub string) bool {
|
||||
return len(s) >= len(sub) && (indexOf(s, sub) >= 0)
|
||||
}
|
||||
|
||||
func indexOf(s, sub string) int {
|
||||
for i := 0; i+len(sub) <= len(s); i++ {
|
||||
if s[i:i+len(sub)] == sub {
|
||||
return i
|
||||
}
|
||||
}
|
||||
return -1
|
||||
}
|
||||
Reference in New Issue
Block a user