Pin user.home in DocumentAccessPolicyTest so it passes when running as root - #1530
Open
rq-jwhitlock wants to merge 2 commits into
Open
Conversation
Three 'safe path' cases build their expected directory from user.home, but
'/root/' is a built-in blocked prefix and checkBlockedPaths runs before the
allow-list. When user.home is /root — which it is whenever the build runs as
root, as it does in a container-based CI — the fallback ~/worker-payload/
directory is itself blocked, and those cases fail for a reason unrelated to what
they assert:
DocumentAccessDeniedException: Access denied: path matches blocked prefix '/root/'
Pins user.home to a neutral value for each test and restores it afterwards, so
these tests describe the policy rather than the identity of the user running
them.
Reproduce on an unmodified checkout with:
docker run --rm -v $PWD:/w -v ~/.gradle:/root/.gradle -w /w <jdk21-image> \
sh -c './gradlew --offline :conductor-ai:test --tests "*DocumentAccessPolicyTest*"'
Note this is only the test-side fix. Because checkBlockedPaths runs before the
allow-list, an application running as root also cannot read from its own default
~/worker-payload/ directory at runtime. That ordering question is left alone
here.
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.
Make
DocumentAccessPolicyTestindependent of the running userProblem
Three
DocumentAccessPolicyTestcases fail whenever the build runs asroot, which is the norm for container-based CI:All three build their expected directory from
user.home:"/root/"is a built-in entry inDEFAULT_BLOCKED_PATH_PREFIXES, andvalidateAccessrunscheckBlockedPathsbeforecheckAllowedDirectories. So whenuser.homeis/root, the default payload directory is itself blocked, and these "safe path" cases fail for a reason unrelated to what they assert.Reproducing
On an unmodified checkout, in any JDK 21 container image (which run as root with
user.home=/root):Note that
-Duser.home=/rootalone does not reproduce it — the value has to be the JVM's actual resolved home.Fix
Pins
user.hometo a neutral value for the duration of each test and restores the original afterwards, so these tests describe the policy rather than the identity of the user running them. No production code changes.Verified on
main@1bad2c88d: green both locally as a normal user (49 tests, 0 failures) and as root insideeclipse-temurin:21-jdk. On unmodifiedmainthe same command in the same image fails 3 of 49 —shouldAllowFileUriUnderPayloadDir,shouldAllowPathUnderDefaultPayloadDir, andshouldFallbackToDefaultPayloadDirWhenParentDirNotSet— so the fix is load-bearing and the bug is pre-existing rather than introduced here.A related runtime concern, deliberately not addressed here
The same ordering affects production, not just tests. Because
checkBlockedPathsruns before the allow-list, an application running asroot— common in containers — cannot read from its own default~/worker-payload/directory, since that resolves under/root/.This PR does not change that, because the fix is a design decision rather than an obvious correction. Options include exempting the effective allowed directories from the blocklist, checking the allow-list first, or treating
/root/as blocked only when it is not the resolved payload root. Happy to follow up with a separate PR if you have a preference on which of those you'd want.