feat: sign record's public url #358
Conversation
github-actions
bot
added
the
t-platform
| return `https://api.apify.com/v2/key-value-stores/${this.id}/records/${key}`; | ||
| } | ||
|
|
||
| return `https://api.apify.com/v2/key-value-stores/${this.id}/records/${key}?signature=${createHmacSignature(this.storageObject.urlSigningSecretKea as string, key)}`; |
There was a problem hiding this comment.
Can we pls compose the URL with URL.searchParams and similar? Maybe concatting the path with path.posix.join() too?
There was a problem hiding this comment.
Aside from ensuring the correct URL syntax, this procedural approach might also allow us to only specify the pathname once (and just conditionally add the query parameter if the secret exists).
There was a problem hiding this comment.
Yeah, makes sense. Refactored it 🙃
| @@ -19,7 +20,11 @@ export class KeyValueStore extends CoreKeyValueStore { | |||
| return getPublicUrl.call(this, key); | |||
| } | |||
|
|
|||
| return `https://api.apify.com/v2/key-value-stores/${this.id}/records/${key}`; | |||
| if (!this.storageObject?.urlSigningSecretKea) { | |||
There was a problem hiding this comment.
urlSigningSecretKea instead of ...key might be a typo?
github-actions
bot
added
the
tested
This PR is part of [Issue #19363](apify/apify-core#19363), which updates `KeyValueStore.getPublicUrl(recordKey)` to generate signed links using HMAC. **This PR**: - creates signature and appends it to the public URL of the record in KV store P.S. Before merging, we need to wait for the release of Crawlee, so we can update version of Crawlee. P.P.S It's hard to test the changes, since master branch of SDK and master branch of Crawlee are out of sync. Crawlee has some breaking changes in version 6.0, which are not yet addressed in master branch of SDK. Previous PR that adds storageObject to Crawlee Python is [here](apify/crawlee-python#993) Same PR in SDK JS is [here](apify/apify-sdk-js#358)
| @@ -19,7 +20,13 @@ export class KeyValueStore extends CoreKeyValueStore { | |||
| return getPublicUrl.call(this, key); | |||
| } | |||
|
|
|||
| return `https://api.apify.com/v2/key-value-stores/${this.id}/records/${key}`; | |||
| const publicUrl = new URL(`https://api.apify.com/v2/key-value-stores/${this.id}/records/${key}`); | |||
There was a problem hiding this comment.
(nit) Isn't the API base URL available in the env variables?
That way the Actor will create valid urls in all environments. You could leverage that to create an actual e2e test that not only creates & runs an Actor that produces the signed URL, but actually tests the URL against Apify API.
(The current test below basically just copies the implementation... you are calling the same code twice and checking that it produces the same value 😅 )
This PR is part of Issue #19363, which updates
KeyValueStore.getPublicUrl(recordKey)to generate signed links using HMAC.This PR:
P.S. Before merging, we need to wait for the release of Crawlee, so we can update version of Crawlee.
Previous PR that adds storageObject to Crawlee is here