Skip to content
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

[5.7] Signed route allowed for passing $absolute false but was never checked as such #26265

Merged
merged 1 commit into from
Oct 26, 2018

Conversation

patrick-radius
Copy link

Signed route allowed for passing $absolute false but was never checked as such

@jmarcher
Copy link
Contributor

This is a breaking change please target master.

@patrick-radius
Copy link
Author

Can you elaborate on how you think this is breaking? It was intended as a bugfix
The signature for public function signedRoute($name, $parameters = [], $expiration = null, $absolute = true) already allowed for passing $absolute but it could never be checked.

the default value for $absolute hasValidSignature(Request $request, $absolute = true) should make it backward compatible.

If i'm missing something please inform me.

@jmarcher
Copy link
Contributor

It is not if you are extending UrlGenerator and overwriting the hasValidSignature method, it will throw a warning. (Some environments tend to convert warnings to errors)

@jmarcher
Copy link
Contributor

And I believe the $absolute parameter should only be used for $this->route() for the outside one not for the one inside hash_hmac() but maybe im wrong.

@patrick-radius
Copy link
Author

This last point is actually the bug I'm trying to fix.
We need signed relative urls. But when it is validated it is invalid because it checks the signature against the entire url.

@taylorotwell taylorotwell reopened this Oct 26, 2018
@taylorotwell taylorotwell merged commit eec3eb6 into laravel:5.7 Oct 26, 2018
@sisve
Copy link
Contributor

sisve commented Oct 27, 2018

I agree, the changed method signature for hasValidSignature is a breaking change.

@patrick-radius
Copy link
Author

patrick-radius commented Oct 27, 2018

I'm open to suggestions. But since the signature of public function signedRoute($name, $parameters = [], $expiration = null, $absolute = true) already existed, is a public interface and not working as suggested, to me it's a bug. Especially since I needed this behavior.

@sisve
Copy link
Contributor

sisve commented Oct 27, 2018

It can be both a bug, and a breaking change. I'm not saying the change is bad, but perhaps it should have targeted the next release to avoid problems with existing packages. These types of changes can be implemented locally in your application until the next release, so you both get the functionality you need, and don't introduce any breaking changes to the framework in the middle of the 5.7 release.

@GrahamCampbell GrahamCampbell changed the title Signed route allowed for passing $absolute false but was never checked as such [5.7] Signed route allowed for passing $absolute false but was never checked as such Oct 30, 2018
@LucasLeandro1204
Copy link
Contributor

I use the signature and expires parameter to valid it through my SPA app by an ajax call, but this pr broke it =(

@patrick-radius
Copy link
Author

@LucasLeandro1204 can you be a bit more specific?

@messi89
Copy link

messi89 commented Nov 15, 2018

is this issue solved ?

I have juste updated to 5.7.13 but the issue persist....

$absolute is never passed and still true even if we set it to false on the temporarySignedRoute method and it throw an invalid signature

=-(

@LucasLeandro1204
Copy link
Contributor

@messi89 create a verifySignature middleware passing false and replace it in kernel

@messi89
Copy link

messi89 commented Nov 15, 2018

@LucasLeandro1204

something like that ?

if ($request->hasValidSignature($request, false)) {
  return $next($request);
}

I will try it

@LucasLeandro1204
Copy link
Contributor

@messi89 yeap, don't forget to throw Illuminate\Routing\Exceptions\InvalidSignatureException

@messi89
Copy link

messi89 commented Nov 15, 2018

@LucasLeandro1204 same issue

"message": "Invalid signature.",
    "exception": "Illuminate\\Routing\\Exceptions\\InvalidSignatureException",
    "file": "/app/Http/Middleware/VerifySignature.php",

the only way i found is to modify

public function hasValidSignature(Request $request, $absolute = true)
to
public function hasValidSignature(Request $request, $absolute = true) {
$absolute= false
....

@messi89
Copy link

messi89 commented Nov 15, 2018

problem solved after check the macro on FoundationServiceProvider

public function registerRequestSignatureValidation()
    {
        Request::macro('hasValidSignature', function ($absolute = true) {
            return URL::hasValidSignature($this, $absolute);
        });
    }

so in my middleware I change

if ($request->hasValidSignature($request, false)) {
  return $next($request);
}

to

if ($request->hasValidSignature($false)) {
  return $next($request);
}

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

Successfully merging this pull request may close these issues.

None yet

7 participants