-
Notifications
You must be signed in to change notification settings - Fork 4
feat: support secure images #10
base: master
Are you sure you want to change the base?
Conversation
|
||
// https://github.com/imgix/imgix-blueprint#securing-urls | ||
const signatureBase = token + pathname + formattedQuery; | ||
const signature = createHash('md5') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 addingtoken
to the mainbuildImgixUrl
.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
s
param support. If the user already has a signature, this allows passing it through.buildImgixUrl
to take in a secret URL token. If this is provided ands
is not provided, ans
parameter is generated.If you think these make sense to add to this library, I can update the README.
Related documentation