Skip to content

fix(db): widen encrypted columns to text to prevent overflow#9802

Closed
nehemiyawicks wants to merge 3 commits intocoollabsio:nextfrom
nehemiyawicks:fix/widen-encrypted-columns
Closed

fix(db): widen encrypted columns to text to prevent overflow#9802
nehemiyawicks wants to merge 3 commits intocoollabsio:nextfrom
nehemiyawicks:fix/widen-encrypted-columns

Conversation

@nehemiyawicks
Copy link
Copy Markdown

@nehemiyawicks nehemiyawicks commented Apr 25, 2026

Changes

Widened the http_basic_auth_password encrypted column on the applications table from varchar(255) to text. Laravel's Crypt::encryptString() expands values well beyond 255 characters (a 32-char password produces ~220+ chars of ciphertext), causing a SQL truncation error when saving.

This follows the same pattern used in existing migrations 2024_05_22_103942 and 2024_12_10_122142 which widened other encrypted/long-value columns to text.

Note: The manual_webhook_secret_* columns were removed from this migration as they are already widened to text in 2026_04_19_000000_backfill_and_encrypt_webhook_secrets.

Issues

Fixes #9643

Category

  • Bug fix
  • Improvement
  • New feature
  • Adding new one click service
  • Fixing or updating existing one click service

Preview

Screenshot 2026-04-25 at 11 20 58 PM Screenshot 2026-04-25 at 11 20 23 PM

AI Assistance

  • AI was NOT used to create this PR
  • AI was used (please describe below)

If AI was used:

  • Tools used: Claude Opus 4.7
  • How extensively: Research and code generation. Migration and test were manually verified and tested on a local dev instance (migrations run, 64-char password saves and decrypts correctly, pint passes).

Testing

  1. Ran migration on local dev instance — completes in 20ms
  2. Saved a 64-char HTTP Basic Auth password via the UI — saved successfully (previously failed with SQL truncation error at 32+ chars)
  3. Verified password decrypts correctly after save
  4. Ran vendor/bin/pint --dirty --format agent — pass
  5. Ran Pest test EncryptedColumnOverflowTest — 1 test, 2 assertions, pass

Contributor Agreement

Important

  • I have read and understood the contributor guidelines. If I have failed to follow any guideline, I understand that this PR may be closed without review.
  • I have searched existing issues and pull requests (including closed ones) to ensure this isn't a duplicate.
  • I have tested all the changes thoroughly with a local development instance of Coolify and I am confident that they will work as expected when a maintainer tests them.

Encrypted values (Laravel Crypt) expand significantly beyond the
original plaintext length. Columns using varchar(255) overflow when
storing encrypted values longer than ~31 characters. This migration
widens http_basic_auth_password and webhook secret columns on the
applications table from string to text.

Fixes coollabsio#9643
Copilot AI review requested due to automatic review settings April 25, 2026 17:53
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Widen encrypted columns on the applications table to text to prevent PostgreSQL varchar(255) truncation errors when storing Laravel-encrypted values.

Changes:

  • Added a migration to change http_basic_auth_password (and webhook secret columns) from string/varchar(255) to text.
  • Added a Pest feature test intended to guard against encrypted-value overflow regressions.

Reviewed changes

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

File Description
database/migrations/2026_04_25_000000_widen_encrypted_columns_on_applications_table.php Alters encrypted application columns to text (and provides a down migration).
tests/Feature/EncryptedColumnOverflowTest.php Adds regression coverage around column types / encrypted output length.

Comment thread tests/Feature/EncryptedColumnOverflowTest.php Outdated
Comment thread tests/Feature/EncryptedColumnOverflowTest.php
Comment thread tests/Feature/EncryptedColumnOverflowTest.php Outdated
Remove redundant webhook secret column changes — already widened to
text in 2026_04_19 backfill migration. Remove unused DB import and
webhook test.
@Iisyourdad
Copy link
Copy Markdown
Contributor

The migration itself looks like the right fix but the added test doesn't work.

tests/Feature/EncryptedColumnOverflowTest.php says it “stores” the password, but it only checks Schema::getColumnType() and the encrypted string length. Because the SQLite testing schema already represents http_basic_auth_password as TEXT, this can pass without proving that an encrypted value can be persisted through the Application model/cast path or that the PostgreSQL varchar(255) overflow is covered.

Could you:

  • make this a real persistence test that saves an Application, verifies the raw stored ciphertext length, and confirms the attribute decrypts back correctly?

Create Application via factory, save 64-char password through the
encrypted cast, verify raw ciphertext length > 255, confirm decryption
round-trips correctly.
@nehemiyawicks
Copy link
Copy Markdown
Author

@Iisyourdad Good call, rewrote the test in 98ba1e8. It now creates an Application via factory, sets a 64-char http_basic_auth_password, saves it, then asserts the raw DB ciphertext is >255 chars and the attribute decrypts back to the original value. Passes locally on the dev instance.

@ShadowArcanist
Copy link
Copy Markdown
Member

Closing this PR because it shows a lack of understanding of the codebase. The tests claimed to pass were not actually working and had to be rewritten just to succeed, which raises concerns about the reliability of the changes themselves.

Please refrain from submitting further PRs like this. This is not the first instance I’ve had to close one of your contributions. If you’re relying on AI-generated code without fully understanding it, it becomes difficult to assess whether the solution is correct, and it ends up costing everyone time. I appreciate the intention to contribute, but meaningful contributions require a solid grasp of the codebase and the changes being proposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants