Skip to content

Conversation

simonLeary42
Copy link
Collaborator

@simonLeary42 simonLeary42 commented Oct 14, 2025

@simonLeary42 simonLeary42 requested a review from Copilot October 14, 2025 15:21
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Disable submit buttons on click to reduce accidental double form submissions by users.

  • Replace inline confirm-return handlers with inline logic that submits the form and disables the button.
  • Add onclick handlers to various submit buttons to call form.submit() and disable the button.

Reviewed Changes

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

Show a summary per file
File Description
webroot/panel/pi.php Replaced confirm-return with inline JS to submit and disable for Approve/Deny.
webroot/panel/new_account.php Added onclick handlers to Cancel/Request buttons to submit and disable.
webroot/panel/modal/new_pi.php Added onclick to Send Request to submit and disable.
webroot/panel/modal/new_key.php Added onclick to Add/Import Key buttons to submit and disable.
webroot/panel/groups.php Added onclick to Leave Group to submit and disable.
webroot/panel/account.php Added onclick for multiple actions (cancel/request PI, delete key, set shell, request deletion).
webroot/admin/pi-mgmt.php Added confirm/submit/disable patterns for Approve/Deny.
webroot/admin/ajax/get_group_members.php Added onclick to Approve/Deny/Remove to submit and disable.
resources/templates/header.php Added onclick to Return to My User to submit and disable.
Comments suppressed due to low confidence (4)

webroot/panel/pi.php:1

  • This handler can double-submit and never disables the button: form.submit() returns undefined (falsy), so the && chain short-circuits before setting disabled; additionally, because the handler does not return a value, the browser proceeds with the default submit after the programmatic submit, resulting in two submissions and bypassing form onsubmit/HTML5 validation. Fix by not calling form.submit() and returning the confirm result after disabling, for example: onclick='if (!confirm("Are you sure you want to approve $uid?")) return false; this.disabled = true; return true;'
<?php

webroot/panel/pi.php:1

  • Same issue as with Approve: form.submit() returns undefined, so disabled never executes; and without returning a value, the default submit also fires, causing potential double-submit and skipping onsubmit/HTML5 validation. Use: onclick='if (!confirm("Are you sure you want to deny $uid?")) return false; this.disabled = true; return true;'
<?php

webroot/admin/pi-mgmt.php:1

  • form.submit() returns undefined, so the && chain prevents setting disabled; and because the handler doesn't return a value, the browser's default submit also runs—potential double-submit and skipped onsubmit/validation. Prefer: onclick='if (!confirm("Are you sure you want to approve $uid?")) return false; this.disabled = true; return true;' (apply the same fix to the Deny button at lines 86–90).
<?php

webroot/panel/account.php:1

  • The && pattern is incorrect here: submit() returns undefined (falsy), so disabled is never set; and without returning a value, the default submit also fires—double-submit and bypassed onsubmit/validation. Replace with: onclick='if (!confirm("Are you sure you want to cancel this request?")) return false; this.disabled = true; return true;' (same for the 'Request PI Account' block at lines 197–201).
<?php

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

<input type='hidden' name='form_type' value='removePIForm'>
<input type='hidden' name='pi' value='" . $group->gid . "'>
<input type='submit' value='Leave Group'>
<input type='submit' value='Leave Group' onclick='this.form.submit(); this.disabled=true;'>
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling this.form.submit() in onclick bypasses the form's onsubmit handler at line 150 (the confirmation) and HTML5 validation, and the handler doesn't prevent the default, so the browser will also perform the native submit—risking double-submit. Remove the programmatic submit and let the form submit naturally after disabling: onclick='this.disabled = true; return true;'

Suggested change
<input type='submit' value='Leave Group' onclick='this.form.submit(); this.disabled=true;'>
<input type='submit' value='Leave Group' onclick='this.disabled=true; return true;'>

Copilot uses AI. Check for mistakes.

style='margin-top: 10px;'
type='submit'
value='Cancel Request'
onclick='this.form.submit(); this.disabled=true;'
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Programmatic submit inside onclick bypasses onsubmit/HTML5 validation and, since the handler does not return false, the native submit also occurs—potential double-submit. Prefer: onclick='this.disabled = true; return true;' (same applies to the Request Account button at lines 158–163).

Suggested change
onclick='this.form.submit(); this.disabled=true;'
onclick='this.disabled = true; return true;'

Copilot uses AI. Check for mistakes.

<div class="searchWrapper" style="display: none;"></div>
</div>
<input type="submit" value="Send Request">
<input type="submit" value="Send Request" onclick="this.form.submit(); this.disabled=true;">
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.form.submit() bypasses onsubmit handlers and constraint validation, and because the handler doesn't return false, the default submit also runs—potential double-submit. Use: onclick='this.disabled = true; return true;'

Suggested change
<input type="submit" value="Send Request" onclick="this.form.submit(); this.disabled=true;">
<input type="submit" value="Send Request" onclick="this.disabled = true; return true;">

Copilot uses AI. Check for mistakes.

Comment on lines 38 to 40
onclick="this.form.submit(); this.disabled=true;"
disabled
/>
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this button is initially disabled, the onclick pattern is unsafe when enabled: form.submit() bypasses onsubmit/validation and may cause double-submit because the default action also fires. Replace with: onclick='this.disabled = true; return true;' (apply the same change to the Import Key button at lines 49–51 and Import Key(s) at lines 66–68).

Copilot uses AI. Check for mistakes.

Comment on lines 74 to 85
<input
type='submit'
name='action'
value='Approve'
onclick='this.form.submit(); this.disabled=true;'
/>
<input
type='submit'
name='action'
value='Deny'
onclick='this.form.submit(); this.disabled=true;'
/>
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using form.submit() here bypasses onsubmit/validation and, without returning false, the native submit still occurs—risking duplicate submissions. Change both to: onclick='this.disabled = true; return true;' (the Remove button at line 48 should be updated similarly).

Copilot uses AI. Check for mistakes.

<input
type='submit'
value='Return to My User'
onclick='this.form.submit(); this.disabled=true;'
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Programmatic submit bypasses onsubmit/validation and the handler doesn't prevent the default, so the native submit also occurs—risking double-submit. Use: onclick='this.disabled = true; return true;'

Suggested change
onclick='this.form.submit(); this.disabled=true;'
onclick='this.disabled=true; return true;'

Copilot uses AI. Check for mistakes.

<input
type='submit'
value='&times;'
onclick='this.form.submit(); this.disabled=true;'
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling form.submit() in onclick bypasses onsubmit and HTML5 validation, and because the handler doesn't return false, the native submit runs too—risking double-submit. Change to: onclick='this.disabled = true; return true;' (also apply to 'Set Login Shell' at lines 250–255 and 'Request Account Deletion' at lines 288–291).

Copilot uses AI. Check for mistakes.

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