Skip to content

feat: sign record's public url #358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Apr 1, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
193 changes: 177 additions & 16 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 5 additions & 5 deletions packages/apify/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,15 @@
"@apify/input_secrets": "^1.1.40",
"@apify/log": "^2.4.3",
"@apify/timeout": "^0.3.0",
"@apify/utilities": "^2.9.3",
"@crawlee/core": "^3.9.0",
"@crawlee/types": "^3.9.0",
"@crawlee/utils": "^3.9.0",
"@apify/utilities": "^2.13.0",
"@crawlee/core": "^3.12.3-beta.12",
"@crawlee/types": "^3.12.3-beta.12",
"@crawlee/utils": "^3.12.3-beta.12",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and don't forget to bump this

"apify-client": "^2.11.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the downgrade here? 👀

"fs-extra": "^11.2.0",
"ow": "^0.28.2",
"semver": "^7.5.4",
"tslib": "^2.6.2",
"ws": "^8.18.0"
}
}
}
9 changes: 8 additions & 1 deletion packages/apify/src/key_value_store.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { createHmacSignature } from '@apify/utilities';
import type { StorageManagerOptions } from '@crawlee/core';
import { KeyValueStore as CoreKeyValueStore } from '@crawlee/core';

Expand All @@ -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}`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (this.storageObject?.urlSigningSecretKey) {
publicUrl.searchParams.append('signature', createHmacSignature(this.storageObject.urlSigningSecretKey as string, key));
}

return publicUrl.toString();
}

/**
Expand Down
Loading