docs: add environment variables reference and secret rotation guides - #714
docs: add environment variables reference and secret rotation guides#714AdEmOnD07 wants to merge 2 commits into
Conversation
|
@AdEmOnD07 is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@AdEmOnD07 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! 🚀 |
📝 WalkthroughWalkthroughThe change adds environment-variable documentation, restores Soroban configuration fields, improves logging and Redis diagnostics, expands campaign query validation, updates reconciliation and search behavior, and adds backend tests for middleware, refunds, routes, SSRF protection, concurrency, and cleanup. ChangesBackend configuration and documentation
Validation and backend behavior
Test coverage and tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/services/cache.ts`:
- Around line 27-42: Update the Redis event handlers and initialization catch
block around redisClient.connect() to match the logger API: pass the required
fields and configured level to logInfo, and pass the original error object plus
context and configured level to logError instead of only error.message. Preserve
the existing connection state updates and messages while ensuring all calls
satisfy the signatures defined by logInfo and logError.
- Around line 27-34: Update the Redis event handling around the client listeners
so isConnected becomes true only on the client’s ready event (or via
client.isReady), not on connect. Modify closeRedisCache() to close whenever a
Redis client instance exists, regardless of isConnected, while preserving the
existing cleanup behavior.
In `@README.md`:
- Around line 510-512: Remove the machine-local file:///c:/Users/user/Drips/...
link from the README environment-variable reference, keeping the existing
repository-relative docs/ENVIRONMENT.md link and its surrounding text unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 184074f5-5026-482f-a7c1-ed3de9f87a79
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
README.mdbackend/src/config.tsbackend/src/services/cache.tsbackend/src/validation/schemas.tsdocs/ENVIRONMENT.mdpr-body.md
| logError("Redis client error", { error: err.message }); | ||
| isConnected = false; | ||
| }); | ||
|
|
||
| redisClient.on("connect", () => { | ||
|
|
||
| logInfo("Redis cache connected"); | ||
| isConnected = true; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate cache.ts and package files =="
fd -a 'cache\.ts$|package\.json$|tsconfig\.json$' . | sed 's#^\./##' | head -200
echo
echo "== backend/src/services/cache.ts outline and relevant lines =="
if [ -f backend/src/services/cache.ts ]; then
wc -l backend/src/services/cache.ts
ast-grep outline backend/src/services/cache.ts || true
sed -n '1,180p' backend/src/services/cache.ts
else
echo "backend/src/services/cache.ts not found"
fi
echo
echo "== package redis versions =="
rg -n '"redis"|dependencies|devDependencies' package.json backend/package.json 2>/dev/null || true
jsonfiles="$(fd '^package.json$' . | tr '\n' ' ')"
python3 - <<'PY'
import glob, json
for p in glob.glob("**/package.json", recursive=True):
try:
with open(p) as f:
data=json.load(f)
for sec in ("dependencies","devDependencies","peerDependencies"):
deps=data.get(sec,{})
if "redis" in deps:
print(p, sec, deps["redis"])
except Exception as e:
pass
PY
echo
echo "== search for closeRedisCache and isConnected usages =="
rg -n "closeRedisCache|isConnected|redisClient|logError|logInfo" backend/src .github 2>/dev/null || trueRepository: ritik4ever/stellar-goal-vault
Length of output: 9780
🌐 Web query:
ioredis 4.6.13 connect event ready event ready only once documentation
💡 Result:
The ioredis ready event is not guaranteed to fire only once during the entire lifecycle of a Redis client instance [1][2]. It reflects the status of the connection rather than a singular initialization milestone [3][4]. Key points regarding the ready event behavior: 1. Lifecycle and Reconnections: The ready event is emitted whenever the client transitions to a "ready" state [3][4]. If the connection to the Redis server is lost and subsequently re-established (a reconnection), the client will emit connect followed by ready again, provided the conditions are met [1][2][3]. 2. Condition for ready: - If enableReadyCheck is true (the default), the ready event is emitted only after the connection is established and the server reports it is ready to receive commands (e.g., it has finished loading data from disk or, in a Cluster setup, is reported as ready by CLUSTER INFO) [5][1][6]. - If enableReadyCheck is false, the ready event is emitted immediately after the connect event [1][2][3]. 3. Connection vs. Ready: The connect event signifies that the TCP or TLS socket has been successfully established [3]. However, at that point, authentication, SELECT commands, and other handshakes may still be in progress [3]. The ready event serves as the appropriate signal that the client is fully initialized and ready to process commands [3]. Because ioredis handles automatic reconnections by default, your application should be prepared to handle the ready event multiple times if the connection is unstable or restarts [3]. If you require code to run only once, you should manage that state manually within your event listener (e.g., using a boolean flag or the .once() event listener method) [3].
Citations:
- 1: https://ioredis.readthedocs.io/en/stable/README/
- 2: https://github.com/redis/ioredis?tab=readme-ov-file
- 3: https://deepwiki.com/redis/ioredis/5.2-events
- 4: https://github.com/redis/ioredis/blob/main/lib/redis/event_handler.ts
- 5: https://github.com/redis/ioredis/blob/v4/API.md
- 6: https://redis.github.io/ioredis/interfaces/CommonRedisOptions.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate Redis library docs/source if present =="
fd -a '.*(redis|ioredis).*|package-lock.json|yarn.lock|pnpm-lock.yaml' . | sed 's#^\./##' | head -200
echo
echo "== lockfile redis entries =="
for f in package-lock.json backend/package-lock.json pnpm-lock.yaml yarn.lock; do
[ -f "$f" ] && { echo "--- $f"; rg -n 'redis@|node_modules/redis|ioredis[^\n]*' "$f" | head -80 || true; }
done
echo
echo "== deterministic source inspection for event handler logic =="
python3 - <<'PY'
from pathlib import Path
p=Path('backend/src/services/cache.ts')
if not p.exists():
print("missing backend/src/services/cache.ts")
raise SystemExit
s=p.read_text()
for needle in ['redisClient.on("error"', 'redisClient.on("connect")', 'closeRedisCache', 'redisClient && isConnected']:
print(f"\n--- {needle!r} ---")
for i,line in enumerate(s.splitlines(),1):
if needle in line:
print(f"{i}: {line}")
PYRepository: ritik4ever/stellar-goal-vault
Length of output: 805
🌐 Web query:
Redis JavaScript client v4.6.13 connect event ready event quit behavior
💡 Result:
In Redis JavaScript client (node-redis) version 4.6.13, the client is an EventEmitter that emits specific lifecycle events to signal network and state changes [1][2]. Below is a summary of the requested event and quit behaviors: Connection and Ready Events - connect: Emitted when the client is initiating a connection to the Redis server [1][2]. It signals that the process of establishing the network socket has begun [3]. - ready: Emitted once the client has successfully connected and is ready to execute commands [1][2]. This indicates that the initial handshake (if applicable) is complete, and the client is fully initialized [4]. Quit and Disconnection Behavior -.quit: This method is intended to close the connection gracefully [1][5]. It sends the QUIT command to the Redis server, ensuring that pending commands in the queue are processed and replied to before the connection is closed [1][5]. -.disconnect: In version 4, this method is deprecated in favor of newer methods [2][6][5]. Depending on the specific sub-version or transition, it may be aliased to.destroy or have similar behavior [2][5]. - end Event: This event is emitted when the connection has been successfully closed, typically following a call to.quit or.disconnect (or their newer counterparts.close and.destroy) [1][2][7]. Best Practices - Error Handling: It is critical to register a listener for the error event [1][8]. If an error occurs (such as a lost connection or network issue) and no error listener is attached, the Node.js process will throw the error and exit [1][2][8]. - Checking Status: Rather than relying solely on events, you can check the client state synchronously using client.isReady (to see if it can send commands) or client.isOpen (to see if the underlying socket is open) [1][9][3]. - Modern Alternatives: While.quit is functional, newer documentation often suggests using.close for graceful shutdowns and.destroy for immediate disconnection [2][6]. Note that the native Redis QUIT command is deprecated in newer Redis server versions, and the client implementation has evolved to reflect this [2][6].
Citations:
- 1: https://npmx.dev/package/redis/v/%5E4.6.13
- 2: https://github.com/redis/node-redis/
- 3: https://redis.io/docs/latest/develop/clients/nodejs/connect/
- 4: https://github.com/redis/node-redis/blob/master/packages/client/lib/client/socket.ts
- 5: https://github.com/redis/node-redis/blob/master/packages/client/lib/client/index.ts
- 6: https://github.com/redis/node-redis/blob/master/packages/redis/README.md
- 7: https://github.com/redis/node-redis?tab=readme-ov-file
- 8: https://redis.io/docs/latest/develop/clients/nodejs/error-handling/
- 9: https://redis.io/docs/latest/develop/clients/nodejs/
Track Redis readiness separately from shutdown state.
connect indicates socket connection, while ready indicates the Redis JavaScript client has completed its handshake and can execute commands; move isConnected=true to ready/client.isReady. Then closeRedisCache() should close when a client instance exists rather than requiring isConnected, since an errored or disconnected Redis client can still need graceful shutdown cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/services/cache.ts` around lines 27 - 34, Update the Redis event
handling around the client listeners so isConnected becomes true only on the
client’s ready event (or via client.isReady), not on connect. Modify
closeRedisCache() to close whenever a Redis client instance exists, regardless
of isConnected, while preserving the existing cleanup behavior.
| The application is configured using environment variables for the backend service, frontend service, and contract deployment. | ||
|
|
||
| - `PORT` defaults to `3001` | ||
| - `DB_PATH` defaults to `backend/data/campaigns.db` | ||
| - `SOROBAN_RPC_URL` defaults to Stellar testnet RPC | ||
| - `CONTRACT_ID` is required for Freighter pledge signing | ||
| - `NETWORK_PASSPHRASE` defaults to Stellar testnet | ||
| - `CONTRACT_AMOUNT_DECIMALS` defaults to `2` and controls display-to-contract unit scaling | ||
| A complete reference of all environment variables, including descriptions, default values, examples, and security/rotation guidelines, can be found in the [Environment Variables Guide](./docs/ENVIRONMENT.md) (accessible locally at [docs/ENVIRONMENT.md](file:///c:/Users/user/Drips/stellar-goal-vault/docs/ENVIRONMENT.md)). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the machine-local file:/// link.
Line 512 already has the correct repository-relative link; the additional file:///c:/Users/user/Drips/... link is unusable for other contributors and exposes a local filesystem path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 510 - 512, Remove the machine-local
file:///c:/Users/user/Drips/... link from the README environment-variable
reference, keeping the existing repository-relative docs/ENVIRONMENT.md link and
its surrounding text unchanged.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/services/sorobanRpc.ts (1)
96-115: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a timeout to the Soroban RPC request.
axiosdefaults to no timeout, so a hung RPC node can hold the/refundrequest path indefinitely. Captureconfig.sorobanRpcUrlin a localstringand pass an explicit request timeout, e.g.${rpcUrl}withtimeout: 5_000; the existing catch already maps timeout/network errors toSOROBAN_RPC_UNAVAILABLE(502).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/sorobanRpc.ts` around lines 96 - 115, Update the Soroban RPC request in the surrounding function to capture config.sorobanRpcUrl in a local string variable and pass an explicit 5,000 ms timeout in the axios.post options. Keep the existing error handling and JSON-RPC request structure unchanged.
🧹 Nitpick comments (6)
backend/src/logger.ts (1)
65-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCached log level has no invalidation hook.
Once any log call runs, later mutations of
process.env.LOG_LEVEL(tests, config reload) are ignored for the lifetime of the process. Export a reset so suites can control it deterministically.♻️ Proposed reset hook
let globalConfiguredLevel: LogLevel | null = null; + +/** Test/reload hook: forces the next read to re-evaluate `process.env.LOG_LEVEL`. */ +export function resetGlobalConfiguredLogLevel(): void { + globalConfiguredLevel = null; +} + export function getGlobalConfiguredLogLevel(): LogLevel {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/logger.ts` around lines 65 - 71, Export a reset function alongside getGlobalConfiguredLogLevel that clears globalConfiguredLevel back to null, allowing subsequent calls to re-read process.env.LOG_LEVEL. Keep the existing caching behavior unchanged until the reset hook is invoked.backend/src/validation/schemas.ts (2)
356-394: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate to
parseCampaignListPaginationQueryinstead of re-implementing it.This block duplicates Lines 206-260 verbatim (same messages, same 1..100 cap). Two copies will drift.
♻️ Proposed refactor
- const pageStr = singleCampaignListQueryParam(query.page); - const limitStr = singleCampaignListQueryParam(query.limit); - - let page: number | undefined; - let limit: number | undefined; - - if (pageStr === undefined && limitStr === undefined) { - /* … duplicated validation … */ - } + const pagination = parseCampaignListPaginationQuery(query); + let page: number | undefined; + let limit: number | undefined; + if (pagination.ok) { + page = pagination.page; + limit = pagination.limit; + } else { + issues.push(...pagination.issues); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/validation/schemas.ts` around lines 356 - 394, Replace the duplicated pagination validation block with a call to the existing parseCampaignListPaginationQuery helper used around lines 206-260. Reuse its returned page, limit, and validation issues in this flow, preserving the existing behavior and messages without maintaining a second implementation.
169-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the repeated issue literals with one typed helper.
These custom Zod issue objects duplicate the same
customissue shape across multiple parsers and trips@typescript-eslint/no-explicit-any. A small helper removes the repeatedas anycasts. Usez.core.$ZodIssue/z.core.$ZodIssueCustomfor the helper type;z.ZodIssuestill works because Zod 4 exposes it as a deprecated alias, but the core issue type is preferred.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/validation/schemas.ts` around lines 169 - 191, The validation parsers in schemas.ts repeat custom Zod issue literals with as any casts. Add one typed helper using z.core.$ZodIssue or z.core.$ZodIssueCustom to construct these issues, then update the affected parsers to call it for their field, message, and path values while preserving existing validation behavior.Source: Linters/SAST tools
backend/src/validation/stellarAddress.test.ts (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth suites now depend on one replaced Stellar key literal — verify its checksum, then share it.
GAAZI4TCR3TY5OJHCTJC2A4QSY6CJWJH5IAJTGKIN2ER7LBNVKOCCWN7is duplicated across two test files and must satisfy the version-byte + CRC-16/XModem check inbackend/src/validation/stellarAddress.ts(Lines 71-94); if it doesn't, both suites fail or pass for the wrong reason.
backend/src/validation/stellarAddress.test.ts#L11-L11: confirm the literal decodes to 35 bytes with version0x30and a matching CRC, then export it as a shared fixture constant.backend/src/validation/schemas.test.ts#L11-L11: import that shared constant forCREATORinstead of re-declaring the literal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/validation/stellarAddress.test.ts` at line 11, Verify the Stellar address literal used in stellarAddress.test.ts decodes to 35 bytes with version 0x30 and a valid CRC-16/XModem checksum, then export it as a shared fixture constant. In backend/src/validation/schemas.test.ts, import and use that constant for CREATOR instead of duplicating the literal.backend/src/security.test.ts (1)
4-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet test env before importing modules.
importdeclarations are hoisted, so./configreadsprocess.env.CONTRACT_ID/process.env.SOROBAN_RPC_URLbefore the top-level assignments run. Move these assignments into a VitestsetupFilesentry orvi.hoisted(...)and remove thebeforeAllsingleton mutations/comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/security.test.ts` around lines 4 - 16, Move the test environment assignments used by config initialization into Vitest setupFiles or a vi.hoisted block so they execute before importing config and initCampaignStore. Remove the beforeAll mutations of config.contractId and config.sorobanRpcUrl, along with the obsolete import-order comment, while preserving initCampaignStore initialization.backend/src/tests/middleware.test.ts (1)
217-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedis mock call history isn't reset between "Cache Service" tests.
createClientalways returns the samemockClientsingleton (per thevi.mock("redis")factory), andmockClient.on.mock.callsis never cleared in this describe block'sbeforeEach/afterEach..find(call => call[0] === 'connect'|'error')at lines 234 and 252 therefore resolves to the FIRST handler ever registered in the file, not the one from the current test'sinitRedisCache()call. It happens to work today because every registered handler mutates the same module-level flag, but this is fragile against reordering or additional tests.♻️ Proposed fix: clear mock call history per test
beforeEach(() => { originalEnv = process.env.NODE_ENV; originalRedisUrl = process.env.REDIS_URL; + vi.clearAllMocks(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/tests/middleware.test.ts` around lines 217 - 267, Reset the shared Redis client mock’s call history between each “Cache Service” test, using the existing setup/teardown hooks around initRedisCache. Clear mockClient.on (and any related mock state needed by the singleton) before each test so the handler lookups in the production and error callback tests select the current test’s registration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/services/campaignStore.concurrent.test.ts`:
- Around line 246-286: In the concurrent claim/pledge test, make the assertion
for pledge rejection unconditional: require pledgeRes.ok to be false, then
assert its error code is INVALID_CAMPAIGN_STATE. Keep the existing claimRes
success assertion and final campaign-state checks unchanged.
- Around line 246-258: Update the test constants used by claimCampaign so
CREATOR and CONTRIBUTOR_3 have the intended relationship: either make CREATOR
intentionally distinct from the uppercase ...JPBARQ account or align it exactly
with CONTRIBUTOR_3. Ensure the chosen value matches the test’s verbatim
creator-address comparison and remains consistent across the concurrent campaign
operations.
In `@backend/src/services/campaignStore.ts`:
- Around line 361-375: Escape LIKE metacharacters in rawQuery before
constructing creatorExactTerm, including the chosen escape character itself, and
use the same escaped value in both creator LIKE predicates. Add the
corresponding ESCAPE clause to each LOWER(campaigns.creator) LIKE LOWER(?)
condition while preserving substring matching and the existing FTS and exact-ID
parameters.
In `@backend/src/tests/routeHandlers.test.ts`:
- Around line 1-6: Remove the unused getDb import from the routeHandlers test
imports while preserving the other database utilities used by the tests.
- Line 116: Replace the explicit any casts on the Error instances in the
affected route handler tests with a narrow, appropriate error or layer type that
satisfies the tested API contracts. Update the declarations around the err
variables at the referenced test cases while preserving their existing error
messages and behavior.
- Around line 19-21: Move the DB_PATH and NODE_ENV test assignments out of the
top-level setup in routeHandlers.test.ts and into Vitest’s global
setup/configuration so they are applied before any application modules load.
Ensure the app import through index observes TEST_DB_PATH and test mode rather
than inherited production configuration.
In `@backend/src/validation/schemas.ts`:
- Line 396: Remove the unused searchQuery local and simplify the returned
q/search handling in the query normalization logic. Reuse normalizeQueryValue
directly for both query.q and query.search without truthiness guards, preserving
undefined for non-string or blank values.
- Around line 329-336: Update parseAssetCodes to accept Express’s array form for
repeated asset query parameters, while preserving the existing comma-separated
string parsing and null behavior for unsupported or empty input. Normalize array
elements using the same trimming, uppercasing, and filtering rules before
returning the codes.
- Around line 74-89: Update the title and description validation chains in the
schema so containsScriptTag and containsSqlComment refinements run on the
trimmed raw input before sanitizeInput transforms it. Preserve the existing
length checks and validation messages, then sanitize only after the security
refinements so matching script tags and SQL comment sequences are rejected
rather than escaped and accepted.
- Around line 316-327: Update parseIso8601Timestamp to validate the string
against an ISO-8601 format before passing it to Date, rejecting inputs such as
locale-formatted dates and partial month values while preserving null for
invalid timestamps. Keep the existing finite-timestamp check and Unix-seconds
conversion; range-order validation is outside this change.
In `@pr-body.md`:
- Around line 1-5: Update the PR body to reference issue `#647` and accurately
summarize the environment-variable documentation and secret-rotation guidance
objective. If the backend fixes and test coverage from the current Summary are
intentionally included, explicitly describe both scopes; otherwise remove that
unrelated summary to avoid misrepresenting the PR and closing the wrong issue.
---
Outside diff comments:
In `@backend/src/services/sorobanRpc.ts`:
- Around line 96-115: Update the Soroban RPC request in the surrounding function
to capture config.sorobanRpcUrl in a local string variable and pass an explicit
5,000 ms timeout in the axios.post options. Keep the existing error handling and
JSON-RPC request structure unchanged.
---
Nitpick comments:
In `@backend/src/logger.ts`:
- Around line 65-71: Export a reset function alongside
getGlobalConfiguredLogLevel that clears globalConfiguredLevel back to null,
allowing subsequent calls to re-read process.env.LOG_LEVEL. Keep the existing
caching behavior unchanged until the reset hook is invoked.
In `@backend/src/security.test.ts`:
- Around line 4-16: Move the test environment assignments used by config
initialization into Vitest setupFiles or a vi.hoisted block so they execute
before importing config and initCampaignStore. Remove the beforeAll mutations of
config.contractId and config.sorobanRpcUrl, along with the obsolete import-order
comment, while preserving initCampaignStore initialization.
In `@backend/src/tests/middleware.test.ts`:
- Around line 217-267: Reset the shared Redis client mock’s call history between
each “Cache Service” test, using the existing setup/teardown hooks around
initRedisCache. Clear mockClient.on (and any related mock state needed by the
singleton) before each test so the handler lookups in the production and error
callback tests select the current test’s registration.
In `@backend/src/validation/schemas.ts`:
- Around line 356-394: Replace the duplicated pagination validation block with a
call to the existing parseCampaignListPaginationQuery helper used around lines
206-260. Reuse its returned page, limit, and validation issues in this flow,
preserving the existing behavior and messages without maintaining a second
implementation.
- Around line 169-191: The validation parsers in schemas.ts repeat custom Zod
issue literals with as any casts. Add one typed helper using z.core.$ZodIssue or
z.core.$ZodIssueCustom to construct these issues, then update the affected
parsers to call it for their field, message, and path values while preserving
existing validation behavior.
In `@backend/src/validation/stellarAddress.test.ts`:
- Line 11: Verify the Stellar address literal used in stellarAddress.test.ts
decodes to 35 bytes with version 0x30 and a valid CRC-16/XModem checksum, then
export it as a shared fixture constant. In
backend/src/validation/schemas.test.ts, import and use that constant for CREATOR
instead of duplicating the literal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c5c5c86d-a268-45e8-900b-7c5253be5343
📒 Files selected for processing (27)
backend/src/api.test.tsbackend/src/historyEndpoint.test.tsbackend/src/index.test.tsbackend/src/index.tsbackend/src/logger.tsbackend/src/pledgesEndpoint.test.tsbackend/src/rateLimiter.test.tsbackend/src/requestId.test.tsbackend/src/security.test.tsbackend/src/services/__tests__/eventMetadata.test.tsbackend/src/services/__tests__/mutation.test.tsbackend/src/services/campaignStore.concurrent.test.tsbackend/src/services/campaignStore.test.tsbackend/src/services/campaignStore.tsbackend/src/services/eventHistory.tsbackend/src/services/seedDeterministic.tsbackend/src/services/sorobanRpc.tsbackend/src/tests/middleware.test.tsbackend/src/tests/refundLogic.test.tsbackend/src/tests/routeHandlers.test.tsbackend/src/validation/schemas.test.tsbackend/src/validation/schemas.tsbackend/src/validation/stellarAddress.test.tsbackend/src/validation/urlSafety.tsbackend/tests/integration.test.tsbackend/vitest.config.tspr-body.md
| const operations = [ | ||
| claimCampaign(campaignId, CREATOR), | ||
| addPledge(campaignId, { | ||
| contributor: CONTRIBUTOR_3, | ||
| amount: 100, | ||
| assetCode: "USDC", | ||
| }), | ||
| (async () => { | ||
| try { | ||
| const res = claimCampaign(campaignId, { | ||
| creator: CREATOR, | ||
| transactionHash: "a".repeat(64), | ||
| confirmedAt: Math.floor(Date.now() / 1000), | ||
| }); | ||
| return { type: 'claim', ok: true, res }; | ||
| } catch (err) { | ||
| return { type: 'claim', ok: false, err }; | ||
| } | ||
| })(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'CREATOR\s*=' backend/src/services/campaignStore.concurrent.test.ts
rg -n 'isValidEd25519PublicKey|StrKey' backend/src/services/campaignStore.ts backend/src/validationRepository: ritik4ever/stellar-goal-vault
Length of output: 708
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Stellar address validation implementation =="
sed -n '1,130p' backend/src/validation/stellarAddress.ts
echo
echo "== Campaign claim validation usages =="
rg -n 'creator: CREATOR|reconcileOnChainClaim|isValidEd25519PublicKey|isValidContract|stellarAddress' backend/src/services backend/src/validation -S
echo
echo "== Relevant campaignStore section =="
sed -n '220,280p' backend/src/services/campaignStore.concurrent.test.ts
echo
echo "== Stellar SDK package if present =="
node - <<'JS'
try { const mod = require('`@stellar/stellar-sdk`'); console.log('stellar-sdk available'); console.log(mod.StrKey.isValidEd25519PublicKey('GCZST3XVCDTUJ76ZAV2HA72KYQM4YO4EQQ5FILWIXNJNHKS4JF7JVbarq')); console.log(mod.StrKey.isValidEd25519PublicKey('GCZST3XVCDTUJ76ZAV2HA72KYQM4YO4EQQ5FILWIXNJNHKS4JF7JVBARQ')); }
catch (e) { console.log('stellar-sdk not available:', e.message); }
JSRepository: ritik4ever/stellar-goal-vault
Length of output: 11384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== validateStellarPublicKey call sites =="
rg -n 'validateStellarPublicKey|creator|contributor' backend/src/validation/schemas.ts backend/src -S
echo
echo "== schemas implementation =="
sed -n '1,120p' backend/src/validation/schemas.ts
echo
echo "== campaignStore claim paths around claimCampaign/reconcileOnChainClaim =="
sed -n '930,1065p' backend/src/services/campaignStore.ts
echo
echo "== test constants definitions =="
sed -n '1,45p' backend/src/services/campaignStore.concurrent.test.tsRepository: ritik4ever/stellar-goal-vault
Length of output: 43531
Make CREATOR and CONTRIBUTOR_3 match.
CONTRIBUTOR_3 is the uppercase ...JPBARQ version used elsewhere as a valid claim input, while CREATOR stores lowercase ...JVbarq in this test. Since claimCampaign compares creator addresses verbatim here, make this lowercase test account intentionally distinct or align it with the uppercase constant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/services/campaignStore.concurrent.test.ts` around lines 246 -
258, Update the test constants used by claimCampaign so CREATOR and
CONTRIBUTOR_3 have the intended relationship: either make CREATOR intentionally
distinct from the uppercase ...JPBARQ account or align it exactly with
CONTRIBUTOR_3. Ensure the chosen value matches the test’s verbatim
creator-address comparison and remains consistent across the concurrent campaign
operations.
| const operations = [ | ||
| claimCampaign(campaignId, CREATOR), | ||
| addPledge(campaignId, { | ||
| contributor: CONTRIBUTOR_3, | ||
| amount: 100, | ||
| assetCode: "USDC", | ||
| }), | ||
| (async () => { | ||
| try { | ||
| const res = claimCampaign(campaignId, { | ||
| creator: CREATOR, | ||
| transactionHash: "a".repeat(64), | ||
| confirmedAt: Math.floor(Date.now() / 1000), | ||
| }); | ||
| return { type: 'claim', ok: true, res }; | ||
| } catch (err) { | ||
| return { type: 'claim', ok: false, err }; | ||
| } | ||
| })(), | ||
| (async () => { | ||
| try { | ||
| const res = addPledge(campaignId, { | ||
| contributor: CONTRIBUTOR_3, | ||
| amount: 100, | ||
| assetCode: "USDC", | ||
| }); | ||
| return { type: 'pledge', ok: true, res }; | ||
| } catch (err) { | ||
| return { type: 'pledge', ok: false, err }; | ||
| } | ||
| })(), | ||
| ]; | ||
|
|
||
| const results = await Promise.all(operations); | ||
| const claimRes = results.find(r => r.type === 'claim')!; | ||
| const pledgeRes = results.find(r => r.type === 'pledge')!; | ||
|
|
||
| expect(claimRes.ok).toBe(true); | ||
|
|
||
| // Both operations should complete | ||
| expect(results).toHaveLength(2); | ||
| if (!pledgeRes.ok) { | ||
| expect((pledgeRes as any).err.code).toBe('INVALID_CAMPAIGN_STATE'); | ||
| } | ||
|
|
||
| // Verify final state | ||
| const campaign = getCampaign(campaignId); | ||
| expect(campaign).toBeDefined(); | ||
| expect(campaign?.claimedAt).toBeDefined(); // Campaign should be claimed | ||
| // Pledge after claim should still be recorded | ||
| expect(campaign?.pledgedAmount).toBe(600); // 250 + 250 + 100 | ||
| expect(campaign?.claimedAt).toBeDefined(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Weak assertion lets a fund-safety regression pass silently.
expect(claimRes.ok).toBe(true) is solid, but the pledge check is conditional: if (!pledgeRes.ok) { expect(err.code).toBe('INVALID_CAMPAIGN_STATE'); }. If pledgeRes.ok is ever true (i.e., a pledge is incorrectly accepted after/alongside a claim), the test asserts nothing and passes anyway. Since both operations are async IIFEs with no internal await, execution here is actually deterministic (claim's body fully runs before the pledge's body starts), so the pledge should always be rejected — the assertion should be unconditional to actually guard the "no pledging after claim" invariant.
🐛 Proposed fix: make the pledge-rejection assertion unconditional
- expect(claimRes.ok).toBe(true);
-
- if (!pledgeRes.ok) {
- expect((pledgeRes as any).err.code).toBe('INVALID_CAMPAIGN_STATE');
- }
+ expect(claimRes.ok).toBe(true);
+ expect(pledgeRes.ok).toBe(false);
+ expect((pledgeRes as { err: { code: string } }).err.code).toBe('INVALID_CAMPAIGN_STATE');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const operations = [ | |
| claimCampaign(campaignId, CREATOR), | |
| addPledge(campaignId, { | |
| contributor: CONTRIBUTOR_3, | |
| amount: 100, | |
| assetCode: "USDC", | |
| }), | |
| (async () => { | |
| try { | |
| const res = claimCampaign(campaignId, { | |
| creator: CREATOR, | |
| transactionHash: "a".repeat(64), | |
| confirmedAt: Math.floor(Date.now() / 1000), | |
| }); | |
| return { type: 'claim', ok: true, res }; | |
| } catch (err) { | |
| return { type: 'claim', ok: false, err }; | |
| } | |
| })(), | |
| (async () => { | |
| try { | |
| const res = addPledge(campaignId, { | |
| contributor: CONTRIBUTOR_3, | |
| amount: 100, | |
| assetCode: "USDC", | |
| }); | |
| return { type: 'pledge', ok: true, res }; | |
| } catch (err) { | |
| return { type: 'pledge', ok: false, err }; | |
| } | |
| })(), | |
| ]; | |
| const results = await Promise.all(operations); | |
| const claimRes = results.find(r => r.type === 'claim')!; | |
| const pledgeRes = results.find(r => r.type === 'pledge')!; | |
| expect(claimRes.ok).toBe(true); | |
| // Both operations should complete | |
| expect(results).toHaveLength(2); | |
| if (!pledgeRes.ok) { | |
| expect((pledgeRes as any).err.code).toBe('INVALID_CAMPAIGN_STATE'); | |
| } | |
| // Verify final state | |
| const campaign = getCampaign(campaignId); | |
| expect(campaign).toBeDefined(); | |
| expect(campaign?.claimedAt).toBeDefined(); // Campaign should be claimed | |
| // Pledge after claim should still be recorded | |
| expect(campaign?.pledgedAmount).toBe(600); // 250 + 250 + 100 | |
| expect(campaign?.claimedAt).toBeDefined(); | |
| const operations = [ | |
| (async () => { | |
| try { | |
| const res = claimCampaign(campaignId, { | |
| creator: CREATOR, | |
| transactionHash: "a".repeat(64), | |
| confirmedAt: Math.floor(Date.now() / 1000), | |
| }); | |
| return { type: 'claim', ok: true, res }; | |
| } catch (err) { | |
| return { type: 'claim', ok: false, err }; | |
| } | |
| })(), | |
| (async () => { | |
| try { | |
| const res = addPledge(campaignId, { | |
| contributor: CONTRIBUTOR_3, | |
| amount: 100, | |
| assetCode: "USDC", | |
| }); | |
| return { type: 'pledge', ok: true, res }; | |
| } catch (err) { | |
| return { type: 'pledge', ok: false, err }; | |
| } | |
| })(), | |
| ]; | |
| const results = await Promise.all(operations); | |
| const claimRes = results.find(r => r.type === 'claim')!; | |
| const pledgeRes = results.find(r => r.type === 'pledge')!; | |
| expect(claimRes.ok).toBe(true); | |
| expect(pledgeRes.ok).toBe(false); | |
| expect((pledgeRes as { err: { code: string } }).err.code).toBe('INVALID_CAMPAIGN_STATE'); | |
| // Verify final state | |
| const campaign = getCampaign(campaignId); | |
| expect(campaign).toBeDefined(); | |
| expect(campaign?.claimedAt).toBeDefined(); |
🧰 Tools
🪛 ESLint
[error] 280-280: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/services/campaignStore.concurrent.test.ts` around lines 246 -
286, In the concurrent claim/pledge test, make the assertion for pledge
rejection unconditional: require pledgeRes.ok to be false, then assert its error
code is INVALID_CAMPAIGN_STATE. Keep the existing claimRes success assertion and
final campaign-state checks unchanged.
| // Allow case-insensitive substring search for creator public key | ||
| const creatorExactTerm = `%${rawQuery}%`; | ||
| const exactTerm = rawQuery; | ||
|
|
||
| if (ftsMatchTerm) { | ||
| whereClauses.push(`( | ||
| campaigns.id IN (SELECT id FROM campaigns_fts WHERE campaigns_fts MATCH ?) | ||
| OR LOWER(campaigns.creator) = LOWER(?) | ||
| OR LOWER(campaigns.creator) LIKE LOWER(?) | ||
| OR campaigns.id = ? | ||
| )`); | ||
| params.push(ftsMatchTerm, creatorExactTerm, exactTerm); | ||
| } else { | ||
| // Fallback if cleaning the query stripped all characters | ||
| whereClauses.push(`(LOWER(campaigns.creator) = LOWER(?) OR campaigns.id = ?)`); | ||
| whereClauses.push(`(LOWER(campaigns.creator) LIKE LOWER(?) OR campaigns.id = ?)`); | ||
| params.push(creatorExactTerm, exactTerm); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape LIKE wildcards in the user-supplied search term.
rawQuery goes straight into %…%, so a search of % or _ matches every creator and any embedded wildcard silently broadens the match. Escape the metacharacters and declare an ESCAPE character.
🛡️ Proposed fix
- // Allow case-insensitive substring search for creator public key
- const creatorExactTerm = `%${rawQuery}%`;
+ // Allow case-insensitive substring search for creator public key.
+ // Escape LIKE metacharacters so user input cannot act as a wildcard.
+ const likeEscaped = rawQuery.replace(/[\\%_]/g, (ch) => `\\${ch}`);
+ const creatorExactTerm = `%${likeEscaped}%`;
const exactTerm = rawQuery;
if (ftsMatchTerm) {
whereClauses.push(`(
campaigns.id IN (SELECT id FROM campaigns_fts WHERE campaigns_fts MATCH ?)
- OR LOWER(campaigns.creator) LIKE LOWER(?)
+ OR LOWER(campaigns.creator) LIKE LOWER(?) ESCAPE '\\'
OR campaigns.id = ?
)`);
params.push(ftsMatchTerm, creatorExactTerm, exactTerm);
} else {
// Fallback if cleaning the query stripped all characters
- whereClauses.push(`(LOWER(campaigns.creator) LIKE LOWER(?) OR campaigns.id = ?)`);
+ whereClauses.push(`(LOWER(campaigns.creator) LIKE LOWER(?) ESCAPE '\\' OR campaigns.id = ?)`);Note also that LOWER(creator) LIKE '%…%' cannot use an index, so every search now full-scans campaigns twice (count + data). Fine at current volumes; revisit with a normalized lowercase creator column if the table grows.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Allow case-insensitive substring search for creator public key | |
| const creatorExactTerm = `%${rawQuery}%`; | |
| const exactTerm = rawQuery; | |
| if (ftsMatchTerm) { | |
| whereClauses.push(`( | |
| campaigns.id IN (SELECT id FROM campaigns_fts WHERE campaigns_fts MATCH ?) | |
| OR LOWER(campaigns.creator) = LOWER(?) | |
| OR LOWER(campaigns.creator) LIKE LOWER(?) | |
| OR campaigns.id = ? | |
| )`); | |
| params.push(ftsMatchTerm, creatorExactTerm, exactTerm); | |
| } else { | |
| // Fallback if cleaning the query stripped all characters | |
| whereClauses.push(`(LOWER(campaigns.creator) = LOWER(?) OR campaigns.id = ?)`); | |
| whereClauses.push(`(LOWER(campaigns.creator) LIKE LOWER(?) OR campaigns.id = ?)`); | |
| params.push(creatorExactTerm, exactTerm); | |
| // Allow case-insensitive substring search for creator public key. | |
| // Escape LIKE metacharacters so user input cannot act as a wildcard. | |
| const likeEscaped = rawQuery.replace(/[\\%_]/g, (ch) => `\\${ch}`); | |
| const creatorExactTerm = `%${likeEscaped}%`; | |
| const exactTerm = rawQuery; | |
| if (ftsMatchTerm) { | |
| whereClauses.push(`( | |
| campaigns.id IN (SELECT id FROM campaigns_fts WHERE campaigns_fts MATCH ?) | |
| OR LOWER(campaigns.creator) LIKE LOWER(?) ESCAPE '\\' | |
| OR campaigns.id = ? | |
| )`); | |
| params.push(ftsMatchTerm, creatorExactTerm, exactTerm); | |
| } else { | |
| // Fallback if cleaning the query stripped all characters | |
| whereClauses.push(`(LOWER(campaigns.creator) LIKE LOWER(?) ESCAPE '\\' OR campaigns.id = ?)`); | |
| params.push(creatorExactTerm, exactTerm); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/services/campaignStore.ts` around lines 361 - 375, Escape LIKE
metacharacters in rawQuery before constructing creatorExactTerm, including the
chosen escape character itself, and use the same escaped value in both creator
LIKE predicates. Add the corresponding ESCAPE clause to each
LOWER(campaigns.creator) LIKE LOWER(?) condition while preserving substring
matching and the existing FTS and exact-ID parameters.
| import { describe, expect, it, beforeEach, afterEach, vi } from "vitest"; | ||
| import request from "supertest"; | ||
| import fs from "fs"; | ||
| import path from "path"; | ||
| import express from "express"; | ||
| import { initDb, resetDbForTests, getDb } from "../services/db"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unused getDb import.
ESLint reports getDb is unused.
🧰 Tools
🪛 ESLint
[error] 6-6: 'getDb' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/tests/routeHandlers.test.ts` around lines 1 - 6, Remove the
unused getDb import from the routeHandlers test imports while preserving the
other database utilities used by the tests.
Source: Linters/SAST tools
| process.env.DB_PATH = TEST_DB_PATH; | ||
| process.env.NODE_ENV = "test"; | ||
| import { app } from "../index"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
nl -ba backend/src/tests/routeHandlers.test.ts | sed -n '15,32p'
rg -n -C 3 'initDb\(|initCampaignStore\(|DB_PATH|NODE_ENV|process\.env' \
backend/src/index.ts backend/src/services/db.tsRepository: ritik4ever/stellar-goal-vault
Length of output: 4869
🏁 Script executed:
#!/bin/bash
set -eu
echo "== routeHandlers test first lines =="
sed -n '1,35p' backend/src/tests/routeHandlers.test.ts
echo
echo "== db.ts relevant init/get path =="
sed -n '1,100p' backend/src/services/db.ts
echo
echo "== index.ts imports/module-loading-relevant section =="
sed -n '1,180p' backend/src/index.ts
echo
echo "== package config/test files mentioning vitest/setup =="
rg -n -C 2 '"test"|vitest|setupFiles|setup|routeHandlers\.test' package.json package-lock.json pnpm-lock.yaml yarn.lock backend 2>/dev/null || true
echo
echo "== static assignment before import probe in plain javascript =="
node - <<'JS'
const fs = require("node:fs");
fs.writeFileSync("/tmp/probe-module-a.js", `
process.env.X = process.env.X || "default";
console.log("module-a saw during load:", process.env.X);
module.exports = { seen: process.env.X };
`);
fs.writeFileSync("/tmp/probe-runner.js", `
process.env.X = "assigned-before";
const { seen } = require("./module-a");
console.log("after assignment, require sees:", seen);
`);
require("/tmp/probe-runner");
fs.unlinkSync("/tmp/probe-module-a.js");
fs.unlinkSync("/tmp/probe-runner.js");
JS
echo
echo "== routeHandlers test exact diff/stat context if available =="
git diff -- backend/src/tests/routeHandlers.test.ts || trueRepository: ritik4ever/stellar-goal-vault
Length of output: 32777
Configure the test environment in Vitest setup.
The assignments still run before the top-level import { app } from "../index", so ../index can still observe inherited values such as production-only middleware/rate-limit config and the production DB path. Set DB_PATH and NODE_ENV in a Vitest setup/global test config before module loading, or load the app dependency graph through a dynamic/import helper after setup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/tests/routeHandlers.test.ts` around lines 19 - 21, Move the
DB_PATH and NODE_ENV test assignments out of the top-level setup in
routeHandlers.test.ts and into Vitest’s global setup/configuration so they are
applied before any application modules load. Ensure the app import through index
observes TEST_DB_PATH and test mode rather than inherited production
configuration.
| title: z | ||
| .string() | ||
| .trim() | ||
|
|
||
| .min(4, 'Title must be at least 4 characters.') | ||
| .max(80) | ||
| .transform(sanitizeInput) | ||
| .refine((val) => !containsScriptTag(val), { message: 'Title cannot contain script tags.' }) | ||
| .refine((val) => !containsSqlComment(val), { message: 'Title cannot contain SQL comment sequences.' }), | ||
| description: z | ||
| .string() | ||
| .trim() | ||
| .min(20, 'Description must be at least 20 characters.') | ||
| .max(500) | ||
| .transform(sanitizeInput) | ||
| .refine((val) => !containsScriptTag(val), { message: 'Description cannot contain script tags.' }) | ||
| .refine((val) => !containsSqlComment(val), { message: 'Description cannot contain SQL comment sequences.' }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Script-tag / SQL-comment refinements are unreachable — they run after sanitization.
sanitizeInput escapes <, > and / first, so by the time the refines execute <script is <script and /*/*/ are /*/*/. containsScriptTag can never match, and containsSqlComment only still catches --. Malicious payloads are silently accepted (escaped) instead of rejected. Validate the raw input, then transform.
🐛 Proposed fix
title: z
.string()
.trim()
.min(4, 'Title must be at least 4 characters.')
.max(80)
- .transform(sanitizeInput)
.refine((val) => !containsScriptTag(val), { message: 'Title cannot contain script tags.' })
- .refine((val) => !containsSqlComment(val), { message: 'Title cannot contain SQL comment sequences.' }),
+ .refine((val) => !containsSqlComment(val), { message: 'Title cannot contain SQL comment sequences.' })
+ .transform(sanitizeInput),
description: z
.string()
.trim()
.min(20, 'Description must be at least 20 characters.')
.max(500)
- .transform(sanitizeInput)
.refine((val) => !containsScriptTag(val), { message: 'Description cannot contain script tags.' })
- .refine((val) => !containsSqlComment(val), { message: 'Description cannot contain SQL comment sequences.' }),
+ .refine((val) => !containsSqlComment(val), { message: 'Description cannot contain SQL comment sequences.' })
+ .transform(sanitizeInput),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| title: z | |
| .string() | |
| .trim() | |
| .min(4, 'Title must be at least 4 characters.') | |
| .max(80) | |
| .transform(sanitizeInput) | |
| .refine((val) => !containsScriptTag(val), { message: 'Title cannot contain script tags.' }) | |
| .refine((val) => !containsSqlComment(val), { message: 'Title cannot contain SQL comment sequences.' }), | |
| description: z | |
| .string() | |
| .trim() | |
| .min(20, 'Description must be at least 20 characters.') | |
| .max(500) | |
| .transform(sanitizeInput) | |
| .refine((val) => !containsScriptTag(val), { message: 'Description cannot contain script tags.' }) | |
| .refine((val) => !containsSqlComment(val), { message: 'Description cannot contain SQL comment sequences.' }), | |
| title: z | |
| .string() | |
| .trim() | |
| .min(4, 'Title must be at least 4 characters.') | |
| .max(80) | |
| .refine((val) => !containsScriptTag(val), { message: 'Title cannot contain script tags.' }) | |
| .refine((val) => !containsSqlComment(val), { message: 'Title cannot contain SQL comment sequences.' }) | |
| .transform(sanitizeInput), | |
| description: z | |
| .string() | |
| .trim() | |
| .min(20, 'Description must be at least 20 characters.') | |
| .max(500) | |
| .refine((val) => !containsScriptTag(val), { message: 'Description cannot contain script tags.' }) | |
| .refine((val) => !containsSqlComment(val), { message: 'Description cannot contain SQL comment sequences.' }) | |
| .transform(sanitizeInput), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/validation/schemas.ts` around lines 74 - 89, Update the title and
description validation chains in the schema so containsScriptTag and
containsSqlComment refinements run on the trimmed raw input before sanitizeInput
transforms it. Preserve the existing length checks and validation messages, then
sanitize only after the security refinements so matching script tags and SQL
comment sequences are rejected rather than escaped and accepted.
| function parseIso8601Timestamp(value: unknown): number | null { | ||
| if (typeof value !== 'string') { | ||
| return null; | ||
| } | ||
|
|
||
| const timestamp = new Date(value).getTime(); | ||
| if (!Number.isFinite(timestamp)) { | ||
| return null; | ||
| } | ||
|
|
||
| return Math.floor(timestamp / 1000); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Timestamp parsing is looser than the error message claims.
new Date(value) accepts non-ISO inputs (12/31/2024, Dec 2024) with implementation-defined behavior, so createdAfter/createdBefore accept values the message says are rejected. Gate on an ISO-8601 shape first.
🛡️ Proposed fix
+const ISO_8601_RE = /^\d{4}-\d{2}-\d{2}([T ]\d{2}:\d{2}(:\d{2}(\.\d+)?)?(Z|[+-]\d{2}:?\d{2})?)?$/;
+
function parseIso8601Timestamp(value: unknown): number | null {
- if (typeof value !== 'string') {
+ if (typeof value !== 'string' || !ISO_8601_RE.test(value)) {
return null;
}Separately, consider rejecting inverted ranges (createdAfter > createdBefore) instead of silently returning an empty page.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parseIso8601Timestamp(value: unknown): number | null { | |
| if (typeof value !== 'string') { | |
| return null; | |
| } | |
| const timestamp = new Date(value).getTime(); | |
| if (!Number.isFinite(timestamp)) { | |
| return null; | |
| } | |
| return Math.floor(timestamp / 1000); | |
| } | |
| const ISO_8601_RE = /^\d{4}-\d{2}-\d{2}([T ]\d{2}:\d{2}(:\d{2}(\.\d+)?)?(Z|[+-]\d{2}:?\d{2})?)?$/; | |
| function parseIso8601Timestamp(value: unknown): number | null { | |
| if (typeof value !== 'string' || !ISO_8601_RE.test(value)) { | |
| return null; | |
| } | |
| const timestamp = new Date(value).getTime(); | |
| if (!Number.isFinite(timestamp)) { | |
| return null; | |
| } | |
| return Math.floor(timestamp / 1000); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/validation/schemas.ts` around lines 316 - 327, Update
parseIso8601Timestamp to validate the string against an ISO-8601 format before
passing it to Date, rejecting inputs such as locale-formatted dates and partial
month values while preserving null for invalid timestamps. Keep the existing
finite-timestamp check and Unix-seconds conversion; range-order validation is
outside this change.
| function parseAssetCodes(value: unknown): string[] | null { | ||
| if (typeof value !== 'string') { | ||
| return null; | ||
| } | ||
|
|
||
| const codes = value.split(',').map(code => code.trim().toUpperCase()).filter(code => code.length > 0); | ||
| return codes.length > 0 ? codes : null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Repeated asset query params are rejected.
Express parses ?asset=XLM&asset=USDC into an array, which fails the typeof value !== 'string' check and returns a 400 for a reasonable request. Handle array form as the rest of this file does.
🛡️ Proposed fix
function parseAssetCodes(value: unknown): string[] | null {
- if (typeof value !== 'string') {
+ const raw = Array.isArray(value)
+ ? value.filter((v): v is string => typeof v === 'string').join(',')
+ : value;
+ if (typeof raw !== 'string') {
return null;
}
- const codes = value.split(',').map(code => code.trim().toUpperCase()).filter(code => code.length > 0);
+ const codes = raw.split(',').map((code) => code.trim().toUpperCase()).filter((code) => code.length > 0);
return codes.length > 0 ? codes : null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parseAssetCodes(value: unknown): string[] | null { | |
| if (typeof value !== 'string') { | |
| return null; | |
| } | |
| const codes = value.split(',').map(code => code.trim().toUpperCase()).filter(code => code.length > 0); | |
| return codes.length > 0 ? codes : null; | |
| } | |
| function parseAssetCodes(value: unknown): string[] | null { | |
| const raw = Array.isArray(value) | |
| ? value.filter((v): v is string => typeof v === 'string').join(',') | |
| : value; | |
| if (typeof raw !== 'string') { | |
| return null; | |
| } | |
| const codes = raw.split(',').map((code) => code.trim().toUpperCase()).filter((code) => code.length > 0); | |
| return codes.length > 0 ? codes : null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/validation/schemas.ts` around lines 329 - 336, Update
parseAssetCodes to accept Express’s array form for repeated asset query
parameters, while preserving the existing comma-separated string parsing and
null behavior for unsupported or empty input. Normalize array elements using the
same trimming, uppercasing, and filtering rules before returning the codes.
| } | ||
| } | ||
|
|
||
| const searchQuery = normalizeQueryValue(query.search) || normalizeQueryValue(query.q); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Dead searchQuery local; simplify the returned q/search.
searchQuery is never read (ESLint no-unused-vars), and the query.q ? … guards are redundant because normalizeQueryValue already yields undefined for non-strings and blanks.
🧹 Proposed cleanup
- const searchQuery = normalizeQueryValue(query.search) || normalizeQueryValue(query.q);
-
@@
- q: query.q ? normalizeQueryValue(query.q) : undefined,
- search: query.search ? normalizeQueryValue(query.search) : undefined,
+ q: normalizeQueryValue(query.q),
+ search: normalizeQueryValue(query.search),Also applies to: 519-534
🧰 Tools
🪛 ESLint
[error] 396-396: 'searchQuery' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/validation/schemas.ts` at line 396, Remove the unused searchQuery
local and simplify the returned q/search handling in the query normalization
logic. Reuse normalizeQueryValue directly for both query.q and query.search
without truthiness guards, preserving undefined for non-string or blank values.
Source: Linters/SAST tools
| Closes #636 | ||
|
|
||
| ## Summary | ||
|
|
||
| - Smoothly animates the campaign progress bar when funding percentage changes after a pledge | ||
| - Uses 0.4s cubic-bezier transition for responsive feel | ||
| - Animation only triggers on update, not on initial page load | ||
| This PR addresses issue #636 by increasing the backend test coverage to **81.41% branch coverage** and **81.90% statement/line coverage**. It fixes pre-existing runtime bugs and test failures, refactors the `addPledge` service to be synchronous, enforces strict CRC validation on Stellar public keys, and introduces a robust suite of new unit tests covering Express middlewares, route handlers, and refund/token balance logic. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the PR body with the actual PR objective.
This body closes #636 and describes backend runtime/test fixes, while the stated objective is to close #647 and add environment-variable documentation plus secret-rotation guidance. Update the issue reference and summary, or clarify if both scopes are intentionally included; otherwise this can close the wrong issue and mislead reviewers.
Also applies to: 7-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-body.md` around lines 1 - 5, Update the PR body to reference issue `#647`
and accurately summarize the environment-variable documentation and
secret-rotation guidance objective. If the backend fixes and test coverage from
the current Summary are intentionally included, explicitly describe both scopes;
otherwise remove that unrelated summary to avoid misrepresenting the PR and
closing the wrong issue.
|
Hi @AdEmOnD07, This PR could not be merged because it has merge conflicts with the target branch. Please resolve the merge conflicts, push the updated changes, and the PR can be reviewed and merged. Thank you! |
1 similar comment
|
Hi @AdEmOnD07, This PR could not be merged because it has merge conflicts with the target branch. Please resolve the merge conflicts, push the updated changes, and the PR can be reviewed and merged. Thank you! |
2 participants
Closes #647
Summary
This PR addresses issue #647 by adding a comprehensive environment variable reference table and secret rotation guide, updating the README, and fixing a backend TypeScript compiler error.
...
Summary by CodeRabbit
Documentation
Bug Fixes
New Features
Tests