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

[6.x] Signed URL generation fails silently when a parameter is named 'signature' #30439

Closed
wants to merge 1 commit into from

Conversation

martinhinrichs
Copy link
Contributor

This is a bug report in the form of a pull request containing a failing test, as recommended in the contribution guide.

Bug

When a route contains a parameter named 'signature', generation of a signed URL for that route fails silently. UrlGenerator->signedRoute returns the unsigned route instead.

Why does it matter?

Signatures can be things in the real world. For example, documents like contracts or petitions have signatures. Or a forum user or e-mail account may have a default signature that is appended to every message.

So there are cases where App\Signature is an Eloquent model and could end up as a parameter in a route, like '/emailaccounts/{account}/signatures/{signature}', or '/petition/signatures/{signature}'.

If this is a signed route (for example '/contracts/{contract}/signatures/{signature}/verify'), UrlGenerator->signedRoute currently silently fails and returns the unsigned route instead.

This behavior is confusing. I don't know if it can be fixed, but at least URLGenerator->signedRoute should throw an exception when one of the parameters is called 'signature'.

@GrahamCampbell GrahamCampbell changed the title [Bug] Signed URL generation fails silently when a parameter is named 'signature' [6.x] Signed URL generation fails silently when a parameter is named 'signature' Oct 27, 2019
@taylorotwell
Copy link
Member

Yes, feel free to make a PR that throws an exception if signature is present.

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.

2 participants