Fix Civil ID S3 hardening and env-based S3 credentials#71
Open
mzgamers28-cmyk wants to merge 13 commits into
Open
Fix Civil ID S3 hardening and env-based S3 credentials#71mzgamers28-cmyk wants to merge 13 commits into
mzgamers28-cmyk wants to merge 13 commits into
Conversation
68 tasks
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR externalizes S3 credentials to environment variables across all deployment environments, hardens Civil ID file operations with validation and error handling, and improves S3 resource manager methods for file existence detection and deletion. ChangesS3 Credentials and Civil ID Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Related Knowledge 2 documents with suggested updates are ready for review. BAWES Universe Civil ID Upload FlowView Suggested Changes@@ -81,14 +81,12 @@
'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.
@@ -118,6 +116,19 @@
],
```
+**Production Railway** (`environments/prod-railway/common/config/main-local.php`) uses environment variables for credentials:
+
+```php
+'resourceManager' => [
+ 'class' => 'common\components\S3ResourceManager',
+ 'authMethod' => \common\components\S3ResourceManager::AUTH_VIA_KEY_AND_SECRET,
+ 'region' => getenv('AWS_PERMANENT_S3_REGION') ?: 'eu-west-2',
+ 'bucket' => getenv('AWS_PERMANENT_S3_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
@@ -151,8 +162,12 @@
| 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` | `params['aws_temp_access_key_id']` and `temporaryBucketResourceManager.key` | Temp bucket key served to frontend via `/aws/config` |
+| `AWS_TEMP_BUCKET_SECRET` | `params['aws_temp_secret_access_key']` and `temporaryBucketResourceManager.secret` | Temp bucket secret served to frontend via `/aws/config` |
+| `AWS_PERMANENT_S3_ACCESS_KEY_ID` | `resourceManager.key` (Railway/docker environments) | Permanent bucket access key for Railway/docker |
+| `AWS_PERMANENT_S3_SECRET_ACCESS_KEY` | `resourceManager.secret` (Railway/docker environments) | Permanent bucket secret for Railway/docker |
+| `AWS_PERMANENT_S3_REGION` | `resourceManager.region` (Railway/docker environments) | Permanent bucket region for Railway/docker |
+| `AWS_PERMANENT_S3_BUCKET` | `resourceManager.bucket` (Railway/docker environments) | Permanent bucket name for Railway/docker |
| `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 |
@@ -411,26 +426,48 @@
? 'candidate_civil_photo_front'
: 'candidate_civil_photo_back';
- // Delete previous file from permanent bucket if one exists
- if (!empty($this->oldAttributes[$idSide])) {
- $this->deleteFile('civil-id', $side);
- }
-
- $fileName = $this->$idSide;
+ $fileName = $this->$idSide;
+
+ if (!$fileName) {
+ $this->addError($idSide, Yii::t('app', 'file not available to save.'));
+
+ return false;
+ }
+
$sourceBucket = Yii::$app->temporaryBucketResourceManager->bucket;
- $targetPath = "photos/" . $fileName;
-
+
+ $targetPath = strpos($fileName, 'photos/') === 0 ? $fileName : "photos/" . $fileName;
+
+ // Copy using S3ResourceManager Component
+
try {
- return Yii::$app->resourceManager->copy($fileName, $targetPath, $sourceBucket);
+
+ $result = Yii::$app->resourceManager->copy($fileName, $targetPath, $sourceBucket);
+
+ if (!Yii::$app->resourceManager->fileExists($targetPath)) {
+ throw new \RuntimeException('Copied Civil ID file was not found at destination.');
+ }
+
+ if (!empty($this->oldAttributes[$idSide]) && $this->oldAttributes[$idSide] !== $fileName) {
+ $this->deleteFile('civil-id', $side);
+ }
+
+ return $result;
} catch (\Aws\S3\Exception\S3Exception $e) {
+
Yii::error($e->getMessage(), 'candidate');
+
$this->addError($idSide, Yii::t('app', 'file not available to save.'));
+
return false;
} catch (\Exception $e) {
+
Yii::error($e->getMessage(), 'candidate');
+
$this->addError($idSide, Yii::t('app', 'file not available to save.'));
+
return false;
}
}
@@ -438,8 +475,9 @@
Key behaviours:
-- **Old file cleanup:** If the candidate already has a civil photo stored (`oldAttributes[$idSide]` is set), the old file is deleted from the permanent bucket before the new one is copied. This prevents orphaned files accumulating.
-- **Target path:** Files are stored as `photos/{filename}` in the permanent bucket.
+- **Copy then verify:** The new file is first copied to the permanent bucket, then its existence is verified using `fileExists()`. Only after confirmation does the old file get deleted. This ordering prevents data loss if the copy fails.
+- **Best-effort old file cleanup:** If the candidate already has a civil photo stored (`oldAttributes[$idSide]` is set) and it differs from the new filename, the old file is deleted from the permanent bucket. The deletion is wrapped in try-catch logic in `deleteFile()`, and for `'civil-id'` type deletions, failures are logged but do not block the operation.
+- **Target path:** Files are stored as `photos/{filename}` in the permanent bucket. The code handles both legacy paths and the current convention.
- **Error handling:** Both `S3Exception` and generic `\Exception` are caught. Errors are logged to the `candidate` category (routed to Slack/Sentry) and surfaced as a model validation error.
***
@@ -476,6 +514,60 @@
$sourceBucket // "studenthub-public-anyone-can-upload-24hr-expiry"
);
```
+
+***
+
+### S3ResourceManager — fileExists()
+
+**File:** `common/components/S3ResourceManager.php`
+
+The `fileExists()` method checks whether a file exists in S3. For S3 object keys (non-URL strings), it uses the SDK's `headObject()` call [[60]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3ResourceManager.php#L174-L188):
+
+```php
+public function fileExists($filenameOrUrl)
+{
+ if (!$filenameOrUrl) {
+ return false;
+ }
+
+ $isUrl = false;
+ if (strpos($filenameOrUrl, 'http') === 0) {
+ $isUrl = true;
+ }
+
+ if (!$isUrl) {
+ try {
+ $this->getClient()->headObject([
+ 'Bucket' => $this->bucket,
+ 'Key' => $filenameOrUrl,
+ ]);
+
+ return true;
+ } catch (AwsException $e) {
+ if ((int) $e->getStatusCode() === 404 || $e->getAwsErrorCode() === 'NoSuchKey' || $e->getAwsErrorCode() === 'NotFound') {
+ return false;
+ }
+
+ Yii::warning('Unable to check S3 object existence for bucket=' . $this->bucket . ' key=' . $filenameOrUrl . ': ' . $e->getMessage(), 's3');
+
+ return false;
+ } catch (\Exception $e) {
+ Yii::warning('Unable to check S3 object existence for bucket=' . $this->bucket . ' key=' . $filenameOrUrl . ': ' . $e->getMessage(), 's3');
+
+ return false;
+ }
+ }
+
+ // If URL, fall back to HTTP HEAD request
+ $http = new \GuzzleHttp\Client(['base_uri' => $isUrl ? $filenameOrUrl : $this->getUrl($filenameOrUrl)]);
+ try {
+ $response = $http->request('HEAD');
+ // ...
+ }
+}
+```
+
+For URL strings, the method falls back to an HTTP HEAD request. The SDK-based approach works for both public and private objects as long as the configured credentials have appropriate permissions.
***
@@ -599,6 +691,10 @@
|---|---|---|
| `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, docker, CircleCI environments | IAM access key ID for the permanent bucket (Railway/docker only; production/dev use IAM roles) |
+| `AWS_PERMANENT_S3_SECRET_ACCESS_KEY` | Railway, docker, CircleCI environments | IAM secret access key for the permanent bucket (Railway/docker only; production/dev use IAM roles) |
+| `AWS_PERMANENT_S3_REGION` | Railway, docker environments | AWS region for the permanent bucket |
+| `AWS_PERMANENT_S3_BUCKET` | Railway, docker environments | Bucket name for the permanent 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) |
@@ -660,10 +756,12 @@
- The credentials returned by `/aws/config` should be scoped to `s3:PutObject` on this bucket only (verify this in your IAM policy).
- The CORS filter on `/aws/config` restricts the response to `allowedOrigins` — browser-based attackers from other origins cannot retrieve the credentials. [[53]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/admin/modules/v1/controllers/AwsController.php#L17-L36)
-#### Permanent Bucket: IAM Role, No Keys in Config
+#### Permanent Bucket: IAM Role or Environment Variables Depending on Environment
Production and dev servers access the permanent bucket via an attached IAM role. No access key or secret is stored in the server's configuration files for `resourceManager`. This is the correct posture. [[5]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/environments/prod/common/config/main-local.php#L152-L165) [[10]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/environments/dev/common/config/main-local.php#L82-L93)
+Railway and docker environments use `AUTH_VIA_KEY_AND_SECRET` and read credentials from environment variables (`AWS_PERMANENT_S3_ACCESS_KEY_ID`, `AWS_PERMANENT_S3_SECRET_ACCESS_KEY`, `AWS_PERMANENT_S3_REGION`, `AWS_PERMANENT_S3_BUCKET`) rather than hardcoded values in config files.
+
#### S3FileExistValidator Guards the Copy Operation
Before the backend copies a file from the temp bucket, it validates that the key actually exists in S3 (`HEAD` request via GuzzleHttp). This prevents an attacker from specifying an arbitrary S3 key and tricking the backend into copying a file they did not upload. [[54]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3FileExistValidator.php#L46-L58)
@@ -688,12 +786,6 @@
**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.
@@ -706,6 +798,6 @@
**Recommendation:** Consider integrating an AWS Lambda function triggered on temp-bucket `PutObject` events to run ClamAV or a commercial AV solution before the backend is permitted to copy the file.
-#### 🟢 Low Priority: `fileExists()` Uses HTTP HEAD (Public Access Required)
-
-The `S3FileExistValidator` checks file existence using a GuzzleHttp `HEAD` request to the public URL of the file [[60]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3ResourceManager.php#L174-L188). This means the file must be publicly accessible in the temp bucket for validation to work. If the ACL recommendation above is ever applied to the temp bucket, this validation approach would need to be replaced with an SDK `headObject` call using credentials.
+#### 🟢 Low Priority: fileExists() Now Uses SDK-Based headObject for Non-URL Keys
+
+The `S3ResourceManager::fileExists()` method now uses the AWS SDK's `headObject()` call for S3 object keys (non-URL strings) rather than relying purely on HTTP HEAD requests [[60]](https://github.com/BAWES-Universe/studenthub/blob/7b023ff371b8a33dc49a350984a5adf263d120f2/common/components/S3ResourceManager.php#L174-L188). This approach works for both public and private objects as long as the configured credentials have appropriate permissions. For URL strings, the method still falls back to an HTTP HEAD request.DB, API endpoint, S3, Slack, and payment keysView Suggested Changes@@ -14,11 +14,18 @@
# 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 (24hr expiry for uploads)
+AWS_TEMP_BUCKET_KEY=your_temp_bucket_access_key
+AWS_TEMP_BUCKET_SECRET=your_temp_bucket_secret_key
+
+# Permanent Bucket (Railway/Docker environments)
+# Note: CircleCI uses IAM roles instead of these env vars
+AWS_PERMANENT_S3_ACCESS_KEY_ID=your_permanent_s3_access_key_id
+AWS_PERMANENT_S3_SECRET_ACCESS_KEY=your_permanent_s3_secret_access_key
+AWS_PERMANENT_S3_REGION=eu-west-2
+AWS_PERMANENT_S3_BUCKET=studenthub-uploads-dev-server
# --- Slack Configuration ---
SLACK_URL=https://hooks.slack.com/services/your/slack/webhook
@@ -68,6 +75,7 @@
**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).
+- The S3 configuration includes both temporary bucket credentials (for 24-hour expiry uploads) and permanent bucket credentials (used in Railway/Docker environments). CircleCI uses IAM roles for S3 access rather than environment variables.
+- 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).
- 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
environments/prod-railway/common/config/main-local.php (1)
154-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate comment to reflect Railway's intentional use of key/secret authentication.
Line 154 sets
authMethodtoAUTH_VIA_KEY_AND_SECRET, but the comment at lines 160-161 states "For Dev and Production servers, access is via server embedded IAM roles so no key/secret required." This comment is incorrect for the Railway environment—Railway requires key/secret authentication and does not support IAM roles in the same way as standard AWS EC2 deployments.Update the comment to clarify that this Railway production environment deliberately uses key/secret authentication, or remove the misleading comment about IAM roles if it's inapplicable here.
🤖 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 `@environments/prod-railway/common/config/main-local.php` around lines 154 - 166, The comment incorrectly implies production uses embedded IAM roles; update the block around authMethod (set to \common\components\S3ResourceManager::AUTH_VIA_KEY_AND_SECRET) to state that the Railway environment intentionally uses key/secret authentication and that IAM role-based access is not available here; adjust or remove the lines mentioning "For Dev and Production servers, access is via server embedded IAM roles so no key/secret required" and optionally note the relevant env vars (AWS_PERMANENT_S3_ACCESS_KEY_ID, AWS_PERMANENT_S3_SECRET_ACCESS_KEY) are required in this deployment.environments/dev-server-railway/common/config/main-local.php (1)
106-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winContradiction confirmed: auth method setup conflicts with IAM role comment across all Railway configs.
Both
dev-server-railwayandprod-railwayhave identical configuration:authMethodis explicitly set toAUTH_VIA_KEY_AND_SECRET, yet the comment states "access is via server embedded IAM roles so no key/secret required."The S3ResourceManager validation accepts empty strings (checks
=== null), so if the AWS environment variables aren't set at runtime, empty credentials will be passed to S3Client, causing failures.To resolve:
- If AWS credentials are always provided via environment variables in Railway: Update the comment to clarify that key/secret auth is used, not IAM roles (e.g., "Credentials sourced from AWS_PERMANENT_S3_* environment variables").
- If IAM roles should be used instead: Change
authMethodto\common\components\S3ResourceManager::AUTH_VIA_IAM_ROLEand remove thekeyandsecretentries. This allows the SDK's default credential chain to handle IAM role assumption.🤖 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 `@environments/dev-server-railway/common/config/main-local.php` around lines 106 - 117, The config currently sets 'authMethod' => \common\components\S3ResourceManager::AUTH_VIA_KEY_AND_SECRET while the comment claims IAM role usage and the file still defines 'key' and 'secret' from AWS_PERMANENT_S3_* env vars; because S3ResourceManager treats empty strings as valid values this can pass empty credentials into S3Client. Either (A) if Railway provides keys via AWS_PERMANENT_S3_ACCESS_KEY_ID / AWS_PERMANENT_S3_SECRET_ACCESS_KEY, update the comment to state that credentials are sourced from those env vars and keep authMethod as AUTH_VIA_KEY_AND_SECRET, or (B) if IAM role chaining should be used, change authMethod to \common\components\S3ResourceManager::AUTH_VIA_IAM_ROLE and remove the 'key' and 'secret' entries so the SDK default credential provider (IAM role) is used; ensure the chosen option is applied consistently across dev-server-railway and prod-railway.
🧹 Nitpick comments (2)
candidate/tests/functional/AccountCest.php (1)
468-493: ⚡ Quick winAdd a regression test for missing
civil_expiry_dateNice additions. One branch added in the controller is still untested: empty/missing
civil_expiry_dateshould returnoperation=errorwith"Civil ID Expiry Date is required".Suggested test addition
+ public function tryUpdateCivilExpiryRejectsMissingExpiryDate(FunctionalTester $I) + { + $I->amGoingTo('try to update civil id expiry date without expiry date'); + $I->sendPOST('v1/account/update-civil-id-expiry-date', [ + 'civil_id' => '70' + ]); + $I->seeResponseCodeIs(HttpCode::OK); // 200 + $I->seeResponseContainsJson([ + 'operation' => 'error', + 'message' => 'Civil ID Expiry Date is required' + ]); + }🤖 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 `@candidate/tests/functional/AccountCest.php` around lines 468 - 493, Add a new functional test that covers the missing civil_expiry_date branch: create a method (e.g. tryUpdateCivilExpiryRejectsMissingExpiryDate) modeled like tryUpdateCivilExpiryRejectsMissingCivilId and tryUpdateCivilExpiryRejectsInvalidDate that sends a POST to 'v1/account/update-civil-id-expiry-date' including a valid 'civil_id' but omitting 'civil_expiry_date', then assert HttpCode::OK and that the JSON response contains 'operation' => 'error' and 'message' => 'Civil ID Expiry Date is required' to cover the untested controller branch.README.md (1)
35-45: ⚡ Quick winEnhance S3 environment variable documentation.
The documentation lists the required environment variables but lacks critical details. Consider adding:
- Which variables are required vs optional for each environment
- Example values or format guidance
- Which deployment environments require which subset of variables
- Links to setup instructions or deployment notes
📝 Proposed documentation enhancement
## S3 Environment Variables -The application expects S3 credentials to be supplied through environment variables rather than committed config values. +The application requires S3 credentials to be supplied through environment variables rather than committed config values. + +### Temporary Bucket (24-hour expiry uploads) +Required for all environments: - `AWS_TEMP_BUCKET_KEY` - `AWS_TEMP_BUCKET_SECRET` + +### Permanent Storage Bucket +Required for production and Railway deployments: + - `AWS_PERMANENT_S3_ACCESS_KEY_ID` - `AWS_PERMANENT_S3_SECRET_ACCESS_KEY` - `AWS_PERMANENT_S3_REGION` - `AWS_PERMANENT_S3_BUCKET` + +See deployment notes in this PR for setup instructions.🤖 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 `@README.md` around lines 35 - 45, Update the "S3 Environment Variables" README section to clearly mark which env vars are required vs optional, provide example values/formats (e.g., AWS_TEMP_BUCKET_KEY=AKIA... , AWS_PERMANENT_S3_REGION=us-east-1), indicate which deployment environments (local dev, staging, production, CI) need each subset of variables, and add links to setup/deployment guides or AWS IAM/S3 credential docs; reference the exact variable names listed (AWS_TEMP_BUCKET_KEY, AWS_TEMP_BUCKET_SECRET, AWS_PERMANENT_S3_ACCESS_KEY_ID, AWS_PERMANENT_S3_SECRET_ACCESS_KEY, AWS_PERMANENT_S3_REGION, AWS_PERMANENT_S3_BUCKET) so readers can map requirements to their environment.
🤖 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 `@candidate/modules/v1/controllers/AccountController.php`:
- Around line 1489-1496: The code uses strtotime on $candidate_civil_expiry_date
which is too permissive; replace this with strict parsing using
DateTime::createFromFormat (e.g. 'Y-m-d' or the expected format) and validate
with DateTime::getLastErrors to ensure the input is a calendar-valid date, then
use ->getTimestamp() for $candidate_civil_expiry_timestamp; if parsing or
getLastErrors indicates problems, return the same error response. Ensure you
update the logic around $candidate_civil_expiry_timestamp and the existing error
return so invalid/malformed dates are rejected.
In `@common/components/S3ResourceManager.php`:
- Line 165: The delete() method is incorrectly treating DeleteMarker=false as a
failure; instead, rely on the absence of exceptions/SDK errors to indicate
success and always return true after a successful S3 delete call. In
S3ResourceManager::delete(), stop interpreting $result['DeleteMarker'] as
failure — replace the current return expression with an unconditional true (so
successful SDK responses return true) and ensure any exceptions from the S3
client continue to propagate or are handled upstream.
In `@common/config/main.php`:
- Around line 11-12: The config currently uses empty-string fallbacks for
AWS_TEMP_BUCKET_KEY and AWS_TEMP_BUCKET_SECRET which defers credential errors;
change the fallbacks from '' to null (or false) in the configuration array so
the AWS SDK can use its default credential chain or fail fast, and add a startup
validation step that checks the AWS_TEMP_BUCKET_KEY/AWS_TEMP_BUCKET_SECRET
values (or SDK credential resolution result) and throws a clear, early
configuration error if no credentials are available; update references to these
keys in the code that constructs the S3 client (e.g., the S3 client creation
path) to rely on null rather than empty strings.
In `@common/models/Candidate.php`:
- Around line 2786-2788: Normalize the S3 keys before comparing old vs new to
avoid deleting a freshly-uploaded file: when checking
$this->oldAttributes[$idSide] !== $fileName in Candidate.php, canonicalize both
values (e.g., strip known prefixes like "photos/", normalize leading/trailing
slashes or use basename-equivalent) and then compare those normalized strings;
only call $this->deleteFile('civil-id', $side) if the normalized old key is
non-empty and different from the normalized new key. Ensure you apply the
normalization consistently to both the value from $this->oldAttributes[$idSide]
and $fileName.
In `@environments/circle-ci/common/config/main-local.php`:
- Around line 74-75: The config currently falls back to empty strings for 'key'
and 'secret' (getenv('AWS_TEMP_BUCKET_KEY') ?: '' and
getenv('AWS_TEMP_BUCKET_SECRET') ?: ''), which lets AWS init succeed but fails
later; change these fallbacks to null (e.g., return null instead of '') so the
AWS SDK's credential provider chain is used, or add an explicit early validation
that throws/terminates if either getenv('AWS_TEMP_BUCKET_KEY') or
getenv('AWS_TEMP_BUCKET_SECRET') is missing to fail fast in CI; update the
entries for 'key' and 'secret' accordingly and, if adding validation, place it
during config/bootstrap so missing credentials cause a clear startup error.
In `@environments/dev-server-railway/common/config/main-local.php`:
- Around line 109-110: The config currently uses empty-string fallbacks for
'key' and 'secret' which hides missing AWS credentials; change the getenv
fallback from '' to null so the AWS SDK can use its default credential chain
(i.e., set the 'key' => getenv('AWS_PERMANENT_S3_ACCESS_KEY_ID') ?: null and
'secret' => getenv('AWS_PERMANENT_S3_SECRET_ACCESS_KEY') ?: null), and
optionally add a startup validation routine that checks these config values (or
the resolved SDK credentials) and throws a clear error if neither environment
credentials nor an SDK provider are available; update references in the config
array where 'key' and 'secret' are defined to apply this change.
In `@environments/docker/common/config/main-local.php`:
- Around line 93-94: The config currently falls back to empty strings for 'key'
and 'secret' which defers credential errors; change the fallbacks for
AWS_PERMANENT_S3_ACCESS_KEY_ID and AWS_PERMANENT_S3_SECRET_ACCESS_KEY to null
instead of '' so missing env vars produce immediate/clear errors during SDK
initialization; update the entries for 'key' and 'secret' to return null when
getenv(...) is not set (preserving the getenv calls and variable names) so the
AWS SDK fails fast.
In `@environments/krushn-nginx/common/config/main-local.php`:
- Around line 95-96: The config currently falls back to empty strings for 'key'
and 'secret', which defers credential failures; change the fallbacks so
getenv('AWS_PERMANENT_S3_ACCESS_KEY_ID') and
getenv('AWS_PERMANENT_S3_SECRET_ACCESS_KEY') return null instead of '' (so the
AWS SDK will use its default credential chain or surface a clearer error), e.g.
replace the empty-string fallbacks for the 'key' and 'secret' entries in the
config array with null (or explicitly convert false -> null) and optionally add
environment validation that throws a clear exception if credentials remain null
in environments where they must be present.
In `@environments/krushn/common/config/main-local.php`:
- Around line 94-95: The config currently uses empty-string fallbacks for 'key'
and 'secret' (getenv('AWS_PERMANENT_S3_ACCESS_KEY_ID') ?: '' and
getenv('AWS_PERMANENT_S3_SECRET_ACCESS_KEY') ?: ''), which hides missing env
vars and defers failures; change the fallbacks to null (or remove the
empty-string fallback) so the AWS SDK will use its default credential provider
chain or produce a clear error, and optionally add an explicit check that throws
a clear exception if both values are null to fail fast during initialization.
In `@environments/prod-railway/common/config/main-local.php`:
- Around line 157-158: The config currently falls back to empty strings for the
S3 credentials (the 'key' and 'secret' entries built from
getenv('AWS_PERMANENT_S3_ACCESS_KEY_ID') and
getenv('AWS_PERMANENT_S3_SECRET_ACCESS_KEY')), which defers credential errors to
runtime; update the fallback to null so the AWS SDK can consult its default
credential chain (replace the empty-string fallback with null) and/or add a
startup validation that checks these env vars and throws a clear error (or
exits) if both are missing to fail fast in production; locate the getenv usage
that sets 'key' and 'secret' and implement one of these fixes.
---
Outside diff comments:
In `@environments/dev-server-railway/common/config/main-local.php`:
- Around line 106-117: The config currently sets 'authMethod' =>
\common\components\S3ResourceManager::AUTH_VIA_KEY_AND_SECRET while the comment
claims IAM role usage and the file still defines 'key' and 'secret' from
AWS_PERMANENT_S3_* env vars; because S3ResourceManager treats empty strings as
valid values this can pass empty credentials into S3Client. Either (A) if
Railway provides keys via AWS_PERMANENT_S3_ACCESS_KEY_ID /
AWS_PERMANENT_S3_SECRET_ACCESS_KEY, update the comment to state that credentials
are sourced from those env vars and keep authMethod as AUTH_VIA_KEY_AND_SECRET,
or (B) if IAM role chaining should be used, change authMethod to
\common\components\S3ResourceManager::AUTH_VIA_IAM_ROLE and remove the 'key' and
'secret' entries so the SDK default credential provider (IAM role) is used;
ensure the chosen option is applied consistently across dev-server-railway and
prod-railway.
In `@environments/prod-railway/common/config/main-local.php`:
- Around line 154-166: The comment incorrectly implies production uses embedded
IAM roles; update the block around authMethod (set to
\common\components\S3ResourceManager::AUTH_VIA_KEY_AND_SECRET) to state that the
Railway environment intentionally uses key/secret authentication and that IAM
role-based access is not available here; adjust or remove the lines mentioning
"For Dev and Production servers, access is via server embedded IAM roles so no
key/secret required" and optionally note the relevant env vars
(AWS_PERMANENT_S3_ACCESS_KEY_ID, AWS_PERMANENT_S3_SECRET_ACCESS_KEY) are
required in this deployment.
---
Nitpick comments:
In `@candidate/tests/functional/AccountCest.php`:
- Around line 468-493: Add a new functional test that covers the missing
civil_expiry_date branch: create a method (e.g.
tryUpdateCivilExpiryRejectsMissingExpiryDate) modeled like
tryUpdateCivilExpiryRejectsMissingCivilId and
tryUpdateCivilExpiryRejectsInvalidDate that sends a POST to
'v1/account/update-civil-id-expiry-date' including a valid 'civil_id' but
omitting 'civil_expiry_date', then assert HttpCode::OK and that the JSON
response contains 'operation' => 'error' and 'message' => 'Civil ID Expiry Date
is required' to cover the untested controller branch.
In `@README.md`:
- Around line 35-45: Update the "S3 Environment Variables" README section to
clearly mark which env vars are required vs optional, provide example
values/formats (e.g., AWS_TEMP_BUCKET_KEY=AKIA... ,
AWS_PERMANENT_S3_REGION=us-east-1), indicate which deployment environments
(local dev, staging, production, CI) need each subset of variables, and add
links to setup/deployment guides or AWS IAM/S3 credential docs; reference the
exact variable names listed (AWS_TEMP_BUCKET_KEY, AWS_TEMP_BUCKET_SECRET,
AWS_PERMANENT_S3_ACCESS_KEY_ID, AWS_PERMANENT_S3_SECRET_ACCESS_KEY,
AWS_PERMANENT_S3_REGION, AWS_PERMANENT_S3_BUCKET) so readers can map
requirements to their environment.
🪄 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: d6e26ece-016a-4198-88f1-4b43d505c132
📒 Files selected for processing (12)
README.mdcandidate/modules/v1/controllers/AccountController.phpcandidate/tests/functional/AccountCest.phpcommon/components/S3ResourceManager.phpcommon/config/main.phpcommon/models/Candidate.phpenvironments/circle-ci/common/config/main-local.phpenvironments/dev-server-railway/common/config/main-local.phpenvironments/docker/common/config/main-local.phpenvironments/krushn-nginx/common/config/main-local.phpenvironments/krushn/common/config/main-local.phpenvironments/prod-railway/common/config/main-local.php
Comment on lines
+1489
to
+1496
| $candidate_civil_expiry_timestamp = strtotime($candidate_civil_expiry_date); | ||
|
|
||
| $candidate->scenario = "updateCivilExpiryDateAndCivilID"; | ||
| if ($candidate_civil_expiry_timestamp === false) { | ||
| return [ | ||
| "operation" => "error", | ||
| "message" => Yii::t('candidate', "Civil ID Expiry Date is invalid"), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Use strict calendar validation for civil_expiry_date
strtotime() is too permissive and can accept malformed dates by coercion. This allows invalid civil expiry values to be stored as different valid dates.
Suggested fix
- $candidate_civil_expiry_timestamp = strtotime($candidate_civil_expiry_date);
-
- if ($candidate_civil_expiry_timestamp === false) {
+ $dt = \DateTimeImmutable::createFromFormat('!Y-m-d', $candidate_civil_expiry_date);
+ $dtErrors = \DateTimeImmutable::getLastErrors();
+
+ if (
+ !$dt ||
+ $dtErrors['warning_count'] > 0 ||
+ $dtErrors['error_count'] > 0 ||
+ $dt->format('Y-m-d') !== $candidate_civil_expiry_date
+ ) {
return [
"operation" => "error",
"message" => Yii::t('candidate', "Civil ID Expiry Date is invalid"),
];
}
@@
- $candidate->candidate_civil_expiry_date = date('Y-m-d', $candidate_civil_expiry_timestamp);
+ $candidate->candidate_civil_expiry_date = $dt->format('Y-m-d');🤖 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 `@candidate/modules/v1/controllers/AccountController.php` around lines 1489 -
1496, The code uses strtotime on $candidate_civil_expiry_date which is too
permissive; replace this with strict parsing using DateTime::createFromFormat
(e.g. 'Y-m-d' or the expected format) and validate with DateTime::getLastErrors
to ensure the input is a calendar-valid date, then use ->getTimestamp() for
$candidate_civil_expiry_timestamp; if parsing or getLastErrors indicates
problems, return the same error response. Ensure you update the logic around
$candidate_civil_expiry_timestamp and the existing error return so
invalid/malformed dates are rejected.
Comment on lines
+11
to
+12
| 'key' => getenv('AWS_TEMP_BUCKET_KEY') ?: '', | ||
| 'secret' => getenv('AWS_TEMP_BUCKET_SECRET') ?: '', |
There was a problem hiding this comment.
Empty-string fallbacks silently defer credential failures to runtime.
When AWS_TEMP_BUCKET_KEY or AWS_TEMP_BUCKET_SECRET are unset, the configuration falls back to empty strings. The AWS SDK accepts empty credentials during initialization but fails later when S3 operations are attempted, producing "invalid credentials" errors that are harder to diagnose than a missing-configuration error at startup.
Consider using null or false as the fallback so the application fails fast during initialization if credentials are missing, or add explicit validation at startup.
🔒 Proposed fix to fail fast on missing credentials
- 'key' => getenv('AWS_TEMP_BUCKET_KEY') ?: '',
- 'secret' => getenv('AWS_TEMP_BUCKET_SECRET') ?: '',
+ 'key' => getenv('AWS_TEMP_BUCKET_KEY') ?: null,
+ 'secret' => getenv('AWS_TEMP_BUCKET_SECRET') ?: null,This allows the S3 client to fall back to AWS SDK's default credential chain (environment, IAM role, etc.) or fail explicitly if no credentials are available.
🤖 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 `@common/config/main.php` around lines 11 - 12, The config currently uses
empty-string fallbacks for AWS_TEMP_BUCKET_KEY and AWS_TEMP_BUCKET_SECRET which
defers credential errors; change the fallbacks from '' to null (or false) in the
configuration array so the AWS SDK can use its default credential chain or fail
fast, and add a startup validation step that checks the
AWS_TEMP_BUCKET_KEY/AWS_TEMP_BUCKET_SECRET values (or SDK credential resolution
result) and throws a clear, early configuration error if no credentials are
available; update references to these keys in the code that constructs the S3
client (e.g., the S3 client creation path) to rely on null rather than empty
strings.
Comment on lines
+2786
to
+2788
| if (!empty($this->oldAttributes[$idSide]) && $this->oldAttributes[$idSide] !== $fileName) { | ||
| $this->deleteFile('civil-id', $side); | ||
| } |
There was a problem hiding this comment.
Normalize both old/new keys before deciding deletion
Line 2786 compares oldAttributes[$idSide] to raw $fileName. If old is photos/foo.jpg and new is foo.jpg, this condition is true even though both target the same S3 key, so the subsequent delete can remove the newly copied file.
Suggested fix
- if (!empty($this->oldAttributes[$idSide]) && $this->oldAttributes[$idSide] !== $fileName) {
- $this->deleteFile('civil-id', $side);
- }
+ $oldPath = !empty($this->oldAttributes[$idSide]) ? $this->oldAttributes[$idSide] : null;
+ $oldTargetPath = $oldPath
+ ? (strpos($oldPath, 'photos/') === 0 ? $oldPath : "photos/" . $oldPath)
+ : null;
+
+ if ($oldTargetPath && $oldTargetPath !== $targetPath) {
+ $this->deleteFile('civil-id', $side);
+ }🤖 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 `@common/models/Candidate.php` around lines 2786 - 2788, Normalize the S3 keys
before comparing old vs new to avoid deleting a freshly-uploaded file: when
checking $this->oldAttributes[$idSide] !== $fileName in Candidate.php,
canonicalize both values (e.g., strip known prefixes like "photos/", normalize
leading/trailing slashes or use basename-equivalent) and then compare those
normalized strings; only call $this->deleteFile('civil-id', $side) if the
normalized old key is non-empty and different from the normalized new key.
Ensure you apply the normalization consistently to both the value from
$this->oldAttributes[$idSide] and $fileName.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Collaborator
|
🤖 Operations Status Check This PR has been idle for 632h. Please provide a status update or push changes to keep it moving. If it's ready for review, please re-request. If blocked, please note what's needed. |
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
2 participants
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.
Summary
/claim #55
Implements Phase 2 and the code-side parts of Phase 4 for the AWS S3 Security Remediation & Civil ID Upload Fix bounty.
candidate-civil-id/prefix to the currentphotos/prefixHEAD-based object existence checks and safer delete return handlingcivil_idandcivil_expiry_dateonupdate-civil-id-expiry-date, returningoperation:errorinstead of raw failuresValidation
candidate-civil-id/is no longer used for Civil ID deletes.php -lor the PHP test suite in this environment because PHP, Composer, Docker, and Git are not installed.Deployment Notes
Before deploying, set these environment variables in Railway/CI/local secret stores:
AWS_TEMP_BUCKET_KEYAWS_TEMP_BUCKET_SECRETAWS_PERMANENT_S3_ACCESS_KEY_IDAWS_PERMANENT_S3_SECRET_ACCESS_KEYAWS_PERMANENT_S3_REGIONAWS_PERMANENT_S3_BUCKETSummary by CodeRabbit
Release Notes
Documentation
New Features
Bug Fixes
Tests