AOP: Fix errors for Instrumentation regarding Pointcuts
#30
Conversation
…Unit properties for test cases
WalkthroughThe changes in this pull request involve modifications to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…ityException and caused false positives 2. Added Thread creation tests for ArchUnit 3. Improved CustomClassResolver for efficiency 4. Improved pointcuts for the Instrumentation
…ore/fix-println-error
|
To fix:
Class def not found:
|
…scuss further with Markus 2. Fix Def not found error 3. Remaining failing tests are due to missing pointcuts
sarpsahinalp
changed the title
AOP: Fix errors for Instrumentation regarding Pointcuts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range and nitpick comments (13)
src/test/resources/archunit.properties (1)
1-3: New ArchUnit configuration file added.This new configuration file introduces two important settings for ArchUnit:
resolveMissingDependenciesFromClassPath=false: This setting instructs ArchUnit to ignore missing dependencies in the classpath, as they are resolved manually by theCustomClassResolverclass. This is well-documented with a comment.
archRule.failOnEmptyShould=false: This setting likely prevents ArchUnit rules from failing when encountering empty conditions. However, the purpose and impact of this setting are not documented.These configurations may significantly affect how architecture tests behave in the project. Ensure that all team members are aware of these changes and understand their implications.
Consider adding a comment for the
archRule.failOnEmptyShouldproperty, similar to the one forresolveMissingDependenciesFromClassPath, to maintain consistency and clarity in the configuration file.src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/FileHandlerConstants.java (1)
17-17: LGTM! Consider grouping related constants.The addition of
JAVA_THREAD_CREATION_METHODSis consistent with the existing structure and naming conventions. It expands the set of predefined paths for method categories, which aligns with the class's purpose.Consider grouping related constants together for better organization. For example, you could group thread-related constants near other runtime or JVM-related constants like
JAVA_JVM_TERMINATION_METHODS.src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/postcompile/CustomClassResolver.java (1)
33-35: Add a comment explaining the special case.The new condition bypasses the resolution process for a specific class. While this might be intentional, it's not clear why this particular class requires special handling. Adding a comment to explain the rationale behind this exception would improve code maintainability.
Consider extracting the long class name as a constant.
The class name
"de.tum.cit.ase.ares.api.aop.java.aspectj.adviceandpointcut.JavaAspectJFileSystemAdviceDefinitions"is quite long. Extracting it as a private static final constant would improve readability and make it easier to update if needed.Here's a suggested refactor:
+ private static final String BYPASS_RESOLUTION_CLASS = "de.tum.cit.ase.ares.api.aop.java.aspectj.adviceandpointcut.JavaAspectJFileSystemAdviceDefinitions"; public static Optional<JavaClass> tryResolve(String typeName) { - if (typeName.startsWith("de.tum.cit.ase.ares.api.aop.java.aspectj.adviceandpointcut.JavaAspectJFileSystemAdviceDefinitions")) { + if (typeName.startsWith(BYPASS_RESOLUTION_CLASS)) { return Optional.empty(); }LGTM: Special case handling added.
The addition of this special case handling looks good, assuming it's intentional. It effectively bypasses the resolution process for the specified class without affecting the existing logic for other classes.
src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/JavaArchUnitSecurityTestCase.java (1)
92-92: Approved: Thread creation check implemented correctly.The implementation of the
THREAD_CREATIONcase has been updated to perform an actual check usingJavaArchitectureTestCaseCollection.NO_CLASSES_SHOULD_CREATE_THREADS.check(classes). This change aligns with the behavior of other cases and improves the overall functionality of the method.For consistency with other cases, consider moving the method call to a new line:
- JavaArchitectureTestCaseCollection.NO_CLASSES_SHOULD_CREATE_THREADS.check(classes); + JavaArchitectureTestCaseCollection + .NO_CLASSES_SHOULD_CREATE_THREADS + .check(classes);This formatting would make it easier to read and maintain, especially if additional configuration is needed in the future.
src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/postcompile/JavaArchitectureTestCaseCollection.java (1)
149-155: LGTM! Consider adding a brief comment explaining the rule's purpose.The new rule
NO_CLASSES_SHOULD_CREATE_THREADSis well-implemented and consistent with the existing architecture rules. It addresses potential security concerns related to thread creation, which aligns with the overall purpose of this class.Consider adding a brief comment above the rule to explain its purpose and potential security implications, similar to the comments for other rules in this file. For example:
/** * This method checks if any class in the given package creates threads, which could lead to unexpected behavior or security vulnerabilities. */ public static final ArchRule NO_CLASSES_SHOULD_CREATE_THREADS = createNoClassShouldHaveMethodRule( "creates threads", FileHandlerConstants.JAVA_THREAD_CREATION_METHODS );This addition would improve the documentation and make the purpose of the rule clearer to other developers.
src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/postcompile/TransitivelyAccessesMethodsCondition.java (1)
133-134: Approve the added comment with a minor suggestion.The added comment provides valuable insight into the implementation details, explaining the use of Class Hierarchy Analysis (CHA). This information is crucial for understanding the method's behavior and limitations.
Consider expanding the comment slightly to mention potential performance implications of using CHA, especially when dealing with large class hierarchies. This could help future maintainers understand any scalability concerns.
src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java (3)
164-166: Approve addition ofjava.nio.file.Filesmethods.The addition of
java.nio.file.Fileswith various write-related methods is appropriate and improves coverage of file write operations using modern Java NIO APIs.Consider adding the
movemethod to the list, as it can also result in file overwrites when the target file already exists.
194-196: Approve addition ofjava.nio.file.Files.delete.The addition of
java.nio.file.Files.deleteis appropriate and improves coverage of file delete operations using modern Java NIO APIs.Consider adding the
deleteIfExistsmethod to the list forjava.nio.file.Files, as it's a commonly used variant for file deletion.
Line range hint
1-201: Overall improvements with minor suggestions.The changes in this file generally improve the coverage of file operations, especially for modern Java NIO APIs. However, there are a few points to consider:
- The addition of
java.lang.ObjecttomethodsWhichCanReadFilesneeds reconsideration due to its broad scope.- Consider adding
moveto the write operations,deleteIfExiststo the delete operations, and verifying the impact of removingcheckAccessfrom some file system classes.These changes will enhance the precision and coverage of the instrumentation pointcuts.
To further improve the maintainability and flexibility of this class, consider:
- Using an enum or constants for the operation types (READ, WRITE, EXECUTE, DELETE) instead of separate maps.
- Implementing a builder pattern or factory method to construct these maps, allowing for easier updates and testing.
- Adding unit tests to verify the correctness and completeness of these definitions.
src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/archunit/methods/file-system-access-methods.txt (2)
46-46: Minor typographical issue: Missing space after period.There's a missing space after the period in the following line:
java.nio.channels.FileChannel java.nio.file.Files java.security.KeyStore$Builder.newInsta...Consider adding a space after "Files" to improve readability.
🧰 Tools
🪛 LanguageTool
[typographical] ~46-~46: Il manque une espace après le point.
Context: ...a.nio.channels.FileChannel java.nio.file.Files java.security.KeyStore$Builder.newInsta...(ESPACE_APRES_POINT)
Line range hint
1-238: Overall structure and organization of the file.The file maintains a clear and organized structure, listing various Java methods related to file system access. The methods are grouped by their respective classes, which aids in readability and makes it easier to locate specific methods.
However, there are a few suggestions for improvement:
Consider adding comments or section headers to group related classes or functionalities. This would enhance the overall organization and make it easier to navigate the file.
Some entries, particularly those from internal Sun classes (e.g.,
sun.awt,sun.nio.fs), might not be necessary for general file system access monitoring. Consider reviewing these entries and removing any that are not essential for your specific use case.The file includes methods from various Java versions. It might be helpful to add version information or organize methods based on the Java version they were introduced in, to assist users working with different Java versions.
Would you like assistance in implementing these suggestions to improve the file's structure and organization?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: Une espace doit être ajoutée entre ces deux mots ou intégrez ce nouveau mot à votre dictionnaire personnel.
Context: ...a.net.http.HttpResponse$BodySubscribers.ofFile(java.nio.file.Path) java.net.http.HttpR...(MOTS_COLLES)
[typographical] ~43-~43: Il manque une espace après le point.
Context: ...nse$BodySubscribers.ofFile(java.nio.file.Path) java.net.http.HttpResponse$BodySubscri...(ESPACE_APRES_POINT)
[uncategorized] ~44-~44: Une espace doit être ajoutée entre ces deux mots ou intégrez ce nouveau mot à votre dictionnaire personnel.
Context: ...a.net.http.HttpResponse$BodySubscribers.ofFile(java.nio.file.Path, [Ljava.nio.file.Ope...(MOTS_COLLES)
[typographical] ~44-~44: Pas de correspondance fermante ou ouvrante pour le caractère « ) »
Context: ...http.HttpResponse$BodySubscribers.ofFile(java.nio.file.Path, [Ljava.nio.file.Open...(UNPAIRED_BRACKETS)
[typographical] ~44-~44: Il manque une espace après le point.
Context: ...nse$BodySubscribers.ofFile(java.nio.file.Path, [Ljava.nio.file.OpenOption;) java.nio....(ESPACE_APRES_POINT)
[typographical] ~44-~44: Pas de correspondance fermante ou ouvrante pour le caractère « ] »
Context: ...ySubscribers.ofFile(java.nio.file.Path, [Ljava.nio.file.OpenOption;) java.nio.cha...(UNPAIRED_BRACKETS)
[typographical] ~44-~44: Une parenthèse fermante semble être requise pour clore l’incise présente dans cette phrase.
Context: ...File(java.nio.file.Path, [Ljava.nio.file.OpenOption;) java.nio.channels.FileChann...(PARENTHESES)
[typographical] ~46-~46: Il manque une espace après le point.
Context: ...a.nio.channels.FileChannel java.nio.file.Files java.security.KeyStore$Builder.newInsta...(ESPACE_APRES_POINT)
[uncategorized] ~47-~47: Une espace doit être ajoutée entre ces deux mots ou intégrez ce nouveau mot à votre dictionnaire personnel.
Context: ...le.Files java.security.KeyStore$Builder.newInstance(java.io.File, java.security.KeyStore$Pr...(MOTS_COLLES)
[typographical] ~47-~47: Une parenthèse fermante est nécessaire.
Context: ...va.security.KeyStore$Builder.newInstance(java.io.File, java.security.KeyStore$Protect...(PARENTHESES)
[typographical] ~47-~47: Il manque une espace après le point.
Context: ...ity.KeyStore$Builder.newInstance(java.io.File, java.security.KeyStore$ProtectionParam...(ESPACE_APRES_POINT)
[uncategorized] ~48-~48: Une espace doit être ajoutée entre ces deux mots ou intégrez ce nouveau mot à votre dictionnaire personnel.
Context: ...rameter) java.security.KeyStore$Builder.newInstance(java.lang.String, java.security.Provide...(MOTS_COLLES)
[typographical] ~48-~48: Une parenthèse fermante est nécessaire.
Context: ...va.security.KeyStore$Builder.newInstance(java.lang.String, java.security.Provider, ja...(PARENTHESES)
[typographical] ~48-~48: Il manque une espace après le point.
Context: ....String, java.security.Provider, java.io.File, java.security.KeyStore$ProtectionParam...(ESPACE_APRES_POINT)
[uncategorized] ~49-~49: Une espace doit être ajoutée entre ces deux mots ou intégrez ce nouveau mot à votre dictionnaire personnel.
Context: ...ectionParameter) java.security.KeyStore.getInstance(java.io.File, [C) java.security.KeyStor...(MOTS_COLLES)
[typographical] ~49-~49: Pas de correspondance fermante ou ouvrante pour le caractère « ) »
Context: ...eter) java.security.KeyStore.getInstance(java.io.File, [C) java.security.KeyStore...(UNPAIRED_BRACKETS)
[typographical] ~49-~49: Il manque une espace après le point.
Context: ...va.security.KeyStore.getInstance(java.io.File, [C) java.security.KeyStore.getInstance...(ESPACE_APRES_POINT)
[typographical] ~49-~49: Pas de correspondance fermante ou ouvrante pour le caractère « ] »
Context: ...rity.KeyStore.getInstance(java.io.File, [C) java.security.KeyStore.getInstance(ja...(UNPAIRED_BRACKETS)
src/main/java/de/tum/cit/ase/ares/api/jupiter/JupiterSecurityExtension.java (1)
Line range hint
49-55: Remove the emptyfinallyblock to clean up the code.Since the uninstallation of
ArtemisSecurityManagerhas been removed, thefinallyblock containing an emptytryblock is unnecessary and can be safely removed to improve code clarity.Apply this diff to remove the unnecessary
finallyblock:} catch (Throwable t) { failure = t; } finally { - try { - //REMOVED: Uninstallation of ArtemisSecurityManager - } catch (Exception e) { - if (failure == null) - failure = e; - else - failure.addSuppressed(e); - } }src/test/java/de/tum/cit/ase/ares/integration/FileSystemAccessTest.java (1)
Line range hint
958-961: Missingthrows IOExceptionin method signatureThe method
test_accessFileSystemViaJFileChooserInstrumentation()does not declarethrows IOExceptionlike its AspectJ counterpart at line 951~. IfFileSystemAccessPenguin.accessFileSystemViaJFileChooser()can throwIOException, consider addingthrows IOExceptionto the method signature for consistency and proper exception handling.Suggested fix:
- void test_accessFileSystemViaJFileChooserInstrumentation() { + void test_accessFileSystemViaJFileChooserInstrumentation() throws IOException {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceToolbox.java (2 hunks)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java (4 hunks)
- src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/FileHandlerConstants.java (1 hunks)
- src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/JavaArchUnitSecurityTestCase.java (1 hunks)
- src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/postcompile/CustomClassResolver.java (1 hunks)
- src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/postcompile/JavaArchitectureTestCaseCollection.java (1 hunks)
- src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/postcompile/TransitivelyAccessesMethodsCondition.java (2 hunks)
- src/main/java/de/tum/cit/ase/ares/api/jupiter/JupiterSecurityExtension.java (1 hunks)
- src/main/java/de/tum/cit/ase/ares/api/securitytest/java/JavaSecurityTestCaseFactoryAndBuilder.java (0 hunks)
- src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/archunit/methods/file-system-access-methods.txt (1 hunks)
- src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/archunit/methods/thread-creation-methods.txt (1 hunks)
- src/test/java/de/tum/cit/ase/ares/integration/FileSystemAccessTest.java (34 hunks)
- src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/fileSystem/FileSystemAccessPenguin.java (2 hunks)
- src/test/resources/archunit.properties (1 hunks)
- src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/EverythingForbiddenPolicy.yaml (1 hunks)
- src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/OnePathAllowedInstrumentationDelete.yaml (1 hunks)
- src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/OnePathAllowedInstrumentationExecute.yaml (1 hunks)
- src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/OnePathAllowedInstrumentationRead.yaml (1 hunks)
- src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/OnePathAllowedInstrumentationWrite.yaml (1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/ase/ares/api/securitytest/java/JavaSecurityTestCaseFactoryAndBuilder.java
🧰 Additional context used
🪛 LanguageTool
src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/archunit/methods/file-system-access-methods.txt
[typographical] ~46-~46: Il manque une espace après le point.
Context: ...a.nio.channels.FileChannel java.nio.file.Files java.security.KeyStore$Builder.newInsta...(ESPACE_APRES_POINT)
🔇 Additional comments (23)
src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/archunit/methods/thread-creation-methods.txt (2)
1-2: LGTM: Correct thread-related methods includedThe inclusion of
java.lang.Thread.startandjava.lang.Thread.runis correct and appropriate for monitoring thread creation and execution in ArchUnit tests.
3-3: Verify theunstartedmethodThe method
java.lang.Thread.unstartedis not a standard method in the Java Thread class. This might be a typo or a custom method. Please verify if this is intentional or if it should be removed/replaced.If this is not intentional, consider removing this line or replacing it with another relevant thread-related method, such as
java.lang.Thread.interrupt().To verify the usage of this method in the codebase, you can run the following script:
src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/EverythingForbiddenPolicy.yaml (1)
19-19: LGTM! Verify the necessity of this import.The addition of the new package import
de.tum.cit.ase.ares.integration.testuser.subject.thirdpartypackageis syntactically correct and follows the existing pattern in the file. This change aligns with the PR objectives of fixing errors related to Instrumentation and Pointcuts.To ensure this change is necessary and doesn't introduce any security vulnerabilities, please run the following verification script:
This script will help verify the necessity and security implications of adding this new package import.
src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/FileHandlerConstants.java (1)
17-17: Verify usage of the new constant in the codebase.The addition of
JAVA_THREAD_CREATION_METHODSenhances the functionality for architecture testing related to thread creation. To ensure its proper integration:
- Verify that the "thread-creation-methods.txt" file exists in the specified resource directory.
- Check if there are any new or updated test cases that utilize this constant.
- Ensure that any existing architecture rules are updated to include thread creation checks if necessary.
Run the following script to verify the existence of the resource file and potential usage of the new constant:
✅ Verification successful
Verification Successful:
JAVA_THREAD_CREATION_METHODSis properly integrated.
- The
thread-creation-methods.txtfile exists in the specified resource directory.- The constant
JAVA_THREAD_CREATION_METHODSis utilized inJavaArchitectureTestCaseCollection.javafor defining architecture rules related to thread creation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the thread-creation-methods.txt file and check for potential usage of JAVA_THREAD_CREATION_METHODS # Test 1: Check if the resource file exists echo "Checking for thread-creation-methods.txt:" fd --type f "thread-creation-methods.txt" --exec echo "Found: {}" # Test 2: Search for potential usage of JAVA_THREAD_CREATION_METHODS in Java files echo -e "\nChecking for usage of JAVA_THREAD_CREATION_METHODS:" rg --type java "JAVA_THREAD_CREATION_METHODS" -C 3Length of output: 3150
src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/OnePathAllowedInstrumentationDelete.yaml (1)
17-17: Clarify the inclusion ofsun.awt.X11FontManageras a test classThe addition of
sun.awt.X11FontManagerto the list of test classes is unusual and potentially problematic:
- This class is part of the Java Runtime Environment (JRE) and not typically a user-defined test class.
- Including it as a test class may lead to unexpected behavior in security policy enforcement.
Could you please clarify the rationale behind this addition? If this is intended as a workaround for a specific issue (possibly related to the instrumentation errors mentioned in the PR objectives), consider exploring alternative approaches that don't involve treating JRE classes as test classes.
To better understand the context, let's check for any references to this class in the codebase:
This will help determine if and where this class is being used, which might provide insights into why it's being added as a test class.
src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/OnePathAllowedInstrumentationExecute.yaml (1)
17-17: Verify the necessity of addingsun.awt.X11FontManageras a test classThe addition of
sun.awt.X11FontManagerto the list of test classes is unusual, as this is a JRE class rather than a typical user-defined test class. While this might be related to resolving font management issues during testing in a Unix/Linux environment, it's important to consider the following:
- Is this addition intentional and necessary for your specific testing scenario?
- What is the rationale behind treating a JRE class as a test class?
- Have you considered the potential impact on security policy enforcement by including this class?
If this addition is indeed necessary, please document the reason for including this JRE class in your test classes. Otherwise, consider removing it to maintain the integrity of your security policies.
To verify the usage of this class in your tests, you can run the following command:
This will help determine if and where the
X11FontManageris being used in your test suite, justifying its inclusion in the test classes list.✅ Verification successful
Further verify the usage of
sun.awt.X11FontManageracross the entire codebaseThe initial search in test Java files did not yield any results. To ensure comprehensive verification, please execute the following command to search for any references to
X11FontManagerthroughout the entire codebase:
No issues found with adding
sun.awt.X11FontManagerto security policiesThe inclusion of
sun.awt.X11FontManagerin your security policies YAML files is appropriate for defining permitted resource accesses during integration testing. Since there are no references to this class in your test Java files, its addition solely pertains to security policy enforcement and does not impact your test classes directly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of X11FontManager in test files rg --type java -g '*Test.java' 'X11FontManager'Length of output: 47
Script:
#!/bin/bash # Search for usage of X11FontManager in the entire codebase rg 'X11FontManager'Length of output: 616
src/test/resources/de/tum/cit/ase/ares/integration/testuser/securitypolicies/OnePathAllowedInstrumentationWrite.yaml (1)
17-17: Clarification needed: Addition of JRE class to test classes listThe addition of
sun.awt.X11FontManagerto the list of test classes is unusual and potentially concerning. This is a JRE class, not a typical test class from your project. Including it in this list might affect security policies and instrumentation in unexpected ways.Could you please clarify:
- Why is this JRE class being added to the test classes list?
- What specific problem or requirement does this addition address?
- Have you considered the potential security implications of treating this class as a test class?
To help understand the usage of this class in the project, let's search for its occurrences:
This will help us understand if and how this class is being used in the project, which might provide context for its inclusion in the test classes list.
src/main/java/de/tum/cit/ase/ares/api/architecture/java/archunit/postcompile/TransitivelyAccessesMethodsCondition.java (1)
167-167: Please clarify the rationale for simplifying the access filtering condition.The condition for filtering accesses has been simplified by removing the
isExceptionOrErrorcheck on the target owner. This change could potentially broaden the scope of accesses considered, including those to exception or error classes that were previously filtered out.Could you please explain:
- The reasoning behind this change?
- Any potential impacts on the method's behavior and its callers?
- Whether this change aligns with the intended functionality of the
getAccessesFromClassmethod?To help verify the impact of this change, please run the following script:
This will help us understand the usage context and potential new inclusions due to the changed filtering logic.
✅ Verification successful
Simplification Verified Successfully
The removal of the
isExceptionOrErrorcheck in the access filtering condition does not introduce any new accesses to exception or error classes based on the executed verification scripts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of getAccessesFromClass and analyze potential impacts # Search for usages of getAccessesFromClass echo "Usages of getAccessesFromClass:" rg --type java "getAccessesFromClass" -C 3 # Check for any Exception or Error related accesses that might now be included echo "\nPotential new Exception or Error related accesses:" rg --type java "getAccessesFromClass.*Exception|getAccessesFromClass.*Error" -C 3Length of output: 4443
Script:
#!/bin/bash # Description: Enhanced search for usages of getAccessesFromClass with Exception or Error classes # Search for usages of getAccessesFromClass echo "Usages of getAccessesFromClass:" rg --type java "getAccessesFromClass" -C 3 # Improved search for Exception or Error related accesses by looking for class names ending with Exception or Error as arguments echo "\nPotential new Exception or Error related accesses:" rg --type java 'getAccessesFromClass\s*\(\s*.*(Exception|Error)\b.*\)' -C 3Length of output: 4437
src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java (1)
177-183: Approve changes to execution-related methods.The removal of
checkAccessfromjava.io.UnixFileSystemandjava.io.WinNTFileSystemmay help reduce false positives in execution detection. The addition ofsetPosixFilePermissionsforjava.nio.file.Filesis appropriate for monitoring changes to file permissions.Verify that removing
checkAccessdoesn't miss any critical execution attempts:src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceToolbox.java (2)
229-229: Approve change from "write" to "overwrite", but verify impact.The modification from "write" to "overwrite" in the switch statement aligns with the PR objectives of fixing errors related to instrumentation and pointcuts. This change suggests a more specific categorization of file system actions.
- The change looks good and likely addresses some of the issues mentioned in the PR description.
- Please verify that this change doesn't negatively impact the test cases mentioned in the PR comments, particularly:
test_accessFileSystemViaFileWriteInstrumentationtest_accessFileSystemViaFileChannelWriteInstrumentation- Ensure that this change is consistent with other parts of the codebase that deal with file system operations.
- Consider updating any relevant documentation to reflect this new distinction between "write" and "overwrite" operations.
Let's check for other occurrences of "write" or "overwrite" in relation to file system operations:
#!/bin/bash # Search for "write" or "overwrite" in relation to file system operations rg "(write|overwrite).*(file|path|directory)" --type java
Line range hint
1-248: Summary of changes and next stepsThe changes in this file align with the PR objectives of fixing errors related to instrumentation and pointcuts. Here's a summary of the review:
A TODO comment has been added, indicating a potential issue with method access permissions for file system operations. This needs clarification and possibly tracking as a separate issue if out of scope for this PR.
The change from "write" to "overwrite" in the
checkFileSystemInteractionmethod has been approved, but requires verification of its impact on related test cases and other parts of the codebase.Next steps:
- Provide clarification on the TODO comment and decide whether to address it in this PR or create a separate issue.
- Verify the impact of the "write" to "overwrite" change on the mentioned test cases and overall file system operation handling.
- Update any relevant documentation to reflect the new distinction between "write" and "overwrite" operations.
- Ensure all changes are consistent with the codebase and don't introduce unintended side effects.
To get a broader view of the changes related to file system operations in this PR, let's check for modified files that might be affected:
#!/bin/bash # List modified files in the PR that might be related to file system operations gh pr view 30 --json files -q '.files[].path' | grep -E '(file|path|directory|instrumentation|advice)'src/main/resources/de/tum/cit/ase/ares/api/templates/architecture/java/archunit/methods/file-system-access-methods.txt (1)
Line range hint
1-46: Newly added methods enhance file system access capabilities.The additions to this file, particularly the methods from
java.nio.file.Filesandjava.awt.Desktopclasses, significantly improve the coverage of file system access methods. These additions are valuable for monitoring and controlling file system operations in Java applications.For example:
java.nio.file.Files.readAllBytes(java.nio.file.Path)andjava.nio.file.Files.write(java.nio.file.Path, byte[], java.nio.file.OpenOption...)provide modern, efficient ways to read from and write to files.java.awt.Desktop.browse(java.net.URI)andjava.awt.Desktop.open(java.io.File)allow for better integration with the desktop environment, which can be useful for certain types of applications.These additions will help in creating more comprehensive rules for monitoring file system access in Java applications.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: Une espace doit être ajoutée entre ces deux mots ou intégrez ce nouveau mot à votre dictionnaire personnel.
Context: ...a.net.http.HttpResponse$BodySubscribers.ofFile(java.nio.file.Path) java.net.http.HttpR...(MOTS_COLLES)
[typographical] ~43-~43: Il manque une espace après le point.
Context: ...nse$BodySubscribers.ofFile(java.nio.file.Path) java.net.http.HttpResponse$BodySubscri...(ESPACE_APRES_POINT)
[uncategorized] ~44-~44: Une espace doit être ajoutée entre ces deux mots ou intégrez ce nouveau mot à votre dictionnaire personnel.
Context: ...a.net.http.HttpResponse$BodySubscribers.ofFile(java.nio.file.Path, [Ljava.nio.file.Ope...(MOTS_COLLES)
[typographical] ~44-~44: Pas de correspondance fermante ou ouvrante pour le caractère « ) »
Context: ...http.HttpResponse$BodySubscribers.ofFile(java.nio.file.Path, [Ljava.nio.file.Open...(UNPAIRED_BRACKETS)
[typographical] ~44-~44: Il manque une espace après le point.
Context: ...nse$BodySubscribers.ofFile(java.nio.file.Path, [Ljava.nio.file.OpenOption;) java.nio....(ESPACE_APRES_POINT)
[typographical] ~44-~44: Pas de correspondance fermante ou ouvrante pour le caractère « ] »
Context: ...ySubscribers.ofFile(java.nio.file.Path, [Ljava.nio.file.OpenOption;) java.nio.cha...(UNPAIRED_BRACKETS)
[typographical] ~44-~44: Une parenthèse fermante semble être requise pour clore l’incise présente dans cette phrase.
Context: ...File(java.nio.file.Path, [Ljava.nio.file.OpenOption;) java.nio.channels.FileChann...(PARENTHESES)
[typographical] ~46-~46: Il manque une espace après le point.
Context: ...a.nio.channels.FileChannel java.nio.file.Files java.security.KeyStore$Builder.newInsta...(ESPACE_APRES_POINT)
[uncategorized] ~47-~47: Une espace doit être ajoutée entre ces deux mots ou intégrez ce nouveau mot à votre dictionnaire personnel.
Context: ...le.Files java.security.KeyStore$Builder.newInstance(java.io.File, java.security.KeyStore$Pr...(MOTS_COLLES)
[typographical] ~47-~47: Une parenthèse fermante est nécessaire.
Context: ...va.security.KeyStore$Builder.newInstance(java.io.File, java.security.KeyStore$Protect...(PARENTHESES)
[typographical] ~47-~47: Il manque une espace après le point.
Context: ...ity.KeyStore$Builder.newInstance(java.io.File, java.security.KeyStore$ProtectionParam...(ESPACE_APRES_POINT)
[uncategorized] ~48-~48: Une espace doit être ajoutée entre ces deux mots ou intégrez ce nouveau mot à votre dictionnaire personnel.
Context: ...rameter) java.security.KeyStore$Builder.newInstance(java.lang.String, java.security.Provide...(MOTS_COLLES)
[typographical] ~48-~48: Une parenthèse fermante est nécessaire.
Context: ...va.security.KeyStore$Builder.newInstance(java.lang.String, java.security.Provider, ja...(PARENTHESES)
[typographical] ~48-~48: Il manque une espace après le point.
Context: ....String, java.security.Provider, java.io.File, java.security.KeyStore$ProtectionParam...(ESPACE_APRES_POINT)
[uncategorized] ~49-~49: Une espace doit être ajoutée entre ces deux mots ou intégrez ce nouveau mot à votre dictionnaire personnel.
Context: ...ectionParameter) java.security.KeyStore.getInstance(java.io.File, [C) java.security.KeyStor...(MOTS_COLLES)
[typographical] ~49-~49: Pas de correspondance fermante ou ouvrante pour le caractère « ) »
Context: ...eter) java.security.KeyStore.getInstance(java.io.File, [C) java.security.KeyStore...(UNPAIRED_BRACKETS)
[typographical] ~49-~49: Il manque une espace après le point.
Context: ...va.security.KeyStore.getInstance(java.io.File, [C) java.security.KeyStore.getInstance...(ESPACE_APRES_POINT)
[typographical] ~49-~49: Pas de correspondance fermante ou ouvrante pour le caractère « ] »
Context: ...rity.KeyStore.getInstance(java.io.File, [C) java.security.KeyStore.getInstance(ja...(UNPAIRED_BRACKETS)
src/main/java/de/tum/cit/ase/ares/api/jupiter/JupiterSecurityExtension.java (4)
27-27: No issues with the added line.The blank line added at line 27 improves code readability by separating variable declarations from the subsequent logic.
Line range hint
27-51: Removal ofArtemisSecurityManagerreferences simplifies the code.The elimination of
ArtemisSecurityManagerinstallation and uninstallation within theinterceptGenericInvocationmethod streamlines the method and reflects a shift in the security handling approach. This change reduces complexity and potential overhead associated with managing the security manager's lifecycle.
Line range hint
65-77: Validate thewithinPathstarting with 'classes' or 'test-classes'.The method
testAndGetPolicyWithinPaththrows aSecurityExceptionifwithinPathdoes not start with "classes" or "test-classes". Ensure that this constraint aligns with the project's directory structure and that it won't cause unintended exceptions.Run the following script to list all
withinPathusages and verify their values:#!/bin/bash # Description: Find all usages of the Policy annotation and extract withinPath values. # Test: Extract withinPath values. Expect: Values start with 'classes' or 'test-classes'. rg --type java '@Policy\\(.*withinPath\\s*=\\s*"([^"]+)"' --replace '$1'
Line range hint
32-44: Ensure the reset of security test case settings is still necessary.The code in the
elseblock resets the security test case settings by invoking theresetmethod via reflection. With the removal ofArtemisSecurityManager, verify if resetting these settings is still required for the proper functioning of the tests.Run the following script to check where
JavaSecurityTestCaseSettings.reset()is used:✅ Verification successful
Reset Method Unused - Removing Reset Logic is Safe
After verifying, there are no usages of
JavaSecurityTestCaseSettings.reset(). Therefore, the reset of security test case settings in theelseblock is no longer necessary and can be safely removed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of JavaSecurityTestCaseSettings.reset() to determine if it is still necessary. # Test: Search for calls to the reset method. Expect: Understand where and how it's used. rg --type java $'JavaSecurityTestCaseSettings\\.reset\\(\\)'Length of output: 319
Script:
#!/bin/bash # Description: Corrected search for usages of JavaSecurityTestCaseSettings.reset() to determine if it is still necessary. # Search for calls to the reset method. Expect: Understand where and how it's used. rg --type java 'JavaSecurityTestCaseSettings\.reset\(\)'Length of output: 58
src/test/java/de/tum/cit/ase/ares/integration/testuser/subject/fileSystem/FileSystemAccessPenguin.java (2)
302-304: Ensure Proper Handling in Third-Party Method CallThe method
accessPathThroughThirdPartyPackage()callsThirdPartyPackagePenguin.accessFileSystem(). Ensure that any resources opened in the third-party method are properly managed, and exceptions are appropriately handled.Consider reviewing the implementation of
ThirdPartyPackagePenguin.accessFileSystem()to confirm resource management.
288-291: Verify Correct Usage ofFileSystemProviderWhen using
FileSystemProvider, ensure that the provider obtained is the appropriate one for your file operations. Also, consider handling potential exceptions that may arise from deleting or reading attributes.Run the following script to list the installed file system providers:
src/test/java/de/tum/cit/ase/ares/integration/FileSystemAccessTest.java (5)
15-16: Addition ofimport java.io.IOExceptionThe import statement
import java.io.IOException;is necessary to support thethrows IOExceptiondeclarations in the updated method signatures.
Line range hint
202-478: Consistent addition ofthrows IOExceptiontoReadOperationstest methodsThe addition of
throws IOExceptionto the test methods in theReadOperationsclass is appropriate, as these methods invoke code that may throwIOException.
Line range hint
495-823: Consistent addition ofthrows IOExceptiontoWriteOperationstest methodsThe method signatures have been updated to include
throws IOExceptionin theWriteOperationsclass, which is appropriate since the tested methods may throw this exception during file write operations.
866-882: Addition ofthrows IOExceptiontoExecuteOperationstest methodsIncluding
throws IOExceptionin the method signatures of theExecuteOperationstest methods is appropriate for handling potentialIOExceptions during execution operations.
Line range hint
899-941: Addition ofthrows IOExceptiontoDeleteOperationstest methodsThe test methods in the
DeleteOperationsclass have been updated to includethrows IOException, which is appropriate given that they may encounterIOExceptions during file deletion operations.
...um/cit/ase/ares/integration/testuser/securitypolicies/OnePathAllowedInstrumentationRead.yaml
Outdated
Show resolved
Hide resolved
...t/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
Outdated
Show resolved
Hide resolved
...e/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceToolbox.java
Outdated
Show resolved
Hide resolved
...ava/de/tum/cit/ase/ares/integration/testuser/subject/fileSystem/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...ava/de/tum/cit/ase/ares/integration/testuser/subject/fileSystem/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...ava/de/tum/cit/ase/ares/integration/testuser/subject/fileSystem/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...ava/de/tum/cit/ase/ares/integration/testuser/subject/fileSystem/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...ava/de/tum/cit/ase/ares/integration/testuser/subject/fileSystem/FileSystemAccessPenguin.java
Show resolved
Hide resolved
...ava/de/tum/cit/ase/ares/integration/testuser/subject/fileSystem/FileSystemAccessPenguin.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java (4 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java (3)
125-126: Repeated Concern: Broad Instrumentation ofjava.lang.Object.As previously noted, adding
java.lang.Objectwith the<init>method tomethodsWhichCanReadFilesis overly broad. This could lead to unnecessary instrumentation of all object creations, significantly impacting performance and likely not reflecting the intended behavior for file read operations.
164-164:⚠️ Potential issueAvoid Instrumenting Private Native Methods in
java.util.prefs.FileSystemPreferences.The methods
lockFile0andunlockFile0are private native methods and are not accessible for instrumentation. Including them may not have any effect and could introduce unnecessary complexity. Consider removing these methods from the instrumentation map.To confirm that these methods are private and not suitable for instrumentation, run the following script:
#!/bin/bash # Description: Verify the access modifiers of methods in `java.util.prefs.FileSystemPreferences` # Search for declarations of `lockFile0` and `unlockFile0` methods rg --type java 'private native void (lockFile0|unlockFile0)\(' -A 2 -B 2
178-178:⚠️ Potential issueVerify Existence of
setPermissionMethod injava.io.UnixFileSystem.The method
setPermissiondoes not appear to exist injava.io.UnixFileSystem. Please verify the correct method name or consider if this entry is necessary.To check for the existence of the
setPermissionmethod injava.io.UnixFileSystem, run the following script:
...t/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
Outdated
Show resolved
Hide resolved
...t/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
Outdated
Show resolved
Hide resolved
...t/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj (1)
89-91: Consistent use ofgetClassName()when returning violating classWhen returning the violating class information, you are currently using
element.toString(). For consistency and clarity, it would be better to useelement.getClassName()to return just the class name.Apply this change:
if (!checkIfCallstackElementIsAllowed(allowedClasses, element)) { - return element.toString(); + return element.getClassName(); }This provides a cleaner output focused on the class name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj (2 hunks)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceToolbox.java (2 hunks)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceToolbox.java
- src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
🧰 Additional context used
🔇 Additional comments (2)
src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj (2)
81-95: Handle potential NullPointerExceptions and empty arraysEnsure that
allowedClasses,restrictedPackage, andstackTraceare notnullbefore using them. Additionally, consider handling cases whereallowedClassesmight be empty, which could affect the logic of your checks.Please verify that these variables are properly initialized and cannot be
nullor empty when the method is called. If necessary, add null and emptiness checks to prevent unexpected exceptions.
51-67: Ensure Javadoc comments align with code implementationThe Javadoc comments for
checkIfCallstackElementIsAllowedare well-written. Just make sure they remain updated if any changes are made to the method logic.Good job on providing clear documentation.
...it/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj
Show resolved
Hide resolved
...it/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj
Outdated
Show resolved
Hide resolved
…nputStream constructor should be looked at
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/aspectj/adviceandpointcut/JavaAspectJFileSystemAdviceDefinitions.aj (2 hunks)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceToolbox.java (2 hunks)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- 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/pointcut/JavaInstrumentationPointcutDefinitions.java
🧰 Additional context used
...e/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceToolbox.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceToolbox.java (2 hunks)
- src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java (5 hunks)
🧰 Additional context used
🔇 Additional comments (4)
src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java (2)
Line range hint
124-205: Summary of changes and recommendations.The changes in this file significantly expand the coverage of file system operations across various Java classes. However, there are a few areas that require attention:
- There's a TODO comment about improper instrumentation of read methods that needs to be addressed.
- Some method removals (e.g.,
getLastModifiedTime,checkAccess) need verification to ensure they don't create monitoring gaps.- A few method names in
java.nio.file.Filesneed correction.- Consider adding
setAttributetomethodsWhichCanExecuteFilesandwalkFileTreetomethodsWhichCanDeleteFilesfor more comprehensive coverage.Please review these recommendations and make the necessary adjustments to ensure robust and accurate file system operation monitoring.
182-186: Verify impact of removingcheckAccessmethod.The
checkAccessmethod has been removed fromUnixFileSystemandWinNTFileSystementries. This might affect the monitoring of execute permission checks on these systems.Please confirm if this removal is intentional and doesn't create any gaps in execute permission monitoring. Consider running the following script to check for any remaining usages:
#!/bin/bash # Search for any remaining usages of checkAccess in UnixFileSystem and WinNTFileSystem rg --type java '(UnixFileSystem|WinNTFileSystem).*checkAccess'src/main/java/de/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceToolbox.java (2)
103-111: Improved class name comparison and false positive preventionGreat job addressing the previous review comment! The changes in this segment improve the accuracy of class name comparisons and prevent false positives. Specifically:
- Using
element.getClassName()instead ofelement.toString()ensures more accurate class comparisons.- Skipping
OutputTester,InputTester, andX11FontManagerclasses prevents false positives in violation checks.- Returning
element.getClassName()instead ofelement.toString()provides more precise information about violating classes.These improvements enhance the reliability of the call stack criteria checks.
Line range hint
1-258: Summary of changes and next stepsOverall, the changes in this file improve the accuracy of call stack checks and update the file system interaction logic. Here's a summary of the review:
- The
checkIfCallstackCriteriaIsViolatedmethod now uses more accurate class name comparisons and prevents false positives.- The
checkFileSystemInteractionmethod has been updated to use "overwrite" instead of "write".Next steps:
- Clarify the reasoning behind changing "write" to "overwrite" and ensure consistency across the codebase.
- Verify the proper initialization of
pathsAllowedToBeOverwrittenusing the provided script.- Address any findings from the verification script.
Once these points are addressed, the changes will be ready for approval.
...t/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
Show resolved
Hide resolved
...t/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
Show resolved
Hide resolved
...t/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
Show resolved
Hide resolved
...t/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
Show resolved
Hide resolved
...t/ase/ares/api/aop/java/instrumentation/pointcut/JavaInstrumentationPointcutDefinitions.java
Show resolved
Hide resolved
...e/tum/cit/ase/ares/api/aop/java/instrumentation/advice/JavaInstrumentationAdviceToolbox.java
Show resolved
Hide resolved
| @@ -89,7 +89,7 @@ public void executeArchitectureTestCase(JavaClasses classes) { | |||
| .NO_CLASSES_SHOULD_ACCESS_NETWORK | |||
| .check(classes); | |||
| case THREAD_CREATION -> | |||
| throw new UnsupportedOperationException("Ares Security Error (Reason: Ares-Code; Stage: Execution): Thread creation not implemented yet."); | |||
| JavaArchitectureTestCaseCollection.NO_CLASSES_SHOULD_CREATE_THREADS.check(classes); | |||
There was a problem hiding this comment.
Consider if executeArchitectureTestCase(JavaClasses classes) can be designed more efficiently, as nearly each case calls .check(classes) on a constant on JavaArchitectureTestCaseCollection.
| @@ -15,6 +14,7 @@ public class FileHandlerConstants { | |||
| public static final Path JAVA_JVM_TERMINATION_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "jvm-termination-methods.txt"); | |||
| public static final Path JAVA_REFLECTION_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "reflection-methods.txt"); | |||
| public static final Path JAVA_COMMAND_EXECUTION_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "command-execution-methods.txt"); | |||
| public static final Path JAVA_THREAD_CREATION_METHODS = FileTools.resolveOnResources("templates", "architecture" , "java", "archunit", "methods", "thread-creation-methods.txt"); | |||
There was a problem hiding this comment.
Conside if there is a more efficient way to create the Paths of these, as there is a lot of code repitition.
| @@ -32,7 +30,9 @@ private CustomClassResolver() { | |||
| * @return The resolved class if it exists. | |||
| */ | |||
| public static Optional<JavaClass> tryResolve(String typeName) { | |||
| ArchConfiguration.get().setClassResolver(ClassResolverFromClasspath.class); | |||
| if (typeName.startsWith("de.tum.cit.ase.ares.api.aop.java.aspectj.adviceandpointcut.JavaAspectJFileSystemAdviceDefinitions")) { | |||
There was a problem hiding this comment.
Please add a comment why we have de.tum.cit.ase.ares.api.aop.java.aspectj.adviceandpointcut.JavaAspectJFileSystemAdviceDefinitions here.
Checklist
General
Ares
Motivation and Context
Fix errors and bugs
Description
New Features
Bug Fixes
SecurityExceptionfor unauthorized access attempts.Review Progress
Code Review
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores