Include requested URI config in ListenerConfig; fix config keys (heli…

…don-io#8371)

* Include requested URI config in ListenerConfig; fix config keys

* Forward-port requested URI discovery tests from 3.x

* Review comments

http/http/pom.xml

@@ -60,6 +60,16 @@
             <artifactId>helidon-config-metadata</artifactId>
             <optional>true</optional>
         </dependency>
+        <dependency>
+            <groupId>io.helidon.config</groupId>
+            <artifactId>helidon-config</artifactId>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>io.helidon.config</groupId>
+            <artifactId>helidon-config-yaml</artifactId>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>io.helidon.common.testing</groupId>
             <artifactId>helidon-common-testing-junit5</artifactId>

http/http/src/main/java/io/helidon/http/RequestedUriDiscoveryContext.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2023, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -93,7 +93,7 @@ final class Builder implements io.helidon.common.Builder<RequestedUriDiscoveryCo
         private Boolean enabled;
         private final List<RequestedUriDiscoveryType> discoveryTypes = new ArrayList<>();
         private AllowList trustedProxies;
-        private String socketId;
+        private String socketId = "@default";
 
         private Builder() {
         }
@@ -115,9 +115,14 @@ public Builder config(Config requestedUriDiscoveryConfig) {
             requestedUriDiscoveryConfig.get("enabled")
                     .as(Boolean.class)
                     .ifPresent(this::enabled);
+            // TODO - discoveryTypes as a key was never documented but was hand-coded this way. Keep for compatibility
+            // in case existing apps happen to use it. Remove as soon as practical.
             requestedUriDiscoveryConfig.get("discoveryTypes")
                     .asList(RequestedUriDiscoveryType.class)
                     .ifPresent(this::discoveryTypes);
+            requestedUriDiscoveryConfig.get("types")
+                    .asList(RequestedUriDiscoveryType.class)
+                    .ifPresent(this::types);
             requestedUriDiscoveryConfig.get("trusted-proxies")
                     .map(AllowList::create)
                     .ifPresent(this::trustedProxies);
@@ -130,7 +135,7 @@ public Builder config(Config requestedUriDiscoveryConfig) {
          * @param value new enabled state
          * @return updated builder
          */
-        @ConfiguredOption(value = "true if 'discoveryTypes' or 'trusted-proxies' is set; false otherwise")
+        @ConfiguredOption(value = "true if 'types' or 'trusted-proxies' is set; false otherwise")
         public Builder enabled(boolean value) {
             enabled = value;
             return this;
@@ -154,13 +159,25 @@ public Builder trustedProxies(AllowList trustedProxies) {
          * @param discoveryTypes discovery types to use
          * @return updated builder
          */
-        @ConfiguredOption
-        public Builder discoveryTypes(List<RequestedUriDiscoveryType> discoveryTypes) {
+        @ConfiguredOption()
+        public Builder types(List<RequestedUriDiscoveryType> discoveryTypes) {
             this.discoveryTypes.clear();
             this.discoveryTypes.addAll(discoveryTypes);
             return this;
         }
 
+        /**
+         * Sets the discovery types for requested URI discovery for requests arriving on the socket.
+         *
+         * @param discoveryTypes discovery types to use
+         * @return updated builder
+         * @deprecated Use {@link #types(java.util.List)} instead
+         */
+        @Deprecated(since = "4.0.6", forRemoval = true)
+        public Builder discoveryTypes(List<RequestedUriDiscoveryType> discoveryTypes) {
+            return types(discoveryTypes);
+        }
+
         /**
          * Adds a discovery type for requested URI discovery for requests arriving on the socket.
          *
@@ -202,9 +219,6 @@ public Builder socketId(String socketId) {
          * </p>
          */
         private void prepareAndCheckRequestedUriSettings() {
-            if (socketId == null) {
-                throw new IllegalArgumentException("Required socket ID not specified");
-            }
             boolean isDiscoveryEnabledDefaulted = (enabled == null);
             if (enabled == null) {
                 enabled = !discoveryTypes.isEmpty() || trustedProxies != null;

http/http/src/test/java/io/helidon/http/RequestedUriConfigTest.java

@@ -0,0 +1,79 @@
+/*
+ * Copyright (c) 2024 Oracle and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.helidon.http;
+
+import io.helidon.config.Config;
+import io.helidon.config.ConfigSources;
+import io.helidon.http.RequestedUriDiscoveryContext.RequestedUriDiscoveryType;
+import io.helidon.http.RequestedUriDiscoveryContext.UnsafeRequestedUriSettingsException;
+
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import static io.helidon.http.RequestedUriDiscoveryContext.Builder.REQUESTED_URI_DISCOVERY_CONFIG_KEY;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class RequestedUriConfigTest {
+
+    private static Config config;
+
+    @BeforeAll
+    static void initConfig() {
+        config = Config.just(ConfigSources.classpath("/requestUriDiscovery.yaml"));
+    }
+
+    @Test
+    void checkUnsafeDetectionOnEnable() {
+        Config c = config.get("test-enabled-no-details.server." + REQUESTED_URI_DISCOVERY_CONFIG_KEY);
+        assertThrows(UnsafeRequestedUriSettingsException.class, () ->
+                             RequestedUriDiscoveryContext.builder()
+                                     .config(c)
+                                     .build(),
+                     "defaulted non-HOST discovery type with no proxy settings");
+    }
+
+    @Test
+    void checkExplicitTypesNoDetails() {
+        Config c = config.get("test-explicit-types-no-details.server." + REQUESTED_URI_DISCOVERY_CONFIG_KEY);
+        assertThrows(UnsafeRequestedUriSettingsException.class, () ->
+                             RequestedUriDiscoveryContext.builder()
+                                     .config(c)
+                                     .build(),
+                     "explicit non-HOST discovery types with no proxy settings");
+    }
+
+    @Test
+    void checkEnum() {
+        String v = "x-forwarded";
+        Class<?> eClass = RequestedUriDiscoveryType.class;
+        if (eClass.isEnum()) {
+            RequestedUriDiscoveryType type = null;
+            for (Object o : eClass.getEnumConstants()) {
+                if (((Enum<?>) o).name().equalsIgnoreCase((v.replace('-', '_')))) {
+                    type = (RequestedUriDiscoveryType) o;
+                    break;
+                }
+            }
+            assertThat("Mapped string to discovery type",
+                       type,
+                       allOf(notNullValue(),is(RequestedUriDiscoveryType.X_FORWARDED)));
+        }
+    }
+}

http/http/src/test/resources/requestUriDiscovery.yaml

@@ -0,0 +1,104 @@
+#
+# Copyright (c) 2023, 2024 Oracle and/or its affiliates.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# Should throw unsafe exception - defaulted non-HOST type without trusted proxy settings.
+test-enabled-no-details:
+  server:
+    port: 0
+    requested-uri-discovery:
+      enabled: true
+
+# Should be OK - HOST does not require trusted proxies.
+test-enabled-choose-host:
+  server:
+    port: 0
+    requested-uri-discovery:
+      types: host,forwarded
+      trusted-proxies:
+        allow:
+          exact: trust.com
+        deny:
+          exact: otherBadProxy.com
+
+
+# Should throw unsafe exception - non-HOST type without trusted proxy settings.
+test-explicit-types-no-details:
+  server:
+    port: 0
+    requested-uri-discovery:
+      types: forwarded,x-forwarded
+
+# Should be OK - trusted-proxies is set with non-HOST discovery type.
+test-defaulted-discovery-type:
+  server:
+    port: 0
+    requested-uri-discovery:
+      trusted-proxies:
+        allow:
+          exact: trust.com
+
+# Should reflect only the Forwarded header, not the X-Forwarded family.
+test-forwarded-only:
+  server:
+    port: 0
+    requested-uri-discovery:
+      types: forwarded
+      trusted-proxies:
+        allow:
+          all: true
+        deny:
+          exact: otherUntrustedProxy.com,untrustedProxy.com
+
+# Should reflect only the X-Forwarded headers, not Forwarded.
+test-x-forwarded-only:
+  server:
+    port: 0
+    requested-uri-discovery:
+      types: x-forwarded
+      trusted-proxies:
+        allow:
+          all: true
+        deny:
+          exact: otherUntrustedProxy.com,untrustedProxy.com
+
+# Should reflect only the X-Forwarded headers, not Forwarded.
+test-both-x-forwarded-first:
+  server:
+    port: 0
+    requested-uri-discovery:
+      types: x-forwarded,forwarded
+      trusted-proxies:
+        allow:
+          all: true
+        deny:
+          exact: otherUntrustedProxy.com,untrustedProxy.com
+
+# Should reflect only the X-Forwarded headers, not Forwarded.
+test-both-forwarded-first:
+  server:
+    port: 0
+    requested-uri-discovery:
+      types: forwarded,x-forwarded
+      trusted-proxies:
+        allow:
+          all: true
+        deny:
+          exact: otherUntrustedProxy.com,untrustedProxy.com
+
+# Should reflect the Host header by default.
+test-default-discovery:
+  server:
+    port: 0

webserver/http2/src/main/java/io/helidon/webserver/http2/Http2ServerRequest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2022, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -31,6 +31,7 @@
 import io.helidon.common.uri.UriQuery;
 import io.helidon.http.Header;
 import io.helidon.http.HttpPrologue;
+import io.helidon.http.RequestedUriDiscoveryContext;
 import io.helidon.http.RoutedPath;
 import io.helidon.http.ServerRequestHeaders;
 import io.helidon.http.WritableHeaders;
@@ -47,6 +48,11 @@
  * HTTP/2 server request.
  */
 class Http2ServerRequest implements RoutingRequest {
+    private static final RequestedUriDiscoveryContext DEFAULT_REQUESTED_URI_DISCOVERY_CONTEXT =
+            RequestedUriDiscoveryContext.builder()
+                    .build();
+
+
     private static final Runnable NO_OP_RUNNABLE = () -> {
     };
     private final Http2Headers http2Headers;
@@ -228,11 +234,13 @@ public Optional<ProxyProtocolData> proxyProtocolData() {
     }
 
     private UriInfo createUriInfo() {
-        return ctx.listenerContext().config().requestedUriDiscoveryContext().uriInfo(remotePeer().address().toString(),
-                                                                                     localPeer().address().toString(),
-                                                                                     path.path(),
-                                                                                     headers,
-                                                                                     query(),
-                                                                                     isSecure());
+        return ctx.listenerContext().config().requestedUriDiscoveryContext()
+                .orElse(DEFAULT_REQUESTED_URI_DISCOVERY_CONTEXT)
+                .uriInfo(remotePeer().address().toString(),
+                         localPeer().address().toString(),
+                         path.path(),
+                         headers,
+                         query(),
+                         isSecure());
     }
 }