Repository navigation
Restore release vote-site formatting and fix proxy service logs - #1711
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 2 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9e9c282f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Override public boolean siteEnabled(VoteSite site) { return site.isEnabled(); } | ||
| @Override public String siteKey(VoteSite site) { return site.getKey(); } | ||
| @Override public String siteDisplayName(VoteSite site) { return site.getDisplayNameForFormatting(); } | ||
| @Override public String siteDisplayName(VoteSite site) { return site.getDisplayName(); } |
There was a problem hiding this comment.
When AutoCreateVoteSites is enabled (the shipped default), a Votifier sender can supply an otherwise-valid service such as %player_ip%, which becomes the generated site's fallback display name. Returning that raw value here lets BroadcastFormat insert it into %site%, after which BroadcastHandler.broadcastNow runs PlaceholderUtils.replacePlaceHolders, turning attacker-controlled service text into a second-pass PlaceholderAPI expression; installed expansions may therefore disclose data or perform expansion-specific effects instead of displaying the service literally. Preserve the inert display value for broadcasts and other rendering paths while retaining the raw identifier only for matching and action placeholders.
AGENTS.md reference: AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
|
Regarding the P1 formatting finding: acknowledged and deferred to follow-up work. This PR intentionally restores the 7.1.1 Bukkit/Paper formatting behavior, including raw site values entering the existing formatter. The interpreter-boundary risk is documented in service-site-rendering.md and the PR description. The finding remains valid; this rollback does not claim to prevent expansion of untrusted service text. The immediate scope restores normal site-name spacing and corrects proxy service-name logging. The proposed AutoCreateVoteSites setup reminder is separate work and does not resolve this boundary. The formatting-security follow-up remains deferred. No change is being represented as a security fix here. |
Restore vote-site display formatting to the GitHub 7.1.1 release behavior and preserve ordinary service punctuation in proxy receipt logs.
This intentionally restores the release's formatting trust semantics: external/fallback values can participate in existing formatting and placeholder interpretation. It does not retain the later interpreter-boundary protection. The separate NeoForge replay adapter and shared guard helper are unchanged.
Validation: 82 focused tests passed. The final latest-master clean package ran 2,435 tests: 2,434 passed, zero failures/errors and one existing optional paired-Votifier diagnostics test skipped because its optional artifact was not supplied. All five packaged-artifact tests passed. Verified ZIP integrity, intended class bytes, absence of packaged test dependencies, exact public AdvancedCore snapshot identity and the isolated 20-file PR scope. Build used bounded local JVM heaps after an environment OOM. The JAR is 10,632,801 bytes, exceeding the advisory 10 MiB limit by 147,041 bytes. No native Minecraft client/Paper/Folia gameplay verification is claimed.
Fresh independent source-only review of the final base-to-HEAD diff: No findings.