You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "paulk-asert (via GitHub)" <gi...@apache.org> on 2026/09/06 04:59:18 UTC
[PR] GROOVY-12369: grape uninstall: keep artifact deletion inside the modu… [groovy]
paulk-asert opened a new pull request, #2893:
URL: https://github.com/apache/groovy/pull/2893
…le cache
uninstallArtifact deletes the jars named by a module's cached ivy descriptor. The jar path was new File(jardir, name) with name taken from the descriptor's artifact attributes unchecked, so a name carrying a path separator or a ".." segment deleted a file outside the module's jars directory:
<artifact name="../victim" .../>
-> new File(jardir, "../victim-1.0.jar").delete() above jardir
The deletion is now skipped, with a warning, unless the resolved file is inside the jars directory. The check is on canonical paths, so a ".." or a symbolic link along the way cannot carry it out, and it covers the version, classifier and extension that are concatenated into the name as well.
Strictly this is out of the security model: a crafted descriptor is cached only by resolving from a repository, and resolving from an untrusted repository already runs its code, so a delete-on-uninstall is weaker than what such a repository already achieved. It is fixed anyway, as defense in depth and because a tool should not delete outside its own cache whatever a descriptor says, matching the containment added for deleteDir and groovydoc.
The regression test builds a module cache with a traversing artifact name, runs uninstall, and asserts the outside file survives while an ordinary artifact is still deleted; it was confirmed to delete the outside file without the guard. It runs under -Djunit.network=true with the other groovy.grape tests, though it needs no network itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12369: grape uninstall: keep artifact deletion inside the modu… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert merged PR #2893:
URL: https://github.com/apache/groovy/pull/2893
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12369: grape uninstall: keep artifact deletion inside the modu… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on PR #2893:
URL: https://github.com/apache/groovy/pull/2893#issuecomment-5557100913
Thanks @daniellansun for the careful notes — all three suggestions taken in the follow-up commit (1a575020a4), plus Copilot's lock point which was the same as your first bullet under (2).
**Production**
- The hidden-type check now sits above the `try` beside the caller-sensitive / abstract declines, so it reads as a policy decline rather than a define failure.
- `allTypesNameable` takes the `CachedMethod` and reads the declaring class, return type and native parameter types the same way `isPubliclyInvocableFromInvokerFactory` does, so it no longer forces accessibility on the underlying `Method`. (Either form yields the same classes as `InvokerBytecode` writes; this one just avoids the side effect.)
- The class javadoc gains a paragraph after the four define-path steps stating the hidden-type precondition, so a reader of `tryStep1` no longer has to go via GROOVY-12361.
**Tests**
- `testHiddenProxyStaysInvocablePastThreshold` now takes `@ResourceLock(Resources.SYSTEM_PROPERTIES)`, uses `restoreProperty`, drives a fresh `CachedMethod` past the threshold and asserts `invokerAttempted == true` / `invoker == null` before re-invoking, mirroring `testCachedMethodStickyFailsWhenAllDefineStepsFail`. For the record I also confirmed it is not vacuous: with the gate removed it fails with the original `NoClassDefFoundError` on the `_groovyProxy` hidden class, and the two factory-level tests fail on the null assertion.
- The old Step 3 fall-through test is renamed `testTryCreateDeclinesNonPublicHiddenHost`, keeps `assertNull(tryCreate(...))`, adds an `allTypesNameable` check, and drops the direct `tryCreateClassData` definition of an uninvokable trampoline. You were right that the "remaining classData tests rely on it" comment was inaccurate — they all use ordinary subjects.
One small note on the lock: JUnit parallel execution is not enabled in this build, so the race was theoretical, but consistency with the rest of the file is reason enough.
Not chased, as you suggested: a shared nameable-type helper with `CallSiteGenerator`, and extra coverage for hidden return/parameter types without a hidden declaring class (the three uncovered lines Codecov flags). The TestLens failure is the known-flaky `ChannelSelectTest` timer test, unrelated to this change.
`InvokerFactoryTest` (62) and `CachedMethodDirectInvokerTest` (33) are green locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12369: grape uninstall: keep artifact deletion inside the modu… [groovy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2893:
URL: https://github.com/apache/groovy/pull/2893#issuecomment-5557232787
## [Codecov](https://app.codecov.io/gh/apache/groovy/pull/2893?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `7 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 71.0769%. Comparing base ([`7a66bf0`](https://app.codecov.io/gh/apache/groovy/commit/7a66bf0472789d32a64e04345850d7ca82e38874?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`05d5931`](https://app.codecov.io/gh/apache/groovy/commit/05d59317ab500e7d35e5d2b6872df40f816bd7ea?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2893?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...y/src/main/groovy/groovy/grape/ivy/GrapeIvy.groovy](https://app.codecov.io/gh/apache/groovy/pull/2893?src=pr&el=tree&filepath=subprojects%2Fgroovy-grape-ivy%2Fsrc%2Fmain%2Fgroovy%2Fgroovy%2Fgrape%2Fivy%2FGrapeIvy.groovy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LWdyYXBlLWl2eS9zcmMvbWFpbi9ncm9vdnkvZ3Jvb3Z5L2dyYXBlL2l2eS9HcmFwZUl2eS5ncm9vdnk=) | 0.0000% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/groovy/pull/2893?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2893?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2893 +/- ##
==================================================
- Coverage 71.0843% 71.0769% -0.0074%
+ Complexity 37332 37331 -1
==================================================
Files 1579 1579
Lines 135480 135487 +7
Branches 25105 25106 +1
==================================================
- Hits 96305 96300 -5
- Misses 30492 30503 +11
- Partials 8683 8684 +1
```
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2893?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...y/src/main/groovy/groovy/grape/ivy/GrapeIvy.groovy](https://app.codecov.io/gh/apache/groovy/pull/2893?src=pr&el=tree&filepath=subprojects%2Fgroovy-grape-ivy%2Fsrc%2Fmain%2Fgroovy%2Fgroovy%2Fgrape%2Fivy%2FGrapeIvy.groovy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LWdyYXBlLWl2eS9zcmMvbWFpbi9ncm9vdnkvZ3Jvb3Z5L2dyYXBlL2l2eS9HcmFwZUl2eS5ncm9vdnk=) | `33.5802% <0.0000%> (-0.5906%)` | :arrow_down: |
... and [10 files with indirect coverage changes](https://app.codecov.io/gh/apache/groovy/pull/2893/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12369: grape uninstall: keep artifact deletion inside the modu… [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2893:
URL: https://github.com/apache/groovy/pull/2893#issuecomment-5557344561
## ✅ All tests passed ✅
🏷️ Commit: 05d59317ab500e7d35e5d2b6872df40f816bd7ea
▶️ Tests: 116803 executed
⚪️ Checks: 32/32 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org