fix(validation): return 422, add missing schemas and PATCH metadata e… - #722
fix(validation): return 422, add missing schemas and PATCH metadata e…#722praizeD10 wants to merge 1 commit into
Conversation
|
Someone is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@praizeD10 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! 🚀 |
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds campaign metadata PATCH handling, strengthens campaign and comment validation, standardizes validation failures on HTTP 422, and introduces typed parsing for campaign list queries. ChangesCampaign validation and metadata updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant CampaignAPI
participant CampaignStore
Client->>CampaignAPI: PATCH /api/campaigns/:id/metadata
CampaignAPI->>CampaignAPI: Rate-limit and validate request
CampaignAPI->>CampaignStore: Update campaign metadata
CampaignAPI->>CampaignStore: Invalidate campaign cache
CampaignAPI-->>Client: Return campaign and progress
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ 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: 7
🧹 Nitpick comments (4)
backend/src/validation/schemas.ts (4)
155-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared title/description field schemas.
Lines 155-173 duplicate the chains from
createCampaignPayloadSchema(lines 70-88) verbatim. A drift here (e.g. bumping max length in one place) silently produces divergent contracts between create and update.♻️ Suggested extraction
+const campaignTitleSchema = z + .string() + .trim() + .min(3, 'title must be at least 3 characters.') + .max(120, 'title must be at most 120 characters.') + .refine((v) => !containsSqlComment(v) && !containsScriptTag(v), { + message: 'title contains disallowed characters.', + }) + .transform(sanitizeInput); + +const campaignDescriptionSchema = z + .string() + .trim() + .max(2000, 'description must be at most 2000 characters.') + .refine((v) => !containsSqlComment(v) && !containsScriptTag(v), { + message: 'description contains disallowed characters.', + }) + .transform(sanitizeInput); + +const campaignMetadataSchema = z.object({ + imageUrl: httpsOnlyUrlSchema.optional(), + externalLink: httpsOnlyUrlSchema.optional(), +});Then reuse:
title: campaignTitleSchema/title: campaignTitleSchema.optional(), etc.🤖 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 155 - 173, Extract the duplicated title and description validation chains from createCampaignPayloadSchema and the update schema into shared campaignTitleSchema and campaignDescriptionSchema symbols. Reuse those schemas in both payload definitions, applying optional() at the field declarations where needed, while preserving the existing validation, sanitization, and optionality behavior.
73-88: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
containsSqlCommenton free-text fields rejects legitimate copy.Any
--,/*or*/in a title/description fails validation (e.g."Rescue dogs -- help us", em-dash substitutes, URLs in descriptions). SQL injection is already prevented by parameterized statements (updateCampaignuses bound params), so this check buys no security while producing confusing rejections with a generic "contains disallowed characters" message. Consider limiting the refinement to script/HTML content.🤖 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 73 - 88, Remove containsSqlComment from the refinement callbacks for the title and description schemas, while retaining containsScriptTag validation and the existing messages, sanitization, and optional/default behavior. Update both affected fields in the schema without changing their length or trimming rules.
376-378: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTie the allow-lists to the imported union types.
VALID_SORT_FIELDS/VALID_SORT_ORDERS/VALID_STATUSESare unrelated literals; ifCampaignSortFieldorCampaignStatusinbackend/src/services/campaignStore.tsgains a member, this file silently rejects it and the casts at lines 446/461/476 hide the mismatch.♻️ Suggested change
-const VALID_SORT_FIELDS = ['createdAt', 'deadline', 'pledgedAmount', 'targetAmount'] as const; -const VALID_SORT_ORDERS = ['asc', 'desc'] as const; -const VALID_STATUSES = ['open', 'funded', 'claimed', 'failed'] as const; +const VALID_SORT_FIELDS = ['createdAt', 'deadline', 'pledgedAmount', 'targetAmount'] as const satisfies readonly CampaignSortField[]; +const VALID_SORT_ORDERS = ['asc', 'desc'] as const satisfies readonly SortOrder[]; +const VALID_STATUSES = ['open', 'funded', 'claimed', 'failed'] as const satisfies readonly CampaignStatus[];🤖 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 376 - 378, Update the VALID_SORT_FIELDS, VALID_SORT_ORDERS, and VALID_STATUSES declarations in schemas.ts to derive or validate their values against the imported CampaignSortField, CampaignSortOrder, and CampaignStatus union types from campaignStore.ts. Ensure the allow-lists cannot silently diverge when those unions change, while preserving their current runtime values and validation behavior.
457-470: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
sortis compared case-sensitively whilestatusandorderare lowercased.Inconsistent handling across sibling parameters:
?order=DESCis accepted but?sort=CreatedAtis rejected. Sincesortvalues are camelCase, an exact-match lookup is fine — but consider documenting it, or normalize consistently (e.g. case-insensitive lookup mapping back to the canonical value).🤖 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 457 - 470, The sort query parameter is validated case-sensitively unlike the sibling status and order parameters. Update the sort handling near rawSort and VALID_SORT_FIELDS to normalize input consistently, using a case-insensitive lookup that maps accepted values back to their canonical CampaignSortField values while preserving the existing invalid-value issue message.
🤖 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/index.ts`:
- Around line 520-521: Update backend/src/index.ts lines 520-521 around
updateCampaign to merge the incoming metadata with the existing campaign
metadata before persistence, preserving omitted imageUrl or externalLink keys
while applying supplied values. Update backend/src/validation/schemas.ts lines
174-183 to reject an empty metadata object, while retaining the existing
requirement that at least one patch field is provided.
- Around line 510-514: Add the existing creator ownership check used by claim
operations to the PATCH /api/campaigns/:id/metadata handler before invoking
updateCampaign, and reject requests without an authorized campaign creator.
Preserve the current body validation and metadata update flow for owned
campaigns.
- Line 521: Update the campaignStore.ts implementation of updateCampaign so
missing campaigns throw the existing toServiceError('Campaign not found.', 404,
'NOT_FOUND') shape instead of a plain CAMPAIGN_NOT_FOUND object. Keep the route
in backend/src/index.ts unchanged so the global handler returns HTTP 404 without
adding route-level translation.
In `@backend/src/middleware/validateBody.ts`:
- Line 39: Update validateBody and sendValidationError to use the same
validation error status and payload shape for invalid bodies, campaign IDs, and
list query parameters. Reuse the existing shared contract rather than
maintaining separate 422 and 400 responses across these validation paths.
In `@backend/src/validation/schemas.ts`:
- Around line 424-439: Update the asset parsing logic around rawAsset and codes
so inputs containing no non-empty asset codes, including empty or comma-only
values, add a validation issue instead of assigning asset to an empty array.
Preserve the existing normalization and unsupported-code validation for
non-empty codes, and ensure asset is only set when at least one valid code
remains.
- Around line 518-541: Update the campaign list query validation around
includeDeleted to pass query.includeDeleted through singleCampaignListQueryParam
before interpreting it. Accept only the established boolean representations,
preserve the normalized boolean in the returned data, and add a validation issue
at path ['includeDeleted'] for repeated or unrecognized values such as TRUE or 1
instead of silently defaulting to false.
- Around line 487-516: Replace the inline Date.parse validation in the
createdAfter and createdBefore handling with the shared parseIsoDateQueryParam
helper, preserving issue reporting for invalid values and Unix-second
normalization. After both values are parsed, validate that createdAfter is not
later than createdBefore and add a validation issue when the range is reversed.
---
Nitpick comments:
In `@backend/src/validation/schemas.ts`:
- Around line 155-173: Extract the duplicated title and description validation
chains from createCampaignPayloadSchema and the update schema into shared
campaignTitleSchema and campaignDescriptionSchema symbols. Reuse those schemas
in both payload definitions, applying optional() at the field declarations where
needed, while preserving the existing validation, sanitization, and optionality
behavior.
- Around line 73-88: Remove containsSqlComment from the refinement callbacks for
the title and description schemas, while retaining containsScriptTag validation
and the existing messages, sanitization, and optional/default behavior. Update
both affected fields in the schema without changing their length or trimming
rules.
- Around line 376-378: Update the VALID_SORT_FIELDS, VALID_SORT_ORDERS, and
VALID_STATUSES declarations in schemas.ts to derive or validate their values
against the imported CampaignSortField, CampaignSortOrder, and CampaignStatus
union types from campaignStore.ts. Ensure the allow-lists cannot silently
diverge when those unions change, while preserving their current runtime values
and validation behavior.
- Around line 457-470: The sort query parameter is validated case-sensitively
unlike the sibling status and order parameters. Update the sort handling near
rawSort and VALID_SORT_FIELDS to normalize input consistently, using a
case-insensitive lookup that maps accepted values back to their canonical
CampaignSortField values while preserving the existing invalid-value issue
message.
🪄 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: d30452d9-dce0-4288-8260-bd2a430d7c9b
📒 Files selected for processing (4)
backend/src/index.tsbackend/src/middleware/validateBody.test.tsbackend/src/middleware/validateBody.tsbackend/src/validation/schemas.ts
| app.patch( | ||
| '/api/campaigns/:id/metadata', | ||
| applyRateLimit(WRITE_RATE_LIMIT_MAX_REQUESTS), | ||
| validateBody(updateMetadataPayloadSchema), | ||
| (req: Request, res: Response) => { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Is there auth/ownership middleware on other campaign write routes?
rg -n --type=ts -C6 "app\.(post|patch|put|delete)\(" backend/src/index.ts
rg -n --type=ts -C3 'requireAuth|authenticate|verifySignature|creator\s*!==|assertOwner' backend/src
fd -t f . backend/src/middleware -x echo {}Repository: ritik4ever/stellar-goal-vault
Length of output: 3542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant index sections =="
sed -n '1,90p;200,230p;487,625p' backend/src/index.ts
echo
echo "== auth-related middleware candidates =="
for f in backend/src/middleware/* middleware/* .github/workflows/*; do
[ -e "$f" ] || continue
printf '\n--- %s ---\n' "$f"
rg -n -C3 'requireAuth|authenticate|verifySignature|creator|claimCampaign|refunds|request.signature|HMAC|apiKey|Authorization|X-.*Signature|campaign.*creator|assertOwner|owner' "$f" || true
done
echo
echo "== global auth symbols search (all tracked TS) =="
rg -n -C3 'requireAuth|authenticate|verifySignature|campaignClaim|creator.*signature|signature|X-.*Signature|request\.signature|creator !==|creatorCheck|claimCampaign' $(git ls-files '*.ts' | tr '\n' ' ') || true
echo
echo "== service authorization implementation snippets =="
rg -n -C4 'campaign.*claim|claimCampaign|applySignature|verifySignature|creator|refunds|request.signature' backend/src/services/campaignStore.ts backend/src/services -S || trueRepository: ritik4ever/stellar-goal-vault
Length of output: 50386
Add creator ownership before allowing metadata updates.
PATCH /api/campaigns/:id/metadata runs validate body and then mutates the campaign directly; it does not verify that the caller owns the campaign before overwriting the displayed imageUrl/externalLink. Add the creator/ownership check used by claim operations before calling updateCampaign, or reject creator-less requests where creator ownership is required.
🤖 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/index.ts` around lines 510 - 514, Add the existing creator
ownership check used by claim operations to the PATCH
/api/campaigns/:id/metadata handler before invoking updateCampaign, and reject
requests without an authorized campaign creator. Preserve the current body
validation and metadata update flow for owned campaigns.
| const body = req.body as z.infer<typeof updateMetadataPayloadSchema>; | ||
| const campaign = updateCampaign(parsedId.value, body); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
PATCH metadata silently destroys unsent metadata fields. The validated body is forwarded verbatim to updateCampaign, which persists metadata_json = JSON.stringify(patch.metadata) — a whole-object replacement. Since imageUrl and externalLink are independently optional, a client updating only the image loses the stored external link, and metadata: {} clears everything while still satisfying the "at least one field" refinement.
backend/src/index.ts#L520-L521: merge the incoming metadata over the existing campaign's metadata before callingupdateCampaign(or fetch-then-merge insideupdateCampaign), so omitted keys are preserved and PATCH semantics hold.backend/src/validation/schemas.ts#L174-L183: require themetadataobject to be non-empty so an empty object cannot pass the top-level refinement as a "provided" field.
📍 Affects 2 files
backend/src/index.ts#L520-L521(this comment)backend/src/validation/schemas.ts#L174-L183
🤖 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/index.ts` around lines 520 - 521, Update backend/src/index.ts
lines 520-521 around updateCampaign to merge the incoming metadata with the
existing campaign metadata before persistence, preserving omitted imageUrl or
externalLink keys while applying supplied values. Update
backend/src/validation/schemas.ts lines 174-183 to reject an empty metadata
object, while retaining the existing requirement that at least one patch field
is provided.
| } | ||
|
|
||
| const body = req.body as z.infer<typeof updateMetadataPayloadSchema>; | ||
| const campaign = updateCampaign(parsedId.value, body); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --type=ts -C5 'CAMPAIGN_NOT_FOUND' backend/srcRepository: ritik4ever/stellar-goal-vault
Length of output: 997
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files backend/src | sed -n '1,120p'
echo "== outline backend/src/index.ts =="
ast-grep outline backend/src/index.ts --view compact 2>/dev/null | sed -n '1,220p' || true
echo "== relevant index lines =="
sed -n '460,555p' backend/src/index.ts | cat -n
echo "== app error and error handler usages =="
rg -n --type=ts -C4 'class AppError|function [A-Z].*Error|ErrorReport|AppError\\(' backend/src | sed -n '1,260p'
echo "== campaignStore relevant sections =="
sed -n '1140,1225p' backend/src/services/campaignStore.ts | cat -nRepository: ritik4ever/stellar-goal-vault
Length of output: 5511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AppError and error handler definitions/usages =="
rg -n --type=ts -C4 'class AppError|function .*Error|AppError\\(' backend/src
echo "== backend/src/index.ts top and error handler section =="
sed -n '1,140p' backend/src/index.ts | cat -n
sed -n '550,720p' backend/src/index.ts | cat -n
echo "== campaignStore get/update/pledge error handling context =="
rg -n --type=ts -C4 'function get|function add|function update|AppError|not found|campaignId' backend/src/services/campaignStore.ts
echo "== test assertions for 404/error codes =="
rg -n --type=ts -C5 'not found|404|CAMPAIGN_NOT_FOUND|Campaign not found' backend/srcRepository: ritik4ever/stellar-goal-vault
Length of output: 322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AppError and error handler definitions/usages =="
rg -n --type=ts -C4 'class AppError|function .*Error|AppError\(' backend/src
echo "== backend/src/index.ts top and error handler section =="
sed -n '1,140p' backend/src/index.ts | cat -n
sed -n '550,720p' backend/src/index.ts | cat -n
echo "== campaignStore get/update/pledge error handling context =="
rg -n --type=ts -C4 'function get|function add|function update|AppError|not found|campaignId' backend/src/services/campaignStore.ts
echo "== test assertions for 404/error codes =="
rg -n --type=ts -C5 'not found|404|CAMPAIGN_NOT_FOUND|Campaign not found' backend/src
echo "== deterministic throw types in campaignStore =="
python3 - <<'PY'
from pathlib import Path
p=Path('backend/src/services/campaignStore.ts')
text=p.read_text()
for token in ['getCampaign', 'updateCampaign', 'addPledge', 'AppError', 'Object.assign(new Error']:
print(f'{token}:', text.count(token))
PYRepository: ritik4ever/stellar-goal-vault
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== serviceError / AppError mapping in index.ts =="
sed -n '730,795p' backend/src/index.ts | cat -n
echo "== ServiceError and AppError types =="
sed -n '110,155p' backend/src/services/campaignStore.ts | cat -n
sed -n '49,65p' backend/src/types/errors.ts | cat -n
echo "== focused AppError construction count with line numbers =="
rg -n --type=ts -C2 'new AppError' backend/src/index.ts backend/src/services/campaignStore.ts backend/src/services/sorobanRpc.ts backend/src/middleware/apiKeyAuth.ts backend/src/index.ts
echo "== behavioral probe: plain Error with code vs AppError routing =="
node - <<'JS'
class AppError extends Error {
constructor(message, statusCode = 500, code = 'INTERNAL_SERVER_ERROR') {
super(message);
this.statusCode = statusCode;
this.code = code;
}
}
function isServiceError(error) {
try {
return error.statusCode != null && error.code != null;
} catch {
return false;
}
}
function isAppError(error) {
return error instanceof AppError;
}
const plain = Object.assign(new Error('Campaign X not found'), { code: 'CAMPAIGN_NOT_FOUND' });
const app = new AppError('Campaign not found.', 404, 'CAMPAIGN_NOT_FOUND');
console.log({
plain_isServiceError: isServiceError(plain),
plain_isAppError: isAppError(plain),
plain_statusCode: plain.statusCode,
plain_code: plain.code,
app_isServiceError: isServiceError(app),
app_isAppError: isAppError(app),
app_statusCode: app.statusCode,
app_code: app.code,
});
JSRepository: ritik4ever/stellar-goal-vault
Length of output: 12970
Surface missing campaign IDs as 404s in updateCampaign.
updateCampaign throws a plain { code: 'CAMPAIGN_NOT_FOUND' } error instead of a ServiceError with statusCode: 404, so the global handler uses the default 500 path. Have campaignStore.ts throw the existing toServiceError('Campaign not found.', 404, 'NOT_FOUND') shape here to avoid a separate route-level translator.
🤖 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/index.ts` at line 521, Update the campaignStore.ts implementation
of updateCampaign so missing campaigns throw the existing
toServiceError('Campaign not found.', 404, 'NOT_FOUND') shape instead of a plain
CAMPAIGN_NOT_FOUND object. Keep the route in backend/src/index.ts unchanged so
the global handler returns HTTP 404 without adding route-level translation.
| details: (parsed.error as ZodError).issues, | ||
| }; | ||
| res.status(400).json(body); | ||
| res.status(422).json(body); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Remaining 400 validation statuses, assertions, and documented response codes.
rg -n --type=ts -C3 'VALIDATION_ERROR' backend/src
rg -n --type=ts -C2 'toBe\(400\)|status\(400\)|"400"|\x27400\x27' backend/src
fd -e yaml -e yml -e json --iglob '*openapi*' --iglob '*swagger*' -x rg -n -C3 '422|400' {}Repository: ritik4ever/stellar-goal-vault
Length of output: 9356
🏁 Script executed:
#!/bin/bash
set -e
echo "== candidate files =="
git ls-files 'backend/src/**' | sed -n '1,200p'
echo "== validateBody.ts outline =="
ast-grep outline backend/src/middleware/validateBody.ts || true
echo "== validateBody.ts =="
cat -n backend/src/middleware/validateBody.ts | sed -n '1,140p'
echo "== index.ts validation handlers outline =="
ast-grep outline backend/src/index.ts --match sendValidationError || true
echo "== index.ts validation helper/envelope =="
cat -n backend/src/index.ts | sed -n '140,220p'
echo "== route usages around metadata/validations =="
rg -n --type=ts -C4 'sendValidationError|validateBody|parseCampaignListQuery|validateCampaignMetadata|metadata|campaigns/:id' backend/src/index.ts backend/src/api.test.ts backend/src/historyEndpoint.test.ts
echo "== openapi response codes =="
fd -e yaml -e yml -e json --glob '*openapi*' --glob '*swagger*' . | xargs -r rg -n -C3 '422|400|string\(\)|VALIDATION_ERROR' || trueRepository: ritik4ever/stellar-goal-vault
Length of output: 19441
🏁 Script executed:
#!/bin/bash
set -e
echo "== validateBody middleware test relevant sections =="
cat -n backend/src/middleware/validateBody.test.ts | sed -n '1,180p'
echo "== AppError / error envelope definitions and handlers =="
cat -n backend/src/types/errors.ts | sed -n '1,220p'
rg -n --type=ts -C3 'class AppError|function zodIssuesToValidationIssues|function zodIssuesToErrorMessage|sendResponseForError|sendAppError|validationIssueSchema|apiErrorSchema' backend/src/types errors.ts backend/src validation schemas.ts backend/src/openapi.ts backend/src/index.ts | sed -n '1,220p'
echo "== openapi generation status/response docs =="
cat -n backend/src/openapi.ts | sed -n '1,180p'
echo "== route tests for metadata id query validation statuses =="
cat -n backend/src/api.test.ts | sed -n '320,370p'
rg -n --type=ts -C3 "metadata|invalid .*id|campaignIdSchema|page invalid|limit 999|asset INVALID" backend/src/*.test.ts backend/src/api.test.tsRepository: ritik4ever/stellar-goal-vault
Length of output: 21984
Align validation errors to a single status/payload contract.
validateBody now returns 422 for invalid request bodies, but sendValidationError still returns 400 for invalid campaign IDs and list query parameters on the same routes. Use one shared validation error status and payload shape across body, path, and query validation.
🤖 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/middleware/validateBody.ts` at line 39, Update validateBody and
sendValidationError to use the same validation error status and payload shape
for invalid bodies, campaign IDs, and list query parameters. Reuse the existing
shared contract rather than maintaining separate 422 and 400 responses across
these validation paths.
| // createdAfter / createdBefore — accept ISO 8601 strings, convert to unix timestamp | ||
| let createdAfter: number | undefined; | ||
| const rawCreatedAfter = singleCampaignListQueryParam(query.createdAfter); | ||
| if (rawCreatedAfter !== undefined) { | ||
| const ts = Date.parse(rawCreatedAfter); | ||
| if (Number.isNaN(ts)) { | ||
| issues.push({ | ||
| code: 'custom', | ||
| message: 'createdAfter must be a valid ISO 8601 date string.', | ||
| path: ['createdAfter'], | ||
| }); | ||
| } else { | ||
| createdAfter = Math.floor(ts / 1000); | ||
| } | ||
| } | ||
|
|
||
| let createdBefore: number | undefined; | ||
| const rawCreatedBefore = singleCampaignListQueryParam(query.createdBefore); | ||
| if (rawCreatedBefore !== undefined) { | ||
| const ts = Date.parse(rawCreatedBefore); | ||
| if (Number.isNaN(ts)) { | ||
| issues.push({ | ||
| code: 'custom', | ||
| message: 'createdBefore must be a valid ISO 8601 date string.', | ||
| path: ['createdBefore'], | ||
| }); | ||
| } else { | ||
| createdBefore = Math.floor(ts / 1000); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Zod 4 z.iso.datetime offset option and z.iso.date validation
💡 Result:
In Zod 4, validation for ISO date and datetime strings is provided through the z.iso namespace [1][2]. z.iso.datetime The z.iso.datetime method is used to validate ISO 8601 datetime strings [1]. By default, it requires the 'Z' suffix and does not allow timezone offsets or local (timezone-less) strings [1]. To allow timezone offsets, you can pass the { offset: true } option to the method [1][3]. Note that Zod adheres to standard ISO 8601/RFC 3339 formats; it requires the offset to include the colon separator (e.g., +02:00) [1][4]. Formats like +02 or +0200 are not supported [1][4]. Example usage: const datetime = z.iso.datetime({ offset: true }); datetime.parse("2020-01-01T06:15:00+02:00"); // Valid Additionally, you can allow local datetimes by using the { local: true } option, and you can constrain the time precision using the precision option [1][3]. z.iso.date The z.iso.date method is a dedicated validator for ISO date strings (format YYYY-MM-DD) [2]. It is part of the standard Zod 4 API [2]. Example usage: const schema = z.iso.date; schema.parse("2020-01-01"); // Valid If you encounter issues where z.iso is not recognized, ensure that you are using Zod version 4.x or higher, as these methods were introduced in Zod 4 [2].
Citations:
- 1: https://zod.dev/api
- 2: z.iso.date() not recognized in Zod v4.3.6 (TypeScript) colinhacks/zod#5783
- 3: https://github.com/colinhacks/zod/blob/v4.4.3/packages/docs/content/api.mdx
- 4: ISO datetime doesn't accept offset with only hours colinhacks/zod#4666
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)schemas\.ts$|package\.json$|pnpm-lock\.yaml$|package-lock\.json$|yarn\.lock$' || true
echo "== schemas outline =="
if [ -f backend/src/validation/schemas.ts ]; then
wc -l backend/src/validation/schemas.ts
sed -n '1,90p' backend/src/validation/schemas.ts
sed -n '470,530p' backend/src/validation/schemas.ts
fi
echo "== zod version =="
if [ -f package.json ]; then cat package.json | sed -n '1,220p' | rg -n '"zod"|dependencies|devDependencies' -A20 -B20 || true; fi
if [ -f backend/package.json ]; then cat backend/package.json | sed -n '1,220p' | rg -n '"zod"|dependencies|devDependencies' -A20 -B20 || true; fi
if [ -f pnpm-lock.yaml ]; then rg -n 'zod@|/zod@|zod:' pnpm-lock.yaml | head -80 || true; fi
if [ -f package-lock.json ]; then node -e 'const p=require("./package-lock.json"); console.log(JSON.stringify({packages: Object.keys(p.packages||{}).filter(x=>/node_modules\/zod$/.test(x)).slice(0,20), deps:p.packages?.[""].dependencies?.zod, devdeps:p.packages?.[""].devDependencies?.zod}, null,2));' || true; fi
echo "== timestamp parser tests via local node Date.parse =="
node - <<'JS'
const inputs = ['Dec 25 2024', '12/25/2024', '2024-01-01', '2024-01-01T00:00:00', '2024-01-01+02:00', 'invalid'];
const results = new Map();
for (const s of inputs) {
const ts = Date.parse(s);
results.set(s, isNaN(ts) ? 'invalid' : ts.toString());
}
for (const [s, ts] of results) console.log(`${s || '(blank)'} => ${ts}`);
const dateOnly = Math.floor(Date.parse('2024-01-01') / 1000);
const localMidnight = Math.floor(Date.parse('2024-01-01T00:00:00') / 1000);
const utcMidnight = Math.floor(Date.parse('2024-01-01T00:00:00Z') / 1000);
console.log(`offset_diff_ms=${localMidnight*1000 - dateOnly*1000}`);
console.log(`utc_vs_local_ms=${utcMidnight*1000 - localMidnight*1000}`);
const d = new Date();
console.log(`timezone=${d.getTimezoneOffset()} min`);
JSRepository: ritik4ever/stellar-goal-vault
Length of output: 8610
🌐 Web query:
MDN Date.parse non-ISO strings implementation specific date-only parsed as UTC
💡 Result:
In JavaScript, the parsing behavior of Date.parse depends on whether the input string conforms to the ECMAScript "Date Time String Format" (a simplified subset of ISO 8601) [1][2][3]. When a string conforms to this standard format, the ECMAScript specification mandates a specific behavior regarding time zones: 1. Date-only forms (e.g., "YYYY-MM-DD") are interpreted as UTC [1][3]. 2. Date-time forms (e.g., "YYYY-MM-DDTHH:mm:ss") without a time zone offset are interpreted as local time [1][3]. This distinction—where date-only strings default to UTC—is a long-standing behavior that deviates from standard ISO 8601 (which treats date-only strings as local time), but it is maintained in ECMAScript for web compatibility [1]. For non-standard strings—those that do not conform to the ECMAScript Date Time String Format—the behavior of Date.parse is entirely implementation-dependent [1][3][4]. While many browser engines historically attempted to support various non-standard formats (often defaulting to the local time zone), there is no cross-browser guarantee [5][1][6]. Because these heuristics are not formally standardized, they can vary significantly between environments and may change over time as browsers update their parsing logic [5][7][6]. Due to this inconsistency, it is widely considered best practice to avoid relying on Date.parse for non-standard strings, as the output may be unpredictable and inconsistent across different devices, locales, or browser versions [5][6][2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date
- 2: https://stackoverflow.com/questions/2587345/why-does-date-parse-give-incorrect-results
- 3: https://tc39.es/proposal-uniform-interchange-date-parsing/
- 4: https://learn.microsoft.com/en-us/openspecs/ie_standards/ms-es5/3fbe8262-3f94-4d8d-8bb0-69d05b66a1d8
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse
- 6: https://github.com/mdn/content/blob/main/files/en-us/web/javascript/reference/global_objects/date/parse/index.md
- 7: Update "Non-standard date strings" section of
Date.parseJS reference mdn/content#30363
🌐 Web query:
MDN Date.parse non-ISO strings implementation specific date-only parsed as UTC
💡 Result:
The behavior of Date.parse in JavaScript regarding non-ISO strings is fundamentally divided into two categories: standardized (ISO-compliant) and implementation-specific (non-standard) [1][2]. Standardized ISO Date Strings When a date string conforms to the ECMAScript Date Time String Format (a simplified subset of ISO 8601), it follows strict parsing rules defined by the ECMAScript specification [1][2]: - Date-only strings (e.g., YYYY-MM-DD) without a time zone offset are parsed as UTC [1][3]. - Date-time strings (e.g., YYYY-MM-DDTHH:mm:ss) without a time zone offset are parsed as local time [1][4]. This distinction, particularly the UTC interpretation of date-only strings, is a historical artifact maintained for web compatibility, despite it differing from the broader ISO 8601 standard which typically defaults to local time [1][4]. Non-Standard / Non-ISO Strings If a string does not conform to the Date Time String Format, Date.parse behavior becomes implementation-defined [2][5]. Implementations (browsers and runtimes) are free to apply their own heuristics to parse these strings [1][6]. Key characteristics of non-standard parsing include: - Local Time Bias: Most implementations default to the local time zone when parsing non-standard strings, which is a major source of inconsistency across different environments [7][6]. - Browser Lenience: Browsers are notoriously lenient and often replicate each other's behaviors for compatibility, meaning non-standard formats often "work" in browsers while potentially failing in other JavaScript runtimes (like Node.js or older engines) [7][8]. - Unreliability: Because non-standard parsing depends on the specific engine's heuristics, results can vary significantly based on the device's time zone, Daylight Saving Time (DST) settings, and even the specific browser version [7][8][6]. Due to these inconsistencies, the use of Date.parse with non-standard strings is strongly discouraged [1][9]. Developers are advised to rely on ISO 8601 strings or modern date/time libraries (like Temporal, where available, or date-fns/Luxon) to ensure predictable, cross-environment behavior [9].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date
- 2: https://tc39.es/proposal-uniform-interchange-date-parsing/
- 3: https://stackoverflow.com/questions/2587345/why-does-date-parse-give-incorrect-results
- 4: Normative: Date.parse treats date-only no offset as UTC tc39/ecma262#138
- 5: Request For Clarification: Normative requirements of Date.parse() tc39/ecma262#3526
- 6: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global%5FObjects/Date/parse
- 7: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse
- 8: https://github.com/mdn/content/blob/main/files/en-us/web/javascript/reference/global_objects/date/parse/index.md
- 9: Update "Non-standard date strings" section of
Date.parseJS reference mdn/content#30363
Normalize and validate the created date filters with ISO validation
Date.parse accepts non-ISO inputs and non-ISO parsing is implementation-defined, so the ISO-only error message can mislead clients. Date-only strings are parsed as UTC while timezone-less datetimes are parsed as local time, making deploy-region offsets affect identical-looking filters. Add a createdAfter <= createdBefore check to avoid silently returning zero rows.
🐛 Proposed fix using Zod 4.3.6 ISO validators
+const isoDateTimeSchema = z.union([z.iso.datetime({ offset: true }), z.iso.date()]);
+
+function parseIsoDateQueryParam(
+ raw: string,
+ field: string,
+): { ok: true; value: number } | { ok: false; issues: z.core.$ZodIssue[] } {
+ if (!isoDateTimeSchema.safeParse(raw).success) {
+ return {
+ ok: false,
+ issues: [{ code: 'custom', message: `${field} must be a valid ISO 8601 date string.`, path: [field] }],
+ };
+ }
+ // Normalize date-only values to UTC midnight so results are deploy-independent.
+ const normalized = /^\d{4}-\d{2}-\d{2}$/.test(raw) ? `${raw}T00:00:00Z` : raw;
+ return { ok: true, value: Math.floor(Date.parse(normalized) / 1000) };
+}Then use parseIsoDateQueryParam(rawCreatedAfter, 'createdAfter') / (rawCreatedBefore, 'createdBefore') in place of the inline Date.parse blocks.
🤖 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 487 - 516, Replace the inline
Date.parse validation in the createdAfter and createdBefore handling with the
shared parseIsoDateQueryParam helper, preserving issue reporting for invalid
values and Unix-second normalization. After both values are parsed, validate
that createdAfter is not later than createdBefore and add a validation issue
when the range is reversed.
|
Hi @praizeD10, 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 @praizeD10, 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! |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
backend/src/validation/schemas.ts (3)
406-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPagination validation is duplicated from
parseCampaignListPaginationQuery.Lines 406-436 repeat the page/limit logic of
parseCampaignListPaginationQuery(lines 263-310), including the both-or-neither rule, the 1-100 bound, and the messages. The two copies will diverge. Delegate instead.♻️ 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) { - // Unpaginated lists are supported when both parameters are omitted. - } else if (pageStr === undefined || limitStr === undefined) { - issues.push({ - code: 'custom', - message: 'Pagination requires both page and limit query parameters.', - path: pageStr === undefined ? ['page'] : ['limit'], - }); - } else { - const pageNum = Number(pageStr); - const limitNum = Number(limitStr); - if (!Number.isFinite(pageNum) || !Number.isInteger(pageNum) || pageNum < 1) { - issues.push({ code: 'custom', message: 'page must be a positive integer.', path: ['page'] }); - } else { - page = pageNum; - } - if ( - !Number.isFinite(limitNum) || - !Number.isInteger(limitNum) || - limitNum < 1 || - limitNum > 100 - ) { - issues.push({ - code: 'custom', - message: 'limit must be an integer from 1 to 100.', - path: ['limit'], - }); - } else { - limit = limitNum; - } - } + let page: number | undefined; + let limit: number | undefined; + const pagination = parseCampaignListPaginationQuery(query); + if (!pagination.ok) { + issues.push(...pagination.issues); + } else { + page = pagination.page; + limit = pagination.limit; + }🤖 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 406 - 436, Replace the duplicated pagination validation block in the schema refinement with a call to parseCampaignListPaginationQuery, reusing its both-or-neither handling, bounds, and messages. Propagate the helper’s validation issues and parsed page/limit values through the existing schema flow, while preserving the current unpaginated behavior.
101-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe whitespace refinement on
titleis unreachable. Both title schemas call.trim()before.min(4), so the value reaching the refinement is already trimmed andmin(4)rejects whitespace-only input first. The message "Title cannot be only whitespace." can never appear.
backend/src/validation/schemas.ts#L101-L106: remove.refine((val) => val.trim().length >= 4, 'Title cannot be only whitespace.')fromcreateCampaignPayloadSchema.title.backend/src/validation/schemas.ts#L184-L190: remove the same refinement fromupdateMetadataPayloadSchema.title.🤖 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 101 - 106, Remove the unreachable whitespace-only refinement from the title definitions in backend/src/validation/schemas.ts at lines 101-106 (createCampaignPayloadSchema.title) and 184-190 (updateMetadataPayloadSchema.title), while leaving the surrounding length, script-tag, SQL-comment, and sanitization validations unchanged.
23-48: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate base64 payloads before accepting data URLs.
The current regex accepts any payload after
base64,, so malformed inputs likedata:image/jpeg;base64,@@@@pass validation and fail only when decoded downstream. Use a strict base64 character/padding pattern and compute size from the decoded length, includingimage/jpgin the accepted MIME list if that client behavior must remain supported.🤖 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 23 - 48, Update imageUrlSchema’s data-URL branch to validate the base64 payload with a strict character and padding pattern before accepting it, rejecting malformed values such as “@@@@”. Compute the 2MB limit from the decoded payload length rather than the encoded string length, and extend the MIME pattern to accept image/jpg alongside jpeg and png if required by existing clients.
🤖 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/validation/schemas.ts`:
- Around line 364-370: Update parseIso8601Timestamp to accept only explicitly
supported ISO 8601 timestamp forms, including a required timezone for datetimes,
instead of relying on implementation-defined new Date parsing. Apply the same
strict validation to the related timestamp schema handling, and add validation
ensuring createdAfter is less than or equal to createdBefore.
- Around line 191-198: Update the description validator in the relevant update
payload schema to enforce the same 20-character minimum as
createCampaignPayloadSchema, while retaining its existing maximum, refinements,
transformation, and optional behavior.
- Around line 461-509: Make case handling consistent in the campaign query
validation around status, sort, and order: accept case-insensitive values for
all three filters. Preserve the camelCase CampaignSortField values by performing
a case-insensitive lookup for sort rather than storing a lowercased value, and
apply the same normalized matching to order while retaining the existing
validation messages and typed outputs.
- Around line 644-651: Update createCommentPayloadSchema’s content validation to
match createCampaignPayloadSchema by rejecting script tags and SQL comment
sequences, then applying sanitizeInput before accepting the value. If output
escaping is intentionally handled in the render layer instead, document that
decision directly in this schema to make the validation asymmetry explicit.
- Around line 199-210: Update the PATCH metadata flow around the campaign
validation schema and updateCampaign call so partial metadata updates preserve
existing metadata fields: merge the validated patch metadata with the campaign’s
current metadata before serializing metadata_json. Ensure an empty metadata
object does not clear existing fields, or explicitly reject null metadata if
null-removal is unsupported.
- Around line 513-527: Update the validation block around includeDeletedValue to
track whether includeDeleted or include_archived supplied the value, then use
the present parameter’s name in the issue path. Preserve the existing validation
message and boolean conversion behavior.
---
Nitpick comments:
In `@backend/src/validation/schemas.ts`:
- Around line 406-436: Replace the duplicated pagination validation block in the
schema refinement with a call to parseCampaignListPaginationQuery, reusing its
both-or-neither handling, bounds, and messages. Propagate the helper’s
validation issues and parsed page/limit values through the existing schema flow,
while preserving the current unpaginated behavior.
- Around line 101-106: Remove the unreachable whitespace-only refinement from
the title definitions in backend/src/validation/schemas.ts at lines 101-106
(createCampaignPayloadSchema.title) and 184-190
(updateMetadataPayloadSchema.title), while leaving the surrounding length,
script-tag, SQL-comment, and sanitization validations unchanged.
- Around line 23-48: Update imageUrlSchema’s data-URL branch to validate the
base64 payload with a strict character and padding pattern before accepting it,
rejecting malformed values such as “@@@@”. Compute the 2MB limit from the
decoded payload length rather than the encoded string length, and extend the
MIME pattern to accept image/jpg alongside jpeg and png if required by existing
clients.
🪄 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: 938f0283-b7aa-4038-bcf5-43af8b0ff95c
📒 Files selected for processing (2)
backend/src/middleware/validateBody.test.tsbackend/src/validation/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/middleware/validateBody.test.ts
| description: z | ||
| .string() | ||
| .trim() | ||
| .max(500) | ||
| .refine((val) => !containsScriptTag(val), 'Description cannot contain script tags.') | ||
| .refine((val) => !containsSqlComment(val), 'Description cannot contain SQL comment sequences.') | ||
| .transform((val) => sanitizeInput(val)) | ||
| .optional(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
description loses the 20-character minimum on update.
createCampaignPayloadSchema.description requires .min(20). This update schema only sets .max(500). A PATCH request with description: "" therefore passes validation and clears a description that could never have been created that way. Align the two schemas.
🐛 Proposed fix
description: z
.string()
.trim()
+ .min(20, 'Description must be at least 20 characters.')
.max(500)📝 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.
| description: z | |
| .string() | |
| .trim() | |
| .max(500) | |
| .refine((val) => !containsScriptTag(val), 'Description cannot contain script tags.') | |
| .refine((val) => !containsSqlComment(val), 'Description cannot contain SQL comment sequences.') | |
| .transform((val) => sanitizeInput(val)) | |
| .optional(), | |
| description: z | |
| .string() | |
| .trim() | |
| .min(20, 'Description must be at least 20 characters.') | |
| .max(500) | |
| .refine((val) => !containsScriptTag(val), 'Description cannot contain script tags.') | |
| .refine((val) => !containsSqlComment(val), 'Description cannot contain SQL comment sequences.') | |
| .transform((val) => sanitizeInput(val)) | |
| .optional(), |
🤖 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 191 - 198, Update the
description validator in the relevant update payload schema to enforce the same
20-character minimum as createCampaignPayloadSchema, while retaining its
existing maximum, refinements, transformation, and optional behavior.
| metadata: z | ||
| .object({ | ||
| imageUrl: imageUrlSchema.optional(), | ||
| externalLink: httpsOnlyUrlSchema.optional(), | ||
| }) | ||
| .optional(), | ||
| }) | ||
| .refine( | ||
| (data) => | ||
| data.title !== undefined || data.description !== undefined || data.metadata !== undefined, | ||
| { message: 'At least one field (title, description, or metadata) must be provided.' }, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the PATCH metadata route applies updateMetadataPayloadSchema output.
set -euo pipefail
rg -n -C 15 'updateMetadataPayloadSchema' --type=ts
# Find the metadata update path in the store/route layer.
rg -n -C 10 'metadata' --type=ts -g 'backend/src/services/campaignStore.ts'Repository: ritik4ever/stellar-goal-vault
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked relevant files:\n'
git ls-files | rg 'backend/src/(validation/schemas\.ts|.*metadata|.*campaign).*\.ts$|backend/src/services/campaignStore\.ts' || true
printf '\nSchemas around metadata:\n'
if [ -f backend/src/validation/schemas.ts ]; then
nl -ba backend/src/validation/schemas.ts | sed -n '1,280p'
fi
printf '\nSearch metadata terms:\n'
rg -n -C 8 'metadata|updateMetadataPayloadSchema|imageUrlSchema|httpsOnlyUrlSchema|PATCH|patch' backend/src --type=ts || trueRepository: ritik4ever/stellar-goal-vault
Length of output: 473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Schema file excerpts:\n'
if [ -f backend/src/validation/schemas.ts ]; then
awk '{printf "%6d\t%s\n", NR, $0}' backend/src/validation/schemas.ts | sed -n '1,280p'
else
echo 'backend/src/validation/schemas.ts not found'
fi
printf '\nMetadata and route/update references:\n'
rg -n -C 8 'metadata|updateMetadataPayloadSchema|imageUrlSchema|httpsOnlyUrlSchema|PATCH|patch|partial' backend/src --type=ts || true
printf '\nCampaign store metadata handling:\n'
awk '{printf "%6d\t%s\n", NR, $0}' backend/src/services/campaignStore.ts | sed -n '1,260p'
printf '\nSearch updateMetadata implementation/assignment:\n'
rg -n -C 8 'updateMetadata|metadata\s*=|metadata update|patchMetadata|partialMetadata' backend/src --type=ts || trueRepository: ritik4ever/stellar-goal-vault
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'campaignStore outline:\n'
ast-grep outline backend/src/services/campaignStore.ts --view expanded 2>/dev/null || true
printf '\ncampaignStore metadata/updateCampaign excerpts:\n'
awk '{printf "%6d\t%s\n", NR, $0}' backend/src/services/campaignStore.ts | sed -n '1,280p'
printf '\nFocused updateCampaign implementation:\n'
rg -n -C 25 'function updateCampaign|export function updateCampaign|updateCampaign\(' backend/src/services/campaignStore.ts --type=ts
printf '\nCampaignStore tests around metadata updates:\n'
rg -n -C 20 'metadata|updateMetadataPayloadSchema|imageUrl|externalLink' backend/src/services/campaignStore.test.ts backend/src/services/__tests__ --type=ts || trueRepository: ritik4ever/stellar-goal-vault
Length of output: 50385
Preserve existing metadata fields on PATCH.
/api/campaigns/:id/metadata passes the validated payload to updateCampaign, which writes metadata_json = ? with JSON.stringify(patch.metadata). A valid request like {"metadata": {"imageUrl": "..."}} clears any existing externalLink, and {"metadata": {}} clears both fields. Merge unknown metadata fields with the current campaign metadata, or reject metadata entirely if null should remove it.
🤖 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 199 - 210, Update the PATCH
metadata flow around the campaign validation schema and updateCampaign call so
partial metadata updates preserve existing metadata fields: merge the validated
patch metadata with the campaign’s current metadata before serializing
metadata_json. Ensure an empty metadata object does not clear existing fields,
or explicitly reject null metadata if null-removal is unsupported.
| function parseIso8601Timestamp(value: unknown): number | null { | ||
| if (typeof value !== 'string') { | ||
| return null; | ||
| } | ||
| const timestamp = new Date(value).getTime(); | ||
| return Number.isFinite(timestamp) ? Math.floor(timestamp / 1000) : null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
new Date(value) does not enforce ISO 8601.
parseIso8601Timestamp accepts implementation-defined formats such as Dec 25 2024, so the message "must be a valid ISO 8601 timestamp" can mislead clients. Date-only strings parse as UTC while timezone-less datetimes parse as local time, so the deployment region changes the result. A createdAfter <= createdBefore check is also still absent. This repeats a finding from an earlier commit.
Also applies to: 529-552
🤖 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 364 - 370, Update
parseIso8601Timestamp to accept only explicitly supported ISO 8601 timestamp
forms, including a required timezone for datetimes, instead of relying on
implementation-defined new Date parsing. Apply the same strict validation to the
related timestamp schema handling, and add validation ensuring createdAfter is
less than or equal to createdBefore.
| const statusValue = normalizeQueryValue(query.status)?.toLowerCase(); | ||
| const statuses: CampaignStatus[] = ['open', 'funded', 'claimed', 'failed']; | ||
| let status: CampaignStatus | undefined; | ||
| if (statusValue !== undefined) { | ||
| if (!statuses.includes(statusValue as CampaignStatus)) { | ||
| issues.push({ | ||
| code: 'custom', | ||
| message: `status must be one of: ${statuses.join(', ')}`, | ||
| path: ['status'], | ||
| }); | ||
| } else { | ||
| status = statusValue as CampaignStatus; | ||
| } | ||
| } | ||
|
|
||
| const sortValue = normalizeQueryValue(query.sort); | ||
| const sortFields: CampaignSortField[] = [ | ||
| 'createdAt', | ||
| 'deadline', | ||
| 'pledgedAmount', | ||
| 'targetAmount', | ||
| ]; | ||
| let sort: CampaignSortField | undefined; | ||
| if (sortValue !== undefined) { | ||
| if (!sortFields.includes(sortValue as CampaignSortField)) { | ||
| issues.push({ | ||
| code: 'custom', | ||
| message: `sort must be one of: ${sortFields.join(', ')}`, | ||
| path: ['sort'], | ||
| }); | ||
| } else { | ||
| sort = sortValue as CampaignSortField; | ||
| } | ||
| } | ||
|
|
||
| const orderValue = normalizeQueryValue(query.order); | ||
| const orders: SortOrder[] = ['asc', 'desc']; | ||
| let order: SortOrder | undefined; | ||
| if (orderValue !== undefined) { | ||
| if (!orders.includes(orderValue as SortOrder)) { | ||
| issues.push({ | ||
| code: 'custom', | ||
| message: `order must be one of: ${orders.join(', ')}`, | ||
| path: ['order'], | ||
| }); | ||
| } else { | ||
| order = orderValue as SortOrder; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Case handling is inconsistent across the enum filters.
Line 461 lowercases status, so ?status=OPEN is accepted. Lines 476 and 496 do not lowercase sort and order, so ?order=DESC and ?sort=CreatedAt are rejected. Pick one rule for all three filters. sort field names are camelCase, so lowercasing needs a case-insensitive lookup rather than a plain toLowerCase().
♻️ Proposed fix for `order`
- const orderValue = normalizeQueryValue(query.order);
+ const orderValue = normalizeQueryValue(query.order)?.toLowerCase();🤖 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 461 - 509, Make case handling
consistent in the campaign query validation around status, sort, and order:
accept case-insensitive values for all three filters. Preserve the camelCase
CampaignSortField values by performing a case-insensitive lookup for sort rather
than storing a lowercased value, and apply the same normalized matching to order
while retaining the existing validation messages and typed outputs.
| const includeDeletedValue = | ||
| singleCampaignListQueryParam(query.includeDeleted) ?? | ||
| singleCampaignListQueryParam(query.include_archived); | ||
| let includeDeleted: boolean | undefined; | ||
| if (includeDeletedValue !== undefined) { | ||
| if (includeDeletedValue !== 'true' && includeDeletedValue !== 'false') { | ||
| issues.push({ | ||
| code: 'custom', | ||
| message: "includeDeleted (or include_archived) must be 'true' or 'false'.", | ||
| path: ['includeDeleted'], | ||
| }); | ||
| } else { | ||
| includeDeleted = includeDeletedValue === 'true'; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The issue path names the wrong query parameter for the alias.
If a client sends ?include_archived=yes, the 422 response reports path: ['includeDeleted']. The client cannot map that field back to the parameter it sent. Report the parameter that was actually present.
🐛 Proposed fix
- const includeDeletedValue =
- singleCampaignListQueryParam(query.includeDeleted) ??
- singleCampaignListQueryParam(query.include_archived);
+ const canonicalIncludeDeleted = singleCampaignListQueryParam(query.includeDeleted);
+ const includeDeletedField = canonicalIncludeDeleted === undefined ? 'include_archived' : 'includeDeleted';
+ const includeDeletedValue =
+ canonicalIncludeDeleted ?? singleCampaignListQueryParam(query.include_archived);
let includeDeleted: boolean | undefined;
if (includeDeletedValue !== undefined) {
if (includeDeletedValue !== 'true' && includeDeletedValue !== 'false') {
issues.push({
code: 'custom',
message: "includeDeleted (or include_archived) must be 'true' or 'false'.",
- path: ['includeDeleted'],
+ path: [includeDeletedField],
});📝 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 includeDeletedValue = | |
| singleCampaignListQueryParam(query.includeDeleted) ?? | |
| singleCampaignListQueryParam(query.include_archived); | |
| let includeDeleted: boolean | undefined; | |
| if (includeDeletedValue !== undefined) { | |
| if (includeDeletedValue !== 'true' && includeDeletedValue !== 'false') { | |
| issues.push({ | |
| code: 'custom', | |
| message: "includeDeleted (or include_archived) must be 'true' or 'false'.", | |
| path: ['includeDeleted'], | |
| }); | |
| } else { | |
| includeDeleted = includeDeletedValue === 'true'; | |
| } | |
| } | |
| const canonicalIncludeDeleted = singleCampaignListQueryParam(query.includeDeleted); | |
| const includeDeletedField = canonicalIncludeDeleted === undefined ? 'include_archived' : 'includeDeleted'; | |
| const includeDeletedValue = | |
| canonicalIncludeDeleted ?? singleCampaignListQueryParam(query.include_archived); | |
| let includeDeleted: boolean | undefined; | |
| if (includeDeletedValue !== undefined) { | |
| if (includeDeletedValue !== 'true' && includeDeletedValue !== 'false') { | |
| issues.push({ | |
| code: 'custom', | |
| message: "includeDeleted (or include_archived) must be 'true' or 'false'.", | |
| path: [includeDeletedField], | |
| }); | |
| } else { | |
| includeDeleted = includeDeletedValue === 'true'; | |
| } | |
| } |
🤖 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 513 - 527, Update the
validation block around includeDeletedValue to track whether includeDeleted or
include_archived supplied the value, then use the present parameter’s name in
the issue path. Preserve the existing validation message and boolean conversion
behavior.
| export const createCommentPayloadSchema = z.object({ | ||
| author: stellarAccountIdSchema, | ||
| content: z | ||
| .string() | ||
| .trim() | ||
| .min(1, 'Comment content cannot be empty.') | ||
| .max(500, 'Comment content cannot exceed 500 characters.'), | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
content skips the sanitization applied to campaign text.
createCampaignPayloadSchema rejects script tags and SQL comment sequences and then applies sanitizeInput. This comment schema applies neither. Comment bodies are user-supplied and rendered, so apply the same treatment for a consistent posture. If the render layer escapes output instead, state that in a comment here so the asymmetry is intentional and visible.
🛡️ Proposed fix
content: z
.string()
.trim()
.min(1, 'Comment content cannot be empty.')
- .max(500, 'Comment content cannot exceed 500 characters.'),
+ .max(500, 'Comment content cannot exceed 500 characters.')
+ .refine((val) => !containsScriptTag(val), 'Comment cannot contain script tags.')
+ .refine((val) => !containsSqlComment(val), 'Comment cannot contain SQL comment sequences.')
+ .transform((val) => sanitizeInput(val)),📝 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.
| export const createCommentPayloadSchema = z.object({ | |
| author: stellarAccountIdSchema, | |
| content: z | |
| .string() | |
| .trim() | |
| .min(1, 'Comment content cannot be empty.') | |
| .max(500, 'Comment content cannot exceed 500 characters.'), | |
| }); | |
| export const createCommentPayloadSchema = z.object({ | |
| author: stellarAccountIdSchema, | |
| content: z | |
| .string() | |
| .trim() | |
| .min(1, 'Comment content cannot be empty.') | |
| .max(500, 'Comment content cannot exceed 500 characters.') | |
| .refine((val) => !containsScriptTag(val), 'Comment cannot contain script tags.') | |
| .refine((val) => !containsSqlComment(val), 'Comment cannot contain SQL comment sequences.') | |
| .transform((val) => sanitizeInput(val)), | |
| }); |
🤖 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 644 - 651, Update
createCommentPayloadSchema’s content validation to match
createCampaignPayloadSchema by rejecting script tags and SQL comment sequences,
then applying sanitizeInput before accepting the value. If output escaping is
intentionally handled in the render layer instead, document that decision
directly in this schema to make the validation asymmetry explicit.
…H metadata endpoint - validateBody middleware returns 422 Unprocessable Entity (was 400) - validateBody.test.ts assertions updated to expect 422 - Add updateMetadataPayloadSchema to validation/schemas.ts - Add PATCH /api/campaigns/:id/metadata route to index.ts with creator ownership check, metadata merge semantics, and rate limiting
d276bbe to
4eb8c81
Compare
|
conflict resolved @ritik4ever |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
3 participants
closes #575
Summary by CodeRabbit
New Features
Bug Fixes
Tests