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

feat: support secure images #10

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

Conversation

angeloashmore
Copy link
Contributor

  • Adds s param support. If the user already has a signature, this allows passing it through.
  • Adds a second argument to buildImgixUrl to take in a secret URL token. If this is provided and s is not provided, an s parameter is generated.

If you think these make sense to add to this library, I can update the README.

Related documentation


// https://github.com/imgix/imgix-blueprint#securing-urls
const signatureBase = token + pathname + formattedQuery;
const signature = createHash('md5')
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for sending this PR, we should definitely add support for s.

One thing we will have to consider here is the cost of importing crypto, especially the impact it will have on the bundle size when considering the fact that this library may be used in the browser.

I ran a quick test using a webpack project (in production mode) with the following code:

import { createHash } from 'crypto';
createHash

The result was 110 kB gzipped! 😱

https://github.com/OliverJAsh/tree-shaking-test/tree/crypto

Given that crypto is costly to import, I wonder whether it's right for this library to be responsible for encoding the signature. I agree it would be convenient to be able to provide a decoded token to the build function and let it handle the rest, but if it comes at such a large cost then I think we should reconsider.

If this wasn't handled by the library, it would be very easy for users to wrap buildImgixUrl to handle this:

import { buildImgixUrl } from 'ts-imgix';

const encode = (token: string) => { /* … return encoded token */ }

export const buildImgixUrlWithToken = (url: string, token: string) =>
  buildImgixUrl(url)({ s: encode(token) })

We would of course still need to add support for the s query parameter, but the library would just assume it's already encoded. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I didn't consider browser usage. There are much smaller MD5 hashing functions that could be used instead of crypto.

tiny-hashes includes a "good enough" MD5 algorithm that could be a candidate.

https://github.com/jbt/tiny-hashes/blob/master/md5/md5.js

Signing should only happen server-side since the token is a secret. It could make more sense to export an additional function like buildImgixUrlWithToken rather than adding token to the main buildImgixUrl.

The s param is generated from the query params so I definitely think there's value in including this in the library. That being said, I could also see leaving it out to simplify the scope of the package. It's not difficult for someone to write their own function to append the signature.

Copy link
Member

Choose a reason for hiding this comment

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

It could make more sense to export an additional function like buildImgixUrlWithToken rather than adding token to the main buildImgixUrl.

That could work, since tree shaking will detect that function is not used in the browser, so consumers won't pay the price of importing crypto. However we don't currently distribute this module in ESM, so we'd have to do that first (since tree shaking only works with ES Modules).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you be open to adopting a bundler like Rollup, or even a tool like TSDX? If so, I can make a separate PR with the infrastructure changes.

Copy link
Member

Choose a reason for hiding this comment

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

I would be open to that. Personally I think I would prefer Rollup.

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

2 participants