feat: add signature to ZIP tarball URL stored in KV store#875
feat: add signature to ZIP tarball URL stored in KV store#875vladfrangu merged 5 commits intomasterfrom
Conversation
yarn.lock
Outdated
| languageName: node | ||
| linkType: hard | ||
|
|
||
| "apify-client@npm:^2.13.0": |
There was a problem hiding this comment.
please deduplicate the lockfile so we dont have two installations of the client
(just run yarn dedupe)
tobice
left a comment
There was a problem hiding this comment.
Great, thanks for taking care of this! ❤️
| const store = await testUserClient.keyValueStores().getOrCreate(`actor-${testActor.id}-source`); | ||
|
|
||
| expect(store.urlSigningSecretKey).toBeDefined(); | ||
| const signature = createHmacSignature(store.urlSigningSecretKey!, `version-${actorJson.version}.zip`); |
There was a problem hiding this comment.
(opt) I'm never fond of when tests simply replicate the code being tested 😄 It's not much of a test...
Is this running against actual Apify?
If so, I'd suggest to:
- Send a request to restrict access on the key-value store.
- Test that the
tarballUrlcan be downloaded.
There was a problem hiding this comment.
Is this running against actual Apify?
Yes, yes it is!
There was a problem hiding this comment.
Refactored the test
| const tempTarballUrl = new URL( | ||
| `${apifyClient.baseUrl}/key-value-stores/${store.id}/records/${key}?disableRedirect=true`, | ||
| ); | ||
|
|
||
| /** | ||
| * Signs the tarball URL to grant temporary access for restricted resources. | ||
| * When a store is set to 'RESTRICTED', direct URLs are disabled. Instead of | ||
| * appending a security token, we add a signature to the URL parameters. | ||
| * https://github.com/apify/apify-core/issues/22197 | ||
| * | ||
| * TODO: Use keyValueStore(:storeId).getRecordPublicUrl from apify-client instead once it is released. | ||
| */ | ||
| if (store?.urlSigningSecretKey) { | ||
| const signature = createHmacSignature(store.urlSigningSecretKey, key); | ||
| tempTarballUrl.searchParams.set('signature', signature); | ||
| } | ||
|
|
||
| tarballUrl = tempTarballUrl.toString(); |
There was a problem hiding this comment.
Shouldn't this use the same getRecordPublicUrl?
There was a problem hiding this comment.
I'd leave it as it is! It helps to make sure, that the URL has correct signature.
If for some reason getRecordPublicUrl will return URL without signature, we'll catch it in the test
Recently we introduced restricted resource access, which disables anonymous access to resources. More context
Later we found a bug 👇
When user with disabled anonymous access tries to
apify pushActor, the build can't be done, because worker tries to download ZIP file stored in KV-store without token.This PR fixes it by generating KV-store record signature and appending it to the download URL.
More context about bug: Slack conversation
Note: I'll merge it after, we'll release new apify-client method: apify/apify-client-js#725