mutcianm/IDEA-293570

[plugins] increase verbosity of warnings in PluginRepositoryAuthService IDEA-293570

[plugins] drop domain checks in PluginRepositoryAuthService IDEA-293570

it was decided that these security limitations are an overkill and do not solve any practical issues

Merge-request: IJ-MR-24467
Merged-by: Mikhail Mutcianko <mikhail.mutcianko@jetbrains.com>
GitOrigin-RevId: 94a342d3765cca42ae614a7c904616c218d431a0

diff --git a/platform/platform-impl/src/com/intellij/ide/plugins/auth/PluginRepositoryAuthService.kt b/platform/platform-impl/src/com/intellij/ide/plugins/auth/PluginRepositoryAuthService.kt
index ac08b6a4e2e31615e0fa54ead2ec7beef6be12ed..849a8f0af5312aceffee0abbf7980404015cfa8f 100644 (file)
--- a/platform/platform-impl/src/com/intellij/ide/plugins/auth/PluginRepositoryAuthService.kt
+++ b/platform/platform-impl/src/com/intellij/ide/plugins/auth/PluginRepositoryAuthService.kt
@@ -1,24 +1,16 @@
 // Copyright 2000-2022 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
 package com.intellij.ide.plugins.auth
 
-import com.google.common.net.UrlEscapers
 import com.intellij.openapi.components.Service
 import com.intellij.openapi.diagnostic.logger
 import com.intellij.util.io.HttpRequests
 import org.jetbrains.annotations.NotNull
-import java.net.URI
-import java.util.*
 
 /**
- * Collects custom auth headers from EPs and validates them
- *
- * This service keeps track of [PluginRepositoryAuthProvider] injected headers to ensure that:
- * - each contributor handles only 1 domain
- * - each domain has no more than 1 contributor
+ * Collects custom auth headers from EPs and optionally provides a HttpRequests.ConnectionTuner with injected headers
  */
 @Service(Service.Level.APP)
 class PluginRepositoryAuthService {
-  private val contributorToDomainCache = Collections.synchronizedMap(WeakHashMap<PluginRepositoryAuthProvider, String>())
 
   val connectionTuner = HttpRequests.ConnectionTuner { connection ->
     try {
@@ -26,64 +18,40 @@ class PluginRepositoryAuthService {
         .forEach { (k, v) -> connection.addRequestProperty(k, v) }
     }
     catch (e: Exception) {
-      LOG.error(e)
+      LOG.warn("Filed to inject headers into request(${connection.url})", e)
     }
   }
 
   @NotNull
   fun getAllCustomHeaders(@NotNull url: String): Map<String, String> {
     val allContributors = PluginRepositoryAuthProvider.EP_NAME.extensionsIfPointIsRegistered
+    val matchingContributors = allContributors.filter { it.canHandleSafe(url) }
 
-    val domain = getDomainFromUrl(url) ?: return cancelWithWarning("Can't get domain from url: $url")
-
-    val domainMatchingContributors = allContributors.filter { it.canHandleSafe(domain) }
-
-    if (domainMatchingContributors.isEmpty())
+    if (matchingContributors.isEmpty())
       return emptyMap()
-    if (domainMatchingContributors.size > 1)
-      return cancelWithWarning("Multiple contributors found for domain: $domain")
-
-    val primeCandidate = domainMatchingContributors.first()
-
-    if (!handlesSingleDomain(primeCandidate, domain))
-      return cancelWithWarning("Contributor $primeCandidate tried to inject into multiple domains")
-
-    return primeCandidate
-             .also { contributor -> updateCaches(domain, contributor) }
-             .getCustomHeadersSafe(url)
-  }
-
-  private fun handlesSingleDomain(@NotNull contributor: PluginRepositoryAuthProvider, @NotNull domain: String): Boolean {
-    val lastKnownDomain = contributorToDomainCache[contributor]
-    return domain == lastKnownDomain || lastKnownDomain == null
-  }
-
-  private fun getDomainFromUrl(url: String): String? = URI(UrlEscapers.urlFragmentEscaper().escape(url)).host
+    if (matchingContributors.size > 1)
+      LOG.warn("Multiple contributors tried to inject headers in to url($url): ${matchingContributors.joinToString { it.javaClass.simpleName }}")
 
-  private fun updateCaches(@NotNull url: String, @NotNull contributor: PluginRepositoryAuthProvider) {
-    contributorToDomainCache[contributor] = url
-  }
+    val primeCandidate = matchingContributors.first()
 
-  private fun cancelWithWarning(message: String): Map<String, String> {
-    LOG.warn(message)
-    return emptyMap()
+    return primeCandidate.getCustomHeadersSafe(url)
   }
 }
 
 private val LOG = logger<PluginRepositoryAuthService>()
 
-private fun PluginRepositoryAuthProvider.canHandleSafe(domain: String): Boolean = withLogging(false) { canHandle(domain) }
+private fun PluginRepositoryAuthProvider.canHandleSafe(url: String): Boolean = try {
+    canHandle(url)
+  } catch (e: Exception) {
+    LOG.warn("Error while checking if a provider can handle URL($url), assuming false", e)
+    false
+  }
 
 private fun PluginRepositoryAuthProvider.getCustomHeadersSafe(url: String): Map<String, String> {
-  return withLogging(emptyMap()) { getAuthHeaders(url) }
-}
-
-private inline fun <T> withLogging(default: T, f: () -> T): T {
   return try {
-    f()
-  }
-  catch (e: Exception) {
-    LOG.error(e)
-    default
+    getAuthHeaders(url)
+  } catch (e: Exception) {
+    LOG.warn("Failed to get custom headers from provider for URL($url), returning emptyMap()", e)
+    emptyMap()
   }
 }
\ No newline at end of file

diff --git a/platform/platform-tests/testSrc/com/intellij/ide/plugins/ExternalAuthContributorServiceTest.kt b/platform/platform-tests/testSrc/com/intellij/ide/plugins/ExternalAuthContributorServiceTest.kt
index bbee36e58bb7425eeb19825484c6196b34b88ad7..d8012d1fe089d5e8c8db56fec7018c40cf27053c 100644 (file)
--- a/platform/platform-tests/testSrc/com/intellij/ide/plugins/ExternalAuthContributorServiceTest.kt
+++ b/platform/platform-tests/testSrc/com/intellij/ide/plugins/ExternalAuthContributorServiceTest.kt
@@ -30,51 +30,47 @@ class PluginRepositoryAuthServiceTest {
 
   private val unescapedUrl = "https://buildserver.labs.intellij.net/guestAuth/repository/download/Documentation_Stardust/lastSuccessful/updatePlugins.xml?branch=master-internal &build=IU-221.3427.103"
 
-  private val anyUrlInvalidContributor = object : PluginRepositoryAuthProvider {
-    override fun canHandle(domain: String): Boolean = true
-    override fun getAuthHeaders(url: String?): Map<String, String> = headers
-  }
-
-  private inner class FakeUrlMatchingContributor(private val myDomain: String): PluginRepositoryAuthProvider {
-    override fun canHandle(domain: String): Boolean = (domain == myDomain)
-    override fun getAuthHeaders(url: String?): Map<String, String>  = headers
+  private inner class FakeUrlMatchingContributor(private val myDomain: String,
+                                                 private val contributorHeaders: Map<String, String> = headers): PluginRepositoryAuthProvider {
+    override fun canHandle(url: String): Boolean = (url.contains(myDomain))
+    override fun getAuthHeaders(url: String?): Map<String, String>  = contributorHeaders
   }
 
   @Test
   fun basicTest() {
     setupContributors(FakeUrlMatchingContributor(fooDomain))
-    assertEquals(PluginRepositoryAuthService().getAllCustomHeaders(fooUrl), headers)
+    assertEquals(headers, PluginRepositoryAuthService().getAllCustomHeaders(fooUrl))
   }
 
   @Test
-  fun singleUrlHandling() {
+  fun `no headers are returned for an unmatched URL`() {
     val authService = PluginRepositoryAuthService()
-    setupContributors(anyUrlInvalidContributor)
-    assertEquals(authService.getAllCustomHeaders(fooUrl), headers)
-    assertEquals(authService.getAllCustomHeaders(barUrl), emptyMap())
+    setupContributors(FakeUrlMatchingContributor(fooDomain))
+    assertEquals(headers, authService.getAllCustomHeaders(fooUrl))
+    assertEquals(emptyMap(), authService.getAllCustomHeaders(barUrl))
   }
 
   @Test
-  fun singleContributorPerUrl() {
+  fun `only the first contributor is used in case of multiple matching`() {
     val authService = PluginRepositoryAuthService()
-    setupContributors(FakeUrlMatchingContributor(fooDomain), FakeUrlMatchingContributor(fooDomain))
-    assertEquals(authService.getAllCustomHeaders(fooUrl), emptyMap())
+    setupContributors(FakeUrlMatchingContributor(fooDomain), FakeUrlMatchingContributor(fooDomain, mapOf("foo" to "bar")))
+    assertEquals(headers, authService.getAllCustomHeaders(fooUrl))
   }
 
   @Test
-  fun nonEscapedUrlTest() {
+  fun `malformed URL doesn't crash getAllCustomHeaders`() {
     val authService = PluginRepositoryAuthService()
     authService.getAllCustomHeaders(unescapedUrl)
   }
 
   @Test
-  fun multipleValidRepos() {
+  fun `getAllCustomHeaders doesn't crash when no contributors match`() {
     val authService = PluginRepositoryAuthService()
     setupContributors(FakeUrlMatchingContributor(fooDomain))
     authService.getAllCustomHeaders(barUrl)
     authService.getAllCustomHeaders(fooUrl)
     authService.getAllCustomHeaders(barUrl)
-    assertEquals(authService.getAllCustomHeaders(fooUrl), headers)
+    assertEquals(headers, authService.getAllCustomHeaders(fooUrl))
   }
 
   private fun setupContributors(vararg contributor: PluginRepositoryAuthProvider) {