Wrong URL rewrite using SiteMapPathHelper.SiteMapPath(this MvcSiteMapHtmlHelper helper) #278

sherif-elmetainy · 2014-02-11T22:55:49Z

Greetings,

First, as it's my first time posting here, I would like to thank you for this great component and for making it free. I love it :) Very good job.

I encountered what I think is a little bug using the component (I might be wrong).

First my environment:
I am using visual studio 2013, .NET 4.51, Mvc5. Since my project is big with many nodes, I created a smaller test project to reproduce the issue. the project is VS2013 MVC5 web application (default template) with authentication disabled.

My site map looks like this:

<?xml version="1.0" encoding="utf-8" ?>
<mvcSiteMap xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
            xmlns="http://mvcsitemap.codeplex.com/schemas/MvcSiteMap-File-4.0"
            xsi:schemaLocation="http://mvcsitemap.codeplex.com/schemas/MvcSiteMap-File-4.0 MvcSiteMapSchema.xsd">
  <mvcSiteMapNode title="Home" controller="Home" action="Index">
    <mvcSiteMapNode title="About" controller="Home" action="About" preservedRouteParameters="id">
      <mvcSiteMapNode title="Contact" controller="Home" action="Contact" type="x"/>
    </mvcSiteMapNode>
    <mvcSiteMapNode title="MyControllerPage" controller="My" action="Index" >
      <mvcSiteMapNode title="Hello" action="Hello" />
    </mvcSiteMapNode>
  </mvcSiteMapNode>
</mvcSiteMap>

My web.config is like this (with only parts relevant to MVC site map provider)

<appSettings>
    <add key="webpages:Version" value="3.0.0.0" />
    <add key="webpages:Enabled" value="false" />
    <add key="ClientValidationEnabled" value="true" />
    <add key="UnobtrusiveJavaScriptEnabled" value="true" />
    <add key="MvcSiteMapProvider_DefaultSiteMapNodeVisibiltyProvider" value="MvcSiteMapProvider.FilteredSiteMapNodeVisibilityProvider, MvcSiteMapProvider" />
    <add key="MvcSiteMapProvider_SecurityTrimmingEnabled" value="true" />
    <add key="MvcSiteMapProvider_IncludeAssembliesForScan" value="WebApplication4" />
    <add key="MvcSiteMapProvider_UseExternalDIContainer" value="false" />
    <add key="MvcSiteMapProvider_ScanAssembliesForSiteMapNodes" value="true" />
  </appSettings>
<system.web>
    <compilation debug="true" targetFramework="4.5.1" />
    <httpRuntime targetFramework="4.5.1" />
    <pages>
      <namespaces>
        <add namespace="MvcSiteMapProvider.Web.Html" />
        <add namespace="MvcSiteMapProvider.Web.Html.Models" />
      </namespaces>
    </pages>
  </system.web>

My _Layout.cshtml looks like this (irrelevant parts removed)

<body>
    @Request.Url.ToString()<br />

    @Html.MvcSiteMap().SiteMapPath() <br />

    @Request.Url.ToString()
......
</body>

Note that node /home/contact in the site map has an attribute called type.

Now when I browse to any node and the code above is executed I get result similar to the following:

http://localhost:44549/My/hello?helloArgument=5&t=1
Home > MyControllerPage > Hello 
http://localhost:44549/My/hello?type=x

Note that before the call to SiteMapPath(), the URL had 2 query string parameters, after the call the URL in the HttpContext was rewritten to remove all URL parameters (Regardless of whether those parameters are mapped to parameters in the controller action or not), and the type=x (which is applied to another node) is shown.

I had a custom html helper extension that I wrote which depends on reading query string parameters, and it's located after the call to SiteMapPath() in the page. The extension was behaving weirdly and after some investigation I found that SiteMapPath() call was the cause.

Thanks a lot for taking the time to read this, and thanks again for the great component.

Best regards,
Sherif

The text was updated successfully, but these errors were encountered:

NightOwl888 · 2014-02-11T23:56:59Z

By default nodes with no preservedRouteParameters argument cache the resolved URL that is generated at the time the SiteMap is built. However, this isn't ideal if you know you will have nodes that will change in value at runtime that you don't know ahead of time. You can disable the caching by setting cacheResolvedUrl="false" on your node or you can also disable it globally with the configuration setting.

<add key="MvcSiteMapProvider_EnableResolvedUrlCaching" value="false"/>

I believe that MVC will automatically copy the values helloArgument and t from the ambient context of the current request into the resolved URL after you have disabled caching. If not, you can force them in by including the keys in preservedRouteParameters (preservedRouteParameters="helloArgument,t").

sherif-elmetainy · 2014-02-12T08:45:41Z

Hello

Thanks a lot for your reply. I tried setting cacheResourcedUrl="false" on that node and then on every node in the site map file, and it didn't work. I also tried <add key="MvcSiteMapProvider_EnableResolvedUrlCaching" value="false"/> which also didn't work.

As for preservedRouteParameters="helloArgument,t", it didn't work on the node that has the routeParameters but it worked on the node that has type="x" attribute which I found weird that one node affects the behavior of all other nodes. And since my real project is rather big (unlike the example I mentioned to reproduce the bug), adding all route parameters from all nodes in the side in one attribute doesn't seem to cut it.

For now I settled with removing type="x" from the node in question as a workaround until I can figure a better solution.

I would rather that MVC Site Map Provider store its resolved URL in it's own, and leave the Request object in the HttpContext alone since this behavior has side effects on other components in the application.

Anyway, thanks again for taking the time to reply to my comment.

Best regards,
Sherif

NightOwl888 · 2014-02-12T09:57:30Z

MvcSiteMapProvider does not rewrite values in HttpContext. What I was describing is a "feature" of MVC that many people find confusing. I was able to prove that it isn't caused by MvcSiteMapProvider by removing MvcSiteMapProvider completely from the "failing" project and it was still "failing". Have a look at #123 and the associated MVC work item.

The UrlResolver module is a thin wrapper around the MVC UrlHelper class, which is what actually creates the URL, using the parameters you configure on the node. However, MVC will automatically grab any "ambient" values and add them in the URL, as well. I am not sure if it is any value, or just values you have defined in your routes, but I suspect is is only the latter because copying unknown values from the URL into the generated URL would open you up to cross-site scripting attacks.

Also note that the URLs that the MVC UrlHelper class generate are based entirely on the routes that are configured in your /App_Start/RouteConfig.cs file. If the default URL resolution behavior is unsuitable, you can configure the routes to generate URLs differently. And I recommend this approach rather than implementing your own ISiteMapNodeUrlResolver in MvcSiteMapProvider because it will affect your URLs application wide, not just the URLs in MvcSiteMapProvider.

The URL resolution caching is done in MvcSiteMapProvider internally. If it is enabled, the effect is that the URL that is resolved at application startup by the MVC UrlHelper is cached. If disabled, every request will be passed on to the MVC UrlHelper to resolve the URL, not just the one time at application startup.

BTW - cacheResourcedUrl will have no effect, the correct attribute name is cacheResolvedUrl.

Also, there are 2 things that MvcSiteMapProvider deals with when it comes to URLs/Routes:

Generating the URL based on routes
Finding the "Current" node based on the URL/route in HttpContext

There is a whole post about matching the URLs titled How to Make MvcSiteMapProviderRemember a User's Position that goes into depth about the node matching, but the URL generation is almost exactly the same to how you would create an ActionLink in MVC - it is based on the values that you have configured in your node. But the way routes are matched based on unknown values is to use preservedRouteParameters, which copy values from the current request into the current node when both generating URLs and matching URLs. The reason why these seem to bleed across different nodes is because only the current request is ever considered regardless of where the node is in the hierarchy. Whatever values are in the current request that are configured on the node are copied over to the internal RouteValues dictionary and then used to generate the URL - the next request could have completely different values and therefore get a completely different URL.

All things considered, there is nothing you have described here that is unusual behavior. This is exactly the way it is supposed to work, which is very similar to how MVC works.

sherif-elmetainy · 2014-02-12T11:31:38Z

Hello

Thanks you for the very detailed reply. About cacheResourcedUrl=false, this was a typo from me while writing the post, I checked in the project and it was written correctly in the project.

I downloaded the source code for the project (previously I was only using the nuget package), and started debugging to find the problem.

I found the part that is doing the URL rewriting which is
class AuthorizeAttributeAclModule

Method:

RouteData FindRoutesForNode(ISiteMapNode node, string originalPath, HttpContextBase httpContext)
{
      var routes = mvcContextFactory.GetRoutes();
      var originalRoutes = routes.GetRouteData(httpContext);
      var nodeUrl = node.Url;
      httpContext.RewritePath(nodeUrl, true);
      RouteData routeData = node.GetRouteData(httpContext);
}

The line that does the URL rewrite is line 130 in src\MvcSiteMapProvider\MvcSiteMapProvider\Security\AuthorizeAttributeAclModule.cs

The method calling this is VerifyNode has a try/finally to restore the originalPath

// Find routes for the sitemap node's url
var routes = this.FindRoutesForNode(node, originalPath, httpContext);
try
{
     if (routes == null)
       return true; // Static URL's will have no route data, therefore return true.
     return this.VerifyController(node, routes, controllerType);
}
finally
{
      // Restore HttpContext
      httpContext.RewritePath(originalPath, true);
}

When I attached a debugger, I found that the debugger enters the finally block and does execute the httpContext.RewritePath passing the correct value. In most cases it is restored successfully. In a rare case (my case) it is not restored. I am still trying to figure out why.

So there you go, this is the part that does the URL rewrite in the HttpContext and was causing the issue I have. I highly doubt it this is behaving as expected.

Best regards,
Sherif

NightOwl888 · 2014-02-12T12:22:24Z

Ahh, ok now I understand what you are referring to. This is actually the same exact problem (or very similar to the one) described in #181. However, the person who reported the problem "solved" it by disabling security trimming and was unwilling (or unable) to provide any way to reproduce the problem. I actually just closed it a few days ago because nobody else seemed to run into the problem.

I agree that rewriting the URL in this case is probably not the best solution, however I haven't had a chance to look into possible alternative ways that the security context can be checked on a URL that is not the current one (if you have any suggestions, please tell). This was carried over from the AuthorizeAttributeAclModule in v3, and as far as I know is the way it has always been done.

You mentioned that you created a demo that can reproduce this, so I would appreciate if you post the demo on GitHub or alternatively just zip it and make it available for download somewhere. If you do it quickly, I will hold up the v4.5 release that I was just about to do to see if I can solve this and include it.

NightOwl888 · 2014-02-12T15:13:32Z

I made an attempt to sidestep rewriting the URL on the current context by cloning it. At first glance, I found that cloning HttpContext is not possible. However, I searched a bit more and discovered this little extension, which I tried.

I found I needed to rewire a few things in order to use the cloned context everywhere, but all went well until I tried to rewrite the URL. The handy little helper didn't implement the RewiteUrl() method. I also tried it without rewriting the URL, and of course that doesn't work at all.

I took a look at the source of ASP.NET to see how HttpContext.RewriteUrl() works and it turns out rewriting a URL is a very involved process that calls many internal services. So, for the moment, I am at a loss how this can be done without rewriting the current context and then rewriting it back, like it is done currently, or going through an extremely labor intensive process of working out what is required in the RewriteUrl method that will make the AuthorizeAttribute check work (and always work).

@maartenba

I would appreciate you pointing me to any references (questions on SO, documentation, etc) that you used to pull the AuthorizeAttributeAclModule together - in particular the bit about rewriting the URL to check the authorization. My hope is that there is some new information that was not considered when it was implemented that can be used as an alternative to URL rewriting.

In the meantime, I don't see any readily available solutions to this problem so I won't hold up the release of v4.5 any longer.

sherif-elmetainy · 2014-02-12T17:31:43Z

Hello
I am glad you understand the problem, sorry if my original explanation wasn't clear.

I made the demo available at
https://skydrive.live.com/?cid=0B0D303A0F54ADFF&id=B0D303A0F54ADFF%21185

The file is called Webapplication4.zip

Build the project and then browse to /My/hello?helloArgument=5&t=1 to reproduce the problem.

Meanwhile, I will try to look for an alternative to URL rewriting to check.

Best regards,
Sherif

sherif-elmetainy · 2014-02-12T19:18:01Z

Hello

I tried the following as a solution and it seems to work:

I changed the part that does the rewrite in the method protected virtual bool VerifyNode(ISiteMap siteMap, ISiteMapNode node, HttpContextBase httpContext) as follows

var originalPath = httpContext.Request.Path;

// Rather than use URL rewriting which can have side effects on other components
// a new request/response is contstructed to using the current node URL
// The new response is using a textwriter with null stream as a backing stream 
// which doesn't consume resources
using (var nullWriter = new StreamWriter(Stream.Null))
{
    var newRequestUri = new Uri(httpContext.Request.Url, node.Url);
    var newRequest = new HttpRequest("", newRequestUri.ToString(), newRequestUri.Query);

    var newResponse = new HttpResponse(nullWriter);
    var newContext = new HttpContextWrapper(new HttpContext(newRequest, newResponse));

    // Find routes for the sitemap node's url
    var routes = this.FindRoutesForNode(node, originalPath, newContext);
    if (routes == null)
        return true; // Static URL's will have no route data, therefore return true.

    return this.VerifyController(node, routes, controllerType);
}

And I commented the line that does rewrite in protected virtual RouteData FindRoutesForNode(ISiteMapNode node, string originalPath, HttpContextBase httpContext)

I tested this solution and it seems to work, one issue that I see with this is that HttpMethod for the request creates is always GET rather than matching the HttpMethod for the Node, but this by the way was an issue in the original code where the authorization check is done against a request which has the same HttpMethod as the current request rather than the HttpMethod of the node being checked.

Best regards,
Sherif

NightOwl888 · 2014-02-12T19:39:47Z

Thanks for looking into this. I am hopeful this solution will work, I will give it a try. Unfortunately, v4.5.0 was released less than 5 seconds before I got this email. But this can probably go into a patch release.

The default set of HTML helpers that MvcSiteMapProvider uses always generate anchor tags, so using GET is exactly the set of permissions we are after (see #14). The only reason why anyone would want to change that is if they altered the HTML helper template to do a post, in which the option is available by setting the HttpMethod attribute/property. Using the HttpMethod of the current request causes nodes to drop from view when posting back the page and security trimming is enabled.

maartenba · 2014-02-12T20:05:19Z

Have checked on CodePlex if I could find the issue which originated in that piece of code, but no luck. If I recall correctly, it had something to do with being able to get route data for a URL, which back then was only possible through a fake / rewritten HttpContext.

NightOwl888 · 2014-02-14T07:35:50Z

@maartenba

I am trying to work out what is going on here...

if (originalRouteData != null && (!routeData.Route.Equals(originalRouteData.Route) || originalPath != nodeUrl || node.Area == string.Empty))
{
    routeData.DataTokens.Remove("area");
    //routeData.DataTokens.Remove("Namespaces");
    //routeData.Values.Remove("area");
}

If I understand correctly, the !routeData.Route.Equals(originalRouteData.Route) and originalPath != nodeUrl were some kind of attempt to preserve the routeData of the httpContext (albeit a poor one) because the assumption was made that we were using the current HTTP context and we were going to rewrite it back to the original state. Put another way, why else would we care if the current routeValues and URL match the routeValues and URL on the node?

The reason I ask is that there are several supporting lines of code that exist only to make this comparison. And all of it seems to be based on the assumption that we are trying to preserve the HttpContext the best we can so we can use URL rewriting on the HTTP context.

While using the alternate HttpContext method that @sherif-elmetainy suggested, I tried replacing the above with the following code, and it doesn't seem to make any difference (whether the controller is area or non-area).

if (node.Area == string.Empty)
{
    routeData.DataTokens.Remove("area");
}

The bottom line is, I can't think of any reason why we should care about the current routeData or URL when dealing with a node that has nothing to do with the current routeData or URL other than to support the URL rewriting. Thoughts?

maartenba · 2014-02-14T07:42:55Z

If it behaves the same, it can go. This code comes from the MVC v1 version if I'm not mistaken.

NightOwl888 · 2014-02-14T15:09:31Z

@sherif-elmetainy

Thanks for the valuable contribution. Hats off to you. Be sure to let us know if there are any other rough edges that need smoothing.

sherif-elmetainy · 2014-02-15T19:29:19Z

You are welcome. I will sure let you know if I find any more issue.

Thanks for everything :)

NightOwl888 closed this as completed in 45998ef Feb 14, 2014

NightOwl888 mentioned this issue Aug 28, 2015

Sitemap without HttpContext #408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrong URL rewrite using SiteMapPathHelper.SiteMapPath(this MvcSiteMapHtmlHelper helper) #278

Wrong URL rewrite using SiteMapPathHelper.SiteMapPath(this MvcSiteMapHtmlHelper helper) #278

sherif-elmetainy commented Feb 11, 2014

NightOwl888 commented Feb 11, 2014

sherif-elmetainy commented Feb 12, 2014

NightOwl888 commented Feb 12, 2014

sherif-elmetainy commented Feb 12, 2014

NightOwl888 commented Feb 12, 2014

NightOwl888 commented Feb 12, 2014

sherif-elmetainy commented Feb 12, 2014

sherif-elmetainy commented Feb 12, 2014

NightOwl888 commented Feb 12, 2014

maartenba commented Feb 12, 2014

NightOwl888 commented Feb 14, 2014

maartenba commented Feb 14, 2014

NightOwl888 commented Feb 14, 2014

sherif-elmetainy commented Feb 15, 2014

Wrong URL rewrite using SiteMapPathHelper.SiteMapPath(this MvcSiteMapHtmlHelper helper) #278

Wrong URL rewrite using SiteMapPathHelper.SiteMapPath(this MvcSiteMapHtmlHelper helper) #278

Comments

sherif-elmetainy commented Feb 11, 2014

NightOwl888 commented Feb 11, 2014

sherif-elmetainy commented Feb 12, 2014

NightOwl888 commented Feb 12, 2014

sherif-elmetainy commented Feb 12, 2014

NightOwl888 commented Feb 12, 2014

NightOwl888 commented Feb 12, 2014

sherif-elmetainy commented Feb 12, 2014

sherif-elmetainy commented Feb 12, 2014

NightOwl888 commented Feb 12, 2014

maartenba commented Feb 12, 2014

NightOwl888 commented Feb 14, 2014

maartenba commented Feb 14, 2014

NightOwl888 commented Feb 14, 2014

sherif-elmetainy commented Feb 15, 2014