harden auth-free redirect, ssrf pinning, first-admin and quota races, service.name - #263
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes five additional issues surfaced by KIberblick.de during the review that produced #262. They were shared as unverified leads; each was confirmed against the code, fixed, and tested. Independent commits, no database migrations.
1. Open redirect on auth-free login/register
In
authMode === 'none'deployments, the login and register pages forwarded the user-suppliedredirectquery param viagoto(redirectUrl)with no validation, while the normal submit path already checked it. A sharedisSafeInternalPath/safeRedirecthelper now guards both paths on both pages, requiring a single-leading-slash path and rejecting protocol-relative forms including the backslash variant (/\evil.com) some browsers normalize to//.2. SSRF guard DNS rebinding
safeFetchresolved+validated the host, then letfetch()re-resolve at connect, leaving a resolve-then-connect window (the TCP monitor path pinned, the HTTP path did not). The HTTP(S) path now connects through a per-request undici dispatcher whose lookup is pinned to the validated IP; TLS SNI and certificate validation still use the original hostname. Addsundicias a direct backend dependency.3. First-admin bootstrap race
createUserdecided the automatic first-admin promotion with a non-atomichasAnyAdmin()check then a separate insert, so concurrent registrations in the zero-admin window could all become admin. The check+insert now runs in a transaction holding a Postgres advisory lock, so at most one wins. Only reachable before the first admin exists (closed entirely whenINITIAL_ADMIN_*is set).4. Capability limit check-then-act race
Resource-creating routes (api keys, custom dashboards, alert rules, sigma import/enable, notification channels) ran
COUNT -> assertWithinLimit -> insertwithout serialization, so parallel requests could each pass the check then all insert, exceeding a finite cap (quota bypass, not a tenant boundary; OSS default has no finite limits). A sharedwithLimitLocknow serializes the count+create per org+capability via an in-process mutex wrapping a transaction-scoped Postgres advisory lock. The in-process mutex bounds DB-lock waiters to one per key per instance, so waiters never hold a connection while queued (an earlier transaction-only version exhausted the pool under concurrency).5. Defense-in-depth on OTLP service.name
Complementing the service-map output-encoding in #262, ingested
service.name(logs, spans, metrics) now runs through a sharedsanitizeServiceNamethat strips control characters (C0/DEL/C1, incl. null bytes) and caps length, while preserving otherwise legitimate characters (escaping still happens at each sink). Keeps a raw payload from resurfacing through a sink added later.Tests
users-service.test.ts41/41 incl. a concurrent-registration race test.apikeys-limit.test.ts5/5 incl. a concurrent-create race test; all capability suites 44/44; route suites for the wrapped modules 146/146.ssrf-guard.test.ts18/18 incl. pinned-lookup tests.redirect.test.ts9/9;svelte-checkat baseline.tsc --noEmitclean.Reported via KIberblick.de (https://kiberblick.de), coordinated disclosure.