Address remaining post-merge review findings#1054
Conversation
- Preserve headers and content for 307/308 redirects in EgressEnvelopeHandler - Exclude ProposalCreated from active run count to prevent permanent quota lockout - Broaden daily digest quota to count all trigger types and profiles per user - Negate FTS5 rank scores so higher=better, consistent with vector/RRF sources - Resolve pre-commit hook path from git root to avoid cwd-dependent failures
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements, including a fix for the pre-commit hook path, score normalization for FTS5 results, and a more robust user-level quota check for agent runs. However, the changes to EgressEnvelopeHandler for handling 307/308 redirects require attention. The current implementation of copying request content directly can lead to InvalidOperationException or issues with non-seekable streams. Furthermore, copying all headers without filtering (especially Authorization and Host) introduces security risks and potential protocol violations during cross-host redirects.
| // 307/308 require preserving the original headers and body | ||
| if (statusCode is 307 or 308) | ||
| { | ||
| redirectRequest.Content = request.Content; |
There was a problem hiding this comment.
Directly assigning request.Content to the new redirectRequest is problematic for two reasons:
- Repeatability: If the content is a non-seekable stream (e.g.,
StreamContent), it cannot be read again for the redirect request. - Ownership: In .NET, an
HttpContentinstance can only be associated with oneHttpRequestMessageat a time. Attempting to send the new request while the content is still 'associated' with the previous one (which hasn't been disposed) will throw anInvalidOperationExceptionin many environments.
Consider buffering the content if redirects are expected for POST/PUT requests, or ensuring the content is detached/cloned.
| foreach (var header in request.Headers) | ||
| { | ||
| redirectRequest.Headers.TryAddWithoutValidation(header.Key, header.Value); | ||
| } |
There was a problem hiding this comment.
Copying all headers blindly during a redirect is a security risk. The Host header should always be excluded as it must match the new destination. Additionally, the Authorization header should be stripped if the redirect host differs from the original host to prevent credential leakage to unauthorized third parties, following standard HttpClient security behavior.
1 participant
Summary
Test plan
SendAsync_307Redirect_PreservesHeadersAndContentverifies method, headers, and content are forwarded