ChrisRistoff / RecipeHub

0 stars 2 forks source link

Potential security risk in a production environment #7

Open MichaelAGill opened 8 months ago

MichaelAGill commented 8 months ago

https://github.com/krasenHristov/RecipeHub/blob/044988987d5b9ff93af027177481e4607bae18ab/OpenSourceRecipe/Program.cs#L77

This statement causes concern. It only takes a simple human error before a dev accidentally destroys your production database.

You can however, defend against this with essentially 'double checking' your environment variable

You have your environment 'testing', 'development', 'production', etc. So you can make another environment variable like ENABLE_DB_CLEANUP

You're code could look something like:

// Get environment variable for connection string and default to Development if not defined.
string env = Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT") ?? "Development";

// This is a 'switch expression' from C# 8.0 (We're on 12.0 now)
var connectionStringName = env switch
{
    "Development" => "DefaultConnection",
    "Production" => "ProductionConnection",
    "Testing" => "TestConnection",
    _ => "DefaultConnection"
};

var connectionString = builder.Configuration.GetConnectionString(connectionStringName);

// Ensure that DB cleanup is only done in a controlled testing environment
if (env == "Testing" && Environment.GetEnvironmentVariable("ENABLE_DB_CLEANUP") == "true")
{
    try
    {
        PerformDatabaseCleanup(connectionString);
    }
    catch (Exception ex)
    {
        // Log the exception details
        Console.WriteLine($"Error during database cleanup: {ex.Message}");
    }
}

void PerformDatabaseCleanup(string connString)
{
    // Define and run your delete SQL command
}

This way it will be much harder (still not impossible) for someone to inadvertantly run the delete code with your Production connection string.

MichaelAGill commented 8 months ago

image

So let me give an example of how it could become an issue in it's current state.

If a dev was to accidentally change a single character from

if (env == "Testing") into if (env != "Testing")

Or. A dev were to simply mess up the scope to that if statement. Then by the time this runs in a production environment, it could affect prod then because your env would be "production" giving you the prod connection strings.

I agree with you however. It is very unlikely. Especially since you are all now aware. So no code change is actually required. Just advised.