Skip to content

fix(codes): refuse an invite code created with no uses to offer - #844

Closed
darbyjack wants to merge 4 commits into
masterfrom
fix/invite-code-usage-validation
Closed

darbyjack wants to merge 4 commits into
masterfrom
fix/invite-code-usage-validation

Conversation

@darbyjack

@darbyjack darbyjack commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

/guild code create takes the number of uses as an argument and never looked at it. A guild master could ask for a code with zero uses or with a negative number, and the code was created.

Both are dead on arrival. Guild.getActiveCodes() keeps the codes with more than zero uses, so such a code never counts towards the active code limit and never shows up in /guild code list, and GuildHandler.handleInvite refuses to redeem a code with no uses left. The player is told the code is out of uses about a code the plugin accepted a minute earlier.

The negative count is the worse of the two. Since the code is filtered out of the active list, it does not count towards codes.amount either, so a guild could create as many of them as it wanted. A scratch test against the current code created five codes with -1 uses and left the guild with stored=5 active=0 atLimit(2)=false.

The fix

GuildCode.MIN_USES is 1, and both sides of the creation path use it.

CommandCode.create refuses a lower count with the new codes.invalid-uses message, before the active code limit check and before anything is stored. Guild.addCode throws IllegalArgumentException for the same input, so no other caller can put an impossible code into a guild.

Redeeming, /guild code list, /guild code info and the "out of uses" refusal are unchanged. A code that has been redeemed down to zero is a normal spent code and is left alone. /guild code create with no argument still makes a code with one use, and /guild code create 2 still makes a code with two.

codes.invalid-uses went into en-US.yml, the source file crowdin.yml points at. The other locale files are Crowdin-managed translations and pick the key up on the next sync, the way admin.cant-remove-master and transfer.failed did in #805. Until a locale has been synced, ACF resolves the key through the default locale, which is en-US with the shipped settings.messagesLanguage, so a player gets the English string. A server that points settings.messagesLanguage at a locale that has not been synced yet sees the key placeholder until Crowdin catches up.

Tests

New unit test src/test/java/me/glaremasters/guilds/guild/GuildCodeUsageTest.java:

  • a code with 0 uses is refused, nothing is stored, the active code limit is untouched
  • a code with -1 uses is refused the same way
  • after both refusals the guild can still create as many codes as the limit allows
  • a code with 1 use is created and stops being active once it has been redeemed
  • a code with 2 uses can be redeemed twice
  • Guild.addCode refuses a count below the minimum

New E2E tests in src/test/e2e/tests/codes.spec.ts:

  • a code with no uses is refused and nothing is stored
  • a code with a negative number of uses is refused and nothing is stored
  • a refused code does not use up the guild allowance of codes
  • a code with two uses can be redeemed twice

Results

./gradlew test testJava11 spotlessCheck   BUILD SUCCESSFUL
./gradlew check                            BUILD SUCCESSFUL
./gradlew plugwrightTest                   148 passed, 0 failed, 0 skipped

The 6 new unit tests and the 4 new E2E tests are part of those runs. check includes the 1.8.8 legacy Material probe, which reports "2 Material constant(s) checked against 1.8.8, 0 allow-listed".

Before the fix, the new unit test failed the way the defect predicts:

GuildCodeUsageTest > a code with no uses is refused and nothing is stored FAILED
    AssertionFailedError: a code with no uses must be refused ==> Expected
    me.glaremasters.guilds.exceptions.ExpectationNotMet to be thrown, but nothing was thrown.

GuildCodeUsageTest > the guild refuses to store a code with no uses of its own accord FAILED
    AssertionFailedError: a code with no uses cannot be stored ==> Expected
    java.lang.IllegalArgumentException to be thrown, but nothing was thrown.

Alongside #849

#849 (fix/invite-code-membership-consistency) works on the other end of the same feature. It rewrites Guild.addMember and Guild.addMemberByCode to share one registration, fires GuildJoinEvent before a code is consumed, and refuses a redeem while the join cooldown runs.

Neither change reaches this fix. The two guards here sit in CommandCode.create and in Guild.addCode. #849 changes Guild.addMember, Guild.addMemberByCode and GuildHandler.handleInvite, and adds a cooldown check to the code redeem subcommand. The redeem path keeps its own code.getUses() <= 0 check for a spent code, which #849 leaves alone and this PR does not touch.

I merged both branches in a scratch clone to be sure of that. Guild.java, CommandCode.kt and tests/codes.spec.ts merge without a conflict, and the only file that does is docs/e2e-coverage.md. The merged tree keeps both guards:

src/main/java/me/glaremasters/guilds/guild/Guild.java:417:        if (uses < GuildCode.MIN_USES) {
src/main/kotlin/me/glaremasters/guilds/commands/codes/CommandCode.kt:46:        if (uses < GuildCode.MIN_USES) {

The unit tests of both PRs pass together in that merged tree (22 tests, 6 from here and 16 from #849), and the merged suite passes end to end: 150 passed, 0 failed, 0 skipped.

One note on the CI run: the Plugwright job failed once on a homes test whose bot connection had already ended (socketClosed), in a file this PR does not touch. The re-run of the same job passed, and the suite passes locally as well.

The coverage inventory is the file that needs a decision, because the open pull requests add tests to the suite and each of them counts them. This copy holds the canonical numbers the merged file will have whatever order they land in: 162 tests across 14 spec files in the header, 18 in the invite codes section, 21 in the guild lifecycle and membership sections and 8 in the admin commands section. The tables carry the rows for #849's, #848's and #851's tests, since #848 and #851 do not touch this file at all, and every test in the merged suite is listed once. The two sections this branch does not own are placeholders here, a heading line each for the startup loading spec (4 tests) and the runtime smoke suite (7 tests), with their tables left to the pull requests that wrote them, so the headings in this copy add up to 162 on their own. Merging will conflict on this file, and the resolution is the tables as they stand here.

Compatibility

  • No new config key, no new dependency, no change to a serialized format.
  • Java 11 bytecode is unaffected and testJava11 passes.
  • No Material constant is touched, so the 1.8.8 probe has nothing new to check.
  • Permissions are unchanged. The refusal happens inside the same handler that already required guilds.command.code.create and the CREATE_CODE role, so a player without them is stopped before the count is looked at.

Guild.addCode is public and now throws

Guild.addCode(code, uses, creator) is a public method on a public class. It now throws IllegalArgumentException when uses is below 1, where it used to store the code. A plugin that called it with zero or a negative number used to get a stored code that could never be redeemed, which is the defect this PR fixes, so callers outside Guilds have to check their value before calling.

The alternative was to keep the guard in the command handler alone. That covers the plugin's own path and leaves the public contract alone, but GuildsAPI hands out Guild objects, so a third-party plugin could still store a code with no uses and open the same hole. I kept the model-level guard because a code with no uses is invalid everywhere, not only at the command line. If you would rather have the command-only version, it is the removal of one guard and the matching test.

Limitations

  • There is no upper bound on the number of uses. codes.amount limits how many active codes a guild may hold, not how many uses one code may have, and the config has no key for a per-code maximum. A code with a very large count is bounded in practice by the guild's member limit, since every redemption adds a member. A maximum would need a new config key and would change the behaviour of servers that legitimately hand out codes with many uses, so I left it out.
  • Codes that were stored with zero or negative uses before this fix stay in the guild data. They are inert, they cannot be redeemed, and they still appear in /guild code list while codes.list-inactive-codes is on. Removing them on load would be silent data loss.
  • The wording of the new message is Crowdin's to translate. Until a locale is synced, players see the English string.

/guild code create takes the number of uses as an argument and never looked
at it, so a guild master could ask for zero uses or for a negative number
and the code was created.

Both kinds are dead on arrival. Guild.getActiveCodes() keeps the codes with
more than zero uses, so such a code was invisible to the active code limit
and to /guild code list, and GuildHandler.handleInvite refuses to redeem a
code with no uses left. The player is told the code is out of uses about a
code that was accepted a minute earlier. The negative case is worse: because
the code is filtered out of the active list, it does not count towards
codes.amount either, so a guild could create as many of them as it liked.

GuildCode.MIN_USES is 1 and both sides of the creation path use it.
CommandCode.create refuses a lower count with the new codes.invalid-uses
message before the limit check and before anything is stored, and
Guild.addCode throws IllegalArgumentException for the same input so no other
caller can put an impossible code into a guild. A code that has been
redeemed down to zero is a normal spent code and is left alone.

The message was added to en-US.yml, the source crowdin.yml points at. The
other locale files are Crowdin-managed translations and pick the key up on
the next sync.

Adds a unit test for the rule and E2E coverage for the refusals, the
allowance of codes and a code that can be redeemed twice.
t3 added 3 commits October 22, 2026 12:07
…land

#844 and #849 both add tests to tests/codes.spec.ts, so the counts in the
coverage inventory have to be the ones the merged file will have: 18 in the
invite codes section and 147 in the suite total. The table carries the rows
from both pull requests, with no row twice.

Nothing else in the inventory changes.
The coverage inventory is contended between the open pull requests, and the
header and the per-section counts have to be the ones the merged file will
have whatever order they land in: 160 tests across 14 spec files, 18 in the
invite codes section, 21 in the guild lifecycle and membership sections and 8
in the admin commands section.

The rows for the tests that #848 and #851 add are in the tables here, since
neither of those pull requests touches this file, and the rows for #849's
tests were already here. Every test in the merged suite is listed once.

The membership section heading said 19 while the spec file holds 20 tests,
which is where the 21 comes from.
The smoke suite grew two recovery checks after the 160 figure was worked
out, so smoke.spec.ts holds 7 tests rather than 5 and the merged total is
162 across 14 spec files.

The two sections this branch does not own are placeholders: a heading line
each for the startup loading spec (4 tests) and the runtime smoke suite
(7 tests), with their tables left to the pull requests that wrote them. The
section headings in this copy now add up to 162.
@darbyjack darbyjack closed this Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant