feat: sign record's public url #358

danpoletaev · 2025-02-17T14:10:09Z

This PR is part of Issue #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.

Previous PR that adds storageObject to Crawlee is here

barjin · 2025-02-17T14:14:16Z

packages/apify/src/key_value_store.ts

+            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)}`;


Can we pls compose the URL with URL.searchParams and similar? Maybe concatting the path with path.posix.join() too?

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).

Yeah, makes sense. Refactored it 🙃

barjin · 2025-02-17T14:14:56Z

packages/apify/src/key_value_store.ts

@@ -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) {


urlSigningSecretKea instead of ...key might be a typo?

barjin

LGTM, thanks!

(as long as we wait for the new stable Crawlee version, but I see you mentioned that in the PR description already 👍🏽 )

B4nan

looking good, except the red CI which is not related to your changes

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)

tobice · 2025-03-10T09:47:51Z

packages/apify/src/key_value_store.ts

@@ -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}`);


(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 😅 )

i think there was a reason for this. if you ask what reason, i have no idea, but there is something at the back of my mind telling me there was one 🙃

@B4nan I think it should be safe to use config.get('apiPublicBaseUrl') variable as it has fallback to https://api.apify.com?

packages/apify/src/configuration.ts

B4nan · 2025-03-17T09:56:40Z

package.json

@@ -67,7 +67,7 @@
        "@typescript-eslint/eslint-plugin": "^7.0.0",
        "@typescript-eslint/parser": "^7.0.0",
        "commitlint": "^19.3.0",
-        "crawlee": "^3.11.5",
+        "crawlee": "^3.13",


please keep the full version definition here

B4nan · 2025-03-17T09:56:54Z

packages/apify/package.json

+        "@crawlee/core": "^3.12.3-beta.12",
+        "@crawlee/types": "^3.12.3-beta.12",
+        "@crawlee/utils": "^3.12.3-beta.12",


and don't forget to bump this

B4nan · 2025-03-17T09:57:11Z

packages/apify/package.json

+        "@crawlee/core": "^3.12.3-beta.12",
+        "@crawlee/types": "^3.12.3-beta.12",
+        "@crawlee/utils": "^3.12.3-beta.12",
+        "apify-client": "^2.11.1",


why the downgrade here? 👀

B4nan · 2025-03-17T09:58:19Z

packages/apify/src/key_value_store.ts

            return getPublicUrl.call(this, key);
        }

-        return `https://api.apify.com/v2/key-value-stores/${this.id}/records/${key}`;
+        const publicUrl = new URL(`${config.get('apiPublicBaseUrl', 'https://api.apify.com')}/v2/key-value-stores/${this.id}/records/${key}`);


no need for the explicit default here, its the exact same value as you define on the config level

tobice

Same as @B4nan wondering why apiPublicBaseUrl when we already have apiBaseUrl.

As in, I know that we're injecting as env variable APIFY_API_PUBLIC_BASE_URL... but I had no idea we have apparently also APIFY_API_BASE_URL 😅

tobice · 2025-03-17T10:33:13Z

test/e2e/sdk/publicUrl/src/main.mjs

+import { Actor, log } from 'apify';
+
+// Also needs to be changed in test.mjs
+const PUBLIC_RECORD_KEY = 'public-record-key';


(opt) How about you pass this as input to the Actor? All three values...

data to upload

key where to upload the data

key where to store the public URL

Then you don't need to define anything in two places and the test will be more readable (not that it is super complicated or anything 😄 )

That's a good point, thank you 🙏 Refactored it

B4nan · 2025-03-17T12:42:45Z

As in, I know that we're injecting as env variable APIFY_API_PUBLIC_BASE_URL... but I had no idea we have apparently also APIFY_API_BASE_URL 😅

Hmm, maybe it was about that actually. APIFY_API_BASE_URL is some local URL that won't work from outside of the platform/worker, right?

tobice · 2025-03-17T13:06:24Z

Hmm, maybe it was about that actually. APIFY_API_BASE_URL is some local URL that won't work from outside of the platform/worker, right?

I checked the code, and that exactly seems to be the case 👍 I think the traffic from Actors to Apify API does not go through the Internet. It's routed internally through AWS, presumably as it's cheaper.

So APIFY_API_PUBLIC_BASE_URL is indeed the correct value to use in this particular case.

danpoletaev · 2025-03-17T13:13:42Z

This is right, that's why I used APIFY_API_PUBLIC_BASE_URL, because apiBaseUrl was an IP address

B4nan · 2025-03-17T14:22:21Z

packages/apify/src/configuration.ts

@@ -16,6 +16,7 @@ export interface ConfigurationOptions extends CoreConfigurationOptions {
    actorRunId?: string;
    actorTaskId?: string;
    apiBaseUrl?: string;
+    apiPublicBaseUrl?: string;


lets add a comment here about the difference between apiBaseUrl and apiPublicBaseUrl

tobice · 2025-03-17T14:27:43Z

test/e2e/sdk/publicUrl/test.mjs

+
+const actor = client.actor(process.argv[2]);
+
+const run = await actor.call({ data: PUBLIC_DATA, recordKey: 'public-record-key' }, { waitSecs: 15 });


(opt) public-record-key is actually not needed to be passed as input, it's completely internal 🙈 You will get it as part of the public URL, but you don't care what's in it, you only care that the URL works.

What you might want to pass in is urlToPublicData. But you could also just upload the URL to the dataset and fetch it from there.

Anyway, sorry for all the back and forths on this 😅

feat: sign public url

17241fe

danpoletaev requested review from B4nan and barjin February 17, 2025 14:10

github-actions bot assigned danpoletaev Feb 17, 2025

github-actions bot added this to the 109th sprint - Platform team milestone Feb 17, 2025

github-actions bot added the t-platform Issues with this label are in the ownership of the platform team. label Feb 17, 2025

barjin reviewed Feb 17, 2025

View reviewed changes

danpoletaev added 2 commits February 17, 2025 15:24

refactor: compose url with URL()

3b40e57

refactor: clean up

13cdc63

danpoletaev requested a review from barjin February 17, 2025 14:28

barjin approved these changes Feb 17, 2025

View reviewed changes

B4nan approved these changes Feb 17, 2025

View reviewed changes

danpoletaev mentioned this pull request Feb 19, 2025

feat: add signing of public URL apify/apify-sdk-python#407

Merged

danpoletaev added 2 commits March 5, 2025 14:09

Merge branch 'master' into feat/sign-public-url

b01c22a

chore: added test for publicUrl

7a74be9

github-actions bot added the tested Temporary label used only programatically for some analytics. label Mar 5, 2025

danpoletaev requested a review from tobice March 10, 2025 09:36

tobice requested changes Mar 10, 2025

View reviewed changes

danpoletaev added 2 commits March 14, 2025 14:21

fix: use apiPublicBaseUrl to generate public url

a404dbf

fix: improve test

23505f1

danpoletaev requested a review from tobice March 14, 2025 15:09

B4nan requested changes Mar 17, 2025

View reviewed changes

tobice requested changes Mar 17, 2025

View reviewed changes

danpoletaev added 3 commits March 17, 2025 14:36

refactor: clean up

af0f70b

refactor: improved test

d2a11f3

Merge branch 'master' into feat/sign-public-url

1bf60b2

danpoletaev requested review from B4nan and tobice March 17, 2025 13:54

B4nan approved these changes Mar 17, 2025

View reviewed changes

tobice approved these changes Mar 17, 2025

View reviewed changes

danpoletaev and others added 3 commits March 17, 2025 16:06

refactor: clean up

d830ed1

Merge remote-tracking branch 'origin/master' into feat/sign-public-url

9477a14

fix: CI

cf7144e

janbuchar mentioned this pull request Mar 31, 2025

chore: Update Crawlee and fix related issues #373

Merged

Merge remote-tracking branch 'origin/master' into feat/sign-public-url

00a96f7

danpoletaev merged commit 6274cc0 into master Apr 1, 2025
8 checks passed

fnesveda added the validated Issues that are resolved and their solutions fulfill the acceptance criteria. label Apr 7, 2025


		const actor = client.actor(process.argv[2]);

		const run = await actor.call({ data: PUBLIC_DATA, recordKey: 'public-record-key' }, { waitSecs: 15 });

feat: sign record's public url #358

feat: sign record's public url #358

Uh oh!

Conversation

danpoletaev commented Feb 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barjin left a comment

Choose a reason for hiding this comment

Uh oh!

B4nan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobice left a comment

Choose a reason for hiding this comment

Uh oh!

tobice Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

B4nan commented Mar 17, 2025

Uh oh!

tobice commented Mar 17, 2025

Uh oh!

danpoletaev commented Mar 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tobice Mar 17, 2025 •

edited

Loading