Address security issues in the signup module by vagisha · Pull Request #667 · LabKey/MacCossLabModules

vagisha · 2026-07-26T17:03:24Z

Rationale

This PR addresses the security issues found in the signup module following an internal review. It tightens validation on self-service group changes, makes signup responses consistent, improves auditing of administrative changes, and avoids surfacing internal mail errors to callers.

Security review findings (signup module)

The review flagged six items in the signup module. Four are addressed by this PR; the two rate-limiting items were already handled by the kaptcha work in #638.

[Low-Med] Self-service group change did not re-validate the resolved groups before changing membership, so a misconfigured transition rule could move a user into a more-privileged, site, or other-project group. Addressed here.
[Low] Signup responses differed depending on whether the submitted address already had an account. Addressed here.
[Low] Administrative configuration actions wrote no audit event on success (group-change rules and per-folder target group). Addressed here.
[Low] The unauthenticated signup endpoint could send mail without rate limiting. Addressed by kaptcha in Add kaptcha verification to signup module #638.
[Informational] No rate limiting on the unauthenticated signup email endpoints. Addressed by kaptcha in Add kaptcha verification to signup module #638.
[Informational] Mail-send errors were returned to the caller. Addressed here.

Related Pull Requests

Add kaptcha verification to signup module #638.

Changes

Group-change validation. The self-service group change re-validates the resolved groups before changing any membership: both must be project groups in the same project, and the target must not carry administrative permission in the project or any of its subfolders. Requests that don't meet these constraints are rejected with a distinct status, and successful moves are audited.
Consistent signup responses. The signup endpoints return the same response whether or not an address already has an account, including when email delivery fails. The owner of an existing address receives a notice pointing to the password-reset flow, and the existing account is never modified.
Auditing. The administrative configuration actions now write an audit event on success.
Error handling. Mail-send errors are logged server-side, and callers receive a generic message.
Test. Added SignUpGroupChangeSecurityTest, covering the group-change validation and its auditing.

Deployment note. Two DB-stored wiki pages on the skyline website need updating after deployment: the signup form should branch on status == 'SUCCESS', and the Skyline-daily register form should handle the TARGET_NOT_ALLOWED status returned by a rejected group change (plus a fallback for any unrecognized status).

Co-Authored-By: Claude noreply@anthropic.com

… that carries AdminPermission (admin rights) in any subfolder of the project, not only at the project root. Co-Authored-By: Claude <noreply@anthropic.com>

* SignUpApiAction and BeginAction now email the owner of an already-registered address (new sendExistingAccountEmail helper) instead of doing nothing. Both signup paths now send an email, so response timing no longer reveals whether an account exists. * The existing account is never modified, and send failures are logged and swallowed so the response stays identical to a new signup. The email points the owner to the "Forgot your password?" reset flow. * Renamed the SignUpApiAction success status from USER_ADDED to SUCCESS, which was misleading in the existing-account case where no user is added. The external signup wiki form must key its success branch off SUCCESS. Co-Authored-By: Claude <noreply@anthropic.com>

…ion status * ChangeGroupsApiAction returns TARGET_NOT_ALLOWED when validateGroupChangeTarget refuses a privileged, site, or cross-project target. NO_PERMISSIONS still covers callers that are ineligible for the transition. * Added SignUpGroupChangeSecurityTest. As a non-admin it plants each dangerous transition rule, then verifies the move is refused and membership unchanged, while a plain same-project move still succeeds. Co-Authored-By: Claude <noreply@anthropic.com>

…e group-change test * SignUpApiAction and BeginAction now run the existing-account and new-account branches through one try/catch, so a mail-send failure returns the same generic error for both. * SignUpGroupChangeSecurityTest now verifies through the auditLog schema that a successful self-service group change adds one "Client API Actions" event and a refused move adds none, comparing counts before and after each move since the event is recorded in the root container that outlives project cleanup. See ai/todos/active/TODO-LK-20260723_signup-security.md Co-Authored-By: Claude <noreply@anthropic.com>

* SignUpApiAction and BeginAction also catch SQLException from the new-account insert, so a DB failure returns the same generic error as any other and cannot reveal whether the account existed. * validateGroupChangeTarget drops the workbook-only container filter and its stale comment. The admin check is unchanged. * RemoveGroupChangeProperty now guards against a null rule map and removes the entry with the correct string key. Group-change audit events log group names instead of bare ids. * SignUpGroupChangeSecurityTest adds a case for a target that inherits admin via membership in an admin group. Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Pull request overview

This PR hardens the signup module by preventing privilege escalation through self-service group changes, making signup responses non-enumerable, improving audit logging of admin configuration changes, and ensuring mail-send failures don’t leak internal details to callers.

Changes:

Adds server-side validation for self-service group transitions to block moves into site groups, cross-project groups, or any group with administrative permission anywhere in the project tree; successful moves are now audited.
Makes signup responses uniform whether or not the email already has an account (and logs internal mail errors while returning a generic message).
Adds an integration test (SignUpGroupChangeSecurityTest) that verifies group-change validation and audit behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
signup/src/org/labkey/signup/SignUpController.java	Adds audit logging, uniform signup behavior, and target validation for self-service group moves.
signup/test/src/org/labkey/test/tests/signup/SignUpGroupChangeSecurityTest.java	New test covering group-change security validation and auditing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

vagisha · 2026-07-26T22:38:34Z

            String existingRules = m.get(String.valueOf(oldgroup));
+            if(existingRules == null) // no rules configured for this source group - nothing to remove
+                return false;


The null guard was added only to prevent an NPE when the source group has no rules. The false return matches the branch just below it, where !rules.contains(newgroup) also returns false when the rule isn't present, so this keeps the two "nothing to remove" cases consistent rather than changing pre-existing behavior. The action is only reached from the site-admin Remove link on rules that currently exist, so the "nothing to remove" path is a rare edge such as a stale page or double-submit.

Returning false without adding to the errors collection may result in a blank 200/success page. Consider calling errors.addErrors() for these two return false; branches if you think it's likely enough to want to help the user.

…dit message, matching the admin-config audit events

labkey-jeckels · 2026-07-27T22:52:07Z

            String existingRules = m.get(String.valueOf(oldgroup));
+            if(existingRules == null) // no rules configured for this source group - nothing to remove
+                return false;


Returning false without adding to the errors collection may result in a blank 200/success page. Consider calling errors.addErrors() for these two return false; branches if you think it's likely enough to want to help the user.

labkey-jeckels · 2026-07-27T22:54:53Z

+        // Reject a target group with admin permission anywhere in the project.
+        for (Container c : ContainerManager.getAllChildren(project))
+        {
+            if (c.hasPermission(newgroup, AdminPermission.class))


This protects against admin escalation. Is that enough, or do you also need to check for trying to gain Editor or similar?

On skyline.ms the target group has read-only access, so anything more than that should not be required.
Fixed in 6aaeeb3. validateGroupChangeTarget now rejects a target that grants write (Insert/Update/Delete) or admin permissions anywhere in the project tree.

labkey-jeckels · 2026-07-27T23:00:14Z

+                // site, or other-project). Report this with its own status so a refused misconfiguration is
+                // distinguishable from an ineligible caller (NO_PERMISSIONS above). The specific reason is
+                // logged server-side only, not returned to the caller.
+                response.put("status", "TARGET_NOT_ALLOWED");


Reveals the distinction between groups that exist and don't. Probably acceptable if it gives a usability win on the client side.

There isn't a client-side win, since we would show the same message that is shown for NO_PERMISSIONS. I collapsed the rejection to NO_PERMISSIONS. Fixed in 6aaeeb3

labkey-jeckels · 2026-07-27T23:01:52Z

+        if (_plantedRules.isEmpty())
+            return;
+
+        ensureSignedInAsPrimaryTestUser();


Normally we'd avoid something like this in an @After because it prevents collecting the right screenshots as artifacts (since it does browser navigation). However, I think it's OK here because this is all API-testing, so there's no browser state worth capturing, right?

Right. This test is entirely API-driven so there is no browser state worth capturing on failure.

* validateGroupChangeTarget now rejects a target that grants write (Editor/Author/Submitter) or admin access. * Refused moves return the generic NO_PERMISSIONS instead of a distinct TARGET_NOT_ALLOWED, so the response no longer reveals which configured rules are misconfigured (the reason is logged server-side only). * RemoveGroupChangeProperty adds a validation error on each no-op path instead of returning a blank success page. * Added a SignUpGroupChangeSecurityTest case for a write-enabled (Editor) target. Co-Authored-By: Claude <noreply@anthropic.com>

vagisha and others added 6 commits July 24, 2026 17:56

Fix security issues in signup module.

c5a2963

SignUpController.validateGroupChangeTarget now rejects a target group…

bd77688

… that carries AdminPermission (admin rights) in any subfolder of the project, not only at the project root. Co-Authored-By: Claude <noreply@anthropic.com>

vagisha requested a review from Copilot July 26, 2026 20:21

Copilot started reviewing on behalf of vagisha July 26, 2026 20:21 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Removed unused import

9dbfa81

vagisha marked this pull request as ready for review July 26, 2026 22:56

vagisha requested a review from labkey-jeckels July 26, 2026 22:56

SignUpController: added group ids to the self-service group-change au…

7919517

…dit message, matching the admin-config audit events

labkey-jeckels approved these changes Jul 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Address security issues in the signup module - #667

Address security issues in the signup module#667
vagisha wants to merge 9 commits into
release26.7-SNAPSHOTfrom
26.7_fb_signup-security

vagisha commented Jul 26, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

vagisha Jul 26, 2026

Uh oh!

labkey-jeckels Jul 27, 2026

Uh oh!

Uh oh!

labkey-jeckels Jul 27, 2026

Uh oh!

labkey-jeckels Jul 27, 2026

Uh oh!

vagisha Jul 28, 2026

Uh oh!

labkey-jeckels Jul 27, 2026

Uh oh!

vagisha Jul 28, 2026

Uh oh!

labkey-jeckels Jul 27, 2026

Uh oh!

vagisha Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

vagisha commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Security review findings (signup module)

Related Pull Requests

Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vagisha commented Jul 26, 2026 •

edited

Loading