Conversation
…arnOfficial#485) date_trunc's first argument and an interval cast both accept a plain text bound parameter in Postgres, so trunc/interval never needed sql.raw() to begin with. Replaced both call sites with normal Drizzle parameter binding. Also retyped truncMap/intervalMap from Record<string, string> to Record<EnrollmentTrendsQuery["range"/"granularity"], string> so adding a new range/granularity enum value without updating the map is now a compile error instead of a runtime undefined reaching the query. 5 new tests covering the schema's enum validation, including SQL-injection-shaped input.
…LearnOfficial#486) Extract the avatar route's filename handling into resolveSafeStaticPath(): path.basename() strips any directory components before the extension allowlist check runs (so an encoded/double-encoded ../ sequence can't survive into the join), then the resolved path is independently verified to fall inside the upload directory before ever touching the filesystem. Also fixes an unrelated pre-existing bug on the same two lines: the old regex literal used \\. (a literal backslash followed by any character) instead of \. (an escaped dot), so the route 404'd on every legitimate avatar filename before this change. 8 new unit tests covering plain traversal, basename-stripped traversal, disallowed extensions, absolute paths, and a null-byte injection attempt.
…earnOfficial#487) Add validateWebhookUrl() (src/utils/ssrf-guard.ts) and wire it into createWebhookSchema/updateWebhookSchema via superRefine, so a webhook pointed at loopback, RFC 1918 private ranges, link-local addresses (including 169.254.169.254, the AWS/GCP metadata endpoint), 0.0.0.0, or localhost is rejected at creation/update time with a message stating why. Only http/https protocols are accepted. 12 new unit tests. Deliberately out of scope: DNS-rebinding protection. This validates the literal hostname a client submits; a hostname that resolves publicly today but is later repointed at an internal address wouldn't be caught, since that requires re-resolving DNS immediately before each dispatch rather than once at creation time, a larger change to the dispatcher itself. Flagging this limitation rather than silently expanding scope into the dispatch path.
…hainLearnOfficial#488) The existing authRateLimit only keys by source IP, so a distributed attempt against one specific stellarAddress from many IPs was unbounded. Add auth-attempt-tracker.ts: Redis-backed per-address failure tracking (INCR + EXPIRE for a sliding window, matching the issue's own suggested key pattern) with an escalating lockout once a threshold is crossed, doubling per failure past it and capped at 15 minutes, plus an audit log entry each time a lockout triggers. Wired into AuthService: createChallenge rejects with RateLimitError (429, Retry-After) before doing any work if the address is currently locked out, so a locked-out address can't even draw a fresh challenge. verifyChallenge is now a thin wrapper around the renamed verifyChallengeInternal (whose own SEP-10 verification logic is untouched): checks the lockout gate first, records a failure on any UnauthorizedError from the inner method, and clears the address's failure history on success. 11 new tests. Deliberately left refresh-token rotation alone: rotateRefreshToken already detects reuse of a spent token and revokes the entire token family immediately (a stronger response than a rate limit, appropriate since any replay of a consumed refresh token indicates compromise, not guessing, refresh tokens are long random secrets, not brute-forceable). Adding a second, weaker per-user rate limit on top would be largely redundant for that endpoint's actual threat model.
|
@theladyanina Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
…LearnOfficial#488) sep10-auth.test.ts and challenge-lockout.test.ts both call into AuthService.createChallenge/verifyChallenge, which now check checkAuthLockout() on every call. Their redis mocks only stubbed setex/getdel, so the added redis.ttl() call inside checkAuthLockout threw "redis.ttl is not a function" in both files (14 failures in sep10-auth.test.ts, the whole suite; all 3 tests in challenge-lockout.test.ts errored on missing config mocks that AuthService also needs). Fixed by extending sep10-auth's redis mock with ttl/incr/expire/del (ttl resolving to -2, meaning "no active lockout", so existing test scenarios are unaffected), and adding the same config/stellar.js and config/database.js mocks challenge-lockout.test.ts's AuthService import needs but was missing. Verified against a clean upstream/main checkout: before this fix, this branch failed 2 more test files than main (54 vs 52); after, the sets of failing files are identical, confirming the pre-existing failures are unrelated to this branch and this was a genuine regression from ChainLearnOfficial#488, now fixed.
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.
Summary
Four security issues from the same batch: a SQL injection vector via sql.raw(), a path traversal gap in avatar serving (which also turned out to be completely non-functional due to an unrelated regex bug on the same lines), missing SSRF protection on webhook URLs, and no per-account brute-force protection on auth endpoints.
closes #485
closes #486
closes #487
closes #488
Changes
course.service.ts's enrollment-trends query interpolatedtrunc/intervalmap lookups into raw SQL text viasql.raw(). Both values are now bound parameters instead:date_trunc()'s first argument and an interval cast both accept a plain text bound parameter in Postgres, sosql.raw()was never actually necessary. Also retypedtruncMap/intervalMapfromRecord<string, string>toRecord<EnrollmentTrendsQuery["range"/"granularity"], string>, so adding a new enum value to the query schema without updating the map is now a compile error instead of a runtimeundefinedreaching the query. 5 new tests, including SQL-injection-shaped input rejected by the schema.resolveSafeStaticPath()(src/utils/safe-static-path.ts):path.basename()strips any directory components before the extension allowlist runs, then the resolved path is independently verified to fall inside the upload directory before ever touching the filesystem. Also fixes an unrelated bug on the same two lines: the old regex literal used\\.(a literal backslash followed by any character) instead of\.(an escaped dot), so this route 404'd on every legitimate avatar filename before this change, confirmed by testing the regex directly againstmain. 8 new unit tests covering plain traversal, basename-stripped traversal, disallowed extensions, absolute paths, and a null-byte injection attempt.validateWebhookUrl()(src/utils/ssrf-guard.ts), wired intocreateWebhookSchema/updateWebhookSchemaviasuperRefine. Rejects loopback, RFC 1918 private ranges, link-local addresses (including169.254.169.254, the AWS/GCP metadata endpoint),0.0.0.0,localhost, and any non-http(s) protocol, with a message stating why. 12 new unit tests.webhook-dispatcher.ts, a larger change than validating the submitted URL. Flagging this limitation rather than silently expanding scope into the dispatch path.authRateLimitonly keys by source IP, so a distributed attempt against one specificstellarAddressfrom many IPs was unbounded. Addedauth-attempt-tracker.ts: Redis-backed per-address failure tracking (INCR+EXPIREfor a sliding window, matching the issue's own suggestedauth:attempts:{stellarAddress}key pattern) with an escalating lockout once a threshold is crossed (doubling per failure past it, capped at 15 minutes), plus an audit log entry each time a lockout triggers.createChallengenow rejects withRateLimitError(429,Retry-After) before doing any work if the address is locked out.verifyChallengeis now a thin wrapper around the renamedverifyChallengeInternal(whose own SEP-10 verification logic is untouched): checks the lockout gate first, records a failure on anyUnauthorizedErrorfrom the inner method, clears the address's failure history on success. 11 new tests.refreshalone:rotateRefreshTokenalready detects reuse of a spent refresh token and revokes the entire token family immediately, a stronger response than a rate limit, and appropriate since refresh tokens are long random secrets (not brute-forceable) so any replay of a consumed one indicates compromise, not guessing. A second, weaker per-user rate limit on top would be largely redundant for that endpoint's actual threat model.Test plan
#485) + 8 (#486) + 12 (#487) + 11 (#488) = 36 new tests acrosstests/unit/, all pass.npx tsc --noEmitandnpx eslinton every changed file, same error/warning count as an unmodifiedmaincheckout.tests/e2e/webhooks.test.tssuite's URLs (https://example.com/webhooks) still pass the new SSRF validator, so "existing valid webhooks are unaffected" per SSRF risk in webhook URL validation #487's acceptance criteria.Caveats / pre-existing issues found while working on this, unrelated to this PR's diff (verified against a pristine
maincheckout before attributing):npx tsc --noEmitfails onmainitself with syntax errors insrc/modules/credentials/credential.service.ts,src/modules/quizes/quiz.service.ts(a separatequizes/typo directory), andsrc/modules/rewards/reward.service.ts, same 10 errors before and after this diff.src/modules/courses/course.service.tshas a duplicateexport class CourseService { ... }block that already exists onmain, unrelated to the enrollment-trends fix in this same file.Update: a follow-up commit (
test(auth): mock ttl/incr/expire/del on redis for auth lockout) fixes a regression the #488 work introduced into the pre-existingtests/unit/services/sep10-auth.test.tssuite: that file'sredismock only stubbedsetex/getdel, so the newcheckAuthLockout()call insidecreateChallenge/verifyChallenge(viaredis.ttl()) threwredis.ttl is not a functionacross all 14 of its tests. Extended that mock withttl/incr/expire/del(all inert,ttlresolving to "no active lockout") so those 14 pre-existing tests pass again unchanged. Verified via agit worktreediff against a cleanupstream/maincheckout that this branch now fails exactly the same set of pre-existing-broken test files asmainitself, no more and no fewer.