refactor: replace validateURL with isValidTokenListSource for URL validation#7406
Conversation
|
@Kropiunig is attempting to deploy a commit to the cow Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughReplace duplicated URL/ENS validation with a new shared ChangesToken-list source validation (single cohort)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Kropiunig
force-pushed
the
fix/validate-url-non-http-schemes
branch
from
68e7d68 to
01530cc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/widget-configurator/src/app/configurator/utils/validateURL.test.ts (1)
11-24: Add coverage for the ENS path andtrim()behavior.The implementation also added two behaviors that aren't asserted here: (1) ENS acceptance via
parseENSAddress(the earlyreturn trueon line 6 ofvalidateURL.ts) and (2) input trimming (line 4). Without tests, regressions in either branch would slip through silently — e.g., someone moving thetrim()after the protocol check would not break any test, even though' https://example.com 'would start failing.🧪 Suggested additions
+ it('accepts ENS names', () => { + expect(validateURL('tokens.uniswap.eth')).toBe(true) + expect(validateURL('list.tokens.eth/path')).toBe(true) + }) + + it('trims surrounding whitespace before validating', () => { + expect(validateURL(' https://example.com/list.json ')).toBe(true) + expect(validateURL('\thttps://example.com/list.json\n')).toBe(true) + expect(validateURL(' ')).toBe(false) + }) + + it('rejects empty input', () => { + expect(validateURL('')).toBe(false) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/widget-configurator/src/app/configurator/utils/validateURL.test.ts` around lines 11 - 24, Add tests to validateURL.test.ts to cover two missing behaviors: ensure validateURL accepts ENS-like inputs by asserting validateURL('example.eth') (or other ENS strings) returns true to exercise the parseENSAddress early-return branch in validateURL, and ensure trimming is applied by asserting that inputs with surrounding whitespace like ' https://example.com ' still return true to exercise the trim() behavior; update or add expectations accordingly so both the ENS and trimmed-input branches are covered.apps/cowswap-frontend/src/modules/tokensList/containers/ManageListsAndTokens/index.tsx (1)
47-54: Consider extracting the shared URL validation into a utility.Lines 47–54 now duplicate the exact logic in
apps/widget-configurator/src/app/configurator/utils/validateURL.ts(trim → ENS-first → http/https allowlist →uriToHttp(...).length > 0). Keeping two copies in sync invites future drift the next time the rules change (e.g., another scheme is rejected/added). Consider promoting it to a shared helper (e.g., avalidateListUrl/isValidListLocationexport in@cowprotocol/common-utils) and consuming it from both call sites.♻️ Sketch
// libs/common-utils/src/validateListLocation.ts import { parseENSAddress } from './parseENSAddress' import { uriToHttp } from './uriToHttp' export function validateListLocation(input: string): boolean { const value = input.trim() if (parseENSAddress(value)) return true const protocol = value.split(':')[0]?.toLowerCase() if (protocol !== 'http' && protocol !== 'https') return false return uriToHttp(value).length > 0 }Then in this file:
- const value = listInput.trim() - - if (parseENSAddress(value)) return true - - const protocol = value.split(':')[0]?.toLowerCase() - if (protocol !== 'http' && protocol !== 'https') return false - - return uriToHttp(value).length > 0 + return validateListLocation(listInput)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/tokensList/containers/ManageListsAndTokens/index.tsx` around lines 47 - 54, The URL validation logic in ManageListsAndTokens (trim → parseENSAddress → protocol check → uriToHttp length) is duplicated; extract it into a shared helper (e.g., validateListLocation or isValidListLocation) in the common-utils library and export it, then replace the inline logic in ManageListsAndTokens/index.tsx with a call to that helper; reference the existing helpers parseENSAddress and uriToHttp when implementing the new validateListLocation and update callers to import and use validateListLocation instead of repeating the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/cowswap-frontend/src/modules/tokensList/containers/ManageListsAndTokens/index.tsx`:
- Around line 47-54: The URL validation logic in ManageListsAndTokens (trim →
parseENSAddress → protocol check → uriToHttp length) is duplicated; extract it
into a shared helper (e.g., validateListLocation or isValidListLocation) in the
common-utils library and export it, then replace the inline logic in
ManageListsAndTokens/index.tsx with a call to that helper; reference the
existing helpers parseENSAddress and uriToHttp when implementing the new
validateListLocation and update callers to import and use validateListLocation
instead of repeating the logic.
In `@apps/widget-configurator/src/app/configurator/utils/validateURL.test.ts`:
- Around line 11-24: Add tests to validateURL.test.ts to cover two missing
behaviors: ensure validateURL accepts ENS-like inputs by asserting
validateURL('example.eth') (or other ENS strings) returns true to exercise the
parseENSAddress early-return branch in validateURL, and ensure trimming is
applied by asserting that inputs with surrounding whitespace like '
https://example.com ' still return true to exercise the trim() behavior; update
or add expectations accordingly so both the ENS and trimmed-input branches are
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 04fcbcaa-7474-4b2c-a452-daeb715324a8
📒 Files selected for processing (3)
apps/cowswap-frontend/src/modules/tokensList/containers/ManageListsAndTokens/index.tsxapps/widget-configurator/src/app/configurator/utils/validateURL.test.tsapps/widget-configurator/src/app/configurator/utils/validateURL.ts
|
Thank you @Kropiunig, we'll take a look soon-ish :) |
kernelwhisperer
force-pushed
the
fix/validate-url-non-http-schemes
branch
from
01530cc to
eb79d4a
Compare
kernelwhisperer
left a comment
kernelwhisperer
left a comment
There was a problem hiding this comment.
LGTM, but I need to test it.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@libs/common-utils/src/isValidTokenListSource.ts`:
- Around line 4-6: The validator function isValidTokenListSource currently
accepts sources that map to non-HTTP schemes via uriToHttp; trim the input and
explicitly require the source's scheme to be http or https before relying on
uriToHttp or parseENSAddress. Update isValidTokenListSource to trim the source
string, check that it starts with 'http://' or 'https://' (case-insensitive) and
only then call uriToHttp(source) to validate, falling back to
Boolean(parseENSAddress(source)) for ENS inputs.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2f93cb07-0a70-4ebc-b3ed-31412bcc112a
📒 Files selected for processing (7)
apps/cowswap-frontend/src/modules/tokensList/containers/ManageListsAndTokens/index.tsxapps/widget-configurator/src/app/configurator/controls/AddCustomListDialog.tsxapps/widget-configurator/src/app/configurator/utils/validateURL.test.tsapps/widget-configurator/src/app/configurator/utils/validateURL.tslibs/common-utils/src/index.tslibs/common-utils/src/isValidTokenListSource.test.tslibs/common-utils/src/isValidTokenListSource.ts
💤 Files with no reviewable changes (2)
- apps/widget-configurator/src/app/configurator/utils/validateURL.test.ts
- apps/widget-configurator/src/app/configurator/utils/validateURL.ts
✅ Files skipped from review due to trivial changes (2)
- libs/common-utils/src/index.ts
- libs/common-utils/src/isValidTokenListSource.test.ts
| export function isValidTokenListSource(source: string): boolean { | ||
| return uriToHttp(source).length > 0 || Boolean(parseENSAddress(source)) | ||
| } |
There was a problem hiding this comment.
Protocol allowlist is still missing in the shared validator.
Line 5 still treats uriToHttp(source).length > 0 as valid, so non-HTTP schemes can continue to pass when uriToHttp returns mapped URLs. Please enforce http/https explicitly (and trim first), then call uriToHttp.
Suggested fix
export function isValidTokenListSource(source: string): boolean {
- return uriToHttp(source).length > 0 || Boolean(parseENSAddress(source))
+ const value = source.trim()
+ if (!value) return false
+
+ if (parseENSAddress(value)) return true
+
+ const protocol = value.split(':')[0]?.toLowerCase()
+ if (protocol !== 'http' && protocol !== 'https') return false
+
+ return uriToHttp(value).length > 0
}🤖 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 `@libs/common-utils/src/isValidTokenListSource.ts` around lines 4 - 6, The
validator function isValidTokenListSource currently accepts sources that map to
non-HTTP schemes via uriToHttp; trim the input and explicitly require the
source's scheme to be http or https before relying on uriToHttp or
parseENSAddress. Update isValidTokenListSource to trim the source string, check
that it starts with 'http://' or 'https://' (case-insensitive) and only then
call uriToHttp(source) to validate, falling back to
Boolean(parseENSAddress(source)) for ENS inputs.
There was a problem hiding this comment.
Why should we disallow non-http schemes?
There was a problem hiding this comment.
@kernelwhisperer, that's a fair challenge — let me look at what uriToHttp does with those schemes before responding further.
@kernelwhisperer, that's a great challenge — my comment was overly restrictive for this specific context, and I think you're right to question it.
Here's the distinction I should have drawn more carefully:
validateURL (widget configurator) — a user-facing URL input field where enforcing http/https is a reasonable defence-in-depth measure, since there's no legitimate UX reason for a widget integrator to enter ipfs:// or data: directly.
isValidTokenListSource (shared utility) — a broader validator for token list sources, where non-HTTP schemes are entirely legitimate:
- The Uniswap token list spec explicitly recommends pinning lists to IPFS and referencing them by an ENS name that resolves to the contenthash. So
ipfs://,ipns://, andar://are expected inputs here. uriToHttpalready acts as a translation layer that convertsipfs://,ipns://, andar://into HTTP gateway URLs — so those schemes result in safe HTTP fetches regardless. IfuriToHttpreturns an empty array for a given scheme (e.g.javascript:,file:,blob:), the source is already rejected.- The test suite for
isValidTokenListSourceconfirms this intent: it acceptsipfs://,ipns://,ar://, and rejectsjavascript:,file:, andblob:— the truly dangerous ones.
So the current implementation of isValidTokenListSource (uriToHttp(source).length > 0 || Boolean(parseENSAddress(source))) is correct for this use case. I was wrong to flag it as an issue — the safety boundary is appropriately enforced by uriToHttp itself, not by an explicit protocol allowlist. I'll withdraw this concern.
✏️ Learnings added
Learnt from: kernelwhisperer
Repo: cowprotocol/cowswap PR: 7406
File: libs/common-utils/src/isValidTokenListSource.ts:4-6
Timestamp: 2026-05-08T11:00:38.516Z
Learning: In the cowprotocol/cowswap repo, `isValidTokenListSource` in `libs/common-utils/src/isValidTokenListSource.ts` intentionally accepts non-HTTP schemes like `ipfs://`, `ipns://`, and `ar://` because `uriToHttp` converts them to safe HTTP gateway URLs. The Uniswap token list spec recommends IPFS as the preferred hosting. Rejecting non-HTTP schemes here would be incorrect; the protocol allowlist restriction applies only to the widget-configurator's user-facing `validateURL` helper.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
kernelwhisperer
left a comment
kernelwhisperer
left a comment
There was a problem hiding this comment.
-
Correction : We do want to support these token-list sources (ipfs://, ipns://, ENS, data:, etc.). The confusion came from CodeRabbit interpreting validateURL as a generic HTTP(S)-only URL validator, but this input is actually a token-list source validator and the logic was copied from the swap app.
-
Pushed a commit updating this. I moved the shared behavior into isValidTokenListSource and removed the misleading widget-local validateURL wrapper.
-
I tested that
t2crtokens.ethworks andipfs://hashdoesn't error withEnter valid list location
kernelwhisperer
force-pushed
the
fix/validate-url-non-http-schemes
branch
from
9289b41 to
ec99444
Compare
kernelwhisperer
changed the title
kernelwhisperer
force-pushed
the
fix/validate-url-non-http-schemes
branch
from
ec99444 to
c383c07
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/widget-configurator/src/app/configurator/controls/AddCustomListDialog.tsx (1)
149-162:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate helper text and input type to match broader validation scope.
The validation now accepts more than just URLs (ENS names, ipfs:, ipns:, ar:, data:, /@fs/ schemes), but:
- The helper text on line 159 still says "Enter a valid URL", which is misleading.
- The
type="url"attribute on line 154 may trigger HTML5 browser validation that could reject valid ENS names or non-HTTP schemes.🔧 Suggested fix
<TextField error={hasErrors} margin="dense" id="url" label="List URL" - type="url" + type="text" fullWidth variant="outlined" value={customListUrl} onChange={handleUrlInputChange} - helperText={hasErrors && 'Enter a valid URL'} + helperText={hasErrors && 'Enter a valid token list source (URL or ENS name)'} required autoComplete="off" />🤖 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/widget-configurator/src/app/configurator/controls/AddCustomListDialog.tsx` around lines 149 - 162, The TextField currently enforces URL-only semantics: update the input to accept broader list identifiers by removing or changing type="url" to type="text" on the TextField used for customListUrl (the component using value={customListUrl} and onChange={handleUrlInputChange}), and update the helperText (currently showing 'Enter a valid URL' via hasErrors) to a neutral message like 'Enter a valid list address (URL, ENS, IPFS, AR, data, or /@fs/)' so validation and user guidance reflect supported schemes.
🤖 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.
Outside diff comments:
In
`@apps/widget-configurator/src/app/configurator/controls/AddCustomListDialog.tsx`:
- Around line 149-162: The TextField currently enforces URL-only semantics:
update the input to accept broader list identifiers by removing or changing
type="url" to type="text" on the TextField used for customListUrl (the component
using value={customListUrl} and onChange={handleUrlInputChange}), and update the
helperText (currently showing 'Enter a valid URL' via hasErrors) to a neutral
message like 'Enter a valid list address (URL, ENS, IPFS, AR, data, or /@fs/)'
so validation and user guidance reflect supported schemes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4af072c5-69cd-4eb3-b832-ec1fa670af0a
📒 Files selected for processing (7)
apps/cowswap-frontend/src/modules/tokensList/containers/ManageListsAndTokens/index.tsxapps/widget-configurator/src/app/configurator/controls/AddCustomListDialog.tsxapps/widget-configurator/src/app/configurator/utils/validateURL.test.tsapps/widget-configurator/src/app/configurator/utils/validateURL.tslibs/common-utils/src/index.tslibs/common-utils/src/isValidTokenListSource.test.tslibs/common-utils/src/isValidTokenListSource.ts
💤 Files with no reviewable changes (2)
- apps/widget-configurator/src/app/configurator/utils/validateURL.ts
- apps/widget-configurator/src/app/configurator/utils/validateURL.test.ts
✅ Files skipped from review due to trivial changes (1)
- libs/common-utils/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/common-utils/src/isValidTokenListSource.test.ts
- libs/common-utils/src/isValidTokenListSource.ts
- apps/cowswap-frontend/src/modules/tokensList/containers/ManageListsAndTokens/index.tsx
3 participants
Summary
isValidTokenListSourcevalidateURLhelper causing code rabbit to open fix(widget-configurator): validateURL accepts non-HTTP(S) schemes via uriToHttp #7235http:,https:,ipfs:,ipns:,ar:,data:,/@fs/, and ENS namesSummary by CodeRabbit
Release Notes
Bug Fixes
Tests