Added more tests and specify comments a bit more

Author: somiljain2006

core/src/main/java/jenkins/security/csp/AvatarContributor.java

@@ -49,13 +49,13 @@ public class AvatarContributor implements Contributor {
     private static final Logger LOGGER = Logger.getLogger(AvatarContributor.class.getName());
 
     private final Set<String> domains = ConcurrentHashMap.newKeySet();
-        private final Set<String> allowedSources = ConcurrentHashMap.newKeySet();
+    private final Set<String> allowedSources = ConcurrentHashMap.newKeySet();
 
-        @Override
-        public void apply(CspBuilder cspBuilder) {
-                domains.forEach(d -> cspBuilder.add("img-src", d));
-                allowedSources.forEach(s -> cspBuilder.add("img-src", s));
-        }
+    @Override
+    public void apply(CspBuilder cspBuilder) {
+        domains.forEach(d -> cspBuilder.add("img-src", d));
+        allowedSources.forEach(s -> cspBuilder.add("img-src", s));
+    }
 
     /**
      * Request addition of the domain of the specified URL to the allowed set of avatar image domains.
@@ -94,20 +94,21 @@ public static void allow(@CheckForNull String url) {
      * @param url The full avatar image URL to allow.
      */
     @SuppressWarnings("unused")
-        public static void allowUrl(@CheckForNull String url) {
-                String normalized = normalizeUrl(url);
+    public static void allowUrl(@CheckForNull String url) {
+        AvatarContributor self = ExtensionList.lookupSingleton(AvatarContributor.class);
 
-                if (normalized == null) {
-                        LOGGER.log(Level.FINE, "Skipping invalid or unsupported avatar URL: " + url);
-                        return;
-                }
+        if (addNormalizedUrl(url, self.allowedSources)) {
+            LOGGER.log(Level.CONFIG, "Adding allowed avatar URL: {0}", url);
+        }
+    }
 
-                if (ExtensionList.lookupSingleton(AvatarContributor.class).allowedSources.add(normalized)) {
-                        LOGGER.log(Level.CONFIG, "Adding allowed avatar URL: " + normalized + " (from: " + url + ")");
-                } else {
-                        LOGGER.log(Level.FINEST, "Skipped adding duplicate allowed url: " + normalized + " (from: " + url + ")");
-                }
+    static boolean addNormalizedUrl(@CheckForNull String url, Set<String> target) {
+        String normalized = normalizeUrl(url);
+        if (normalized == null) {
+            return false;
         }
+        return target.add(normalized);
+    }
 
     /**
      * Utility method extracting the domain specification for CSP fetch directives from a specified URL.
@@ -157,14 +158,33 @@ public static String extractDomainFromUrl(@CheckForNull String url) {
     /**
      * Normalizes a URL for use in Content-Security-Policy.
      * <p>
-     * This method performs several steps:
+     * This method validates and canonicalizes the URL to ensure it is safe for use in a CSP header.
      * </p>
      * <ul>
-     * <li>Only http or https are accepted.</li>
-     * <li>Removes embedded credentials for security.</li>
-     * <li>Converts IDN to ASCII.</li>
-     * <li>Normalizes IPv6 addresses.</li>
-     * <li>Removes default ports.</li>
+     *   <li>
+     *     <strong>Accepts only http and https schemes</strong> -
+     *     Avatar images are fetched over the network. Other schemes such as file, data, or JavaScript are either unsafe
+     *     or meaningless in a CSP img-src context.
+     *   </li>
+     *   <li>
+     *     <strong>Rejects URLs with embedded credentials</strong> —
+     *     URLs containing user:password@host can accidentally expose credentials in logs or browser requests. Such URLs
+     *     are then rejected.
+     *   </li>
+     *   <li>
+     *     <strong>Converts IDN to ASCII</strong> —
+     *     Browser internally normalize hostnames to their ASCII representation. Normalizing here ensures Jenkins
+     *     generates CSP entries that match browser behavior consistently.
+     *   </li>
+     *   <li>
+     *     <strong>Normalizes IPv6 literals</strong> —
+     *     IPv6 addresses must be enclosed in square brackets in URLs.
+     *   </li>
+     *   <li>
+     *     <strong>Removes default ports</strong> —
+     *     Ports  80 (HTTP) and 443 (HTTPS) are removed. Removing them avoids duplicate CSP entries that differ only
+     *     by the presence of a default port.
+     *   </li>
      * </ul>
      *
      * @param url The raw input URL.

core/src/test/java/jenkins/security/csp/AvatarContributorTest.java

@@ -2,11 +2,15 @@
 
 import static jenkins.security.csp.AvatarContributor.extractDomainFromUrl;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.Matchers.startsWith;
 
+import java.util.HashSet;
+import java.util.Set;
 import org.junit.jupiter.api.Test;
 import org.jvnet.hudson.test.For;
 
@@ -144,17 +148,52 @@ void testNormalizeUrl_KeepsNonDefaultPort() {
         );
     }
 
+    // This test does not assert the exact full string. Its purpose is to verify that IPv6 literals are correctly bracketed,
+    // while the path and other components are covered by separate tests.
     @Test
     void testNormalizeUrl_Ipv6HostIsBracketed() {
         String normalized = AvatarContributor.normalizeUrl("https://[2001:db8::1]/a.png");
         assertThat(normalized, startsWith("https://[2001:db8::1]"));
         assertThat(normalized, containsString("/a.png"));
     }
 
+    // Verifies that the Unicode hostname is converted to its ASCII representation while preserving the
+    // rest of the URL structure.
     @Test
     void testNormalizeUrl_IdnIsConvertedToAscii() {
         String normalized = AvatarContributor.normalizeUrl("https://例え.テスト/a.png");
         assertThat(normalized, startsWith("https://xn--r8jz45g.xn--zckzah"));
     }
 
+    @Test
+    void testAddNormalizedUrl_AddsValidUrl() {
+        Set<String> set = new HashSet<>();
+
+        boolean added = AvatarContributor.addNormalizedUrl("https://example.com/a.png", set);
+
+        assertThat(added, is(true));
+        assertThat(set, contains("https://example.com/a.png"));
+    }
+
+    @Test
+    void testAddNormalizedUrl_RejectsInvalidUrl() {
+        Set<String> set = new HashSet<>();
+
+        boolean added = AvatarContributor.addNormalizedUrl("ftp://example.com/a.png", set);
+
+        assertThat(added, is(false));
+        assertThat(set, empty());
+    }
+
+    @Test
+    void testAddNormalizedUrl_Deduplicates() {
+        Set<String> set = new HashSet<>();
+
+        AvatarContributor.addNormalizedUrl("https://example.com/a.png", set);
+        boolean addedAgain = AvatarContributor.addNormalizedUrl("https://example.com/a.png", set);
+
+        assertThat(addedAgain, is(false));
+        assertThat(set.size(), is(1));
+    }
+
 }