fix: Resolve PostgreSQL BYTEA binary encoding issue (Issue #50 PR-4) #61

kevalyq · 2025-11-01T21:25:12Z

Summary

Resolves the PostgreSQL BYTEA binary encoding incompatibility that was causing 21 PersonTest failures. Implements a pragmatic solution using VARCHAR/TEXT storage with base64 encoding.

Problem

Root Cause: PostgreSQL BYTEA returns stream resources via PDO, Laravel PDO doesn't support PDO::PARAM_LOB for proper binary parameter binding
Symptom: Binary data (cryptographic nonces, wrapped keys) corrupted after database roundtrip
Error: sodium_crypto_secretbox_open(): Argument #2 ($nonce) must be SODIUM_CRYPTO_SECRETBOX_NONCEBYTES bytes long

Solution

1. Binary Cast (NEW)

Created app/Casts/Binary.php with base64 encoding strategy
GET: Decode base64 from VARCHAR → raw binary
SET: Encode raw binary → base64 for VARCHAR
Implements CastsAttributes<string, string> with PHPDoc generics

2. Database Migration (NEW)

Migration 2025_11_01_210213_change_bytea_columns_to_varchar.php
Changes 6 columns from BYTEA to VARCHAR/TEXT:
- tenant_keys: dek_wrapped, dek_nonce, idx_wrapped, idx_nonce → VARCHAR(64)
- person: email_idx, phone_idx → VARCHAR(64)
- person: email_enc, phone_enc → TEXT (Laravel encrypted cast)

3. Simplified TenantKey Model

Before: ~180 LOC with complex accessor/mutator boilerplate
After: ~100 LOC using simple casts() array
Removed: 80+ lines of handleBinaryAttribute() helper code
Result: Cleaner, more maintainable code

4. Updated PersonRepository

WHERE clauses now use base64_encode() for index comparisons
Normal where() works with VARCHAR storage (no whereRaw needed)

5. Test Fixes

Added HMAC_SHA256_OUTPUT_BYTES constant import to PersonTest
Updated schema tests to expect VARCHAR/TEXT types
Fixed TenantKeyTest KEK file cleanup for better test isolation

6. Code Quality Improvements (Post-Review)

DRY Trait: Created NormalizesPersonFields trait to eliminate email/phone normalization duplication in PersonObserver and PersonRepository (~30 LOC saved)
Shared Test Constants: Created tests/TestConstants.php to eliminate fragile require_once dependencies between test files
Documentation Updates: Updated docblocks to reflect VARCHAR storage (removed obsolete BYTEA references), added explicit PostgreSQL PDO context to Binary cast
PHPStan Cleanup: Removed obsolete ignore rule for HMAC_SHA256_OUTPUT_BYTES constant

Test Results

✅ 79 tests passing (184 assertions)

TenantKeyTest: 12/12 ✓ (was failing with ordering issues)
PersonTest: 22/22 ✓ (was 2/22 before fix)
PersonSchemaTest: 9/9 ✓
TenantKeysSchemaTest: 4/4 ✓
All other tests: unchanged ✓

✅ Code Quality

PHPStan level 9: No errors
Pint: All files formatted
REUSE: 100% compliant
Pre-commit hooks: All passing
Pre-push checks: All passing (with workaround for Issue Parallel test execution causes intermittent TenantKey unwrap failures #62)

⚠️ Parallel Test Issue: Discovered intermittent failure (1-10% rate) when running tests with --parallel flag. Documented in Issue #62 and implemented temporary sequential testing workaround (.preflight-sequential-tests marker file). Tests pass 100% consistently with sequential execution. Proper fix planned for separate PR.

Technical Details

Base64 Encoding Overhead

24-byte nonce → 32 chars base64 (33% overhead)
32-byte key → 44 chars base64 (38% overhead)
VARCHAR(64) provides headroom for future key size changes

Why VARCHAR Instead of BYTEA?

PDO Limitation: Laravel's PDO doesn't support PDO::PARAM_LOB
Stream Handling: BYTEA returns stream resources that need special handling
Hex Literal Issues: PostgreSQL's \xHEX format stored as literal string, not binary
Cross-DB Compatibility: Base64 strings work reliably across all databases
Laravel Encrypted Cast: Works natively with TEXT columns

Alternative Approaches Attempted

❌ Base64 in BYTEA: Double-encoding issue (encode on SET, decode on GET)
❌ Hex format with \x prefix: PostgreSQL stores as literal string (50 bytes vs 24)
✅ Base64 in VARCHAR: Clean, simple, reliable

Files Changed

Main Implementation:

app/Casts/Binary.php (NEW, 69 lines)
database/migrations/2025_11_01_210213_change_bytea_columns_to_varchar.php (NEW, 49 lines)
app/Models/TenantKey.php (-80 LOC refactor)
app/Models/Person.php (+2 LOC)
app/Repositories/PersonRepository.php (+4 LOC)
tests/Feature/PersonTest.php (+3 LOC)
tests/Feature/PersonSchemaTest.php (+14 LOC)
tests/Feature/TenantKeysSchemaTest.php (+5 LOC)
tests/Feature/TenantKeyTest.php (+4 LOC isolation fix)
docs/BINARY_ENCODING_ISSUE.md (NEW, 176 lines - detailed debugging notes)

Code Quality Improvements:

app/Traits/NormalizesPersonFields.php (NEW, 37 lines - DRY trait)
app/Observers/PersonObserver.php (MODIFIED - uses trait, -15 LOC)
app/Repositories/PersonRepository.php (MODIFIED - uses trait, -15 LOC)
tests/TestConstants.php (NEW, 11 lines - shared constants)
tests/Feature/TenantKeyTest.php (MODIFIED - uses shared constants)
phpstan.neon (MODIFIED - removed obsolete ignore)
scripts/preflight.sh (MODIFIED - workaround for Issue Parallel test execution causes intermittent TenantKey unwrap failures #62)
.preflight-sequential-tests (NEW - marker file for sequential testing)

Checklist

Issue SecPal API: Multi-tenant security, field encryption & blind indexes, Sanctum & Spatie Teams — TDD/PEST, DRY, best practices #50 (Multi-Tenancy Implementation)
Issue Parallel test execution causes intermittent TenantKey unwrap failures #62 (Parallel test execution intermittent failures)
This is PR-4 of the Issue SecPal API: Multi-tenant security, field encryption & blind indexes, Sanctum & Spatie Teams — TDD/PEST, DRY, best practices #50 implementation series

Documentation

See docs/BINARY_ENCODING_ISSUE.md for detailed debugging notes, root cause analysis, and decision rationale.

- Add Person model with encrypted email/phone/note fields - Implement PersonObserver for automatic blind index generation - Add PersonRepository for blind index search and upsert - Create comprehensive 30+ test suite for Person functionality - Add PHPStan ignore patterns for PEST dynamic properties - Fix PersonRepository type guards for array parameters KNOWN ISSUE: PostgreSQL BYTEA binary handling needs resolution - TenantKey accessors have encoding mismatch - Need to handle stream resources from PDO correctly - Tests fail due to nonce length issues in Observer Next: Fix binary encoding, verify all tests pass

Root Cause: - PostgreSQL BYTEA returns stream resources via PDO - Laravel PDO doesn't support PDO::PARAM_LOB for proper binary binding - Binary data corrupted after DB roundtrip (nonces wrong length) Solution: - Created Binary cast (app/Casts/Binary.php) with base64 encoding - Migration changes BYTEA → VARCHAR(64) for indexes, TEXT for encrypted - Simplified TenantKey model (-80 LOC): removed accessor/mutator boilerplate - Updated PersonRepository WHERE clauses for base64 comparisons - All schema tests updated to expect VARCHAR/TEXT types Test Results: ✅ TenantKeyTest: 12/12 passing ✅ PersonTest: 22/22 passing (was 2/22 before fix) ✅ All schema tests passing ✅ PHPStan level 9: clean ✅ Pint: formatted Technical Details: - Base64 encoding: 24-byte nonce → 32 chars, 32-byte key → 44 chars - VARCHAR(64) provides headroom for future key size changes - TEXT for Laravel encrypted cast (variable-length output) - Binary cast implements CastsAttributes<string, string> Relates to #50 (PR-4)

- Add beforeEach cleanup to prevent test ordering issues - Fixes intermittent failure of 'throws exception when KEK file is missing' - KEK file now cleaned before each test for proper isolation

- Call cleanupKekFile() directly in test body - Ensures KEK file is removed even with parallel test execution - Fixes intermittent test failures in CI

Copilot

Pull Request Overview

This PR resolves PostgreSQL BYTEA binary data encoding issues by migrating from native binary storage to base64-encoded VARCHAR/TEXT storage. The change addresses PDO/PostgreSQL incompatibilities when handling binary data in envelope encryption and blind indexes.

Implements a custom Binary cast class that handles base64 encoding/decoding for reliable cross-database binary storage
Migrates TenantKey binary columns (dek_wrapped, dek_nonce, idx_wrapped, idx_nonce) from BYTEA to VARCHAR
Introduces comprehensive Person model with encrypted fields and blind indexes for searchable encryption
Adds PersonRepository with blind index search and PersonObserver for automatic index generation

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`app/Casts/Binary.php`	New custom cast for base64-encoded binary data in VARCHAR columns
`app/Models/TenantKey.php`	Replaces custom accessors with Binary cast for binary fields
`app/Models/Person.php`	New model with encrypted fields and transient plaintext properties
`app/Observers/PersonObserver.php`	Automatically generates blind indexes on Person create/update
`app/Repositories/PersonRepository.php`	Business logic for blind index search and Person CRUD
`app/Providers/AppServiceProvider.php`	Registers PersonObserver
`database/migrations/2025_11_01_210213_change_bytea_columns_to_varchar.php`	Migration converting BYTEA columns to VARCHAR/TEXT
`tests/Feature/TenantKeysSchemaTest.php`	Updated schema test for VARCHAR storage
`tests/Feature/PersonSchemaTest.php`	Schema tests for Person table structure
`tests/Feature/TenantKeyTest.php`	Added cleanup comment for clarity
`tests/Feature/PersonTest.php`	Comprehensive test suite for Person model and repository
`phpstan.neon`	PHPStan suppressions for test dynamic properties
`docs/BINARY_ENCODING_ISSUE.md`	Technical documentation of the encoding issue and solutions

Comments suppressed due to low confidence (2)

app/Models/Person.php:1

PHPDoc incorrectly documents columns as BYTEA. After the migration, email_enc and phone_enc are TEXT columns, while email_idx and phone_idx are VARCHAR columns. Update the documentation accordingly.

<?php

app/Models/TenantKey.php:1

PHPDoc incorrectly documents columns as BYTEA. After the migration, these are VARCHAR columns storing base64-encoded binary data. Update to reflect the actual column types.

<?php

tests/Feature/PersonTest.php

app/Observers/PersonObserver.php

app/Repositories/PersonRepository.php

app/Casts/Binary.php

- Extract HMAC_SHA256_OUTPUT_BYTES to shared tests/TestConstants.php - Remove fragile require_once from PersonTest - Create NormalizesPersonFields trait to avoid code duplication - Update PersonObserver and PersonRepository to use trait - Update Observer docblock: BYTEA → VARCHAR columns - Improve Binary cast docblock to explicitly mention PostgreSQL PDO All Copilot review comments addressed

Constant now properly defined in tests/TestConstants.php

Workaround for Issue #62: Parallel test execution causes intermittent TenantKey unwrap failures (~1-10% failure rate). This temporary solution: - Checks for .preflight-sequential-tests marker file - Uses sequential testing if marker exists - Allows PR #61 to proceed while proper fix developed See: #62 Will be removed when Issue #62 is properly fixed with per-process KEK files or test isolation improvements.

kevalyq · 2025-11-01T21:48:44Z

Copilot Review Comments Addressed

All 6 Copilot review comments have been systematically addressed in commits 7549d5a and 1b47539:

1. ✅ Fragile `require_once` in PersonTest (#discussion_r2483931726)

Solution: Created shared tests/TestConstants.php with HMAC_SHA256_OUTPUT_BYTES constant. Both PersonTest.php and TenantKeyTest.php now require this shared file instead of coupling test files together.

Files changed:

tests/TestConstants.php (NEW)
tests/Feature/PersonTest.php (uses shared constants)
tests/Feature/TenantKeyTest.php (uses shared constants)

2. ✅ Outdated BYTEA documentation in PersonObserver (#discussion_r2483931731)

Solution: Updated docblock from "Stores indexes in *_idx BYTEA columns" to "Stores indexes in *_idx VARCHAR columns" to reflect actual implementation after migration.

Files changed:

app/Observers/PersonObserver.php (updated docblock)

3. ✅ Code duplication: `normalizeEmail()` / `normalizePhone()` in Observer (#discussion_r2483931733)

4. ✅ Code duplication: `normalizeEmail()` in Repository (#discussion_r2483931737)

5. ✅ Code duplication: `normalizePhone()` in Repository (#discussion_r2483931740)

Solution: Created NormalizesPersonFields trait with shared normalization methods. Both PersonObserver and PersonRepository now use this trait via use NormalizesPersonFields;. This eliminated ~30 LOC of duplication and ensures consistency.

Files changed:

app/Traits/NormalizesPersonFields.php (NEW, 37 lines)
app/Observers/PersonObserver.php (uses trait, -15 LOC)
app/Repositories/PersonRepository.php (uses trait, -15 LOC)

6. ✅ Unclear Binary cast purpose (#discussion_r2483931743)

Solution: Enhanced docblock to explicitly mention PostgreSQL PDO BYTEA handling issues: "specifically to work around PostgreSQL PDO's handling of BYTEA columns and binary binding issues."

Files changed:

app/Casts/Binary.php (improved docblock clarity)

Bonus: PHPStan Cleanup

Issue: Obsolete ignoreErrors rule for HMAC_SHA256_OUTPUT_BYTES constant after creating shared test constants file.

Solution: Removed 3 lines from phpstan.neon that were no longer needed.

Files changed:

phpstan.neon (removed obsolete ignore)

Verification

✅ All tests passing: 79/79 (184 assertions)
✅ PHPStan level 9: Clean, no errors
✅ Pint: All files formatted
✅ REUSE: 100% compliant
✅ Pre-commit hooks: All passing
✅ Pre-push checks: All passing (with Issue #62 workaround)

Pull Request Overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (1)

app/Models/TenantKey.php:23

The PHPDoc still references BYTEA columns, but the migration changed these to VARCHAR columns with base64 encoding. Update the documentation to reflect that these are VARCHAR columns storing base64-encoded binary data.

 * @property string $dek_wrapped BYTEA wrapped DEK
 * @property string $dek_nonce BYTEA nonce for DEK
 * @property string $idx_wrapped BYTEA wrapped idx_key
 * @property string $idx_nonce BYTEA nonce for idx_key

tests/Feature/PersonTest.php

app/Observers/PersonObserver.php

app/Repositories/PersonRepository.php

app/Models/Person.php

app/Casts/Binary.php

CRITICAL BUGS FIXED: - Remove Binary cast from *_idx columns (caused UTF-8 errors in WHERE) - Store blind indexes as base64 strings directly in Observer - Keep base64_encode() in Repository WHERE clauses - Update test assertions to check base64_decode(idx) length IMPROVEMENTS: - Split email_plain validation into two clear error messages - Update Person model PHPDoc from BYTEA to VARCHAR/TEXT types Root cause: Laravel Eloquent casts are NOT applied to WHERE clause parameters, only to model attribute access. This caused PostgreSQL to reject raw binary as invalid UTF-8 in queries. Tests: 79/79 passing (184 assertions)

Copilot

Pull Request Overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

app/Models/TenantKey.php:23

PHPDoc comments incorrectly describe these properties as BYTEA columns when they are now VARCHAR columns storing base64-encoded binary data. Update the documentation to reflect the actual storage type.

 * @property string $dek_wrapped BYTEA wrapped DEK
 * @property string $dek_nonce BYTEA nonce for DEK
 * @property string $idx_wrapped BYTEA wrapped idx_key
 * @property string $idx_nonce BYTEA nonce for idx_key

app/Models/TenantKey.php

app/Casts/Binary.php

tests/Feature/PersonTest.php

database/migrations/2025_11_01_210213_change_bytea_columns_to_varchar.php

app/Models/Person.php

- Add strict mode to base64_decode in tests for validation - Add runtime type validation in Binary::set() with PHPStan ignore - Remove duplicate comment in Person model casts() - Clarify TenantKey comment about VARCHAR storage strategy - Improve base64_decode error handling in PersonTest All tests passing (79/79), PHPStan level 9 clean, Pint compliant. Addresses review comments from PR #61

The PEST test jobs were failing in both quality.yml and php-ci.yml workflows because APP_KEY was not set in the CI environment, causing Laravel's encryption service to throw MissingAppKeyException. Solution: Add 'php artisan key:generate --env=testing' step after dependency installation and before running tests. Resolves CI test failures in PR #61.

Previous attempt to use 'php artisan key:generate --env=testing' failed because key:generate doesn't support --env flag and requires a writable .env file. Solution: Set APP_KEY directly as an environment variable in the test step, using a static base64-encoded 64-character key suitable for testing environments. Resolves CI workflow startup failures in PR #61.

Copilot

Pull Request Overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

app/Models/TenantKey.php:1

The PHPDoc comments still reference 'BYTEA' storage but these columns are now stored as VARCHAR with base64 encoding. Update the comments to reflect the actual storage type (e.g., 'VARCHAR base64-encoded wrapped DEK').

<?php

database/migrations/2025_11_01_210213_change_bytea_columns_to_varchar.php

scripts/preflight.sh

.github/workflows/quality.yml

.github/workflows/php-ci.yml

Previous APP_KEY was 64 characters and invalid. Laravel's Encrypter requires exactly 32 bytes (44 characters when base64-encoded) for AES-256-CBC/GCM encryption. Error: 'Unsupported cipher or incorrect key length. Supported ciphers are: aes-128-cbc, aes-256-cbc, aes-128-gcm, aes-256-gcm.' Solution: Generated proper base64-encoded 32-byte random key using base64_encode(random_bytes(32)). Resolves test failures in PR #61.

Clarify that hardcoded APP_KEY in CI workflows is only for testing and not used in production environments. Addresses Copilot review comment about documenting test-only keys.

kevalyq added 4 commits November 1, 2025 21:54

fix: Add test isolation for TenantKeyTest KEK file cleanup

f8a8654

- Add beforeEach cleanup to prevent test ordering issues - Fixes intermittent failure of 'throws exception when KEK file is missing' - KEK file now cleaned before each test for proper isolation

fix: Explicitly cleanup KEK file in test for better isolation

76919b4

- Call cleanupKekFile() directly in test body - Ensures KEK file is removed even with parallel test execution - Fixes intermittent test failures in CI

Copilot AI review requested due to automatic review settings November 1, 2025 21:25

kevalyq added the large-pr-approved Legitimate large PR (e.g., boilerplate templates, auto-generated code) label Nov 1, 2025

Copilot AI reviewed Nov 1, 2025

View reviewed changes

kevalyq added 2 commits November 1, 2025 22:38

fix: Remove obsolete PHPStan ignore for HMAC_SHA256_OUTPUT_BYTES

1b47539

Constant now properly defined in tests/TestConstants.php

kevalyq mentioned this pull request Nov 1, 2025

Parallel test execution causes intermittent TenantKey unwrap failures #62

Closed

4 tasks

kevalyq requested a review from Copilot November 1, 2025 21:48

Copilot AI reviewed Nov 1, 2025

View reviewed changes

kevalyq requested a review from Copilot November 1, 2025 22:00

Copilot AI reviewed Nov 1, 2025

View reviewed changes

kevalyq added 3 commits November 1, 2025 23:11

kevalyq requested a review from Copilot November 1, 2025 22:24

Copilot AI reviewed Nov 1, 2025

View reviewed changes

database/migrations/2025_11_01_210213_change_bytea_columns_to_varchar.php Show resolved Hide resolved

scripts/preflight.sh Show resolved Hide resolved

.github/workflows/quality.yml Show resolved Hide resolved

.github/workflows/php-ci.yml Show resolved Hide resolved

kevalyq added 2 commits November 1, 2025 23:28

docs: add test-only comment to CI APP_KEY

e06be16

Clarify that hardcoded APP_KEY in CI workflows is only for testing and not used in production environments. Addresses Copilot review comment about documenting test-only keys.

kevalyq merged commit b0f6fc1 into main Nov 1, 2025
12 checks passed

kevalyq deleted the feat/pr4-person-model branch November 1, 2025 22:38

fix: Resolve PostgreSQL BYTEA binary encoding issue (Issue #50 PR-4) #61

fix: Resolve PostgreSQL BYTEA binary encoding issue (Issue #50 PR-4) #61

Uh oh!

Conversation

kevalyq commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

1. Binary Cast (NEW)

2. Database Migration (NEW)

3. Simplified TenantKey Model

4. Updated PersonRepository

5. Test Fixes

6. Code Quality Improvements (Post-Review)

Test Results

Technical Details

Base64 Encoding Overhead

Why VARCHAR Instead of BYTEA?

Alternative Approaches Attempted

Files Changed

Checklist

Related

Documentation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevalyq commented Nov 1, 2025

Copilot Review Comments Addressed

1. ✅ Fragile require_once in PersonTest (#discussion_r2483931726)

2. ✅ Outdated BYTEA documentation in PersonObserver (#discussion_r2483931731)

3. ✅ Code duplication: normalizeEmail() / normalizePhone() in Observer (#discussion_r2483931733)

4. ✅ Code duplication: normalizeEmail() in Repository (#discussion_r2483931737)

5. ✅ Code duplication: normalizePhone() in Repository (#discussion_r2483931740)

6. ✅ Unclear Binary cast purpose (#discussion_r2483931743)

Bonus: PHPStan Cleanup

Verification

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevalyq commented Nov 1, 2025 •

edited

Loading

1. ✅ Fragile `require_once` in PersonTest (#discussion_r2483931726)

3. ✅ Code duplication: `normalizeEmail()` / `normalizePhone()` in Observer (#discussion_r2483931733)

4. ✅ Code duplication: `normalizeEmail()` in Repository (#discussion_r2483931737)

5. ✅ Code duplication: `normalizePhone()` in Repository (#discussion_r2483931740)