From 369a00cb717b7c14839862d51704b1f15d52dfd0 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Wed, 18 Mar 2015 10:35:57 +0000 Subject: [PATCH] Validate hostname (serviceId) in @FeignClient Since hostnames ahve quite strict syntax (e.g. no underscores) we should validate the feign client name to prevent nasty surprises at runtime. Fixes gh-263 --- .../netflix/feign/FeignClientsRegistrar.java | 19 +++++- .../RibbonClientConfigurationRegistrar.java | 9 ++- .../invalid/FeignClientValidationTests.java | 60 +++++++++++++++++++ .../valid/FeignClientValidationTests.java | 54 +++++++++++++++++ 4 files changed, 134 insertions(+), 8 deletions(-) create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/invalid/FeignClientValidationTests.java create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientValidationTests.java diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientsRegistrar.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientsRegistrar.java index 4d7ac511..ca1704d4 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientsRegistrar.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/FeignClientsRegistrar.java @@ -17,6 +17,8 @@ package org.springframework.cloud.netflix.feign; import java.lang.annotation.Annotation; +import java.net.URI; +import java.net.URISyntaxException; import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -117,10 +119,21 @@ public class FeignClientsRegistrar implements ImportBeanDefinitionRegistrar, } private String getServiceId(Map attributes) { - if (StringUtils.hasText((String) attributes.get("name"))) { - return (String) attributes.get("name"); + String name = (String) attributes.get("name"); + if (!StringUtils.hasText(name)) { + name = (String) attributes.get("value"); } - return (String) attributes.get("value"); + if (!StringUtils.hasText(name)) { + return ""; + } + String host = null; + try { + host = new URI("http://" + name).getHost(); + } + catch (URISyntaxException e) { + } + Assert.state(host != null, "Service id not legal hostname (" + name + ")"); + return name; } private String getUrl(Map attributes) { diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationRegistrar.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationRegistrar.java index 44a2ff96..8465f5b7 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationRegistrar.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationRegistrar.java @@ -38,7 +38,7 @@ public class RibbonClientConfigurationRegistrar implements ImportBeanDefinitionR if (attrs != null && attrs.containsKey("value")) { AnnotationAttributes[] clients = (AnnotationAttributes[]) attrs.get("value"); for (AnnotationAttributes client : clients) { - registerClientConfiguration(registry, client.get("name"), + registerClientConfiguration(registry, getClientName(client), client.get("configuration")); } } @@ -60,11 +60,10 @@ public class RibbonClientConfigurationRegistrar implements ImportBeanDefinitionR return null; } String value = (String) client.get("value"); - if (value != null && StringUtils.hasText(value)) { - return value; + if (!StringUtils.hasText(value)) { + value = (String) client.get("name"); } - value = (String) client.get("name"); - if (value != null && StringUtils.hasText(value)) { + if (StringUtils.hasText(value)) { return value; } throw new IllegalStateException( diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/invalid/FeignClientValidationTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/invalid/FeignClientValidationTests.java new file mode 100644 index 00000000..2c152d8d --- /dev/null +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/invalid/FeignClientValidationTests.java @@ -0,0 +1,60 @@ +/* + * Copyright 2013-2015 the original author or authors. + * + * 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 org.springframework.cloud.netflix.feign.invalid; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.springframework.cloud.netflix.feign.EnableFeignClients; +import org.springframework.cloud.netflix.feign.FeignClient; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.annotation.Configuration; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; + +import static org.junit.Assert.assertNotNull; + +/** + * @author Dave Syer + */ +public class FeignClientValidationTests { + + @Rule + public ExpectedException expected = ExpectedException.none(); + + @Test + public void invalid() { + this.expected.expectMessage("not legal hostname (foo_bar)"); + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( + BadConfiguration.class); + assertNotNull(context.getBean(BadConfiguration.Client.class)); + context.close(); + } + + @Configuration + @EnableFeignClients() + protected static class BadConfiguration { + + @FeignClient("foo_bar") + interface Client { + @RequestMapping(method = RequestMethod.GET, value = "/") + String get(); + } + + } + +} diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientValidationTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientValidationTests.java new file mode 100644 index 00000000..2d96c24a --- /dev/null +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/valid/FeignClientValidationTests.java @@ -0,0 +1,54 @@ +/* + * Copyright 2013-2015 the original author or authors. + * + * 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 org.springframework.cloud.netflix.feign.valid; + +import org.junit.Test; +import org.springframework.cloud.netflix.feign.EnableFeignClients; +import org.springframework.cloud.netflix.feign.FeignClient; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.annotation.Configuration; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; + +import static org.junit.Assert.assertNotNull; + +/** + * @author Dave Syer + */ +public class FeignClientValidationTests { + + @Test + public void valid() { + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( + GoodConfiguration.class); + assertNotNull(context.getBean(GoodConfiguration.Client.class)); + context.close(); + } + + @Configuration + @EnableFeignClients + protected static class GoodConfiguration { + + @FeignClient("foo") + interface Client { + @RequestMapping(method = RequestMethod.GET, value = "/") + String get(); + } + + } + +}