From 5167825ad1792d9a5871dbfb35fee5156aeefc34 Mon Sep 17 00:00:00 2001 From: Anatol Sialitski Date: Fri, 17 Jan 2025 14:51:19 +0100 Subject: [PATCH 1/2] `/admin/tool` must return 404 #10861 --- .../xp/portal/impl/url/PortalUrlBuilder.java | 13 +++++ .../impl/url/UniversalApiUrlBuilder.java | 49 +++++++++++-------- .../url/PortalUrlServiceImpl_apiUrlTest.java | 18 +++++++ 3 files changed, 60 insertions(+), 20 deletions(-) diff --git a/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/PortalUrlBuilder.java b/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/PortalUrlBuilder.java index 2336b2aae96..4108c5d8971 100644 --- a/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/PortalUrlBuilder.java +++ b/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/PortalUrlBuilder.java @@ -23,6 +23,8 @@ import com.enonic.xp.portal.url.UrlTypeConstants; import com.enonic.xp.repository.RepositoryUtils; import com.enonic.xp.resource.ResourceService; +import com.enonic.xp.web.HttpStatus; +import com.enonic.xp.web.WebException; import com.enonic.xp.web.servlet.ServletRequestUrlHelper; import com.enonic.xp.web.servlet.UriRewritingResult; @@ -67,6 +69,17 @@ public final String build() { return doBuild(); } + catch ( WebException e ) + { + if ( e.getStatus() == HttpStatus.NOT_FOUND ) + { + throw e; + } + else + { + return buildErrorUrl( e ); + } + } catch ( final Exception e ) { return buildErrorUrl( e ); diff --git a/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/UniversalApiUrlBuilder.java b/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/UniversalApiUrlBuilder.java index fd7361923c3..e80857fdf70 100644 --- a/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/UniversalApiUrlBuilder.java +++ b/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/UniversalApiUrlBuilder.java @@ -10,6 +10,8 @@ import com.enonic.xp.portal.impl.ContentResolverResult; import com.enonic.xp.portal.url.ApiUrlParams; import com.enonic.xp.site.Site; +import com.enonic.xp.web.HttpStatus; +import com.enonic.xp.web.WebException; import static com.enonic.xp.portal.impl.url.UrlBuilderHelper.appendPathSegments; import static com.enonic.xp.portal.impl.url.UrlBuilderHelper.appendSubPath; @@ -45,29 +47,36 @@ protected void buildUrl( final StringBuilder url, final Multimap final String requestURI = this.portalRequest.getRawRequest().getRequestURI(); - if ( requestURI.equals( ADMIN_PREFIX ) ) + try { - processHome( url ); - } - else if ( requestURI.startsWith( ADMIN_SITE_PREFIX ) ) - { - processSite( url, requestURI, true ); - } - else if ( requestURI.startsWith( TOOL_PREFIX ) ) - { - processTool( url, requestURI ); - } - else if ( requestURI.startsWith( SITE_PREFIX ) ) - { - processSite( url, requestURI, false ); - } - else if ( requestURI.startsWith( WEBAPP_PREFIX ) ) - { - processWebapp( url, requestURI ); + if ( requestURI.equals( ADMIN_PREFIX ) ) + { + processHome( url ); + } + else if ( requestURI.startsWith( ADMIN_SITE_PREFIX ) ) + { + processSite( url, requestURI, true ); + } + else if ( requestURI.startsWith( TOOL_PREFIX ) ) + { + processTool( url, requestURI ); + } + else if ( requestURI.startsWith( SITE_PREFIX ) ) + { + processSite( url, requestURI, false ); + } + else if ( requestURI.startsWith( WEBAPP_PREFIX ) ) + { + processWebapp( url, requestURI ); + } + else + { + appendPart( url, "api" ); + } } - else + catch ( IllegalArgumentException e ) { - appendPart( url, "api" ); + throw new WebException( HttpStatus.NOT_FOUND, "Mount not found", e ); } if ( !requestURI.startsWith( API_PREFIX ) ) diff --git a/modules/portal/portal-impl/src/test/java/com/enonic/xp/portal/impl/url/PortalUrlServiceImpl_apiUrlTest.java b/modules/portal/portal-impl/src/test/java/com/enonic/xp/portal/impl/url/PortalUrlServiceImpl_apiUrlTest.java index e697d6953a8..654429d14f4 100644 --- a/modules/portal/portal-impl/src/test/java/com/enonic/xp/portal/impl/url/PortalUrlServiceImpl_apiUrlTest.java +++ b/modules/portal/portal-impl/src/test/java/com/enonic/xp/portal/impl/url/PortalUrlServiceImpl_apiUrlTest.java @@ -7,8 +7,11 @@ import com.enonic.xp.content.ContentPath; import com.enonic.xp.portal.url.ApiUrlParams; import com.enonic.xp.site.Site; +import com.enonic.xp.web.HttpStatus; +import com.enonic.xp.web.WebException; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -67,6 +70,21 @@ void testCreateUrlAdminToolWithAppFromRequest() assertEquals( "/admin/myapplication/toolname/_/com.enonic.app.myapp:myapi", url ); } + @Test + void testCreateUrlMountNotFound() + { + when( portalRequest.getRawRequest().getRequestURI() ).thenReturn( "/admin/toolname" ); + + final ApiUrlParams params = new ApiUrlParams(); + params.portalRequest( this.portalRequest ); + params.application( "com.enonic.app.myapp" ); + params.api( "myapi" ); + + WebException ex = assertThrows( WebException.class, () -> this.service.apiUrl( params ) ); + assertEquals( "Mount not found", ex.getMessage() ); + assertEquals( HttpStatus.NOT_FOUND, ex.getStatus() ); + } + @Test void testCreateUrlAdminSite() { From f67caa6a7cccc29ba24ece347c3a992e10a654b6 Mon Sep 17 00:00:00 2001 From: Anatol Sialitski Date: Mon, 20 Jan 2025 13:29:46 +0100 Subject: [PATCH 2/2] `/admin/tool` must return 404 #10861 --- .../admin/impl/portal/AdminToolHandler.java | 11 +++++ .../impl/portal/AdminToolHandlerTest.java | 48 ++++++++++++------ .../xp/portal/impl/url/PortalUrlBuilder.java | 13 ----- .../impl/url/UniversalApiUrlBuilder.java | 49 ++++++++----------- .../url/PortalUrlServiceImpl_apiUrlTest.java | 18 ------- 5 files changed, 65 insertions(+), 74 deletions(-) diff --git a/modules/admin/admin-impl/src/main/java/com/enonic/xp/admin/impl/portal/AdminToolHandler.java b/modules/admin/admin-impl/src/main/java/com/enonic/xp/admin/impl/portal/AdminToolHandler.java index 55d8a034d59..82383f21026 100644 --- a/modules/admin/admin-impl/src/main/java/com/enonic/xp/admin/impl/portal/AdminToolHandler.java +++ b/modules/admin/admin-impl/src/main/java/com/enonic/xp/admin/impl/portal/AdminToolHandler.java @@ -1,5 +1,7 @@ package com.enonic.xp.admin.impl.portal; +import java.util.regex.Pattern; + import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Reference; @@ -12,6 +14,7 @@ import com.enonic.xp.portal.handler.WebHandlerHelper; import com.enonic.xp.trace.Trace; import com.enonic.xp.trace.Tracer; +import com.enonic.xp.web.WebException; import com.enonic.xp.web.WebRequest; import com.enonic.xp.web.WebResponse; import com.enonic.xp.web.handler.BaseWebHandler; @@ -22,6 +25,8 @@ public final class AdminToolHandler extends BaseWebHandler { + private static final Pattern TOOL_CXT_PATTERN = Pattern.compile( "^/admin/([^/]+)/([^/]+)" ); + private AdminToolDescriptorService adminToolDescriptorService; private ControllerScriptFactory controllerScriptFactory; @@ -43,6 +48,12 @@ protected boolean canHandle( final WebRequest webRequest ) protected WebResponse doHandle( final WebRequest webRequest, final WebResponse webResponse, final WebHandlerChain webHandlerChain ) throws Exception { + final String rawPath = webRequest.getRawPath(); + if ( !( rawPath.equals( "/admin" ) || rawPath.equals( "/admin/" ) ) && !TOOL_CXT_PATTERN.matcher( rawPath ).find() ) + { + throw WebException.notFound( "Invalid admin tool mount" ); + } + WebHandlerHelper.checkAdminAccess( webRequest ); PortalRequest portalRequest = (PortalRequest) webRequest; diff --git a/modules/admin/admin-impl/src/test/java/com/enonic/xp/admin/impl/portal/AdminToolHandlerTest.java b/modules/admin/admin-impl/src/test/java/com/enonic/xp/admin/impl/portal/AdminToolHandlerTest.java index 4afe37440ee..51c0ae11a58 100644 --- a/modules/admin/admin-impl/src/test/java/com/enonic/xp/admin/impl/portal/AdminToolHandlerTest.java +++ b/modules/admin/admin-impl/src/test/java/com/enonic/xp/admin/impl/portal/AdminToolHandlerTest.java @@ -16,6 +16,7 @@ import com.enonic.xp.portal.controller.ControllerScriptFactory; import com.enonic.xp.resource.ResourceKey; import com.enonic.xp.security.PrincipalKeys; +import com.enonic.xp.web.HttpStatus; import com.enonic.xp.web.WebException; import com.enonic.xp.web.WebResponse; import com.enonic.xp.web.handler.BaseHandlerTest; @@ -24,6 +25,9 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class AdminToolHandlerTest extends BaseHandlerTest @@ -47,21 +51,21 @@ public class AdminToolHandlerTest public final void setup() { - this.adminToolDescriptorService = Mockito.mock( AdminToolDescriptorService.class ); - ControllerScript controllerScript = Mockito.mock( ControllerScript.class ); + this.adminToolDescriptorService = mock( AdminToolDescriptorService.class ); + ControllerScript controllerScript = mock( ControllerScript.class ); this.portalResponse = PortalResponse.create().build(); - Mockito.when( controllerScript.execute( Mockito.any( PortalRequest.class ) ) ).thenReturn( this.portalResponse ); + when( controllerScript.execute( any( PortalRequest.class ) ) ).thenReturn( this.portalResponse ); - final ControllerScriptFactory controllerScriptFactory = Mockito.mock( ControllerScriptFactory.class ); - Mockito.when( controllerScriptFactory.fromScript( Mockito.any( ResourceKey.class ) ) ).thenReturn( controllerScript ); + final ControllerScriptFactory controllerScriptFactory = mock( ControllerScriptFactory.class ); + when( controllerScriptFactory.fromScript( any( ResourceKey.class ) ) ).thenReturn( controllerScript ); this.handler = new AdminToolHandler(); this.handler.setAdminToolDescriptorService( this.adminToolDescriptorService ); this.handler.setControllerScriptFactory( controllerScriptFactory ); - this.rawRequest = Mockito.mock( HttpServletRequest.class ); - Mockito.when( this.rawRequest.isUserInRole( Mockito.anyString() ) ).thenReturn( true ); + this.rawRequest = mock( HttpServletRequest.class ); + when( this.rawRequest.isUserInRole( Mockito.anyString() ) ).thenReturn( true ); this.portalRequest = new PortalRequest(); this.portalRequest.setRawRequest( this.rawRequest ); @@ -73,7 +77,7 @@ public final void setup() this.webResponse = WebResponse.create().build(); - this.chain = Mockito.mock( WebHandlerChain.class ); + this.chain = mock( WebHandlerChain.class ); } @Test @@ -87,14 +91,14 @@ public void testCanHandle() public void testWithoutPermissions() { this.portalRequest.setRawPath( "/admin/webapp/tool/1" ); - Mockito.when( this.rawRequest.isUserInRole( Mockito.anyString() ) ).thenReturn( false ); + when( this.rawRequest.isUserInRole( Mockito.anyString() ) ).thenReturn( false ); assertThrows( WebException.class, () -> this.handler.doHandle( this.portalRequest, this.webResponse, this.chain ) ); } @Test public void testWithNoDescriptor() { - Mockito.when( this.adminToolDescriptorService.getByKey( Mockito.any( DescriptorKey.class ) ) ).thenReturn( null ); + when( this.adminToolDescriptorService.getByKey( any( DescriptorKey.class ) ) ).thenReturn( null ); this.portalRequest.setRawPath( "/admin/webapp/tool/1" ); assertThrows( WebException.class, () -> this.handler.doHandle( this.portalRequest, this.webResponse, this.chain ) ); } @@ -119,11 +123,27 @@ public void test() assertEquals( "/admin/webapp/tool", this.portalRequest.getContextPath() ); } + @Test + void testInvalidAdminToolMount() + { + this.portalRequest.setBaseUri( "/admin" ); + this.portalRequest.setRawPath( "/admin/tool" ); + WebException ex = + assertThrows( WebException.class, () -> this.handler.doHandle( this.portalRequest, this.webResponse, this.chain ) ); + assertEquals( HttpStatus.NOT_FOUND, ex.getStatus() ); + assertEquals( "Invalid admin tool mount", ex.getMessage() ); + + this.portalRequest.setRawPath( "/admin/tool/" ); + ex = assertThrows( WebException.class, () -> this.handler.doHandle( this.portalRequest, this.webResponse, this.chain ) ); + assertEquals( HttpStatus.NOT_FOUND, ex.getStatus() ); + assertEquals( "Invalid admin tool mount", ex.getMessage() ); + } + private void mockDescriptor( DescriptorKey descriptorKey, boolean hasAccess ) { - AdminToolDescriptor descriptor = Mockito.mock( AdminToolDescriptor.class ); - Mockito.when( descriptor.getKey() ).thenReturn( descriptorKey ); - Mockito.when( descriptor.isAccessAllowed( Mockito.any( PrincipalKeys.class ) ) ).thenReturn( hasAccess ); - Mockito.when( this.adminToolDescriptorService.getByKey( Mockito.any( DescriptorKey.class ) ) ).thenReturn( descriptor ); + AdminToolDescriptor descriptor = mock( AdminToolDescriptor.class ); + when( descriptor.getKey() ).thenReturn( descriptorKey ); + when( descriptor.isAccessAllowed( any( PrincipalKeys.class ) ) ).thenReturn( hasAccess ); + when( this.adminToolDescriptorService.getByKey( any( DescriptorKey.class ) ) ).thenReturn( descriptor ); } } diff --git a/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/PortalUrlBuilder.java b/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/PortalUrlBuilder.java index 4108c5d8971..2336b2aae96 100644 --- a/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/PortalUrlBuilder.java +++ b/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/PortalUrlBuilder.java @@ -23,8 +23,6 @@ import com.enonic.xp.portal.url.UrlTypeConstants; import com.enonic.xp.repository.RepositoryUtils; import com.enonic.xp.resource.ResourceService; -import com.enonic.xp.web.HttpStatus; -import com.enonic.xp.web.WebException; import com.enonic.xp.web.servlet.ServletRequestUrlHelper; import com.enonic.xp.web.servlet.UriRewritingResult; @@ -69,17 +67,6 @@ public final String build() { return doBuild(); } - catch ( WebException e ) - { - if ( e.getStatus() == HttpStatus.NOT_FOUND ) - { - throw e; - } - else - { - return buildErrorUrl( e ); - } - } catch ( final Exception e ) { return buildErrorUrl( e ); diff --git a/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/UniversalApiUrlBuilder.java b/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/UniversalApiUrlBuilder.java index e80857fdf70..fd7361923c3 100644 --- a/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/UniversalApiUrlBuilder.java +++ b/modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/url/UniversalApiUrlBuilder.java @@ -10,8 +10,6 @@ import com.enonic.xp.portal.impl.ContentResolverResult; import com.enonic.xp.portal.url.ApiUrlParams; import com.enonic.xp.site.Site; -import com.enonic.xp.web.HttpStatus; -import com.enonic.xp.web.WebException; import static com.enonic.xp.portal.impl.url.UrlBuilderHelper.appendPathSegments; import static com.enonic.xp.portal.impl.url.UrlBuilderHelper.appendSubPath; @@ -47,36 +45,29 @@ protected void buildUrl( final StringBuilder url, final Multimap final String requestURI = this.portalRequest.getRawRequest().getRequestURI(); - try + if ( requestURI.equals( ADMIN_PREFIX ) ) { - if ( requestURI.equals( ADMIN_PREFIX ) ) - { - processHome( url ); - } - else if ( requestURI.startsWith( ADMIN_SITE_PREFIX ) ) - { - processSite( url, requestURI, true ); - } - else if ( requestURI.startsWith( TOOL_PREFIX ) ) - { - processTool( url, requestURI ); - } - else if ( requestURI.startsWith( SITE_PREFIX ) ) - { - processSite( url, requestURI, false ); - } - else if ( requestURI.startsWith( WEBAPP_PREFIX ) ) - { - processWebapp( url, requestURI ); - } - else - { - appendPart( url, "api" ); - } + processHome( url ); + } + else if ( requestURI.startsWith( ADMIN_SITE_PREFIX ) ) + { + processSite( url, requestURI, true ); + } + else if ( requestURI.startsWith( TOOL_PREFIX ) ) + { + processTool( url, requestURI ); } - catch ( IllegalArgumentException e ) + else if ( requestURI.startsWith( SITE_PREFIX ) ) + { + processSite( url, requestURI, false ); + } + else if ( requestURI.startsWith( WEBAPP_PREFIX ) ) + { + processWebapp( url, requestURI ); + } + else { - throw new WebException( HttpStatus.NOT_FOUND, "Mount not found", e ); + appendPart( url, "api" ); } if ( !requestURI.startsWith( API_PREFIX ) ) diff --git a/modules/portal/portal-impl/src/test/java/com/enonic/xp/portal/impl/url/PortalUrlServiceImpl_apiUrlTest.java b/modules/portal/portal-impl/src/test/java/com/enonic/xp/portal/impl/url/PortalUrlServiceImpl_apiUrlTest.java index 654429d14f4..e697d6953a8 100644 --- a/modules/portal/portal-impl/src/test/java/com/enonic/xp/portal/impl/url/PortalUrlServiceImpl_apiUrlTest.java +++ b/modules/portal/portal-impl/src/test/java/com/enonic/xp/portal/impl/url/PortalUrlServiceImpl_apiUrlTest.java @@ -7,11 +7,8 @@ import com.enonic.xp.content.ContentPath; import com.enonic.xp.portal.url.ApiUrlParams; import com.enonic.xp.site.Site; -import com.enonic.xp.web.HttpStatus; -import com.enonic.xp.web.WebException; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -70,21 +67,6 @@ void testCreateUrlAdminToolWithAppFromRequest() assertEquals( "/admin/myapplication/toolname/_/com.enonic.app.myapp:myapi", url ); } - @Test - void testCreateUrlMountNotFound() - { - when( portalRequest.getRawRequest().getRequestURI() ).thenReturn( "/admin/toolname" ); - - final ApiUrlParams params = new ApiUrlParams(); - params.portalRequest( this.portalRequest ); - params.application( "com.enonic.app.myapp" ); - params.api( "myapi" ); - - WebException ex = assertThrows( WebException.class, () -> this.service.apiUrl( params ) ); - assertEquals( "Mount not found", ex.getMessage() ); - assertEquals( HttpStatus.NOT_FOUND, ex.getStatus() ); - } - @Test void testCreateUrlAdminSite() {