Conversation
WalkthroughThe changes in Changes
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
lib/assets_context.js (1)
Line range hint
44-47: Enhance crossorigin attribute handling for better compatibility.The current implementation only adds the
crossoriginattribute without its value. For better compatibility and security, it should support the standard values 'anonymous' and 'use-credentials'.- return '<script' + (crossorigin ? ' crossorigin' : '') + ` src="${url}"></script>`; + const crossAttr = crossorigin === true ? ' crossorigin="anonymous"' : + (crossorigin ? ` crossorigin="${crossorigin}"` : ''); + return '<script' + crossAttr + ` src="${url}"></script>`;
🧹 Nitpick comments (1)
lib/assets_context.js (1)
Line range hint
15-15: Add validation for crossorigin configuration.The
crossoriginattribute can be a boolean or accept specific string values ('anonymous' or 'use-credentials'). Consider adding validation to ensure the configured value is valid.this.config = ctx.app.config.assets; + // Validate crossorigin value + if (this.config.crossorigin !== undefined && + this.config.crossorigin !== true && + this.config.crossorigin !== 'anonymous' && + this.config.crossorigin !== 'use-credentials') { + throw new Error('Invalid crossorigin value. Must be true, "anonymous", or "use-credentials"'); + } this.crossorigin = this.config.crossorigin;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/assets_context.js(1 hunks)
🔇 Additional comments (1)
lib/assets_context.js (1)
Line range hint
58-72: Consider URL sanitization for security.While the
getURLmethod validates URL format, it doesn't sanitize the URLs. When using thecrossoriginattribute with external resources, it's important to ensure URLs are properly sanitized to prevent XSS attacks.Consider adding URL sanitization:
if (urlpath.startsWith('http://') || urlpath.startsWith('https://')) { + // Sanitize external URLs + try { + const url = new URL(urlpath); + urlpath = url.toString(); + } catch (e) { + throw new Error(`Invalid URL: ${urlpath}`); + } return urlpath; }Let's verify the current URL handling in the codebase:
|
Nice feature, can you add a unit test for this change? |
Wait a moment, I'll add it. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/assets.test.js (1)
557-558: Enhance test coverage for crossorigin attribute.While the basic functionality is tested, consider adding the following test cases for more comprehensive coverage:
- Verify the actual value of the crossorigin attribute
- Test when crossorigin is disabled/not configured
- Test with different crossorigin values (e.g., 'anonymous', 'use-credentials')
const style = ctx.helper.assets.getStyle(); -assert(style.includes('crossorigin')); -assert(script.includes('crossorigin')); +assert(style.includes('crossorigin="anonymous"'), 'Style should have crossorigin="anonymous"'); +assert(script.includes('crossorigin="anonymous"'), 'Script should have crossorigin="anonymous"'); + +// Test when crossorigin is disabled +mock(app.config.assets, 'crossorigin', false); +const styleWithoutCrossorigin = ctx.helper.assets.getStyle(); +const scriptWithoutCrossorigin = ctx.helper.assets.getScript(); +assert(!styleWithoutCrossorigin.includes('crossorigin'), 'Style should not have crossorigin when disabled'); +assert(!scriptWithoutCrossorigin.includes('crossorigin'), 'Script should not have crossorigin when disabled'); + +// Test with use-credentials +mock(app.config.assets, 'crossorigin', 'use-credentials'); +const styleWithCredentials = ctx.helper.assets.getStyle(); +const scriptWithCredentials = ctx.helper.assets.getScript(); +assert(styleWithCredentials.includes('crossorigin="use-credentials"'), 'Style should have crossorigin="use-credentials"'); +assert(scriptWithCredentials.includes('crossorigin="use-credentials"'), 'Script should have crossorigin="use-credentials"');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/assets_context.js(2 hunks)test/assets.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/assets_context.js
🔇 Additional comments (1)
test/assets.test.js (1)
Line range hint
546-562: Verify test configuration in the fixture app.The test relies on configuration from the 'apps/crossorigin' fixture. Let's verify its setup:
✅ Verification successful
Test configuration in the crossorigin fixture app is properly set up ✅
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the configuration in the crossorigin fixture app echo "Checking config.default.js for crossorigin configuration..." cat test/fixtures/apps/crossorigin/config/config.default.js echo -e "\nChecking package.json for dependencies..." cat test/fixtures/apps/crossorigin/package.jsonLength of output: 694
|
@fengmk2 PTAL |
[skip ci] ## [1.10.0](v1.9.0...v1.10.0) (2025-01-23) ### Features * css support crossorigin ([#50](#50)) ([bbb2794](bbb2794))
2 participants
Support cross-origin for style
Summary by CodeRabbit
crossoriginattribute for external resources.crossoriginattribute in generated HTML for styles and scripts.