fix: validate cow-fi CMS inputs and revalidation flows#7618
Open
fairlighteth wants to merge 4 commits into
Open
fix: validate cow-fi CMS inputs and revalidation flows#7618fairlighteth wants to merge 4 commits into
fairlighteth wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds CMS validation/normalization utilities and applies them: slug validation and early 404s in article/topic routes, normalized search input and typed search action results, unified query-parameter serialization, and header-based, normalized revalidation request handling. ChangesCMS Validation and Input Normalization
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant SearchAction as searchArticlesAction
participant Normalize as normalizeSearchArticlesInput
participant Service as searchArticlesService
participant CMS as CMS Client
Client->>SearchAction: call with search input
SearchAction->>Normalize: validate/normalize term+pagination
Normalize-->>SearchAction: {searchTerm, page, pageSize}
SearchAction->>Service: call with normalized input
Service->>CMS: GET with toQueryParams(query)
CMS-->>Service: articles + pagination metadata
Service-->>SearchAction: results
SearchAction-->>Client: {success: true, data} or {success: false, error}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying explorer-dev with
|
| Latest commit: |
0920e11
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://836b03a9.explorer-dev-dxz.pages.dev |
| Branch Preview URL: | https://deepsec-medium-02-cowfi-cms.explorer-dev-dxz.pages.dev |
Deploying swap-dev with
|
| Latest commit: |
0920e11
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6d26327e.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://deepsec-medium-02-cowfi-cms.swap-dev-5u6.pages.dev |
fairlighteth
commented
Jun 6, 2026
fairlighteth
left a comment
fairlighteth
left a comment
Contributor
Author
There was a problem hiding this comment.
✅ AI Review (Codex GPT-5, worked 2m): follow-up addressed
Rechecked
- Author response: follow-up commit
5138ef9f3bc6d4645d4502b400ddb8acd6699b8f(fix: reject malformed cow-fi CMS validator inputs). - Code path:
apps/cow-fi/util/cmsValidation.ts. - Test coverage:
apps/cow-fi/util/cmsValidation.test.ts.
Result: Fixed. The current code validates page and pageSize before the blank-search early return and rejects non-plain-object revalidation payloads, so the prior malformed-input paths no longer pass through normalization.
🤖 Verification notes for AI agents
Verify the prior finding against current code only. Confirm whether the author's claimed fix addresses the specific failure mode, and avoid reopening broader or unrelated issues.
Prior finding:
- Blank searches bypassed pagination validation in normalizeSearchArticlesInput()
- normalizeRevalidateRequest() accepted arrays/non-plain objects because the object guard was too loose
Current verification:
- normalizeSearchArticlesInput() now validates page/pageSize before the blank-search early return
- normalizeRevalidateRequest() now rejects non-plain objects
- cmsValidation regression tests cover both malformed-input cases and pass on the current head
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/cow-fi/app/api/revalidate/route.ts (1)
33-33: ⚡ Quick winConsider constant-time comparison for secret validation.
Direct string comparison with
!==can be vulnerable to timing attacks, where an attacker measures response times to infer secret characters. For authentication secrets, usingcrypto.timingSafeEqualis best practice.🔒 Proposed fix using timingSafeEqual
+import { timingSafeEqual } from 'crypto' + +function constantTimeCompare(a: string, b: string): boolean { + const aBuffer = Buffer.from(a) + const bBuffer = Buffer.from(b) + if (aBuffer.length !== bBuffer.length) { + return false + } + return timingSafeEqual(aBuffer, bBuffer) +} + // Check for secret to confirm this is a valid request - if (secret !== REVALIDATE_SECRET) { + if (!secret || !constantTimeCompare(secret, REVALIDATE_SECRET)) { return NextResponse.json({ message: 'Invalid secret' }, { status: 401 }) }🤖 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 `@apps/cow-fi/app/api/revalidate/route.ts` at line 33, Replace the direct string comparison (secret !== REVALIDATE_SECRET) with a constant-time comparison: ensure both values are Buffers of the same length, return unauthorized immediately if lengths differ, then use crypto.timingSafeEqual to compare the Buffers; update the route handler that reads secret and REVALIDATE_SECRET to perform this check and reject requests when timingSafeEqual returns false.apps/cow-fi/app/(learn)/learn/topic/[topicSlug]/page.tsx (1)
40-46: ⚡ Quick winAdd null category check for consistent metadata handling.
After validation,
getCategoryBySlugcan still returnnullif the category doesn't exist in the CMS. Currently,category?.attributes || {}yields an empty object, makingnameundefined, which results in metadata like"undefined - Knowledge base".The article page (lines 57-62 in
learn/[article]/page.tsx) explicitly checksif (!article || !article.attributes)and returns proper "Article Not Found" metadata. Consider applying the same pattern here for consistency.♻️ Suggested fix
const category = await getCategoryBySlug(topicSlug) + + if (!category || !category.attributes) { + return getPageMetadata({ + absoluteTitle: 'Topic Not Found - Knowledge base', + description: 'The requested topic could not be found.', + }) + } + const { name, description = '' } = category.attributes return getPageMetadata({ absoluteTitle: `${name} - Knowledge base`, description, })🤖 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 `@apps/cow-fi/app/`(learn)/learn/topic/[topicSlug]/page.tsx around lines 40 - 46, The metadata generation can produce "undefined - Knowledge base" because category may be null; update the block that calls getCategoryBySlug(topicSlug) to explicitly check if (!category || !category.attributes) and return a not-found metadata via getPageMetadata (e.g., title "Category Not Found" and a suitable description) before destructuring attributes; keep the existing return path that uses const { name, description = '' } = category.attributes when category is present.
🤖 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.
Nitpick comments:
In `@apps/cow-fi/app/`(learn)/learn/topic/[topicSlug]/page.tsx:
- Around line 40-46: The metadata generation can produce "undefined - Knowledge
base" because category may be null; update the block that calls
getCategoryBySlug(topicSlug) to explicitly check if (!category ||
!category.attributes) and return a not-found metadata via getPageMetadata (e.g.,
title "Category Not Found" and a suitable description) before destructuring
attributes; keep the existing return path that uses const { name, description =
'' } = category.attributes when category is present.
In `@apps/cow-fi/app/api/revalidate/route.ts`:
- Line 33: Replace the direct string comparison (secret !== REVALIDATE_SECRET)
with a constant-time comparison: ensure both values are Buffers of the same
length, return unauthorized immediately if lengths differ, then use
crypto.timingSafeEqual to compare the Buffers; update the route handler that
reads secret and REVALIDATE_SECRET to perform this check and reject requests
when timingSafeEqual returns false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c4857883-066b-40d5-9f7d-f4eff21c856a
📒 Files selected for processing (9)
apps/cow-fi/app/(learn)/learn/[article]/page.tsxapps/cow-fi/app/(learn)/learn/topic/[topicSlug]/page.tsxapps/cow-fi/app/actions.tsapps/cow-fi/app/api/revalidate/route.tsapps/cow-fi/services/cms/index.tsapps/cow-fi/util/cmsValidation.test.tsapps/cow-fi/util/cmsValidation.tsapps/cow-fi/util/queryParams.test.tsapps/cow-fi/util/queryParams.ts
Contributor
Author
|
@coderabbitai following up on #7618 (review) I fixed the topic metadata issue in I did not apply the |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
Contributor
Author
|
@coderabbitai yes, please create the follow-up issue for the |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh issue create \
--repo cowprotocol/cowswap \
--title "hardening: use timingSafeEqual for bearer secret comparison in cow-fi revalidate route" \
--assignee fairlighteth \
--body "## Summary
The revalidation API route at \`apps/cow-fi/app/api/revalidate/route.ts\` currently compares the bearer secret using a plain \`===\` string comparison. This is susceptible to timing-based side-channel attacks, where an attacker could infer the secret length or partial matches through response timing differences.
## Suggested improvement
Replace the \`===\` comparison with Node.js \`crypto.timingSafeEqual\` to perform a constant-time comparison, eliminating the timing side-channel.
Example:
\`\`\`ts
import { timingSafeEqual } from 'crypto'
function isValidSecret(provided: string, expected: string): boolean {
const a = Buffer.from(provided)
const b = Buffer.from(expected)
if (a.length !== b.length) return false
return timingSafeEqual(a, b)
}
\`\`\`
## Context
This was identified during review of PR `#7618` (https://github.com/cowprotocol/cowswap/pull/7618) and intentionally deferred as a non-blocking hardening improvement rather than a practical bug for this endpoint. See the discussion at https://github.com/cowprotocol/cowswap/pull/7618#issuecomment-0.
Requested by `@fairlighteth`."Length of output: 1417
It documents the timing side-channel concern in 🧠 Learnings used |
Contributor
Author
✅ PR - Automated QA: cow-fi CMS boundary hardening verified in browserOutcome
Source under test
Environment:
Wallet state reached:
If re-running locally:
Expected results:
Browser coverage:
Why these adjacent flows were included:
Note: Firefox automation flakeImpact:
Observed behavior:
Interpretation:
Commands run:
Still manual if desired:
Artifacts:
Residual gaps:
|
alfetopito
reviewed
Jun 11, 2026
Comment on lines
+9
to
+17
| function getSecretFromHeaders(request: NextRequest): string | null { | ||
| const authorization = request.headers.get('authorization') | ||
|
|
||
| if (authorization?.startsWith('Bearer ')) { | ||
| return authorization.slice('Bearer '.length) | ||
| } | ||
|
|
||
| return request.headers.get('x-revalidate-secret') | ||
| } |
Collaborator
There was a problem hiding this comment.
I recall where this is coming from (invalidating cache manually and so forth). But isn't there a middleware or similar thing built-in in Nextjs for handling authentication?
I don't want to over complicate this, though. Only if it makes sense and it's a small change.
Comment on lines
+12
to
+14
| function isRecord(value: unknown): value is Record<string, unknown> { | ||
| return Object.prototype.toString.call(value) === '[object Object]' | ||
| } |
Collaborator
There was a problem hiding this comment.
Something similar already exists
cowswap/libs/common-utils/src/json-utils.ts
Lines 3 to 5 in 80fd25f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the cow-fi MEDIUM findings around CMS query construction and public cache/search entry points.
This PR hardens the CMS boundary while keeping the learn pages and article search behavior intact:
qsserialization.To Test
NX_DAEMON=false ./node_modules/.bin/nx run cow-fi:test --runInBandpasses.NX_DAEMON=false ./node_modules/.bin/nx run cow-fi:lint --verbosecompletes with 0 errors.bad slug,%2Fetc, or values containing query syntax; they should fail closed instead of altering the CMS query.pageandpageSizeshould be clamped to the configured limits.GET /api/revalidatereturns405.POST /api/revalidatewithout an auth header returns401.POST /api/revalidatewith a valid bearer secret and an allowlisted learn path succeeds.400and do not revalidate arbitrary routes.Background
Most of these findings shared one root cause: user-controlled values could reach Strapi query construction or cache invalidation without a strong normalization layer first.
This PR centralizes that normalization in cow-fi utilities so routes, server actions, and revalidation all fail closed from the same validation rules.
Summary by CodeRabbit
New Features
Bug Fixes
Tests