fix: add try-catch for window.top access to avoid cross-origin error #488
Conversation
|
@three-water666 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
概述修复了在跨域 iframe（如 CodeSandbox 沙盒环境）中访问 window.top 时触发 SecurityError 的问题。通过为 window.top 事件监听器的移除操作添加 try-catch 防护，阻止组件卸载时产生崩溃，同时保留原有的清理逻辑。新增测试验证了该场景下的容错行为。 变更详情
代码审查工作量评估🎯 2 (简单) | ⏱️ ~12 分钟 需要关注的点：
诗
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Summary of ChangesHello @three-water666, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical "SecurityError" that occurs when the "Image" component attempts to clean up event listeners from "window.top" in cross-origin sandbox environments. By wrapping the "window.top" access in a "try-catch" block, the component can now gracefully handle security restrictions during unmounting, preventing application crashes and improving stability in diverse deployment contexts. A new test has been added to validate this behavior. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a SecurityError that occurs in sandboxed environments by wrapping access to window.top within a try-catch block. The change is simple and effective. The newly added test case in tests/crossOrigin.test.tsx properly simulates the cross-origin error and verifies that the component unmounts without crashing. I have one suggestion to improve the test's robustness by ensuring the cleanup logic is always executed, which is a best practice for test isolation.
| // Try to define a property that throws when accessed | ||
| // Note: In some jsdom environments window.top might be non-configurable. | ||
| // If this fails, we might need a different strategy, but this is the standard way to mock cross-origin. | ||
| try { | ||
| Object.defineProperty(window, 'top', { | ||
| get: () => { | ||
| throw new DOMException('Permission denied to access property "removeEventListener" on cross-origin object', 'SecurityError'); | ||
| }, | ||
| configurable: true, | ||
| }); | ||
| } catch (e) { | ||
| // Fallback: If we can't mock window.top, we skip this specific test implementation details | ||
| // and rely on the fact that we modified the source code. | ||
| // But let's try to verify if we can mock it. | ||
| console.warn('Could not mock window.top:', e); | ||
| return; | ||
| } | ||
|
|
||
| const { container, unmount } = render( | ||
| <Image | ||
| src="https://zos.alipayobjects.com/rmsportal/jkjgkEfvpUPVyRjUImniVslZfWPnJuuZ.png" | ||
| />, | ||
| ); | ||
|
|
||
| // Open preview to trigger the effect that binds events | ||
| fireEvent.click(container.querySelector('.rc-image')); | ||
| act(() => { | ||
| jest.runAllTimers(); | ||
| }); | ||
|
|
||
| // Unmount should trigger the cleanup function where the crash happened | ||
| expect(() => { | ||
| unmount(); | ||
| }).not.toThrow(); | ||
|
|
||
| // Restore window.top | ||
| Object.defineProperty(window, 'top', { | ||
| value: originalTop, | ||
| configurable: true, | ||
| }); |
There was a problem hiding this comment.
To ensure that window.top is restored even if an assertion within the test fails, it's a best practice to place the cleanup logic in a finally block. This guarantees that the original state is restored, improving test isolation and preventing potential side effects on other tests.
try {
// Try to define a property that throws when accessed
// Note: In some jsdom environments window.top might be non-configurable.
// If this fails, we might need a different strategy, but this is the standard way to mock cross-origin.
try {
Object.defineProperty(window, 'top', {
get: () => {
throw new DOMException('Permission denied to access property "removeEventListener" on cross-origin object', 'SecurityError');
},
configurable: true,
});
} catch (e) {
// Fallback: If we can't mock window.top, we skip this specific test implementation details
// and rely on the fact that we modified the source code.
// But let's try to verify if we can mock it.
console.warn('Could not mock window.top:', e);
return;
}
const { container, unmount } = render(
<Image
src="https://zos.alipayobjects.com/rmsportal/jkjgkEfvpUPVyRjUImniVslZfWPnJuuZ.png"
/>,
);
// Open preview to trigger the effect that binds events
fireEvent.click(container.querySelector('.rc-image'));
act(() => {
jest.runAllTimers();
});
// Unmount should trigger the cleanup function where the crash happened
expect(() => {
unmount();
}).not.toThrow();
} finally {
// Restore window.top
Object.defineProperty(window, 'top', {
value: originalTop,
configurable: true,
});
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/hooks/useMouseEvent.ts (1)
121-127: 卸载阶段对 window.top 的防御性清理已解决跨域崩溃问题，可细化异常处理与注释这里用
try { window.top?.removeEventListener(...) } catch {}可以有效防止 iframe 跨域环境下的SecurityError把卸载流程搞崩，和 PR 目标高度契合。不过有两个小点可以考虑优化：
catch (error)现在完全静默，会把所有类型的异常都吞掉，包括可能的真实逻辑错误。可以考虑只对跨域场景“特赦”，例如判断error instanceof DOMException && error.name === 'SecurityError'时才静默，其余情况用warning(false, ...)记一条日志，排查会更容易。- 卸载时无条件尝试从
window.top移除监听，而挂载时只有在window.top !== window.self时才注册。虽然removeEventListener是幂等的，这么做在行为上是安全的，但建议在try上方补一句注释说明“删除监听是幂等的，即使没注册也安全”，后续维护者更容易理解这里的意图。tests/crossOrigin.test.tsx (1)
15-58: window.top mock 的测试场景覆盖到位，但可以提升健壮性与信号质量整体思路很好：通过
Object.defineProperty(window, 'top', { get: () => { throw new DOMException(...) } })人为制造跨域访问异常，再验证unmount()不再抛错，和源码修改高度对齐。不过有两点小建议：
- 行 22-35：如果
Object.defineProperty抛错（例如某些环境里window.top不可配置），当前做法是console.warn后直接return，这个it会被标记为“通过”，但实际上没有真正验证跨域场景，相当于潜在“假绿”。更稳妥的做法是：要么让测试在这种环境下显式失败（方便及早发现 CI 环境不支持该用例），要么在注释中明确说明这里接受“无法模拟则放弃校验”的权衡，以免后续读代码时误解测试的有效性。- 行 49-58：
unmount()包在expect(() => { unmount(); }).not.toThrow()里，如果未来又出现回归导致这里抛异常，后续恢复window.top的代码不会执行，可能污染后续测试环境。可以考虑把“记录原始window.top+ mock + 还原”这套逻辑用try/finally包起来，确保无论断言是否失败都能恢复全局状态。这些都是测试层面的健壮性优化，不影响当前修复的正确性，可以视时间情况择机调整。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #488 +/- ##
==========================================
- Coverage 99.41% 99.41% -0.01%
==========================================
Files 17 17
Lines 511 509 -2
Branches 152 152
==========================================
- Hits 508 506 -2
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Description
This PR fixes a
SecurityErrorthat occurs when using the Image component in sandbox environments like CodeSandbox or StackBlitz.Related Issue
Fixes ant-design/ant-design#56111
Root Cause
When the component unmounts, it attempts to remove event listeners from
window.top. In sandbox environments where the preview runs inside an iframe, accessingwindow.top(which is cross-origin) triggers a browser security restriction.Solution
Wrap the
window.topaccess in atry-catchblock to safely handle the cross-origin error. This ensures the component cleans up gracefully without crashing the application.Test
tests/crossOrigin.test.tsxto simulate the cross-origin access scenario and verify that the unmount process no longer throws an error.Summary by CodeRabbit
发布说明
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.