Skip to content

Require UPDATE permission for markAppsAsLatest and updateSourceControlMeta - #16193

Open
herdiyana256 wants to merge 1 commit into
cdapio:developfrom
herdiyana256:fix-applifecycle-marklatest-missing-authz
Open

Require UPDATE permission for markAppsAsLatest and updateSourceControlMeta#16193
herdiyana256 wants to merge 1 commit into
cdapio:developfrom
herdiyana256:fix-applifecycle-marklatest-missing-authz

Conversation

@herdiyana256

Copy link
Copy Markdown

ApplicationLifecycleService.markAppsAsLatest() and updateSourceControlMeta() (cdap-app-fabric/src/main/java/io/cdap/cdap/internal/app/services/ApplicationLifecycleService.java) mutate application state with no authorization check at all, unlike every other mutating method on this class: deleteAllStates and removeApplication both call accessEnforcer.enforce() before touching the store, these two didn't call it anywhere.

Both are reachable from any authenticated principal via AppLifecycleHttpHandlerInternal (POST /v3Internal/namespaces/{namespace-id}/apps/markLatest and .../apps/updateSourceControlMeta). That handler is registered on the exact same APP_FABRIC_HTTP server / SERVER_HANDLERS_BINDING Guice multibinder as every public v3 handler (AppFabricServiceRuntimeModule.java:539-541), so it's reachable on the general HTTP surface, not some network-isolated internal-only service. The handler itself only checks namespaceQueryAdmin.exists(namespaceId), namespace existence, not caller access to that namespace.

Impact: markAppsAsLatest flips which application version is treated as "latest", the version used when a program run is triggered without an explicit version pinned (this is the same isLatest flag AppMetadataStore/DefaultStore use to resolve the default version for a run). updateSourceControlMeta overwrites an application's git commit hash / source-control metadata. A caller with zero access to a namespace, someone who only knows or guesses that namespace exists, can perform either mutation against applications they don't own.

Fix adds a StandardPermission.UPDATE enforce() call per ApplicationId before either mutation, mirroring the existing deleteAllStates/removeApplication pattern, plus a regression test (ApplicationLifecycleServiceMarkLatestAuthorizationTest) built on the same real InMemoryAccessController/DefaultContextAccessEnforcer/AuthenticationTestContext stack used by ConfigHandlerAuthorizationTest and FileFetcherHttpHandlerInternalAuthorizationTest, verifying both methods now reject an unprivileged caller and never touch the store.

Verification note: the full Maven reactor build is too slow to complete in this environment (same caveat as #16188/#16191/#16192). The fix was checked against the real source and sibling enforce() call sites line by line; the vulnerable control flow (both methods having zero enforcement while sibling methods in the same class have it) was independently reproduced in a standalone JDK harness that copies the real method bodies verbatim against a stub AccessEnforcer, confirming an unprivileged principal is rejected by the sibling method but not by the two unfixed ones. The new test was checked for type/signature correctness against the real constructor and utility classes but not executed locally; correctness will be confirmed by CI.

ApplicationLifecycleService.markAppsAsLatest() and updateSourceControlMeta()
mutate application state (which version is treated as latest for run
triggers, and an app's git source-control metadata) without any
accessEnforcer call, unlike every other mutating method on this class
(deleteAllStates, removeApplication, deleteApplication). Both are reachable
by any authenticated caller through AppLifecycleHttpHandlerInternal
(POST /v3Internal/namespaces/{namespace-id}/apps/markLatest and
.../apps/updateSourceControlMeta), which registers on the same
APP_FABRIC_HTTP server surface as every public v3 handler; the handler only
checks that the namespace exists, not that the caller has any access to it.

This adds a StandardPermission.UPDATE enforce() check per application before
either mutation runs, mirroring the pattern already used by
deleteAllStates/removeApplication in the same class, plus a regression test.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds authorization checks to the markAppsAsLatest and updateSourceControlMeta methods in ApplicationLifecycleService to ensure that callers have StandardPermission.UPDATE permissions on the target applications. Additionally, a new unit test class ApplicationLifecycleServiceMarkLatestAuthorizationTest is introduced to verify that unauthorized requests are correctly rejected. There are no review comments, and I have no feedback to provide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants