Skip to content

Fix CodeQL security vulnerabilities in CORS, JWT, logging, and GitHub Actions #53

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.

Sign up for GitHub

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

Merged
PhantomDave merged 3 commits into from
Nov 14, 2025
Merged

Fix CodeQL security vulnerabilities in CORS, JWT, logging, and GitHub Actions #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b1d1ffb
Initial plan
Copilot Nov 14, 2025
4ac712c
Fix critical security issues in CORS and JWT configuration
Copilot Nov 14, 2025
fe145a8
Fix potential token exposure in GitHub Actions workflow
Copilot Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/deploy-tailscale.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ jobs:

- name: Log in to GitHub Container Registry
run: |
tailscale ssh "${{ steps.ssh.outputs.target }}" \
"echo ${{ secrets.GITHUB_TOKEN }} | docker login ghcr.io -u ${{ github.actor }} --password-stdin"
echo "${{ secrets.GITHUB_TOKEN }}" | tailscale ssh "${{ steps.ssh.outputs.target }}" \
"docker login ghcr.io -u ${{ github.actor }} --password-stdin"

- name: Clean up old Docker images
run: |
Expand Down
6 changes: 4 additions & 2 deletions PhantomDave.BankTracking.Api/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ public static void Main(string[] args)
})
.AddJwtBearer(options =>
{
options.RequireHttpsMetadata = false; // enable HTTPS in production
// WARNING: RequireHttpsMetadata should be true in production environments
// Set to false only for local development
Comment on lines +61 to +62
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "Set to false only for local development" but the code builder.Environment.IsDevelopment() == false evaluates to true in production (not development) and false in development. Consider rephrasing the comment to be clearer:

// RequireHttpsMetadata is disabled in development and enabled in production
Suggested change
// WARNING: RequireHttpsMetadata should be true in production environments
// Set to false only for local development
// RequireHttpsMetadata is disabled in development and enabled in production
// (i.e., set to false in development, true in production)

Copilot uses AI. Check for mistakes.
options.RequireHttpsMetadata = builder.Environment.IsDevelopment() == false;
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expression 'A == false' can be simplified to '!A'.

Suggested change
options.RequireHttpsMetadata = builder.Environment.IsDevelopment() == false;
options.RequireHttpsMetadata = !builder.Environment.IsDevelopment();

Copilot uses AI. Check for mistakes.
options.SaveToken = true;
options.TokenValidationParameters = new TokenValidationParameters
{
Expand All @@ -79,7 +81,7 @@ public static void Main(string[] args)
{
options.AddDefaultPolicy(policy =>
policy
.WithOrigins("http://localhost:4200", "http://localhost:5095/graphql", "*")
.WithOrigins("http://localhost:4200", "http://localhost:5095")
.AllowAnyHeader()
.AllowAnyMethod()
.AllowCredentials());
Expand Down
14 changes: 10 additions & 4 deletions PhantomDave.BankTracking.Api/Services/FileImportService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Text;
using CsvHelper;
using CsvHelper.Configuration;
using Microsoft.Extensions.Logging;
using OfficeOpenXml;
using PhantomDave.BankTracking.Api.Types.ObjectTypes;
using PhantomDave.BankTracking.Library.Models;
Expand All @@ -24,8 +25,10 @@ public class ParsedFileData
public FileType FileTypeExt { get; set; } = FileType.Xlsx;
public int HeaderRowIndex { get; set; } = 1;
}
public class FileImportService
public class FileImportService(ILogger<FileImportService> logger)
{
private readonly ILogger<FileImportService> _logger = logger;

public async Task<ParsedFileData> ParseFileAsync(IFile file)
{
await using var reader = file.OpenReadStream();
Expand Down Expand Up @@ -239,8 +242,9 @@ private static Encoding DetectEncoding(Stream stream)
Encoding.UTF8.GetString(buffer, 0, bytesRead);
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Encoding.UTF8.GetString by default uses DecoderFallback.ReplacementFallback, which replaces invalid bytes with '�' instead of throwing DecoderFallbackException. The catch block will never be reached. To properly detect UTF-8, you need to create a UTF-8 decoder with DecoderFallback.ExceptionFallback:

var utf8 = Encoding.GetEncoding("UTF-8", new EncoderExceptionFallback(), new DecoderExceptionFallback());
utf8.GetString(buffer, 0, bytesRead);
Suggested change
Encoding.UTF8.GetString(buffer, 0, bytesRead);
var strictUtf8 = Encoding.GetEncoding("UTF-8", new EncoderExceptionFallback(), new DecoderExceptionFallback());
strictUtf8.GetString(buffer, 0, bytesRead);

Copilot uses AI. Check for mistakes.
return Encoding.UTF8;
}
catch
catch (DecoderFallbackException)
{
// If UTF-8 decoding fails, fall back to ISO-8859-1
return Encoding.GetEncoding("ISO-8859-1");
}
}
Expand Down Expand Up @@ -283,11 +287,13 @@ public IEnumerable<FinanceRecord> FromParsedData(int accountId, ParsedFileData p

records.Add(record);
}
catch
catch (Exception ex)
{
failedCount++;
_logger.LogWarning(ex, "Failed to parse row {RowIndex} during import", records.Count + failedCount);
}
Comment on lines +290 to 294
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generic catch clause.

Suggested change
catch (Exception ex)
{
failedCount++;
_logger.LogWarning(ex, "Failed to parse row {RowIndex} during import", records.Count + failedCount);
}
catch (FormatException ex)
{
failedCount++;
_logger.LogWarning(ex, "Failed to parse row {RowIndex} due to format error during import", records.Count + failedCount);
}
catch (KeyNotFoundException ex)
{
failedCount++;
_logger.LogWarning(ex, "Failed to parse row {RowIndex} due to missing column during import", records.Count + failedCount);
}
catch (InvalidCastException ex)
{
failedCount++;
_logger.LogWarning(ex, "Failed to parse row {RowIndex} due to invalid cast during import", records.Count + failedCount);
}

Copilot uses AI. Check for mistakes.
Console.WriteLine($"Processed {records.Count + failedCount} / {parsedData.Rows.Count}, Failed: {failedCount}");
_logger.LogDebug("Processed {ProcessedRows} / {TotalRows}, Failed: {FailedRows}",
records.Count + failedCount, parsedData.Rows.Count, failedCount);
}

Comment on lines +295 to 298
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The debug log is being written inside the loop for every row, which could generate excessive log output. This should be moved outside the foreach loop to log the final count only once after all rows have been processed.

Suggested change
_logger.LogDebug("Processed {ProcessedRows} / {TotalRows}, Failed: {FailedRows}",
records.Count + failedCount, parsedData.Rows.Count, failedCount);
}
}
_logger.LogDebug("Processed {ProcessedRows} / {TotalRows}, Failed: {FailedRows}",
records.Count + failedCount, parsedData.Rows.Count, failedCount);

Copilot uses AI. Check for mistakes.
return records;
Expand Down
Loading