feat: Restrict importing URLs which are not allowlisted (by domain) for the API key #511

swetabar · 2024-09-25T16:13:12Z

Related Issues

An API key owner has to specifically allowlist domains before they can be processed by Import as a Service.
We do not want to allow imports on domains which are not part of the allowlist for a given user.

github-actions · 2024-09-25T16:13:38Z

This PR will trigger a minor release when merged.

codevoyage · 2024-09-25T17:28:26Z

src/controllers/import.js

+  /**
+   * Check if the URLs in urlList belong to any of the base domains.
+   * @param urlList
+   * @param baseDomainList


The documentation for the @param tags can be improved for clarity, for example:

* Check if all URLs in the list belong to any of the allowed base domains. * @param {string[]} urlList - List of URLs to check. * @param {string[]} baseDomainList - List of allowed base domains.

Same for the other functions.

bhellema · 2024-09-25T16:51:41Z

src/controllers/import.js

+        log.error(`Missing imports.write scope for creating a new import job: ${error.message}`);
+        // Check if the user has the write_all_domains API scope.
+        // This scope is restricted to a few users.
+        validateAccessScopes([SCOPE.WRITE_ALL_DOMAINS]);


I don't think we should be validating that the user has this scope here. Looking at the log trace it would seem odd that we got an error, yet continued.

I think we should actually rebrand this scope from WRITE_ALL_DOMAINS to ALL_DOMAINS. Then users who have elevated powers should have both the WRITE + ALL_DOMAINS scopes. The initial scope validation done for SCOPE.WRITE on line 201 makes sense to me answering the question does the user have the privilege to create jobs, if so continue.

Then later on, validateUrlsByDomain shouldn't care about the scopes. The code could be wrapped with a scope check,

if (!validateAccsesScope([SCOPE.WRITE_ALL_DOMAINS]) { validateUrlsByDomain(urls); }

The method validateAccessScope throws an error in case the scopes do not match. So we might need to use this check instead:

if (!scopes.some((scope) => scope.name === SCOPE.WRITE_ALL_DOMAINS)) { validateUrlsByDomain(urls); }

bhellema · 2024-09-25T16:54:33Z

src/controllers/import.js

+    if (scopes.some((scope) => scope.name === SCOPE.WRITE_ALL_DOMAINS)) {
+      return;
+    }


We should probably stick to using the validateAccsesScope to tell us if the user has the scope. See my comment below about possibly removing scopes from this function all together.

@bhellema - validateAccessScope is just checking if the user has the given scope.
The scopes information (with the name and domains) is loaded to the profile later.
So, we cannot check if the scope has the required domains in that function.

How about adding a function validateScopeDomainsIsNotEmpty - to check if the domain list is not empty for a given input scope?
We will invoke it for APIs requiring scopes imports.read and imports.write. We will not need to invoke it for API that require scopes imports.read_all and for imports.write_all.

Another option could be returning the profile information in validateAccessScope -> const { authInfo: { profile } } = attributes;

In that case we could add the check for empty domain lists for scopes with read / write access.

bhellema · 2024-09-25T17:02:08Z

src/controllers/import.js

+   * @param inputUrl
+   * @return domain


Flush out missing jsdocs

bhellema · 2024-09-25T17:02:13Z

src/controllers/import.js

+   * @param urlList
+   * @param baseDomainList


Flush out missing jsdocs

bhellema · 2024-09-25T17:03:52Z

src/controllers/import.js

+      if (!belongsToBaseDomain) {
+        throw new ErrorWithStatusCode(`Invalid request: URL: ${inputUrl} not allowed`, STATUS_BAD_REQUEST);
+      }


Let's collect all the invalid URLs then throw the error if the list has a length > 0. That way the user isn't going back and forth potentially. We should be able to tell them the potential list of invalid URLs they provided.

bhellema · 2024-09-25T17:04:36Z

src/controllers/import.js

+   * @param urls
+   * @param scopes


Flush out missing jsdocs

bhellema · 2024-09-25T17:09:25Z

src/controllers/import.js

+          if (!scope.domains || scope.domains.length === 0) {
+            throw new ErrorWithStatusCode('Missing domain information', STATUS_UNAUTHORIZED);
+          }


This check should be done as part of validateAccessScopes. If we have a scope of SCOPE.WRITE (and does not have _WRITE__ALL_DOMAINS) then verify that the domains are set.

codevoyage · 2024-09-25T17:32:50Z

src/controllers/import.js

@@ -156,6 +222,10 @@ function ImportController(context) {
        urls, options, importScript, customHeaders,
      } = multipartFormData;

+      validateUrlsByDomain(urls, profile.getScopes());
+
+      log.debug(`Creating a new import job with ${urls.length} URLs.`);


Is there any reason this is a debug-level log and not info?

+1 to that Shikha. Having the logs tell us the story in Coralogix is critical.

Could modify it to an info log. I kept it as debug, to not pollute the logs.

Logs are critical to the tracing story. It's critical that we are able to track the progress of the job. It's absolutely okay to be logging information like this, and I would encourage more of it so that we can clearly see what's happening.

bhellema · 2024-09-27T15:36:57Z

src/controllers/import.js

+
+      // We only check if the URLs belong to the allowed domains if the user has the write scope
+      // We do not need to check the domains for users with scope: all_domains
+      if (!scopes.some((scope) => scope.name === SCOPE.ALL_DOMAINS)) {


You could use flatMap.

if (!scopes.some(scope => scope.name === SCOPE.ALL_DOMAINS)) { const allowedDomains = scopes .filter(scope => scope.name === SCOPE.WRITE && scope.domains && scope.domains.length > 0) .flatMap(scope => scope.domains.map(getDomain)); if (allowedDomains.length === 0) { throw new ErrorWithStatusCode('Missing domain information', STATUS_UNAUTHORIZED); } isUrlInBaseDomains(urls, allowedDomains); }

Nice! I'll add that in!

A little helper function to perform this check might improve readability, such as:

Suggested change

if (!scopes.some((scope) => scope.name === SCOPE.ALL_DOMAINS)) {

if (hasAllDomainsScope(scopes)) {

# [1.67.0](v1.66.3...v1.67.0) (2024-09-27) ### Features * Restrict importing URLs which are not allowlisted (by domain) for the API key ([#511](#511)) ([f8c7a84](f8c7a84))

solaris007 · 2024-09-27T15:56:50Z

🎉 This issue has been resolved in version 1.67.0 🎉

The release is available on:

v1.67.0
GitHub release

Your semantic-release bot 📦🚀

codevoyage · 2024-09-27T16:49:35Z

src/controllers/import.js

+      const scopes = profile.getScopes();
+
+      // We only check if the URLs belong to the allowed domains if the user has the write scope
+      // We do not need to check the domains for users with scope: all_domains


Since there is no check for the "write" scope for all_domains, then what happens if the user only has the ALL_DOMAINS scope and not the WRITE scope?

blefebvre

Shoot, looks like I just missed getting my comments in. Few minor items to consider.

blefebvre · 2024-09-27T18:26:51Z

package.json

@@ -77,6 +77,7 @@
    "@aws-sdk/s3-request-presigner": "3.654.0",
    "@slack/bolt": "3.21.4",
    "busboy": "1.6.0",
+    "psl": "^1.9.0",


Remove the ^ for consistency with other dependencies.

blefebvre · 2024-09-27T18:29:46Z

src/controllers/import.js

+   * @return {true} if all URLs belong to an allowed base domain
+   * @throws {ErrorWithStatusCode} if any URL does not belong to an allowed base domain
+   */
+  function isUrlInBaseDomains(urlList, baseDomainList) {


Suggestion: isUrlInAllowedDomains

blefebvre · 2024-09-27T18:30:57Z

src/controllers/import.js

+    });
+
+    if (invalidUrls.length > 0) {
+      throw new ErrorWithStatusCode(`Invalid request: URLs not allowed: ${invalidUrls.join(', ')}`, STATUS_BAD_REQUEST);


This could lead to a very large x-error header, which might lead to other issues. Perhaps just include the first invalid URL, as an example?

I added this change after initial reviews - we wanted to prevent back and forth with the customer if there are multiple URLs which are invalid. But, I could change it to the first URL.

blefebvre · 2024-09-27T18:33:18Z

src/controllers/import.js

+
+      // We only check if the URLs belong to the allowed domains if the user has the write scope
+      // We do not need to check the domains for users with scope: all_domains
+      if (!scopes.some((scope) => scope.name === SCOPE.ALL_DOMAINS)) {


A little helper function to perform this check might improve readability, such as:

Suggested change

if (!scopes.some((scope) => scope.name === SCOPE.ALL_DOMAINS)) {

if (hasAllDomainsScope(scopes)) {

blefebvre · 2024-09-27T18:35:01Z

test/controllers/import.test.js

+    });
+
+    it('should create an import job for the user scope imports.write', async () => {
+      baseContext.attributes.authInfo.profile.getScopes = () => [{ name: 'imports.write', domains: ['https://www.example.com'] }, { name: 'imports.read', domains: ['https://www.example.com'] }];


{ name: 'imports.read', domains: ['https://www.example.com']

Is domains needed/supported on imports.read?

Currently we do not need it. I added this unit test to test that the code picks up domains from only WRITE scope and not READ.

swetabar added 3 commits September 25, 2024 11:28

feat: Restrict importing urls which are not allowed by the api key

844430b

fix: Add additional unit test

a043833

fix: Modify comments for functions

27b4cc4

swetabar added the enhancement New feature or request label Sep 25, 2024

swetabar requested review from blefebvre and bhellema September 25, 2024 16:13

swetabar self-assigned this Sep 25, 2024

Merge branch 'main' into SITES-22756-restrict-importing-urls

9e6289a

Merge branch 'main' into SITES-22756-restrict-importing-urls

dd6527b

codevoyage reviewed Sep 25, 2024

View reviewed changes

bhellema reviewed Sep 25, 2024

View reviewed changes

codevoyage reviewed Sep 25, 2024

View reviewed changes

swetabar added 9 commits September 26, 2024 06:21

fix: refactor based on review comments

60db155

fix: add additional docs

703ea9c

fix: Refactor based on discussions around additive scopes

75a30c2

fix: add additional space

6e0cf22

fix: Add an additional check for empty domains

47e01ee

fix: Update spacecat-shared version to latest

1ffd5d0

fix: Add additional unit test

40078e8

Merge branch 'main' into SITES-22756-restrict-importing-urls

80773e9

fix: Fix the comments

d7bde58

bhellema approved these changes Sep 27, 2024

View reviewed changes

fix: refactor to flat map

a705e1d

swetabar merged commit f8c7a84 into main Sep 27, 2024
4 checks passed

swetabar deleted the SITES-22756-restrict-importing-urls branch September 27, 2024 15:55

solaris007 pushed a commit that referenced this pull request Sep 27, 2024

chore(release): 1.67.0 [skip ci]

38ffe89

# [1.67.0](v1.66.3...v1.67.0) (2024-09-27) ### Features * Restrict importing URLs which are not allowlisted (by domain) for the API key ([#511](#511)) ([f8c7a84](f8c7a84))

solaris007 added the released label Sep 27, 2024

codevoyage reviewed Sep 27, 2024

View reviewed changes

blefebvre reviewed Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Restrict importing URLs which are not allowlisted (by domain) for the API key #511

feat: Restrict importing URLs which are not allowlisted (by domain) for the API key #511

swetabar commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

codevoyage Sep 25, 2024 •

edited

Loading

bhellema Sep 25, 2024

swetabar Sep 25, 2024

bhellema Sep 25, 2024

swetabar Sep 25, 2024

swetabar Sep 25, 2024 •

edited

Loading

swetabar Sep 25, 2024 •

edited

Loading

bhellema Sep 25, 2024

bhellema Sep 25, 2024

bhellema Sep 25, 2024

bhellema Sep 25, 2024

bhellema Sep 25, 2024

codevoyage Sep 25, 2024

bhellema Sep 25, 2024

swetabar Sep 25, 2024

bhellema Sep 26, 2024

bhellema Sep 27, 2024

swetabar Sep 27, 2024

blefebvre Sep 27, 2024

solaris007 commented Sep 27, 2024

codevoyage Sep 27, 2024

blefebvre left a comment

blefebvre Sep 27, 2024

blefebvre Sep 27, 2024

blefebvre Sep 27, 2024

swetabar Sep 27, 2024

blefebvre Sep 27, 2024

blefebvre Sep 27, 2024

swetabar Sep 27, 2024

	if (!scopes.some((scope) => scope.name === SCOPE.ALL_DOMAINS)) {
	if (hasAllDomainsScope(scopes)) {

feat: Restrict importing URLs which are not allowlisted (by domain) for the API key #511

feat: Restrict importing URLs which are not allowlisted (by domain) for the API key #511

Conversation

swetabar commented Sep 25, 2024

Related Issues

github-actions bot commented Sep 25, 2024

codevoyage Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swetabar Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

swetabar Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solaris007 commented Sep 27, 2024

Choose a reason for hiding this comment

blefebvre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codevoyage Sep 25, 2024 •

edited

Loading

swetabar Sep 25, 2024 •

edited

Loading

swetabar Sep 25, 2024 •

edited

Loading