Skip to content

Fix Civil ID S3 hardening and env-based S3 credentials #71

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
mzgamers28-cmyk wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Fix Civil ID S3 hardening and env-based S3 credentials #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9b30fbd
Update README S3 env docs
mzgamers28-cmyk May 14, 2026
98f9655
Harden Civil ID account endpoints
mzgamers28-cmyk May 14, 2026
1a9cbdd
Fix Civil ID S3 file handling
mzgamers28-cmyk May 14, 2026
63ade98
Use S3 HEAD checks for file existence
mzgamers28-cmyk May 14, 2026
e4c2b2d
Move temp S3 credentials to env vars
mzgamers28-cmyk May 14, 2026
bbe55ab
Add Civil ID expiry validation coverage
mzgamers28-cmyk May 14, 2026
f8c4e17
Move prod Railway S3 credentials to env vars
mzgamers28-cmyk May 14, 2026
933ea47
Move dev Railway S3 credentials to env vars
mzgamers28-cmyk May 14, 2026
d0b01e8
Move docker S3 credentials to env vars
mzgamers28-cmyk May 14, 2026
338877d
Move CI S3 credentials to env vars
mzgamers28-cmyk May 14, 2026
27c1db2
Move krushn S3 credentials to env vars
mzgamers28-cmyk May 14, 2026
73c4bb2
bounty/civil-id-s3-hardening-55
mzgamers28-cmyk May 14, 2026
388ed85
Address CodeRabbit review suggestions
mzgamers28-cmyk May 14, 2026
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
12 changes: 11 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,17 @@ Detailed documentation is available in the `docs/` directory:
- Access backend container: `docker-compose exec backend bash`
- Code generator: http://localhost:8888/bawes/studenthub/admin/web/gii

## S3 Environment Variables

The application expects S3 credentials to be supplied through environment variables rather than committed config values.

- `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`

## allow access from docker to local mysql server

`GRANT ALL PRIVILEGES ON *.* TO 'root'@'192.168.1.5' IDENTIFIED BY 'root' WITH GRANT OPTION;`
Expand Down Expand Up @@ -204,4 +215,3 @@ END $$
DELIMITER;



96 changes: 81 additions & 15 deletions candidate/modules/v1/controllers/AccountController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,19 +382,33 @@ public function actionRemovePhoto() {
public function actionRemoveCivilPhotoBack() {

$model = Candidate::findOne(Yii::$app->user->getId());

if (!$model) {
throw new \yii\web\HttpException(404, Yii::t('candidate', 'The requested Item could not be found.'));
}

if ($model->candidate_civil_photo_back) {
$model->deleteFile('civil-id', 'back');
}

$model->candidate_civil_photo_back = null;
$model->candidate_civil_need_verification = true;

$model->scenario = 'updateCivilPhotoBack';

if (!$model->save(false)) {
try {
if (!$model->save(false)) {
return [
'operation' => 'error',
'message' => $model->getErrors()
];
}
} catch (\Throwable $e) {
Yii::error('Failed to remove back civil photo for candidate_id=' . $model->candidate_id . ': ' . $e->getMessage(), 'candidate');

return [
'operation' => 'error',
'message' => $model->getErrors()
'message' => Yii::t('candidate', 'Unable to remove Civil ID photo. Please try again.')
];
}

Expand All @@ -409,18 +423,32 @@ public function actionRemoveCivilPhotoBack() {
public function actionRemoveCivilPhotoFront() {
$model = Candidate::findOne(Yii::$app->user->getId());

if (!$model) {
throw new \yii\web\HttpException(404, Yii::t('candidate', 'The requested Item could not be found.'));
}

if ($model->candidate_civil_photo_front) {
$model->deleteFile('civil-id', 'front');
}

$model->candidate_civil_photo_front = null;
$model->candidate_civil_need_verification = true;

$model->scenario = 'updateCivilPhotoFront';

if (!$model->save(false)) {
try {
if (!$model->save(false)) {
return [
'operation' => 'error',
'message' => $model->getErrors()
];
}
} catch (\Throwable $e) {
Yii::error('Failed to remove front civil photo for candidate_id=' . $model->candidate_id . ': ' . $e->getMessage(), 'candidate');

return [
'operation' => 'error',
'message' => $model->getErrors()
'message' => Yii::t('candidate', 'Unable to remove Civil ID photo. Please try again.')
];
}

Expand Down Expand Up @@ -1301,7 +1329,12 @@ public function actionUpdateCivilPhotoBack() {
];
}

$model->updateCivilId('back');
if (!$model->updateCivilId('back')) {
return [
'operation' => 'error',
'message' => $model->getErrors()
];
}

//reset to remove old id's data
$model->candidate_civil_expiry_date = null;
Expand Down Expand Up @@ -1353,7 +1386,12 @@ public function actionUpdateCivilPhotoFront() {
];
}

$model->updateCivilId('front');
if (!$model->updateCivilId('front')) {
return [
'operation' => 'error',
'message' => $model->getErrors()
];
}

//reset to remove old id's data
$model->candidate_civil_expiry_date = null;
Expand Down Expand Up @@ -1430,24 +1468,52 @@ public function actionUpdateCivilIdExpiryDate() {
throw new \yii\web\HttpException(404, Yii::t('candidate', 'The requested Item could not be found.'));
}

$candidate_civil_id = Yii::$app->request->getBodyParam('civil_id');
$candidate_civil_id = trim((string) Yii::$app->request->getBodyParam('civil_id'));

$candidate_civil_expiry_date = Yii::$app->request->getBodyParam('civil_expiry_date');
$candidate_civil_expiry_date = trim((string) Yii::$app->request->getBodyParam('civil_expiry_date'));

$candidate->candidate_civil_id = $candidate_civil_id;
if ($candidate_civil_id === '') {
return [
"operation" => "error",
"message" => Yii::t('candidate', "Civil ID is required"),
];
}

if($candidate_civil_expiry_date)
$candidate->candidate_civil_expiry_date = date('Y-m-d', strtotime($candidate_civil_expiry_date));
if ($candidate_civil_expiry_date === '') {
return [
"operation" => "error",
"message" => Yii::t('candidate', "Civil ID Expiry Date is required"),
];
}

$candidate->candidate_civil_need_verification = true;
$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"),
];
}
Comment on lines +1489 to +1496

@coderabbitai coderabbitai Bot May 14, 2026

Copy link
Copy Markdown

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

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.


if (!$candidate->save()) {
try {
$candidate->candidate_civil_id = $candidate_civil_id;
$candidate->candidate_civil_expiry_date = date('Y-m-d', $candidate_civil_expiry_timestamp);
$candidate->candidate_civil_need_verification = true;

$candidate->scenario = "updateCivilExpiryDateAndCivilID";

if (!$candidate->save()) {
return [
"operation" => "error",
"message" => $candidate->errors
];
}
} catch (\Throwable $e) {
Yii::error('Failed to update Civil ID expiry for candidate_id=' . $candidate->candidate_id . ': ' . $e->getMessage(), 'candidate');

return [
"operation" => "error",
"message" => $candidate->errors
"message" => Yii::t('candidate', "Unable to update Civil ID details. Please try again.")
];
}

Expand Down
28 changes: 27 additions & 1 deletion candidate/tests/functional/AccountCest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,33 @@ public function tryUpdateCivilExpiry(FunctionalTester $I)
$I->seeResponseCodeIs(HttpCode::OK); // 200
}

public function tryUpdateCivilExpiryRejectsMissingCivilId(FunctionalTester $I)
{
$I->amGoingTo('try to update civil id expiry date without civil id');
$I->sendPOST('v1/account/update-civil-id-expiry-date', [
'civil_expiry_date' => '3033-12-12'
]);
$I->seeResponseCodeIs(HttpCode::OK); // 200
$I->seeResponseContainsJson([
'operation' => 'error',
'message' => 'Civil ID is required'
]);
}

public function tryUpdateCivilExpiryRejectsInvalidDate(FunctionalTester $I)
{
$I->amGoingTo('try to update civil id expiry date with invalid date');
$I->sendPOST('v1/account/update-civil-id-expiry-date', [
'civil_id' => '70',
'civil_expiry_date' => 'not-a-date'
]);
$I->seeResponseCodeIs(HttpCode::OK); // 200
$I->seeResponseContainsJson([
'operation' => 'error',
'message' => 'Civil ID Expiry Date is invalid'
]);
}

public function tryUpdateKuwaitiNational(FunctionalTester $I)
{
$I->amGoingTo('try to update if mother kuwaity');
Expand Down Expand Up @@ -547,4 +574,3 @@ public function tryUpdatePreferredTime(FunctionalTester $I)
// $I->seeResponseCodeIs(HttpCode::OK); // 200
// }
}

36 changes: 32 additions & 4 deletions common/components/S3ResourceManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,22 +162,50 @@ public function delete($name)
'Key' => $name
]);

return $result['DeleteMarker'];
// deleteObject success is determined by request success (no exception).
return true;
}

/**
* Checks whether a file exists or not. This method only works for public resources, private resources will throw
* a 403 error exception.
* Checks whether a file exists or not.
* S3 object keys are checked with HeadObject; URLs are checked with an HTTP HEAD request.
* @param string $filenameOrUrl the name or url of the file
* @return boolean
*/
public function fileExists($filenameOrUrl)
{
if (!$filenameOrUrl) {
return false;
}

$isUrl = false;
if (strpos($filenameOrUrl, 'http') !== 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;
}
}

$http = new \GuzzleHttp\Client(['base_uri' => $isUrl ? $filenameOrUrl : $this->getUrl($filenameOrUrl)]);
try {
$response = $http->request('HEAD');
Expand Down
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') ?: '',
Comment on lines +11 to +12

@coderabbitai coderabbitai Bot May 14, 2026

Copy link
Copy Markdown

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

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.

'bucket' => 'studenthub-public-anyone-can-upload-24hr-expiry'
/**
* You can access the Temporary bucket with:
Expand Down
Loading