[Backport 3.3] Preserve response headers across context restore in SecurityInterceptor

[KishoreKicha14]

Description

Backport of #6123

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 3bc417d)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Resource Leak The restorableContextSupplier.get() at line 403 is called without storing the returned StoredContext in a variable or using try-with-resources. If this StoredContext holds resources that need cleanup, they will leak. The old code called restore() which likely handled cleanup, but get() returns a new context that should be closed. contextToRestore.get(); Inconsistent Pattern In handleException() (line 434), the code uses try-with-resources to properly close the context from contextToRestore.get(). However, in handleResponse() (line 403), the same call is made without try-with-resources or any cleanup. This inconsistency suggests either handleResponse() has a resource leak or handleException() has unnecessary cleanup. Given the try-with-resources pattern in handleException(), the handleResponse() call likely needs the same treatment. contextToRestore.get();

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 3bc417d
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Properly close restored context resource The restored context from contextToRestore.get() is not being closed or managed with try-with-resources, which could lead to context leaks. The context should be properly closed after use to prevent resource leaks and ensure proper cleanup. src/main/java/org/opensearch/security/transport/SecurityInterceptor.java [403-429] -contextToRestore.get(); +try (ThreadContext.StoredContext ignore = contextToRestore.get()) { + final boolean isDebugEnabled = log.isDebugEnabled(); + if (response instanceof ClusterSearchShardsResponse) { + // ... rest of the logic + } + + innerHandler.handleResponse(response); +} -final boolean isDebugEnabled = log.isDebugEnabled(); -if (response instanceof ClusterSearchShardsResponse) { - Suggestion importance[1-10]: 10 __ Why: Critical resource leak issue. The contextToRestore.get() returns a StoredContext that must be closed to prevent context leaks. The current code at line 403 calls get() without managing the returned resource, while handleException() correctly uses try-with-resources. This inconsistency could cause context pollution and resource leaks in production.	High

---

Previous suggestions

Suggestions up to commit ff7c5f6

Category	Suggestion	Impact
Possible issue	Ensure context is properly closed The restored context from contextToRestore.get() is not being used or closed, which may lead to resource leaks. Wrap the context restoration in a try-with-resources block to ensure proper cleanup, similar to how handleException() is implemented. src/main/java/org/opensearch/security/transport/SecurityInterceptor.java [403-429] -contextToRestore.get(); +try (ThreadContext.StoredContext ignore = contextToRestore.get()) { + final boolean isDebugEnabled = log.isDebugEnabled(); + if (response instanceof ClusterSearchShardsResponse) { + ... + } + + innerHandler.handleResponse(response); +} -final boolean isDebugEnabled = log.isDebugEnabled(); -if (response instanceof ClusterSearchShardsResponse) { - Suggestion importance[1-10]: 9 __ Why: This is a critical resource leak issue. The contextToRestore.get() returns a StoredContext that should be closed to prevent resource leaks. The suggestion correctly identifies that it should be wrapped in try-with-resources, matching the pattern used in handleException().	High

[github-actions]

Persistent review updated to latest commit 3bc417d