fix: teach the doctor about orphaned two_factor rows (#6639)

If a row in the two_factor table references a non existent user, it may contain a secret that has an invalid format. Such an orphaned row is never used and should be removed.

Improve the error message to suggest using the doctor to remove it.

Fixes: https://codeberg.org/forgejo/forgejo/issues/6637

## Testing

- make TAGS='sqlite sqlite_unlock_notify' watch
- make TAGS='sqlite  sqlite_unlock_notify' forgejo
- sqlite3 data/gitea.db 'INSERT INTO two_factor VALUES( 0, 500, "", "", "", "", 0, 0)'
- ./forgejo doctor check --run check-db-consistency
  ```
   [1] Check consistency of database
    - [W] Found 1 Orphaned TwoFactor without existing User
   OK

   All done (checks: 1).
   ```
-  ./forgejo doctor check --run check-db-consistency --fix
   ```
   [1] Check consistency of database
    - [I] Deleted 1 Orphaned TwoFactor without existing User
   OK

   All done (checks: 1).
   ```

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests

- I added test coverage for Go changes...
  - [ ] in their respective `*_test.go` for unit tests.
  - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I added test coverage for JavaScript changes...
  - [ ] in `web_src/js/*.test.js` if it can be unit tested.
  - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [ ] I do not want this change to show in the release notes.
- [x] I want the title to show in the release notes with a link to this pull request.
- [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.

<!--start release-notes-assistant-->

## Release notes
<!--URL:https://codeberg.org/forgejo/forgejo-->
- Bug fixes
  - [PR](https://codeberg.org/forgejo/forgejo/pulls/6639): <!--number 6639 --><!--line 0 --><!--description VGVhY2ggdGhlIGRvY3RvciB0byByZW1vdmUgb3JwaGFuZWQgdHdvX2ZhY3RvciB3aXRoIGBmb3JnZWpvIGRvY3RvciBjaGVjayAtLXJ1biBjaGVjay1kYi1jb25zaXN0ZW5jeSAtLWZpeGAuIFN1Y2ggcm93cyBtYXkgY29udGFpbiBpbnZhbGlkIGRhdGEgYW5kIFtibG9jayB0aGUgbWlncmF0aW9uIHRvIHYxMF0oaHR0cHM6Ly9jb2RlYmVyZy5vcmcvZm9yZ2Vqby9mb3JnZWpvL2lzc3Vlcy82NjM3KSB3aXRoIGEgbWVzc2FnZSBzdWNoIGFzIGBmYWlsZWQ6IEFlc0RlY3J5cHQgaW52YWxpZCBkZWNyeXB0ZWQgYmFzZTY0IHN0cmluZzogaWxsZWdhbCBiYXNlNjQgZGF0YSBhdCBpbnB1dCBieXRlIDBgLg==-->Teach the doctor to remove orphaned two_factor with `forgejo doctor check --run check-db-consistency --fix`. Such rows may contain invalid data and [block the migration to v10](https://codeberg.org/forgejo/forgejo/issues/6637) with a message such as `failed: AesDecrypt invalid decrypted base64 string: illegal base64 data at input byte 0`.<!--description-->
<!--end release-notes-assistant-->

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/6639
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
(cherry picked from commit c92fe83c40)
This commit is contained in:
Earl Warren 2025-01-21 17:30:08 +00:00 committed by forgejo-backport-action
parent 5c5e1c87ba
commit e808fda9b9
3 changed files with 5 additions and 1 deletions

View file

@ -47,7 +47,7 @@ func AesDecrypt(key, text []byte) ([]byte, error) {
cfb.XORKeyStream(text, text)
data, err := base64.StdEncoding.DecodeString(string(text))
if err != nil {
return nil, fmt.Errorf("AesDecrypt invalid decrypted base64 string: %w", err)
return nil, fmt.Errorf("AesDecrypt invalid decrypted base64 string: %w - it can be caused by a change of the [security].SECRET_KEY setting or a database corruption - `forgejo doctor check --run check-db-consistency --fix` will get rid of orphaned rows found in the `two_factor` table and may fix this problem if they are the one with the invalid content", err)
}
return data, nil
}

1
release-notes/6639.md Normal file
View file

@ -0,0 +1 @@
Teach the doctor to remove orphaned two_factor with `forgejo doctor check --run check-db-consistency --fix`. Such rows may contain invalid data and [block the migration to v10](https://codeberg.org/forgejo/forgejo/issues/6637) with a message such as `failed: AesDecrypt invalid decrypted base64 string: illegal base64 data at input byte 0`.

View file

@ -246,6 +246,9 @@ func checkDBConsistency(ctx context.Context, logger log.Logger, autofix bool) er
// find authorization tokens without existing user
genericOrphanCheck("Authorization token without existing User",
"forgejo_auth_token", "user", "forgejo_auth_token.uid=`user`.id"),
// find two_factor without existing user
genericOrphanCheck("Orphaned TwoFactor without existing User",
"two_factor", "user", "`two_factor`.uid=`user`.id"),
)
for _, c := range consistencyChecks {