Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

feat: support secure images #10

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

feat: support secure images #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a53d664
feat: s param and optional URL signing
angeloashmore Apr 16, 2020
57611f5
docs: add link to imgix-blueprint on url signing
angeloashmore Apr 16, 2020
3ba8004
test: add token to signature passthrough test
angeloashmore Apr 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// tslint:disable-next-line match-default-export-name
import { createHash } from 'crypto';
import { ParsedUrlQueryInput } from 'querystring';
import * as urlHelpers from 'url';
import { addQueryToUrl } from 'url-transformers';
import { pickBy } from './helpers';
import { catMaybesDictionary, mapValueIfDefined } from './helpers/maybe';
Expand Down Expand Up @@ -71,6 +73,7 @@ export type ImgixUrlQueryParams = {
cs?: ImgixColorSpace;
faceindex?: number;
facepad?: number;
s?: string;
};

const pickTrueInObject = <K extends string>(obj: Record<K, boolean>): Partial<Record<K, true>> =>
Expand All @@ -87,6 +90,22 @@ const serializeImgixUrlQueryParamListValue = pipe(
joinWithComma,
undefinedIfEmptyString,
);
const addImgixUrlQueryParamSignature = (url: string, token?: string) => (
query: ParsedUrlQueryInput,
) => {
if (token === undefined || query.hasOwnProperty('s')) return query;

const formattedQuery = urlHelpers.format({ query });
const { pathname } = new urlHelpers.URL(url);

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

@OliverJAsh OliverJAsh Apr 16, 2020 •
edited
Loading

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.

.update(signatureBase)
.digest('hex');

return { ...query, s: signature };
};

const mapToSerializedListValueIfDefined = mapValueIfDefined(serializeImgixUrlQueryParamListValue);

Expand All @@ -110,12 +129,14 @@ const serializeImgixUrlQueryParamValues = (query: ImgixUrlQueryParams): ParsedUr
blur: query.blur,
faceindex: query.faceindex,
facepad: query.facepad,
s: query.s,
}),
catMaybesDictionary,
)({});

export const buildImgixUrl = (url: string) =>
export const buildImgixUrl = (url: string, token?: string) =>
pipe(
serializeImgixUrlQueryParamValues,
addImgixUrlQueryParamSignature(url, token),
query => addQueryToUrl({ url })({ queryToAppend: query }),
);
21 changes: 21 additions & 0 deletions src/tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,24 @@ assert.strictEqual(
}),
'https://foo.com/?auto=format&w=300',
);

assert.strictEqual(
buildImgixUrl('https://foo.com', 'token')({
auto: {
format: true,
},
w: 300,
s: 'signature',
}),
'https://foo.com/?auto=format&w=300&s=signature',
);

assert.strictEqual(
buildImgixUrl('https://foo.com', 'token')({
auto: {
format: true,
},
w: 300,
}),
'https://foo.com/?auto=format&w=300&s=d82d76f9f31379083b452f98bcd7f670',
);