Skip to content

Fix domain capability checks on the user edit form#297

Merged
rimas-kudelis merged 1 commit into
vexim:masterfrom
kaluang:fix/user-edit-capability-checks
Jun 8, 2026
Merged

Fix domain capability checks on the user edit form#297
rimas-kudelis merged 1 commit into
vexim:masterfrom
kaluang:fix/user-edit-capability-checks

Conversation

@kaluang

@kaluang kaluang commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

The checks in adminuserchangesubmit.php that decide whether on_piped,
on_avscan and on_spamassassin may be enabled use a single =, so they
assign 1 to $row[...] and the condition is always true. The add form
(adminuseraddsubmit.php) uses == and behaves as intended.

on_piped is the main issue: a piped user's smtp value is used directly
as the pipe_transport command in the virtual_domains router, so while
the check is broken a domain admin can switch a user to piped delivery
even when the domain doesn't have pipe enabled. on_avscan and
on_spamassassin are less of an issue because the routers also check the
domain flag, but they have the same typo.

Changes the three comparisons to ==.

The checks in adminuserchangesubmit.php that decide whether on_piped,
on_avscan and on_spamassassin may be enabled use a single `=`, so they
assign 1 to $row[...] and the condition is always true. The add form
(adminuseraddsubmit.php) uses `==` and behaves as intended.

on_piped is the main issue: a piped user's smtp value is used directly
as the pipe_transport command in the virtual_domains router, so while
the check is broken a domain admin can switch a user to piped delivery
even when the domain doesn't have pipe enabled. on_avscan and
on_spamassassin are less of an issue because the routers also check the
domain flag, but they have the same typo.

Changes the three comparisons to ==.
@rimas-kudelis

Copy link
Copy Markdown
Collaborator

Hi, thanks for this PR!

Not sure I'm a fan of this intended approach at all though:

  • first of all, I think we should check for on_piped in Exim too. If we don't, it's a bug
  • secondly, overwriting submitted values seems a bit wrong. IMO, we should probably just ignore them and either hide the fields in question, or render them disabled in the UI (I haven't checked, maybe we do that already).

Anyway, current code is wrong, so I'm merging this. Thanks again!

@rimas-kudelis rimas-kudelis merged commit ac49637 into vexim:master Jun 8, 2026
@kaluang

kaluang commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, and agreed on both!

I opened a new PR #300 for the Exim side: a virtual_piped router that only pipes when on_piped and the domain's pipe flag are both set, plus forbid_pipe on virtual_domains. The forms already hide those checkboxes when the domain lacks the capability, so this just adds the server-side guarantee. (Left the catchall/group routers as-is since their data isn't the raw smtp field. I would suggest putting this change somewhere into the Readme to notify existing admins that they should implement this manually on existing installations

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.

2 participants