Repository navigation
Add extra player details and mission events - #128
Conversation
Added: - Auto-restart and connection event buffering - New CBA settings to configure everything these changes are touching Changed: - Player counting and Steam ID handling
|
Ratatoskr reviewed this pull request. Changes requested on Finished 2026-10-17 16:25 UTC. |
fank
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — 3 blocking findings.
Review details
Reviewed head 48e4def4ecf81d5ddda4e975c48b38a332300c2a against base 0b6c444c54b215dfdb4166287e9c0dd6f0a434c6.
-
[P2] Authenticate snapshot ownership before updating another unit's recording.
fnc_submitPlayerSnapshot.sqf:54–60accepts any non-null object with an OCAP id. A client can call this endpoint with another player's object and an invented medical/inventory payload; the server then stamps the victim's real id/UID and replaces their diff baseline. Deriving identity from a caller-supplied object does not authenticate the caller. This remains exploitable when only this documented endpoint is whitelisted in CfgRemoteExec. Validate the remote caller against the unit's owner before accepting snapshots, and apply equivalent caller/curator validation to the new Zeus submission endpoints. Add a negative test showing that a client cannot submit data for another player's unit. -
[P2] Accumulate small servicing improvements instead of discarding them every sample.
fnc_trackServicing.sqf:265–275requires each individual sample to improve by at least0.01, but advances the baseline even when it does not. For example, fuel readings0.500 → 0.505 → 0.510 → … → 1.000never open a refuel session, despite adding half a tank. Slow refuelling of a large tank, particularly at shorter configurable sample intervals, can therefore disappear completely; an existing session also stops extending when increments fall below the threshold. Accumulate sub-threshold positive changes and apply the noise threshold to the accumulated/session change. Pin this with a sequence of individually sub-threshold samples. -
[P2] Reset the new entity-id-keyed trackers when exporting a recording.
fnc_exportData.sqf:174–186clears snapshot/Zeus state and resets entity ids, but retainsserviceBaselineandstaticWeaponSnapshot. Their PFHs survive export, and their initializers run only from mission init. On the next recording, a reused vehicle id is compared with the previous recording's fuel/damage, generating fictitious repairs/refuels. Likewise, a formerly tracked static weapon whose id is absent from the new static set produces a false disassembly pointing at an old/reused id (fnc_trackConstructions.sqf:240–247). Clear recording-scoped servicing/static snapshots and pending claims alongside the other reset state, and test export → start with changed entity membership/order.
Verification: GitHub's current build job succeeded; its Run HEMTT check and Run HEMTT build steps both report success. Locally, hemtt check and hemtt build could not run (hemtt: command not found). git diff --check 0b6c444c54b215dfdb4166287e9c0dd6f0a434c6 HEAD passed, and Python ElementTree parsed addons/recorder/stringtable.xml successfully. No automated behavioral tests were found in this checkout. Findings above are source-level analysis, not an executed Arma multiplayer reproduction; gameplay/mod integration remains unverified locally.
Earlier discussions: GraphQL enumeration returned no review threads (hasNextPage: false), so there were no discussions to resolve, continue, or skip. The reviewed checkout was not modified.
|
Ratatoskr reviewed this pull request. Changes requested on Finished 2026-10-17 18:48 UTC. |
fank
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — 2 blocking findings.
Review details
Reviewed dff0faacb3acc1ba7c603f53f2fcf139cb9be5cc against the PR's current base.
-
[P2] Authenticate static-weapon submissions before recording attribution.
addons/recorder/fnc_submitStaticWeapon.sqf:76–81: this remotely callable endpoint accepts any non-null_unitand uses its identity without checking the sender. A client can submit['assembled', anotherPlayer, existingGun]and write a fabricated assembly credited to another player, or inject disassembly claims under that player's identity. Reading the ID from the caller-selected object does not authenticate the caller. Apply the remote sender/owner _unitcheck already used bysubmitPlayerSnapshotand the Zeus endpoints, before modifying claims/dedupe state; retain direct server AI calls. Add a remote spoofing regression for this endpoint with legitimate-owner and direct-server positive controls. -
[P2] Reset sampling caches when the controlled player unit changes.
addons/recorder/fnc_installPlayerTrackingClients.sqf:1404–1408: cache invalidation only compares recording start time, so respawn/team switch retains the previous unit's timestamps and fingerprints. For example, switching to a new unit with an identical kit before the 120-second inventory interval expires produces no initial inventory snapshot for its new OCAP ID: the fingerprint matches and the timer is not due. The new entity's Gear tab can remain absent until the old unit's interval expires; with a short life and death snapshots disabled, that life can have no inventory data at all. Reset the per-kind timestamps, fingerprints and radio-seen state on player-object/OCAP-ID changes as well as recording changes. Cover switching/respawning to an identical-loadout unit before the interval elapses and verify a full initial snapshot for the new ID.
Verification: GitHub build run 39059216778 passed both hemtt check and hemtt build. Local hemtt check exited 127 (hemtt: command not found). Neither arma3server nor arma3 is available on PATH, so the multiplayer regression script in tests/README.md was not run; the findings above are source-derived, not claimed in-game reproductions. Existing regressions do not exercise static-weapon sender authentication or client sampler unit changes.
Earlier discussions: GraphQL enumeration returned zero review threads (no further pages), so none required replies, resolution, or skipping.
The reviewed checkout was not modified.
|
Ratatoskr reviewed this pull request. Changes requested on Finished 2026-10-17 19:15 UTC. |
fank
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — 1 blocking correctness finding.
Review details
Reviewed head cf0c71c9adeb79f756d0e81329d1776f22089e7a against PR base 0b6c444c54b215dfdb4166287e9c0dd6f0a434c6.
Required correction
[P2] Unwrap the LR radio before querying its config property — addons/recorder/fnc_installPlayerTrackingClients.sqf:1024–1025.
buildRadioSnapshot passes _lr from TFAR_fnc_activeLrRadio into getTfarCode. This is [radioObject, qualifier], which is correct for TFAR_fnc_getLrRadioCode, but not for TFAR_fnc_getLrRadioProperty: that API expects the object itself and immediately executes _radio getVariable _property (TFAR implementation). When the live LR code is empty while TFAR_radioCodesDisabled is false—for example, an empty configured side code or the initialization case this fallback explicitly handles—this branch supplies an array as the receiver of getVariable and causes a client-side SQF type error instead of recovering the encryption code. The subsequent class-config fallback does not make that invalid API call safe.
Pass the tuple's object element to getLrRadioProperty, while retaining the full tuple for getLrRadioCode. Add a regression covering an active LR tuple with an empty live code and radio codes not globally disabled. This is source-confirmed against TFAR's API; I did not execute an Arma reproduction.
Checks and limitations
git diff --check 0b6c444c54b215dfdb4166287e9c0dd6f0a434c6...HEAD: passed.hemtt check: unavailable (hemtt: command not found). No local build claimed.- The PR's GitHub
buildcheck is successful; its workflow runshemtt checkandhemtt build, not the SQF regression scripts. tests/README.mdrequires a disposable Arma multiplayer mission, two distinct client owners, and dedicated/listen-server runs. Neither Arma nor an SQF runtime is installed here, so neither regression suite nor gameplay behavior was executed. The documented suites do not exercise this TFAR fallback.- GitHub GraphQL enumeration returned zero review threads, with
hasNextPage: false. Resolved: 0; still-open prior discussions: 0; skipped: 0. No thread mutations were needed. - The reviewed checkout was left unchanged. No merge, commit, or push performed.
|
Ratatoskr reviewed this pull request. Approved on Finished 2026-10-17 19:28 UTC. |
fank
left a comment
There was a problem hiding this comment.
APPROVE — 0 remaining blocking findings.
Review details
Reviewed head 3e353b84585c6ac300b3c1f67b3819141df883f7 against base 0b6c444c54b215dfdb4166287e9c0dd6f0a434c6. No further actionable blocking defects identified.
Earlier findings rechecked against current code:
- The snapshot/Zeus authentication, servicing accumulation and export-reset findings are addressed: submission endpoints authenticate remote ownership (and curator assignment where applicable), servicing retains sub-threshold positive steps and filters completed sessions, and export clears servicing/static recording-scoped state.
- The static-submission authentication and sampler-cache findings are addressed in
fnc_submitStaticWeapon.sqfand the client sampler's player-object/entity-id reset guards. - The TFAR LR fallback finding is addressed in
fnc_installPlayerTrackingClients.sqf:1019–1032: the live-code API retains the full tuple, while the property API receives only the radio object.tests/client_sampler_regressions.sqfexercises the actual installed helper with tuple-sensitive mocks, empty/live/disabled code cases; reverting the object-unwrapping fix would fail that property-API assertion.
Checks and limitations:
git diff --check 0b6c444c54b215dfdb4166287e9c0dd6f0a434c6...HEAD: passed.- Python ElementTree parsed
addons/recorder/stringtable.xmlsuccessfully. - GitHub build run 39062156360: successful; both
Run HEMTT checkandRun HEMTT buildsucceeded. - Local
hemtt check: unavailable (hemtt: command not found); no local build claimed. - Neither Arma nor an SQF runtime is available on PATH. The dedicated/listen-server multiplayer suites documented in
tests/README.mdwere inspected, not executed. Gameplay and optional-mod integration remain unverified locally; regression coverage is not a claim of runtime success.
Discussions: Authenticated reviewer identity is fank. GraphQL review-thread enumeration returned zero threads with hasNextPage: false; no comment pages remained to enumerate. Resolved: 0; still-open threads: 0; skipped thread actions: 0. The earlier findings were in review bodies linked above, not resolvable threads. No replies or thread mutations were needed.
The reviewed checkout remains unchanged; no commit, push or merge performed.
Records extra player details and mission events from the server.
Added
Changed
Fixed