Add managed server hostname setting#46
Merged
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@CodeRabbit help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
templates/network_file_sharing.html (1)
424-434: 💤 Low value
data.hostnamefallback may be dead code — add a brief comment if intentionalcurrentServerName = data.server_name || data.hostname || '';The API contract (
GET /api/server_identity) returns aserver_namefield. Ifdata.hostnameis a fallback for an older or alternative API response shape, a one-line comment explaining the intent would avoid confusion months from now. If it's genuinely unused, remove it. As per coding guidelines, sprinkle comments on things we might forget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@templates/network_file_sharing.html` around lines 424 - 434, In loadServerIdentity, clarify or remove the apparent dead fallback data.hostname used in the assignment to currentServerName (currentServerName = data.server_name || data.hostname || ''); either remove data.hostname if unused by the API, or add a one-line comment immediately above that assignment explaining that data.hostname is an intentional backward-compatibility fallback for older API responses; update the related references (serverNameValue and serverNameInput usage) only if you remove the fallback to ensure behavior remains consistent.tests/test_server_identity_routes.py (2)
8-55: ⚡ Quick winExtract shared fake-mode app setup into a pytest fixture to eliminate ~25 lines of duplication
Both test functions are identical from lines 9–36 / 59–86 (save/restore globals, patch env vars, reset runtime, create app, create admin user) — only the request body and assertions differ. A shared fixture removes the duplication and ensures both tests get the same environment.
♻️ Suggested fixture-based refactor
import pytest `@pytest.fixture` def fake_app_client(tmp_path): """Spin up a fake-mode app with a temp data dir and a single admin user. Resets the module-level runtime singletons so get_runtime() re-initialises from the patched environment variables on each call. """ import os from tempfile import TemporaryDirectory from unittest.mock import patch from simple_safer_server.services import runtime as _runtime_mod previous_runtime = _runtime_mod._runtime previous_fake_state = _runtime_mod._fake_state try: with TemporaryDirectory() as temp_dir: with patch.dict( os.environ, {"SSS_MODE": "fake", "SSS_SKIP_LOGIN": "true", "SSS_DATA_DIR": temp_dir}, clear=False, ): # Force get_runtime() to create a new Runtime from the patched env vars. _runtime_mod._runtime = None _runtime_mod._fake_state = None from simple_safer_server.app_factory import create_app app, _ = create_app() app.config["TESTING"] = True with app.app_context(): svc = app.extensions["simple_safer_server"] svc.config_manager.set_value("system", "setup_complete", "true") svc.config_manager.set_value("system", "username", "admin") ok, msg = svc.user_manager.create_user("admin", "password", is_admin=True) assert ok, msg with app.test_client() as client: yield app, client finally: _runtime_mod._runtime = previous_runtime _runtime_mod._fake_state = previous_fake_state def test_server_identity_api_updates_fake_mode_config(fake_app_client): app, client = fake_app_client response = client.put("/api/server_identity", json={"server_name": "Family-NAS"}) assert response.status_code == 200 assert response.get_json()["data"]["server_name"] == "family-nas" with app.app_context(): svc = app.extensions["simple_safer_server"] assert svc.config_manager.get_value("system", "server_name") == "family-nas" def test_server_identity_api_rejects_invalid_name_in_fake_mode(fake_app_client): _, client = fake_app_client response = client.put("/api/server_identity", json={"server_name": "bad name"}) assert response.status_code == 400 assert response.get_json()["detail"].startswith("Server name may only contain")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_server_identity_routes.py` around lines 8 - 55, Extract the duplicated fake-mode setup into a pytest fixture named fake_app_client: move the TemporaryDirectory/patch.dict block and the runtime reset/restore of runtime._runtime and runtime._fake_state into the fixture, call create_app() and create the admin user inside the fixture, yield (app, client) from it, and restore the previous runtime values in the fixture's finally block; then simplify the tests test_server_identity_api_updates_fake_mode_config and test_server_identity_api_rejects_invalid_name_in_fake_mode to accept fake_app_client, unpack app and client, perform only the request/assertions, and reference the same services via app.extensions["simple_safer_server"] as before.
8-98: ⚡ Quick winMissing test for GET
/api/server_identityand for a missingserver_namekeyThe test suite covers only the PUT path. The GET endpoint (
api_get_server_identity) has separate code paths (including anexcept Exceptionhandler) with no coverage. A basic GET test after the PUT would also confirm the value is readable and the response shape is correct.Similarly, sending
{}(valid JSON but noserver_namekey) exercises thedata.get("server_name") → Nonepath that goes throughnormalize_server_name(None)— worth a one-liner assertion to lock in that behaviour.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_server_identity_routes.py` around lines 8 - 98, Add two assertions/tests to cover the GET path and the missing server_name case: after the successful PUT in test_server_identity_api_updates_fake_mode_config call GET "/api/server_identity" via the same test_client and assert status_code == 200 and the JSON shape includes data.server_name == "family-nas" (verifies api_get_server_identity path and response shape); also add a small test (or extend test_server_identity_api_rejects_invalid_name_in_fake_mode) that sends PUT "/api/server_identity" with json={} and assert the response is a 400 and the error detail matches the expected message when data.get("server_name") is None (exercises normalize_server_name(None) path). Ensure you reuse create_app/app.test_client and the same setup code (services.config_manager and creating admin) so tests run in the fake mode context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/network_file_sharing.md`:
- Around line 4-5: Confirm whether the server name is actually used in alert
emails in the alert-email implementation, and then update both the docs
reference and the template hint consistently: if alert emails do not include the
server name, remove or reword the phrase "and in alert emails" in
docs/network_file_sharing.md (the sentence mentioning server name usage) and in
templates/network_file_sharing.html's form-hint (the line flagged near the
server name hint); if they do include it, add a brief note in both places
describing where/how the server name appears in alerts. Ensure both files use
identical wording so the documentation and UI hint remain consistent with the
implementation.
In `@simple_safer_server/routes/server_identity.py`:
- Around line 31-50: Hoist the call to json_request_data() out of the try block
so client validation errors aren't swallowed: call data = json_request_data()
before entering the try that wraps
_get_services().server_identity_service.update_server_name(...),
ServerIdentityError handling, and the generic Exception handler; leave the
existing ServerIdentityError except (ServerIdentityError as exc ->
json_problem(ValidationProblem(...))) and the final except Exception block
intact so only server-side errors are converted to OperationProblem.
In `@templates/network_file_sharing.html`:
- Around line 120-123: Remove the incorrect claim that the server name "appears
in alert emails" by deleting that clause from the form hint associated with the
serverNameInput (the form-hint text), from the SERVER_NAME_HELP_TEXT constant in
server identity code, and from the network file sharing documentation
(network_file_sharing.md); update the texts so they only describe how the server
name is used on the network and in UI, or replace the clause with a qualified
statement if you prefer (e.g., "may be referenced in notifications" only if you
add that behavior later).
---
Nitpick comments:
In `@templates/network_file_sharing.html`:
- Around line 424-434: In loadServerIdentity, clarify or remove the apparent
dead fallback data.hostname used in the assignment to currentServerName
(currentServerName = data.server_name || data.hostname || ''); either remove
data.hostname if unused by the API, or add a one-line comment immediately above
that assignment explaining that data.hostname is an intentional
backward-compatibility fallback for older API responses; update the related
references (serverNameValue and serverNameInput usage) only if you remove the
fallback to ensure behavior remains consistent.
In `@tests/test_server_identity_routes.py`:
- Around line 8-55: Extract the duplicated fake-mode setup into a pytest fixture
named fake_app_client: move the TemporaryDirectory/patch.dict block and the
runtime reset/restore of runtime._runtime and runtime._fake_state into the
fixture, call create_app() and create the admin user inside the fixture, yield
(app, client) from it, and restore the previous runtime values in the fixture's
finally block; then simplify the tests
test_server_identity_api_updates_fake_mode_config and
test_server_identity_api_rejects_invalid_name_in_fake_mode to accept
fake_app_client, unpack app and client, perform only the request/assertions, and
reference the same services via app.extensions["simple_safer_server"] as before.
- Around line 8-98: Add two assertions/tests to cover the GET path and the
missing server_name case: after the successful PUT in
test_server_identity_api_updates_fake_mode_config call GET
"/api/server_identity" via the same test_client and assert status_code == 200
and the JSON shape includes data.server_name == "family-nas" (verifies
api_get_server_identity path and response shape); also add a small test (or
extend test_server_identity_api_rejects_invalid_name_in_fake_mode) that sends
PUT "/api/server_identity" with json={} and assert the response is a 400 and the
error detail matches the expected message when data.get("server_name") is None
(exercises normalize_server_name(None) path). Ensure you reuse
create_app/app.test_client and the same setup code (services.config_manager and
creating admin) so tests run in the fake mode context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2fe8ce2-ef77-41d2-a5cc-13034dc5ec38
📒 Files selected for processing (19)
docs/network_file_sharing.mddocs/setup.mddocs/uninstall.mdnotes/0027-cloud-backup-setup-service-refactor.mdnotes/0028-server-identity-hostname.mdsimple_safer_server/adapters/server_identity_commands.pysimple_safer_server/app_factory.pysimple_safer_server/routes/server_identity.pysimple_safer_server/routes/setup_wizard.pysimple_safer_server/services/container.pysimple_safer_server/services/server_identity.pystatic/css/styles.csstemplates/network_file_sharing.htmltemplates/setup.htmltests/test_server_identity.pytests/test_server_identity_routes.pytests/test_setup_wizard.pytests/test_uninstall.pyuninstall.sh
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
simple_safer_server/routes/setup_wizard.py (1)
76-87: 💤 Low valueInconsistent fallback handling between service helpers.
_cloud_backup_service()has no fallback and will raiseKeyErrorif the extension isn't registered, while_server_identity_service()gracefully falls back to the module-level instance. This asymmetry is likely intentional (cloud backup is always app-factory managed), but consider adding a brief comment on Line 78 explaining why no fallback is needed here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@simple_safer_server/routes/setup_wizard.py` around lines 76 - 87, The helper _cloud_backup_service currently accesses current_app.extensions["simple_safer_server"] without a fallback which differs from _server_identity_service; add a short explanatory comment above the _cloud_backup_service function stating that the cloud-backup service is always registered by the app factory (so absence is a configuration error and a KeyError is intentional) and therefore no module-level fallback like server_identity_service is provided, referencing the function name _cloud_backup_service and the contrasted helper _server_identity_service to make intent clear for future readers.simple_safer_server/services/drive_health.py (1)
801-804: 💤 Low valueAdd a comment explaining the double-default pattern.
The outer
or "SimpleSaferServer"guard handles the case where the config key exists but is explicitlyNoneor an empty string (the innerget_valuedefault only fires when the key is absent). This is non-obvious and easy to wonder about on a future read. Per coding guidelines, things we might forget in a few months should have a brief comment.💡 Suggested comment
+ # get_value's default covers a missing key; the `or` covers an explicitly + # blank/None value stored in config. server_name = ( config_manager.get_value("system", "server_name", "SimpleSaferServer") or "SimpleSaferServer" ).strip()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@simple_safer_server/services/drive_health.py` around lines 801 - 804, Add a brief inline comment above the server_name assignment explaining the double-default pattern: note that config_manager.get_value("system", "server_name", "SimpleSaferServer") provides a default only when the key is missing, whereas the trailing or "SimpleSaferServer" handles cases where the key exists but is None or an empty string, and then .strip() is applied; reference the server_name variable and the config_manager.get_value call so future readers understand why both defaults are present.tests/test_drive_health.py (1)
173-197: 💤 Low valueConsider using
call_args.args[2]and adding a default-fallback test.Two minor improvements worth considering:
Accessor style —
call_args[0](positional args tuple) andcall_args.argsare identical references, butcall_args.args[2]is the more explicit, named form and avoids the magic numeric index into the outer tuple.Missing coverage for the default path — the test only exercises the configured-name path (
"nas-01"). A second test (or asubTest) that omits("system", "server_name")from the mock map would verify that the double-default falls back correctly to"SimpleSaferServer"in the subject line.✨ Suggested improvements
- self.assertIn("Subject: Drive health - nas-01", mock_send_email.call_args[0][2]) + self.assertIn("Subject: Drive health - nas-01", mock_send_email.call_args.args[2])Additional test for the default fallback:
`@patch`( "simple_safer_server.services.drive_health.drive_health_command_adapter.send_email", side_effect=TimeoutExpired(cmd=["msmtp"], timeout=30), ) def test_log_and_email_alert_uses_default_server_name(self, mock_send_email): config_manager = SimpleNamespace( log_alert=MagicMock(), get_value=lambda section, key, default="": { ("backup", "email_address"): "admin@example.com", ("backup", "from_address"): "server@example.com", # ("system", "server_name") intentionally absent → default kicks in }.get((section, key), default), ) runtime = SimpleNamespace(is_fake=False) drive_health._log_and_email_alert( config_manager, runtime, "Drive health", "message", alert_type="warning", source="drive_health", ) self.assertIn( "Subject: Drive health - SimpleSaferServer", mock_send_email.call_args.args[2], )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_drive_health.py` around lines 173 - 197, Update the test to use the explicit call-args accessor and add coverage for the default server name: replace mock_send_email.call_args[0][2] with mock_send_email.call_args.args[2] when asserting the Subject line, and add a second test (or subTest) that constructs the config_manager.get_value map without the ("system","server_name") entry and calls drive_health._log_and_email_alert (with runtime SimpleNamespace(is_fake=False)) to assert that mock_send_email.call_args.args[2] contains "Subject: Drive health - SimpleSaferServer".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@simple_safer_server/routes/setup_wizard.py`:
- Around line 935-938: The responses for related endpoints are inconsistent:
update mega_connect to return the full MegaFolderList-shaped object (not just
{"folders": [...]}) so it matches mega_list_folders; specifically, in the
mega_connect handler build/return the same MegaFolderList structure (including
"folders", "path", and "parent")—use the same
_cloud_backup_service().list_mega_folders call or construct a MegaFolderList
dataclass and pass it through the same serialize/json helper (e.g.,
serialize_api_data/json_data) so both mega_connect and mega_list_folders produce
the same response shape.
In `@simple_safer_server/services/drive_health.py`:
- Around line 801-809: The server_name value retrieved via
config_manager.get_value and used when constructing email_body must have any
CR/LF characters removed to prevent header injection; instead of only calling
.strip(), sanitize server_name (the variable assigned from
config_manager.get_value(...)) by using the existing re import to remove or
replace any \r or \n characters (e.g., re.sub to replace them with a single
space) before embedding it into the Subject header in the email_body
construction.
---
Nitpick comments:
In `@simple_safer_server/routes/setup_wizard.py`:
- Around line 76-87: The helper _cloud_backup_service currently accesses
current_app.extensions["simple_safer_server"] without a fallback which differs
from _server_identity_service; add a short explanatory comment above the
_cloud_backup_service function stating that the cloud-backup service is always
registered by the app factory (so absence is a configuration error and a
KeyError is intentional) and therefore no module-level fallback like
server_identity_service is provided, referencing the function name
_cloud_backup_service and the contrasted helper _server_identity_service to make
intent clear for future readers.
In `@simple_safer_server/services/drive_health.py`:
- Around line 801-804: Add a brief inline comment above the server_name
assignment explaining the double-default pattern: note that
config_manager.get_value("system", "server_name", "SimpleSaferServer") provides
a default only when the key is missing, whereas the trailing or
"SimpleSaferServer" handles cases where the key exists but is None or an empty
string, and then .strip() is applied; reference the server_name variable and the
config_manager.get_value call so future readers understand why both defaults are
present.
In `@tests/test_drive_health.py`:
- Around line 173-197: Update the test to use the explicit call-args accessor
and add coverage for the default server name: replace
mock_send_email.call_args[0][2] with mock_send_email.call_args.args[2] when
asserting the Subject line, and add a second test (or subTest) that constructs
the config_manager.get_value map without the ("system","server_name") entry and
calls drive_health._log_and_email_alert (with runtime
SimpleNamespace(is_fake=False)) to assert that mock_send_email.call_args.args[2]
contains "Subject: Drive health - SimpleSaferServer".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c391b51-cdaf-4637-a9b5-1ab463b22533
📒 Files selected for processing (9)
docs/network_file_sharing.mdsimple_safer_server/routes/server_identity.pysimple_safer_server/routes/setup_wizard.pysimple_safer_server/services/drive_health.pysimple_safer_server/services/server_identity.pytemplates/network_file_sharing.htmltemplates/setup.htmltests/test_drive_health.pytests/test_server_identity_routes.py
✅ Files skipped from review due to trivial changes (2)
- docs/network_file_sharing.md
- templates/network_file_sharing.html
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_server_identity_routes.py
- templates/setup.html
- simple_safer_server/routes/server_identity.py
- simple_safer_server/services/server_identity.py
Comment on lines
+935
to
+938
| folder_list = _cloud_backup_service().list_mega_folders( | ||
| {'email': email, 'password': password, 'path': path} | ||
| ) | ||
| return json_data(folder_list) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the return type of list_mega_folders and json_data signature
echo "=== Checking list_mega_folders return type ==="
rg -n -A 10 'def list_mega_folders' --type py
echo "=== Checking json_data signature ==="
rg -n -A 5 'def json_data' --type pyRepository: chrismin13/SimpleSaferServer
Length of output: 2579
🏁 Script executed:
#!/bin/bash
echo "=== Checking serialize_api_data implementation ==="
rg -n -A 15 'def serialize_api_data' --type py
echo "=== Checking MegaFolderList class definition ==="
rg -n -B 2 -A 10 'class MegaFolderList' --type py
echo "=== Checking mega_connect return format (line 916) ==="
rg -n -B 5 -A 2 'mega_connect' simple_safer_server/routes/setup_wizard.py | head -40Repository: chrismin13/SimpleSaferServer
Length of output: 2693
🏁 Script executed:
#!/bin/bash
echo "=== Checking mega_connect function (around line 916) ==="
sed -n '903,925p' simple_safer_server/routes/setup_wizard.py
echo "=== Checking mega_list_folders function (around line 935) ==="
sed -n '925,945p' simple_safer_server/routes/setup_wizard.pyRepository: chrismin13/SimpleSaferServer
Length of output: 1840
Inconsistent API response structure between mega_connect and mega_list_folders.
mega_connect returns {"data": {"folders": [...]}} while mega_list_folders returns {"data": {"folders": [...], "path": "...", "parent": "..."}}. Although serialize_api_data will successfully convert the MegaFolderList dataclass, the response shapes differ, which may confuse API clients expecting a consistent structure across related endpoints.
Consider either updating mega_connect to return the full MegaFolderList object for consistency, or document why the endpoints intentionally return different shapes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@simple_safer_server/routes/setup_wizard.py` around lines 935 - 938, The
responses for related endpoints are inconsistent: update mega_connect to return
the full MegaFolderList-shaped object (not just {"folders": [...]}) so it
matches mega_list_folders; specifically, in the mega_connect handler
build/return the same MegaFolderList structure (including "folders", "path", and
"parent")—use the same _cloud_backup_service().list_mega_folders call or
construct a MegaFolderList dataclass and pass it through the same serialize/json
helper (e.g., serialize_api_data/json_data) so both mega_connect and
mega_list_folders produce the same response shape.
Comment on lines
+801
to
+809
| server_name = ( | ||
| config_manager.get_value("system", "server_name", "SimpleSaferServer") | ||
| or "SimpleSaferServer" | ||
| ).strip() | ||
| if not email_address or not from_address: | ||
| LOGGER.warning("Skipping email alert because email settings are incomplete.") | ||
| return | ||
|
|
||
| email_body = f"Subject: {title}\nFrom: {from_address}\n\n{message}" | ||
| email_body = f"Subject: {title} - {server_name}\nFrom: {from_address}\n\n{message}" |
Contributor
There was a problem hiding this comment.
Strip CRLF from server_name before embedding it in the email Subject header.
.strip() only trims leading/trailing whitespace. If a newline is present anywhere inside the stored value (e.g., "nas-01\nBcc: attacker@example.com"), it will split the Subject: line and inject arbitrary headers into the raw email string built at line 809. Even though server_name is admin-configured, defence-in-depth applies.
🛡️ Proposed fix
- server_name = (
- config_manager.get_value("system", "server_name", "SimpleSaferServer")
- or "SimpleSaferServer"
- ).strip()
+ # Read the configured server name; fall back to "SimpleSaferServer" if the
+ # key is absent or explicitly blank. Strip whitespace and control characters
+ # (including embedded newlines) to prevent email-header injection.
+ server_name = re.sub(
+ r"[\r\n]",
+ "",
+ (
+ config_manager.get_value("system", "server_name", "SimpleSaferServer")
+ or "SimpleSaferServer"
+ ).strip(),
+ )re is already imported at the top of the module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@simple_safer_server/services/drive_health.py` around lines 801 - 809, The
server_name value retrieved via config_manager.get_value and used when
constructing email_body must have any CR/LF characters removed to prevent header
injection; instead of only calling .strip(), sanitize server_name (the variable
assigned from config_manager.get_value(...)) by using the existing re import to
remove or replace any \r or \n characters (e.g., re.sub to replace them with a
single space) before embedding it into the Subject header in the email_body
construction.
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests