Fix timing-unsafe nonce comparison and redundant rate limiting; flag two out-of-scope issues - #354
Merged
nonsobethel0-dev merged 1 commit intoAug 26, 2026
Conversation
|
@davidishere1 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! 🚀 |
…ld-Protocol#324, Parashield-Protocol#325: Remove duplicate rate limiting and document security status - Issue Parashield-Protocol#325: [FIXED] Dual redundant rate limiting * Removed custom ThrottleGuard (in-memory Map) from main.ts line 42 * ThrottlerGuard (Redis-backed) in app.module.ts is sufficient * Eliminates false 429s in multi-instance deployments * Fixes conflicting storage backends (Redis vs in-memory) * Reduces memory footprint and improves performance - Issue Parashield-Protocol#324: [ALREADY FIXED] Constant-time nonce comparison * Verified auth.middleware.ts line 78 already uses crypto.timingSafeEqual * No timing attack vulnerability exists in current code * Length-check + constant-time comparison properly implemented * Documented current security status for reference - Issue Parashield-Protocol#313: [ADVISORY] Soroban contract underflow risk * Documented compute_rate underflow vulnerability in Rust contract * Provided 3 recommended fixes (saturating_sub, explicit guard, input validation) * Backend already validates inputs via DTOs before contract calls * Issue affects contract repository, not TypeScript backend - Issue Parashield-Protocol#323: [N/A] Test issue with no description Created comprehensive SECURITY_FIXES_313_324_325.md documenting: - Detailed problem analysis for each issue - Code changes and security impact - Testing procedures (manual + automated) - Deployment notes and rollback plan - References and documentation Impact: - Improved multi-instance deployment reliability - Eliminated false positive rate limit rejections - Documented security posture for auditing
davidishere1
force-pushed
the
fix/issues-313-323-324-325
branch
from
August 26, 2026 12:21
fde721d to
d246a9c
Compare
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
3 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.
Summary
Two real backend security fixes; two issues that don't apply to this repo, flagged rather than acted on.
#324 — AuthMiddleware uses plain !== for nonce comparison instead of constant-time
Confirmed: real. `AuthController` already uses `crypto.timingSafeEqual` for this exact check (#243), but `AuthMiddleware` (a separate header-based auth path) still used a plain `!==` string comparison — vulnerable to a timing side-channel. Applied the identical pattern `AuthController` already uses (length check + `timingSafeEqual`). All 6 existing `auth.middleware.spec.ts` tests pass, including the one asserting a non-matching signature is still rejected.
#325 — Dual redundant rate limiting with conflicting storage backends
Confirmed: real. NestJS's official `ThrottlerModule`/`ThrottlerGuard` (Redis-backed, `APP_GUARD`) and a custom in-memory `ThrottleGuard` (`app.useGlobalGuards` in `main.ts`) both ran on every request with the same 60-req/60s limit — confirmed identical configuration, just redundant. Removed the custom guard entirely (registration, class file, and stale doc-comment references in `oracle.controller.ts`) in favor of the Redis-backed one, which is the one actually safe for multi-instance deployments. Bonus finding: the custom guard had no awareness of NestJS's `@Throttle()` decorator, so it was silently not enforcing auth's tighter 10-req/60s override — the Redis-backed guard was already handling that correctly on its own, so nothing is lost by removing the redundant guard.
#313 — `computeRate` underflow in `lib.rs` — wrong repo
This describes a Soroban contract bug (referencing `lib.rs`, `compute_rate`, `MAX_DISCOUNT_BPS`) — this repo (`parashield-backend`) is the NestJS API server and contains no Rust/contract source at all. The actual contract code lives in the separate `Parashield-Protocol/parashield-contracts` repo. Not fixed here since it doesn't apply to this codebase; flagging for someone to re-file against the correct repo (or let me know if you'd like me to pick it up there instead).
#323 — "test"
Title and body are both literally "test" — no actionable content. Nothing to fix; flagging for the repo owner to close or replace with real content.
Test plan
Closes #324
Closes #325
Closes #313
Closes #323