Skip to content
This repository has been archived by the owner on Nov 22, 2018. It is now read-only.

fix unicode url 404 error #256

Closed
wants to merge 1 commit into from
Closed

fix unicode url 404 error #256

wants to merge 1 commit into from

Conversation

newbienewbie
Copy link
Contributor

When serving a static directory whose path string contains unicode codes , let's say

  • url path string = blog/categories/大道/index.html
  • physical file path string = blog/categories/大道/index.html

The DefaultFileMiddlware gets the directory by var dirContents = _fileProvider.GetDirectoryContents(subpath.Value); . And now when we check whether the path of blog/categories/大道/ exists as below :

The result will be true .

However , when coming into the default index.html , the DefaultFileMiddlware checks the file path by :

var file = _fileProvider.GetFileInfo(subpath + defaultFile);

Here the subpath is often urlencoded , so the file path will be something as below :

"blog\\categories\\%E5%A4%A7%E9%81%93\\index.html"

As a result , file.Exists will return false .

When serving a static directory whose path string contains unicode codes , let's say 

* url path string = `blog/categories/大道/index.html` 
* physical file path string = `blog/categories/大道/index.html`

The `DefaultFileMiddlware` gets the directory by  `var dirContents = _fileProvider.GetDirectoryContents(subpath.Value);` . And now when we check whether the path of `blog/categories/大道/`  exists as below  :

https://github.com/aspnet/StaticFiles/blob/4c8df8e5a4e3abe5e7db51685dbd892d160c1e35/src/Microsoft.AspNetCore.StaticFiles/DefaultFilesMiddleware.cs#L69

The result will be true .  

However , when coming into the default `index.html` ,  the `DefaultFileMiddlware` checks the file path by : 
```csharp
var file = _fileProvider.GetFileInfo(subpath + defaultFile);
```

Here the `subpath` is often urlencoded , so the file path will be something as below : 
```
"blog\\categories\\%E5%A4%A7%E9%81%93\\index.html"
```
As a result , ` file.Exists` will return `false` .
@Tratcher Tratcher self-assigned this Oct 15, 2018
@Tratcher
Copy link
Member

Interesting find. Can you:
A) File an issue with these details, issues are easier to track.
B) Add a test for this scenario.
C) Rebase onto the release/2.2 branch, that's our upcoming release and this would be a good thing to include.

If you can't take care of these in a few days then we'll see about getting someone assigned to it.

@newbienewbie
Copy link
Contributor Author

@Tratcher Hi, I've created a new issue here .
I'll add tests for this scenario .

@poke
Copy link

poke commented Oct 17, 2018

Superseded by #257.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants