fix(manifest): Strip protocol before feeding just the outgoing domain to the manifest API

[arunsathiya]

Summary

Fixes #245

When one or more outgoing domains in the manifest contain the protocol, the protocol is stripped off before feeding just the domain to the manifest API.

Testing

• Clone branch
• In a next-gen Slack project, use an invalid outgoing domain name like http:/example.com or ht*tp://example.com in the manifest.ts file
• Update import_map.json on your Slack project to use the local, patched Deno SDK. Example below.
• Run slack run to notice that the request fails with the appropriate error.

{
  "imports": {
    "deno-slack-sdk/": "/Users/arun/deno-slack-sdk/src/",
    "deno-slack-api/": "https://deno.land/x/deno_slack_api@2.1.1/"
  }
}

Special notes

N/A

Requirements

• I've read and understood the Contributing Guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.
• I've ran deno task test after making the changes.

[arunsathiya]

I just have one question about import_map.json usage. Does the slack run command run --import-map behind the scenes? That part is a bit unclear to me because the Slack CLI is not open source.

[mwbrooks]

Hi @arunsathiya 👋🏻 Thanks for the PR! 🎉

☑️ It looks good to me, so I'll enable the GitHub checks to run. However, I'll let some of the Deno maintainers take a look as well.

🧪 One suggestion would be to consider add some tests, if possible.

I just have one question about import_map.json usage. Does the slack run command run --import-map behind the scenes? That part is a bit unclear to me because the Slack CLI is not open source.

The slack run command starts a SocketMode connections managed by the CLI. When a function event is received, the CLI will invoke the script defined by the "start" hook in slack.json (example). That script currently calls the deno-slack-runtime/src/local-run.ts. In the source, you can see that the SDK reads the outgoing domains from the manifest and adds it to the Deno processes --allow-net.

We're in the process of preparing the CLI to be open sourced 😃 It's written in Golang and I see you're a Go developer who enjoys working on CLIs. Hopefully we'll see you in that repo in the future!

[codecov]

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.95%. Comparing base (ca8e418) to head (b584112).
Report is 30 commits behind head on main.

Files with missing lines	Patch %	Lines
src/manifest/mod.ts	86.36%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
- Coverage   98.07%   97.95%   -0.12%     
==========================================
  Files          58       58              
  Lines        2280     2301      +21     
  Branches      137      140       +3     
==========================================
+ Hits         2236     2254      +18     
- Misses         42       45       +3     
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[arunsathiya]

Thanks for the review, @mwbrooks!

One suggestion would be to consider add some tests, if possible.

Definitely! I have added them in 2cff6ca.

The slack run command starts a SocketMode connections managed by the CLI. When a function event is received, the CLI will invoke the script defined by the "start" hook in slack.json (example). That script currently calls the deno-slack-runtime/src/local-run.ts. In the source, you can see that the SDK reads the outgoing domains from the manifest and adds it to the Deno processes --allow-net.

Appreciate the explanation here, and exciting that Slack CLI will be open source in the future! I look forward to it. 😄

[filmaj]

Thanks a lot for tackling this issue! We really appreciate it ❤️

However, I think we need to add a test for the simple case of including just a domain in the manifest. I.e. outgoingDomains: ["poop.com"] fails with "invalid URL".

For example, testing this PR against our deno-github-functions sample fails when I try to run slack manifest validate:

09:57:05 in deno-github-functions on  main via 🦕 v1.37.1
➜ slak manifest validate
? Select a team filboxworkspace TXXXXX
? Choose an app environment Local AXXXXX
error: Uncaught (in promise) Error: Invalid outgoing domain: api.github.com, error TypeError: Invalid URL: 'api.github.com'
throw new Error(`Invalid outgoing domain: ${domain}, error ${e}`);

Probably because the URL() constructor assumes a fully qualified URL, including protocol? Ideally, the fix for #245 would allow users to include the protocol, or not.

[WilliamBergamin]

This is awesome work @arunsathiya 💯

@filmaj beat me to it, I think the following suggestion should allow users to include the protocol, or not.

[WilliamBergamin]

Throwing an error here may be a breaking change since existing users are providing string values that may not be valid urls, for example raw.githubusercontent.com

Returning the domain when the URL constructor throws an error would allow this change to be none breaking for existing apps and rely on the existing behavior to handle "bad" domains, let me know what you think of this?

Suggested change

	throw new Error(`Invalid outgoing domain: ${domain}, error ${e}`);
	return domain;

[filmaj]

That's a good point about breaking change, but I think it may be warranted here. The current behaviour is: any string is accepted, even non-hostnames. That's not great as it may lead the developer into a false sense of security if they e.g. typo'ed a domain.

We may also want to raise this with the backend team to see if the manifest API should raise an error if the strings provided to outgoingDomains are not a hostname.

[mwbrooks]

Good point about raising this with the backend team. It would be more reliable and consistent to have the backend validate the outgoingDomains format.

[arunsathiya]

Glad these concerns are being discussed, thanks for raising them! I implemented @WilliamBergamin's suggestion above, but have implemented another check where the http protocol is assumed in the absence of the protocol, and then the hostname is returned.

If that constructor fails for some reason, there is another defensive check that validates if the domain is valid and returns it, or throws an error: b584112

deno-slack-sdk/src/manifest/mod.ts

Lines 35 to 57 in b584112

	export const validateOutgoingDomains = (
	domains: string[],
	) => {
	return domains.map((domain) => {
	try {
	const url = domain.includes("://") ? domain : `http://${domain}`;
	return new URL(url).hostname;
	} catch (e) {
	if (isValidDomain(domain)) {
	return domain;
	} else {
	throw new Error(
	`Invalid outgoing domain: ${domain}, error ${e}`,
	);
	}
	}
	});
	};
	const isValidDomain = (domain: string) => {
	const domainPattern = /^[a-zA-Z0-9-]+(\.[a-zA-Z0-9-]+)*\.[a-zA-Z]{2,}$/;
	return domainPattern.test(domain);
	};

Is this the direction the team would like to take, or prefer having these checks in the backend side of things and not the SDK? I am leaning towards the latter. If agreed, I'll limit the changes to William's suggestion.

[arunsathiya]

One concern that still remains is, local hostnames like localhost are considered valid in the current implementation (without this PR's changes as well). Is that expected?

I'd imagine only fully qualified external domains are valid based on this documentation:

https://api.slack.com/automation/admin

Outgoing domains are a new concept, and apply only to apps deployed to Slack's managed infrastructure. These are domains the app may require access to — for example, if a developer writes a function that makes a request to an external API, they will need to include that API in their outgoing domains.

If they shouldn't be supported, this again is something to implement on backend side of things, I guess.

[filmaj]

This is great @arunsathiya ! We can pursue both in parallel; no reason why the SDK can't be improved while we're here, and separately we can work on improving the backend internally, too.

[arunsathiya]

Here are some test cases for the isValidDomain regex:

https://regexr.com/7nknp

[filmaj]

I wouldn't worry about validating the domain. I think we'd just want to make sure we can support named servers and IP addresses - at least IPv4. The outgoing domains field gets passed through directly to deno run --allow-net (see this code) - so whatever the --allow-net deno flag supports, we'd want to make sure the Manifest's outgoingDomains supports it.

[filmaj]

I'd add some more test cases here:

1. Bare IP addresses
2. Fully qualified domain names, including subdomains, i.e. en.wikipedia.org
3. Either of the above w/ port specifiers
4. All of the above with and without protocol specifiers