Repository navigation
Hash SignUp confirmation keys and expire signup links - #682
Merged
Merged
Conversation
* Stored only a SHA-256 hash of the confirmation key in signup.temporaryuser, with a keyTimeout seven days out * Changed a repeat signup to replace the key and the stored name, organization and folder * Cleared the key when the account is created, so each link works once * Added the link expiration to the confirmation email * Masked the key in the admin ConfirmationURL column, and left the column empty for used keys * Added signup-26.000-26.001.sql, which hashes pending keys, sets them to expire seven days after upgrade, and clears used keys * Added SignUpManager unit tests Ports the token hashing and expiration from LabKey/platform#8211. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Confirmation-key validation and consumption are not atomic, allowing a concurrent repeat signup to revoke a key that still succeeds in creating an account.
1 open issue
What changed in this PR
Extends the platform’s verification-token protections to SignUp by hashing confirmation keys, adding seven-day expiration, and clearing keys after account creation.
Changes:
- Replaces keys and signup details on repeat signup, and adds lifecycle tests.
- Includes expiration in confirmation emails and masks keys in the admin view.
- Migrates existing keys to hashes and clears keys for completed signups.
| File | Description |
|---|---|
| signup/src/org/labkey/signup/TempUser.java | Adds the key-expiration field and documents hashed storage. |
| signup/src/org/labkey/signup/SignUpSchema.java | Masks confirmation keys and hides used links. |
| signup/src/org/labkey/signup/SignUpModule.java | Advances the schema version and registers unit tests. |
| signup/src/org/labkey/signup/SignUpManager.java | Implements key hashing, expiration, replacement, consumption, and tests. |
| signup/src/org/labkey/signup/SignUpController.java | Integrates key management and expiration-aware emails. |
| signup/resources/schemas/signup.xml | Declares the key-expiration column. |
| signup/resources/schemas/dbscripts/postgresql/signup-26.000-26.001.sql | Migrates existing keys and assigns pending-link expiration. |
Review effort: Balanced
* Added SignUpManager.clearKeyIfValid, which clears the key only if it still matches and has not expired, and changed ConfirmAction to call it first in the account transaction and roll back when it returns false * Changed a repeat signup to replace only the key, keeping the first signup's name, organization and folder * Replaced the full-row saves in saveWithNewKey and setLabkeyUserId with Table.update calls that set only the changed columns * Changed the ConfirmationURL column to show the masked link only for unused, unexpired keys, in the grid, exports and the JSON API * Moved SignUpManager.TestCase to getIntegrationTests, since it needs the database * Added tests for replaced, used and expired keys Co-Authored-By: Claude <noreply@anthropic.com>
vagisha
marked this pull request as ready for review
October 26, 2026 21:27
labkey-adam
approved these changes
Oct 27, 2026
* Marked the return value and the key parameter @nullable Co-Authored-By: Claude <noreply@anthropic.com>
labkey-adam
added a commit
to LabKey/platform
that referenced
this pull request
Oct 28, 2026
## Rationale Update a few methods to take `@NotNull` link timeouts. The only caller who passed `null` has been migrated. Also, test a couple invariants in `HasPermissionOption.ALL.accept()`. ## Related Pull Requests - LabKey/MacCossLabModules#682
XingY
pushed a commit
to LabKey/platform
that referenced
this pull request
Oct 29, 2026
## Rationale Update a few methods to take `@NotNull` link timeouts. The only caller who passed `null` has been migrated. Also, test a couple invariants in `HasPermissionOption.ALL.accept()`. ## Related Pull Requests - LabKey/MacCossLabModules#682
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale
Store only a hash of the SignUp confirmation key, expire signup links after seven days, and let each link be used once, so that the contents of
signup.temporaryuseralone cannot be used to create an account for a pending signup. This applies the approach of LabKey/platform#8211 to the SignUp module.Related Pull Requests
Changes
keyTimeoutset toLoginManager.SETUP_LINK_TIMEOUT(seven days)ConfirmationURLcolumn masks the key, and is empty once the key is used or has expiredsignup-26.000-26.001.sqlclears the keys of signups that already have an account, and hashes the remaining keys in place with an expiry seven days after upgradeSignUpManager.TestCaseintegration tests cover hashed storage, wrong, expired, replaced and used keys, and repeat signupsCo-Authored-By: Claude noreply@anthropic.com