Skip to content

fix(security): replace hardcoded S3 credentials with environment vari… #99

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

Open
freeediting35-png wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(security): replace hardcoded S3 credentials with environment vari… #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 common/config/main.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
'temporaryBucketResourceManager' => [
'class' => 'common\components\S3ResourceManager',
'region' => 'eu-west-2', // Bucket based in London
'key' => 'AKIAWMITDJRKVN5ODY2X',
'secret' => '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',
'key' => getenv('AWS_TEMP_BUCKET_KEY'),
'secret' => getenv('AWS_TEMP_BUCKET_SECRET'),
'bucket' => 'studenthub-public-anyone-can-upload-24hr-expiry'
/**
* You can access the Temporary bucket with:
Expand Down
4 changes: 2 additions & 2 deletions environments/prod-railway/common/config/main-local.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,8 @@
'authMethod' => \common\components\S3ResourceManager::AUTH_VIA_KEY_AND_SECRET,
'region' => 'eu-west-2', // Bucket based in London
'bucket' => 'studenthub-uploads',
'key' => 'AKIAWMITDJRKWZZEWCUM',//railway-s3-access
'secret' => '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',
'key' => getenv('AWS_PERMANENT_S3_ACCESS_KEY_ID'),//railway-s3-access
'secret' => getenv('AWS_PERMANENT_S3_SECRET_ACCESS_KEY'),
/**
* For Local Development, we access using key and secret
* For Dev and Production servers, access is via server embedded IAM roles so no key/secret required
Expand Down
59 changes: 59 additions & 0 deletions scripts/verify_credentials.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?php

$files = [
'common/config/main.php',
'environments/prod-railway/common/config/main-local.php'
];

$patterns = [
'AKIAWMITDJRKVN5ODY2X',
'zAr8Xov1olqBAaiE8CX+j45qDHaAbO+S3EhUVeaT',
'AKIAWMITDJRKWZZEWCUM',
'M6olF9l1pZ1sKIswrSCjKtGkAG2w9qDV9x230UlI'
];
Comment on lines +8 to +13
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Remove real credential literals from the verifier.

Line 9 through Line 12 stores full AWS credential values in source, which reintroduces secret exposure and is already tripping GitGuardian in this PR.

🧰 Tools
🪛 GitHub Check: GitGuardian Security Checks

[error] 10-10: GitGuardian detected hardcoded AWS IAM Keys (AWS secret) in this file. GitGuardian status: Triggered (incident 23895785).

[error] 12-12: GitGuardian detected hardcoded AWS IAM Keys (AWS secret) in this file. GitGuardian status: Triggered (incident 23895792).

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/verify_credentials.php` around lines 8 - 13, The patterns array in
verify_credentials.php currently contains real AWS credential literals (the
'patterns' variable in the file) which must be removed; replace those hard-coded
secrets with non-sensitive placeholders or mock test values and load any real
credentials from environment variables or a secure secrets store (e.g., getenv
or a config loader) instead, and update any tests or verification logic to use
injected/mock patterns so no real credentials are committed.


$envVars = [
'AWS_TEMP_BUCKET_KEY',
'AWS_TEMP_BUCKET_SECRET',
'AWS_PERMANENT_S3_ACCESS_KEY_ID',
'AWS_PERMANENT_S3_SECRET_ACCESS_KEY'
];

$errors = [];

foreach ($files as $file) {
$content = file_get_contents($file);
if ($content === false) {
$errors[] = "Could not read file: $file";
continue;
}

foreach ($patterns as $pattern) {
if (strpos($content, $pattern) !== false) {
$errors[] = "Found hardcoded credential '$pattern' in $file";
}
}

foreach ($envVars as $envVar) {
if (strpos($content, "getenv('$envVar')") === false) {
// Check if it's the right file for the env var
if ($file === 'common/config/main.php' && (strpos($envVar, 'TEMP') !== false)) {
$errors[] = "Missing getenv('$envVar') in $file";
}
if ($file === 'environments/prod-railway/common/config/main-local.php' && (strpos($envVar, 'PERMANENT') !== false)) {
$errors[] = "Missing getenv('$envVar') in $file";
}
Comment on lines +37 to +45
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make getenv checks syntax-tolerant and file-explicit.

The current check only matches the exact text getenv('VAR') plus TEMP/PERMANENT name heuristics, so valid variants (double quotes/spacing/wrappers) can false-fail and the verifier can false-pass on real regressions.

Suggested hardening 
-$envVars = [
-    'AWS_TEMP_BUCKET_KEY',
-    'AWS_TEMP_BUCKET_SECRET',
-    'AWS_PERMANENT_S3_ACCESS_KEY_ID',
-    'AWS_PERMANENT_S3_SECRET_ACCESS_KEY'
-];
+$expectedEnvVarsByFile = [
+    'common/config/main.php' => [
+        'AWS_TEMP_BUCKET_KEY',
+        'AWS_TEMP_BUCKET_SECRET',
+    ],
+    'environments/prod-railway/common/config/main-local.php' => [
+        'AWS_PERMANENT_S3_ACCESS_KEY_ID',
+        'AWS_PERMANENT_S3_SECRET_ACCESS_KEY',
+    ],
+];
@@
-    foreach ($envVars as $envVar) {
-        if (strpos($content, "getenv('$envVar')") === false) {
-            // Check if it's the right file for the env var
-            if ($file === 'common/config/main.php' && (strpos($envVar, 'TEMP') !== false)) {
-                 $errors[] = "Missing getenv('$envVar') in $file";
-            }
-            if ($file === 'environments/prod-railway/common/config/main-local.php' && (strpos($envVar, 'PERMANENT') !== false)) {
-                 $errors[] = "Missing getenv('$envVar') in $file";
-            }
-        }
-    }
+    foreach ($expectedEnvVarsByFile[$file] as $envVar) {
+        $pattern = '/getenv\(\s*[\'"]' . preg_quote($envVar, '/') . '[\'"]\s*\)/';
+        if (!preg_match($pattern, $content)) {
+            $errors[] = "Missing getenv(...) for $envVar in $file";
+        }
+    }
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/verify_credentials.php` around lines 37 - 45, The current strpos
check in the foreach loop (using $envVars/$envVar against $content) only matches
the exact string "getenv('$envVar')" and the TEMP/PERMANENT heuristics can both
false-fail and false-pass; replace the simple strpos with a regex test that
accepts common syntax variants (e.g., getenv\s*\(\s*['"]ENV['"]\s*\), allowing
single/double quotes and spaces/wrappers) when checking $content for each
$envVar, and replace the name-heuristic file checks with explicit mapping of
which env names must appear in which files (use a lookup like an associative
array keyed by filename listing required env vars) and push errors into $errors
when the regex does not find the env in the mapped file; reference
$envVars/$envVar, $content, $file, $errors in your changes.

}
}
}

if (empty($errors)) {
echo "Verification PASSED: No hardcoded credentials found and environment variables are used.\n";
exit(0);
} else {
echo "Verification FAILED:\n";
foreach ($errors as $error) {
echo "- $error\n";
}
exit(1);
}