fix(milky): 放宽 Content-Type 校验逻辑以支持 charset 参数 #702
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
评审者指南（在小型 PR 上折叠）评审者指南放宽 Milky HTTP access-token 中间件中的 Content-Type 校验逻辑，使带有额外参数的 JSON 请求（例如 Access token 中间件中放宽 JSON Content-Type 校验的时序图sequenceDiagram
actor Client
participant MilkyHttpHandler
participant AccessTokenMiddleware
participant ApiHandler
Client->>MilkyHttpHandler: HTTP request to /prefix/api
activate MilkyHttpHandler
MilkyHttpHandler->>AccessTokenMiddleware: Invoke middleware
activate AccessTokenMiddleware
AccessTokenMiddleware->>AccessTokenMiddleware: Read req.headers[content-type]
AccessTokenMiddleware->>AccessTokenMiddleware: includes application/json ?
alt ContentType_includes_application_json
AccessTokenMiddleware-->>MilkyHttpHandler: next()
MilkyHttpHandler->>ApiHandler: Handle API request
activate ApiHandler
ApiHandler-->>Client: 2xx/4xx/5xx response
deactivate ApiHandler
else ContentType_missing_or_not_json
AccessTokenMiddleware-->>Client: 415 Unsupported Media Type
deactivate MilkyHttpHandler
end
deactivate AccessTokenMiddleware
MilkyHttpHandler access token 中间件 Content-Type 检查的类图classDiagram
class MilkyHttpHandler {
-app
-config
-ctx
+registerApiAccessTokenMiddleware()
}
class ApiAccessTokenMiddleware {
+handle(req,res,next)
+isJsonContentType(contentType)
}
MilkyHttpHandler --> ApiAccessTokenMiddleware: uses
ApiAccessTokenMiddleware : isJsonContentType(contentType) checks includes application/json
文件级变更
技巧和命令与 Sourcery 交互
自定义你的体验访问你的 控制面板 以：
获取帮助Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideRelaxes the Content-Type validation in the Milky HTTP access-token middleware so that JSON requests with additional parameters like charset (e.g., Sequence diagram for relaxed JSON Content-Type validation in access token middlewaresequenceDiagram
actor Client
participant MilkyHttpHandler
participant AccessTokenMiddleware
participant ApiHandler
Client->>MilkyHttpHandler: HTTP request to /prefix/api
activate MilkyHttpHandler
MilkyHttpHandler->>AccessTokenMiddleware: Invoke middleware
activate AccessTokenMiddleware
AccessTokenMiddleware->>AccessTokenMiddleware: Read req.headers[content-type]
AccessTokenMiddleware->>AccessTokenMiddleware: includes application/json ?
alt ContentType_includes_application_json
AccessTokenMiddleware-->>MilkyHttpHandler: next()
MilkyHttpHandler->>ApiHandler: Handle API request
activate ApiHandler
ApiHandler-->>Client: 2xx/4xx/5xx response
deactivate ApiHandler
else ContentType_missing_or_not_json
AccessTokenMiddleware-->>Client: 415 Unsupported Media Type
deactivate MilkyHttpHandler
end
deactivate AccessTokenMiddleware
Class diagram for MilkyHttpHandler access token middleware Content-Type checkclassDiagram
class MilkyHttpHandler {
-app
-config
-ctx
+registerApiAccessTokenMiddleware()
}
class ApiAccessTokenMiddleware {
+handle(req,res,next)
+isJsonContentType(contentType)
}
MilkyHttpHandler --> ApiAccessTokenMiddleware: uses
ApiAccessTokenMiddleware : isJsonContentType(contentType) checks includes application/json
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些高层次的反馈：
- 使用
includes('application/json')可能会匹配到非预期的值（例如application/jsonfoo）；建议使用更严格的检查方式，比如解析媒体类型（例如使用content-type包），或者在去掉参数后使用startsWith('application/json')进行检查。 - 日志消息仍然写着
Content-Type not application/json，即使现在已经允许 charset 变体；建议更新日志文案以反映放宽后的匹配规则，或者直接记录实际的Content-Type值以提高可读性。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- Using `includes('application/json')` could match unintended values (e.g. `application/jsonfoo`); consider using a stricter check like parsing the media type (e.g. via the `content-type` package) or checking `startsWith('application/json')` after splitting off parameters.
- The log message still says `Content-Type not application/json` even though charset variants are now allowed; consider updating the message to reflect the relaxed matching or log the actual `Content-Type` value for clarity.
## Individual Comments
### Comment 1
<location> `src/milky/network/http.ts:48` </location>
<code_context>
if (this.config.accessToken) {
this.app.use(`${this.config.prefix}/api`, (req, res, next) => {
- if (req.headers['content-type'] !== 'application/json') {
+ if (!req.headers['content-type']?.includes('application/json')) {
this.ctx.logger.warn(
'MilkyHttp',
</code_context>
<issue_to_address>
**suggestion:** Consider making the Content-Type check case-insensitive and robust to array values.
`req.headers['content-type']` can be `string | string[] | undefined`, and header names/values may vary in case. Normalizing and flattening first avoids subtle mismatches, e.g.:
```ts
const contentType = req.headers['content-type'];
const contentTypeStr = Array.isArray(contentType) ? contentType.join(',') : contentType || '';
if (!contentTypeStr.toLowerCase().includes('application/json')) {
// ...
}
```
This will handle values like `application/json; charset=utf-8`, mixed casing, and multi-valued headers correctly.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- Using
includes('application/json')could match unintended values (e.g.application/jsonfoo); consider using a stricter check like parsing the media type (e.g. via thecontent-typepackage) or checkingstartsWith('application/json')after splitting off parameters. - The log message still says
Content-Type not application/jsoneven though charset variants are now allowed; consider updating the message to reflect the relaxed matching or log the actualContent-Typevalue for clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `includes('application/json')` could match unintended values (e.g. `application/jsonfoo`); consider using a stricter check like parsing the media type (e.g. via the `content-type` package) or checking `startsWith('application/json')` after splitting off parameters.
- The log message still says `Content-Type not application/json` even though charset variants are now allowed; consider updating the message to reflect the relaxed matching or log the actual `Content-Type` value for clarity.
## Individual Comments
### Comment 1
<location> `src/milky/network/http.ts:48` </location>
<code_context>
if (this.config.accessToken) {
this.app.use(`${this.config.prefix}/api`, (req, res, next) => {
- if (req.headers['content-type'] !== 'application/json') {
+ if (!req.headers['content-type']?.includes('application/json')) {
this.ctx.logger.warn(
'MilkyHttp',
</code_context>
<issue_to_address>
**suggestion:** Consider making the Content-Type check case-insensitive and robust to array values.
`req.headers['content-type']` can be `string | string[] | undefined`, and header names/values may vary in case. Normalizing and flattening first avoids subtle mismatches, e.g.:
```ts
const contentType = req.headers['content-type'];
const contentTypeStr = Array.isArray(contentType) ? contentType.join(',') : contentType || '';
if (!contentTypeStr.toLowerCase().includes('application/json')) {
// ...
}
```
This will handle values like `application/json; charset=utf-8`, mixed casing, and multi-valued headers correctly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (this.config.accessToken) { | ||
| this.app.use(`${this.config.prefix}/api`, (req, res, next) => { | ||
| if (req.headers['content-type'] !== 'application/json') { | ||
| if (!req.headers['content-type']?.includes('application/json')) { |
Contributor
There was a problem hiding this comment.
suggestion: 建议让 Content-Type 的检查对大小写不敏感，并且能正确处理数组形式的值。
req.headers['content-type'] 可能是 string | string[] | undefined，并且头名/头值的大小写都可能不同。先统一大小写并将其拍平成字符串，可以避免一些细微的不匹配，例如：
const contentType = req.headers['content-type'];
const contentTypeStr = Array.isArray(contentType) ? contentType.join(',') : contentType || '';
if (!contentTypeStr.toLowerCase().includes('application/json')) {
// ...
}这样可以正确处理诸如 application/json; charset=utf-8、大小写混合以及多值 header 等情况。
Original comment in English
suggestion: Consider making the Content-Type check case-insensitive and robust to array values.
req.headers['content-type'] can be string | string[] | undefined, and header names/values may vary in case. Normalizing and flattening first avoids subtle mismatches, e.g.:
const contentType = req.headers['content-type'];
const contentTypeStr = Array.isArray(contentType) ? contentType.join(',') : contentType || '';
if (!contentTypeStr.toLowerCase().includes('application/json')) {
// ...
}This will handle values like application/json; charset=utf-8, mixed casing, and multi-valued headers correctly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
问题描述
当前服务端的中间件在校验 Content-Type 时使用了严格全等判断 (!==)：
导致的问题是，一些 HTTP 客户端（如 .NET 的 HttpClient）在发送请求时会自动附加编码信息，将头设置为 application/json; charset=utf-8，从而导致服务端判定为不匹配返回 415
修改内容
改成了包含判断
Summary by Sourcery
Bug Fixes:
application/json; charset=utf-8这种 Content-Type 头的 API 请求通过校验，而不是返回 415 错误。Original summary in English
Summary by Sourcery
Bug Fixes: