Skip to content

Conversation

Mai-icy
Copy link

@Mai-icy Mai-icy commented Oct 10, 2025

🔗 相关问题 / Related Issue

Issue 链接 / Issue Link: #307 👈👈

  • 我已经创建了相关 Issue 并进行了讨论 / I have created and discussed the related issue
  • 这是一个微小的修改(如错别字),不需要 Issue / This is a trivial change (like typo fix) that doesn't need an issue

📋 变更类型 / Type of Change

  • 🐛 Bug 修复 / Bug fix (non-breaking change which fixes an issue)
  • ✨ 新功能 / New feature (non-breaking change which adds functionality)
  • 💥 破坏性变更 / Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📚 文档更新 / Documentation update
  • 🔧 重构 / Refactoring (no functional changes)
  • ⚡ 性能优化 / Performance improvement
  • 📦 依赖升级 / Dependency upgrade (update dependencies to newer versions)
  • 🚀 功能增强 / Feature enhancement (improve existing functionality without breaking changes)
  • 🧹 代码清理 / Code cleanup

📝 变更目的 / Purpose of the Change

当前 HttpClassicServerResponse 在设置 Cookie 时,会错误地将其写入响应头中的 Cookie 字段,而不是标准的 Set-Cookie
这会导致客户端无法正确识别和存储服务端下发的 Cookie,且丢失了 PathSecureHttpOnly 等关键属性。

本次变更修复了该问题,使响应中每个 Cookie 都以单独的 Set-Cookie header 下发,并正确包含所有属性。

📋 主要变更 / Brief Changelog

  • 将响应中的 Cookie 错误写入 Cookie 头的问题,改为使用标准的 Set-Cookie 头。
  • 支持解析和序列化 Cookie 属性,包括 Path、Domain、Secure、HttpOnly、SameSite、Max-Age 等。
  • 修改 Server 端的 Request 和 Client 端的 Response 的 Cookie 为只读。
  • 修改了 CookieCollection 的存储逻辑,将其作为上层的 Cookie 管理器

🧪 验证变更 / Verifying this Change

测试步骤 / Test Steps

  1. 使用demo1添加以下代码
@Component
public class UserController {
    private static int counter = 0;

    @GetMapping(path = "/user")
    public User getUser(@RequestParam("name") String name, @RequestParam("age") String age, HttpClassicServerResponse response) {
        Cookie cookieA = Cookie.builder().name("cookie1").value("value1").path("/").secure(true).build();
        Cookie cookieB = Cookie.builder().name("cookie2").value("value2").maxAge(100).httpOnly(true).build();
        response.cookies().add(cookieA);
        response.cookies().add(cookieB);
        return new User(name, age, ++counter);
    }
}
  1. 通过浏览器访问 http://127.0.0.1:8080/user?name=user&age=19
  2. 通过浏览器开发者工具查看 Response Headers 是否携带 Set-Cookie。

测试覆盖 / Test Coverage

  • 我已经添加了单元测试 / I have added unit tests
  • 所有现有测试都通过 / All existing tests pass
  • 我已经进行了手动测试 / I have performed manual testing

📸 截图 / Screenshots

修改前:
image
修改后:
image

✅ 贡献者检查清单 / Contributor Checklist

请确保你的 Pull Request 符合以下要求 / Please ensure your Pull Request meets the following requirements:

基本要求 / Basic Requirements:

  • 确保有 GitHub Issue 对应这个变更(微小变更如错别字除外)/ Make sure there is a Github issue filed for the change (trivial changes like typos excluded)
  • 你的 Pull Request 只解决一个 Issue,没有包含其他不相关的变更 / Your PR addresses just this issue, without pulling in other changes - one PR resolves one issue
  • PR 中的每个 commit 都有有意义的主题行和描述 / Each commit in the PR has a meaningful subject line and body

代码质量 / Code Quality:

  • 我的代码遵循项目的代码规范 / My code follows the project's coding standards
  • 我已经进行了自我代码审查 / I have performed a self-review of my code
  • 我已经为复杂的代码添加了必要的注释 / I have commented my code, particularly in hard-to-understand areas

测试要求 / Testing Requirements:

  • 我已经编写了必要的单元测试来验证逻辑正确性 / I have written necessary unit-tests to verify the logic correction
  • 当存在跨模块依赖时,我尽量使用了 mock / I have used mocks when cross-module dependencies exist
  • 基础检查通过:mvn -B clean package -Dmaven.test.skip=true,elsa README 中的编译检查 / Basic checks pass
  • 单元测试通过:mvn clean install / Unit tests pass

文档和兼容性 / Documentation and Compatibility:

  • 我已经更新了相应的文档 / I have made corresponding changes to the documentation
  • 如果有破坏性变更,我已经在 PR 描述中详细说明 / If there are breaking changes, I have documented them in detail
  • 我已经考虑了向后兼容性 / I have considered backward compatibility

📋 附加信息 / Additional Notes


审查者注意事项 / Reviewer Notes:

@Mai-icy Mai-icy requested a review from CodeCasterX October 10, 2025 12:07
@Mai-icy Mai-icy self-assigned this Oct 10, 2025
@Mai-icy Mai-icy added type: bug A general bug in: fit Issues in FIT modules labels Oct 10, 2025
@CodeCasterX CodeCasterX added this to the 3.5.4 milestone Oct 10, 2025
@CodeCasterX CodeCasterX linked an issue Oct 10, 2025 that may be closed by this pull request
4 tasks
Copy link
Member

@CodeCasterX CodeCasterX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

检视意见

我对这个PR进行了详细的代码检视。总体来说,这是一个高质量的Bug修复PR,核心逻辑正确,测试覆盖充分。

✅ 优点

  1. 问题定位准确: 正确识别了将Cookie写入Cookie头而非Set-Cookie头的核心问题
  2. 符合RFC标准: 实现遵循RFC 6265规范,支持所有标准Cookie属性
  3. 测试覆盖充分: 新增14个测试用例覆盖Cookie解析、格式化、边界情况
  4. 向后兼容: 将version()comment()标记为@Deprecated而非直接删除
  5. 代码质量高: 代码结构清晰,注释完整,符合项目规范

⚠️ 发现的问题

1. 空指针风险 (重要)

位置: DefaultCookieCollection.java:87

@Override
public void add(Cookie cookie) {
    store.computeIfAbsent(cookie.name(), k -> new ArrayList<>());  // 如果cookie为null会抛NPE
    // ...
}

建议: 添加空值检查

@Override
public void add(Cookie cookie) {
    if (cookie == null || StringUtils.isBlank(cookie.name())) {
        return;
    }
    // 现有逻辑...
}

2. Expires转换精度问题 (中等)

位置: HttpUtils.java:165

当Expires时间超过2038年时,long转int会溢出。

建议:

return (int) Math.min(Math.max(seconds, 0), Integer.MAX_VALUE);

3. Cookie解析鲁棒性 (轻微)

位置: HttpUtils.parseCookies()

建议增加测试用例覆盖边界情况,如引号不完整的value: a="incomplete

4. 文档完整性 (轻微)

PR checklist中"我已经更新了相应的文档"未勾选,建议补充用户文档说明Cookie处理的变更。

📋 总结

建议修复问题1、2(必要)后即可合并,问题3、4可在后续优化。

整体评价: LGTM with minor fixes 👍

@Mai-icy
Copy link
Author

Mai-icy commented Oct 11, 2025

已修复检视意见中的必要问题:

  1. 空指针风险 (DefaultCookieCollection#add)

    • 修复 commit: 8672e5d [fit] add null and blank name check in CookieCollection#add
  2. Expires 转换精度问题 (HttpUtils)

    • 修复 commit: 0a36757 [fit] safely convert Expires to Max-Age within int range
  3. Cookie 名值非法字符检查

    • 修复 commit: ab8e415 [fit] check for invalid characters in cookie name and value
  4. 添加对 Cookie 的校验 (修复增强)

    • 修复 commit: bf6b69e [fit] validate cookie before adding to collection

@Mai-icy Mai-icy requested a review from CodeCasterX October 11, 2025 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in: fit Issues in FIT modules type: bug A general bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HttpClassicServerResponse 内 Set-Cookie 逻辑缺失

2 participants