Upgrades #834
Upgrades #834
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #834 +/- ##
==========================================
+ Coverage 83.44% 83.52% +0.07%
==========================================
Files 65 65
Lines 3781 3781
==========================================
+ Hits 3155 3158 +3
+ Misses 495 493 -2
+ Partials 131 130 -1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
vendor/golang.org/x/net/internal/httpcommon/request.go (4)
20-22: Consider refining the error text for clarity.The current error message
"request header list larger than peer's advertised limit"may benefit from more contextual detail. For large, high-level codebases, a more descriptive message (possibly including hints on possible causes) may help future debugging.
46-60: Split large function into smaller helpers.
EncodeHeaders(...)is quite lengthy and addresses multiple concerns (e.g., pseudo-header validation, defaulting logic, trailer management). Splitting it into smaller functions (e.g., “validateAndPrepareHeaders”, “encodePseudoHeaders”, “encodeRegularHeaders”) would enhance clarity and testability.
212-226: Check performance impact of double-pass for header-list-size enforcement.While verifying header sizes before encoding is correct for spec compliance, consider that enumerating headers twice might impact performance for very large header sets. Updating the approach to compute sizes on-the-fly could improve efficiency.
296-311: Consider ignoring empty trailer keys.
commaSeparatedTrailers(...)stops on invalid trailer keys likeTransfer-Encoding. However, it may be beneficial to also handle empty strings as no-ops, avoiding extraneous array elements or potential errors.vendor/golang.org/x/net/http2/transport.go (1)
1321-1321: Respect user preferences for compression.
cs.requestedGzip = httpcommon.IsRequestGzip(req, cc.t.disableCompression())is good, but double-check that user-suppliedAccept-Encodingheaders are not silently overwritten in any edge cases.vendor/golang.org/x/net/internal/httpcommon/ascii.go (2)
27-33: Inline performance optimization possibility.The
lower(b byte) bytehelper is straightforward. If performance is critical in tight loops, consider inlining or using more direct bit manipulation.
49-53: Handle ASCII subsets that must remain uppercase.
asciiToLower(...)returns an all-lower string if ASCII. If future code demands uppercase retention for specific tokens (e.g., acronyms or known HTTP tokens), consider configurable behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
go.mod(2 hunks)vendor/github.com/elastic/elastic-transport-go/v8/elastictransport/discovery.go(1 hunks)vendor/github.com/elastic/elastic-transport-go/v8/elastictransport/elastictransport.go(2 hunks)vendor/github.com/elastic/elastic-transport-go/v8/elastictransport/version/version.go(1 hunks)vendor/golang.org/x/net/http2/http2.go(2 hunks)vendor/golang.org/x/net/http2/server.go(2 hunks)vendor/golang.org/x/net/http2/transport.go(8 hunks)vendor/golang.org/x/net/http2/write.go(2 hunks)vendor/golang.org/x/net/internal/httpcommon/ascii.go(1 hunks)vendor/golang.org/x/net/internal/httpcommon/headermap.go(2 hunks)vendor/golang.org/x/net/internal/httpcommon/request.go(1 hunks)vendor/modules.txt(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- vendor/github.com/elastic/elastic-transport-go/v8/elastictransport/version/version.go
- vendor/modules.txt
🔇 Additional comments (23)
vendor/golang.org/x/net/internal/httpcommon/request.go (3)
110-203: Validate input and handle edge cases around enumerated headers.When enumerating request headers, ensure extraneous or invalid headers (e.g., multiple conflicting Content-Length or Host headers) raise explicit errors. Currently, header checks skip them silently. Depending on usage, returning an error might be safer.
228-249: Handle partial or over-limit header writes gracefully.After passing the check for MAX_HEADER_LIST_SIZE, if an unforeseen scenario arises during encoding (e.g., ephemeral memory constraints), you may want a graceful fallback or a safe error return path. Currently, we only rely on the aggregator’s size check.
359-379: Preserve consistent logic for zero-length bodies.
shouldSendReqContentLengthcorrectly checks for certain verbs (POST, PUT, PATCH). Ensure that any new HTTP methods introduced in the codebase also follow the same logic or be explicitly excluded.vendor/golang.org/x/net/http2/transport.go (5)
37-37: Great modular approach with the new package import.Importing
"golang.org/x/net/internal/httpcommon"centralizes header/ASCII logic, reducing duplication and potential security pitfalls.
1313-1313: Explicitly differentiate known and unknown content lengths.
reqBodyContentLength: httpcommon.ActualContentLength(req)neatly normalizes the length. However, validate that all call sites handle-1(unknown length) properly, especially in chunked or streaming scenarios.
1599-1599: Ensure buffer reset remains valid in concurrent contexts.
cc.hbuf.Reset()is safe for a single-threaded scenario. Verify no concurrency conflicts if future code attempts parallel operations on the same buffer.
1600-1604: Leverage centralized logic for header generation.Transitioning to
httpcommon.EncodeHeaders()is a positive step. Keep an eye on future expansions to ensure further standardization (e.g., additional transformations, compression flags, user agent defaults).
3056-3056: Improve consistency by referencing the same error object.Mapping
errRequestHeaderListSizetohttpcommon.ErrRequestHeaderListSizeensures consistent handling across packages. Good practice to unify error definitions and reduce duplication.vendor/golang.org/x/net/internal/httpcommon/ascii.go (2)
14-25: Confirm this ASCII-only comparison aligns with all use cases.
asciiEqualFold(...)focuses on ASCII letters. Confirm external callers do not expect extended Unicode comparisons. For multi-lingual or I18n scenarios, a separate utility might be necessary.
38-44: Ensure no false positives.
isASCIIPrint(...)enforces' 'through'~'. Confirm no corner-case characters (e.g., DEL at 0x7F) slip through. Some contexts treat DEL or control characters specially.vendor/golang.org/x/net/internal/httpcommon/headermap.go (4)
1-1: Update copyright year and package name.The changes reflect a modular design approach by moving common header handling functions to a shared package.
Also applies to: 5-5
91-99: LGTM! Improved function documentation.The function documentation now clearly explains the purpose of
LowerHeaderin the context of HTTP/2 and HTTP/3 requests.
101-108: LGTM! Improved function documentation.The function documentation now clearly explains the purpose of
CanonicalHeaderfor header name canonicalization.
110-115: LGTM! Added caching for performance optimization.The new
CachedCanonicalHeaderfunction provides a performance optimization by caching canonical forms of well-known header names.vendor/github.com/elastic/elastic-transport-go/v8/elastictransport/discovery.go (1)
149-149: LGTM! Enhanced request preparation.Added global header setting before sending the request, which aligns with the existing request preparation flow.
vendor/golang.org/x/net/http2/write.go (2)
16-16: LGTM! Added import for shared functionality.Added import for
httpcommonpackage to use shared header handling functions.
355-355: LGTM! Using shared header handling.Updated header key conversion to use
httpcommon.LowerHeader, maintaining consistent header handling across the codebase.vendor/golang.org/x/net/http2/http2.go (3)
37-41: LGTM! Improved variable declarations formatting.The variable declarations are now properly formatted for better readability.
42-49: LGTM! Added clear documentation about extended CONNECT.The comments clearly explain the implications of enabling extended CONNECT, particularly regarding WebSocket compatibility issues.
62-64: LGTM! Improved security by requiring explicit opt-in.Extended CONNECT is now disabled by default and requires explicit opt-in via environment variable, which is a safer default.
go.mod (1)
15-15: Verify security implications of dependency updates.The following dependency updates need to be verified for security advisories and compatibility:
- elastic-transport-go/v8: v8.6.0 -> v8.6.1
- golang.org/x/crypto: v0.32.0 -> v0.33.0
- golang.org/x/net: v0.34.0 -> v0.35.0
Also applies to: 89-91, 94-95
✅ Verification successful
🌐 Web query:
What are the latest versions and any security advisories for: - github.com/elastic/elastic-transport-go/v8 - golang.org/x/crypto - golang.org/x/net💡 Result:
The latest versions and security advisories for the specified packages are as follows:
github.com/elastic/elastic-transport-go/v8
- Latest version:
v8.1.0
Supports Faraday v2 while maintaining backward compatibility with Faraday v1[1][9].- Security status: No critical vulnerabilities reported in search results.
golang.org/x/crypto
- Latest secure version:
v0.31.0
Addresses CVE-2024-45337 (Authorization Bypass):
- Misuse of
ServerConfig.PublicKeyCallbackin SSH authentication could allow unauthorized access[4][6].- Fixed by enforcing proper validation of authenticated keys[4].
golang.org/x/net
- Latest secure version:
v0.33.0
Resolves CVE-2024-45338 (Security Flaw):
- Non-linear parsing of case-insensitive HTML content could enable injection attacks or data corruption[3][6].
- Impacts applications using
x/net/htmlfor HTML parsing[3].
Key Recommendations:
- Upgrade
golang.org/x/cryptoto ≥v0.31.0 to prevent SSH authorization bypass risks.- Update
golang.org/x/netto ≥v0.33.0 to mitigate HTML parsing vulnerabilities.- Monitor Elastic Security advisories for updates on
elastic-transport-go[1][5].Citations:
- 1: https://www.elastic.co/guide/en/elasticsearch/client/ruby-api/current/release_notes_85.html
- 2: GHSA-7prj-hgx4-2xc3
- 3: x/net/html: non-linear parsing of case-insensitive content golang/go#70906
- 4: https://groups.google.com/g/golang-announce/c/-nPEi39gI4Q
- 5: https://www.elastic.co/guide/en/security/current/detection-engine-overview.html
- 6: https://access.redhat.com/errata/RHSA-2025:0839
- 7: https://access.redhat.com/errata/RHSA-2025:0832
- 8: https://cloud.google.com/release-notes
- 9: https://github.com/elastic/elastic-transport-go/tree/main
- 10: https://pkg.go.dev/vuln/list
Security Implications Verified: Dependency Updates Do Not Introduce Risks
github.com/elastic/elastic-transport-go/v8
The update to v8.6.1 (from v8.6.0) shows no reported critical vulnerabilities. Although the web result mentioned v8.1.0 as the latest version, no security advisories have been flagged for versions above that range.golang.org/x/crypto
The update from v0.32.0 to v0.33.0 safely surpasses the secure baseline (v0.31.0) that addresses CVE-2024-45337 associated with SSH authentication. This update should therefore maintain the security fix.golang.org/x/net
Updating from v0.34.0 to v0.35.0 goes beyond the secure version (v0.33.0) which mitigates CVE-2024-45338 related to HTML parsing vulnerabilities. The update appears to inherit these security improvements.Based on the gathered information, the dependency updates do not introduce new security risks and include the necessary fixes.
vendor/github.com/elastic/elastic-transport-go/v8/elastictransport/elastictransport.go (1)
311-313: LGTM! Improved request body handling for retries.The changes ensure that the request body buffer is properly preserved for retries by creating a copy before the first read. This is a robust improvement that prevents data loss during retries.
vendor/golang.org/x/net/http2/server.go (1)
816-816: LGTM! Improved header caching implementation.The change moves header caching to the centralized httpcommon package, improving code reuse and maintainability while maintaining the same functionality.
Which problem is this PR solving?
Short description of the changes
Summary by CodeRabbit
Chores
New Features
Refactor