fix(api): resolve chat security hotspots + dedup habit AI tools (S5693, S6444)

[thomasluizon]

Resolves three SonarCloud findings in the chat surface, all behavior-preserving. Refs thomasluizon/orbit-ui-mobile#243.

1. Security hotspot — ChatController content length (S5693, MEDIUM/dos, lines 27 & 73)

ProcessChat and ProcessChatStream already carried a 10 MB RequestSizeLimit/RequestFormLimits, but the rule fires on literal sizes above its 8 MB default threshold (the sibling transcribe endpoint uses AppConstants.MaxAudioBytes and is not flagged — proof the rule only matches literals). The limit is now expressed through a named MaxChatRequestBytes constant, mirroring that convention, and the attributes that had been mashed onto one line are split out. The bound is unchanged (still 10 MB, matching the global Kestrel MaxRequestBodySize), so legitimate chat image uploads keep working.

2. Security hotspot — CreateHabitTool regex timeout (S6444, MEDIUM/dos, line 332)

HabitWordRegex (a compiled \bhabit\b) had no match timeout. It now passes a 1-second TimeSpan timeout, matching the matchTimeoutMilliseconds: 1000 convention already used in User.cs. Normal path is unaffected; only the pathological case is bounded.

3. Duplication — Chat/Tools/Implementations (~405 lines / 19 blocks)

Extracted the boilerplate the habit tools duplicated verbatim into a new internal HabitToolHelpers:

• ParseHabitIds + ApplyToHabitsAsync — bulk habit-id parsing and load-with-logs iteration, shared by BulkLogHabitsTool and BulkSkipHabitsTool (the largest 46-line block).
• TryParseHabitId + FindHabitAsync + InvalidHabitIdResult/HabitNotFoundResult — the parse-id / find / not-found guard shared by DeleteHabitTool, MoveHabitTool, UpdateHabitTool.
• ResolveOrCreateTagsAsync — the find-or-create-by-name tag logic shared by AssignTagsTool and CreateHabitTool.

The declarative GetParameterSchema() duplications were intentionally left alone: their per-tool-tuned description strings would have to be parameterized to share them, which is over-abstraction of what is really data.

Verification

• dotnet build Orbit.slnx — 0 errors, 0 new warnings.
• dotnet test Orbit.slnx — 5275 passed, 0 failed (Domain 512, Application 2773, Infrastructure 1983, Analyzers 7). All tool error-message assertions preserved.
• No narration comments (ORBIT0001), no orphaned code/usings.

Commits are split hotspots vs dedup.

[claude]

Review Complete

Scope: PR #386 in thomasluizon/orbit-api
Recommendation: APPROVE

Severity	Count
Critical (incl. ⚠️ old-client breaks)	0
High	0
Medium	0
Low / Info	1

Summary

The PR resolves two SonarCloud hotspots (S5693 — request-size literal on ChatController, S6444 — missing regex match-timeout on CreateHabitTool.HabitWordRegex) and extracts ~405 duplicated lines from seven IAiTool habit tools into a new Orbit.Application/Chat/Tools/Implementations/HabitToolHelpers.cs. All three changes are behavior-preserving. All 9 changed files were read in full (ChatController.cs, AssignTagsTool.cs, BulkLogHabitsTool.cs, BulkSkipHabitsTool.cs, CreateHabitTool.cs, DeleteHabitTool.cs, HabitToolHelpers.cs, MoveHabitTool.cs, UpdateHabitTool.cs) and each extracted helper was diffed against the original inline logic it replaced — every helper (ParseHabitIds, ApplyToHabitsAsync, TryParseHabitId/FindHabitAsync/InvalidHabitIdResult/HabitNotFoundResult, ResolveOrCreateTagsAsync) reproduces the original code line-for-line, including the UserId scoping on every repository query.

Findings

Info — src/Orbit.Api/CLAUDE.md states the chat endpoint has a 20MB multipart limit, but the code (both before and after this diff) uses 10MB (MaxChatRequestBytes = 10 * 1024 * 1024, same value as the prior 10_485_760 literal). Pre-existing doc/code mismatch, not introduced or touched by this PR — flagged only so it isn't lost, not blocking.

Subagents

Agent	Verdict
security-reviewer	PASS — no tenant-isolation regression (every extracted helper still filters by UserId), request-size bound numerically unchanged, regex timeout matches the existing User.cs matchTimeoutMilliseconds: 1000 convention
contract-aligner	N/A — no DTO, Controller route, or packages/shared type changed; ToolResult shapes/error strings are byte-for-byte identical

Validation

Not executed in this review session (CI's separate Build/Unit-Test checks are the binding gate). Per the PR body: dotnet build Orbit.slnx — 0 errors/0 new warnings; dotnet test Orbit.slnx — 5275 passed, 0 failed.

Deferred

DESIGN.md/AI-slop, Parity (web↔mobile), i18n — N/A, no frontend files touched. FEATURES.md parity — N/A, pure refactor/hotspot fix with no tool added/removed/renamed and no gating/platform change. Cross-model second opinion — N/A, no Critical finding to check.

What's good

Clean "extract on the third+ use" refactor at the right DRY level (local to Chat/Tools/Implementations, not over-lifted); correctly left GetParameterSchema() duplication alone since those are tuned data strings, not logic; both hotspot fixes are minimal and match existing codebase conventions; no narration comments introduced (the new /// <summary> blocks are proper XML-doc).

[claude]

PR Review — #386

Scope: Incremental review since the last automated (claude, APPROVED, 2026-07-13 16:05:31 UTC) review.
Recommendation: APPROVE

Context

The prior review approved commits 2204893 (chat request-size / regex-timeout hotspot fixes) and a87a271 (initial HabitToolHelpers extraction with ParseHabitIds + ApplyToHabitsAsync). One commit landed after that review: ec264d1 ("collapse bulk-habit orchestration into one shared helper"), merged into the PR head at df7ea74. This review covers only that delta.

What changed since the last review

ec264d1 merges the already-approved ParseHabitIds + ApplyToHabitsAsync into a single HabitToolHelpers.RunBulkHabitActionAsync(habitRepository, userDateService, args, userId, noneAppliedError, tryApply, ct), narrowing ParseHabitIds to private. BulkLogHabitsTool and BulkSkipHabitsTool were updated to call the new signature.

Findings

Critical: None
High: None
Medium: None
Low / Info: None new (the one pre-existing Info from the prior review — Orbit.Api/CLAUDE.md's stale "20MB" doc reference vs. the actual 10MB limit — is unrelated to this commit and already surfaced).

Verification performed on the delta:

• Traced both call sites end-to-end against the pre-refactor logic they replace.
  • BulkLogHabitsTool.ExecuteAsync: date/parse orchestration moved inside the helper; lambda discards the unused today param ((habit, targetDate, _) => TryLogHabit(habit, targetDate, ct)) — matches prior behavior exactly.
  • BulkSkipHabitsTool.ExecuteAsync: weekStartDay correctly stays outside the helper (skip-specific, not shared with log); today/targetDate now flow from the helper into TrySkipHabit(habit, targetDate, today, weekStartDay, ct) in the right order/positions.
• Grepped src/ and tests/ for stale references to the removed public ApplyToHabitsAsync / now-private ParseHabitIds — none found. BulkUpdateHabitEmojisTool.cs's own ParseHabitIds is an unrelated, pre-existing, same-named private local method (different signature: out string? error), not a broken call site.
• No authz, timezone, validation, comment-policy, or security surface touched by this delta — it's a pure method-consolidation refactor within Chat/Tools/Implementations, one feature folder inside Orbit.Application.

Subagents

Agent	Verdict
security-reviewer	N/A for this delta — no new repository query, auth boundary, or user-input surface introduced beyond what the prior review already cleared; UserId scoping preserved unchanged in the moved code
contract-aligner	N/A — no DTO/Controller/route/packages/shared change in this commit

Validation

Skipped per CI-review scope (Build/Unit Tests run as separate required checks). PR body claims (from the prior, already-reviewed state): dotnet build 0 errors/0 new warnings; dotnet test 5275 passed, 0 failed.

Deferred

• DESIGN.md/AI-slop, Parity, i18n — N/A, no frontend files touched.
• FEATURES.md parity — N/A, pure internal refactor, no tool/endpoint added/removed/gated.
• Cross-repo contract (orbit-ui-mobile) — not verifiable in CI (sibling repo not checked out); not applicable anyway since no contract surface changed.
• Cross-model second opinion — not invoked, no surviving Critical finding to check.

What's good

The follow-up commit is a legitimate further-DRY pass on code the previous review had already verified line-for-line — it doesn't just move code around, it correctly threads today/targetDate through the new shared signature and tightens ParseHabitIds's visibility to private now that it has no external callers. No regressions, no new risk surface.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #386 (thomasluizon/orbit-api)

Scope: PR #386 — fix(api): resolve chat security hotspots + dedup habit AI tools (S5693, S6444)
Recommendation: APPROVE

Summary

This is the third automated pass on this PR. I checked prior bot reviews first: two earlier claude reviews already APPROVED this PR — one at commit 2204893+a87a271 (hotspot fixes + initial HabitToolHelpers extraction) and one at ec264d1 (the RunBulkHabitActionAsync consolidation). Since that second review, the only new commit is ab403c6 ("Merge branch 'main' into fix/api-chat-hotspots-dedup"). Diffing ec264d1..ab403c6 directly confirms it touches only files unrelated to this PR — .github/workflows/sonarcloud.yml, MarketingUnsubscribeTokenService.cs, SignedTokenCodec.cs, WaitlistConfirmationTokenService.cs, SignedTokenCodecTests.cs — all pulled in from main (post #385), none part of this PR's own change set. gh pr diff 386 confirms the PR's actual diff is still exactly the same 9 files already reviewed: ChatController.cs, AssignTagsTool.cs, BulkLogHabitsTool.cs, BulkSkipHabitsTool.cs, CreateHabitTool.cs, DeleteHabitTool.cs, HabitToolHelpers.cs, MoveHabitTool.cs, UpdateHabitTool.cs.

Nothing has changed in this PR's own content since the last approved review. A confirmatory security-reviewer pass on the two hotspot fixes (ChatController.MaxChatRequestBytes still = 10,485,760 = the prior literal, both actions retain class-level [Authorize]; HabitWordRegex timeout addition is behavior-preserving) and on the HabitToolHelpers consolidation (every query — FindHabitAsync, RunBulkHabitActionAsync, ResolveOrCreateTagsAsync — still scopes by UserId == userId, so no IDOR regression from the dedup) came back PASS.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

• Carried forward from the first review (pre-existing, not touched by this PR): src/Orbit.Api/CLAUDE.md documents a 20MB chat multipart limit; code (before and after this diff) uses 10MB. Doc/code mismatch, out of scope for this PR.

Subagents

Agent	Verdict
security-reviewer	PASS
contract-aligner	N/A — no DTO, route, or shared-type shape changed (constant rename + internal helper extraction only)

Validation

Check	Result
Build (dotnet)	N/A — skipped per instructions (separate CI check); CI Build check on the PR is already SUCCESS
Tests (dotnet)	N/A — skipped per instructions (separate CI check); PR body reports 5275 passed locally, Unit Tests CI check in progress

Deferred — N/A dimensions & files not verdicted

• Dimension 8 (DESIGN.md/AI-slop), 9 (Parity), 10 (i18n), 14 (FEATURES.md parity): N/A — no apps/* UI files or user-facing feature surface touched.
• Dimension 11 (Contract drift): N/A — no DTO/Zod/endpoint shape changed.
• Cross-repo (orbit-ui-mobile) dimensions: not verifiable — sibling repo not checked out in this pass (per instructions).
• All 9 changed files: verdicted (no new content since the prior two full reviews; re-confirmed via targeted security pass this round).

What's good

• Root-cause fix for both SonarCloud hotspots (named constant vs. literal for S5693, explicit match-timeout for S6444) rather than a suppression/annotation workaround.
• The HabitToolHelpers extraction reproduces original per-tool logic line-for-line, including the UserId ownership scoping on every query — verified across two independent passes now.
• PR body is precise about intent, verification (build/test counts), and explicitly calls out what was not refactored (GetParameterSchema() duplication) and why — good judgment on stopping at the right abstraction level.

Recommendation

No action needed. This is a re-confirmation of an already-clean, twice-approved PR; the only activity since the last approval was an unrelated merge-from-main that doesn't touch this PR's diff. Safe to merge once the remaining in-progress CI checks (SonarCloud, Mutation testing, Unit Tests, Analyze) complete.