Skip to content

[HttpFoundation] Add UriSigner::verify() that throws named exceptions #60102

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 18, 2025
Merged

[HttpFoundation] Add UriSigner::verify() that throws named exceptions #60102

merged 1 commit into from
Apr 18, 2025

Conversation

kbond
Copy link
Member

@kbond kbond commented Mar 31, 2025 •
edited
Loading

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues n/a
License MIT

This adds UriSigner::verify(): Similar to check() but throws UnSignedUriException, UnverifiedSignedUriException or ExpiredSignedUriException if invalid (no return if valid).

Usage example:

// Verifying a Signed Uri
try {
    $uriSigner->verify($uri);
} catch (UnSignedUriException $e) {
    // the uri either not signed
    $e->uri; // the uri value
} catch (UnverifiedSignedUriException $e) {
    // the the signature is invalid
    $e->uri; // the uri value
} catch (ExpiredSignedUriException $e) {
    // the signature is valid but it's expired
    $e->uri; // the uri value
    $e->expiredAt; // \DateTimeImmutable
}

TODO

  • Changelog
  • Tests

@kbond kbond added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Mar 31, 2025
@carsonbot carsonbot changed the title [HttpFoundation] Add UriSigner::signAndWrap(): SignedUri and UriSigner::verify(): SignedUri Add UriSigner::signAndWrap(): SignedUri and UriSigner::verify(): SignedUri Mar 31, 2025
@carsonbot carsonbot added this to the 7.3 milestone Mar 31, 2025
@carsonbot carsonbot changed the title Add UriSigner::signAndWrap(): SignedUri and UriSigner::verify(): SignedUri [HttpFoundation] Add UriSigner::signAndWrap(): SignedUri and UriSigner::verify(): SignedUri Mar 31, 2025
@OskarStark OskarStark changed the title [HttpFoundation] Add UriSigner::signAndWrap(): SignedUri and UriSigner::verify(): SignedUri [HttpFoundation] Add UriSigner::signAndWrap() and UriSigner::verify() Apr 1, 2025
@kbond
Copy link
Member Author

kbond commented Apr 2, 2025 •
edited
Loading

I'm not a huge fan of signAndWrap() and the SignedUri VO. I'd be ok dropping these and just keeping verify() + named exceptions.

@kbond kbond force-pushed the uri-signer-ensure-valid branch 2 times, most recently from ee8c09d to a627d43 Compare April 2, 2025 19:35
@kbond kbond changed the title [HttpFoundation] Add UriSigner::signAndWrap() and UriSigner::verify() [HttpFoundation] Add UriSigner::verify() that throws named exceptions Apr 2, 2025
@kbond
Copy link
Member Author

kbond commented Apr 2, 2025

I've updated this PR to just add SignedUri::verify().

@kbond kbond removed the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Apr 2, 2025
nicolas-grekas
nicolas-grekas reviewed Apr 9, 2025
View reviewed changes
@kbond kbond force-pushed the uri-signer-ensure-valid branch from 9a617d0 to 0a35de0 Compare April 9, 2025 12:39
@kbond kbond mentioned this pull request Apr 9, 2025
Open
nicolas-grekas
nicolas-grekas reviewed Apr 16, 2025
View reviewed changes
@nicolas-grekas
Copy link
Member

(rebase needed)

@kbond kbond force-pushed the uri-signer-ensure-valid branch from 493f298 to 60b8ba7 Compare April 18, 2025 16:01
@nicolas-grekas nicolas-grekas force-pushed the uri-signer-ensure-valid branch from ce0feb3 to ac4de2c Compare April 18, 2025 21:02
@nicolas-grekas
Copy link
Member

Thank you @kbond.

@nicolas-grekas nicolas-grekas merged commit eb71515 into symfony:7.3 Apr 18, 2025
7 of 8 checks passed
@nicolas-grekas nicolas-grekas mentioned this pull request Apr 18, 2025
Closed
@rela589n rela589n mentioned this pull request May 2, 2025
Closed
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
@santysisi santysisi mentioned this pull request May 9, 2025
Open
@alexander-schranz
Copy link
Contributor

alexander-schranz commented Jul 22, 2025 •
edited
Loading

@kbond littel bit curious why are we not throwing a HttpException here?

I must say as signed uris are verified in controllers 99% from requests I would expect a HttpException by default something like (400 or 403). As we are here in the http foundation component.

@kbond kbond deleted the uri-signer-ensure-valid branch July 22, 2025 13:53
@kbond
Copy link
Member Author

kbond commented Jul 22, 2025 •
edited
Loading

litte bit curious why are we not throwing a HttpException here?

The purpose of this feature is to distinguish between the different reasons for a signed url validation error. For example, you can redirect and add a flash message explaining the problem. Named exceptions felt the best.

I get I could have used HTTP exceptions but wanted the user to be able to customize the response code. 60395 adds default http status codes but these can still be customized if desired.

Also, in the future, we can add additional details to these exceptions if desired - like "when it expired" on ExpiredSignedUriException.

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Jul 22, 2025 •
edited
Loading

@kbond I see your case. I definitly totally +1 for the own named exception here, I would have just extend from the HttpException with default status code of 403. As for me was a little bit unexpected to have per default for verify a server error (500) instead of a client error for a false signed URL.

But looks like https://github.com/symfony/symfony/pull/60395/files#diff-d2b702ce5099ca6ea0823843d11127e01f37b33c5eeafb4463b8a2f5d4dbebbc catches this and convert them to a client error, so fine with that.

Really thank you for your fast response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weaverryan weaverryan weaverryan left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Feature HttpFoundation Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
5 participants
@kbond @nicolas-grekas @alexander-schranz @weaverryan @carsonbot