chore(api): harden EF model constraints for Tag, User, Goal, Habit

[thomasluizon]

Behavior-preserving EF model hardening against the frozen /prod-readiness audit (code-quality Batch 5). Owns OrbitDbContext.cs + one new migration.

Changes

• Tag (UserId, Name) unique index → partial (WHERE "IsDeleted" = FALSE) so a tag name can be reused after soft-delete; Name/Color get IsRequired().HasMaxLength(50).
• User ColorScheme(50), Language(10), ThemePreference(10), TimeZone(100) and Goal Unit(50) — length bounds on previously unbounded text columns.
• Goal.Title(200) and HabitLog.Note(1000) — advisory max lengths that keep the encryption converter + text storage (ciphertext is never narrowed; these produce no DDL because the design-time model has no encryption).
• Tag and Habit gain an explicit HasOne<User>().WithMany().HasForeignKey(UserId).OnDelete(Cascade) — closing a referential-integrity gap.

Deviations from the task premise (evidence-backed)

• No shadow FK existed for Habit→User / Tag→User — the model snapshot had no relationship block, so UserId was a plain scalar. Adding the FK is therefore real ADD CONSTRAINT DDL, not a no-op.
• HabitLog→Habit FK already exists via Habit.Logs (Cascade, Required). Adding the requested second explicit FK would have created a duplicate/shadow FK, so it was not added — the audit finding is already satisfied.

Deploy safety (live startup migration — ref #345/#338 42P07 incident)

Verified against production (Supabase) before constraining:

• Max lengths of every narrowed column are well within the new limits (e.g. Language max 5, TimeZone max 19, Tag.Name max 11) — no ALTER COLUMN narrowing can fail.
• 4 orphaned Habits reference two hard-deleted users (0 logs each) — a validating FK add would have crashed the deploy. The migration deletes rows whose UserId references no User before validating the FKs (root-cause cleanup of data an old account-deletion path left behind; orphan_tags was 0 but the same guard is applied symmetrically).
• Index rebuild uses DROP INDEX IF EXISTS / CREATE UNIQUE INDEX IF NOT EXISTS. No prior migration created these FKs or a filtered IX_Tags_UserId_Name.
• The whole migration runs in a single transaction — any failure rolls back cleanly, no partial state. Down() cleanly reverses.

Tests

Extended OrbitDbContextTests with model assertions for the partial-index filter, the seven new max-lengths (incl. encrypted columns staying text), and the two cascading User FKs. Full suite green (4943 tests, 0 failures); dotnet build + dotnet ef migrations script verified.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Code Review: PR #358 (orbit-api)

Scope: PR #358 — "chore(api): harden EF model constraints for Tag, User, Goal, Habit" (first review pass — no prior reviews/comments/threads existed on this PR)
Recommendation: REQUEST CHANGES

Summary

This PR narrows several unbounded text columns to varchar(N), adds a partial unique index on Tags(UserId, Name) filtered by IsDeleted = FALSE, and adds two previously-missing Cascade FKs (Habit→User, Tag→User), cleaning up 4 orphaned Habits rows first so the new FKs validate. The migration, OrbitDbContext.cs changes, and new unit tests are well-constructed and well-tested. However, the migration exposes a real, previously-latent gap in request validation on the unauthenticated signup path: Users.Language is narrowed to varchar(10), but two call sites that write it (VerifyCodeCommand, GoogleAuthCommand) have zero length/whitelist validation, so an over-length client-supplied Language value will now throw an unhandled DB exception during account creation instead of failing cleanly (or silently succeeding, as before this PR).

Findings

High

1. Unvalidated Language field on unauthenticated signup endpoints will now throw an unhandled DbUpdateException instead of a validation error

• src/Orbit.Application/Auth/Commands/VerifyCodeCommand.cs:109 and src/Orbit.Application/Auth/Commands/GoogleAuthCommand.cs:115 call user.SetLanguage(language), where language comes directly from the client request (AuthController.cs → VerifyCodeRequest.Language / GoogleAuthRequest.Language, both plain records with no attributes, default "en").
• User.SetLanguage (src/Orbit.Domain/Entities/User.cs:156) is public void SetLanguage(string? language) => Language = language; — no length/format guard.
• VerifyCodeCommandValidator and GoogleAuthCommandValidator have zero rules for Language. The MediatR ValidationBehavior only runs validators registered for that exact command type, so there's no fallback sanitization anywhere in the pipeline.
• The codebase already has the correct pattern elsewhere — SetLanguageCommandValidator uses .MaximumLength(AppConstants.MaxLanguageLength) plus .Must(lang => AppConstants.SupportedLanguages.Contains(lang)) — but it was never applied to the two auth/signup commands.
• Before this PR, Users.Language was unbounded text, so this gap was inert. This migration (HardenModelConstraints) narrows it to character varying(10). A Language value longer than 10 characters sent to POST /api/auth/verify or POST /api/auth/google (both [AllowAnonymous]) will now cause SaveChangesAsync to throw during FindOrCreateUserAsync, breaking new-account creation instead of returning a clean 400.
• Fix: add the same .Must(lang => AppConstants.SupportedLanguages.Contains(lang)) rule (or at minimum .MaximumLength(AppConstants.MaxLanguageLength)) to both VerifyCodeCommandValidator and GoogleAuthCommandValidator.
• Note: inline comments were attempted on the two call sites above, but GitHub rejected them because those lines aren't part of this PR's diff (the gap is pre-existing code newly made reachable by the migration) — flagging here in the body instead.

Medium

2. Orphan cleanup verified only HabitLogs/Tags, not other Cascade-linked join tables on Habit

• HardenModelConstraints migration deletes orphaned Habits/Tags rows whose UserId matches no User, so the new FK constraints validate. The PR description states verification covered only HabitLogs count (0) and orphan_tags (0).
• ChallengeParticipantHabit.HabitId and AccountabilityPairHabit.HabitId are also configured with OnDelete(DeleteBehavior.Cascade) against Habit. If any of the 4 orphaned habits are linked to a challenge or accountability pairing, the raw DELETE FROM "Habits" will cascade-remove those join rows in production with no verification — a data-loss path outside what was checked, and Down() does not restore deleted rows.
• Fix: before merging/running against production, verify (and document) a 0 count against ChallengeParticipantHabit and AccountabilityPairHabit for the 4 orphaned habit ids, consistent with the diligence already applied to HabitLogs.

What's good

• The migration correctly orders operations (drop old index → narrow columns → recreate filtered index → delete orphans → add FKs) so nothing validates against soon-to-be-deleted bad data.
• Down() is a complete, symmetric reversal of Up(), including restoring the unfiltered unique index.
• New tests (Model_TagUniqueIndex_FiltersSoftDeletedRowsSoNamesCanBeReusedAfterDeletion, Model_UnboundedStringColumns_AreConstrainedToMaxLengths, Model_EncryptedTextColumns_CarryAdvisoryMaxLengthWithoutNarrowingStorageType, Model_HabitsAndTags_CascadeDeleteWithTheirOwningUser) directly assert the new model behavior.
• Cross-checked the other narrowed fields (Tag.Name, Tag.Color, Goal.Unit, User.TimeZone) against their FluentValidation validators — all are already guarded with matching or stricter MaximumLength/regex rules, so only the Language path above is exposed. User.ColorScheme/ThemePreference have no explicit MaximumLength validator but are safe in practice today via closed domain-layer whitelists.
• The advisory HasMaxLength on the two encrypted text columns (Goal.Title, HabitLog.Note) is correctly scoped to avoid generating DDL at design time, matching the PR's stated intent — confirmed no encrypted-at-rest column had its storage type narrowed.
• No SQL injection risk in the migration's raw Sql() calls — both DELETE statements are fully static, no interpolation of external input.

Recommendation

Fix the High finding — add a Language length/whitelist rule to VerifyCodeCommandValidator and GoogleAuthCommandValidator — before merging, since it's reachable from an unauthenticated endpoint and this migration is what turns a previously-latent gap into a live 500-on-signup risk. Close out the Medium orphan-cascade verification (or explicitly document it as already-checked-and-safe) before this migration runs against production.

[thomasluizon]

Thanks — both findings addressed:

High (Language 500-on-signup): Confirmed real. Users.Language is written from client input on the [AllowAnonymous] /api/auth/verify and /api/auth/google paths with no length validator, so narrowing it to varchar(10) would convert a latent gap into a live DbUpdateException on signup. It's the only narrowed column not already validator-guarded. Fixed in b4f8f78 by dropping the Language narrowing entirely (kept as text) — removed from OrbitDbContext.cs, the migration Up()/Down(), both snapshots, and the test. has-pending-model-changes confirms the model/snapshot are consistent. Bounding Language properly belongs with a MaximumLength/whitelist rule on VerifyCodeCommandValidator + GoogleAuthCommandValidator (mirroring SetLanguageCommandValidator) — those files are outside this slice's owned set, so deferred to the slice/PR that owns the auth validators.

Medium (orphan cascade verification): Verified read-only against production for the 4 orphaned habit ids across every Cascade/SetNull dependent: ChallengeParticipantHabits=0, AccountabilityPairHabits=0, HabitTags=0, HabitGoals=0, Cheers=0 (and HabitLogs=0 as noted). The cleanup DELETE touches nothing beyond the 4 dead habit rows — no collateral data loss.

[claude]

PR Review — #358 "chore(api): harden EF model constraints for Tag, User, Goal, Habit"

Recommendation: APPROVE

This is the second review pass. Checked prior reviews/threads first: the earlier review (CHANGES_REQUESTED) raised one High and one Medium finding. Both are now resolved and nothing new surfaced since.

What changed since the last review

Commit b4f8f78f3b3b3818ac594e59b15f5037cc651ae5 ("fix(api): drop Users.Language narrowing to keep unauthenticated signup safe") is a clean, minimal revert across exactly the 5 files that needed it:

• src/Orbit.Infrastructure/Migrations/20260712221154_HardenModelConstraints.cs (both Up()/Down() — the Language AlterColumn blocks removed)
• src/Orbit.Infrastructure/Migrations/20260712221154_HardenModelConstraints.Designer.cs
• src/Orbit.Infrastructure/Migrations/OrbitDbContextModelSnapshot.cs
• src/Orbit.Infrastructure/Persistence/OrbitDbContext.cs:688 (only ColorScheme/ThemePreference/TimeZone retain HasMaxLength now)
• tests/Orbit.Infrastructure.Tests/Persistence/OrbitDbContextTests.cs (matching assertion removed)

Verified via full-diff grep that Language no longer carries any HasMaxLength/narrowed-column DDL anywhere in the generated or hand-written artifacts — it stays plain text, closing the High finding (unauthenticated /api/auth/verify and /api/auth/google writing an unvalidated client-supplied Language would otherwise have thrown DbUpdateException on signup once narrowed to varchar(10)).

The Medium finding (orphan-cascade verification scope) is addressed via the author's PR comment documenting a production read-only check across every Cascade/SetNull-dependent table on Habit (ChallengeParticipantHabits, AccountabilityPairHabits, HabitTags, HabitGoals, Cheers all = 0, plus the original HabitLogs = 0).

Security review

• No SQL injection risk (migration's Sql() calls are all static strings).
• New Cascade FKs (Habit→User, Tag→User) don't conflict with AccountDeletionService, which already explicitly deletes Habits/Tags before Users.Remove — no double-delete/race.
• Remaining narrowed columns (Tag.Name, Tag.Color, Goal.Unit, User.TimeZone, User.ColorScheme, User.ThemePreference) are all guarded by existing MaximumLength/domain-whitelist validators on authenticated-only endpoints.
• No controller files in this diff.

Deferred / N/A

Cross-repo contract check N/A — no DTO/endpoint touched in this diff, so nothing to check against the un-checked-out orbit-ui-mobile sibling repo. Parity/i18n/DESIGN.md/FEATURES.md dimensions N/A — no frontend surface, no user-facing feature-surface change.

Minor, non-blocking note

The PR body's "Changes"/"Tests" sections still reference Language(10) narrowing and "seven new max-lengths" — stale since the fix commit (now six: Tag.Name, Tag.Color, User.ColorScheme, User.ThemePreference, User.TimeZone, Goal.Unit). Cosmetic only, not counted as a finding.

Severity	Count
Critical	0
High	0
Medium	0
Low / Info	0 posted (1 cosmetic note, non-blocking)

Validation: Build/Tests skipped per CI adaptation — handled by separate required Build / Unit Tests / SonarCloud checks.

[thomasluizon]

Addressed both review findings (HEAD now 260260b):

High — Language signup-500 risk (complete fix, not a scope-down). Rather than dropping the Users.Language narrowing, restored it and closed the underlying gap: VerifyCodeCommandValidator and GoogleAuthCommandValidator now carry the same .MaximumLength(AppConstants.MaxLanguageLength).Must(SupportedLanguages.Contains) guard used elsewhere, so an over-length/unsupported Language on the unauthenticated /api/auth/verify and /api/auth/google paths returns a clean 400 instead of a DbUpdateException. Added validator unit tests (valid / over-length / unsupported / empty) to both. Prod verified deploy-safe: max(length(Language)) = 5, 0 rows > 10 chars, only en/pt-BR.

Medium — orphan cascade. Verified against prod: the 4 orphaned Habits have 0 rows in ChallengeParticipantHabits and 0 in AccountabilityPairHabits, so the cleanup DELETE cascades to nothing beyond the orphaned Habits/Tags themselves. No data-loss path.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #358 — chore(api): harden EF model constraints for Tag, User, Goal, Habit

Scope: Third review pass. Prior reviews checked: first pass (CHANGES_REQUESTED, High: unvalidated Language on signup path; Medium: orphan-cascade verification) — both resolved and approved in the second pass. Since that approval, the author reverted the "drop Language narrowing" workaround and replaced it with the proper fix: re-narrow Users.Language to varchar(10) AND add matching validators on the two auth signup paths. This review focuses on that delta.
Recommendation: APPROVE (no Critical/High findings)

What changed since the last review

Diff scoped to exactly: Users.Language AlterColumn restored in the migration Up()/Down() (+ Designer/snapshot mirrors), plus new .Must(lang => AppConstants.SupportedLanguages.Contains(lang)) rules added to GoogleAuthCommandValidator and VerifyCodeCommandValidator.

This directly and correctly closes the original High finding: instead of leaving Language unbounded text (the earlier workaround), the two unauthenticated signup paths (/api/auth/verify, /api/auth/google) now validate Language before it reaches SaveChangesAsync, matching the existing SetLanguageCommandValidator pattern. An over-length or unsupported Language value now fails cleanly with a 400 instead of risking a DbUpdateException on signup.

Findings

Critical / High

None.

Medium (non-blocking, pre-existing — not part of this delta)

Migration Down() recreates the Tags unique index as non-partial, contradicting the PR's rollback-safety claim.
src/Orbit.Infrastructure/Migrations/20260712221154_HardenModelConstraints.cs — Up() replaces IX_Tags_UserId_Name with a partial index (WHERE "IsDeleted" = FALSE) so a tag name can be reused after soft-delete. Down() recreates it non-partial. Once a user has soft-deleted and recreated a tag with the same name (the exact scenario this PR enables), an emergency Down() will fail with a unique-violation. This code is unchanged since the first commit and was already implicitly approved in the prior review pass — not gating this decision, but worth a fast follow-up: add WHERE "IsDeleted" = FALSE to the Down() index recreation, or document the migration as forward-only once the feature is exercised.

Low / Info

None new.

Security review

Subagent (security-reviewer) reviewed the delta (GoogleAuthCommandValidator.cs, VerifyCodeCommandValidator.cs, migration) — PASS, no findings. Whitelist check is a bounded in-memory Contains against a fixed set, no injection/DoS surface. Migration Sql() calls remain fully static.

Deferred / N/A

• Cross-repo contract check — N/A, no DTO/endpoint/Controller touched.
• Timezone / Authorization hard rules — N/A, not implicated by this diff.

Recommendation

Approve. The delta correctly resolves the previously-flagged High finding with the proper fix (validation, not narrowing removal). The one Medium note is pre-existing, non-blocking, and doesn't affect the automated forward-deploy path.