Skip to content

Harden AOP runtime enforcement and fix testing/tooling gaps found in the 2026-07-15 audit - #128

Merged
MarkusPaulsen merged 8 commits into
mainfrom
fix/aop/aop-aspectj-instrumentation-audit-2026
Jul 20, 2026
Merged

Harden AOP runtime enforcement and fix testing/tooling gaps found in the 2026-07-15 audit#128
MarkusPaulsen merged 8 commits into
mainfrom
fix/aop/aop-aspectj-instrumentation-audit-2026

Conversation

@LukaPetrovicTUM

Copy link
Copy Markdown
Contributor

PR: Harden AOP runtime enforcement and fix testing/tooling gaps found in the 2026-07-15 audit

Branch: fix/aop/aop-aspectj-instrumentation-audit-2026
Issue: Markus Paulsen's AOP/AspectJ/instrumentation audit, 2026-07-15 (audited against chore/fix-pmd-spotbugs-findings, PR #116; re-verified against current main post-PR #126)

Overview

This branch consolidates every work item from the 2026-07-15 AOP/AspectJ/instrumentation audit onto
a single branch and PR, rather than Ares2's usual one-PR-per-issue convention — an explicit,
deliberate exception to keep an entire audit's remediation on one reviewable branch/PR instead of
splitting it across many small ones.

Of the audit's 14 tracked work items:

  • 11 implemented: wi-003, wi-009, wi-010, wi-011, wi-012, wi-013 are complete and
    need no further validation beyond Ares2's own test/static-analysis suites; wi-001, wi-002,
    wi-004, wi-005, wi-006 are code-complete but still need a SCORE reproducibility run before
    they're fully closed (see ## Validation).
  • 2 intentionally left open, each requiring a maintainer decision on a genuine design trade-off
    rather than a one-line fix (wi-007, wi-008 — see ## Known remaining work).
  • 1 confirmed already resolved on main with no code change needed (wi-014 / I-077).

Status: not yet ready for review. Every static-analysis gate passes locally (see
## Validation), but the full {ArchUnit, WALA} × {AspectJ, instrumentation} test matrix and the
SCORE reproducibility runs for wi-001/wi-002/wi-004/wi-005/wi-006 have not been executed
in this environment (no mvn test/validate-with-score.sh run yet beyond what's listed below) —
see ## Validation for the exact outstanding commands before merging.

Changes

I-067 + TD-019 — ProcessBuilder.startPipeline command-execution bypass

Source: 2026-07-15 audit, work item wi-001

ProcessBuilder.startPipeline(List<ProcessBuilder>) was a genuinely separate JDK command-execution
entry point that neither the AspectJ pointcut nor the instrumentation method map intercepted, even
though the static-analysis (ArchUnit/WALA) inventories already treated it as a sink. Added
startPipeline to both backends' pointcuts/method-maps, and taught both backends' command-extraction
logic to unpack a List<ProcessBuilder> argument by re-reading each builder's current command()
live at call time (not a constructor-time snapshot) — closing the specific TOCTOU bypass (construct
with an allowed command, mutate afterward, then call startPipeline).

I-068 + TD-020 + TD-013 — composite network methods checked against only one real action

Source: 2026-07-15 audit, work item wi-002

HttpClient.send/sendAsync, URL.openStream, URLConnection.getOutputStream, and
URLConnection.getInputStream each perform two distinct network actions, but both AOP backends
checked each against only one. Added a deriveActionChecks dispatch (new in AspectJ; extended a
pre-existing but previously-unused extension point in instrumentation) so each of the four methods
now checks both real actions independently — a SEND-allowed/RECEIVE-denied policy now correctly
blocks HttpClient.send. Also fixed a separate, architecturally distinct TD-013 bug: Phobos's
collectAllowHostsAndPorts silently dropped receive-only permissions from its generated allowlist
(JavaPhobosTestCase.java); true per-action CONNECT/SEND/RECEIVE parity with the AOP layer isn't
achievable for Phobos's host:port-granularity sandbox, so only the generation-time receive omission
was fixable here — TD-013 is only partially closed by this change, not fully.

I-105 + TD-051 — className.startsWith prefix-collision in runtime trust checks

Source: 2026-07-15 audit, work item wi-003

Both AOP backends' call-stack trust check used a bare className.startsWith(allowedClass) (and the
package equivalent) with no boundary check, while the static-analysis side already used the correct
exact-or-$-boundary comparison — an allowed class p.TrustedTest also wrongly permitted
p.TrustedTestEvil; an allowed package com.foo also wrongly permitted com.foobar. Ported the
static side's exact-or-boundary comparison ($ for classes, . for packages) into both runtime
backends, so runtime and static enforcement finally agree. No SCORE validation needed — this isn't a
JDK-API-access outcome.

I-110 + TD-059 — Unix domain sockets fail open on target-resolution failure

Source: 2026-07-15 audit, work item wi-004

variableToTarget's generic SocketAddress fallback returned null when toString() didn't parse
as host:port — always true for UnixDomainSocketAddress (no colon) — and analyseViolation treated
a null target as "not forbidden", so an intercepted-but-unparseable network operation was silently
allowed. Added explicit UnixDomainSocketAddress support (path as host, -1 as the no-port sentinel)
and changed the generic fallback so any unrecognised SocketAddress now produces a target instead of
null — a fail-closed backstop for future address-family shapes, not just this one.

I-113 + TD-062 — Files.mismatch bypasses all four monitoring layers

Source: 2026-07-15 audit, work item wi-005

Files.mismatch(Path, Path) was absent from all four monitoring inventories (both AOP backends' read
pointcuts/maps and both static-analysis method inventories), so it could read arbitrary files under a
zero-file-access policy. Fixed purely by registering it — no advice-logic change needed, since both of
mismatch's arguments need the same READ action and the existing default parameter-checking
behaviour already checks every parameter.

I-114 + TD-063 (narrowed) — Files.copy source misclassified, transferFrom absent

Source: 2026-07-15 audit, work item wi-006

The audit's Files.move claim is stale (already fixed since the audit — dual write+delete
classification exists in both backends); the parts still genuinely broken: Files.copy's source path
was checked as OVERWRITE instead of READ — a real privilege-escalation path, since a policy
granting only OVERWRITE for a path would incidentally also let a student read its contents via
Files.copy — and FileChannel.transferTo's read side was never checked at all, with
transferFrom completely unmonitored. Added a checkCopyOrTransferSpecialCase (both backends) that
intercepts these three calls before the generic per-action check, independently verifying:
Files.copy source=READ / destination=CREATE-or-OVERWRITE depending on REPLACE_EXISTING;
FileChannel.transferTo receiver=READ; FileChannel.transferFrom receiver=OVERWRITE. Known,
documented limitation: a channel-typed source/destination argument isn't resolvable to a checkable
path with the existing machinery — accepted, since such a channel was already checked at
FileChannel.open() time.

I-032 — JavaAOPTestCaseSettings non-atomic writes (scoped fix)

Source: 2026-07-15 audit, work item wi-009

JavaAOPTestCaseSettings holds the whole active policy as ~22 independent private static volatile
fields with no atomic whole-set update on write or reset — two concurrent security-test-case
setup/reset sequences on the same JVM could interleave field-by-field. The audit's full recommendation
(a single immutable policy object behind one atomically-swapped reference) would require every
settings-reading prelude across all eight check*InteractionImpl methods (both AOP backends) plus
JavaExecuter's setup sequence to also change — too large to safely verify without running the full
test/architecture matrix in this environment, so this PR ships the enrichment's own explicit
fallback instead: JavaAOPTestCaseSettings.reset() and JavaAOPTestCase.setSettingFieldOnLoader's
individual field writes now synchronize on the same SETTINGS_LOCK, resolved via reflection
per-classloader (the bootstrap- and application-classloader copies of the settings class are
different objects with different lock instances, so a static import would silently synchronize on the
wrong copy's lock). Added a concurrent stress test (4 writer + 4 resetter threads, 200 iterations
each) verifying no exceptions/reflection failures — this proves the scoped fix is sound, not that
every possible race across the full settings surface is closed.

I-111 + TD-060 — FileSystemAccessReadTestOld.java's 800 undiscovered tests

Source: 2026-07-15 audit, work item wi-010

FileSystemAccessReadTestOld.java (6,309 lines, 800 @PublicTest methods) never matched any
Surefire profile's **/*Test.java include pattern (the TestOld suffix doesn't end in Test.java),
so its tests silently never ran. git log --follow confirms the 800→92-method reduction into
FileSystemAccessReadTest.java was an intentional, documented decision (a commit literally titled
"Create FileSystemAccessReadTest skeleton by copying old test and removing test methods", plus
read_methods.md's curated 20-method taxonomy) — this PR deletes the old file outright rather
than silently continuing to let it linger unexecuted.

Also added the build-time safety net TD-060 asks for:
PublicTestNamingConventionTest.java scans src/test/java for any @PublicTest-using file whose
name doesn't end in Test.java, exempting a file only if some *Test.java elsewhere in the tree
demonstrably wires it up via @UserBased(<SimpleName>.class) (verified against all ten pre-existing
integration/testuser/*User.java files, not just assumed). Running this check surfaced a second,
previously undocumented instance of the same defect class
: MavenConfigurationUser.java had no
@UserBased wrapper anywhere, so its one @PublicTest method never ran either — fixed with a new
MavenConfigurationTest.java wrapper following the exact pattern used by its siblings. The new safety
net also turned out to need its own fix to actually run: no Surefire profile's <includes> covered
testutilities/** at all (unit-core-tests is api/**-only, integration-core-tests is
integration/**-only) — added **/testutilities/**/*Test.java to unit-core-tests's includes,
without which this whole deliverable would have been inert.

I-063 + TD-017 — NetworkPenguin's echo-response assertion re-enabled

Source: 2026-07-15 audit, work item wi-011

The audit's "external dependency, skip-when-unreachable" design concern is already fixed
(NetworkUser.connectLocallyAllowed already follows AGENTS.md's documented external-echo-server
pattern), but NetworkPenguin.connectAndCheckSentLine's response-content check really was still
commented out. Re-enabled it as assertThat(in.nextLine()).isEqualTo(expectLine) (AssertJ), inside
the existing try-with-resources so the 200ms SoTimeout is genuinely exercised by a real read. No
external echo server is currently provisioned in this repo's CI (.github/workflows/maven.yml has no
reference to port 25565), so this fix cannot newly break CI today — the connection attempt still fails
first with a caught IOException in every run; it unlocks the intended verification for whenever such
a server is provisioned. Also confirmed NetworkUser is genuinely invoked (via
@UserBased(NetworkUser.class) in NetworkTest.java), closing an open question the audit itself
raised about its non-standard naming.

I-086 + TD-030 — JAXB pointcuts unverifiable at weave time

Source: 2026-07-15 audit, work item wi-012

JavaAspectJFileSystemPointcutDefinitions.aj's javax.xml.bind.Marshaller/Unmarshaller pointcuts
referenced types the JDK stopped shipping as a standard module from Java 11 onwards, so on this
repo's Java 17 compliance level they could never be weave-verified. Added
javax.xml.bind:jaxb-api:2.3.1 as a provided-scope dependency, mirroring the existing commons-io
interception-target pattern exactly (never referenced by Ares's own code, kept off consumers'
transitive footprint).

I-109 + TD-056 — four dead JavaAOPModeTest methods reactivated

Source: 2026-07-15 audit, work item wi-012

Four fully-written JavaAOPModeTest methods had been left inside a block comment. They predate a
refactor that split copy-configuration handling into FS/non-FS variants
(getCopyFSConfigurationEntries/getCopyNonFSConfigurationEntries,
fsFilesToCopy/nonFSFilesToCopy, fsTargetsToCopyTo(Path)/nonFSTargetsToCopyTo(Path)) and
replaced per-test Mockito.mockConstruction(JavaCSVFileLoader.class, ...) with a single replaceable
static fileLoader field (AOPMode.setFileLoader(...)) — the commented code no longer compiles
against the current API at all. Rewrote all four tests against the current API rather than a
mechanical uncomment, adding @BeforeEach/@AfterEach reflection-based capture-and-restore of the
static fileLoader field so mocking it in one test can't leak into another sharing the same Surefire
fork.

I-049 — hardcoded developer-machine path in pointcut_comparison.R

Source: 2026-07-15 audit, work item wi-013

tools/pointcut_comparison.R hardcoded /Users/markuspaulsen/Documents/Ares2 as its repo root.
Replaced with a resolve_repo_root() helper deriving the root from the script's own --file=
invocation argument (commandArgs(trailingOnly = FALSE), the standard base-R idiom for a script's own
location — no new package dependency). Also added a header note disclosing the parser's known
unreliability (plain regexes over Java/AspectJ/Markdown source, no real handling of nesting,
string-literal contents, or multi-line signatures) — a full syntax-aware rewrite is explicitly out of
scope for this fix.

I-061 — agent JAR docs falsely claimed to bundle dependencies

Source: 2026-07-15 audit, work item wi-013

docs/HowToMakeAProjectAnAresProject.md claimed the agent JAR bundles "all bundled dependencies
(bundled under their original package names, without relocation)". Re-derived directly from
pom.xml's shade-plugin config rather than re-trusting the audit's report: both shade executions'
<artifactSet><includes> list only de.tum.cit.ase:ares, no dependency jars merged in. Corrected
three passages (§3.1.2, §3.1.3, and the glossary's :agent row) to state the real mechanism: the
agent JAR carries only Ares's own classes plus the Premain-Class manifest entry; Byte Buddy reaches
the instrumented JVM because -javaagent attaches to the same JVM already running the tests, whose
classpath already carries Byte Buddy transitively via testImplementation "de.tum.cit.ase:ares".
Docs-only — no build configuration changed.

I-062 — undocumented (but justified) source-in-JAR shipping

Source: 2026-07-15 audit, work item wi-013

Traced the actual call chain (FileTools.resolveFileOnSourceDirectoryAspectJAJCopyFiles.csv
real .aj/.java source files as copy Source entries, consumed by JavaCSVFileLoader) confirming
that shipping .aj/.java source under src/main/java as a packaged resource is a genuine,
functional dependency of a real code-generation flow, not incidental bloat — the audit's own hedge
("apparently needed") undersold this. Added a comment directly on pom.xml's src/main/java
<resource> block documenting why, and warning against narrowing its includes in isolation.
Deliberately did not attempt to narrow the packaging includes themselves, which would need an
mvn-verified build cycle this environment cannot safely perform.

I-070 + TD-022 — orphaned reproducibility CSV removed

Source: 2026-07-15 audit, work item wi-013

docs/test-comparison-permitted.csv (126KB, stale timestamp) had zero references anywhere in the
repo and no reconstructable reproduction path (git log --follow shows only a single formatting
commit touching it, no "add" commit or generating script). Deleted it, per the audit's own fallback
recommendation to remove it absent a reproduction path.

SpotBugs housekeeping

Running the full static-analysis gate against these changes surfaced one genuine SpotBugs finding on
wi-009's fix: USO_UNSAFE_ACCESSIBLE_OBJECT_SYNCHRONIZATION on JavaAOPTestCaseSettings.reset(),
which synchronizes on SETTINGS_LOCK inside the same class that exposes it via the public
getSettingsLock() accessor. That exposure is intentional — it's exactly how JavaAOPTestCase and
the advice toolboxes reach the other classloader's copy of the same lock reflectively, the whole
point of the wi-009 fix. Added a documented, narrowly-scoped exclusion to
.settings/spotbugs-exclude.xml (Class + Method + Bug pattern), matching every other
exclusion's existing rationale-carrying style, rather than redesigning the locking mechanism.

Known remaining work

  • I-018 / wi-007 (AspectJ intercepts RandomAccessFile.read() directly; instrumentation
    deliberately excludes it citing a recursive self-interception risk during class/JAR loading): a
    re-entrancy guard (enterAdvice/exitAdvice) already exists and is used everywhere else in the
    instrumentation backend, but whether it actually covers the deeper JVM class-loading bootstrapping
    concern the original exclusion comment describes needs a real instrumented build run to verify,
    which this environment cannot do. Write-up prepared for review before implementation:
    .claude/misc/wi-007-review-question-file-engine-symmetry.md.
  • I-116 / wi-008 (~20 file-metadata-probing methods — Files.exists/size/isReadable/etc.,
    legacy File.exists/canRead/etc. — are completely unmonitored by all four layers): three real
    design options exist (fold into READ, a new narrower metadata permission tier, or a
    public/private method split), and the fix changes the SecurityPolicy.yaml schema/policy model,
    not just interception code — squarely a maintainer decision, not something to make unilaterally in
    this PR. Write-up: .claude/misc/wi-008-review-question-file-metadata-permission-model.md.

I-077 / wi-014 (AspectJ Thread.Builder/Thread.Builder.OfPlatform.start monitoring) needed no
action: both AOP backends already use the binary $-nested-class pointcut spelling, confirmed
already resolving fine at Java 17 compliance and documented in docs/aop/BlockThreadSystemAccessAOP.md.

Validation

Static analysis — run and passing:

mvn -DskipTests compile                                          # BUILD SUCCESS (confirms the new
                                                                   # JAXB pointcuts actually weave)
mvn spotless:check checkstyle:check pmd:check spotbugs:check
  • First run failed on spotless:check (27 files had formatting drift from hand-editing without ever
    running the formatter); fixed with mvn spotless:apply (mechanical reflow only, no semantic
    change).
  • Second run: BUILD SUCCESS — 0 Checkstyle violations, 0 PMD violations, 0 SpotBugs findings
    (after adding the one documented exclusion described above).

Not yet run in this environment — outstanding before merge:

mvn test -Punit-core-tests -f pom.xml            # unit tests, incl. JavaAOPModeTest,
                                                  # JavaAOPTestCaseSettingsTest, PublicTestNamingConventionTest
mvn test -Punit-architecture-tests -f pom.xml    # architecture unit tests
mvn test -Pintegration-core-tests -f pom.xml     # integration tests, incl. NetworkTest, MavenConfigurationTest
mvn test -Parchitecture-tests -f pom.xml         # full ArchUnit/WALA architecture matrix

Per this repo's own testing guideline, a change to enforcement/policy code isn't considered
verified until all four {ArchUnit, WALA} × {AspectJ, instrumentation} combinations have run
— none
of the four have been run yet for this branch; no test runner was available in the environment this
PR was prepared in.

SCORE reproducibility — pending for the 5 items that need it:

Work item SCORE attack method Status
wi-001 CommandSystemExecutionAccess#PROCESS_BUILDER_PIPELINE (id 3, existing) pending
wi-002 NetworkSystemSendAccess#HTTP_CLIENT_SEND (id 11, new, uncommitted in ../SCOREReproducibilityPackage) needs authoring review + run
wi-004 NetworkSystemConnectAccess#UNIX_DOMAIN_SOCKET (id 9, new) + ResourceConfig.unixDomainSocketPath() needs authoring review + run
wi-005 FileSystemReadAccess#FILES_MISMATCH (id 16, new) needs authoring review + run
wi-006 FileSystemReadAccess#FILES_COPY (id 17, new; transferFrom SCORE method deliberately deferred) needs authoring review + run

Exact commands for each are in
.claude/test-instructions/fix-aop-aop-aspectj-instrumentation-audit-2026.md. wi-003, wi-009,
wi-010, wi-011, wi-012, wi-013 need no SCORE validation (not JDK-API-access outcomes, or
Ares2's own internal build/test/tooling concerns).

Note

Full per-work-item chronology, file ownership, and the reasoning behind every scoping decision
(what was narrowed, what was deliberately deferred, and why) is maintained in
.claude/branch-context/fix-aop-aop-aspectj-instrumentation-audit-2026.md for anyone reviewing
this branch in detail.

@LukaPetrovicTUM
LukaPetrovicTUM requested review from a team and krusche as code owners July 19, 2026 23:06
@github-actions github-actions Bot added aop Automated area label: aop policy Automated area label: policy docs Automated area label: docs tests Automated area label: tests other Automated area label: other labels Jul 19, 2026
@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@MarkusPaulsen, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 79ccdefb-6be1-49ca-ab50-8e61f03d3a4c

📥 Commits

Reviewing files that changed from the base of the PR and between e73e914 and a9855b3.

📒 Files selected for processing (1)
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java
📝 Walkthrough

Walkthrough

This PR updates Ares security interception for concurrent settings, call-stack boundaries, ProcessBuilder pipelines, filesystem copy/transfer roles, Unix-domain sockets, and composite network actions. It also adds build configuration, localisation, tests, integration fixtures, and policy resources covering the updated behaviour.

Changes

Security enforcement

Layer / File(s) Summary
Settings and build support
pom.xml, src/main/java/..., src/test/java/..., .settings/..., docs/...
Settings writes and resets share a lock; build metadata, agent initialisation, documentation, test selection, localisation, and supporting test isolation are updated.
Call-stack trust boundaries
src/main/java/.../*AbstractAdvice*, src/test/java/{p,com,example}/...
Class and package allow-list matching honours exact, nested-class, and dot boundaries.
Command pipeline enforcement
src/main/java/.../*CommandSystem*, src/test/.../ProcessBuilder*, src/main/resources/.../messages*.properties
Pipeline commands are validated per ProcessBuilder; invalid elements fail closed and violations are localised.
Filesystem operation roles
src/main/java/.../*FileSystem*, src/main/resources/.../file-system-access-methods.txt, src/test/.../fileSystem/*
Files.copy, Files.mismatch, transferTo, and transferFrom use role-specific read, create, and overwrite checks.
Network targets and composite actions
src/main/java/.../*Network*, src/main/java/.../JavaPhobosTestCase.java, src/test/.../Network*
Unix-domain and unparseable socket addresses are handled fail-closed, while composite calls validate connect, send, and receive actions independently.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Suggested labels: security fix

Suggested reviewers: krusche


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
Sandbox Fail-Closed Behaviour ❌ Error FAIL: FileChannel transfer special-cases can still skip checks—AspectJ never resolves FileChannel receivers, and instrumentation returns null on reflection failure, so unresolved inputs are allowed. Deny when FileChannel receiver/path resolution fails, or remove the special-case skip until both backends can resolve it safely; add a hard fail for inaccessible or malformed receiver state.
Docstring Coverage ⚠️ Warning Docstring coverage is 22.70% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and clearly reflects the AOP security-hardening and tooling fixes in this change set.
Description check ✅ Passed The description covers the overview, change details, and rationale, even though it uses different headings than the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Trusted Boundary Preservation ✅ Passed All main-source changes stay in Ares infrastructure; test fixtures/probes remain under src/test, and no src/main code imports test-only packages. New boundary checks tighten class/package matching.
Github Workflow Least Privilege ✅ Passed No .github/workflows files are changed in this PR diff, so the workflow least-privilege check is not applicable.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/aop/aop-aspectj-instrumentation-audit-2026

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java (1)

93-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add pointcut-map coverage assertions for the new I-114 methods.

These new tests exercise checkCopyOrTransferSpecialCase directly via InstrumentationSecurityProbe, bypassing JavaInstrumentationPointcutDefinitions's method-to-action map. testInstrumentationPointcutsContainNewCoverage (lines 93-103) doesn't assert that Files.copy/Files.mismatch/FileChannel.transferTo/transferFrom are actually registered there. If that map were incomplete, production interception would silently never fire while every test here still passes.

Consider extending testInstrumentationPointcutsContainNewCoverage with assertions that METHODS_WHICH_CAN_READ_FILES/METHODS_WHICH_CAN_OVERWRITE_FILES contain the new java.nio.file.Files and java.nio.channels.FileChannel entries.

Also applies to: 228-358

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java`
around lines 93 - 103, Extend testInstrumentationPointcutsContainNewCoverage to
assert that the pointcut maps register the new I-114 methods: Files.copy and
Files.mismatch under the appropriate read/overwrite maps, and
FileChannel.transferTo and transferFrom under the appropriate maps. Use
JavaInstrumentationPointcutDefinitions.METHODS_WHICH_CAN_READ_FILES and
METHODS_WHICH_CAN_OVERWRITE_FILES, verifying the java.nio.file.Files and
java.nio.channels.FileChannel entries so production interception cannot be
missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj`:
- Around line 362-364: Prevent malicious List implementations from executing
before trust validation by updating JavaAspectJCommandSystemAdviceDefinitions.aj
at lines 362-364, 424-426, and 476-478 to call
requireTrustedRuntimeType(observedVariable) immediately upon entering each
List<?> branch in analyseViolation, extractViolationPath, and
extractExecutablePathViolation. At lines 221-226, split the combined
variableToCommand branch so requireTrustedRuntimeType(variableValue) runs before
any stream() invocation.

In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj`:
- Around line 656-698: Update checkCopyOrTransferSpecialCase so each matching
Files.copy, FileChannel.transferTo, and FileChannel.transferFrom branch returns
true only after handling its supported action; for any other action, fail closed
using the same unrecognised-action exception behavior as the generic path.
Preserve the existing validation and return behavior for read and overwrite.

In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java`:
- Around line 653-723: Update checkCopyOrTransferSpecialCase to explicitly
handle SocketChannel counterparts in FileChannel.transferTo and transferFrom.
Add a dedicated guard for non-file channel peers so transferTo(...,
socketChannel) and the corresponding transferFrom case cannot bypass
network-output checks, unless the project’s intended policy explicitly excludes
these operations; preserve the existing file-channel source/destination checks.

In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java`:
- Around line 720-724: Update deriveActionChecks to replace the direct
HttpClient instanceof check with a reflective lookup of HttpClient using
ClassLoader.getPlatformClassLoader(), then test the instance against the
resolved class while preserving the existing send/sendAsync action checks and
return values.

In
`@src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTest.java`:
- Around line 709-750: Update all four test_accessFileSystemViaFilesCopyMaven*
methods to use the assertion helper overload that accepts the actual
copySource.txt path, or extend the helper usage with the expected path
explicitly, instead of the 2-argument assertAresSecurityExceptionRead overload
hardcoded to subject/nottrusted.txt. Add the required Path/Paths imports if
needed, while preserving each test’s policy variant and read-action message
assertion.

In
`@src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java`:
- Around line 29-33: Update the return type of sendToSendOnlyAllowedHost to
HttpResponse<Void> to match the HttpResponse.BodyHandlers.discarding() handler
used by client.send. Preserve the existing request and client behavior.

In
`@src/test/java/de/tum/cit/ase/ares/testutilities/PublicTestNamingConventionTest.java`:
- Around line 91-100: The file-reading helpers use the platform charset and
silently skip unreadable sources, weakening the naming-convention check. In
PublicTestNamingConventionTest, update usesPublicTestAnnotation and
concatenateSources to read with StandardCharsets.UTF_8 and propagate read
failures as UncheckedIOException instead of returning an empty or skipped
result; apply this to both the anchor and sibling sites in
src/test/java/de/tum/cit/ase/ares/testutilities/PublicTestNamingConventionTest.java
(lines 91-100 and 79-89).

In
`@src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml`:
- Around line 17-20: Replace the spaced empty-list values with compact [] values
for regardingNetworkConnections, regardingCommandExecutions,
regardingThreadCreations, and regardingPackageImports in both
PolicyFilesCopySourceOverwriteOnly.yaml files: aspectj lines 17-20 and
instrumentation lines 17-20.

In
`@src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml`:
- Around line 10-19: Remove the unnecessary interior spaces from the empty YAML
arrays in PolicyHttpSendAllowedReceiveDenied, including
regardingFileSystemInteractions and the other empty policy sections, while
preserving the existing non-empty network connection configuration.

In
`@src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml`:
- Around line 10-19: Replace every empty YAML list written as “[ ]” with “[]” in
the listed ranges of
src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml,
src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml,
and
src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml;
make no other changes.

---

Outside diff comments:
In
`@src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java`:
- Around line 93-103: Extend testInstrumentationPointcutsContainNewCoverage to
assert that the pointcut maps register the new I-114 methods: Files.copy and
Files.mismatch under the appropriate read/overwrite maps, and
FileChannel.transferTo and transferFrom under the appropriate maps. Use
JavaInstrumentationPointcutDefinitions.METHODS_WHICH_CAN_READ_FILES and
METHODS_WHICH_CAN_OVERWRITE_FILES, verifying the java.nio.file.Files and
java.nio.channels.FileChannel entries so production interception cannot be
missing.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 58f06e90-e6ea-4259-95fa-688ba14f1b0a

📥 Commits

Reviewing files that changed from the base of the PR and between dc65fff and 8aa1722.

⛔ Files ignored due to path filters (2)
  • docs/test-comparison-permitted.csv is excluded by !**/*.csv and included by docs/**
  • tools/pointcut_comparison.R is excluded by none and included by none
📒 Files selected for processing (57)
  • .settings/spotbugs-exclude.xml
  • docs/HowToMakeAProjectAnAresProject.md
  • pom.xml
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCase.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettings.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJAbstractAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemPointcutDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemPointcutDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceAbstractToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
  • src/main/java/de/tum/cit/ase/ares/api/phobos/JavaPhobosTestCase.java
  • src/main/resources/de/tum/cit/ase/ares/api/localization/messages.properties
  • src/main/resources/de/tum/cit/ase/ares/api/localization/messages_de.properties
  • src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/archunit/methods/file-system-access-methods.txt
  • src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/wala/methods/file-system-access-methods.txt
  • src/test/java/com/foo/AllowedPackageMember.java
  • src/test/java/com/foobar/UnrelatedPackageMember.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/CallstackTrustBoundaryTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPModeTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettingsTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitionsTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/phobos/JavaPhobosTestCaseTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/MavenConfigurationTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/CommandSystemAccessTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTestOld.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/NetworkSystemAccessTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/SystemAccessTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/commandSystem/execute/processBuilder/ProcessBuilderExecuteMain.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/fileSystem/read/copy/FilesCopyMain.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/fileSystem/read/copy/copySource.txt
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/fileSystem/read/mismatch/FilesMismatchMain.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/connect/unixdomainsocket/UnixDomainSocketConnectMain.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java
  • src/test/java/de/tum/cit/ase/ares/testutilities/PublicTestNamingConventionTest.java
  • src/test/java/example/student/AspectJSecurityProbe.java
  • src/test/java/example/student/InstrumentationSecurityProbe.java
  • src/test/java/p/TrustedTest.java
  • src/test/java/p/TrustedTestEvil.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
📜 Review details
⚠️ CI failures not shown inline (2)

GitHub Actions: Java CI with Maven / 3_Build.txt: Harden AOP runtime enforcement and fix testing/tooling gaps found in the 2026-07-15 audit

Conclusion: failure

View job details

##[group]Run mvn clean package -DskipTests
 �[36;1mmvn clean package -DskipTests�[0m
 shell: /usr/bin/bash -e {0}
 env:
   JAVA_HOME: /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/21.0.11-10/x64
   JAVA_HOME_21_X64: /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/21.0.11-10/x64
   MAVEN_ARGS: -ntp
 ##[endgroup]
 [INFO] Scanning for projects...
 [INFO]
 [INFO] ------------------------< de.tum.cit.ase:ares >-------------------------
 [INFO] Building Ares 2.0.1-Beta9
 [INFO]   from pom.xml
 [INFO] --------------------------------[ jar ]---------------------------------
 [INFO]
 [INFO] --- clean:3.5.0:clean (default-clean) @ ares ---
 [INFO] Deleting /home/runner/work/Ares2/Ares2/target
 [INFO]
 [INFO] --- enforcer:3.6.3:enforce (enforce-versions) @ ares ---
 [INFO] Rule 0: org.apache.maven.enforcer.rules.version.RequireMavenVersion passed
 [INFO] Rule 1: org.apache.maven.enforcer.rules.version.RequireJavaVersion passed
 [INFO]
 [INFO] --- resources:3.5.0:resources (default-resources) @ ares ---
 [INFO] Copying 110 resources from src/main/resources to target/classes
 [INFO] Copying 251 resources from src/main/java to target/classes
 [INFO]
 [INFO] --- compiler:3.15.0:compile (default-compile) @ ares ---
 [INFO] Recompiling the module because of changed source code.
 [INFO] Compiling 242 source files with javac [debug target 17] to target/classes
 [WARNING] system modules path not set in conjunction with -source 17
 ##[warning][WARNING] /home/runner/work/Ares2/Ares2/src/main/java/de/tum/cit/ase/ares/api/architecture/java/wala/CustomCallgraphBuilder.java:[105,38] com.ibm.wala.core.java11.Java9AnalysisScopeReader in com.ibm.wala.core.java11 has been deprecated and marked for removal
 ##[warning][WARNING] /home/runner/work/Ares2/Ares2/src/main/java/de/tum/cit/ase/ares/api/architecture/java/wala/CustomCallgraphBuilder.java:[594,90] JAVA in com.ibm.wala.classLoader.Language has been deprecated and marked for removal
 [INFO] /home/runner/work/Ares2/Ares2/src/main/java/de/tum/...

GitHub Actions: Java CI with Maven / Build: Harden AOP runtime enforcement and fix testing/tooling gaps found in the 2026-07-15 audit

Conclusion: failure

View job details

##[group]Run mvn clean package -DskipTests
 �[36;1mmvn clean package -DskipTests�[0m
 shell: /usr/bin/bash -e {0}
 env:
   JAVA_HOME: /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/21.0.11-10/x64
   JAVA_HOME_21_X64: /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/21.0.11-10/x64
   MAVEN_ARGS: -ntp
 ##[endgroup]
 [INFO] Scanning for projects...
 [INFO]
 [INFO] ------------------------< de.tum.cit.ase:ares >-------------------------
 [INFO] Building Ares 2.0.1-Beta9
 [INFO]   from pom.xml
 [INFO] --------------------------------[ jar ]---------------------------------
 [INFO]
 [INFO] --- clean:3.5.0:clean (default-clean) @ ares ---
 [INFO] Deleting /home/runner/work/Ares2/Ares2/target
 [INFO]
 [INFO] --- enforcer:3.6.3:enforce (enforce-versions) @ ares ---
 [INFO] Rule 0: org.apache.maven.enforcer.rules.version.RequireMavenVersion passed
 [INFO] Rule 1: org.apache.maven.enforcer.rules.version.RequireJavaVersion passed
 [INFO]
 [INFO] --- resources:3.5.0:resources (default-resources) @ ares ---
 [INFO] Copying 110 resources from src/main/resources to target/classes
 [INFO] Copying 251 resources from src/main/java to target/classes
 [INFO]
 [INFO] --- compiler:3.15.0:compile (default-compile) @ ares ---
 [INFO] Recompiling the module because of changed source code.
 [INFO] Compiling 242 source files with javac [debug target 17] to target/classes
 [WARNING] system modules path not set in conjunction with -source 17
 ##[warning][WARNING] /home/runner/work/Ares2/Ares2/src/main/java/de/tum/cit/ase/ares/api/architecture/java/wala/CustomCallgraphBuilder.java:[105,38] com.ibm.wala.core.java11.Java9AnalysisScopeReader in com.ibm.wala.core.java11 has been deprecated and marked for removal
 ##[warning][WARNING] /home/runner/work/Ares2/Ares2/src/main/java/de/tum/cit/ase/ares/api/architecture/java/wala/CustomCallgraphBuilder.java:[594,90] JAVA in com.ibm.wala.classLoader.Language has been deprecated and marked for removal
 [INFO] /home/runner/work/Ares2/Ares2/src/main/java/de/tum/...
🧰 Additional context used
📓 Path-based instructions (7)
**/*

⚙️ CodeRabbit configuration file

Dogmatically check all reviewed files for current British English in prose, comments, JavaDoc, documentation, workflow names, step names, issue/PR text, labels, user-facing messages, and review suggestions. Flag American spellings and grammar such as behavior, color, initialize, authorization, canceled, and program when they are natural-language text. Do not flag programming-language syntax, dependency coordinates, API names, class names, method names, package names, paths, URLs, quoted external identifiers, or other literals where American English is required by the technology.

Files:

  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemPointcutDefinitions.aj
  • src/main/resources/de/tum/cit/ase/ares/api/localization/messages.properties
  • src/test/java/de/tum/cit/ase/ares/testutilities/PublicTestNamingConventionTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/CallstackTrustBoundaryTest.java
  • src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/archunit/methods/file-system-access-methods.txt
  • src/test/java/de/tum/cit/ase/ares/integration/MavenConfigurationTest.java
  • src/main/resources/de/tum/cit/ase/ares/api/localization/messages_de.properties
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/fileSystem/read/copy/copySource.txt
  • src/test/java/p/TrustedTestEvil.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/fileSystem/read/mismatch/FilesMismatchMain.java
  • src/test/java/p/TrustedTest.java
  • src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/wala/methods/file-system-access-methods.txt
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/java/com/foobar/UnrelatedPackageMember.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitionsTest.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/java/com/foo/AllowedPackageMember.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/fileSystem/read/copy/FilesCopyMain.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/java/example/student/AspectJSecurityProbe.java
  • src/main/java/de/tum/cit/ase/ares/api/phobos/JavaPhobosTestCase.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCase.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettings.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/connect/unixdomainsocket/UnixDomainSocketConnectMain.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/NetworkSystemAccessTest.java
  • pom.xml
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceAbstractToolbox.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/commandSystem/execute/processBuilder/ProcessBuilderExecuteMain.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettingsTest.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitions.aj
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
  • src/test/java/de/tum/cit/ase/ares/api/phobos/JavaPhobosTestCaseTest.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJAbstractAdviceDefinitions.aj
  • docs/HowToMakeAProjectAnAresProject.md
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/CommandSystemAccessTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolboxTest.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemPointcutDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/SystemAccessTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTest.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
  • src/test/java/example/student/InstrumentationSecurityProbe.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPModeTest.java
**/*Test.java

📄 CodeRabbit inference engine (AGENTS.md)

**/*Test.java: A sandboxed test JVM must never spin up its own server (echo server, socket listener, etc.) to test incoming or outgoing connections
Outgoing-connection tests must connect to an external echo server at a configurable endpoint running as a separate process or CI service on the loopback at port 25565, exercising only the student's client behaviour
If the external echo server is not reachable, the test must skip (using JUnit Assumptions.abort) rather than fail
An Ares SecurityException on an explicitly allowed connection is always a real failure and must propagate (never skipped)
Do not hard-code a self-hosted listener as the connection counterpart; use an external echo service to avoid in-JVM BindException/thread/lifecycle flakiness

Files:

  • src/test/java/de/tum/cit/ase/ares/testutilities/PublicTestNamingConventionTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/CallstackTrustBoundaryTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/MavenConfigurationTest.java
  • src/test/java/p/TrustedTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitionsTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/NetworkSystemAccessTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettingsTest.java
  • src/test/java/de/tum/cit/ase/ares/api/phobos/JavaPhobosTestCaseTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/CommandSystemAccessTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/SystemAccessTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPModeTest.java
src/test/java/**/*.java

⚙️ CodeRabbit configuration file

Require tests to distinguish fixture failures from sandbox failures. Network tests must not start in-process listeners inside the sandbox; external fixtures may be skipped when absent, but explicit Ares SecurityException failures must propagate.

Files:

  • src/test/java/de/tum/cit/ase/ares/testutilities/PublicTestNamingConventionTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/CallstackTrustBoundaryTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/MavenConfigurationTest.java
  • src/test/java/p/TrustedTestEvil.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/fileSystem/read/mismatch/FilesMismatchMain.java
  • src/test/java/p/TrustedTest.java
  • src/test/java/com/foobar/UnrelatedPackageMember.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitionsTest.java
  • src/test/java/com/foo/AllowedPackageMember.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/fileSystem/read/copy/FilesCopyMain.java
  • src/test/java/example/student/AspectJSecurityProbe.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/connect/unixdomainsocket/UnixDomainSocketConnectMain.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/NetworkSystemAccessTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/commandSystem/execute/processBuilder/ProcessBuilderExecuteMain.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettingsTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java
  • src/test/java/de/tum/cit/ase/ares/api/phobos/JavaPhobosTestCaseTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/CommandSystemAccessTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/SystemAccessTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTest.java
  • src/test/java/example/student/InstrumentationSecurityProbe.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPModeTest.java
src/main/java/**/*.java

⚙️ CodeRabbit configuration file

Review as a Java 17 Maven security framework used to test untrusted student code in Artemis programming exercises. Prioritise sandbox escapes, fail-open behaviour, unsafe reflection, classloader/bootstrap boundary mistakes, global mutable state, concurrency races, insufficient canonicalisation, and changes that weaken file, command, thread, network, package, or class access restrictions. Treat unrecognised security-sensitive inputs as a potential fail-closed requirement. Prefer simple Java code and one field or method declaration per line.

Files:

  • src/main/java/de/tum/cit/ase/ares/api/phobos/JavaPhobosTestCase.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCase.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettings.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceAbstractToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
src/main/java/de/tum/cit/ase/ares/api/aop/**/*.java

⚙️ CodeRabbit configuration file

Focus on runtime enforcement integrity: intercepted methods, argument extraction, null handling, recursive advice guards, bootstrap classloader interaction, and whether denied operations can reach the JVM or operating system before checks run.

Files:

  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCase.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettings.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceAbstractToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
pom.xml

⚙️ CodeRabbit configuration file

Review Maven changes for dependency vulnerability exposure, Java 17 compatibility, plugin supply-chain risk, unnecessary module opens, and build behaviour that could weaken sandbox tests or instrumentation.

Files:

  • pom.xml
docs/**/*.md

⚙️ CodeRabbit configuration file

Use current British English. Check that security guarantees, known limitations, and required external fixtures are explicit and do not overstate enforcement.

Files:

  • docs/HowToMakeAProjectAnAresProject.md
🪛 ast-grep (0.44.1)
src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java

[info] 380-380: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information

(unencrypted-socket-java)

🪛 GitHub Actions: Java CI with Maven / 3_Build.txt
src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj

[warning] 1473-1473: AspectJ/Xlint warning [adviceDidNotMatch]: advice defined in JavaAspectJFileSystemAdviceDefinitions has not been applied.

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java

[error] 32-32: COMPILATION ERROR: incompatible types: inference variable T has incompatible equality constraints java.lang.String, java.lang.Void

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj

[warning] 906-906: AspectJ/Xlint warning [adviceDidNotMatch]: advice defined in JavaAspectJCommandSystemAdviceDefinitions has not been applied.

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitions.aj

[warning] 932-932: AspectJ/Xlint warning [adviceDidNotMatch]: advice defined in JavaAspectJNetworkSystemAdviceDefinitions has not been applied.


[warning] 927-927: AspectJ/Xlint warning [adviceDidNotMatch]: advice defined in JavaAspectJNetworkSystemAdviceDefinitions has not been applied.

🪛 GitHub Actions: Java CI with Maven / Build
src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj

[warning] 1473-1473: AspectJ/Xlint warning: advice defined has not been applied. (Xlint:adviceDidNotMatch)

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java

[error] 32-32: COMPILATION ERROR: incompatible types: inference variable T has incompatible equality constraints java.lang.String,java.lang.Void

🪛 GitHub Check: Analyse Java
src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java

[failure] 32-32:
incompatible types: inference variable T has incompatible equality constraints java.lang.String,java.lang.Void


[failure] 32-32:
incompatible types: inference variable T has incompatible equality constraints java.lang.String,java.lang.Void

🪛 GitHub Check: Build
src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java

[failure] 32-32:
incompatible types: inference variable T has incompatible equality constraints java.lang.String,java.lang.Void


[failure] 32-32:
incompatible types: inference variable T has incompatible equality constraints java.lang.String,java.lang.Void

🪛 LanguageTool
src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/archunit/methods/file-system-access-methods.txt

[typographical] ~143-~143: Il manque une espace après le point.
Context: ....list(java.nio.file.Path)
java.nio.file.Files.mismatch(java.nio.file.Path, java.nio.f...

(ESPACE_APRES_POINT)


[typographical] ~143-~143: Une espace est nécessaire.
Context: ...ava.nio.file.Path)
java.nio.file.Files.mismatch(java.nio.file.Path, java.nio.file.Path)
...

(PARENTHESES)


[typographical] ~143-~143: Il manque une espace après le point.
Context: ...va.nio.file.Files.mismatch(java.nio.file.Path, java.nio.file.Path)
java.nio.file.Fil...

(ESPACE_APRES_POINT)


[typographical] ~143-~143: Il manque une espace après le point.
Context: ...smatch(java.nio.file.Path, java.nio.file.Path)
java.nio.file.Files.move(java.nio.fil...

(ESPACE_APRES_POINT)

src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/wala/methods/file-system-access-methods.txt

[typographical] ~122-~122: Il manque une espace après le point.
Context: ...ist(Ljava/nio/file/Path;)
java.nio.file.Files.mismatch(Ljava/nio/file/Path;Ljava/nio/...

(ESPACE_APRES_POINT)


[typographical] ~122-~122: Une espace est nécessaire.
Context: ...va/nio/file/Path;)
java.nio.file.Files.mismatch(Ljava/nio/file/Path;Ljava/nio/file/Path;...

(PARENTHESES)

🪛 YAMLlint (1.37.1)
src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml

[error] 17-17: too many spaces inside empty brackets

(brackets)


[error] 18-18: too many spaces inside empty brackets

(brackets)


[error] 19-19: too many spaces inside empty brackets

(brackets)


[error] 20-20: too many spaces inside empty brackets

(brackets)

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml

[error] 17-17: too many spaces inside empty brackets

(brackets)


[error] 18-18: too many spaces inside empty brackets

(brackets)


[error] 19-19: too many spaces inside empty brackets

(brackets)


[error] 20-20: too many spaces inside empty brackets

(brackets)

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml

[error] 10-10: too many spaces inside empty brackets

(brackets)


[error] 17-17: too many spaces inside empty brackets

(brackets)


[error] 18-18: too many spaces inside empty brackets

(brackets)


[error] 19-19: too many spaces inside empty brackets

(brackets)

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml

[error] 10-10: too many spaces inside empty brackets

(brackets)


[error] 17-17: too many spaces inside empty brackets

(brackets)


[error] 18-18: too many spaces inside empty brackets

(brackets)


[error] 19-19: too many spaces inside empty brackets

(brackets)

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml

[error] 10-10: too many spaces inside empty brackets

(brackets)


[error] 17-17: too many spaces inside empty brackets

(brackets)


[error] 18-18: too many spaces inside empty brackets

(brackets)


[error] 19-19: too many spaces inside empty brackets

(brackets)

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml

[error] 10-10: too many spaces inside empty brackets

(brackets)


[error] 17-17: too many spaces inside empty brackets

(brackets)


[error] 18-18: too many spaces inside empty brackets

(brackets)


[error] 19-19: too many spaces inside empty brackets

(brackets)

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml

[error] 17-17: too many spaces inside empty brackets

(brackets)


[error] 18-18: too many spaces inside empty brackets

(brackets)


[error] 19-19: too many spaces inside empty brackets

(brackets)


[error] 20-20: too many spaces inside empty brackets

(brackets)

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml

[error] 17-17: too many spaces inside empty brackets

(brackets)


[error] 18-18: too many spaces inside empty brackets

(brackets)


[error] 19-19: too many spaces inside empty brackets

(brackets)


[error] 20-20: too many spaces inside empty brackets

(brackets)

🔇 Additional comments (68)
src/test/java/p/TrustedTestEvil.java (1)

1-26: LGTM!

docs/HowToMakeAProjectAnAresProject.md (3)

99-99: LGTM!


129-129: LGTM!


434-434: LGTM!

pom.xml (4)

68-68: LGTM!


272-286: LGTM!


336-345: LGTM!


1020-1025: LGTM!

src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPModeTest.java (6)

3-23: LGTM!


38-57: LGTM!


58-74: LGTM!


76-103: LGTM!


104-127: LGTM!


128-142: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/MavenConfigurationTest.java (1)

1-22: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java (2)

3-4: LGTM!


30-30: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemPointcutDefinitions.aj (1)

8-9: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java (1)

310-342: LGTM!

Also applies to: 365-381, 429-443, 483-495

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java (1)

369-382: LGTM!

Also applies to: 460-469, 603-603

src/main/resources/de/tum/cit/ase/ares/api/localization/messages.properties (1)

69-69: LGTM!

src/main/resources/de/tum/cit/ase/ares/api/localization/messages_de.properties (1)

4-4: LGTM!

src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolboxTest.java (1)

3-9: LGTM!

Also applies to: 100-144

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/commandSystem/execute/processBuilder/ProcessBuilderExecuteMain.java (1)

4-4: LGTM!

Also applies to: 20-34

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/CommandSystemAccessTest.java (1)

159-188: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitions.aj (1)

12-17: LGTM!

Also applies to: 177-183, 207-210, 221-229, 632-676, 909-909

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java (2)

216-223: LGTM!

Also applies to: 248-251, 262-270, 947-947


13-14: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Remove hard dependency on java.net.http to prevent NoClassDefFoundError.

Because this instrumentation toolbox is injected into the bootstrap classloader to intercept JDK classes, it cannot directly link against types in the java.net.http module (which is loaded by the Platform Classloader). Importing HttpClient creates a hard dependency that will crash the agent during class resolution.

Please remove this import and use reflection in deriveActionChecks to check the type safely. As per path instructions, prioritise classloader/bootstrap boundary mistakes.

🐛 Proposed fix for the import
 import java.net.UnixDomainSocketAddress;
-import java.net.http.HttpClient;
			> Likely an incorrect or invalid review comment.

Source: Path instructions

src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitionsTest.java (1)

1-52: LGTM!

src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java (1)

16-24: LGTM!

Also applies to: 307-393

src/main/java/de/tum/cit/ase/ares/api/phobos/JavaPhobosTestCase.java (1)

176-176: LGTM!

src/test/java/de/tum/cit/ase/ares/api/phobos/JavaPhobosTestCaseTest.java (1)

40-49: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/NetworkSystemAccessTest.java (1)

1-100: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/connect/unixdomainsocket/UnixDomainSocketConnectMain.java (1)

1-32: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCase.java (3)

245-253: 📐 Maintainability & Code Quality | ⚡ Quick win

American spelling in new comment: "synchronize" → "synchronise".

As per path instructions, prose must use current British English. Same issue is repeated in three other files touched by this commit (see consolidated comment).

Source: Path instructions


254-266: 🎯 Functional Correctness | ⚡ Quick win

Wrong localisation key reused for lock-resolution failure.

The catch (NoSuchMethodException | InvocationTargetException reflectionFailure) block reuses "security.creation.advice.no.such.field.exception", the same key already used above (lines 196-198) for an actual NoSuchFieldException. A failure to resolve/invoke getSettingsLock() is a different failure mode and this message will misleadingly report a "no such field" error when the real problem is a missing/failing getSettingsLock method — confusing anyone debugging a lock-resolution failure.

🐛 Proposed fix: dedicated message key
-		} catch (NoSuchMethodException | InvocationTargetException reflectionFailure) {
-			throw new SecurityException(JavaInstrumentationAdviceAbstractToolbox
-					.localize("security.creation.advice.no.such.field.exception", adviceSetting), reflectionFailure);
-		} finally {
+		} catch (NoSuchMethodException | InvocationTargetException reflectionFailure) {
+			throw new SecurityException(JavaInstrumentationAdviceAbstractToolbox
+					.localize("security.creation.advice.settings.lock.exception", adviceSetting), reflectionFailure);
+		} finally {

(plus the corresponding new key in both messages.properties and messages_de.properties)


169-243: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettings.java (2)

183-200: 📐 Maintainability & Code Quality | ⚡ Quick win

American spelling in new Javadoc: "synchronizes" (×2) → "synchronises".

As per path instructions, prose in JavaDoc must use current British English. Consolidated below with the same issue in three other files.

Source: Path instructions


201-226: Synchronised whole-set reset — correctly paired with the writer's lock.

reset() now synchronises on SETTINGS_LOCK, the same monitor JavaAOPTestCase.setSettingFieldOnLoader acquires via getSettingsLock(). This correctly prevents reset/write interleaving, and the scoping limitation (no cross-field read atomicity) is honestly documented.

.settings/spotbugs-exclude.xml (1)

126-141: 📐 Maintainability & Code Quality | ⚡ Quick win

American spelling in new comment: "synchronizes" → "synchronises".

Otherwise, the exclusion itself is well-justified and correctly scoped: USO_UNSAFE_ACCESSIBLE_OBJECT_SYNCHRONIZATION is a genuine SpotBugs 4.9+ pattern (FindImproperSynchronization detector), and restricting the Match to JavaAOPTestCaseSettings.reset() is appropriate since that's the only place doing a direct synchronized(SETTINGS_LOCK) on a statically-resolvable exposed field — the reflective lock lookups elsewhere in the toolbox classes are opaque to bytecode analysis and wouldn't trigger this pattern anyway.

Source: Path instructions

src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettingsTest.java (3)

194-216: 📐 Maintainability & Code Quality | ⚡ Quick win

American spelling in new Javadoc: "synchronizing" → "synchronising".

As per path instructions, prose must use current British English.

Source: Path instructions


194-216: 📐 Maintainability & Code Quality | ⚡ Quick win

Javadoc overclaims what the test verifies.

The Javadoc states the test verifies "the field ends up in one of the two valid end states (reset to null, or the last writer's value)", but concurrentResetAndSettingWritesDoNotThrowOrCorruptState never reads or asserts on restrictedPackage's final value — it only asserts failed.get() is false. As written, the test only proves the write/reset path doesn't throw; it doesn't actually catch a corrupted intermediate value landing in the field.

♻️ Suggested addition
 		} finally {
 			executor.shutdownNow();
 			JavaAOPTestCaseSettings.reset();
 		}
 
 		Assertions.assertFalse(failed.get(),
 				() -> "Concurrent reset()/setJavaAdviceSettingValue calls must not throw, but: " + firstFailure.get());
+		// Optionally assert the field settled into one of the two valid end states here,
+		// before the final reset() above, to match the stated intent of this test.

217-264: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJAbstractAdviceDefinitions.aj (1)

461-531: I-105 fix correctly tightens boundary matching.

Both the allowed-class comparison (exact match or $-nested boundary at line 500) and the allowed-package comparison (exact match or .-boundary at line 526) now correctly reject sibling classes/packages that merely share a string prefix (e.g. p.TrustedTestEvil vs p.TrustedTest, com.foobar vs com.foo), while still permitting genuine inner classes and sub-packages. Verified against the new fixtures/tests.

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceAbstractToolbox.java (1)

472-549: Instrumentation-side I-105 fix mirrors the AspectJ backend correctly.

Same exact-or-$-boundary and exact-or-.-boundary tightening as the AspectJ counterpart, with an equivalent null-tolerant handling of allowedClasses (inline check vs. the AspectJ side's up-front safeAllowedClasses normalisation — different style, same effect). Parity between backends is maintained.

src/test/java/example/student/AspectJSecurityProbe.java (1)

1-35: LGTM!

src/test/java/p/TrustedTest.java (1)

1-42: LGTM!

src/test/java/example/student/InstrumentationSecurityProbe.java (1)

4-9: New probe helpers correctly mirror the real JDK method signatures.

Verified Files.copy, FileChannel.transferTo/transferFrom, SocketChannel.connect(SocketAddress), and ProcessBuilder.startPipeline descriptor strings against the actual JDK signatures — all match. The read/overwrite-leg split for copy and transferTo/transferFrom correctly models how the two legs are woven independently (per the I-114 fix). checkCallstackCriteria correctly mirrors AspectJSecurityProbe's equivalent for backend parity.

Also applies to: 38-83, 84-105, 129-145

src/test/java/com/foo/AllowedPackageMember.java (1)

1-25: LGTM!

src/test/java/com/foobar/UnrelatedPackageMember.java (1)

1-26: LGTM!

src/test/java/de/tum/cit/ase/ares/api/aop/java/CallstackTrustBoundaryTest.java (1)

1-96: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj (3)

17-17: LGTM!


1278-1281: Correctly gated dispatch — depends on the fix at Lines 656-698.

The early-return here is sound glue code that skips the generic derivation path once the special case reports it fully handled the call; its correctness is entirely contingent on checkCopyOrTransferSpecialCase never claiming "fully handled" without actually validating (see comment on Lines 656-698).


1473-1481: 🔒 Security & Privacy

Drop this warning The execute advice already includes intentional if(false) placeholder pointcuts in JavaAspectJFileSystemPointcutDefinitions.aj, so the adviceDidNotMatch warning is expected rather than a missing guard on a real execute path.

			> Likely an incorrect or invalid review comment.
src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTest.java (1)

3-4: LGTM!

Also applies to: 19-22, 32-33, 683-708

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/fileSystem/read/copy/FilesCopyMain.java (1)

1-36: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/fileSystem/read/copy/copySource.txt (1)

1-2: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/fileSystem/read/mismatch/FilesMismatchMain.java (1)

1-31: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/SystemAccessTest.java (1)

256-307: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml (1)

1-16: LGTM!

Also applies to: 21-22

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml (1)

1-16: LGTM!

Also applies to: 21-22

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemPointcutDefinitions.aj (2)

105-168: LGTM!

Also applies to: 176-178


219-263: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java (1)

725-802: LGTM!

Also applies to: 1637-1640

src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/archunit/methods/file-system-access-methods.txt (1)

143-143: LGTM!

src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/wala/methods/file-system-access-methods.txt (1)

122-122: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml (1)

1-23: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml (1)

1-23: LGTM!

Comment on lines +17 to +20
regardingNetworkConnections: [ ]
regardingCommandExecutions: [ ]
regardingThreadCreations: [ ]
regardingPackageImports: [ ]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

YAMLlint: empty-bracket spacing ([ ][]) in both new policy fixtures. Both new PolicyFilesCopySourceOverwriteOnly.yaml files copy the same boilerplate [ ] empty-list style, which YAMLlint flags as an error in each file.

  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml#L17-L20: replace [ ] with [] on all four lines (regardingNetworkConnections, regardingCommandExecutions, regardingThreadCreations, regardingPackageImports).
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml#L17-L20: apply the identical [ ][] fix.
🧹 Proposed fix (apply to both files)
-    regardingNetworkConnections: [ ]
-    regardingCommandExecutions: [ ]
-    regardingThreadCreations: [ ]
-    regardingPackageImports: [ ]
+    regardingNetworkConnections: []
+    regardingCommandExecutions: []
+    regardingThreadCreations: []
+    regardingPackageImports: []
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
regardingNetworkConnections: [ ]
regardingCommandExecutions: [ ]
regardingThreadCreations: [ ]
regardingPackageImports: [ ]
regardingNetworkConnections: []
regardingCommandExecutions: []
regardingThreadCreations: []
regardingPackageImports: []
🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 17-17: too many spaces inside empty brackets

(brackets)


[error] 18-18: too many spaces inside empty brackets

(brackets)


[error] 19-19: too many spaces inside empty brackets

(brackets)


[error] 20-20: too many spaces inside empty brackets

(brackets)

📍 Affects 2 files
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml#L17-L20 (this comment)
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml#L17-L20
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml`
around lines 17 - 20, Replace the spaced empty-list values with compact []
values for regardingNetworkConnections, regardingCommandExecutions,
regardingThreadCreations, and regardingPackageImports in both
PolicyFilesCopySourceOverwriteOnly.yaml files: aspectj lines 17-20 and
instrumentation lines 17-20.

Source: Linters/SAST tools

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java`:
- Around line 734-738: Expand the channel guards in both transferTo (lines
734-738) and transferFrom (lines 751-755) within
JavaInstrumentationAdviceFileSystemToolbox.java to include DatagramChannel,
preferably by checking the shared NetworkChannel type. Preserve the existing
network-toolbox check and arguments for both paths.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 218a9835-340d-4fdf-a38a-5155617dcb7a

📥 Commits

Reviewing files that changed from the base of the PR and between 8aa1722 and e1cfd03.

📒 Files selected for processing (24)
  • .settings/spotbugs-exclude.xml
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCase.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettings.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/JavaInstrumentationAgent.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
  • src/main/resources/de/tum/cit/ase/ares/api/localization/messages.properties
  • src/main/resources/de/tum/cit/ase/ares/api/localization/messages_de.properties
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettingsTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java
  • src/test/java/de/tum/cit/ase/ares/testutilities/PublicTestNamingConventionTest.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
  • GitHub Check: Build
  • GitHub Check: Analyse Java
🧰 Additional context used
📓 Path-based instructions (5)
**/*

⚙️ CodeRabbit configuration file

Dogmatically check all reviewed files for current British English in prose, comments, JavaDoc, documentation, workflow names, step names, issue/PR text, labels, user-facing messages, and review suggestions. Flag American spellings and grammar such as behavior, color, initialize, authorization, canceled, and program when they are natural-language text. Do not flag programming-language syntax, dependency coordinates, API names, class names, method names, package names, paths, URLs, quoted external identifiers, or other literals where American English is required by the technology.

Files:

  • src/main/resources/de/tum/cit/ase/ares/api/localization/messages_de.properties
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java
  • src/main/resources/de/tum/cit/ase/ares/api/localization/messages.properties
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/JavaInstrumentationAgent.java
  • src/test/java/de/tum/cit/ase/ares/testutilities/PublicTestNamingConventionTest.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettings.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCase.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettingsTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
src/test/java/**/*.java

⚙️ CodeRabbit configuration file

Require tests to distinguish fixture failures from sandbox failures. Network tests must not start in-process listeners inside the sandbox; external fixtures may be skipped when absent, but explicit Ares SecurityException failures must propagate.

Files:

  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java
  • src/test/java/de/tum/cit/ase/ares/testutilities/PublicTestNamingConventionTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettingsTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
src/main/java/**/*.java

⚙️ CodeRabbit configuration file

Review as a Java 17 Maven security framework used to test untrusted student code in Artemis programming exercises. Prioritise sandbox escapes, fail-open behaviour, unsafe reflection, classloader/bootstrap boundary mistakes, global mutable state, concurrency races, insufficient canonicalisation, and changes that weaken file, command, thread, network, package, or class access restrictions. Treat unrecognised security-sensitive inputs as a potential fail-closed requirement. Prefer simple Java code and one field or method declaration per line.

Files:

  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/JavaInstrumentationAgent.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettings.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCase.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
src/main/java/de/tum/cit/ase/ares/api/aop/**/*.java

⚙️ CodeRabbit configuration file

Focus on runtime enforcement integrity: intercepted methods, argument extraction, null handling, recursive advice guards, bootstrap classloader interaction, and whether denied operations can reach the JVM or operating system before checks run.

Files:

  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/JavaInstrumentationAgent.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettings.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCase.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
**/*Test.java

📄 CodeRabbit inference engine (AGENTS.md)

**/*Test.java: A sandboxed test JVM must never spin up its own server (echo server, socket listener, etc.) to test incoming or outgoing connections
Outgoing-connection tests must connect to an external echo server at a configurable endpoint running as a separate process or CI service on the loopback at port 25565, exercising only the student's client behaviour
If the external echo server is not reachable, the test must skip (using JUnit Assumptions.abort) rather than fail
An Ares SecurityException on an explicitly allowed connection is always a real failure and must propagate (never skipped)
Do not hard-code a self-hosted listener as the connection counterpart; use an external echo service to avoid in-JVM BindException/thread/lifecycle flakiness

Files:

  • src/test/java/de/tum/cit/ase/ares/testutilities/PublicTestNamingConventionTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettingsTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
🪛 ast-grep (0.44.1)
src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/JavaInstrumentationAgent.java

[warning] 150-150: Avoid user-generated class names for reflection
Context: Class.forName(className, false, null)
Note: [CWE-470] Use of Externally-Controlled Input to Select Classes or Code ('Unsafe Reflection').

(unsafe-reflection-java)

🔇 Additional comments (24)
src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCase.java (1)

7-8: LGTM!

Also applies to: 245-265

src/main/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettings.java (1)

30-30: LGTM!

Also applies to: 51-54, 183-225

.settings/spotbugs-exclude.xml (1)

133-139: LGTM!

src/main/resources/de/tum/cit/ase/ares/api/localization/messages.properties (1)

57-57: LGTM!

Also applies to: 70-70

src/main/resources/de/tum/cit/ase/ares/api/localization/messages_de.properties (1)

4-4: LGTM!

Also applies to: 64-64

src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPTestCaseSettingsTest.java (1)

8-16: LGTM!

Also applies to: 193-263

src/test/java/de/tum/cit/ase/ares/testutilities/PublicTestNamingConventionTest.java (1)

81-100: LGTM!

src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java (1)

238-365: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessReadTest.java (1)

710-740: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml (1)

17-20: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml (1)

17-20: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java (1)

251-269: LGTM!

Also applies to: 721-731

src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java (1)

324-343: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java (1)

29-33: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/JavaInstrumentationAgent.java (2)

85-91: LGTM!


149-156: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj (1)

222-226: LGTM!

Also applies to: 314-346, 366-384, 429-445, 482-496

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml (1)

1-21: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml (1)

1-21: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml (1)

1-21: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml (1)

1-21: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj (1)

17-17: LGTM!

Also applies to: 627-785, 1284-1287

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml (1)

1-22: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml (1)

1-22: LGTM!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj (1)

341-350: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift

Close the mutable pipeline race in both backends. These helpers keep each ProcessBuilder.command() list by reference, so a pipeline can be validated against one command set and then executed with a different one if a builder is mutated in the gap. Make the check/execution path atomic, or fail closed on mutable pipeline inputs.

  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj#L341-L350
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java#L337-L346
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj`
around lines 341 - 350, Close the mutable pipeline race in
extractPipelineCommands and its corresponding instrumentation-backend helper: do
not retain ProcessBuilder.command() lists by reference across validation and
execution. Make validation and execution atomic, or defensively snapshot and
validate immutable command data so later builder mutations cannot alter the
executed commands. Apply the same fix at the AspectJ site and the
instrumentation site specified in the consolidated locations.

Source: Path instructions

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java (2)

716-731: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Fail-open trap: missing fallback for unrecognised actions in Files.copy.

The Files.copy branch returns true ("fully handled") unconditionally, even when the action is neither "read" nor "overwrite". If Files.copy is ever intercepted with a different action (e.g., if a future pointcut registers it under "create"), this method will silently swallow the call with zero validation, bypassing security checks.

This issue was addressed in the AspectJ backend and in the transferTo/transferFrom branches, but was missed here. As per path instructions, prioritise fail-open behaviour and treat unrecognised security-sensitive inputs as a fail-closed requirement.

🛡️ Proposed fail-closed fix
 				String settingKey = replaceExisting ? "pathsAllowedToBeOverwritten" : "pathsAllowedToBeCreated";
 				checkSinglePathRole(resolvedAction, settingKey, isolateParameter(parameters, 1), true,
 						fileSystemMethodToCheck, studentCalledMethod, fullMethodSignature);
+			} else {
+				throw new SecurityException(localize("security.advice.file.system.unknown.action", action));
 			}
 			return true;
 		}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java`
around lines 716 - 731, Update the Files.copy branch in
JavaInstrumentationAdviceFileSystemToolbox so it returns true only after
handling the recognised read or overwrite actions; for any other action, do not
swallow the call and instead continue through the existing fail-closed path used
by the surrounding transferTo/transferFrom branches. Preserve the current
validation for read and overwrite.

Source: Path instructions


407-423: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

Fail-open sandbox escape: inaccessible FileChannel paths silently bypass security checks.

On modern JDKs with strong encapsulation, calling pathField.setAccessible(true) on internal classes like sun.nio.ch.FileChannelImpl will reliably throw an InaccessibleObjectException unless explicitly opened via JVM arguments. Catching this exception (or NoSuchFieldException when exhausted) and returning null is a fail-open trap. The upstream checkIfPathIsForbidden method considers a null target as implicitly allowed (return false;), which completely disables path validation for these channels.

As per path instructions, prioritise sandbox escapes and treat unrecognised security-sensitive inputs as a fail-closed requirement. Throw a SecurityException instead of returning null when the path cannot be securely extracted.

🛡️ Proposed fail-closed fix
 	`@Nullable`
 	private static Path fileChannelPath(`@Nonnull` FileChannel fileChannel) {
 		Class<?> currentType = fileChannel.getClass();
 		while (currentType != null) {
 			try {
 				Field pathField = currentType.getDeclaredField("path");
 				pathField.setAccessible(true);
 				Object path = pathField.get(fileChannel);
-				return path instanceof String ? Path.of((String) path).normalize().toAbsolutePath() : null;
+				if (path instanceof String) {
+					return Path.of((String) path).normalize().toAbsolutePath();
+				}
+				throw new SecurityException(localize("security.instrumentation.invalid.path", path));
 			} catch (NoSuchFieldException missing) {
 				currentType = currentType.getSuperclass();
 			} catch (IllegalAccessException | InaccessibleObjectException inaccessible) {
-				return null;
+				throw new SecurityException(localize("security.instrumentation.field.not.found", "path", fileChannel.getClass().getName()));
 			}
 		}
-		return null;
+		throw new SecurityException(localize("security.instrumentation.field.not.found", "path", fileChannel.getClass().getName()));
 	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java`
around lines 407 - 423, Update fileChannelPath so every failure to securely
extract the FileChannel path—including NoSuchFieldException after superclass
traversal, IllegalAccessException, and InaccessibleObjectException—throws a
SecurityException instead of returning null; preserve the normalized absolute
Path return when extraction succeeds, ensuring checkIfPathIsForbidden cannot
treat an unrecognized channel as allowed.

Source: Path instructions

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj (1)

684-708: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift

Fail-open sandbox escapes: missing FileChannel path extraction and NetworkChannel guards.

This new handler introduces two critical fail-open vulnerabilities in the AspectJ backend due to missing logic that is present in the instrumentation backend:

  1. Unresolved FileChannel paths: The handler passes a FileChannel instance to checkSinglePathRole, but the AspectJ backend's variableToPath method does not implement FileChannel extraction. As a result, it returns null. The checkIfPathIsForbidden method evaluates a null target as implicitly allowed (return false), completely bypassing path validation for all transferTo and transferFrom operations.
  2. Missing network guards: The instrumentation backend routes transferTo and transferFrom calls targeting a NetworkChannel to the network system interaction checker. This AspectJ handler lacks that guard, allowing untrusted code to stream file contents directly to a network peer without triggering network pointcuts.

As per path instructions, prioritise sandbox escapes and fail-open behaviour. Please port the fileChannelPath extraction (ensuring it fails closed on reflection errors) and the NetworkChannel target/source checks from the instrumentation backend.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj`
around lines 684 - 708, Update the AspectJ backend’s variableToPath method to
extract FileChannel paths via fileChannelPath, and fail closed when reflection
or extraction fails instead of returning an implicitly allowed null target. In
the transferTo and transferFrom handlers, add the instrumentation backend’s
NetworkChannel target/source guards and route those interactions through the
network system checker before applying file-path checks.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/test/java/de/tum/cit/ase/ares/integration/testuser/MavenConfigurationUser.java`:
- Line 27: Update the test method governed by the `@Policy` annotation so the
Maven dependency operation uses a fixture file under the permitted helloWorld
path, or adjust the policy to permit the intended path. Replace the broad catch
(Exception) handling with separate fixture-error handling and explicit
propagation of Ares SecurityException failures, ensuring sandbox denials cannot
be swallowed and cause the test to pass.

In
`@src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java`:
- Around line 28-31: Update NetworkPenguin and its tryStartServer helper to stop
creating or relying on in-process ServerSocket listeners. Use the configurable
external echo service on loopback port 25565, skip tests only when that service
is unavailable, and let explicit Ares SecurityException failures propagate.

---

Outside diff comments:
In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj`:
- Around line 341-350: Close the mutable pipeline race in
extractPipelineCommands and its corresponding instrumentation-backend helper: do
not retain ProcessBuilder.command() lists by reference across validation and
execution. Make validation and execution atomic, or defensively snapshot and
validate immutable command data so later builder mutations cannot alter the
executed commands. Apply the same fix at the AspectJ site and the
instrumentation site specified in the consolidated locations.

In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj`:
- Around line 684-708: Update the AspectJ backend’s variableToPath method to
extract FileChannel paths via fileChannelPath, and fail closed when reflection
or extraction fails instead of returning an implicitly allowed null target. In
the transferTo and transferFrom handlers, add the instrumentation backend’s
NetworkChannel target/source guards and route those interactions through the
network system checker before applying file-path checks.

In
`@src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java`:
- Around line 716-731: Update the Files.copy branch in
JavaInstrumentationAdviceFileSystemToolbox so it returns true only after
handling the recognised read or overwrite actions; for any other action, do not
swallow the call and instead continue through the existing fail-closed path used
by the surrounding transferTo/transferFrom branches. Preserve the current
validation for read and overwrite.
- Around line 407-423: Update fileChannelPath so every failure to securely
extract the FileChannel path—including NoSuchFieldException after superclass
traversal, IllegalAccessException, and InaccessibleObjectException—throws a
SecurityException instead of returning null; preserve the normalized absolute
Path return when extraction succeeds, ensuring checkIfPathIsForbidden cannot
treat an unrecognized channel as allowed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 801550de-fac0-4039-8077-17758bf53217

📥 Commits

Reviewing files that changed from the base of the PR and between e1cfd03 and d2624b8.

📒 Files selected for processing (24)
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPModeTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitionsTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessOverwriteTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/TestHttpClient.java
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/MavenConfigurationUser.java
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
  • GitHub Check: Integration Tests (WALA + AspectJ)
  • GitHub Check: Integration Tests (WALA + instrumentation)
  • GitHub Check: Integration Tests (ArchUnit + instrumentation)
  • GitHub Check: Integration Tests (ArchUnit + AspectJ)
  • GitHub Check: Static Code Analysis
  • GitHub Check: Unit Tests
  • GitHub Check: Analyse Java
🧰 Additional context used
📓 Path-based instructions (5)
**/*

⚙️ CodeRabbit configuration file

Dogmatically check all reviewed files for current British English in prose, comments, JavaDoc, documentation, workflow names, step names, issue/PR text, labels, user-facing messages, and review suggestions. Flag American spellings and grammar such as behavior, color, initialize, authorization, canceled, and program when they are natural-language text. Do not flag programming-language syntax, dependency coordinates, API names, class names, method names, package names, paths, URLs, quoted external identifiers, or other literals where American English is required by the technology.

Files:

  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitionsTest.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolboxTest.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/MavenConfigurationUser.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessOverwriteTest.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/TestHttpClient.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitions.aj
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPModeTest.java
src/test/java/**/*.java

⚙️ CodeRabbit configuration file

Require tests to distinguish fixture failures from sandbox failures. Network tests must not start in-process listeners inside the sandbox; external fixtures may be skipped when absent, but explicit Ares SecurityException failures must propagate.

Files:

  • src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitionsTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/MavenConfigurationUser.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessOverwriteTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/TestHttpClient.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPModeTest.java
**/*Test.java

📄 CodeRabbit inference engine (AGENTS.md)

**/*Test.java: A sandboxed test JVM must never spin up its own server (echo server, socket listener, etc.) to test incoming or outgoing connections
Outgoing-connection tests must connect to an external echo server at a configurable endpoint running as a separate process or CI service on the loopback at port 25565, exercising only the student's client behaviour
If the external echo server is not reachable, the test must skip (using JUnit Assumptions.abort) rather than fail
An Ares SecurityException on an explicitly allowed connection is always a real failure and must propagate (never skipped)
Do not hard-code a self-hosted listener as the connection counterpart; use an external echo service to avoid in-JVM BindException/thread/lifecycle flakiness

Files:

  • src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitionsTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessOverwriteTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java
  • src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPModeTest.java
src/main/java/**/*.java

⚙️ CodeRabbit configuration file

Review as a Java 17 Maven security framework used to test untrusted student code in Artemis programming exercises. Prioritise sandbox escapes, fail-open behaviour, unsafe reflection, classloader/bootstrap boundary mistakes, global mutable state, concurrency races, insufficient canonicalisation, and changes that weaken file, command, thread, network, package, or class access restrictions. Treat unrecognised security-sensitive inputs as a potential fail-closed requirement. Prefer simple Java code and one field or method declaration per line.

Files:

  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
src/main/java/de/tum/cit/ase/ares/api/aop/**/*.java

⚙️ CodeRabbit configuration file

Focus on runtime enforcement integrity: intercepted methods, argument extraction, null handling, recursive advice guards, bootstrap classloader interaction, and whether denied operations can reach the JVM or operating system before checks run.

Files:

  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolbox.java
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java
🔇 Additional comments (21)
src/test/java/de/tum/cit/ase/ares/api/aop/java/JavaAOPModeTest.java (1)

3-23: LGTM!

Also applies to: 38-57, 58-74, 76-102, 104-126, 128-142

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj (2)

133-146: LGTM!

Also applies to: 154-187


230-237: 🔒 Security & Privacy

Ensure trusted collection checks cover delegated backing collections. Both backends trust the outer JDK List before invoking traversal methods, but JDK wrappers can delegate to student-controlled backing collections. Verify that requireTrustedRuntimeType rejects such wrappers or that traversal uses a trusted, non-delegating representation.

  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj#L230-L237: protect List conversion and toArray().
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj#L341-L350: protect pipeline traversal and command().
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj#L374-L380: protect pipeline detection.
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj#L437-L442: protect violation-path traversal.
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJCommandSystemAdviceDefinitions.aj#L490-L493: protect executable-path traversal.
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java#L226-L233: protect List conversion and toArray().
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java#L337-L346: protect pipeline traversal and command().
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java#L369-L377: protect pipeline detection.
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java#L433-L440: protect violation-path traversal.
  • src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java#L487-L492: protect executable-path traversal.

Source: Path instructions

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolbox.java (1)

129-142: LGTM!

Also applies to: 149-183

src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceCommandSystemToolboxTest.java (1)

3-9: LGTM!

Also applies to: 21-37, 119-162

src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceFileSystemToolboxTest.java (1)

255-441: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/FileSystemAccessOverwriteTest.java (1)

1841-1892: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml (1)

1-29: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml (1)

1-29: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyFilesCopySourceOverwriteOnly.yaml (1)

1-29: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyFilesCopySourceOverwriteOnly.yaml (1)

1-29: LGTM!

src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitions.aj (1)

12-17: LGTM!

Also applies to: 177-183, 207-210, 221-229, 632-676, 909-909

src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolbox.java (1)

13-13: LGTM!

Also applies to: 215-222, 247-250, 261-275, 701-741, 916-926, 966-966

src/test/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceNetworkSystemToolboxTest.java (1)

11-26: LGTM!

Also applies to: 309-322, 324-337, 339-359, 361-378, 380-402, 404-411

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/HttpClientSendMain.java (1)

1-32: LGTM!

src/test/java/de/tum/cit/ase/ares/integration/aop/forbidden/subject/networkSystem/send/httpclient/TestHttpClient.java (1)

1-92: LGTM!

src/test/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJNetworkSystemAdviceDefinitionsTest.java (1)

1-23: LGTM!

Also applies to: 25-37, 39-55

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml (1)

1-24: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml (1)

1-24: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/aspectj/PolicyHttpSendAllowedReceiveDenied.yaml (1)

1-24: LGTM!

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/wala/instrumentation/PolicyHttpSendAllowedReceiveDenied.yaml (1)

1-24: LGTM!

private static final String POM_XML_PATH = "src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/example/build/tools/pom.xml";

@PublicTest
@Policy(withinPath = "test-classes/de/tum/cit/ase/ares/integration/testuser/subject/helloWorld")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not let this policy failure be swallowed.

Line 27 permits only test-classes/.../helloWorld, but the method creates a system-temporary file and passes its absolute path to DependencyManager.addDependenciesAndPluginsForMaven. If that is the intercepted operation, the path is outside the policy; the broad catch (Exception) then logs the resulting denial and allows the test to pass. Use a file under the permitted path or correct the policy, and rethrow Ares security failures while handling fixture errors separately.

As per path instructions, tests must distinguish fixture failures from sandbox failures and explicit Ares SecurityException failures must propagate.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/de/tum/cit/ase/ares/integration/testuser/MavenConfigurationUser.java`
at line 27, Update the test method governed by the `@Policy` annotation so the
Maven dependency operation uses a fixture file under the permitted helloWorld
path, or adjust the policy to permit the intended path. Replace the broad catch
(Exception) handling with separate fixture-error handling and explicit
propagation of Ares SecurityException failures, ensuring sandbox denials cannot
be swallowed and cause the test to pass.

Source: Path instructions

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java`:
- Line 13: Update the Scanner construction in NetworkPenguin to use an explicit
charset, preferably StandardCharsets.UTF_8, when reading from the socket input
stream; leave the socket connection and resource handling unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 51eccb3c-d9c7-4617-9e63-f162ad349b44

📥 Commits

Reviewing files that changed from the base of the PR and between d2624b8 and e73e914.

📒 Files selected for processing (5)
  • src/test/java/de/tum/cit/ase/ares/integration/NetworkTest.java
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/MavenConfigurationUser.java
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/NetworkUser.java
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java
  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyMavenConfigurationUser.yaml
💤 Files with no reviewable changes (2)
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/NetworkUser.java
  • src/test/java/de/tum/cit/ase/ares/integration/NetworkTest.java
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
  • GitHub Check: Static Code Analysis
  • GitHub Check: Integration Tests (ArchUnit + instrumentation)
  • GitHub Check: Integration Tests (WALA + AspectJ)
  • GitHub Check: Integration Tests (WALA + instrumentation)
  • GitHub Check: Integration Tests (ArchUnit + AspectJ)
  • GitHub Check: Analyse Java
🧰 Additional context used
📓 Path-based instructions (2)
**/*

⚙️ CodeRabbit configuration file

Dogmatically check all reviewed files for current British English in prose, comments, JavaDoc, documentation, workflow names, step names, issue/PR text, labels, user-facing messages, and review suggestions. Flag American spellings and grammar such as behavior, color, initialize, authorization, canceled, and program when they are natural-language text. Do not flag programming-language syntax, dependency coordinates, API names, class names, method names, package names, paths, URLs, quoted external identifiers, or other literals where American English is required by the technology.

Files:

  • src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyMavenConfigurationUser.yaml
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/MavenConfigurationUser.java
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java
src/test/java/**/*.java

⚙️ CodeRabbit configuration file

Require tests to distinguish fixture failures from sandbox failures. Network tests must not start in-process listeners inside the sandbox; external fixtures may be skipped when absent, but explicit Ares SecurityException failures must propagate.

Files:

  • src/test/java/de/tum/cit/ase/ares/integration/testuser/MavenConfigurationUser.java
  • src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/architectureTests/network/NetworkPenguin.java
🔇 Additional comments (2)
src/test/java/de/tum/cit/ase/ares/integration/testuser/MavenConfigurationUser.java (1)

4-5: LGTM!

Also applies to: 19-31

src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/java/maven/archunit/aspectj/PolicyMavenConfigurationUser.yaml (1)

1-26: LGTM!

@MarkusPaulsen
MarkusPaulsen merged commit 640a976 into main Jul 20, 2026
10 of 11 checks passed
@MarkusPaulsen
MarkusPaulsen deleted the fix/aop/aop-aspectj-instrumentation-audit-2026 branch July 20, 2026 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aop Automated area label: aop docs Automated area label: docs other Automated area label: other policy Automated area label: policy tests Automated area label: tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants