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.

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

download_endpoint: Adds support for expiring URLs #708

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

download_endpoint: Adds support for expiring URLs #708

wants to merge 3 commits into from

Conversation

davidwessman
Copy link

@davidwessman davidwessman commented Oct 4, 2024 •
edited
Loading

  • Adds expires_in and secret_key to download_endpoint plugin
  • This allows calling attachment.download_url(expires_in: 5 * 60)
    to generate a URL including signature and expires_at as query parameters. The timestamp is included in the signature and will only work before that timestamp.
  • The URL is signed and verified with HMAC with SHA256 digest.

@davidwessman
download_endpoint: Adds support for expiring URLs
41e6767 
- Adds `expires_in` and `verifier_secret` to `download_endpoint` plugin
- This allows calling `attachment.download_url(expires_in: 5 * 60)`
  to generate a URL that expires in 5 minutes.
- The URL is signed and verified with ActiveSupport::MessageVerifier.
@adam12
Copy link

adam12 commented Oct 4, 2024

I suspect the use of AS::Verifier is going to be an issue. We might need to go with OpenSSL::HMAC directly.

@davidwessman
Copy link
Author

I suspect the use of AS::Verifier is going to be an issue. We might need to go with OpenSSL::HMAC directly.

I'll try it out 🙂

@janko
Copy link
Member

janko commented Oct 4, 2024

Yes, Shrine doesn't depend on Active Support, so I want to avoid it if possible, even as an optional dependency. The derivation_endpoint plugin has URL signing implemented using OpenSSL::HMAC, so you can take a look there.

@davidwessman
Copy link
Author

Now I rewrote it to use OpenSSL-directly 🙂
Not sure if some of this functionality should be extracted to the UrlSigner class.

@davidwessman davidwessman requested a review from janko October 5, 2024 17:21
adam12
adam12 reviewed Oct 8, 2024
View reviewed changes
Copy link

@adam12 adam12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done adapting it for OpenSSL::HMAC. Few comments but overall looks good IMHO. Will leave final sign-off for Janko ofc.

test/plugin/download_endpoint_test.rb Show resolved Hide resolved
test/plugin/download_endpoint_test.rb Outdated Show resolved Hide resolved
@davidwessman davidwessman requested a review from adam12 October 15, 2024 11:26
@davidwessman
Copy link
Author

@janko could you allow the tests to run? They crash on my side because of libmagic

@davidwessman
Copy link
Author

@janko Sorry for a second (and last) ping, do you think there would be bandwidth for handling this PR within November?
Totally understandable if you do not know, trying to plan our alternatives with some deadlines coming up.
Thank you for great gems! 🙂

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

@janko janko Awaiting requested review from janko

@adam12 adam12 Awaiting requested review from adam12

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davidwessman @adam12 @janko