Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Umbraco exposes all tags publicly by default #5206

Closed
ronaldbarendse opened this issue Apr 9, 2019 · 13 comments

Comments

@ronaldbarendse
Copy link
Contributor

commented Apr 9, 2019

Umbraco provides a TagsController (an UmbracoApiController) by default that allows to publicly query all tags within the site with a simple GET-request, e.g. https://umbraco.com/umbraco/api/tags/GetAllTags.

The controller is available in both Umbraco 7 & 8, although it is moved in V8:

The following remarks are added in the source, so I don't think this is a BIG security issue (that's why I'm creating this as an issue):

This controller does not contain methods to query for content, media or members based on tags, those methods would require authentication and should not be exposed publicly.

As tags could still expose information that shoudn't be public (e.g. classifications on media or members) and I don't see why Umbraco would include this controller by default, I would recommend removing this controller.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Tags are used for published content and as such are available publicly but that said i do understand that in some rare situations it isn't ideal to have this data public. Removing this controller would result in breaking changes but it is possible for you to remove this controller from the resolvers/containers on startup so it is not auto routed. This controller has been around since 2013 and although i made it, i can't recall the exact reason but I think it was during a trial of allowing easier 'headless' style data access.

So the questions is - how do we remove this? I think we will need to have an opt-in config flag that disables this controller from being routed. In new releases we will ship with this config flag so it's not routed which means for upgrades people will not be affected but they can opt-in with the config switch.

@ronaldbarendse

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

I would argue removing the controller, but marking it as a breaking change would be the best option. If the controller is used in a website (which is unlikely, as this isn't documented), copying the controller to the website project is enough to restore it...

If you could provide an example of how to remove this controller on startup, a temporary 'fix' could be implemented for those that are also concerned about exposing all tags 👍

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

I wrote a quick blog post about how to remove routes during startup so this can be achieved: https://shazwazza.com/post/need-to-remove-an-auto-routed-controller-in-umbraco/

Will chat to the team about this controller soon and get back to you

Cheers!

@ronaldbarendse

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Thanks @Shazwazza!

I've checked the Umbraco source and instead of removing routes based on URL (which might even remove other, custom, routes, since it's quite greedy with the contains check), doing this based on route name seems like a better solution:

RouteTable.Routes.Remove(RouteTable.Routes["umbraco-api-Tags"]);
RouteTable.Routes.Remove(RouteTable.Routes["umbraco-surface-UmbRegister"]);

FYI Route names are case insensitive and if the route name doesn't exist, Remove happily ignores it.

@ronaldbarendse

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

The route names are hard-coded in the RouteLocalApiController and RouteLocalSurfaceController methods of WebBootManager (V7) or WebFinalComponent (V8), so these shoudn't change.

Umbraco could even add an extension method (for convenience) to remove auto-routed controllers, e.g. RouteTable.Routes.Remove<TagsController>() that checks the type (API/Surface, using genetic type constraints), gets the route name based on the same logic as it's added and then removes it...

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Nice! I can update my blog at some stage with that too.

In v8 you can actually remove the TagsController, etc.. by type from the composers and they won't be auto routed at all but in v7 you can't do that so thought I'd write a similar solution that works for both.

But yes having an extension method on routes would be nice too, happy to accept a PR if you have time too :)

@ronaldbarendse

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Looking at WebBootManager, removing automatically registered surface and API controllers could also be done by removing the types from SurfaceControllerResolver.Current.RegisteredSurfaceControllers or UmbracoApiControllerResolver.Current.RegisteredUmbracoApiControllers (from within a ApplicationStarting event). That way it doesn't even try to add the controller to the route table and doesn't rely on the 'magic' route names...

These classes are marked internal, so would making these public be a problem?

@Shazwazza

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Hi, yup i was going to do that originally as the work around but because they are internal in v7 the work around won't work for existing versions which is why I posted how to do it by modifying the routes. But yes, it would work if they are public, and they are in v8 already so that will certainly work too. If you want to make these public in v7 i'd be happy to merge that in.

@ronaldbarendse

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

@Shazwazza Looks like making SurfaceControllerResolver and UmbracoApiControllerResolver publicly won't make it into the last feature update (7.15) anymore and the ability to remove controllers from a composition in V8 is only possible in 8.0.1+ (because of #4874).

So currently the best method for V7 would be as I described earlier (#5206 (comment)).

For 8.0.1+ this can be done with collection builders within a composer:

composition.WithCollectionBuilder<UmbracoApiControllerTypeCollectionBuilder>().Remove<TagsController>();
composition.WithCollectionBuilder<SurfaceControllerTypeCollectionBuilder>().Remove<UmbRegisterController>();
@ronaldbarendse

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

@Shazwazza @zpqrtbnk After removing the publicly available TagsController, the back-office throws the following exception:

[InvalidOperationException: Could not find the umbraco api controller of type Umbraco.Web.Controllers.TagsController]
   Umbraco.Web.UrlHelperExtensions.GetUmbracoApiService(UrlHelper url, String actionName, Type apiControllerType, RouteValueDictionary routeVals) +384
   Umbraco.Web.UrlHelperExtensions.GetUmbracoApiService(UrlHelper url, String actionName, RouteValueDictionary routeVals) +53
   Umbraco.Web.UrlHelperExtensions.GetUmbracoApiServiceBaseUrl(UrlHelper url, Expression`1 methodSelector) +123
   Umbraco.Web.Editors.BackOfficeServerVariables.GetServerVariables() +8720
   Umbraco.Web.Editors.BackOfficeServerVariables.BareMinimumServerVariables() +431
   Umbraco.Web.HtmlHelperBackOfficeExtensions.BareMinimumServerVariablesScript(HtmlHelper html, UrlHelper uri, String externalLoginsUrl, UmbracoFeatures features, IGlobalSettings globalSettings) +63
   ASP._Page_umbraco_Views_Default_cshtml.Execute() in D:\Projects\Umbraco8\Umbraco8\Umbraco8\umbraco\Views\Default.cshtml:115

This is because the BackOfficeServerVariables adds this API controller to the umbracoUrls:

{
"tagApiBaseUrl", _urlHelper.GetUmbracoApiServiceBaseUrl<TagsController>(
controller => controller.GetAllTags(null))
},

It also adds an URL to the TagsDataController (which correctly inherits from UmbracoAuthorizedApiController and is actually used in the umbTagsEditor):

{
"tagsDataBaseUrl", _urlHelper.GetUmbracoApiServiceBaseUrl<TagsDataController>(
controller => controller.GetTags("", "", null))
},

@azure-devops-sync

This comment has been minimized.

Copy link

commented Jun 4, 2019

This item has been added to our backlog AB#1237

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Fixed in 5bf29a9 for v7 and merged up to v8 too.

I've marked it as a breaking change since we're removing a public endpoint. As far as we can tell nobody is actually using this endpoint, at least no packages seem to be using it. We can't be sure individuals aren't using it. Therefore this change is not backwards compatible.

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Note: I've tested the workaround in both 7.14 and 8.0.2 by using RouteTable.Routes.Remove(RouteTable.Routes["umbraco-api-Tags"]); and that doesn't break anything since we're not actually removing the controller from being registered, just making sure it's not routable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.