-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Fix error when calling HttpUtils::generateUri() #20946
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
Conversation
@@ -135,6 +135,10 @@ public function generateUri($request, $path) | |||
return $path; | |||
} | |||
|
|||
if (!$request instanceof Request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo. we should make this the first check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This targets 2.7, preventing the fatal error. Making this the first check would break for people which rely on the current first check, because it doesn't need $request
. I would suggest to throw an exception here as it is and deprecate passing something else than a Request in 3.3, for adding the typehint in 4.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we never add any similar checks if the doctype restricts the argument to a particular type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem is it also lacks a object typehint (Request $request
). We should prefer that over docblocks of course.. however the current policy doesnt seem to allow it.. (?)
#19960 (comment) closed for the same reason. Allthough i still disagree ;-)
And i also prefer a cosmetic error over PHP just crashing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @xabbuh. If we go down this path, we can potentially add such checks in a lot of places, which is a no go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough :)) I guess the thing is we want to improve method signatures / type hints in a upcoming major release, but the policy makes it real hard. And personally i feel it's holding us back from improving (read: simplifying) the codebase.
Ie. this is not about adding safety guards, scalar type checks, etc. but about forcing integrity where possible.
Anyway, as any error will be probably clear enough i understand it's not worth the effort.
So we could get a useful message instead of
Call to a member function getUriForPath() on null
fatal error (or aTrying to get property of non-object
notice one check lower).