Require instance-level access for FileFetcherHttpHandlerInternal downloads - #16191
Open
herdiyana256 wants to merge 2 commits into
Open
Require instance-level access for FileFetcherHttpHandlerInternal downloads#16191herdiyana256 wants to merge 2 commits into
herdiyana256 wants to merge 2 commits into
Conversation
…loads FileFetcherHttpHandlerInternal.download() resolves the path given in the URL as an absolute location against the configured LocationFactory via Locations.getLocationFromAbsolutePath(), then streams that file back to the caller, with no authorization check of any kind. That resolution ignores the LocationFactory's own base/home entirely (create(URI) treats an absolute URI as an absolute location, independent of any configured root), so the effective input-to-sink path is: whatever absolute path substring follows /v3Internal/location/ in the URL becomes exactly the file returned, for the full storage backend the LocationFactory points at, not scoped to any namespace or resource the caller owns. This handler is registered on the same appfabric.http.handler set, same HTTP service, same discoverable service, and same Router path-matching as every public v3 API handler; nothing in RouterPathLookup or AuthenticationHandler special-cases v3Internal paths. Any principal with a valid CDAP access token, i.e. any authenticated non-admin developer, can therefore reach this endpoint and read any file the LocationFactory can access, including other namespaces' data on a shared storage backend. ConfigHandler already establishes the precedent for gating this class of raw-internal-state exposure: contextAccessEnforcer.enforce(InstanceId.SELF, StandardPermission.GET) before returning CConfiguration/HConfiguration. This applies the same check to FileFetcherHttpHandlerInternal.download().
There was a problem hiding this comment.
Code Review
This pull request introduces instance-level authorization checks to the internal file-fetch HTTP handler (FileFetcherHttpHandlerInternal) to prevent unauthorized access to arbitrary storage-backend paths. It also adds a new authorization test class and updates existing test configurations with the necessary Guice modules. The review feedback identifies an issue in the new test where a missing assertion could allow the test to pass even if the expected NotFoundException is not thrown, and provides a code suggestion to fix it.
…test Per gemini-code-assist review on cdapio#16191: without this, the test would silently pass even if download() never reached the location lookup.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FileFetcherHttpHandlerInternal.download() resolves the path given in the URL as an absolute location against the configured LocationFactory via Locations.getLocationFromAbsolutePath(), then streams that file back to the caller, with no authorization check of any kind. That resolution ignores the LocationFactory's own base/home entirely (create(URI) treats an absolute URI as an absolute location, independent of any configured root), so the effective input-to-sink path is: whatever absolute path substring follows /v3Internal/location/ in the URL becomes exactly the file returned, for the full storage backend the LocationFactory points at, not scoped to any namespace or resource the caller owns.
This handler is registered on the same appfabric.http.handler set, same HTTP service, same discoverable service, and same Router path-matching as every public v3 API handler; nothing in RouterPathLookup or AuthenticationHandler special-cases v3Internal paths. Any principal with a valid CDAP access token, i.e. any authenticated non-admin developer, can therefore reach this endpoint and read any file the LocationFactory can access, including other namespaces' data on a shared storage backend.
ConfigHandler already establishes the precedent for gating this class of raw-internal-state exposure: contextAccessEnforcer.enforce(InstanceId.SELF, StandardPermission.GET) before returning CConfiguration/HConfiguration. This applies the same check to FileFetcherHttpHandlerInternal.download(), and adds a dedicated authorization test mirroring ConfigHandlerAuthorizationTest, plus wires the existing FileFetcherHttpHandlerInternalTest's Guice injector with the modules needed to provide a ContextAccessEnforcer now that the constructor requires one.