fix: enhance WebSocket access token validation method#654
Merged
Conversation
Contributor
审阅者指南（在小型 PR 上默认折叠）审阅者指南更新 WebSocket 访问令牌验证逻辑，在原先支持 更新后的 WebSocket 访问令牌验证时序图sequenceDiagram
actor Client
participant MilkyHttpHandler
participant URLParser as URL
participant Logger
Client->>MilkyHttpHandler: WebSocket HTTP upgrade request
activate MilkyHttpHandler
MilkyHttpHandler->>URLParser: new URL(req.url, host)
URLParser-->>MilkyHttpHandler: url
MilkyHttpHandler->>MilkyHttpHandler: read req.headers.authorization
alt Authorization header present
MilkyHttpHandler->>MilkyHttpHandler: inputToken = authHeader.split(Bearer ).pop()
MilkyHttpHandler->>Logger: info(receive ws header token, inputToken)
else Authorization header missing
MilkyHttpHandler->>URLParser: url.searchParams.get(access_token)
URLParser-->>MilkyHttpHandler: inputToken or null
MilkyHttpHandler->>MilkyHttpHandler: inputToken = value or empty string
end
MilkyHttpHandler->>MilkyHttpHandler: compare inputToken with config.accessToken
alt Token invalid or missing
MilkyHttpHandler->>Logger: warn(MilkyHttp, remote -> /event (Credentials invalid))
MilkyHttpHandler-->>Client: Reject WebSocket connection
else Token valid
MilkyHttpHandler-->>Client: Proceed with WebSocket upgrade
end
deactivate MilkyHttpHandler
更新后 MilkyHttpHandler WebSocket 令牌验证的类图classDiagram
class MilkyHttpHandler {
- config
- ctx
handleRequest(req, res)
}
class Config {
+ accessToken : string
}
class Context {
+ logger : Logger
}
class Logger {
+ info(message, token)
+ warn(scope, message)
}
MilkyHttpHandler --> Config : uses
MilkyHttpHandler --> Context : uses
Context --> Logger : uses
%% Updated logic inside handleRequest for WebSocket paths:
%% 1. Build URL from req.url and req.headers.host
%% 2. Prefer token from req.headers.authorization (Bearer scheme)
%% 3. Fallback to access_token query parameter
%% 4. Log header-based token via logger.info
%% 5. Validate token against config.accessToken and log failures via logger.warn
文件级变更
提示与命令与 Sourcery 交互
自定义你的体验访问你的 控制面板 可以：
获取帮助Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates WebSocket access token validation to accept tokens from the Authorization header (Bearer scheme) in addition to the access_token query parameter, with added logging for header-based tokens. Sequence diagram for updated WebSocket access token validationsequenceDiagram
actor Client
participant MilkyHttpHandler
participant URLParser as URL
participant Logger
Client->>MilkyHttpHandler: WebSocket HTTP upgrade request
activate MilkyHttpHandler
MilkyHttpHandler->>URLParser: new URL(req.url, host)
URLParser-->>MilkyHttpHandler: url
MilkyHttpHandler->>MilkyHttpHandler: read req.headers.authorization
alt Authorization header present
MilkyHttpHandler->>MilkyHttpHandler: inputToken = authHeader.split(Bearer ).pop()
MilkyHttpHandler->>Logger: info(receive ws header token, inputToken)
else Authorization header missing
MilkyHttpHandler->>URLParser: url.searchParams.get(access_token)
URLParser-->>MilkyHttpHandler: inputToken or null
MilkyHttpHandler->>MilkyHttpHandler: inputToken = value or empty string
end
MilkyHttpHandler->>MilkyHttpHandler: compare inputToken with config.accessToken
alt Token invalid or missing
MilkyHttpHandler->>Logger: warn(MilkyHttp, remote -> /event (Credentials invalid))
MilkyHttpHandler-->>Client: Reject WebSocket connection
else Token valid
MilkyHttpHandler-->>Client: Proceed with WebSocket upgrade
end
deactivate MilkyHttpHandler
Class diagram for updated MilkyHttpHandler WebSocket token validationclassDiagram
class MilkyHttpHandler {
- config
- ctx
handleRequest(req, res)
}
class Config {
+ accessToken : string
}
class Context {
+ logger : Logger
}
class Logger {
+ info(message, token)
+ warn(scope, message)
}
MilkyHttpHandler --> Config : uses
MilkyHttpHandler --> Context : uses
Context --> Logger : uses
%% Updated logic inside handleRequest for WebSocket paths:
%% 1. Build URL from req.url and req.headers.host
%% 2. Prefer token from req.headers.authorization (Bearer scheme)
%% 3. Fallback to access_token query parameter
%% 4. Log header-based token via logger.info
%% 5. Validate token against config.accessToken and log failures via logger.warn
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 there - 我已经查看了你的改动，发现有一些需要处理的问题。
- 请避免记录原始的访问令牌（
this.ctx.logger.info('receive ws header token', inputToken)），因为这会将敏感凭据泄露到日志中；建议只记录令牌是否存在，或记录哈希/脱敏后的值。 - 使用
authHeader.split('Bearer ').pop()!解析 Authorization 头的方式比较脆弱；建议不区分大小写地校验前缀并去除空白（例如使用正则，或者检查authHeader.toLowerCase().startsWith('bearer ')然后再切片），以更安全地处理格式异常或空格异常的请求头。
给 AI Agent 的提示词
Please address the comments from this code review:
## Overall Comments
- Avoid logging the raw access token (`this.ctx.logger.info('receive ws header token', inputToken)`) as this can leak sensitive credentials into logs; log only the presence of a token or a hashed/obfuscated value instead.
- The Authorization header parsing with `authHeader.split('Bearer ').pop()!` is brittle; consider validating the prefix case-insensitively and trimming whitespace (e.g., using a regex or checking `authHeader.toLowerCase().startsWith('bearer ')` and then slicing) to handle malformed or oddly spaced headers safely.
## Individual Comments
### Comment 1
<location> `src/milky/network/http.ts:93-100` </location>
<code_context>
- const inputToken = url.searchParams.get('access_token')
+
+ let inputToken = ''
+ const authHeader = req.headers['authorization']
+ if (authHeader) {
+ inputToken = authHeader.split('Bearer ').pop()!
+ this.ctx.logger.info('receive ws header token', inputToken)
+ } else {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Authorization header parsing is brittle and can break with minor format variations.
The code assumes a single, exact `'Bearer '` substring, which can fail with lowercase `'bearer'`, extra spaces, or multiple auth schemes. Consider normalizing and checking the prefix instead, e.g.:
```ts
const authHeader = req.headers['authorization']
if (authHeader?.toLowerCase().startsWith('bearer ')) {
inputToken = authHeader.slice(7).trim()
}
```
This is more robust and removes the need for `.split(...).pop()!` and the non-null assertion.
```suggestion
let inputToken = ''
const authHeader = req.headers['authorization']
if (authHeader?.toLowerCase().startsWith('bearer ')) {
inputToken = authHeader.slice(7).trim()
this.ctx.logger.info('receive ws header token', inputToken)
} else {
inputToken = url.searchParams.get('access_token') ?? ''
}
```
</issue_to_address>
### Comment 2
<location> `src/milky/network/http.ts:96` </location>
<code_context>
+ let inputToken = ''
+ const authHeader = req.headers['authorization']
+ if (authHeader) {
+ inputToken = authHeader.split('Bearer ').pop()!
+ this.ctx.logger.info('receive ws header token', inputToken)
+ } else {
</code_context>
<issue_to_address>
**🚨 issue (security):** Logging the raw access token is a security/privacy risk.
`this.ctx.logger.info('receive ws header token', inputToken)` logs the full access token, which is sensitive and should not be written to logs. Please remove the token from the log, mask it (e.g., only last few chars), or log only that a token was present along with non-sensitive context like remote address/path.
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进后续的审查。
Original comment in English
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Avoid logging the raw access token (
this.ctx.logger.info('receive ws header token', inputToken)) as this can leak sensitive credentials into logs; log only the presence of a token or a hashed/obfuscated value instead. - The Authorization header parsing with
authHeader.split('Bearer ').pop()!is brittle; consider validating the prefix case-insensitively and trimming whitespace (e.g., using a regex or checkingauthHeader.toLowerCase().startsWith('bearer ')and then slicing) to handle malformed or oddly spaced headers safely.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Avoid logging the raw access token (`this.ctx.logger.info('receive ws header token', inputToken)`) as this can leak sensitive credentials into logs; log only the presence of a token or a hashed/obfuscated value instead.
- The Authorization header parsing with `authHeader.split('Bearer ').pop()!` is brittle; consider validating the prefix case-insensitively and trimming whitespace (e.g., using a regex or checking `authHeader.toLowerCase().startsWith('bearer ')` and then slicing) to handle malformed or oddly spaced headers safely.
## Individual Comments
### Comment 1
<location> `src/milky/network/http.ts:93-100` </location>
<code_context>
- const inputToken = url.searchParams.get('access_token')
+
+ let inputToken = ''
+ const authHeader = req.headers['authorization']
+ if (authHeader) {
+ inputToken = authHeader.split('Bearer ').pop()!
+ this.ctx.logger.info('receive ws header token', inputToken)
+ } else {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Authorization header parsing is brittle and can break with minor format variations.
The code assumes a single, exact `'Bearer '` substring, which can fail with lowercase `'bearer'`, extra spaces, or multiple auth schemes. Consider normalizing and checking the prefix instead, e.g.:
```ts
const authHeader = req.headers['authorization']
if (authHeader?.toLowerCase().startsWith('bearer ')) {
inputToken = authHeader.slice(7).trim()
}
```
This is more robust and removes the need for `.split(...).pop()!` and the non-null assertion.
```suggestion
let inputToken = ''
const authHeader = req.headers['authorization']
if (authHeader?.toLowerCase().startsWith('bearer ')) {
inputToken = authHeader.slice(7).trim()
this.ctx.logger.info('receive ws header token', inputToken)
} else {
inputToken = url.searchParams.get('access_token') ?? ''
}
```
</issue_to_address>
### Comment 2
<location> `src/milky/network/http.ts:96` </location>
<code_context>
+ let inputToken = ''
+ const authHeader = req.headers['authorization']
+ if (authHeader) {
+ inputToken = authHeader.split('Bearer ').pop()!
+ this.ctx.logger.info('receive ws header token', inputToken)
+ } else {
</code_context>
<issue_to_address>
**🚨 issue (security):** Logging the raw access token is a security/privacy risk.
`this.ctx.logger.info('receive ws header token', inputToken)` logs the full access token, which is sensitive and should not be written to logs. Please remove the token from the log, mask it (e.g., only last few chars), or log only that a token was present along with non-sensitive context like remote address/path.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| let inputToken = '' | ||
| const authHeader = req.headers['authorization'] | ||
| if (authHeader) { | ||
| inputToken = authHeader.split('Bearer ').pop()! |
Contributor
There was a problem hiding this comment.
🚨 issue (security): 记录原始访问令牌是一个安全/隐私风险。
this.ctx.logger.info('receive ws header token', inputToken) 会把完整的访问令牌写进日志，而访问令牌是敏感信息，不应写入日志。请从日志中移除令牌本身，或对其做掩码处理（例如只保留最后几位），或者仅记录“存在令牌”这一事实以及非敏感的上下文信息（如远程地址/请求路径）。
Original comment in English
🚨 issue (security): Logging the raw access token is a security/privacy risk.
this.ctx.logger.info('receive ws header token', inputToken) logs the full access token, which is sensitive and should not be written to logs. Please remove the token from the log, mask it (e.g., only last few chars), or log only that a token was present along with non-sensitive context like remote address/path.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by Sourcery
错误修复：
AuthorizationBearer 头（header）来提供 WebSocket 访问令牌（access token），此外仍支持通过access_token查询参数提供。Original summary in English
Summary by Sourcery
Bug Fixes: