fix: harden frontend security entrypoints#7601
Conversation
- validate external links and bridge explorer URLs before rendering - stop trusting URL-derived recipients and arbitrary proxy recovery - scope Safe SDK iframe relaying to trusted widget and parent origins - remove browser exposure of the Pinata secret export
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces safe URL parsing utilities that validate protocol schemes, reject credentials, and normalize bare origins; applies them to link rendering across hook components, bridge/transaction/notification UI, manifest validation, and explorer helpers. Hardens the iframe SDK bridge to use explicit trusted origins for message posting instead of wildcards. Also includes account proxy input validation, trade state field consolidation, and Pinata environment variable updates. ChangesURL Safety Validation & Security Hardening
Input Validation, Trade State, & Environment Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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: |
d318f9f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bac7ab86.explorer-dev-dxz.pages.dev |
| Branch Preview URL: | https://fix-deepsec-high-frontend-ha.explorer-dev-dxz.pages.dev |
Deploying swap-dev with
|
| Latest commit: |
d318f9f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f223476c.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://fix-deepsec-high-frontend-ha.swap-dev-5u6.pages.dev |
- stop checking an unprefixed secret env var from browser code - warn only on missing client-visible Pinata config
…rontend-hardening # Conflicts: # apps/cowswap-frontend/src/modules/bridge/pure/TransactionLink/TransactionLinkDisplay.tsx # libs/widget-lib/src/cowSwapWidget.ts
- normalize bracketed IPv6 localhost before allowlist checks - add a regression test for http://[::1] development URLs
| const dappName = item.dapp?.name || t`Unknown Hook` | ||
| const safeWebsiteUrl = item.dapp ? getSafeAbsoluteUrl(item.dapp.website) : null | ||
| const websiteHostname = safeWebsiteUrl ? new URL(safeWebsiteUrl).hostname : null | ||
| const safeSimulationUrl = simulationData ? getSafeAbsoluteUrl(simulationData.link) : null |
There was a problem hiding this comment.
Makes hook website/simulation URLs safe before turning them into links, so a malicious hook cannot sneak in a javascript: link.
| @@ -0,0 +1,77 @@ | |||
| import React from 'react' | |||
There was a problem hiding this comment.
Adds a test proving an unsafe simulation URL is shown as text, not as a clickable link.
| simulationTooltip: string | ||
| } | ||
|
|
||
| function BundleSimulationStatus({ |
There was a problem hiding this comment.
Applies the same safety rule to already-applied hook simulation links, so bad simulation URLs are not clickable.
| @@ -0,0 +1,102 @@ | |||
| import React from 'react' | |||
There was a problem hiding this comment.
Adds coverage that applied hook simulation links become plain text when the URL is unsafe.
| INVALID_MANIFEST: msg`Invalid manifest format: Missing "cow_hook_dapp" property in manifest.json`, | ||
| SMART_CONTRACT_INCOMPATIBLE: msg`This hook is not compatible with smart contract wallets. It only supports EOA wallets.`, | ||
| INVALID_HOOK_ID: msg`Invalid hook dapp ID format. The ID must be a 64-character hexadecimal string.`, | ||
| INVALID_WEBSITE_URL: msg`Invalid website URL in manifest. Only http(s) URLs are allowed, with http limited to local development.`, |
There was a problem hiding this comment.
Adds the user-facing error message for rejecting unsafe website URLs in custom hook manifests.
fairlighteth
left a comment
fairlighteth
left a comment
There was a problem hiding this comment.
Adding lightweight reviewer notes for the remaining DeepSec hardening files.
| return i18n._(ERROR_MESSAGES.INVALID_HOOK_ID) | ||
| } | ||
|
|
||
| if (!getSafeAbsoluteUrl(dapp.website)) { |
There was a problem hiding this comment.
Rejects custom hook manifests with unsafe website URLs before saving or rendering them.
| const { i18n } = useLingui() | ||
| const tags = useMemo(() => { | ||
| const { version, website, type, conditions } = dapp | ||
| const safeWebsiteUrl = getSafeAbsoluteUrl(website) |
There was a problem hiding this comment.
Only renders the hook website as a link if it passes the safe URL check; otherwise it stays inert text.
|
|
||
| const { title, description, url, id } = currentNotification | ||
| const linkTarget = url && isInternal(url) ? '_parent' : '_blank' | ||
| const safeLink = |
There was a problem hiding this comment.
Validates CMS notification links before rendering, so CMS content cannot create unsafe Learn more links.
| } | ||
|
|
||
| export function TransactionLinkDisplay({ link, label, linkText }: TransactionLinkDisplayProps): ReactNode { | ||
| const safeLink = getSafeAbsoluteUrl(link) |
There was a problem hiding this comment.
Refuses to render bridge transaction links unless the URL is safe.
| statusResult, | ||
| explorerUrl, | ||
| } = props | ||
| const safeExplorerUrl = getSafeAbsoluteUrl(explorerUrl) || undefined |
There was a problem hiding this comment.
Cleans the bridge explorer URL before passing it down to pending and received bridge UI.
| }) | ||
| }) | ||
|
|
||
| describe('#getBlockExplorerUrl', () => { |
There was a problem hiding this comment.
Adds test coverage for the older explorer URL builder.
| export * from './request' | ||
| export * from './resolveENSContentHash' | ||
| export * from './retry' | ||
| export * from './safeLink' |
There was a problem hiding this comment.
Exports the new safe-link helper so app modules can use the same validation everywhere.
|
|
||
| if (isSafeMessageRequest(event.data)) { | ||
| this.appWindow.parent.postMessage(event.data, '*') | ||
| if ( |
There was a problem hiding this comment.
Locks Safe SDK message forwarding to the expected iframe and parent origins instead of forwarding any Safe-shaped message.
|
|
||
| // 10. Listen for Safe SDK messages from the iframe only when explicitly enabled by the host. | ||
| const iframeSafeSdkBridge = enableSafeSdkBridge ? new IframeSafeSdkBridge(window, iframeWindow) : null | ||
| const iframeSafeSdkBridge = enableSafeSdkBridge |
There was a problem hiding this comment.
Passes the trusted iframe and parent origins into the Safe SDK bridge so it can enforce the origin checks.
| msgstr "This transaction can be simulated before execution to ensure that it will be succeed, generating a detailed report of the transaction execution." | ||
|
|
||
| #: apps/cowswap-frontend/src/modules/hooksStore/pure/AddCustomHookForm/constants.tsx | ||
| msgid "Invalid website URL in manifest. Only http(s) URLs are allowed, with http limited to local development." |
There was a problem hiding this comment.
Adds the translated string for the new invalid hook website URL error.
- reduce Safe SDK bridge lint complexity - preserve UI exports in AppliedHookItem test mock
3 participants
What changed
recipientAddressURL param.Why
This addresses the frontend/widget DeepSec
HIGHfindings that can be fixed with validation and trust-boundary hardening.Out of scope for this PR:
cow-fiMultiSendinner callsValidation
Automated:
recipientAddress-only URL case.Manual QA:
recipientAddress=0x...does not enable recipient mode.postMessagetraffic is ignored.pr-7601-manual-qa.mp4
Reviewer notes:
recipient=<value>remains the intended custom-recipient URL param.recipientAddressparam is intentionally no longer trusted.