fix: sanitize user input in model prompts - #458
Conversation
|
Approach for ORB-93:
Planned files are the five services named in the ticket, AiSlipAlertMessageServiceTests.cs, AiProactiveCheckinMessageServiceTests.cs, and one focused prompt sanitization test file. A non-interpolated prompt redesign and sanitizer changes remain out of scope. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef51fcf2b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
CI follow-up: the first arch-map run treated capitalized payload words in the new tests as domain entity references. The test markers now avoid those false positives. A staged-tree regeneration confirmed both architecture.json and architecture.html are byte-for-byte unchanged before this push. |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
P1 follow-up approach:
This changes the existing goal service, goal query builder, and existing prompt sanitization suite only. The branch remains within the eight file and 400 line limits. |
|
CI follow-up: the regression fixture initially used a capitalized payload marker that arch-map interpreted as a domain entity reference. The marker is now lowercase. Local regeneration left architecture.json and architecture.html byte-for-byte unchanged, and the Linux drift job passes at head 05f9d3f. |
|
DEGRADED: same-vendor review Review verdict: 1 Blocking finding after round 1. F1 High at src/Orbit.Infrastructure/Services/AiGoalReviewService.cs:23: aggregate post-composition sanitization preserves a standalone injected instruction and uses attacker-controlled length as its cap. Frozen for one fixer round. Same-family bias is not eliminated and its magnitude is unmeasured. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
DEGRADED: same-vendor review Same-vendor bias is known to favor same-family output; its magnitude in this PR review is unmeasured. Reviewed head: Verdict: CLEAN APPROVED BY PR-REVIEW: zero blocking findings Findings: none. Follow-ups: none. Applicable dimensions reviewed with no findings: 1 Correctness, 2 Dead / stale code, 3 SOLID / clean architecture, 4 No-workaround / root cause, 5 Test quality, 6 Security, and 11 Backend hard rules. N/A dimensions:
|
1 participant
DEGRADED: same-vendor review
Linear: ORB-93
Summary
Sanitizes user controlled values before they enter the five model prompts named in the ticket. Notification fallback headings now use normalized and capped values, and proactive habit titles are quoted individually before joining so a title containing the separator stays one list item. Goal review sanitization preserves the complete aggregate instead of truncating later active goals.
Root cause
The services interpolated habit titles, descriptions, display names, tag names, and goal context directly even though PromptDataSanitizer already defines the repository boundary for model prompt data. The first goal review fix used SanitizeBlock's default 2,000 character cap on the completed aggregate, which could cut a record and omit every later goal.
Design choice
Goal context remains sanitized defensively inside AiGoalReviewService. The sanitizer receives the original context length as its maximum because its normalization only removes or replaces input characters and never expands the aggregate. This retains every goal record while preserving newline, whitespace, and control character neutralization. Sanitizing at construction would reverse the Application to Infrastructure dependency, and duplicating the sanitizer there would create a second prompt normalization policy.
The prompt capture tests inspect every string in the actual SDK request body and locate the prompt by a stable marker. They do not read an assumed SDK field or construct a successful external response fixture.
Impact
Quotes, newlines, control characters, separators, and overlong individual values can no longer break prompt boundaries in the named paths. Goal reviews no longer silently lose later records when their aggregate exceeds 2,000 characters. Ordinary notification headings remain unchanged.
Validation