Advanced Security Demo by marc-mueller · Pull Request #41 · marc-mueller/FastFoodBetterTogether

marc-mueller · 2026-03-04T07:48:02Z

No description provided.

src/services/finance/FinanceService/Controllers/SqlInjectionController.cs

+        try
+        {
+            await conn.OpenAsync();
+            persons = await conn.QueryAsync<Customer>(query);


In general, the fix is to stop building SQL queries by concatenating user input and instead use parameterized queries (prepared statements). With Dapper, this means writing the SQL with named parameters (for example @Name) and passing an object containing parameter values to QueryAsync.

For this specific code, we should:

Replace the string-concatenated query on line 23 with a parameterized SQL statement that uses a parameter for the search term.

Build the wildcard pattern (%name%) in .NET code, and pass it as the value of that parameter.

Pass both the SQL string and the parameters object into conn.QueryAsync<Customer>.

Concretely, in src/services/finance/FinanceService/Controllers/SqlInjectionController.cs:

Change var query = "SELECT Id, FirstName, LastName FROM Person WHERE FirstName Like '%" + name + "%'"; to use "WHERE FirstName LIKE @Name" (or similar).

Create a parameters object, e.g. new { Name = "%" + name + "%" }.

Change conn.QueryAsync<Customer>(query); to conn.QueryAsync<Customer>(query, parameters);.

No additional imports are needed because Dapper is already in use.

src/services/finance/FinanceService/Services/BadEncryptionService.cs

+{
+    public byte[] Encrypt(string plainText)
+    {
+        var symmetricProvider = new DESCryptoServiceProvider();


In general, the fix is to replace DES (DESCryptoServiceProvider) with a secure, modern cipher such as AES (Aes / AesCryptoServiceProvider / AesManaged), and to use appropriate key and IV sizes and a safe block mode (CBC with random IV is a common baseline; avoid ECB). The key should not be a fixed, short byte array designed for DES.

For this specific method, the minimal functional change while improving security is:

Replace DESCryptoServiceProvider with Aes.Create().

Provide a 256‑bit (32‑byte) static key (since the current code already uses a hard‑coded key) and a fixed IV of the correct size (16 bytes for AES with 128‑bit block size) so that we don’t change the method’s signature or its deterministic behavior.

Ensure the encryptor is created with explicit Key and IV.

We only touch BadEncryptionService.Encrypt in src/services/finance/FinanceService/Services/BadEncryptionService.cs. No new usings are strictly required because System.Security.Cryptography and System.Text are already imported.

Copilot

Pull request overview

This PR introduces intentionally insecure “advanced security demo” endpoints into FinanceService, adding a weak encryption/hashing service and a SQL-injection-prone query endpoint for demonstration purposes.

Changes:

Adds BadEncryptionService / IBadEncryptionService and an EncryptionController endpoint.
Adds SqlInjectionController using Dapper with a dynamically concatenated SQL query.
Registers the encryption service in Program.cs and adds the Dapper package reference.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
`src/services/finance/FinanceService/Services/IBadEncryptionService.cs`	Introduces an interface for insecure hashing/encryption demo functionality.
`src/services/finance/FinanceService/Services/BadEncryptionService.cs`	Implements DES encryption + MD5 hashing (insecure) for the demo.
`src/services/finance/FinanceService/Program.cs`	Registers the demo encryption service in DI.
`src/services/finance/FinanceService/FinanceService.csproj`	Adds Dapper dependency to support the demo SQL endpoint.
`src/services/finance/FinanceService/Controllers/SqlInjectionController.cs`	Adds an endpoint demonstrating unsafe SQL query construction (SQL injection).
`src/services/finance/FinanceService/Controllers/EncryptionController.cs`	Adds an endpoint exposing the demo encryption method over HTTP.

Copilot · 2026-03-04T11:51:14Z

src/services/finance/FinanceService/Services/BadEncryptionService.cs

+        var symmetricProvider = new DESCryptoServiceProvider();
+        byte[] key = { 14, 48, 157, 156, 42, 1, 240, 65 };
+        symmetricProvider.Key = key;
+        var encryptor = symmetricProvider.CreateEncryptor();
+
+        var plainBytes = Encoding.UTF8.GetBytes(plainText);
+
+        var encryptedBytes = encryptor.TransformFinalBlock(plainBytes, 0, plainBytes.Length);


DESCryptoServiceProvider and the encryptor returned by CreateEncryptor() should be disposed to avoid leaking unmanaged resources. Wrap them in using (or using var) so each call reliably releases crypto handles.

Copilot · 2026-03-04T11:51:14Z

src/services/finance/FinanceService/Services/IBadEncryptionService.cs

+public interface IBadEncryptionService
+{
+    string CreateMD5(string input);
+    byte[] Encrypt(string plainText);
+}


IBadEncryptionService / BadEncryptionService names strongly suggest intentionally insecure behavior. If this is demo-only, please isolate it behind an explicit feature flag (or dev-only compilation) so it can’t be accidentally used by production code paths, and consider placing it under a clearly-scoped demo namespace/folder to prevent accidental reuse.

Suggested change

public interface IBadEncryptionService

{

string CreateMD5(string input);

byte[] Encrypt(string plainText);

}

#if DEMO_BAD_ENCRYPTION

/// <summary>

/// Intentionally insecure encryption service used for demo and testing scenarios only.

/// </summary>

/// <remarks>

/// This interface is conditionally compiled and should never be available in production builds.

/// Enable it only by defining the DEMO_BAD_ENCRYPTION compilation symbol in demo configurations.

/// </remarks>

public interface IBadEncryptionService

{

string CreateMD5(string input);

byte[] Encrypt(string plainText);

}

#endif

Copilot · 2026-03-04T11:51:15Z

src/services/finance/FinanceService/Controllers/EncryptionController.cs

+    [HttpGet("Encryption/EncryptString/{input}")]
+    public IActionResult EncryptString(string input)


EncryptString takes the plaintext as a URL path parameter on a GET endpoint. This will commonly end up in access logs, browser history, proxies, etc. If this endpoint is kept, prefer POST with the input in the request body (and keep it dev/feature-flag gated if it’s only for a demo).

Suggested change

[HttpGet("Encryption/EncryptString/{input}")]

public IActionResult EncryptString(string input)

[HttpPost("Encryption/EncryptString")]

public IActionResult EncryptString([FromBody] string input)

Copilot · 2026-03-04T11:51:15Z

src/services/finance/FinanceService/Controllers/SqlInjectionController.cs

+    public async Task<IActionResult> SearchPersonUnsecure(string name)
+    {
+        var conn = _context.Database.GetDbConnection();
+        var query = "SELECT Id, FirstName, LastName FROM Person WHERE FirstName Like '%" + name + "%'";


The query targets table Person, but the storage layer maps customers to table Customer (see FinanceService.Storage/EntityConfigurations/CustomerConfiguration), and there doesn’t appear to be a Person entity/table in this service. As-is, this will fail at runtime unless a separate Person table exists in the DB. Please align the table name with the actual schema used by FinanceStorage.

Suggested change

var query = "SELECT Id, FirstName, LastName FROM Person WHERE FirstName Like '%" + name + "%'";

var query = "SELECT Id, FirstName, LastName FROM Customer WHERE FirstName Like '%" + name + "%'";

Copilot · 2026-03-04T11:51:15Z

src/services/finance/FinanceService/Controllers/SqlInjectionController.cs

+
+        finally
+        {
+            conn.Close();


The DB connection is opened asynchronously but closed synchronously (conn.Close()). Prefer await conn.CloseAsync() (or use await using/OpenConnectionAsync+CloseConnectionAsync on EF’s Database) to keep async flow consistent and reduce risk of thread-pool blocking under load.

Suggested change

conn.Close();

await conn.CloseAsync();

Copilot · 2026-03-04T11:51:16Z

src/services/finance/FinanceService/Controllers/SqlInjectionController.cs

+        var query = "SELECT Id, FirstName, LastName FROM Person WHERE FirstName Like '%" + name + "%'";
+        IEnumerable<Customer> persons;
+
+        try
+        {
+            await conn.OpenAsync();
+            persons = await conn.QueryAsync<Customer>(query);


The SQL query is built via string concatenation with name, making this endpoint trivially SQL-injectable. Use a parameterized query (Dapper supports named parameters) and avoid embedding user input directly into SQL text. If this is intentionally vulnerable for a demo, it should be explicitly disabled in production (feature flag / dev-only) to prevent accidental exposure.

Suggested change

var query = "SELECT Id, FirstName, LastName FROM Person WHERE FirstName Like '%" + name + "%'";

IEnumerable<Customer> persons;

try

{

await conn.OpenAsync();

persons = await conn.QueryAsync<Customer>(query);

var query = "SELECT Id, FirstName, LastName FROM Person WHERE FirstName LIKE @pattern";

var parameters = new { pattern = $"%{name}%" };

IEnumerable<Customer> persons;

try

{

await conn.OpenAsync();

persons = await conn.QueryAsync<Customer>(query, parameters);

Copilot · 2026-03-04T11:51:17Z

src/services/finance/FinanceService/Controllers/EncryptionController.cs

+public class EncryptionController : ControllerBase
+{
+    private IBadEncryptionService _badEncryptionService;
+
+    public EncryptionController(IBadEncryptionService badEncryptionService)
+    {
+        _badEncryptionService = badEncryptionService;
+    }
+
+    [HttpGet("Encryption/EncryptString/{input}")]
+    public IActionResult EncryptString(string input)
+    {
+        return Ok(_badEncryptionService.Encrypt(input));
+    }


This new controller/action doesn’t have corresponding unit tests, while other controllers in FinanceService.Unit.Tests/Controllers are covered. Please add a test for EncryptString (at least verifying it returns Ok and delegates to IBadEncryptionService) and, if this endpoint is meant to be dev-only, a test that the endpoint/DI registration is disabled when the feature flag/environment is off.

Copilot · 2026-03-04T11:51:17Z

src/services/finance/FinanceService/Controllers/SqlInjectionController.cs

+        var query = "SELECT Id, FirstName, LastName FROM Person WHERE FirstName Like '%" + name + "%'";
+        IEnumerable<Customer> persons;
+
+        try
+        {
+            await conn.OpenAsync();
+            persons = await conn.QueryAsync<Customer>(query);


This new controller endpoint is not covered by tests, but the project has controller unit tests already. Please add tests for SearchPersonUnsecure (and, if it’s intended to be demo-only, tests that it’s disabled/hidden when the feature flag/environment is off) to avoid accidental exposure/regressions.

Suggested change

var query = "SELECT Id, FirstName, LastName FROM Person WHERE FirstName Like '%" + name + "%'";

IEnumerable<Customer> persons;

try

{

await conn.OpenAsync();

persons = await conn.QueryAsync<Customer>(query);

var query = "SELECT Id, FirstName, LastName FROM Person WHERE FirstName LIKE @SearchPattern";

var parameters = new { SearchPattern = "%" + name + "%" };

IEnumerable<Customer> persons;

try

{

await conn.OpenAsync();

persons = await conn.QueryAsync<Customer>(query, parameters);

Copilot · 2026-03-04T11:51:17Z

src/services/finance/FinanceService/FinanceService.csproj

    </PropertyGroup>

    <ItemGroup>
+        <PackageReference Include="Dapper" Version="2.1.66" />


This adds a new runtime dependency on Dapper solely for the new SQL injection demo controller. If the goal is just to demonstrate insecure SQL patterns, consider avoiding an extra package and using existing EF Core APIs (or isolate the demo endpoint into a separate demo project/service) to keep the FinanceService dependency surface smaller.

Suggested change

<PackageReference Include="Dapper" Version="2.1.66" />

Copilot · 2026-03-04T11:51:17Z

src/services/finance/FinanceService/Services/BadEncryptionService.cs

+        var symmetricProvider = new DESCryptoServiceProvider();
+        byte[] key = { 14, 48, 157, 156, 42, 1, 240, 65 };
+        symmetricProvider.Key = key;
+        var encryptor = symmetricProvider.CreateEncryptor();
+
+        var plainBytes = Encoding.UTF8.GetBytes(plainText);
+
+        var encryptedBytes = encryptor.TransformFinalBlock(plainBytes, 0, plainBytes.Length);
+
+        return encryptedBytes;
+    }
+


Encrypt uses DES with a hard-coded key. This is cryptographically broken and effectively makes the output reversible by anyone with the code (and will also trigger security scanners). If this is strictly for a demo, please gate it behind a feature flag / dev-only route and ensure it cannot be enabled in production; otherwise switch to a modern algorithm (e.g., AES-GCM) and load keys from secure configuration/secret store rather than source code.

Suggested change

var symmetricProvider = new DESCryptoServiceProvider();

byte[] key = { 14, 48, 157, 156, 42, 1, 240, 65 };

symmetricProvider.Key = key;

var encryptor = symmetricProvider.CreateEncryptor();

var plainBytes = Encoding.UTF8.GetBytes(plainText);

var encryptedBytes = encryptor.TransformFinalBlock(plainBytes, 0, plainBytes.Length);

return encryptedBytes;

}

if (plainText == null)

{

throw new ArgumentNullException(nameof(plainText));

}

using var aes = Aes.Create();

if (aes == null)

{

throw new InvalidOperationException("Unable to create AES encryption algorithm.");

}

aes.Key = GetEncryptionKey();

aes.GenerateIV();

var plainBytes = Encoding.UTF8.GetBytes(plainText);

using var encryptor = aes.CreateEncryptor(aes.Key, aes.IV);

var cipherBytes = encryptor.TransformFinalBlock(plainBytes, 0, plainBytes.Length);

// Prepend IV to ciphertext so it can be used during decryption.

var result = new byte[aes.IV.Length + cipherBytes.Length];

Buffer.BlockCopy(aes.IV, 0, result, 0, aes.IV.Length);

Buffer.BlockCopy(cipherBytes, 0, result, aes.IV.Length, cipherBytes.Length);

return result;

}

private static byte[] GetEncryptionKey()

{

var keyBase64 = Environment.GetEnvironmentVariable("FINANCE_ENCRYPTION_KEY");

if (string.IsNullOrWhiteSpace(keyBase64))

{

throw new InvalidOperationException("Encryption key is not configured. Please set the FINANCE_ENCRYPTION_KEY environment variable to a Base64-encoded AES key.");

}

byte[] keyBytes;

try

{

keyBytes = Convert.FromBase64String(keyBase64);

}

catch (FormatException ex)

{

throw new InvalidOperationException("FINANCE_ENCRYPTION_KEY must be a valid Base64-encoded AES key.", ex);

}

// Valid AES key sizes are 16, 24, or 32 bytes (128, 192, 256 bits).

if (keyBytes.Length != 16 && keyBytes.Length != 24 && keyBytes.Length != 32)

{

throw new InvalidOperationException("FINANCE_ENCRYPTION_KEY must decode to 16, 24, or 32 bytes for AES-128/192/256.");

}

return keyBytes;

}

adding bad code for demo purposes

c816b06

github-advanced-security bot found potential problems Mar 4, 2026

View reviewed changes

marc-mueller requested a review from Copilot March 4, 2026 11:45

Copilot started reviewing on behalf of marc-mueller March 4, 2026 11:46 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

@@ -7,11 +7,27 @@
             {
                 public byte[] Encrypt(string plainText)
                 {
-                    var symmetricProvider = new DESCryptoServiceProvider();
-                    byte[] key = { 14, 48, 157, 156, 42, 1, 240, 65 };
-                    symmetricProvider.Key = key;
-                    var encryptor = symmetricProvider.CreateEncryptor();
+                    using var aes = Aes.Create();
+                    // 256-bit key (32 bytes) for AES
+                    byte[] key =
+                    {
+,  48, 157, 156,  42,   1, 240,  65,
+, 210,  11,  77, 128, 254,  33,  90,
+,  63, 144, 200,  55,  89,  12, 201,
+, 166,  72, 133, 190,  28,  64, 222
+                    };
+                    // 128-bit IV (16 bytes) for AES block size
+                    byte[] iv =
+                    {
+,  20,  30,  40,  50,  60,  70,  80,
+, 100, 110, 120, 130, 140, 150, 160
+                    };
+                    aes.Key = key;
+                    aes.IV = iv;
+                    using var encryptor = aes.CreateEncryptor(aes.Key, aes.IV);
                     var plainBytes = Encoding.UTF8.GetBytes(plainText);
                     var encryptedBytes = encryptor.TransformFinalBlock(plainBytes, 0, plainBytes.Length);

-public interface IBadEncryptionService
-{
-    string CreateMD5(string input);
-    byte[] Encrypt(string plainText);
-}
+#if DEMO_BAD_ENCRYPTION
+/// <summary>
+/// Intentionally insecure encryption service used for demo and testing scenarios only.
+/// </summary>
+/// <remarks>
+/// This interface is conditionally compiled and should never be available in production builds.
+/// Enable it only by defining the DEMO_BAD_ENCRYPTION compilation symbol in demo configurations.
+/// </remarks>
+public interface IBadEncryptionService
+{
+    string CreateMD5(string input);
+    byte[] Encrypt(string plainText);
+}
+#endif

		[HttpGet("Encryption/EncryptString/{input}")]
		public IActionResult EncryptString(string input)

	var query = "SELECT Id, FirstName, LastName FROM Person WHERE FirstName Like '%" + name + "%'";
	var query = "SELECT Id, FirstName, LastName FROM Customer WHERE FirstName Like '%" + name + "%'";

-        var symmetricProvider = new DESCryptoServiceProvider();
-        byte[] key = { 14, 48, 157, 156, 42, 1, 240, 65 };
-        symmetricProvider.Key = key;
-        var encryptor = symmetricProvider.CreateEncryptor();
-        var plainBytes = Encoding.UTF8.GetBytes(plainText);
-        var encryptedBytes = encryptor.TransformFinalBlock(plainBytes, 0, plainBytes.Length);
-        return encryptedBytes;
-    }
+        if (plainText == null)
+        {
+            throw new ArgumentNullException(nameof(plainText));
+        }
+        using var aes = Aes.Create();
+        if (aes == null)
+        {
+            throw new InvalidOperationException("Unable to create AES encryption algorithm.");
+        }
+        aes.Key = GetEncryptionKey();
+        aes.GenerateIV();
+        var plainBytes = Encoding.UTF8.GetBytes(plainText);
+        using var encryptor = aes.CreateEncryptor(aes.Key, aes.IV);
+        var cipherBytes = encryptor.TransformFinalBlock(plainBytes, 0, plainBytes.Length);
+        // Prepend IV to ciphertext so it can be used during decryption.
+        var result = new byte[aes.IV.Length + cipherBytes.Length];
+        Buffer.BlockCopy(aes.IV, 0, result, 0, aes.IV.Length);
+        Buffer.BlockCopy(cipherBytes, 0, result, aes.IV.Length, cipherBytes.Length);
+        return result;
+    }
+    private static byte[] GetEncryptionKey()
+    {
+        var keyBase64 = Environment.GetEnvironmentVariable("FINANCE_ENCRYPTION_KEY");
+        if (string.IsNullOrWhiteSpace(keyBase64))
+        {
+            throw new InvalidOperationException("Encryption key is not configured. Please set the FINANCE_ENCRYPTION_KEY environment variable to a Base64-encoded AES key.");
+        }
+        byte[] keyBytes;
+        try
+        {
+            keyBytes = Convert.FromBase64String(keyBase64);
+        }
+        catch (FormatException ex)
+        {
+            throw new InvalidOperationException("FINANCE_ENCRYPTION_KEY must be a valid Base64-encoded AES key.", ex);
+        }
+        // Valid AES key sizes are 16, 24, or 32 bytes (128, 192, 256 bits).
+        if (keyBytes.Length != 16 && keyBytes.Length != 24 && keyBytes.Length != 32)
+        {
+            throw new InvalidOperationException("FINANCE_ENCRYPTION_KEY must decode to 16, 24, or 32 bytes for AES-128/192/256.");
+        }
+        return keyBytes;
+    }

Conversation

marc-mueller commented Mar 4, 2026

Uh oh!

Check failure

Uh oh!

Copilot Autofix

Check failure

Copilot Autofix

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants