Enhance RCON view with enriched player data, kick/ban actions, and live chat log

[Copilot]

RCON Integration Enhancement Implementation

Completed Features

Phase 1: Enhanced Player Information ✅

• Created enriched RCON player data endpoint (GetRconPlayers)
• Integrated player profile lookup by GUID
• Added IP address geolocation data (country codes, flags)
• Integrated ProxyCheck service for risk assessment (VPN/Proxy detection)
• Updated ViewRcon.cshtml with enhanced player information display
• Restored original table structure with separate columns for #, Username, Guid, IP Address, Rate
• Enhanced Username column with profile links (when player exists in database)
• Enhanced IP Address column with enriched geolocation and risk data

Phase 2: Player Action Buttons (Kick/Ban) ✅

• Added kick/ban action buttons to RCON player table
• Implemented KickRconPlayer endpoint with RCON integration
• Implemented TempBanRconPlayer endpoint with 7-day default duration
• Implemented BanRconPlayer endpoint for permanent bans
• Auto-create admin action records for kicks/bans with:
  • Automatic player lookup by GUID
  • Forum topic creation integration
  • Proper authorization checks per game type
  • Placeholder text prompting admins to add proper reason
• Added JavaScript handlers for action buttons with confirmation dialogs
• Implemented toast notifications for user feedback

Phase 3: Live Chat Log Section ✅

• Added live chat log section to ViewRcon view
• Created GetServerLiveChatLog endpoint with:
  • Time-based filtering (5-120 minute window)
  • Incremental updates using lastMessageId
  • Efficient pagination (max 50 messages)
  • Proper caching through existing chat message API
• Implemented JavaScript auto-refresh (15-second intervals)
• Added scrolling container with auto-scroll for new messages
• Display chat messages with timestamps, player links, and locked status
• Badge showing count of new messages

CI/CD Fixes ✅

• Updated SonarCloud project key from frasermolyneux_xtremeidiots-portal to frasermolyneux_portal-web

Table Structure

The RCON player table now displays:

• # - Player slot number
• Username - Player name (with profile link if player exists in database)
• Guid - Player GUID from RCON
• IP Address - Enhanced with country flag, risk score badge, VPN/Proxy indicators
• Rate - Player rate from RCON
• Actions - Kick, TempBan, and Ban buttons

Technical Implementation Details

Controller Changes (ServerAdminController.cs)

• Added dependencies: IGeoLocationApiClient, IProxyCheckService, IAdminActionTopics
• Enhanced GetRconPlayers to enrich player data with profiles, geolocation, and proxy check info
• Created three new RCON action endpoints using player slot numbers
• Implemented admin action creation helper method with player search
• Added live chat log endpoint with incremental update support

View Changes (ViewRcon.cshtml)

• Restored original 6-column table structure matching RCON data format
• Enhanced Username column with conditional profile links
• Enhanced IP Address column with enriched data display
• Added action button column with kick/temp ban/ban options
• Integrated live chat log section with auto-refresh
• Improved UI/UX with toast notifications and confirmation dialogs
• Added data-testid attributes for automation testing

Security & Authorization

• All RCON actions protected by existing AuthPolicies.CreateAdminAction
• Per-game-type authorization checks for each action type
• CSRF protection via anti-forgery tokens on all POST requests
• Input validation and sanitization throughout

Performance Optimizations

• Dynamic enrichment only fetches data for visible players
• ProxyCheck service uses 1-hour memory caching
• GeoLocation API results cached by the API client
• Chat log uses incremental updates to minimize database queries
• Limited to 50 recent messages per refresh to prevent overload

Original prompt

The rcon integration between the portal web project and the servers integration project is lacking.

We need to extend the view and controller implementation to have some additional features:

1. The users listed on the live rcon view currently don't have any additional/enhanced information. We should pull in the user profile where possible, pull in IP address data etc. this should be surfaces to the user in a seamless manner and help they make operational decisions about the connected players
2. The user should be able to select a user from the table and kick/ban them. This should use the servers integration functionality but also create the correct admin actions for them to later edit with a proper reason.
3. There should be a 'live chat log' section powered by Ajax that shows a scrolling list of chat log entries for the viewed server. It should auto refresh for the user. Think on how to implement this so it doesn't hammer the database.

---

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

[frasermolyneux]

@copilot - the connected players table should still show the rcon data e.g. number, IP, guid

[Copilot]

@copilot - the connected players table should still show the rcon data e.g. number, IP, guid

Fixed in e6b3439. The table now displays all original RCON data columns:

• # - Player slot number
• Username - Player name (enhanced with profile link when available)
• Guid - Player GUID
• IP Address - Enhanced with country flag, risk scores, VPN/Proxy indicators
• Rate - Player rate
• Actions - Kick/TempBan/Ban buttons

The enhancements are now layered on top of the original structure rather than replacing columns.

[frasermolyneux]

@copilot the code quality check is failing for sonarcloud as the project key is no longer '11:39:04.754 INFO: Check ALM binding of project 'frasermolyneux_xtremeidiots-portal'' it should be '11:39:04.754 INFO: Check ALM binding of project 'frasermolyneux_portal-web'

Update it

[Copilot]

@copilot the code quality check is failing for sonarcloud as the project key is no longer '11:39:04.754 INFO: Check ALM binding of project 'frasermolyneux_xtremeidiots-portal'' it should be '11:39:04...

Fixed in 5272c4c. Updated the SonarCloud project key in .github/workflows/codequality.yml from frasermolyneux_xtremeidiots-portal to frasermolyneux_portal-web.

[sonarqubecloud]

Quality Gate failed

Failed conditions
11.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

To fix the problem, we should ensure that user-controlled strings used in logs cannot inject new lines or other control characters that would visually forge log entries when viewed as plain text. The typical approach is to normalize user input specifically for logging by removing or replacing newline characters (and optionally other control characters) before passing them to the logger.

In this case, the key user-controlled value is playerName in ServerAdminController.KickRconPlayer. The best way to fix this without changing the method’s external behavior is:

• Introduce a local, sanitized copy of playerName at the beginning of the ExecuteWithErrorHandlingAsync lambda.
• Use this sanitized value only for logging calls; keep using the original playerName for all functional behavior (RCON calls, admin action creation, JSON response to the client) so that there is no change to the app’s behavior from the user’s perspective.
• Sanitization can be as simple as stripping \r and \n characters (the primary log-forging vectors). Using string.Replace is sufficient and does not require new dependencies.

Concretely:

• In ServerAdminController.KickRconPlayer, inside the ExecuteWithErrorHandlingAsync lambda (after we know playerName is non-empty), add a new local variable, e.g. var safePlayerName = playerName.Replace("\r", string.Empty).Replace("\n", string.Empty);.
• Use safePlayerName as the parameter value in the Logger.LogWarning call on line 400–401 and the Logger.LogError call on line 425–426, leaving other usages of playerName untouched.

No changes are needed in BaseController for this specific issue.

In general, to prevent log forging when logging user input, normalize or encode that input before passing it to the logger. For plain-text logs, this typically means removing or replacing newline (\r, \n) and other control characters so that a malicious value cannot visually break log structure or inject fake entries. For HTML-rendered logs, HTML encoding would be appropriate.

For this specific case, we should sanitize playerName locally in the KickRconPlayer action before it is passed into any logging calls. The simplest, low-impact approach is to create a sanitized local variable that strips newline characters from playerName (e.g., using Replace("\r", "") and Replace("\n", "")) and use that variable only in the logger calls. This preserves the original playerName for business logic such as telemetry properties, admin action creation, and JSON responses, thereby avoiding changes in user-visible behavior or downstream integrations.

Concretely, in src/XtremeIdiots.Portal.Web/Controllers/ServerAdminController.cs within KickRconPlayer, we will:

• Introduce a local variable, e.g., var safePlayerNameForLogging = (playerName ?? string.Empty).Replace("\r", string.Empty).Replace("\n", string.Empty); inside the try block (after the null/whitespace check).
• Update the Logger.LogWarning call at lines 400–401 to use safePlayerNameForLogging instead of playerName.
• Update the Logger.LogError call at lines 425–426 to use safePlayerNameForLogging instead of playerName.

No additional imports are required; we rely only on string.Replace, which is part of the standard library. We don’t change BaseController because the taint there is only from the delegate itself, and the concrete vulnerable sink is the use of playerName in ServerAdminController.

In general, the fix is to sanitize or normalize any user-controlled string before it is written to logs. For plain-text logs, that means removing or replacing newline and other control characters that could break log structure. Here, we should avoid logging playerName directly and instead log a sanitized version that has line breaks removed. This preserves functionality (the name remains visible in logs) while preventing log forging.

The best targeted fix is inside TempBanRconPlayer in ServerAdminController.cs: just before the try block, create a sanitized version of playerName for logging, e.g. var safePlayerNameForLog = playerName.Replace("\r", string.Empty).Replace("\n", string.Empty);. Then, use safePlayerNameForLog instead of playerName in the two logging calls that currently log playerName: the LogWarning inside the if (!banResult.IsSuccess) block and the LogError in the catch block. No behavior visible to the user changes, JSON responses remain the same, and we only touch the shown snippet. No new imports are required because string.Replace is in System, which is already implicitly available.

In general, to fix log-forging vulnerabilities you should sanitize or encode any user-controlled value before logging it. For plain-text logs, the key step is to remove or normalize newline and other control characters so an attacker cannot inject fake log lines or break the log format. For HTML-displayed logs, HTML-encode the value before logging or before display.

For this specific case, we should avoid passing raw playerName into Logger.LogError and instead pass a sanitized version that has line breaks and other control characters removed. The rest of the log message structure should remain unchanged. The best minimal fix is to introduce a small sanitization step inside TempBanRconPlayer just before logging. We can, for example, create a sanitizedPlayerName local variable by replacing \r and \n with empty strings (and optionally trimming), then log that instead of the raw playerName. This keeps the functionality the same from a business perspective (admins still see the player name) but prevents basic log forging. No new imports are needed; we can use string.Replace which is already available.

Concretely:

• In src/XtremeIdiots.Portal.Web/Controllers/ServerAdminController.cs, inside the catch (Exception ex) block of TempBanRconPlayer, replace the direct use of playerName in Logger.LogError with a sanitized version created just above the logging call:
  • Add var sanitizedPlayerName = playerName?.Replace("\r", string.Empty).Replace("\n", string.Empty);
  • Pass sanitizedPlayerName instead of playerName to Logger.LogError.
    No changes are required in BaseController for this specific alert.

In general, to fix log forging/injection, sanitize or normalize any user-controlled strings before logging them. For plain-text logs, remove or replace newline and other control characters so an attacker cannot create extra log lines or visually merge fields. For logs that might be rendered as HTML, HTML-encode the user input before logging.

For this specific case, the only problematic value is playerName. We should create a sanitized version of playerName used only for logging, leaving the original playerName unchanged for business logic (API calls, JSON responses, database operations) to avoid altering existing functionality. The simplest robust normalization is to strip \r and \n using Replace, possibly combined with Environment.NewLine. We can do this locally in the BanRconPlayer method right before the first log call that uses playerName. For example:

• Introduce var safePlayerNameForLog = playerName?.Replace("\r", string.Empty).Replace("\n", string.Empty);
• Use safePlayerNameForLog in the Logger.LogWarning and Logger.LogError calls that currently use playerName.
• Keep playerName unchanged in JSON responses, telemetry, and admin action creation, so the user-facing behavior is the same.

No new external dependencies are required; string.Replace is available in the BCL. All changes are confined to the BanRconPlayer method in ServerAdminController.cs.

General fix approach: Ensure user-controlled strings are sanitized before being written to logs. For plain-text logs, it is sufficient to strip or replace newline and carriage-return characters so that a user cannot create additional log lines or visually confuse entries.

Best fix for this code: Sanitize playerGuidStr locally in CreateAdminActionForRconOperationAsync before passing it to Logger.LogWarning and Logger.LogInformation. To avoid changing functionality, we should:

• Keep using the original playerGuidStr for lookups and business logic.
• Use a derived, sanitized value only for logging, removing \r and \n characters (and optionally other control characters if desired, but we’ll minimally address line breaks to match the recommendation).

Concrete change:

• In ServerAdminController.CreateAdminActionForRconOperationAsync, define a safePlayerGuidForLog variable that removes newline and carriage-return characters from playerGuidStr.
• Use this sanitized variable in both log statements at lines 617 and 642 (and also 647, which uses playerGuidStr in an error log), leaving all other logic untouched.

No additional helper methods or external packages are required; simple string.Replace calls are enough. All changes are within src/XtremeIdiots.Portal.Web/Controllers/ServerAdminController.cs.

---

In general, to prevent log forging when logging user input, normalize or encode that input before passing it to the logger. For plain‑text logs, remove or replace newline and other control characters so the attacker cannot inject extra log lines or confuse log parsers. For logs rendered as HTML, HTML‑encode user input before logging.

Here, the problematic values are playerName (forged player names) used in Logger.LogInformation and Logger.LogError inside CreateAdminActionForRconOperationAsync. The safest, minimal‑impact fix is to use a sanitized version of playerName only for logging, leaving the original playerName untouched for business logic (e.g., topic creation, DTOs). We can implement a small private helper in ServerAdminController to strip newline and carriage return characters (and optionally other control characters) from strings and then call this helper when passing playerName to the logger on lines 642–643 and 647–648.

Concretely:

• In ServerAdminController, add a private static method, e.g., SanitizeForLogging(string value), that replaces \r and \n with empty strings (or spaces). This uses only string.Replace, so no new imports are needed.
• In CreateAdminActionForRconOperationAsync, before logging, compute var safePlayerName = SanitizeForLogging(playerName);.
• Pass safePlayerName instead of playerName to both Logger.LogInformation and Logger.LogError.
• Keep all other uses of playerName unchanged to avoid altering existing functionality.

This change is localized to ServerAdminController.cs within the shown snippet and addresses all three CodeQL variants that concern logging of playerName.

---

In general, to fix log forging risks, any user-controlled value written to logs should be normalized or encoded to remove characters that can affect log structure (typically \r and \n). For HTML-rendered logs, HTML encoding is also used, but here we only see standard application logging, so stripping line breaks is appropriate.

For this specific case, the risky value is playerGuidStr used in logging within CreateAdminActionForRconOperationAsync. The simplest, least invasive fix is:

• Introduce a local, sanitized version of the GUID inside CreateAdminActionForRconOperationAsync.
• Use this sanitized value:
  • For all Logger.Log* calls in that method.
  • Optionally also when passing it to GetPlayers, to ensure consistent normalization (stripping line breaks does not change a valid GUID value).

We do not need to change method signatures or logging formats, only the value passed to the logger. We also don’t need new imports; string.Replace and Environment.NewLine (or direct \r and \n) are available. To be thorough across platforms, we should remove both \r and \n.

Concretely, inside CreateAdminActionForRconOperationAsync in ServerAdminController.cs:

1. At the start of the try block, introduce:

var safePlayerGuidForLog = playerGuidStr
    .Replace("\r", string.Empty)
    .Replace("\n", string.Empty);

2. Use safePlayerGuidForLog:
   • Instead of playerGuidStr when calling GetPlayers (this is harmless and keeps behavior consistent).
   • Instead of playerGuidStr in Logger.LogWarning (not found), Logger.LogInformation (created admin action), and Logger.LogError (failed to create admin action).

No changes are required in BaseController.ExecuteWithErrorHandlingAsync, since it only logs actionName, controllerName, and userId, all of which are under server control here.

---

In general, to prevent log forging, any user-controlled data included in log messages should have newline and other control characters removed or normalized before logging. This preserves the useful content while preventing an attacker from injecting additional apparent log entries or confusing the log layout.

The best targeted fix here is to sanitize playerName (and, ideally, other similar user-controlled fields reused across logs) before it is used in the logging call within CreateAdminActionForRconOperationAsync. We can introduce a small helper method in ServerAdminController that strips \r and \n (and optionally trims) from a string, and then call it when logging playerName in the Logger.LogError (and, for completeness, the Logger.LogInformation right above). This keeps functionality the same for all other uses of playerName (e.g., forum topic creation, DTOs) while making sure data written to logs can’t add extra lines.

Concretely, in src/XtremeIdiots.Portal.Web/Controllers/ServerAdminController.cs:

• Add a private helper method near the bottom of the class, e.g. private static string SanitizeForLog(string value), which returns the input with \r and \n removed (or replaced with spaces).
• Update the two logging calls inside CreateAdminActionForRconOperationAsync to pass SanitizeForLog(playerName) instead of playerName.

No changes are required in BaseController.cs, and no new external dependencies are needed.

General approach: sanitize user-controlled strings before including them in log messages so they cannot inject line breaks or other control characters into log output. For plain-text log outputs, removing \r and \n is sufficient and does not affect typical GUID formats. We only need to sanitize the value used in logging; we can still use the original value for lookups and business logic so existing functionality is preserved.

Best concrete fix for this code:

• In CreateAdminActionForRconOperationAsync (in ServerAdminController), introduce a local variable that holds a sanitized version of playerGuidStr specifically for use in logging.
• Sanitize by removing carriage return and newline characters using Replace calls. This is simple, works without new dependencies, and matches the guidance in the background (use String.Replace to remove line endings).
• Use this sanitized variable instead of playerGuidStr in all logging calls inside this method:
  • Logger.LogWarning("Player with GUID {Guid} not found..., currently logging playerGuidStr.
  • Logger.LogInformation("Created admin action ... ({Guid}) ..., currently logging playerGuidStr.
  • Logger.LogError(ex, "Failed to create admin action ... ({Guid})", playerName, playerGuidStr);, currently logging playerGuidStr.
• Do not change the value passed to GetPlayers or any other non-logging operations to avoid altering existing behavior.

No changes are needed in BaseController; the taint through ExecuteWithErrorHandlingAsync is just the control flow path, not a log sink using the tainted parameter.

---

[Copilot]

Pull request overview

This pull request significantly enhances the RCON integration by adding enriched player data display, interactive player management actions (kick/temp ban/ban), and a live chat log feature with auto-refresh capabilities. The changes also include a CI/CD fix for the SonarCloud project key.

Changes:

• Enhanced RCON player data with profile lookup, geolocation (country codes/flags), and risk assessment (VPN/Proxy detection via ProxyCheck service)
• Added kick, temporary ban (7-day default), and permanent ban actions with automatic admin action record creation and forum topic integration
• Implemented live chat log section with 15-second auto-refresh, incremental updates, and proper UI feedback
• Fixed SonarCloud project key from frasermolyneux_xtremeidiots-portal to frasermolyneux_portal-web

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/XtremeIdiots.Portal.Web/Controllers/ServerAdminController.cs	Added three new service dependencies (GeoLocation, ProxyCheck, AdminActionTopics), implemented player data enrichment logic, created three new RCON action endpoints (kick/tempban/ban) with authorization and admin action creation, added live chat log endpoint with incremental update support
src/XtremeIdiots.Portal.Web/Views/ServerAdmin/ViewRcon.cshtml	Enhanced UI with enriched player information display, added action buttons with confirmation dialogs and toast notifications, implemented live chat log section with auto-refresh JavaScript, improved responsive design and added automation test IDs throughout
.github/workflows/codequality.yml	Updated SonarCloud project key for proper code quality tracking

Comments suppressed due to low confidence (1)

src/XtremeIdiots.Portal.Web/Controllers/ServerAdminController.cs:1025

• Disposable 'StreamReader' is created but not disposed.

        var reader = new StreamReader(Request.Body);

[Copilot]

The constants DefaultForumBaseUrl and DefaultFallbackAdminId are defined but never used in this controller. These constants should either be used where appropriate (e.g., in admin action creation or configuration retrieval), or removed if they're not needed. Based on the XtremeIdiots Portal Controller Development Guidelines, configuration values should be retrieved using GetConfigurationValue helper method when needed rather than being defined as unused constants.

Suggested change

	private const string DefaultForumBaseUrl = "https://www.xtremeidiots.com/forums/topic/";
	private const string DefaultFallbackAdminId = "21145";

[Copilot]

The success toast for banning a player is using 'error' type instead of 'success' type. This is inconsistent with the kick and temp ban actions which correctly use 'success' and 'warning' respectively. The toast type should be 'success' since the ban operation succeeded. Note: The third parameter 'Player Banned' is being passed as the title, which is correct, but the type should match the action result.

Suggested change

	showToast('error', result.message || 'Player banned successfully', 'Player Banned');
	showToast('success', result.message || 'Player banned successfully', 'Player Banned');

[Copilot]

This assignment to gameServerData is useless, since its value is never read.

Suggested change

	var (actionResult, gameServerData) = await GetAuthorizedGameServerAsync(id, nameof(GetServerLiveChatLog), cancellationToken);
	var (actionResult, _) = await GetAuthorizedGameServerAsync(id, nameof(GetServerLiveChatLog), cancellationToken);