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

Don't throw with : in path #147

Merged
merged 1 commit into from
Nov 30, 2015
Merged

Don't throw with : in path #147

merged 1 commit into from
Nov 30, 2015

Conversation

BrennanConroy
Copy link
Member

@Tratcher
Copy link
Member

How about aspnet/StaticFiles#84

@BrennanConroy
Copy link
Member Author

What about it?

@muratg
Copy link

muratg commented Nov 25, 2015

@BrennanConroy Is it supposed to be fixed here too?

@BrennanConroy
Copy link
Member Author

/%3C is a perfectly valid string to pass in and causes no issues.

@davidfowl
Copy link
Member

Yes, we shouldn't conflate the 2 issues. This might be slightly related but it's not the same bug.

@davidfowl
Copy link
Member

@BrennanConroy Where is the test for this fix?

@brentnewbury
Copy link

Sorry, I don't think I was quite clear in aspnet/StaticFiles#84 that /%3C is url decoded to < which causes an exception to be raised from the static files middleware when it's trying to determine the content type and whether to try to serve the file. I've not tried it, but from looking at the code, the file provider would also have a problem if the static files middleware carried on and tried to call IFilerProvider.GetFileInfo(string).

My opinion (and the fix I have waiting to create a pull request for [if you guys think what I have is okay]), is that the static file middleware see's the invalid characters in the request and skips on it, leaving it up to the next middleware in the pipeline... most likely ending up in a 404 returned to the client, not a 500.

{
return null;
// path contains a colon (":") that is not part of a volume identifier (for example, "c:\").
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid first chance exceptions like these in cases where we know it will throw.

@Tratcher
Copy link
Member

I think this and aspnet/StaticFiles#84 are the same class of failure and should be addressed in similar ways. E.g. something like this: https://github.com/brentnewbury/StaticFiles/commit/11e02d1a513f41a8dc704027dd6a50e25f64054e#diff-565cc1cae6ec9b8df50ec6fb281418b4R120 rather than a try/catch.

@BrennanConroy
Copy link
Member Author

That looks good, but we'll still need to handle having a colon in the string

@BrennanConroy BrennanConroy merged commit f0577ce into dev Nov 30, 2015
@Tratcher
Copy link
Member

@BrennanConroy Revert this.
/cc: @muratg

@Tratcher
Copy link
Member

You need to go through and make sure every call to a Path API with external data (e.g. a request path, but not the constructor) is guarded. Also, don't use try/catch, check for invalid characters explicitly.

@davidfowl davidfowl deleted the brecon/issue137 branch November 30, 2015 21:09
@davidfowl
Copy link
Member

+1

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

Successfully merging this pull request may close these issues.

6 participants