-
Notifications
You must be signed in to change notification settings - Fork 389
fix(security): exclude locked properties from API response and validate property presence #1863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: rnwood <1327895+rnwood@users.noreply.github.com>
📊 Code Coverage Report
📁 Coverage by Namespace
Overall Coverage Details:
Enhanced coverage report with namespace breakdown - Azure DevOps Build #3.9.1-ci20250912106_pr1863 20250912.7 |
@copilot do the same for user passwords. Also you need to think about what happens when the UI posts back other changes to non locked properties. currently your change will throw an error because the placeholder value will be sent back. |
…OST requests Co-authored-by: rnwood <1327895+rnwood@users.noreply.github.com>
I've implemented both requested changes in commit 53a602c:
The fix ensures that:
Screenshot shows the Users tab with hidden passwords: https://github.com/user-attachments/assets/9d2e3ba9-7094-4290-b120-501d31ab5520 |
@copilot I would prefer a different method here. Client to send back only the top level properties that are to be modified. The server should then use the presence or non presence of the property in the response (based on the key not the value) to determine if the the property is processed (and rejected if it is locked). |
…ation Co-authored-by: rnwood <1327895+rnwood@users.noreply.github.com>
I've implemented the requested approach in commit e365c10. The solution now: GET endpoint: Completely excludes locked properties from the JSON response instead of sending placeholder values POST endpoint: Detects presence of locked sensitive properties ( This is cleaner and more secure than the placeholder approach since:
The implementation focuses on the actual security-sensitive properties (passwords) rather than trying to validate all locked properties, which avoids issues with default values from model binding. |
This PR addresses a security vulnerability where SMTP relay and user credentials were visible to end users when settings are locked (
LockSettings = true
). Users could inspect the HTML and change password field types to reveal actual password values, defeating the purpose of locking settings for security.Problem
When relay or user settings were locked via command line options or environment variables, the actual password values were still being sent from the API to the frontend. This allowed users to:
type="password"
totype="text"
on the password input fieldThis security vulnerability made the settings lock feature ineffective for protecting sensitive credentials.
Solution
Modified the
ServerController.GetServer()
andServerController.UpdateServer()
methods to implement property presence-based validation. The fix:relayPassword
,relayLogin
, user passwords) and rejects requests with 401 UnauthorizedSecurity Validation
Before: Users could reveal actual passwords using browser dev tools
After: Locked properties are not sent to client, and attempts to modify them are rejected
The approach uses property presence detection for cleaner and more secure validation:
Relay Settings:

User Settings:

Fixes #1524.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.