fix(security): remediate audit findings and resolve dependency vulnerabilities - #248
Merged
Conversation
added 4 commits
August 18, 2026 22:26
Clears every advisory reported by `pnpm audit` (2 critical, 51 high, 47 moderate, 7 low) across the workspace. Direct dependency bumps: - axios ^1.15.2 -> ^1.18.0 (catalog + backend) - mongoose ^9.1.1 -> ^9.7.2 - @sveltejs/kit ^2.58.0 -> ^2.70.2 - vite ^7.3.2 -> ^7.3.5 - postcss ^8.5.12 -> ^8.5.23 Transitive packages are pinned via pnpm overrides, each scoped to the major line actually present in the lockfile so no transitive consumer is forced across a major boundary. The one exception is uuid, which has no fix on the 10.x line (GHSA-w5hq-g745-h8pq is patched only in 11.1.1); it is a test-only path via testcontainers > dockerode. Notable: @xhmikosr/decompress 10.2.0 -> 10.2.1 and tar 7.5.11 -> 7.5.21 close the two critical advisories. Verified: `pnpm audit` reports 0 vulnerabilities; backend, frontend and status-page all build clean.
Four parallel audits (backend authz, backend injection, backend config and secrets, frontend) plus targeted fixes. Highlights, most severe first. Backend - authorization - Cross-tenant IDOR: DELETE /projects/:projectId/metric-register/:id was guarded on projectId but queried on _id alone, so any user could delete another tenant's metric register entry and cascade-delete its history. Now filtered on both, 404 on mismatch. - ClusterMemberGuard.checkForClusterInviteId authorized the *invite's* role instead of the caller's, so any authenticated user passed for any invite id. Now resolves the caller's role, with an addressee fallback so declining an invitation still works for a non-member. - DELETE /public_dashboards/:id/monitors/:monitorId returned another tenant's dashboard because the guard resolved the monitor param first. - The negative role cache returned the string 'null' (truthy), leaving every `if (!role) throw Forbidden` check dead for the cache TTL. - Deleted an unused ProjectMemberGuard whose membership check was `true`. Backend - authentication - OAuth account takeover: GitHub's `verified` and Google's `email_verified` flags were read and discarded, so an attacker could set a victim's address as an unverified primary email and sign in as them. Both are now required, and a login is refused when the stored authMethod differs from the presenting provider. - `forceLocalLogin` was an undecorated field on the public login DTOs, so a client could make production redeem codes minted for the localhost OAuth app. Removed from the DTOs and derived server-side. - AdminGuard failed open when ADMIN_SUPER_SECRET_ADMIN_KEY was unset (both sides `undefined`); it now rejects and compares in constant time. - CLI device authorization was phishable: the consent link pre-filled the user code, nothing bound the request to the initiating client, and one click minted an all-access key with no expiry. The code must now be typed, start() records the initiating IP and user-agent for display, access is an explicit choice, keys expire, and approve is rate limited. - JWT verification now pins algorithms: ['HS256']. - getOurEnv() no longer defaults to Local, and the hardcoded PERSONAL_API_KEY_HMAC_SECRET fallback is gone. Backend - injection and untrusted input - SSRF: HTTP monitor URLs were validated with @isurl() only, so a monitor could be pointed at cloud metadata or internal services and the first 1000 chars of the response were stored and readable. New shared guard rejects loopback/RFC1918/link-local/CGNAT/IPv6-ULA/metadata targets and revalidates every redirect hop. - The notification-channel PUT handler never validated options, bypassing both the free-tier restriction and all webhook validation - full SSRF with attacker-chosen method, headers and body. - ValidationPipe now runs with whitelist + forbidNonWhitelisted. This required decorating seven DTOs that had no class-validator metadata at all (and were therefore completely unvalidated). - Personal API key scopes/access are validated as nested classes; an `ids` string previously turned a membership test into a substring match. - Six ClickHouse queries moved from interpolation to bound query_params. Backend - exposure and abuse - Deleted three public debug routes, including /redis-benchmark/:iterations which took an unvalidated count and would OOM the process on one request. - Added opt-in rate limiting on account creation, OAuth login/claim and CLI auth. Ingest is deliberately excluded. - Stopped logging the Telegram bot token, full project API keys and session JWTs; the API key Redis cache is keyed on a digest rather than the key. - Added helmet, an explicit body size limit, and a namespace length cap. Frontend - Open redirect in both OAuth callbacks via the unsigned `state` blob. - OAuth state is now a random nonce in an httpOnly cookie, closing login CSRF; consent flags no longer round-trip through client-controlled state. - The /ingest PostHog proxy forwarded the session cookie to a third party on every analytics event. - Session cookie scoped to /app; access tokens and OAuth codes are no longer written to the log pipeline; cookie maxAge was in ms where seconds were expected (a 7-day JWT produced a ~19-year cookie). - logout and user/upgrade moved from GET to POST. CI and repo hygiene - Encrypted backups derived their key with a single SHA-256 pass; now PBKDF2-HMAC-SHA256 at 600k iterations. decrypt-backup.sh no longer falls back to treating unrecognised input as plaintext. - Backup artifact upload is gated behind an explicit dispatch input - this is a PUBLIC repo and scheduled runs were publishing production dumps. - Pinned all 11 third-party actions to commit SHAs; appleboy/ssh-action was on @master and receives the production SSH key and every app secret. - Removed hardcoded production ingest keys from stress-test and deleted the stale .env.vault. Production image - apps/backend/Dockerfile builds with npm from apps/backend/package-lock.json and never saw the root pnpm overrides, so the container still shipped 11 vulnerable packages the workspace had already pinned. Mirrored the overrides into the backend manifest and regenerated the lockfile; npm audit now reports 0 alongside pnpm audit. Test infrastructure - Fixed a config bug this surfaced: envConfigs is one literal holding every environment's branch, so an eager requireEnv demanded prod secrets in local and test runs. Made the secret lazy. - Centralised the throttler reset and drained the ClickHouse TRUNCATEs in the shared bootstrap (query() left the response stream open, so a truncate could land after the next test began writing). - Aligned the test ValidationPipe with production, added a jest env setup, and raised the 5s default timeout that e2e suites could not meet. Verified: pnpm audit and npm audit both report 0 vulnerabilities; backend, frontend and status-page build; svelte-check reports 0 errors in both frontend apps; backend suites pass in CI-style batches.
The previous commit gated the artifact upload behind a workflow_dispatch input, which meant scheduled runs dumped production and discarded it. That traded a confidentiality risk for a data-loss risk without asking, and the call belongs to whoever can vouch for the key. BACKEND_BACKUP_ARTIFACT_KEY is a high-entropy random value, so the encrypted dumps are not meaningfully readable by artifact downloaders and the pipeline is restored to its original behaviour. The PBKDF2-HMAC-SHA256 (600k iteration) key derivation from the previous commit is kept - it is a strict improvement over the single SHA-256 pass and costs nothing. The file header now records why the key's entropy is the control that matters, and leaves the private-bucket migration as a TODO.
The blockquote and step 5 said artifact upload was gated off behind a publish_artifact input. No such input exists and both backup workflows upload unconditionally, so the doc described behaviour the CI does not have.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
logdash-io | f7d3092 | Commit Preview URL Branch Preview URL |
Aug 29 2026, 11:41 AM |
setup-node@v5 enables package-manager-cache by default and resolves the package manager from the root package.json packageManager field (pnpm). pnpm is never installed on these runners, so every job using the shared templates failed at setup with 'Unable to locate executable file: pnpm'. - find-tests: drop the setup-node step entirely, the action only runs bash - test / apply-mongo-migrations: cache npm against apps/backend/package-lock.json - apply-clickhouse-migrations: disable package-manager-cache, nothing is installed from a lockfile
b-sw
reviewed
Aug 29, 2026
| data: METHODS_WITH_BODY.includes(dto.method) ? dto.bodyToSend : undefined, | ||
| }); | ||
| } catch (error) { | ||
| this.logger.error('Failed to send message to webhook', { |
Contributor
There was a problem hiding this comment.
P1 — webhook credentials are exposed to logs. On any network or non-2xx failure this logs the complete destination URL, every custom header value (commonly Authorization), the request body, and the remote response body. That turns routine webhook failures into credential disclosure to the internal log stream. Log only the destination origin/status, header names, and bounded sizes; redact URL credentials/query parameters and all header values, and sanitize/bound the remote error payload.
b-sw
reviewed
Aug 29, 2026
| @@ -40,10 +40,7 @@ export class GithubAuthClaimService { | |||
| public async claimAccount(dto: GithubClaimProjectBody): Promise<TokenResponse> { | |||
| this.logger.log(`Claiming account`, { accessToken: dto.accessToken }); | |||
Contributor
There was a problem hiding this comment.
P1 — active session JWT is logged. This writes the caller's bearer session token before it is even verified. In the new-account claim path the same user record survives, so that token remains usable until its normal JWT expiry by anyone who can read the auth log stream. Remove accessToken from the log context, log the verified user ID instead, and add logger-level secret redaction. The Google claim service has the same issue.
b-sw
reviewed
Aug 29, 2026
| for (let hop = 0; hop <= MAX_REDIRECTS; hop++) { | ||
| await assertPublicUrl(currentUrl); | ||
|
|
||
| const response = await axios.request({ |
Contributor
There was a problem hiding this comment.
P1 — redirect responses can exhaust process memory. With maxRedirects: 0, Axios buffers and decompresses the complete redirect response before this code can inspect Location. There is no hard response-size limit, and the monitor's 10-second timeout restarts for every hop, so an attacker-controlled endpoint can return a large body at each redirect and tie up memory/workers for roughly six timeout windows. Stream and immediately destroy redirect bodies (or enforce a small non-overridable cap) and apply one end-to-end abort deadline across all hops.
Contributor
|
P1 — rejected dashboard creation persists a cross-tenant public dashboard.
Resolve every unique monitor and verify its project belongs to the requested cluster before creating the document, and require the number resolved to equal the number submitted. Existing invalid dashboards should also be cleaned up. |
SSRF wrapper (safe-http-request / safe-url): - pin the vetted address. assertPublicUrl now returns every address it checked and the request connects through an agent whose lookup resolves to exactly those, closing the dns rebinding bypass where a TTL 0 record alternates between a public address and 169.254.169.254 - drop authorization/cookie/proxy-authorization/www-authenticate when a redirect leaves the origin, and downgrade 303 (and 301/302 on POST) to a bodyless GET. Manual hop following had lost what follow-redirects did - cap every hop at 5 MiB and put one deadline across the whole chain instead of restarting the timeout per hop Logging: - webhook failures log the destination origin, header names, status and response size only. The full url, header values, request body and remote response body no longer reach the log stream - the github and google claim services no longer log the caller's session jwt before verifying it; they log the verified user id instead - AggregateLogger redacts credential shaped keys as defense in depth Throttling: - the OAuth login/claim routes are called server to server by the BFF, so keying on req.ip made the 10/min budget a service wide cap and 429'd the 11th sign in each minute. Replaced with a 300/min backstop, documenting that per user limiting belongs at the BFF Backup encryption: - HMAC now covers IV || ciphertext. The IV sits in the plaintext header and was unauthenticated, so it could be rewritten while decrypt-backup.sh still reported a passing HMAC and produced a different first block Frontend: - drop the dev and isLocal imports left unused by the is_local_env removal Adds tests covering the pinned lookup, cross origin header stripping, the 303 downgrade, the response cap, the shared deadline and the redaction.
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
2 participants
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.
Remediates the findings from a full-codebase security audit, plus the 107 known dependency vulnerabilities (pinned via
pnpm.overrides, withaxios,mongoose,@sveltejs/kit,viteand friends bumped to patched versions).Backend hardening: a new SSRF guard (
shared/ssrf/) that validates user-supplied URLs and re-checks every redirect hop, applied to HTTP monitors and webhook channels; opt-in per-IP throttling on account-creation and CLI-polling routes;helmet, CSP, a body-size limit andwhitelist/forbidNonWhitelistedvalidation inmain.ts; parameterised ClickHouse queries; constant-time secret comparison in the admin guard and Telegram setup; OAuth now verifies the provider's email is verified and rejects cross-provider logins; and several authorization gaps closed (cached cluster role returning'null'as a truthy role, metric-register and public-dashboard IDOR,forceLocalLoginletting a request pick the dev OAuth app).Frontend: OAuth
statemoved out of a client-forgeable base64 query param into a short-lived httpOnly cookie, open redirects clamped to same-origin paths, logout and tier-upgrade switched from GET to POST, the session cookie scoped to/app, and the PostHog/ingestproxy no longer forwards first-party credentials upstream or lets the vendor set cookies on our origin.CI: backup encryption moved from a single unsalted SHA-256 pass to PBKDF2-HMAC-SHA256 at 600k iterations, the restore path now fails closed instead of copying unauthenticated files through, third-party actions are pinned to SHAs, and the committed
.env.vaultplus hardcoded stress-test API keys are gone.Also documents the reworked CLI device-authorization flow in ADR-0003: the user code is now typed rather than carried in a link, approval requires an explicit access choice, and CLI-minted keys always expire after 30 days.
Review notes
Four issues were raised on the diff and are not addressed here:
@ThrottleAccountCreation()on the GitHub/Google auth controllers keys onreq.ip, but those routes are only called server-to-server by the SvelteKit BFF, so the limit is effectively 10 logins per minute for the whole product rather than per user.assertPublicUrlresolves the hostname and then hands the hostname to axios, which resolves it again, leaving the SSRF guard open to DNS rebinding.safeHttpRequestreplays headers and body verbatim across redirect hops, dropping the cross-host credential strippingfollow-redirectsgave us.Verification
Backend
tsc --noEmitclean, frontendsvelte-check0 errors, and every e2e suite this branch touches passes.🤖 Generated with Claude Code