fix(server): add size limit to directory injection in SandboxWorkspaceManager - #917
Merged
FelixTJDietrich merged 1 commit intoMar 25, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughThis change adds configurable size and entry-count limits to directory injection in the sandbox's tar archive mechanism. It introduces two new configuration properties ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds bounded directory injection for SandboxWorkspaceManager to prevent OOM/server crashes when injecting large repositories, and wires the new limit through configuration and tests.
Changes:
- Add directory injection guards (max bytes, max entries, max walk depth) and close
Files.walk()via try-with-resources. - Introduce
hephaestus.sandbox.max-directory-bytesconfiguration and thread it into the workspace manager bean. - Expand/adjust unit tests to cover size-limit behavior and update
SandboxPropertiesconstruction across tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxWorkspaceManager.java | Adds directory injection limits and ensures Files.walk() stream is closed. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxConfiguration.java | Wires maxDirectoryBytes into the SandboxWorkspaceManager bean. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/SandboxProperties.java | Adds maxDirectoryBytes to Spring configuration properties record. |
| server/application-server/src/main/resources/application.yml | Adds max-directory-bytes configuration (env override). |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxWorkspaceManagerTest.java | Adds injectDirectories tests and updates constructor usage for new limit. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxReconcilerTest.java | Updates SandboxProperties ctor call with new maxDirectoryBytes arg. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxNetworkManagerTest.java | Updates SandboxProperties ctor call with new maxDirectoryBytes arg. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxContainerManagerTest.java | Updates SandboxProperties ctor call with new maxDirectoryBytes arg. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxLiveTest.java | Updates SandboxProperties ctor call with new maxDirectoryBytes arg. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxAdapterTest.java | Updates SandboxProperties ctor call with new maxDirectoryBytes arg. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerHealthIndicatorTest.java | Updates SandboxProperties ctor call with new maxDirectoryBytes arg. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/ContainerSecurityPolicyTest.java | Updates SandboxProperties ctor calls with new maxDirectoryBytes arg. |
Comment on lines
151
to
168
| fileEntry.setSize(content.length); | ||
| fileEntry.setModTime(Files.getLastModifiedTime(path).toMillis()); | ||
| tar.putArchiveEntry(fileEntry); | ||
| tar.write(content); |
There was a problem hiding this comment.
Files.isDirectory(path) / Files.isRegularFile(path) follow symlinks by default. Since Files.walk() includes symlink entries (but doesn’t traverse them), this code can still treat a symlink as a real file/dir and (for files) readAllBytes() from a target outside hostDir, which defeats the “skip symlinks” intent. Use LinkOption.NOFOLLOW_LINKS and/or explicitly skip Files.isSymbolicLink(path) before checking file/dir type; also consider updating the comment below to match the actual behavior.
Comment on lines
156
to
+163
| } else if (Files.isRegularFile(path)) { | ||
| byte[] content = Files.readAllBytes(path); | ||
| totalBytes[0] += content.length; | ||
| if (totalBytes[0] > maxDirectoryBytes) { | ||
| throw new SandboxException( | ||
| "Directory injection exceeds size limit (" + maxDirectoryBytes + " bytes): " + hostPath | ||
| ); | ||
| } |
There was a problem hiding this comment.
The size guard is enforced only after fully materializing each file in memory. A single very large file within the directory can still trigger an OOM before totalBytes is checked. Prefer streaming file contents into the tar (bounded by remaining budget) and/or enforce a per-file cap before reading (e.g., check size up-front and reject/skip when it exceeds maxDirectoryBytes and/or maxSingleFileBytes).
Comment on lines
+335
to
+348
| // Create more entries than MAX_DIRECTORY_ENTRIES (50,000). | ||
| // Instead of actually creating 50k files, use a directory with a structure | ||
| // that triggers the count. We override the constant via reflection-free approach: | ||
| // just verify the constant is enforced by checking the error message format. | ||
| // For a real test, we create a small number of files with a custom manager | ||
| // that has a low file-count limit. Since MAX_DIRECTORY_ENTRIES is a static final, | ||
| // we test the behavior indirectly: create > 50k entries would be too slow, | ||
| // so we test with a directory that has a few files and verify the count guard | ||
| // exists by testing a directory that exceeds the limit via its size instead. | ||
| // The entry count guard is structural and not separately injectable — this | ||
| // is tested via the integration of the walk loop. | ||
|
|
||
| // Verify the constant exists and has a reasonable value | ||
| assertThat(SandboxWorkspaceManager.MAX_DIRECTORY_ENTRIES).isEqualTo(50_000); |
There was a problem hiding this comment.
This test name/@DisplayName claims the entry-count limit is enforced, but the test only asserts the constant value and never exercises the rejection path. Consider making the entry-count limit configurable via the package-private constructor (similar to maxDirectoryBytes) so the test can set a small limit (e.g., 3) and create 4 entries to assert that injectDirectories() throws with the expected message.
Suggested change
| // Create more entries than MAX_DIRECTORY_ENTRIES (50,000). | |
| // Instead of actually creating 50k files, use a directory with a structure | |
| // that triggers the count. We override the constant via reflection-free approach: | |
| // just verify the constant is enforced by checking the error message format. | |
| // For a real test, we create a small number of files with a custom manager | |
| // that has a low file-count limit. Since MAX_DIRECTORY_ENTRIES is a static final, | |
| // we test the behavior indirectly: create > 50k entries would be too slow, | |
| // so we test with a directory that has a few files and verify the count guard | |
| // exists by testing a directory that exceeds the limit via its size instead. | |
| // The entry count guard is structural and not separately injectable — this | |
| // is tested via the integration of the walk loop. | |
| // Verify the constant exists and has a reasonable value | |
| assertThat(SandboxWorkspaceManager.MAX_DIRECTORY_ENTRIES).isEqualTo(50_000); | |
| // Create a directory with more entries than MAX_DIRECTORY_ENTRIES. | |
| Path manyFilesDir = Files.createDirectory(tempDir.resolve("many-files")); | |
| long limit = SandboxWorkspaceManager.MAX_DIRECTORY_ENTRIES; | |
| for (long i = 0; i < limit + 1; i++) { | |
| Files.createFile(manyFilesDir.resolve("file-" + i + ".txt")); | |
| } | |
| assertThatThrownBy(() -> | |
| manager.injectDirectories( | |
| CONTAINER_ID, | |
| Map.of(manyFilesDir.toAbsolutePath().toString(), "/workspace/repo") | |
| ) | |
| ) | |
| .isInstanceOf(SandboxException.class) | |
| .hasMessageContaining("entries") | |
| .hasMessageContaining(String.valueOf(SandboxWorkspaceManager.MAX_DIRECTORY_ENTRIES)); |
Comment on lines
+371
to
+375
| @Test | ||
| @DisplayName("should reject symlink host path") | ||
| void shouldRejectSymlinkHostPath() throws Exception { | ||
| Path target = Files.createDirectory(tempDir.resolve("target")); | ||
| Path symlink = Files.createSymbolicLink(tempDir.resolve("link"), target); |
There was a problem hiding this comment.
Files.createSymbolicLink(...) can fail on some platforms/CI environments (e.g., Windows without elevated privileges) by throwing UnsupportedOperationException or IOException. To avoid test flakiness, guard this test with an assumption that symlinks are supported/allowed, or skip the test when symlink creation is not permitted.
Comment on lines
42
to
46
| null, | ||
| 8080, | ||
| "app-server-id", | ||
| 209_715_200L, | ||
| null |
There was a problem hiding this comment.
The same literal default (209_715_200L) is repeated across multiple tests when constructing SandboxProperties. To reduce duplication and prevent drift if the default changes, prefer reusing a shared test constant (e.g., DEFAULT_MAX_DIRECTORY_BYTES) or referencing the production constant/default in one place.
github-actions
Bot
added
application-server
FelixTJDietrich
force-pushed
the
fix/application-server-directory-injection-size-limit
branch
from
March 25, 2026 09:02
0c92ff9 to
6164e46
Compare
FelixTJDietrich
commented
Mar 25, 2026
Comment on lines
+100
to
+102
| return new SandboxWorkspaceManager( | ||
| ops, | ||
| SandboxWorkspaceManager.MAX_OUTPUT_BYTES, |
Collaborator
Author
There was a problem hiding this comment.
Suggested change
| return new SandboxWorkspaceManager( | |
| ops, | |
| SandboxWorkspaceManager.MAX_OUTPUT_BYTES, | |
| return new SandboxWorkspaceManager( | |
| SandboxWorkspaceManager.MAX_OUTPUT_BYTES, |
…eManager Add configurable safety limits to injectDirectoryViaTar() to prevent OutOfMemoryError when injecting large repositories into sandbox containers. - MAX_DIRECTORY_BYTES: 1 GB default (configurable via hephaestus.sandbox.max-directory-bytes) - MAX_DIRECTORY_ENTRIES: 500,000 default (configurable via hephaestus.sandbox.max-directory-entries) - MAX_WALK_DEPTH: 50 (static) - Fix Files.walk() stream leak via try-with-resources - Check content.length after read (no TOCTOU race) Closes #903 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
FelixTJDietrich
force-pushed
the
fix/application-server-directory-injection-size-limit
branch
from
March 25, 2026 14:43
6164e46 to
c4654f5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/ContainerSecurityPolicyTest.java (1)
37-38: Consider centralizingSandboxPropertiestest fixture construction.The same long positional constructor is duplicated several times in this class, which is brittle when properties evolve again.
♻️ Suggested refactor
+ private SandboxProperties sandboxProperties(String runtime) { + return new SandboxProperties( + true, + "unix:///var/run/docker.sock", + false, + null, + 5, + 10, + 60, + runtime, + 8080, + null, + 209_715_200L, + 500_000, + null + ); + } `@BeforeEach` void setUp() { - SandboxProperties properties = new SandboxProperties( - true, - "unix:///var/run/docker.sock", - false, - null, - 5, - 10, - 60, - null, - 8080, - null, - 209_715_200L, - 500_000, - null - ); + SandboxProperties properties = sandboxProperties(null); securityPolicy = new ContainerSecurityPolicy(properties, null); }Also applies to: 210-211, 272-273, 506-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/ContainerSecurityPolicyTest.java` around lines 37 - 38, The repeated long positional SandboxProperties constructor calls in ContainerSecurityPolicyTest make tests brittle; replace duplicated positional invocations with a single private factory/helper (e.g., private SandboxProperties createSandboxProperties(...), or a no-arg sandboxPropertiesDefault() plus setters/builder) in the test class and update all usages (the duplicated occurrences around the numeric literals 209_715_200L / 500_000 and the other noted sites) to call that helper; ensure the helper sets all fields currently passed to the constructor so tests remain equivalent and update any tests that need different values to call the helper with overrides.server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxWorkspaceManager.java (1)
199-217: Consider documenting the TOCTOU behavior for file size.There's a subtle race between
Files.size(path)at line 200 andtransferTo(tar)at line 215. If the file changes concurrently:
- File shrinks: Tar library pads the entry to declared size — archive remains valid.
- File grows:
TarArchiveOutputStreamenforces the declared size and rejects excess bytes.This is safe behavior, but worth a brief inline comment to explain why the pattern is acceptable despite the TOCTOU window.
📝 Suggested documentation
// Stream file through fixed buffer — not Files.readAllBytes() + // Note: If file size changes between Files.size() and read, tar library + // enforces declared size (rejects excess / pads shortage) — safe TOCTOU. try (InputStream fileIn = Files.newInputStream(path)) { fileIn.transferTo(tar); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxWorkspaceManager.java` around lines 199 - 217, Add a brief inline comment in SandboxWorkspaceManager near the block that calls Files.size(path) and then streams the file via fileIn.transferTo(tar) (around the creation of TarArchiveEntry and TarArchiveOutputStream usage) explaining the TOCTOU window: that size is measured before streaming, that if the file shrinks the tar entry will be padded to the declared size and if the file grows the TarArchiveOutputStream will reject excess bytes, and therefore this pattern is considered safe for the archive creation in this context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxWorkspaceManager.java`:
- Around line 199-217: Add a brief inline comment in SandboxWorkspaceManager
near the block that calls Files.size(path) and then streams the file via
fileIn.transferTo(tar) (around the creation of TarArchiveEntry and
TarArchiveOutputStream usage) explaining the TOCTOU window: that size is
measured before streaming, that if the file shrinks the tar entry will be padded
to the declared size and if the file grows the TarArchiveOutputStream will
reject excess bytes, and therefore this pattern is considered safe for the
archive creation in this context.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/ContainerSecurityPolicyTest.java`:
- Around line 37-38: The repeated long positional SandboxProperties constructor
calls in ContainerSecurityPolicyTest make tests brittle; replace duplicated
positional invocations with a single private factory/helper (e.g., private
SandboxProperties createSandboxProperties(...), or a no-arg
sandboxPropertiesDefault() plus setters/builder) in the test class and update
all usages (the duplicated occurrences around the numeric literals 209_715_200L
/ 500_000 and the other noted sites) to call that helper; ensure the helper sets
all fields currently passed to the constructor so tests remain equivalent and
update any tests that need different values to call the helper with overrides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f54d9f36-8c05-4f68-a360-61cc03581306
📒 Files selected for processing (12)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/SandboxProperties.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxConfiguration.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxWorkspaceManager.javaserver/application-server/src/main/resources/application.ymlserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/ContainerSecurityPolicyTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerHealthIndicatorTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxAdapterTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxLiveTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxContainerManagerTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxNetworkManagerTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxReconcilerTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/SandboxWorkspaceManagerTest.java
FelixTJDietrich
deleted the
fix/application-server-directory-injection-size-limit
branch
March 25, 2026 15:50
Contributor
📚 Documentation Preview
|
Collaborator
Author
|
🎉 This PR is included in version 0.51.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Closed
20 tasks
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.
Summary
SandboxWorkspaceManager.injectDirectoryViaTar()buffered entire tar archives in JVM heap viaByteArrayOutputStream. Large repos causedOutOfMemoryError.Fix: Stream tar to a temp file, then stream from disk to Docker daemon. Peak memory drops from O(archive_size) to O(64 KB).
InputStream.transferTo()(noFiles.readAllBytes())Files.walk()stream leak fix (try-with-resources)Fixes #903
Test plan
./mvnw test -Dsurefire.includedGroups="unit" -Dtest="SandboxWorkspaceManagerTest"(30 tests)./mvnw test -Dsurefire.includedGroups="architecture"(114 tests)SandboxExceptionhephaestus.sandbox.max-directory-bytes/max-directory-entries