[WIP] Add ability to download sources behind authenticated proxy#388
Conversation
- Add org.eclipse.core.net dependency for proxy service - Create ProxyUtil helper class for proxy configuration - Update UrlDownloader to use Eclipse proxy settings - Update AbstractSourceCodeFinder for proxy support - Update NexusSourceCodeFinder for proxy support - Update ArtifactorySourceCodeFinder for proxy support - Update SonatypeSourceCodeFinder for proxy support - Add basic ProxyUtil tests - Maintains backward compatibility when no proxy is configured Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/5415a472-2e86-4422-953d-3d9f5857f1d3 Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/511e1bea-6ddb-4b4f-b85d-5aff47908c6b Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds authenticated system proxy support (Eclipse “Native” proxy settings) to the source-attach plugin so that source downloads and repository queries work when running behind an authenticated proxy.
Changes:
- Introduces a new
ProxyUtilhelper to obtain ajava.net.Proxyfrom EclipseIProxyService(with optional proxy authentication setup). - Updates URL opening logic in
UrlDownloaderand multipleSourceCodeFinderimplementations to use the Eclipse-selected proxy when available. - Adds
org.eclipse.core.netto the source-attach bundle dependencies and introduces a basicProxyUtilTest.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| io.github.nbauma109.decompiler.source.attach/src/io/github/nbauma109/decompiler/source/attach/utils/UrlDownloader.java | Uses Eclipse proxy service to open download connections with proxy support |
| io.github.nbauma109.decompiler.source.attach/src/io/github/nbauma109/decompiler/source/attach/utils/ProxyUtil.java | New helper for selecting proxies (and currently sets proxy auth via global Authenticator) |
| io.github.nbauma109.decompiler.source.attach/src/io/github/nbauma109/decompiler/source/attach/finder/AbstractSourceCodeFinder.java | Adds proxy-aware connection opening for getString() and introduces getProxyService() |
| io.github.nbauma109.decompiler.source.attach/src/io/github/nbauma109/decompiler/source/attach/finder/SonatypeSourceCodeFinder.java | Makes Sonatype API POST requests proxy-aware |
| io.github.nbauma109.decompiler.source.attach/src/io/github/nbauma109/decompiler/source/attach/finder/NexusSourceCodeFinder.java | Makes Nexus HTTP requests proxy-aware |
| io.github.nbauma109.decompiler.source.attach/src/io/github/nbauma109/decompiler/source/attach/finder/ArtifactorySourceCodeFinder.java | Makes Artifactory API requests proxy-aware |
| io.github.nbauma109.decompiler.source.attach/META-INF/MANIFEST.MF | Adds dependency on org.eclipse.core.net |
| io.github.nbauma109.decompiler.source.attach.tests/src/io/github/nbauma109/decompiler/source/attach/utils/ProxyUtilTest.java | Adds minimal unit tests for ProxyUtil.getProxy null-service behavior |
Comments suppressed due to low confidence (1)
io.github.nbauma109.decompiler.source.attach/src/io/github/nbauma109/decompiler/source/attach/utils/UrlDownloader.java:114
- UrlDownloader sets a per-connection Authenticator that always returns
serviceUser/servicePasswordregardless ofRequestorType. When running behind an authenticated proxy, this can cause the proxy challenge to receive the repository/service credentials (and may prevent proxy auth from working, since it overrides the proxy Authenticator). Update the Authenticator to only answer forRequestorType.SERVER(or otherwise differentiate proxy vs server auth), and let proxy credentials be handled separately.
// Set HTTP Basic authentication if credentials are provided
if (serviceUser != null && servicePassword != null) {
((HttpURLConnection) conn).setAuthenticator(new Authenticator() {
@Override
protected PasswordAuthentication getPasswordAuthentication() {
return new PasswordAuthentication(serviceUser, servicePassword.toCharArray());
}
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static void configureProxy(URLConnection connection, URI uri, IProxyService proxyService) { | ||
| if (proxyService == null) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
ProxyUtil.configureProxy(...) is currently unused, and its connection parameter isn’t used at all. The method also sets global System proxy properties / default Authenticator, which is a JVM-wide side effect and is risky in an Eclipse environment where multiple bundles share the same VM. Consider removing this method for now, or refactoring into a side-effect-free API (e.g., openConnection-with-proxy + per-connection authenticator) that callers actually use.
| @Test | ||
| public void getProxyWithNullServiceReturnsNoProxy() throws Exception { | ||
| URI uri = new URI("http://example.com"); | ||
| Proxy proxy = ProxyUtil.getProxy(uri, null); | ||
|
|
||
| assertNotNull(proxy); | ||
| assertEquals(Proxy.NO_PROXY, proxy); | ||
| } | ||
|
|
||
| @Test | ||
| public void getProxyDoesNotThrowWithValidUri() throws Exception { | ||
| // This test verifies that getProxy gracefully handles proxy service errors | ||
| URI uri = new URI("http://example.com"); | ||
|
|
||
| // Should not throw even when proxy service is null | ||
| Proxy proxy = ProxyUtil.getProxy(uri, null); | ||
| assertNotNull(proxy); | ||
| } |
There was a problem hiding this comment.
ProxyUtilTest currently has two tests that both call getProxy(uri, null) and only assert non-null / NO_PROXY. This doesn’t exercise the new behavior (proxy selection and authenticated proxy handling) and the second test is largely redundant. Consider adding a small stub IProxyService/IProxyData to verify the returned Proxy host/port and (if you keep any Authenticator behavior) that authentication is configured as expected.
| private IProxyService getProxyService() { | ||
| try { | ||
| BundleContext bundleContext = FrameworkUtil.getBundle(getClass()).getBundleContext(); | ||
| if (bundleContext != null) { | ||
| ServiceReference<IProxyService> serviceReference = bundleContext.getServiceReference(IProxyService.class); | ||
| if (serviceReference != null) { | ||
| return bundleContext.getService(serviceReference); | ||
| } |
There was a problem hiding this comment.
getProxyService() calls bundleContext.getService(serviceReference) and returns the service without ever ungetting it. Because this method can be called repeatedly (e.g., each download), it can leak OSGi service usage counts. Consider caching the IProxyService once (e.g., in the plugin activator / a ServiceTracker) and ungetting it on bundle stop, or using a ServiceTracker here instead of repeatedly calling getService().
| protected IProxyService getProxyService() { | ||
| try { | ||
| BundleContext bundleContext = FrameworkUtil.getBundle(getClass()).getBundleContext(); | ||
| if (bundleContext != null) { | ||
| ServiceReference<IProxyService> serviceReference = bundleContext.getServiceReference(IProxyService.class); | ||
| if (serviceReference != null) { | ||
| return bundleContext.getService(serviceReference); | ||
| } |
There was a problem hiding this comment.
AbstractSourceCodeFinder.getProxyService() returns bundleContext.getService(serviceReference) without ever calling ungetService. Since getString()/open() can be called many times, this can leak OSGi service usage counts. Prefer a ServiceTracker or caching the service once (and ungetting it on bundle stop) rather than acquiring it on every request.
| if (proxyUser != null && !proxyUser.isEmpty() && proxyPassword != null) { | ||
| Authenticator.setDefault(new Authenticator() { | ||
| @Override | ||
| protected PasswordAuthentication getPasswordAuthentication() { | ||
| if (getRequestorType() == RequestorType.PROXY) { |
There was a problem hiding this comment.
ProxyUtil.getProxy() installs a JVM-global Authenticator.setDefault(...) as a side effect. This is process-wide state, so concurrent downloads/finders (or other Eclipse plugins) can race/overwrite each other’s proxy credentials, and tests may become order-dependent. Prefer configuring authentication per connection (e.g., HttpURLConnection#setAuthenticator(...) with a RequestorType.PROXY check) or encapsulate proxy opening in a helper that returns an already-configured connection without mutating global state.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a825da0f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| String proxyPassword = proxyData.getPassword(); | ||
|
|
||
| if (proxyUser != null && !proxyUser.isEmpty() && proxyPassword != null) { | ||
| Authenticator.setDefault(new Authenticator() { |
There was a problem hiding this comment.
ProxyUtil.getProxy calls Authenticator.setDefault(...), which mutates global JVM state every time a request resolves proxy settings. In Eclipse this can interfere with unrelated network traffic (including other plugins) and creates races when concurrent downloads need different proxy credentials, because the last caller wins. This should avoid process-wide mutation and keep authentication scoped to the specific connection/request.
Useful? React with 👍 / 👎.
Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/58294867-45d8-43c1-9eb1-ee1575a599ff Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/02affcbc-f1a7-4440-82a4-fb4186c10018 Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
…ed ProxyStubs utility Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/815c940a-1134-480d-b933-12310a1f0d65 Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/815c940a-1134-480d-b933-12310a1f0d65 Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
…ception types) Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/2fa88b59-f82f-4d2c-8287-598932ecf8ce Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ance Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/030cef36-33df-493e-b1a6-83af8fb35de1 Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
…/test-classes/ Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/f49fb645-a23e-4f8a-b469-d380b2ff8fd2 Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
…sertion-in-production-code warnings Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/7c27630c-f476-48bb-9976-7c9260015d71 Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
- Add awaitCompletion() to FinderManager to enable join-based waiting - FinderManagerTest: use awaitCompletion() instead of Thread.sleep polling loop, add assertion to cancelAfterFindSourcesWithEmptyListDoesNotThrow - GAVTest: replace assertFalse(equals) with assertNotEquals - Create MavenRepoSourceCodeFinderNullCoordinateTest with @parameterized replacing 3 duplicate tests; remove those from MavenRepoSourceCodeFinderTest - SourceCodeFinderFacadeTest: add assertNull assertion to cancelDoesNotThrow - AttachSourceActionTest: replace Thread.sleep with LockSupport.parkNanos in waitForSourceAttachment - JavaSourceAttacherHandlerTest: assertEquals for path check, LockSupport.parkNanos for sleep - SourceBindingUtilTest: add assertNull assertions to 4 no-op tests - SourceConstantsTest: swap assertEquals arguments to expected, actual order - UrlDownloaderTest: refactor try/catch to @test(expected=UnsupportedOperationException.class) - ExportSourceActionTest: refactor try/catch to @test(expected=IOException.class) - EditorUtilityTest: add assertNotNull assertion to startupEarlyStartupIsNoOp - JavaDecompilerClassFileEditorTest: assertSame instead of assertTrue(==), LockSupport.parkNanos for sleeps - DecompileUtilPluginTest: assertEquals(Boolean.TRUE, ...) instead of assertTrue(equals) - LoggerTest: add assertNoException helper and use it in all 7 test methods Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/d9d5b95c-5085-4876-90ea-204d2512bfda Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
… pattern Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/d9d5b95c-5085-4876-90ea-204d2512bfda Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
…param in tests Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/9eef8519-2d8b-400a-abe6-24a1cf631144 Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
…vaDecompilerClassFileEditorTest Agent-Logs-Url: https://github.com/nbauma109/ecd/sessions/23d4f5da-3ed0-47f6-9457-115bbd24cf65 Co-authored-by: nbauma109 <9403560+nbauma109@users.noreply.github.com>
|
Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.