Skip to content

fix: restore anti-cheat movement defense with stair/slab compatibility#732

Open
lt-name wants to merge 5 commits into
masterfrom
fix/727-anticheat-movement
Open

fix: restore anti-cheat movement defense with stair/slab compatibility#732
lt-name wants to merge 5 commits into
masterfrom
fix/727-anticheat-movement

Conversation

@lt-name

@lt-name lt-name commented May 26, 2026

Copy link
Copy Markdown
Member

No description provided.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

This was linked to issues May 26, 2026
@tachibana-kanade-angel tachibana-kanade-angel added [Type] Bug | 漏洞 Something isn't working [Status] AI Confirmed Pending | AI 确认待核 AI 在代码中确认问题存在,待人工复核 labels May 26, 2026
tachibana-kanade-angel

This comment was marked as duplicate.

@tachibana-kanade-angel tachibana-kanade-angel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📋 PR Review

✅ Summary

The restructured anti-cheat movement defense introduces two significant security bypass vectors. While the intent to restore speed-checking and stair/slab compatibility is reasonable, the implementation has critical flaws that need addressing before merge.

🔍 Issues Found

Blocking:

  • [CRITICAL] Y-axis shrink(0.1, stepHeight, 0.1) creates noclip vectors (Player.java:2160)

    • The bounding box is shrunk by getStepHeight() on Y from both top and bottom before collision checking. For a standing player (height ~1.8, stepHeight ~0.5-1.0), only the middle body slice is checked, allowing noclip through bottom slabs, lower stair sections, and head-level partial blocks. For swimming/crawling/gliding where player height is similar to stepHeight, the probe can collapse entirely and skip collision detection.
  • [CRITICAL] knockBackTime <= 0 completely disables raw speed check (Player.java:2143)

    • The new anti-speed-hack gate is fully bypassed when knockBackTime > 0. Since any melee hit sets knockBackTime = 10 server-side, a player can collude with mobs or other players to keep this timer refreshed, allowing arbitrary horizontal speed during combat without triggering the check. This is a regression compared to the old code that checked correction amount regardless of knockback state.

Minor:

  • [MINOR] knockBackTime is an incomplete proxy for legitimate velocity (Player.java:2143)
    • Some server-applied motion uses setMotion() without setting knockBackTime (e.g., Wind Charge, Mace Wind Burst), while melee knockback sets the timer even when the actual horizontal displacement is small or cancelled by plugins. This creates inconsistent behavior where some legitimate velocity sources bypass the check while others do not.

💡 Recommendations

  1. For the noclip probe, use full-height collision checking at the destination position or apply only a narrow foot-level Y tolerance for stair/slab stepping instead of symmetric stepHeight shrink.
  2. Replace the blanket knockBackTime <= 0 exemption with an expected-velocity model: track server-applied motion and allow client delta within a bounded margin of the expected velocity, consuming that allowance per-tick.

📝 Notes

These two blocking issues should be addressed before merge. The minor issue can be addressed separately.


🤖 AI Review

@lt-name

lt-name commented May 26, 2026

Copy link
Copy Markdown
Member Author

@tachibana-kanade-angel review

tachibana-kanade-angel

This comment was marked as duplicate.

@tachibana-kanade-angel tachibana-kanade-angel removed the [Status] AI Confirmed Pending | AI 确认待核 AI 在代码中确认问题存在,待人工复核 label May 26, 2026
@tachibana-kanade-angel

Copy link
Copy Markdown
Contributor

📋 PR Review — AI Review

✅ Summary

本次 review 未发现阻塞性问题。PR 将防作弊移动检测从"根据 fastMove 修正量判断"重构为两个独立机制:(1) 服务器运动配额 — 精确追踪服务器下发的 motion,只处罚超出预期的客户端位移;(2) 目标碰撞验证 — 在接受位置前直接检查目的地碰撞,不再依赖 fastMove 的滑动修正。新增的 9 个测试覆盖了碰撞阻断、台阶/半砖边缘、游泳低矮碰撞、击退配额、多 tick 累计等关键场景。整体设计思路清晰,代码质量良好。

🔍 Issues Found

无严重问题。

💡 Recommendations

  • destinationBox.shrink(0.1, 0, 0.1) 的水平容差 (0.1) 是 magic number,建议提取为常量并加注释说明与台阶/半砖碰撞盒的关系。
  • consumeServerMotionAllowance 存在副作用(修改 serverMotionAllowanceX/Z/Ticks)但方法名暗示只读,可考虑拆分或重命名(如 consumeAndAccumulateServerMotionAllowance)。

📝 Notes

  • .m2/repository 只读导致 CI 编译失败是环境问题,非 PR 代码问题。从 diff 逻辑分析,新旧 API 调用一致,编译应无问题。
  • 旧有的 fastMove 滑动碰撞解析已完全移除。服务器不再主动修正玩家位移,改为二元判断(通过/回退)。这更严格也更安全 — 客户端不应依赖服务器的滑动修正来穿墙。

🤖 AI Review

@lt-name 可以合并

tachibana-kanade-angel

This comment was marked as duplicate.

# Conflicts:
#	src/main/java/cn/nukkit/Player.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Bug | 漏洞 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Anti Cheat Broken falling through blocks

2 participants