Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
101 changes: 101 additions & 0 deletions DEVELOPMENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,107 @@ git push
- Use descriptive commit messages (conventional commits)
- Easy to reset: close PR, delete branch, start clean from main

### Pre-PR Quality Checklist

**Before opening a Pull Request, ensure you've completed these checks:**

#### 1. Parallel Test Execution βœ…

```bash
# Catches race conditions and test isolation issues
ddev . ./vendor/bin/pest --parallel
```

**Why?** Tests may pass sequentially but fail in parallel (Issue #50: PR #63)

#### 2. Code Duplication Check πŸ”

```bash
# Search for duplicated logic that should be extracted
grep -r "function normalize" app/
grep -r "base64_encode.*generateBlindIndex" app/
grep -r "strtolower(trim(" app/

# Or use PHPStan:
ddev . ./vendor/bin/phpstan analyse --no-progress
```

**Action:** Extract duplicated code into traits, helpers, or methods.

#### 3. Magic Numbers & Constants πŸ”’

```bash
# Search for hardcoded values
grep -r "0700\|sha256\|32\|24\|16" app/ --exclude-dir=vendor
```

**Action:** Replace with named constants:

```php
// ❌ Bad
mkdir($dir, 0700, true);
hash_hmac('sha256', $data, $key);

// βœ… Good
mkdir($dir, self::KEY_DIRECTORY_PERMISSIONS, true);
hash_hmac(self::HMAC_ALGORITHM, $data, $key);
```

#### 4. Quality Gates (Automated) βœ…

```bash
# This runs automatically in pre-push hook:
ddev . ./vendor/bin/pint # PSR-12 style
ddev . ./vendor/bin/phpstan analyse # Level 9 static analysis
ddev . ./vendor/bin/pest # All tests
```

**Action:** Fix all errors before push.

#### 5. Commit Message Format πŸ“

Use [Conventional Commits](https://www.conventionalcommits.org/):

```bash
feat: add key rotation commands
fix: resolve test isolation issue
refactor: extract NormalizesPersonFields trait
docs: update README security section
test: add parallel execution tests
```

#### 6. PR Size Discipline πŸ“Š

- **Target:** <600 LOC per PR
- **Acceptable exceptions:**
- Critical bugfix + refactor (must be atomic)
- Architectural changes (command suite)
- Vendor migrations (can't be split)

**Action:** If >600 LOC, document justification in PR body.

#### 7. Review ALL Copilot Suggestions πŸ€–

- Don't blindly accept/reject suggestions
- Understand WHY Copilot suggests changes
- Group related fixes into ONE commit (not iterative pushes)

**Action:** Review β†’ Bulk-fix β†’ Test β†’ Push (not Push β†’ Review β†’ Push β†’ Review)

---

### Quick Pre-PR Command

```bash
# Run this before opening PR:
ddev . ./vendor/bin/pest --parallel && \
ddev . ./vendor/bin/pint && \
ddev . ./vendor/bin/phpstan analyse && \
echo "βœ… Ready for PR!"
```

---

### Resolving Copilot Review Comments

**After addressing Copilot review comments, resolve them via the GitHub UI:**
Expand Down
13 changes: 7 additions & 6 deletions app/Console/Commands/RebuildIndexCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

use App\Models\Person;
use App\Models\TenantKey;
use App\Traits\NormalizesPersonFields;
use Illuminate\Console\Command;

/**
Expand All @@ -28,6 +29,8 @@
*/
class RebuildIndexCommand extends Command
{
use NormalizesPersonFields;

/**
* The name and signature of the console command.
*
Expand Down Expand Up @@ -72,7 +75,7 @@ public function handle(): int
if ($person->getAttributes()['email_enc']) {
$emailPlain = $person->email_enc; // Uses cast to decrypt
if ($emailPlain !== null && $emailPlain !== '') {
$normalized = strtolower(trim($emailPlain));
$normalized = $this->normalizeEmail($emailPlain);
$rawIdx = $tenant->generateBlindIndex($normalized);
$person->email_idx = base64_encode($rawIdx); // Store as base64
}
Expand All @@ -82,11 +85,9 @@ public function handle(): int
if ($person->getAttributes()['phone_enc']) {
$phonePlain = $person->phone_enc; // Uses cast to decrypt
if ($phonePlain !== null && $phonePlain !== '') {
$normalized = preg_replace('/\D/', '', $phonePlain);
if ($normalized !== null) {
$rawIdx = $tenant->generateBlindIndex($normalized);
$person->phone_idx = base64_encode($rawIdx); // Store as base64
}
$normalized = $this->normalizePhone($phonePlain);
$rawIdx = $tenant->generateBlindIndex($normalized);
$person->phone_idx = base64_encode($rawIdx); // Store as base64
}
}

Expand Down
14 changes: 12 additions & 2 deletions app/Models/TenantKey.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,16 @@
*/
class TenantKey extends Model
{
/**
* File system permissions for keys directory (owner read/write/execute only)
*/
private const KEY_DIRECTORY_PERMISSIONS = 0700;

/**
* HMAC algorithm used for blind index generation
*/
private const HMAC_ALGORITHM = 'sha256';

/**
* The table associated with the model.
*
Expand Down Expand Up @@ -141,7 +151,7 @@ public static function generateKek(): void
$path = self::getKekPath();
$dir = dirname($path);

if (! is_dir($dir) && ! mkdir($dir, 0700, true)) {
if (! is_dir($dir) && ! mkdir($dir, self::KEY_DIRECTORY_PERMISSIONS, true)) {
throw new \RuntimeException('Failed to create keys directory');
}

Expand Down Expand Up @@ -283,7 +293,7 @@ public function decrypt(string $ciphertext, string $nonce): string
public function generateBlindIndex(string $plaintext): string
{
$idxKey = $this->unwrapIdxKey();
$index = hash_hmac('sha256', $plaintext, $idxKey, true);
$index = hash_hmac(self::HMAC_ALGORITHM, $plaintext, $idxKey, true);

sodium_memzero($idxKey);

Expand Down