fix(security): replace hardcoded S3 credentials with environment vari…#99
Open
freeediting35-png wants to merge 1 commit into
Open
fix(security): replace hardcoded S3 credentials with environment vari…#99freeediting35-png wants to merge 1 commit into
freeediting35-png wants to merge 1 commit into
Conversation
68 tasks
📝 WalkthroughWalkthroughThe PR migrates AWS credentials from hardcoded values to environment variables across two configuration files and introduces a verification script to validate the pattern is correctly applied throughout the codebase. ChangesCredentials Migration and Verification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | AWS IAM Keys | 056b4ff | scripts/verify_credentials.php | View secret |
| - | - | AWS IAM Keys | 056b4ff | scripts/verify_credentials.php | View secret |
| 23895785 | Triggered | AWS IAM Keys | 056b4ff | scripts/verify_credentials.php | View secret |
| 23895792 | Triggered | AWS IAM Keys | 056b4ff | scripts/verify_credentials.php | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Related Knowledge 2 documents with suggested updates are ready for review. BAWES Universe Civil ID Upload FlowView Suggested Changes@@ -74,22 +74,19 @@
### Temporary Bucket
-The temporary bucket is configured as a Yii2 component named `temporaryBucketResourceManager` in `common/config/main.php`. It uses static key-and-secret credentials [[1]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/config/main.php#L8-L18):
+The temporary bucket is configured as a Yii2 component named `temporaryBucketResourceManager` in `common/config/main.php`. The key and secret are loaded from environment variables [[1]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/config/main.php#L8-L18):
```php
// common/config/main.php
'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'
// Access URL: https://studenthub-public-anyone-can-upload-24hr-expiry.s3.amazonaws.com/
],
```
-
-> ⚠️ The key/secret for the temp bucket are committed to `common/config/main.php`. These credentials are deliberately limited to upload-only access on a bucket with a 24-hour object expiry policy and no sensitive data. That said, consider migrating these to environment variables (`AWS_TEMP_BUCKET_KEY` / `AWS_TEMP_BUCKET_SECRET`) for consistency with production practice.
-
The key and secret are **also** read from environment variables for the parameters array that is served to the frontend [[9]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/config/params.php#L18-L19):
@@ -118,6 +115,19 @@
],
```
+**Production Railway** (`environments/prod-railway/common/config/main-local.php`) uses key-and-secret authentication loaded from environment variables:
+
+```php
+'resourceManager' => [
+ 'class' => 'common\components\S3ResourceManager',
+ 'authMethod' => \common\components\S3ResourceManager::AUTH_VIA_KEY_AND_SECRET,
+ 'region' => 'eu-west-2',
+ 'bucket' => 'studenthub-uploads',
+ 'key' => getenv('AWS_PERMANENT_S3_ACCESS_KEY_ID'),
+ 'secret' => getenv('AWS_PERMANENT_S3_SECRET_ACCESS_KEY'),
+],
+```
+
**Dev / Staging** (`environments/dev/common/config/main-local.php`) [[10]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/environments/dev/common/config/main-local.php#L82-L93):
```php
@@ -130,7 +140,7 @@
],
```
-Both environments use `AUTH_VIA_IAM_ROLE`, meaning no access key or secret is stored in config — the EC2/ECS instance's attached IAM role is used automatically by the AWS SDK. [[11]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3ResourceManager.php#L263-L283)
+Both environments use `AUTH_VIA_IAM_ROLE`, meaning no access key or secret is stored in config — the EC2/ECS instance's attached IAM role is used automatically by the AWS SDK. [[11]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3ResourceManager.php#L263-L283) The Railway production environment uses `AUTH_VIA_KEY_AND_SECRET` with credentials loaded from environment variables.
***
@@ -151,8 +161,10 @@
| Variable | Mapped To | Used For |
|---|---|---|
-| `AWS_TEMP_BUCKET_KEY` | `params['aws_temp_access_key_id']` | Temp bucket key served to frontend via `/aws/config` |
-| `AWS_TEMP_BUCKET_SECRET` | `params['aws_temp_secret_access_key']` | Temp bucket secret served to frontend via `/aws/config` |
+| `AWS_TEMP_BUCKET_KEY` | `temporaryBucketResourceManager.key` + `params['aws_temp_access_key_id']` | AWS access key ID for the temporary S3 bucket (studenthub-public-anyone-can-upload-24hr-expiry) |
+| `AWS_TEMP_BUCKET_SECRET` | `temporaryBucketResourceManager.secret` + `params['aws_temp_secret_access_key']` | AWS secret access key for the temporary S3 bucket |
+| `AWS_PERMANENT_S3_ACCESS_KEY_ID` | `resourceManager.key` (Railway prod) | AWS access key ID for the permanent S3 bucket (studenthub-uploads) |
+| `AWS_PERMANENT_S3_SECRET_ACCESS_KEY` | `resourceManager.secret` (Railway prod) | AWS secret access key for the permanent S3 bucket |
| `AWS_TEXTRACT_ACCESS_KEY_ID` | `idExpiryDateExtractor.key` | AWS Textract for future ID OCR extraction |
| `AWS_TEXTRACT_SECRET_ACCESS_KEY` | `idExpiryDateExtractor.secret` | AWS Textract for future ID OCR extraction |
@@ -599,6 +611,8 @@
|---|---|---|
| `AWS_TEMP_BUCKET_KEY` | All environments | IAM access key ID for the temporary upload bucket |
| `AWS_TEMP_BUCKET_SECRET` | All environments | IAM secret access key for the temporary upload bucket |
+| `AWS_PERMANENT_S3_ACCESS_KEY_ID` | Railway prod | AWS access key ID for the permanent S3 bucket (studenthub-uploads) |
+| `AWS_PERMANENT_S3_SECRET_ACCESS_KEY` | Railway prod | AWS secret access key for the permanent S3 bucket |
| `AWS_TEXTRACT_ACCESS_KEY_ID` | All environments | Access key for AWS Textract (ID OCR extraction) |
| `AWS_TEXTRACT_SECRET_ACCESS_KEY` | All environments | Secret key for AWS Textract (ID OCR extraction) |
@@ -684,16 +698,10 @@
#### 🔴 High Priority: Temp Bucket Credentials Are Not Time-Limited
-The `/aws/config` endpoint returns long-lived static credentials [[57]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/admin/modules/v1/controllers/AwsController.php#L60-L65). The `// todo: key with expiry` comment in the controller acknowledges this gap. If these credentials are leaked or intercepted, they remain valid indefinitely.
+The `/aws/config` endpoint returns credentials loaded from environment variables [[57]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/admin/modules/v1/controllers/AwsController.php#L60-L65). The `// todo: key with expiry` comment in the controller acknowledges this gap. If these credentials are leaked or intercepted, they remain valid indefinitely.
**Recommendation:** Replace the static key/secret with **AWS STS `AssumeRole` temporary credentials** that expire in 15–60 minutes. The `/aws/config` endpoint would call `sts:AssumeRole` each request and return short-lived `AccessKeyId`, `SecretAccessKey`, and `SessionToken`.
-#### 🟡 Medium Priority: Temp Bucket Credentials Hardcoded in Source
-
-The `temporaryBucketResourceManager` component in `common/config/main.php` contains a hardcoded key and secret [[58]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/config/main.php#L11-L12). While the params array reads from environment variables for the frontend-facing values, the component definition itself does not.
-
-**Recommendation:** Change `common/config/main.php` to read the key and secret from environment variables, matching the pattern already used in `params.php`.
-
#### 🟡 Medium Priority: No Rate Limiting on Upload Endpoints
`POST /v1/account/update-civil-photo-front` and `POST /v1/account/update-civil-photo-back` have no rate limiting. An authenticated candidate could issue many upload requests in quick succession.DB, API endpoint, S3, Slack, and payment keysView Suggested Changes@@ -14,11 +14,15 @@
# Not found in main.php. Set this to your backend API base URL.
API_BASE_URL=http://localhost:8000/api
-# --- S3 Configuration (temporaryBucketResourceManager) ---
-S3_REGION=eu-west-2
-S3_KEY=your_s3_access_key
-S3_SECRET=your_s3_secret_key
-S3_BUCKET=plugn-public-anyone-can-upload-24hr-expiry
+# --- S3 Configuration ---
+
+# Temporary Bucket (common/config/main.php)
+AWS_TEMP_BUCKET_KEY=your_aws_temp_bucket_access_key_id
+AWS_TEMP_BUCKET_SECRET=your_aws_temp_bucket_secret_access_key
+
+# Permanent Bucket (environments/prod-railway/common/config/main-local.php)
+AWS_PERMANENT_S3_ACCESS_KEY_ID=your_aws_permanent_s3_access_key_id
+AWS_PERMANENT_S3_SECRET_ACCESS_KEY=your_aws_permanent_s3_secret_access_key
# --- Slack Configuration ---
SLACK_URL=https://hooks.slack.com/services/your/slack/webhook
@@ -69,5 +73,6 @@
**Notes:**
- Replace all `your_*` values with your actual credentials or secrets.
- The S3, Slack, and payment keys are directly mapped from the structure in [`common/config/main.php`](https://github.com/BAWES-Universe/plugn/blob/bc485b0a1da61d516955c5dc4fc29e95afccea92/common/config/main.php#L3-L172).
+- S3 credentials are configured via environment variables: `AWS_TEMP_BUCKET_KEY` and `AWS_TEMP_BUCKET_SECRET` for the temporary bucket (studenthub-public-anyone-can-upload-24hr-expiry) in `common/config/main.php`, and `AWS_PERMANENT_S3_ACCESS_KEY_ID` and `AWS_PERMANENT_S3_SECRET_ACCESS_KEY` for the permanent bucket (studenthub-uploads) in `environments/prod-railway/common/config/main-local.php`.
- Database and API endpoint keys are placeholders, as they are not defined in `main.php` and may be set elsewhere in your project.
- All other configuration keys in `main.php` are optional and can be set if your local environment requires them.How did I do? Any feedback? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@scripts/verify_credentials.php`:
- Around line 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.
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2a99cd9-a702-4f16-aad6-f8ee812cbaac
📒 Files selected for processing (3)
common/config/main.phpenvironments/prod-railway/common/config/main-local.phpscripts/verify_credentials.php
Comment on lines
+8
to
+13
| $patterns = [ | ||
| 'AKIAWMITDJRKVN5ODY2X', | ||
| 'zAr8Xov1olqBAaiE8CX+j45qDHaAbO+S3EhUVeaT', | ||
| 'AKIAWMITDJRKWZZEWCUM', | ||
| 'M6olF9l1pZ1sKIswrSCjKtGkAG2w9qDV9x230UlI' | ||
| ]; |
There was a problem hiding this comment.
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.
Comment on lines
+37
to
+45
| 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"; | ||
| } |
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: replace hardcoded S3 credentials with environment variables #55 /claim #55
Summary by CodeRabbit