Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 11, 2025

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:

  1. Open browser developer tools
  2. Change type="password" to type="text" on the password input field
  3. View the actual password in plain text

This security vulnerability made the settings lock feature ineffective for protecting sensitive credentials.

Solution

Modified the ServerController.GetServer() and ServerController.UpdateServer() methods to implement property presence-based validation. The fix:

  • GET endpoint: Completely excludes locked properties from the JSON response instead of sending any placeholder values
  • POST endpoint: Detects presence of locked sensitive properties (relayPassword, relayLogin, user passwords) and rejects requests with 401 Unauthorized
  • Extended to user passwords: Now also hides user passwords when user settings are locked
  • Property presence validation: Uses key presence detection rather than value comparison to determine if locked properties are being modified
  • Preserves normal functionality when settings are unlocked
  • Requires no frontend changes as the UI naturally adapts to missing properties in API response

Security 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:

  • GET requests: Locked properties are completely excluded from the JSON response
  • POST requests: Any presence of locked sensitive properties results in immediate rejection

Relay Settings:
Fixed relay settings showing hidden password

User Settings:
Fixed user settings showing hidden passwords

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.

Co-authored-by: rnwood <1327895+rnwood@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Smtp creds are visible to end user - Security Risk fix(security): hide relay password when settings are locked Sep 11, 2025
@Copilot Copilot AI requested a review from rnwood September 11, 2025 17:40
Copilot finished work on behalf of rnwood September 11, 2025 17:40
@rnwood
Copy link
Owner

rnwood commented Sep 11, 2025

📊 Code Coverage Report

Coverage Type Percentage Status
Line Coverage 12.7% 🔴
Branch Coverage 3.7% 🔴
Method Coverage 15.3% 🔴

📁 Coverage by Namespace

Namespace Line Coverage Branch Coverage Method Coverage Classes
LumiSoft.Net 0% 🔴 0% 🔴 0% 🔴 23
LumiSoft.Net.AUTH 0% 🔴 0% 🔴 0% 🔴 18
LumiSoft.Net.IMAP 1.7% 🔴 0.4% 🔴 3.9% 🔴 122
LumiSoft.Net.IMAP.Client 0% 🔴 0% 🔴 0% 🔴 8
LumiSoft.Net.IMAP.Server 0% 🔴 0% 🔴 0% 🔴 25
LumiSoft.Net.IO 0% 🔴 0% 🔴 0% 🔴 13
LumiSoft.Net.Log 0% 🔴 0% 🔴 0% 🔴 3
LumiSoft.Net.Mail 0% 🔴 0% 🔴 0% 🔴 14
LumiSoft.Net.MIME 0% 🔴 0% 🔴 0% 🔴 41
LumiSoft.Net.TCP 0% 🔴 0% 🔴 0% 🔴 6
LumiSoft.Net.UDP 0% 🔴 0% 🔴 0% 🔴 3
Rnwood.Smtp4dev 2.1% 🔴 3.5% 🔴 11.4% 🔴 14
Rnwood.Smtp4dev.ApiModel 59.1% 🔴 54.7% 🔴 45.6% 🔴 17
Rnwood.Smtp4dev.Controllers 3.5% 🔴 4.8% 🔴 9.7% 🔴 10
Rnwood.Smtp4dev.Data 55.4% 🔴 40.9% 🔴 61.1% 🟡 3
Rnwood.Smtp4dev.DbModel 70.8% 🟡 0% 🔴 65.9% 🟡 5
Rnwood.Smtp4dev.DbModel.Projections 100% 🟢 0% 🔴 100% 🟢 1
Rnwood.Smtp4dev.Hubs 0% 🔴 0% 🔴 0% 🔴 1
Rnwood.Smtp4dev.Migrations 91.1% 🟢 0% 🔴 75% 🟡 20
Rnwood.Smtp4dev.Server 8.5% 🔴 3.9% 🔴 11% 🔴 10
Rnwood.Smtp4dev.Server.Imap 84.8% 🟢 85.3% 🟢 78.9% 🟡 2
Rnwood.Smtp4dev.Server.Settings 45.1% 🔴 25% 🔴 46.9% 🔴 10
Rnwood.Smtp4dev.Service 0% 🔴 0% 🔴 0% 🔴 1
Rnwood.SmtpServer 71% 🟡 63.1% 🟡 67.6% 🟡 35
Rnwood.SmtpServer.Extensions 67.9% 🟡 25% 🔴 91.3% 🟢 6
Rnwood.SmtpServer.Extensions.Auth 59% 🔴 47.6% 🔴 57.5% 🔴 23
Rnwood.SmtpServer.Verbs 92.9% 🟢 92.3% 🟢 100% 🟢 12

Overall Coverage Details:

  • Covered Lines: 4778 / 37476
  • Covered Branches: 412 / 11118
  • Covered Methods: 577 / 3753
  • Assemblies Tested: 3
  • Total Classes: 446

📈 View Full Coverage Report

Enhanced coverage report with namespace breakdown - Azure DevOps Build #3.9.1-ci20250912106_pr1863 20250912.7

@rnwood
Copy link
Owner

rnwood commented Sep 11, 2025

@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>
Copy link
Contributor Author

Copilot AI commented Sep 11, 2025

@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...

I've implemented both requested changes in commit 53a602c:

  1. Extended password hiding to user passwords: When user settings are locked via command line, user passwords now show "***" instead of actual values, just like relay passwords.

  2. Fixed placeholder value handling in POST requests: Added logic to detect when "***" placeholder values are submitted back and preserve the original passwords instead of trying to save the placeholder value.

The fix ensures that:

  • GET requests return "***" for locked passwords (both relay and user)
  • POST requests detect "***" values and preserve original passwords
  • No errors occur when forms are submitted with placeholder values
  • All locked settings remain properly secured

Screenshot shows the Users tab with hidden passwords: https://github.com/user-attachments/assets/9d2e3ba9-7094-4290-b120-501d31ab5520

@Copilot Copilot AI changed the title fix(security): hide relay password when settings are locked fix(security): hide passwords when settings are locked and handle placeholder POST values Sep 11, 2025
Copilot finished work on behalf of rnwood September 11, 2025 18:20
@rnwood
Copy link
Owner

rnwood commented Sep 12, 2025

@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>
Copy link
Contributor Author

Copilot AI commented Sep 12, 2025

@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 ...

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 (relayPassword, relayLogin, user passwords) and rejects requests with 401 Unauthorized if any are found

This is cleaner and more secure than the placeholder approach since:

  • No sensitive data is ever sent to the client
  • Client can only send properties that should be modifiable
  • Any attempt to include locked properties is immediately rejected

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.

@Copilot Copilot AI changed the title fix(security): hide passwords when settings are locked and handle placeholder POST values fix(security): exclude locked properties from API response and validate property presence Sep 12, 2025
Copilot finished work on behalf of rnwood September 12, 2025 06:10
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.

Smtp creds are visible to end user - Security Risk

2 participants