Add claim proxy, traffic protection, session fixes #81
No reviewers
Labels
No labels
bug
dependencies
documentation
duplicate
enhancement
github_actions
go
good first issue
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
pokebedrock/gobds!81
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "claim-managment-upgrades"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Introduce a full claim proxy and traffic-protection system plus related session/server/entity improvements. Added a claim package (factory, service, snapshot, model, metrics, tests) with HTTP revalidation, immutable snapshots, fail-open semantics, and policy golden fixtures. Implement deny block, deny-rendering of subchunks, corrective chunk logic, and claim decision helpers used by many new session packet handlers. Add per-session traffic rate limiting, traffic metrics, XUID reservation to prevent duplicate logins, improved entity tracking, config fields/defaults, and numerous tests. CI updated to run go test -race and policy JSON included in repo.
Needs: https://github.com/smell-of-curry/bds-manager/pull/10 and https://github.com/smell-of-curry/pokebedrock-beh/pull/670
Summary by CodeRabbit
New Features
Bug Fixes
📝 Walkthrough
Walkthrough
Adds immutable claim snapshots, feature-based authorization, traffic protection, metrics, entity tracking, duplicate-XUID admission, rendering corrections, expanded session handlers, configuration, and comprehensive tests.
Changes
Claims and session hardening
gobds/claim/*,gobds/config.go,gobds/user_config.go,config.example.toml,policy/*,gobds/block/*gobds/claim/factory.go,gobds/claim/service.go,gobds/claim/metrics.gogobds/entity/*,gobds/session/config.go,gobds/session/data.go,gobds/session/handler_*gobds/session/util_claim_action.go,gobds/session/handler_inventory_transaction.go,gobds/session/handler_player_auth_input.go,gobds/session/handler_sub_chunk.gogobds/session/traffic.go,gobds/session/session.go,gobds/server.go,gobds/gobds.goEstimated code review effort: 5 (Critical) | ~120 minutes
Sequence Diagram(s)
Possibly related PRs
Suggested reviewers:
glancist🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
claim-managment-upgradesThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
❤️ Share
Comment
@coderabbitai helpto get the list of available commands.Actionable comments posted: 7
🧹 Nitpick comments (5)
🤖 Prompt for all review comments with AI agents
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
b0887fe4-19e6-40eb-b4e4-450a3bd26ffb📥 Commits
Reviewing files that changed from the base of the PR and between
8e4a5764e7and5a9da19152.📒 Files selected for processing (55)
.github/workflows/lint.yml.gitignoreconfig.example.tomlgobds/block/deny.gogobds/block/factory.gogobds/block/hash.gogobds/claim/factory.gogobds/claim/factory_test.gogobds/claim/metrics.gogobds/claim/metrics_test.gogobds/claim/model.gogobds/claim/model_test.gogobds/claim/service.gogobds/claim/service_test.gogobds/claim/snapshot.gogobds/claim/snapshot_test.gogobds/config.gogobds/config_test.gogobds/entity/entity.gogobds/entity/factory.gogobds/entity/factory_test.gogobds/gobds.gogobds/server.gogobds/server_test.gogobds/service/authentication/service.gogobds/service/vpn/service.gogobds/session/claim_policy_golden_test.gogobds/session/config.gogobds/session/data.gogobds/session/handler_add_actor.gogobds/session/handler_add_painting.gogobds/session/handler_change_dimension.gogobds/session/handler_command_request.gogobds/session/handler_inventory_transaction.gogobds/session/handler_inventory_transaction_test.gogobds/session/handler_item_registry.gogobds/session/handler_item_stack_request.gogobds/session/handler_modal_form_response.gogobds/session/handler_move_actor.gogobds/session/handler_player_auth_input.gogobds/session/handler_player_auth_input_test.gogobds/session/handler_remove_actor.gogobds/session/handler_set_player_game_type.gogobds/session/handler_sub_chunk.gogobds/session/handler_text.gogobds/session/handler_update_abilities.gogobds/session/handler_update_player_game_type.gogobds/session/session.gogobds/session/traffic.gogobds/session/traffic_test.gogobds/session/util.gogobds/session/util_claim_action.gogobds/session/util_claim_action_test.gogobds/user_config.gopolicy/claim_policy.v1.json@ -15,6 +14,7 @@ import (// Service ...🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
200 OK path clears
lastModifiedon a header-less response, unlike the 304 path.Line 86 unconditionally overwrites
s.lastModifiedwithresponse.Header.Get("last-modified"), even when empty, dropping any previously cached value and disabling futureIf-Modified-Sincerevalidation until the server happens to send the header again. The 304 branch (lines 92-94) correctly guards this withif modified != "".🩹 Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Addressed in commit
cc0d3af@ -86,0 +107,4 @@LastModified: s.lastModified,}, nil, falsecase http.StatusNotModified:if modified := response.Header.Get("last-modified"); modified != "" {📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
FetchClaimsexceeds the gocyclo complexity threshold (CI lint failure).The lint check reports complexity 17 (> 15). Extracting the per-status-code handling (e.g. a helper that closes the body, cancels the context, and returns the appropriate
(FetchResult, error, bool retry)) would bring this back under the threshold and improve readability without changing behavior.🧰 Tools
🪛 GitHub Check: lint
[failure] 33-33:
cyclomatic complexity 17 of func
(*Service).FetchClaimsis high (> 15) (gocyclo)🤖 Prompt for AI Agents
Source: Linters/SAST tools
✅ Confirmed as addressed by @smell-of-curry
@ -30,0 +128,4 @@dimensionRange cube.Range,claims []*claim.PlayerClaim,denyID uint32,actor ClaimActor,📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Reduce
Handle's cyclomatic complexity (CI lint failure).CI reports
gocyclocomplexity 18 (>15) for this function. It currently mixes latency instrumentation, panic-recovery setup, snapshot/dimension resolution, and per-entry border+claim filtering in one body. Consider extracting the snapshot/dimension resolution block (lines 39-53) and the per-entry filtering logic (lines 56-95) into separate helper functions to bring this under the threshold.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 57-57: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(entry.Offset[0])
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 58-58: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(entry.Offset[2])
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🪛 GitHub Check: lint
[failure] 20-20:
cyclomatic complexity 18 of func
(*SubChunkHandler).Handleis high (> 15) (gocyclo)🤖 Prompt for AI Agents
Source: Linters/SAST tools
✅ Addressed in commit
cc0d3af@ -30,0 +185,4 @@chunk.EncodeSubChunk(virtualChunk, chunk.NetworkEncoding, int(index)),blockEntityPayload...,)return entry🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 2114
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 3898
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 4915
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 4029
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 4908
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 2024
applyClaimDenyBlocksonly rewrites the bottom-most layer.sectionYlimits this to the dimension’s minimum section, andSetBlock(x, 0, z, 0, denyID)hard-codesy=0, so this affects just one 16×16 slice at the world floor. If the goal is to hide claimed areas, this needs to cover the full visible Y range, not a single slice.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 111-111: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(dimensionRange.Min()>>4)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 139-139: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(x)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 139-139: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(z)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 148-148: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint8(ClaimActionRender)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 Prompt for AI Agents
✅ Addressed in commit
cc0d3af@ -0,0 +39,4 @@}s.WriteToClient(correction)return nil}🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 10869
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 5929
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 11673
Reuse
correctiveLevelChunkfor this refresh. This packet leavesDimensionandHighestSubChunkunset, so the forced chunk refresh can target the wrong dimension outside the Overworld. Reuse the shared helper here instead of buildingpacket.LevelChunkmanually.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 33-33: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(math.Floor(float64(position.X())))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 34-34: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(math.Floor(float64(position.Z())))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 Prompt for AI Agents
✅ Addressed in commit
cc0d3af🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Raw XUID persisted into traffic metrics output.
WriteTrafficMetricspassess.IdentityData().XUIDintoTrafficMetrics.WriteDelta, which JSON-serializes it as thesessionfield of every emittedtraffic_protection_metricsrecord. This writes a persistent per-player identifier into metrics/log output, which may need redaction, hashing, or an explicit retention policy depending on compliance requirements (XUID can be used to identify/correlate an individual player over time).Consider hashing/truncating the XUID before logging, or gating raw-XUID inclusion behind an explicit config flag if it's needed for abuse investigation.
🤖 Prompt for AI Agents
@ -0,0 +72,4 @@return claimActionItemDropPermitted(cl, data)}return true}📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix the failing lint check:
ClaimActionPermittedcyclomatic complexity is 16 (> 15).The
GitHub Check: lintpipeline reports this function over the gocyclo threshold. Extracting the per-action special-case branches (e.g. the admin-claim"*"handling for block-interact/entity-interact/item-drop) into small named helpers would reduce complexity without changing behavior.♻️ Proposed refactor sketch
📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: lint
[failure] 47-47:
cyclomatic complexity 16 of func
ClaimActionPermittedis high (> 15) (gocyclo)🤖 Prompt for AI Agents
Source: Pipeline failures
✅ Addressed in commit
cc0d3af@ -86,0 +107,4 @@LastModified: s.lastModified,}, nil, falsecase http.StatusNotModified:if modified := response.Header.Get("last-modified"); modified != "" {Fixed in
cc0d3af— extractedhandleFetchResponsesoFetchClaimsstays under the gocyclo threshold.@ -15,6 +14,7 @@ import (// Service ...Fixed in
cc0d3af— 200 OK now only updateslastModifiedwhen the header is non-empty, matching the 304 path.@ -30,0 +128,4 @@dimensionRange cube.Range,claims []*claim.PlayerClaim,denyID uint32,actor ClaimActor,Fixed in
cc0d3af— snapshot/dimension setup and per-entry filtering extracted;dimensionRangeByIDhoisted above the loop.@ -30,0 +185,4 @@chunk.EncodeSubChunk(virtualChunk, chunk.NetworkEncoding, int(index)),blockEntityPayload...,)return entryIntentional — deny rendering places a floor marker at the dimension min section (
y=0only). Full-Y rewrite would change player-visible claim rendering and cost decode/modify on every subchunk; keeping floor-only.@ -0,0 +39,4 @@}s.WriteToClient(correction)return nil}Fixed in
cc0d3af— operator refresh now usescorrectiveLevelChunksoDimensionandHighestSubChunkare populated.Keeping raw XUID — traffic metrics are operational abuse telemetry keyed by session; hashing would break correlating rate-limit spikes to a player. Same ops model as claim proxy counters.
@ -0,0 +72,4 @@return claimActionItemDropPermitted(cl, data)}return true}Fixed in
cc0d3af— extractedclaimActionBlockInteractPermitted/EntityInteract/ItemDrophelpers.@ -86,0 +107,4 @@LastModified: s.lastModified,}, nil, falsecase http.StatusNotModified:if modified := response.Header.Get("last-modified"); modified != "" {@smell-of-curry, thanks — the extractedhandleFetchResponsekeepsFetchClaimsfocused on retry orchestration and addresses the gocyclo finding. 🐇If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
@ -15,6 +14,7 @@ import (// Service ...@smell-of-curry, confirmed—the guarded update preserves the cached validator on header-less200 OKresponses, consistent with the304 Not Modifiedpath. Thanks!If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
@ -0,0 +39,4 @@}s.WriteToClient(correction)return nil}@smell-of-curry, thanks for addressing this. ReusingcorrectiveLevelChunkensures the operator-change refresh carries the correct dimension metadata and subchunk range.If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
@ -0,0 +72,4 @@return claimActionItemDropPermitted(cl, data)}return true}@smell-of-curry, thanks—those helper extractions address the complexity finding and keep the policy dispatch clear. 🐇If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
@smell-of-curry, understood. Since raw XUID is an intentional, established operational identifier for correlating abuse telemetry, this finding does not apply. I’ll treat this as the accepted telemetry policy for traffic metrics and claim proxy counters.✏️ Learnings added
@ -30,0 +128,4 @@dimensionRange cube.Range,claims []*claim.PlayerClaim,denyID uint32,actor ClaimActor,@smell-of-curry, confirmed. The extraction ofresolveClaimSubChunkContextandfilterSubChunkEntry, plus hoistingdimensionRangeByIDoutside the loop, resolves the complexity concern while keeping the handler’s fail-open flow intact.If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
@ -30,0 +185,4 @@chunk.EncodeSubChunk(virtualChunk, chunk.NetworkEncoding, int(index)),blockEntityPayload...,)return entry@smell-of-curry, understood — the floor-only deny marker is intentional. My comment assumed full-volume concealment was the goal, so it does not apply. I’m withdrawing it.✏️ Learnings added