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.
|
|
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 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: |
88f524e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://42ec195f.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: |
88f524e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ab7b67c5.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
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cowswap-frontend/src/modules/hooksStore/pure/AppliedHookItem/index.tsx (1)
40-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReduce
AppliedHookItemcomplexity to unblock CI.Line 40 currently fails lint (
complexity21, max 20), so this PR won’t pass CI until this component is split/refactored.Suggested minimal refactor (extract simulation status branch)
+interface SimulationStatusContentProps { + safeSimulationUrl: string | null + simulationStatus: string +} + +function SimulationStatusContent({ safeSimulationUrl, simulationStatus }: SimulationStatusContentProps) { + return safeSimulationUrl ? ( + <a href={safeSimulationUrl} target="_blank" rel="noopener noreferrer"> + {simulationStatus} + <ExternalLinkIcon size={14} /> + </a> + ) : ( + <span>{simulationStatus}</span> + ) +} + export function AppliedHookItem({ account, hookDetails, @@ - {safeSimulationUrl ? ( - <a href={safeSimulationUrl} target="_blank" rel="noopener noreferrer"> - {simulationStatus} - <ExternalLinkIcon size={14} /> - </a> - ) : ( - <span>{simulationStatus}</span> - )} + <SimulationStatusContent safeSimulationUrl={safeSimulationUrl} simulationStatus={simulationStatus} />🤖 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/cowswap-frontend/src/modules/hooksStore/pure/AppliedHookItem/index.tsx` around lines 40 - 135, AppliedHookItem exceeds linted complexity; extract the simulation-status rendering branch into a new child component to reduce complexity. Create a new component (e.g., HookSimulationStatus) that accepts props used by the branch (isBundleSimulationReady, account, simulationData, safeSimulationUrl, simulationStatus, simulationTooltip) and moves the JSX that renders styledEl.SimulateContainer / SVG / link / InfoTooltip into that component; keep the TenderlySimulate / OldSimulateContainer logic in AppliedHookItem but replace the inlined bundle-simulation JSX with <HookSimulationStatus ... /> and pass through isValidating/mutate usage unchanged (leave useTenderlyBundleSimulation/useSimulationData calls in AppliedHookItem and pass needed values down). Ensure prop names match hookDetails.uuid usage and that aria-labels/attributes (e.g., target/rel) remain identical so behavior and tests stay the same.
🤖 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 `@apps/cowswap-frontend/src/legacy/state/application/localWarning.ts`:
- Around line 4-12: The variable warningMsg is implicitly any; update its
declaration to have an explicit type (string | undefined) and optionally
initialize to undefined so it respects noImplicitAny, and ensure the exported
localWarning has the matching type (string | undefined) — locate the warningMsg
declaration and the export of localWarning and add the type annotation to both
places (or initialize warningMsg to undefined) to fix the type error.
In
`@apps/cowswap-frontend/src/modules/bridge/pure/TransactionLink/TransactionLinkDisplay.tsx`:
- Around line 3-4: Reorder the top-level imports so the asset import comes
before the utility import to satisfy the import/order rule: move the import of
iconReceiptSrc from '`@cowprotocol/assets/cow-swap/icon-receipt.svg`' above the
import of getSafeAbsoluteUrl from '`@cowprotocol/common-utils`' (references:
iconReceiptSrc and getSafeAbsoluteUrl) and run the linter to confirm the
violation is resolved.
In `@apps/cowswap-frontend/src/modules/hooksStore/validateHookDappManifest.tsx`:
- Around line 3-4: The imports are out of order causing an import/order lint
failure: swap the two imports so that getChainInfo from
'`@cowprotocol/common-const`' is imported before getSafeAbsoluteUrl from
'`@cowprotocol/common-utils`', preserving module boundaries and existing import
direction constraints; update the import statements that reference getChainInfo
and getSafeAbsoluteUrl accordingly to satisfy the linter.
---
Outside diff comments:
In `@apps/cowswap-frontend/src/modules/hooksStore/pure/AppliedHookItem/index.tsx`:
- Around line 40-135: AppliedHookItem exceeds linted complexity; extract the
simulation-status rendering branch into a new child component to reduce
complexity. Create a new component (e.g., HookSimulationStatus) that accepts
props used by the branch (isBundleSimulationReady, account, simulationData,
safeSimulationUrl, simulationStatus, simulationTooltip) and moves the JSX that
renders styledEl.SimulateContainer / SVG / link / InfoTooltip into that
component; keep the TenderlySimulate / OldSimulateContainer logic in
AppliedHookItem but replace the inlined bundle-simulation JSX with
<HookSimulationStatus ... /> and pass through isValidating/mutate usage
unchanged (leave useTenderlyBundleSimulation/useSimulationData calls in
AppliedHookItem and pass needed values down). Ensure prop names match
hookDetails.uuid usage and that aria-labels/attributes (e.g., target/rel) remain
identical so behavior and tests stay the same.
🪄 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: d9a4de0c-ccfc-4b58-94f1-535fde82a4a0
📒 Files selected for processing (27)
apps/cowswap-frontend/src/common/containers/OrderHooksDetails/HookItem/index.test.tsxapps/cowswap-frontend/src/common/containers/OrderHooksDetails/HookItem/index.tsxapps/cowswap-frontend/src/legacy/state/application/localWarning.tsapps/cowswap-frontend/src/locales/en-US.poapps/cowswap-frontend/src/modules/accountProxy/containers/AccountProxyRecoverPage/index.tsxapps/cowswap-frontend/src/modules/application/containers/AppContainer/CowSpeechBubble/CowSpeechBubbleNotificationBanner.tsxapps/cowswap-frontend/src/modules/bridge/pure/TransactionLink/TransactionLinkDisplay.tsxapps/cowswap-frontend/src/modules/bridge/pure/contents/BridgingProgressContent/index.tsxapps/cowswap-frontend/src/modules/bridge/updaters/PendingBridgeOrdersUpdater.tsxapps/cowswap-frontend/src/modules/hooksStore/pure/AddCustomHookForm/constants.tsxapps/cowswap-frontend/src/modules/hooksStore/pure/AppliedHookItem/index.test.tsxapps/cowswap-frontend/src/modules/hooksStore/pure/AppliedHookItem/index.tsxapps/cowswap-frontend/src/modules/hooksStore/pure/HookDappDetails/index.tsxapps/cowswap-frontend/src/modules/hooksStore/validateHookDappManifest.tsxapps/cowswap-frontend/src/modules/orders/containers/BridgingSuccessNotification/index.tsxapps/cowswap-frontend/src/modules/trade/hooks/setupTradeState/useSetupTradeStateFromUrl.tsapps/cowswap-frontend/src/modules/trade/hooks/useWithRecipient.test.tslibs/common-const/src/ipfs.tslibs/common-utils/src/getExplorerLink.test.tslibs/common-utils/src/getExplorerLink.tslibs/common-utils/src/index.tslibs/common-utils/src/legacyAddressUtils.test.tslibs/common-utils/src/legacyAddressUtils.tslibs/common-utils/src/safeLink.test.tslibs/common-utils/src/safeLink.tslibs/widget-lib/src/IframeSafeSdkBridge.tslibs/widget-lib/src/cowSwapWidget.ts
💤 Files with no reviewable changes (1)
- libs/common-const/src/ipfs.ts
| let warningMsg | ||
|
|
||
| if ((!PINATA_SECRET_API_KEY || !PINATA_API_KEY) && isLocal) { | ||
| if (!PINATA_API_KEY && isLocal) { | ||
| warningMsg = | ||
| "Pinata env vars not set. Order appData upload won't work! " + | ||
| 'Set REACT_APP_PINATA_API_KEY and REACT_APP_PINATA_SECRET_API_KEY' | ||
| "Pinata public env var not set. Order appData upload won't work! " + | ||
| 'Set REACT_APP_PINATA_API_KEY and configure the required server-side Pinata credentials separately.' | ||
| } | ||
|
|
||
| export const localWarning = warningMsg |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify TypeScript strictness and check implicit-any patterns around this file.
fd -i 'tsconfig.*json' -x sh -c 'echo "--- $1"; jq ".compilerOptions | {strict, noImplicitAny}" "$1"' sh {}
rg -n -C2 'let\s+warningMsg\s*$|let\s+\w+\s*$' apps/cowswap-frontend/src/legacy/state/application/localWarning.tsRepository: cowprotocol/cowswap
Length of output: 7535
Avoid implicit any for warningMsg (apps/cowswap-frontend/src/legacy/state/application/localWarning.ts:4)
let warningMsg has no initializer or type annotation, so with noImplicitAny: true it becomes implicitly any. Type it as string | undefined.
Proposed fix
-let warningMsg
+let warningMsg: string | undefined🤖 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/cowswap-frontend/src/legacy/state/application/localWarning.ts` around
lines 4 - 12, The variable warningMsg is implicitly any; update its declaration
to have an explicit type (string | undefined) and optionally initialize to
undefined so it respects noImplicitAny, and ensure the exported localWarning has
the matching type (string | undefined) — locate the warningMsg declaration and
the export of localWarning and add the type annotation to both places (or
initialize warningMsg to undefined) to fix the type error.
There was a problem hiding this comment.
Not applying this one.
let warningMsg here is not a demonstrated noImplicitAny failure in TypeScript; this pattern is accepted as an evolving type in this control flow. Adding : string | undefined would be harmless cleanup, but I am keeping this PR scoped to the actual lint blockers.
There was a problem hiding this comment.
@fairlighteth, understood — TypeScript's control flow analysis infers the evolving string | undefined type correctly here without an explicit annotation, so the existing pattern is fine. Happy to leave it as-is.
🐇 (mild acceptance)
✏️ Learnings added
Learnt from: fairlighteth
Repo: cowprotocol/cowswap PR: 7601
File: apps/cowswap-frontend/src/legacy/state/application/localWarning.ts:4-12
Timestamp: 2026-06-05T11:04:07.197Z
Learning: In `apps/cowswap-frontend/src/legacy/state/application/localWarning.ts`, the `let warningMsg` declaration without an explicit type annotation is intentional. TypeScript's control flow analysis correctly infers the evolving `string | undefined` type, so adding `: string | undefined` is purely cosmetic cleanup and should not be flagged as a required fix or a `noImplicitAny` violation in this file.
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.
🧠 Learnings used
Learnt from: lgahdl
Repo: cowprotocol/cowswap PR: 7394
File: .env.local.example:3-5
Timestamp: 2026-04-24T20:02:41.035Z
Learning: In `cowprotocol/cowswap`, the `.env.local.example` comment for `REACT_APP_SAFE_API_AUTH_TOKEN` intentionally says "required" (not "optional") even though `getSafeApiHeaders()` treats it as optional at runtime. The rationale is that unauthenticated calls to `api.safe.global` still work but are subject to monthly quota limits (429 Too Many Requests), so the strong wording is meant to proactively encourage developers to obtain and set a token. Do not flag this comment wording as misleading.
Learnt from: limitofzero
Repo: cowprotocol/cowswap PR: 7504
File: .github/workflows/vercel.yml:86-86
Timestamp: 2026-05-13T18:35:50.478Z
Learning: In the cowprotocol/cowswap frontend (pure SPA), do not treat `REACT_APP_*` environment variables (e.g., `REACT_APP_SAFE_API_AUTH_TOKEN`) as a security vulnerability by itself just because they are included in the built frontend bundle. In CRA-style setups, `REACT_APP_*` values are injected at build time into the client bundle, and this repository has no server-side execution layer that could safely keep these values secret. Only flag an issue if there is additional evidence of unintended privilege exposure or missing authorization/validation beyond the expected build-time injection of `REACT_APP_*` tokens.
Learnt from: CR
Repo: cowprotocol/cowswap PR: 0
File: apps/cowswap-frontend/AGENTS.md:0-0
Timestamp: 2026-05-22T12:57:32.776Z
Learning: Applies to apps/cowswap-frontend/**/*.{ts,tsx} : Use explicit return types where they improve safety/readability. Keep local trivial internals inferred; keep exported APIs aligned with root `AGENTS.md` requirements
Learnt from: crutch12
Repo: cowprotocol/cowswap PR: 6969
File: apps/cowswap-frontend/src/lib/localeMessages.ts:12-20
Timestamp: 2026-02-04T18:23:26.408Z
Learning: In apps/cowswap-frontend/src/lib/localeMessages.ts, the loadActiveLocaleMessages() function runs before initial render (in initApp()), at which point feature flags like isInternationalizationEnabled are not yet available. This early locale loading is intentional to avoid white screens and extra rerenders.
Learnt from: fairlighteth
Repo: cowprotocol/cowswap PR: 7044
File: apps/cowswap-frontend/src/modules/wallet/utils/getWalletConnectionErrorMessage.ts:25-37
Timestamp: 2026-04-07T15:30:41.352Z
Learning: In `apps/cowswap-frontend/src/modules/wallet/utils/getWalletConnectionErrorMessage.ts`, the `JSON.stringify` fallback inside `getNormalizedWalletConnectionErrorMessage` is intentional. The function is only invoked when `getProviderErrorMessage()` does not return a string, so structured/non-string message payloads are already handled upstream. The `JSON.stringify` call is kept to preserve diagnostic detail for truly unknown error objects, rather than collapsing them to `[object Object]` or a generic string. Do not flag this as a leakage/noise risk.
Learnt from: CR
Repo: cowprotocol/cowswap PR: 0
File: apps/cowswap-frontend/AGENTS.md:0-0
Timestamp: 2026-05-22T12:57:32.776Z
Learning: Applies to apps/cowswap-frontend/**/modules/**/index.ts : Public module API must be re-exported via the module `index.ts`
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6009
File: apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/HighFeeWarningTooltipContent.tsx:23-33
Timestamp: 2025-07-24T16:42:53.154Z
Learning: In apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/HighFeeWarningTooltipContent.tsx, the use of toFixed(2) for percentage formatting in tooltip content is intentional and differs from the banner message formatting that uses toSignificant(2, undefined, Rounding.ROUND_DOWN). This formatting difference serves different UX purposes and should not be flagged as inconsistent.
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 7216
File: libs/common-utils/src/environments.ts:45-45
Timestamp: 2026-03-24T15:01:01.995Z
Learning: In `libs/common-utils/src/environments.ts`, the `forceProdApi` and `forceStagingApi` localStorage keys are intentionally unversioned. They are internal developer/hack overrides (not user-facing keys) and are exempt from the `camelCaseBase:v{number}` versioning convention that applies to regular user-facing or data-shape-sensitive localStorage keys.
Learnt from: CR
Repo: cowprotocol/cowswap PR: 0
File: apps/cowswap-frontend/AGENTS.md:0-0
Timestamp: 2026-05-22T12:57:32.776Z
Learning: Applies to apps/cowswap-frontend/**/*.{ts,tsx} : Prefer `unknown` to `any`
Learnt from: CR
Repo: cowprotocol/cowswap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-26T08:28:03.489Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : MUST NOT use `any` or non-null assertions (`!`) in production code
Learnt from: CR
Repo: cowprotocol/cowswap PR: 0
File: apps/cowswap-frontend/AGENTS.md:0-0
Timestamp: 2026-05-22T12:57:32.776Z
Learning: Applies to apps/cowswap-frontend/**/*.{ts,tsx} : Prefer `as const`, `satisfies`, and `as const satisfies` where they improve literal safety
Learnt from: CR
Repo: cowprotocol/cowswap PR: 0
File: apps/cowswap-frontend/AGENTS.md:0-0
Timestamp: 2026-05-22T12:57:32.776Z
Learning: Applies to apps/cowswap-frontend/**/*.{ts,tsx} : Avoid generic type names like `Props`/`Options`; use specific names (for example, `TradeWidgetContainerProps`)
Learnt from: kernelwhisperer
Repo: cowprotocol/cowswap PR: 7228
File: libs/widget-lib/src/types.ts:447-448
Timestamp: 2026-04-01T21:32:33.975Z
Learning: In `libs/widget-lib/src/types.ts` (cowprotocol/cowswap PR `#7228`), `WidgetHookPayload<T>` intentionally remains a generic interface rather than a discriminated union over `WidgetHookEvents`. The team considered converting it to a distributive conditional type but found it too complex; correctness of event/payload pairings is enforced via unit tests instead of TypeScript. Do not flag `WidgetHookPayload<WidgetHookEvents>` or the `[WidgetMethodsEmit.PROCESS_HOOK]` mapping as a type-safety issue in this repository.
Learnt from: Danziger
Repo: cowprotocol/cowswap PR: 7153
File: apps/cowswap-frontend/src/modules/advancedOrders/pure/Settings/AdvancedOrdersSettings.tsx:25-25
Timestamp: 2026-03-16T16:33:33.440Z
Learning: In `apps/cowswap-frontend/src/modules/advancedOrders/pure/Settings/AdvancedOrdersSettings.tsx`, the comment `// TODO: we should use limit orders settings in Advanced Orders!` (line 25) is a pre-existing TODO that predates PR `#7153`. It is intentionally deferred and should not be flagged as a blocker in reviews of that PR or subsequent PRs unless there is a dedicated effort to address it.
Learnt from: CR
Repo: cowprotocol/cowswap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-26T08:28:03.489Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Avoid introducing new `common/** -> modules/**` imports; treat existing cases as legacy debt and track cleanup in `docs/QUALITY.md`
Learnt from: Danziger
Repo: cowprotocol/cowswap PR: 6997
File: apps/cowswap-frontend/src/common/state/fetchTokens.utils.ts:37-49
Timestamp: 2026-03-20T18:17:17.482Z
Learning: In `apps/cowswap-frontend/src/common/state/fetchTokens.utils.ts`, the use of `Promise.all` (instead of `Promise.allSettled`) for batching `fetchTokenFromBlockchain` calls in `fetchTokens` is intentional. The app cannot function correctly when any tokens are missing, so failing the entire batch on a single rejection is the desired fail-fast behavior. Do not flag this as a missing error-handling issue.
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 7581
File: libs/wallet/src/wagmi/Web3Provider.tsx:160-174
Timestamp: 2026-06-04T07:39:09.775Z
Learning: In cowprotocol/cowswap (`libs/wallet/src/wagmi`), `SafeConnectionHandler` contains an internal early-return guard: if the currently connected connector is `COW_WIDGET_CONNECTOR_ID` (the widget connector), `SafeConnectionHandler` skips its Safe auto-connection/reconnection logic entirely. Therefore, `Web3Provider` does not need to gate `SafeConnectionHandler` on `standaloneMode` — even in dappMode, where the widget connector is active, `SafeConnectionHandler` will not override it. Do not flag the unused `standaloneMode` prop in `Web3ProviderProps` as a bug; the defence is implemented inside `SafeConnectionHandler`.
- restore import ordering in bridge and hook manifest modules - extract hook simulation status rendering to reduce component complexity
|
Addressed the CodeRabbit outside-diff complexity note in I extracted the bundle simulation status rendering out of |
Summary
This PR hardens a set of frontend trust boundaries in
cowswap-frontendand shared frontend/widget libraries.Main changes:
recipientAddressURL paramsThis addresses the remaining DeepSec
HIGHfindings that are in scope for frontend and widget code, excludingcow-fiand excluding the larger deferred items listed below.To Test
http://localhost:...andhttp://[::1]:...explorer URLs still work when explicitly configured.recipientAddress=0x...in the URL does not enable recipient mode.postMessagetraffic from other windows/origins is ignored.Automated coverage added in this PR:
recipientAddressis presentgit diff --checkpassed locally.Background
Deferred
HIGHfindings intentionally left out of this PR because they need larger product/flow changes:apps/cowswap-frontend/src/modules/hooksStore/containers/HookDappContainer/index.tsxapps/cowswap-frontend/src/modules/twap/services/fetchTwapOrdersFromSafe.tsMultiSendinner calls instead of selector substring scanningNotes:
recipient=<value>remains the intended visible custom-recipient param.recipientAddressparam.