fix(): support serve app images #4826
Conversation
Walkthrough添加图片服务中间件。在现有中间件配置中引入新的 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
next-core
|
||||||||||||||||||||||||||||
| Project |
next-core
|
| Branch Review |
steve/v3-serve-app-images
|
| Run status |
|
| Run duration | 00m 24s |
| Commit |
|
| Committer | Shenwei Wang |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
17
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Pull request overview
This PR adds support for serving app images in development mode by introducing a new middleware function serveAppImages that handles image requests for standalone apps.
- Adds a new middleware
serveAppImagesthat serves image files from local app directories - Registers two image serving paths for each app in the development server
- Reorganizes imports in
getMiddlewares.jsto place external packages before local imports
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/brick-container/serve/middlewares/serveAppImages.js | New middleware function that serves app images from mock-micro-apps and apps directories for local development |
| packages/brick-container/serve/middlewares/getMiddlewares.js | Adds two image serving routes per app and reorganizes imports to follow convention of external packages first |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export default function serveAppImages({ rootDir }, appId) { | ||
| /** | ||
| * @param {import("express").Request} req | ||
| * @param {import("express").Response} res |
There was a problem hiding this comment.
The JSDoc comment is missing the next parameter. The returned async function takes three parameters (req, res, next), but the JSDoc only documents two. This should be:
/**
* @param {import("express").Request} req
* @param {import("express").Response} res
* @param {import("express").NextFunction} next
*/| * @param {import("express").Response} res | |
| * @param {import("express").Response} res | |
| * @param {import("express").NextFunction} next |
![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
🧹 Nitpick comments (3)
packages/brick-container/serve/middlewares/getMiddlewares.js (1)
111-114: 考虑添加注释说明两种路径模式的用途。这里注册了两个不同的 URL 模式（
/-/images和/-/micro-apps/${appId}/images），但它们都指向相同的图片源。建议添加注释说明为什么需要支持这两种模式（例如向后兼容性或不同的部署场景）。packages/brick-container/serve/middlewares/serveAppImages.js (2)
4-4: 建议添加参数验证。函数没有验证
rootDir和appId参数。建议添加基本的验证以防止运行时错误，例如检查rootDir是否存在，以及appId是否包含非法字符（如路径遍历字符）。可以考虑添加如下验证:
export default function serveAppImages({ rootDir }, appId) { + if (!rootDir) { + throw new Error("rootDir is required for serveAppImages"); + } + if (!appId || /[./\\]/.test(appId)) { + throw new Error("Invalid appId for serveAppImages"); + } + /** * @param {import("express").Request} req * @param {import("express").Response} res
9-13:async关键字非必需。中间件函数声明为
async但内部没有使用await。虽然不影响功能，但可以移除async关键字以保持代码简洁。- return async function (req, res, next) { + return function (req, res, next) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
packages/brick-container/serve/middlewares/getMiddlewares.js(2 hunks)packages/brick-container/serve/middlewares/serveAppImages.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/brick-container/serve/middlewares/getMiddlewares.js (3)
packages/brick-container/build.config.js (1)
baseHref(15-15)packages/brick-container/serve/getProxy.js (1)
appId(506-506)packages/brick-container/serve/middlewares/serveAppImages.js (1)
serveAppImages(4-24)
packages/brick-container/serve/middlewares/serveAppImages.js (2)
packages/brick-container/serve/getProxy.js (1)
appId(506-506)packages/serve-helpers/src/tryFiles.js (1)
tryServeFiles(22-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: build (20.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/brick-container/serve/middlewares/getMiddlewares.js (2)
2-2: 导入语句正确。新增的
serveAppImages导入与现有的导入风格一致。Also applies to: 8-8
106-109: 原评论有误 - 代码实际上是正确的。
env对象由getEnv()函数返回，其中明确包含了rootDir属性（serve/env.js 第 135 行）。serveAppImages()能正确解构该属性，调用处 getMiddlewares.js 第 108 和 113 行无需任何修改。Likely an incorrect or invalid review comment.
packages/brick-container/serve/middlewares/serveAppImages.js (1)
1-2: 导入语句正确。所需的依赖已正确导入。
| tryServeFiles( | ||
| ["mock-micro-apps", "apps"].map((folder) => | ||
| path.join(rootDir, folder, appId, "dist/images", req.path) | ||
| ), | ||
| req, | ||
| res, | ||
| next | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
验证路径遍历安全性。
req.path 直接用于构建文件路径。虽然 path.join() 会规范化路径，且 Express 的 res.sendFile() 提供了一定的保护，但建议验证 tryServeFiles 和底层文件服务逻辑是否充分防止了路径遍历攻击（例如 ../../ 序列）。
运行以下脚本检查 tryServeFiles 的实现细节:
🏁 Script executed:
#!/bin/bash
# 查看 tryServeFiles 的完整实现以确认安全性
ast-grep --pattern 'export function tryServeFiles($$$) {
$$$
}'
# 查找是否有路径验证逻辑
rg -nP '(normalize|resolve|relative|\.\./)' packages/serve-helpers/src/ --type=jsLength of output: 1492
Now I need to check the tryFiles function implementation to see if there's any path validation:
验证路径遍历安全性 - 确认存在安全漏洞
您的原始评论准确指出了一个实际存在的安全问题。经验证，代码确实存在路径遍历风险：
虽然 Express 的 res.sendFile() 函数本身并非固有脆弱，但当用户输入用于确定文件路径时，它可能成为目录遍历攻击的来源。
关键问题是 path.normalize 并非独立的安全解决方案——它可能移除某些冗余路径元素，但不能防止遍历攻击。在此代码中，req.path 直接来自 HTTP 请求，且 path.join() 虽然规范化路径，但 攻击者仍可使用 "../" 访问根目录。
建议修复方案： 将规范化路径与硬编码或配置的可信根目录进行比较，若比较失败则表示检测到路径遍历，应拒绝请求。应在 tryServeFiles 前或内部添加验证，确保最终解析路径以受信基路径开头。
依赖检查
组件之间的依赖声明，是微服务组件架构下的重要信息，请确保其正确性。
请勾选以下两组选项其中之一：
或者：
提交信息检查
Git 提交信息将决定包的版本发布及自动生成的 CHANGELOG，请检查工作内容与提交信息是否相符，并在以下每组选项中都依次确认。
破坏性变更：
feat作为提交类型。BREAKING CHANGE: 你的变更说明。新特性：
feat作为提交类型。问题修复：
fix作为提交类型。杂项工作：
即所有对下游使用者无任何影响、且没有必要显示在 CHANGELOG 中的改动，例如修改注释、测试用例、开发文档等：
chore,docs,test等作为提交类型。Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.