-
Notifications
You must be signed in to change notification settings - Fork 27
Add API endpoint for community templates and update CLI to use it #170
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: main
Are you sure you want to change the base?
Conversation
- Create CommunityTemplatesApiController in PSW web app with endpoints:
- GET /api/communitytemplates/index (returns index.json)
- GET /api/communitytemplates/template/{fileName} (returns template YAML)
- GET /api/communitytemplates/clearcache (clears template cache)
- Copy community-templates folder to src/PSW for API serving
- Update PSW.csproj to include community-templates in build output
- Update CommunityTemplateService in CLI to call PSW API instead of GitHub
- Add CommunityTemplatesApiUrl configuration to PSWConfig and appsettings.json
- Update service registration to inject API URL from configuration
This change allows the CLI to fetch templates from the PSW website API instead of directly from GitHub, while still allowing pull requests to GitHub for template updates.
- Update CommunityTemplateService to use ApiConfiguration.ApiBaseUrl - Remove CommunityTemplatesApiUrl from PSWConfig (not needed) - Remove CommunityTemplatesApiUrl from CLI appsettings.json - Update service registration to not inject API URL from config The API base URL (https://psw.codeshare.co.uk) is now hard-coded in ApiConfiguration.cs to prevent tampering on user machines.
- Update README files to reflect templates are served via PSW API - Change references from GitHub to https://psw.codeshare.co.uk - Update template submission path to src/PSW/community-templates/ - Update index file reference from index.yaml to index.json - Add notes about API deployment and caching (1-hour TTL) - Update troubleshooting to reference API instead of GitHub This documentation now matches the implementation where the CLI fetches templates from the PSW API instead of directly from GitHub.
| // Security: Ensure the filename doesn't contain path traversal attempts | ||
| if (fileName.Contains("..") || fileName.Contains("/") || fileName.Contains("\\")) | ||
| { | ||
| _logger.LogWarning("Invalid filename attempted: {FileName}", fileName); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To fix this kind of issue, user-controlled values should be sanitized before being written to logs. For log files that are stored or viewed as plain text, this typically means removing or normalizing newline and other control characters so that a malicious user cannot inject extra log lines or otherwise break the log structure. The functional behavior of the application should remain unchanged; only what is written to the logs is adjusted.
In this specific controller, the only problematic sink is _logger.LogWarning("Invalid filename attempted: {FileName}", fileName); in GetTemplate. The best minimal fix is to create a sanitized version of fileName that strips newline characters (and, optionally, carriage returns) before passing it to the logger. We should keep this change local to the GetTemplate method and avoid altering how fileName is used elsewhere (e.g., in responses or when constructing paths) so existing functionality is preserved.
Concretely, in src/PSW/Controllers/CommunityTemplatesApiController.cs, within the if (fileName.Contains("..") || fileName.Contains("/") || fileName.Contains("\\") ) block, introduce a new variable sanitizedFileName that uses Replace to remove Environment.NewLine and \n/\r characters from fileName, and then log sanitizedFileName instead of fileName. No new methods or imports are required; string.Replace and Environment.NewLine are already available from System, which is typically imported at the project level (and we do not need to modify imports in the snippet).
-
Copy modified lines R105-R110
| @@ -102,7 +102,12 @@ | ||
| // Security: Ensure the filename doesn't contain path traversal attempts | ||
| if (fileName.Contains("..") || fileName.Contains("/") || fileName.Contains("\\")) | ||
| { | ||
| _logger.LogWarning("Invalid filename attempted: {FileName}", fileName); | ||
| var sanitizedFileName = fileName | ||
| .Replace(Environment.NewLine, string.Empty) | ||
| .Replace("\n", string.Empty) | ||
| .Replace("\r", string.Empty); | ||
|
|
||
| _logger.LogWarning("Invalid filename attempted: {FileName}", sanitizedFileName); | ||
| return BadRequest(new { error = "Invalid filename" }); | ||
| } | ||
|
|
|
|
||
| if (!System.IO.File.Exists(templatePath)) | ||
| { | ||
| _logger.LogWarning("Community template not found at {TemplatePath}", templatePath); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix log-forging issues you should sanitize or encode any user-controlled data before logging. For plain-text logs, the key is to remove or neutralize newline and other control characters so a user cannot visually create fake subsequent log entries. This is typically done with Replace calls or a small helper that strips dangerous characters before logging.
For this specific case, the problematic value is templatePath (derived from user-provided fileName) being logged at line 127. We should ensure that any control characters in fileName do not reach the log. The least intrusive and most localized fix is to sanitize fileName to remove newline characters (and optionally other control characters) right before we use it in that log statement, without changing how the actual file path is built. To keep behavior identical for file access but safer for logs, we can create a sanitized version used only for logging, e.g. var safeTemplatePath = templatePath.Replace("\r", "").Replace("\n", ""); and log that instead. This does not affect file reading and only adjusts what gets written to logs.
Concretely, in src/PSW/Controllers/CommunityTemplatesApiController.cs, within the lambda passed to _memoryCache.GetOrCreateAsync, we will:
- Introduce a local variable
safeTemplatePathimmediately before logging when the file is not found, removing\rand\ncharacters fromtemplatePath. - Update the
LogWarningcall to logsafeTemplatePathinstead oftemplatePath.
No new methods or imports are required; we can rely on string.Replace, which is already available.
-
Copy modified lines R127-R128
| @@ -124,7 +124,8 @@ | ||
|
|
||
| if (!System.IO.File.Exists(templatePath)) | ||
| { | ||
| _logger.LogWarning("Community template not found at {TemplatePath}", templatePath); | ||
| var safeTemplatePath = templatePath.Replace("\r", string.Empty).Replace("\n", string.Empty); | ||
| _logger.LogWarning("Community template not found at {TemplatePath}", safeTemplatePath); | ||
| return null; | ||
| } | ||
|
|
| return null; | ||
| } | ||
|
|
||
| _logger.LogInformation("Reading community template from {TemplatePath}", templatePath); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix log forging issues you should sanitize or encode any user-controlled data before writing it to logs. For plain-text logs, this usually means stripping or replacing newline characters and, optionally, other control characters that may affect log structure. For logs intended for HTML display, HTML-encode user input before logging.
For this specific code, the main sensitive log entry is at line 131, logging templatePath derived from the user-controlled fileName. The most robust and minimal-change fix is to introduce a small private helper method in CommunityTemplatesApiController that takes a string and returns a version safe for logging (e.g., replaces \r and \n with spaces and optionally removes other control characters). Then, call this helper when logging any value that derives from user input: in practice, this affects the templatePath logged at line 131 and possibly the invalid filename logged at line 105 (and the error log at line 144 which includes fileName). This preserves all functionality while ensuring that even if a user submits a filename containing line breaks or control characters, the logs cannot be forged or visually confused.
Concretely:
- Add a
private static string SanitizeForLogging(string value)method near the top or bottom of the controller class. This method should:- Return the input if it is
null. - Replace
\rand\nwith spaces (or remove them). - Optionally strip other non-printable control characters (ASCII
< 32, except perhaps tab).
- Return the input if it is
- Update the relevant log statements to pass sanitized values instead of the raw
fileName/templatePath:- Line 105: log
SanitizeForLogging(fileName). - Line 127: log
SanitizeForLogging(templatePath). - Line 131: log
SanitizeForLogging(templatePath). - Line 144: log
SanitizeForLogging(fileName).
- Line 105: log
No new external dependencies are required; the sanitization can be implemented with standard System string operations.
-
Copy modified lines R16-R45 -
Copy modified line R135 -
Copy modified line R157 -
Copy modified line R161 -
Copy modified line R171
| @@ -13,6 +13,36 @@ | ||
| private readonly IWebHostEnvironment _webHostEnvironment; | ||
| private readonly IMemoryCache _memoryCache; | ||
| private readonly ILogger<CommunityTemplatesApiController> _logger; | ||
|
|
||
| /// <summary> | ||
| /// Sanitizes a string for safe inclusion in log messages by removing line breaks | ||
| /// and non-printable control characters that could enable log forging. | ||
| /// </summary> | ||
| /// <param name="value">The string to sanitize.</param> | ||
| /// <returns>A sanitized string safe for logging.</returns> | ||
| private static string SanitizeForLogging(string value) | ||
| { | ||
| if (string.IsNullOrEmpty(value)) | ||
| { | ||
| return value; | ||
| } | ||
|
|
||
| // Replace CR/LF with spaces to avoid log injection via new lines | ||
| var sanitized = value.Replace("\r", " ").Replace("\n", " "); | ||
|
|
||
| // Optionally remove other control characters | ||
| var chars = sanitized.ToCharArray(); | ||
| for (int i = 0; i < chars.Length; i++) | ||
| { | ||
| char c = chars[i]; | ||
| if (char.IsControl(c) && c != '\t') | ||
| { | ||
| chars[i] = ' '; | ||
| } | ||
| } | ||
|
|
||
| return new string(chars); | ||
| } | ||
| private const int CacheTimeInMinutes = 60; | ||
|
|
||
| public CommunityTemplatesApiController( | ||
| @@ -102,7 +132,7 @@ | ||
| // Security: Ensure the filename doesn't contain path traversal attempts | ||
| if (fileName.Contains("..") || fileName.Contains("/") || fileName.Contains("\\")) | ||
| { | ||
| _logger.LogWarning("Invalid filename attempted: {FileName}", fileName); | ||
| _logger.LogWarning("Invalid filename attempted: {FileName}", SanitizeForLogging(fileName)); | ||
| return BadRequest(new { error = "Invalid filename" }); | ||
| } | ||
|
|
||
| @@ -124,11 +154,11 @@ | ||
|
|
||
| if (!System.IO.File.Exists(templatePath)) | ||
| { | ||
| _logger.LogWarning("Community template not found at {TemplatePath}", templatePath); | ||
| _logger.LogWarning("Community template not found at {TemplatePath}", SanitizeForLogging(templatePath)); | ||
| return null; | ||
| } | ||
|
|
||
| _logger.LogInformation("Reading community template from {TemplatePath}", templatePath); | ||
| _logger.LogInformation("Reading community template from {TemplatePath}", SanitizeForLogging(templatePath)); | ||
| return await System.IO.File.ReadAllTextAsync(templatePath); | ||
| }); | ||
|
|
||
| @@ -141,7 +168,7 @@ | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error reading community template {FileName}", fileName); | ||
| _logger.LogError(ex, "Error reading community template {FileName}", SanitizeForLogging(fileName)); | ||
| return StatusCode(500, new { error = "Error reading community template", message = ex.Message }); | ||
| } | ||
| } |
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error reading community template {FileName}", fileName); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix log forging stemming from user input, you should sanitize or normalize any user-controlled values before sending them to the logger. Common mitigations include stripping newline and carriage return characters (and optionally other control characters) so that a single logged value cannot visually break log structure or masquerade as multiple entries. For structured logs, this is often as simple as passing a cleaned version of the string to the logger.
For this specific code, the primary problematic usage is at line 144 where fileName is logged in an error message. We can mitigate the issue by creating a sanitized version of fileName that removes line breaks and then using that sanitized value in the log call. To avoid changing existing behavior beyond sanitization, we should only modify the arguments passed to the logger, not the control flow or response body. A reasonable, lightweight approach within the shown snippet is to create a local variable (e.g., safeFileName) in the catch block that calls .Replace("\r", "").Replace("\n", "") on fileName, and then pass safeFileName to _logger.LogError. This approach keeps changes minimal and does not require altering method signatures or external dependencies.
Concretely, in src/PSW/Controllers/CommunityTemplatesApiController.cs, within the catch (Exception ex) block of GetTemplate, insert a line creating safeFileName using simple string replacement and then use safeFileName instead of fileName in the _logger.LogError call. No new imports are needed; we rely only on string.Replace, which is already available.
-
Copy modified lines R144-R145
| @@ -141,7 +141,8 @@ | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error reading community template {FileName}", fileName); | ||
| var safeFileName = fileName?.Replace("\r", string.Empty).Replace("\n", string.Empty); | ||
| _logger.LogError(ex, "Error reading community template {FileName}", safeFileName); | ||
| return StatusCode(500, new { error = "Error reading community template", message = ex.Message }); | ||
| } | ||
| } |
- Remove test using obsolete repository/branch parameters - Add test for creating service with HttpClient - Update to match new CommunityTemplateService constructor signature
This change allows the CLI to fetch templates from the PSW website API instead of directly from GitHub, while still allowing pull requests to GitHub for template updates.