Feature/fix exceptions on invalid parameters #81
Conversation
|
Did you check the ws-fed spec what the exact status codes should be? |
|
Haven't changed/added any status codes (or validated their validity), just ensured that they didn't get swallowed in an Internal Server error. |
|
@scottbrady91 could you have a look please? |
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| return BadRequest("Invalid WS-Federation request: " + e.Message); |
There was a problem hiding this comment.
We're leaking exception details here. Any exception thrown in this try/catch block will have its message displayed to the end user.
The exceptions in IdentityServer are intentionally vague to not leak internal info.
There was a problem hiding this comment.
Yes, it leaks exceptions from WSFederationMessage.TryCreateFromUri and WSFederationMessage.CreateFromNameValueCollection, an alternative would be to just logg these and return "Invalid WS-Federation request".
There was a problem hiding this comment.
Is this not what it does already?
There was a problem hiding this comment.
Current version "crash" when eg. CreateFromNameValueCollection throws an exception. The TryCreateFromUri method does not contain a try/catch block (the name is a bit misleading).
|
If the try catch block is the only change, I'm happy for this to be merged once the exception message is removed from the |
ghost
deleted a comment from
Cleaned up some fault handling which previously resulted in thrown exceptions