feat(web-api): add configs to toggle absolute url usage in dynamic api calls #2176

zimeg · 2025-03-13T03:39:06Z

Summary

This PR adds a new allowAbsoluteUrls option to the WebClient constructor that can toggle if the slackApiUrl is always the prefix of the request URL when calling Slack API methods or if an absolute URL can override this.

To keep the current behavior, allowAbsoluteUrls defaults to true 📚

Preview

The following code attempts to make a request to "https://slack.com/api/https://example.com/method"!

const client = new webapi.WebClient(process.env.SLACK_TOKEN, {
  allowAbsoluteUrls: false,
});

const _response = await client.apiCall("https://example.com/method", { /* ... */ })

[DEBUG]  web-api:WebClient:0 initialized
[DEBUG]  web-api:WebClient:0 apiCall('https://example.com/method') start
[DEBUG]  web-api:WebClient:0 http request url: https://slack.com/api/https://example.com/method

Reviewers

The above example can be used with Slack API methods in "dot" notation - chat.postMessage - and different values of the allowAbsoluteUrls option 👾

For fast reference, it might be neat to use the absolute "https://slack.com/api/chat.postMessage" URL.

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 calls

codecov · 2025-03-13T03:40:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.99%. Comparing base (e1e3df0) to head (b154b6b).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2176      +/-   ##
==========================================
+ Coverage   91.94%   91.99%   +0.05%     
==========================================
  Files          38       38              
  Lines       10328    10386      +58     
  Branches      652      657       +5     
==========================================
+ Hits         9496     9555      +59     
+ Misses        820      819       -1     
  Partials       12       12

Flag	Coverage Δ
cli-hooks	`95.23% <ø> (ø)`
cli-test	`94.76% <ø> (ø)`
oauth	`77.39% <ø> (ø)`
socket-mode	`61.82% <ø> (ø)`
web-api	`96.93% <100.00%> (+0.04%)`	⬆️
webhook	`96.65% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

zimeg

📝 A few quick notes on these changes for the kind reviewers!

zimeg · 2025-03-13T03:41:33Z

docs/content/packages/web-api.md

+A dynamic method name can either be in the format shown above, to be sent with the [`slackApiUrl`](#custom-api-url)
+option, or as an absolute URL. Setting the
+[`allowAbsoluteUrls`](/reference/web-api/interfaces/WebClientOptions#allowabsoluteurls)
+option to `false` sends all requests to the `slackApiUrl` option value:
+
+```javascript
+const { WebClient } = require('@slack/web-api');
+
+const web = new WebClient(token, {
+  allowAbsoluteUrls: false,
+});
+```


Open to suggestions as always, but wanted to make sure a note of this option is included in these details 📚 ✨

🔗 And a link for reference: https://tools.slack.dev/node-slack-sdk/web-api#call-a-method

zimeg · 2025-03-13T03:44:29Z

packages/web-api/src/WebClient.ts

+        const requestURL = url.startsWith('https' || 'http') && this.allowAbsoluteUrls
+          ? url
+          : `${this.axios.getUri() + url}`;


📝 The requestURL variable is only used in debug logs, axios details configured separately, though this did seem close to what the logic of allowAbsoluteUrls might do!

WilliamBergamin

These changes look good 💯

I'm just not clear on when a developer would want to do this, why would they be passing and override URL if it will have not effect 🤔 instead of this addition should we advise them to not pass the override value if they don't want to override the URL?

WilliamBergamin · 2025-03-13T20:53:35Z

packages/web-api/src/WebClient.ts

@@ -618,7 +646,8 @@ export class WebClient extends Methods {
    // TODO: better input types - remove any
    const task = () =>
      this.requestQueue.add(async () => {
-        const requestURL = url.startsWith('https' || 'http') ? url : `${this.axios.getUri() + url}`;
+        const requestURL =
+          url.startsWith('https' || 'http') && this.allowAbsoluteUrls ? url : `${this.axios.getUri() + url}`;


The complexity of this line seems to be growing, could it be more readable if we broke it up in an if statement 🤔 ?

I was thinking the same... So much is happening here 😓

I'll explore the if statements, but do also like the const allowed in the current setup 👾

Maybe breaking the logic out into its own function could make sense

private deriveRequestUrl(url: string): string { if (this.allowAbsoluteUrls) { return url.startsWith('https' || 'http') ? url : `${this.axios.getUri() + url}` } return `${this.axios.getUri() + url}`; }

Similar logic was moved into a deriveRequestUrl function in 5f98b56! I forget private functions can be useful sometimes - it's a great call 😅

zimeg · 2025-03-13T21:26:32Z

@WilliamBergamin Thanks for checking out the changes! ✨

I don't believe this option is needed for scripts or apps making known API calls, but this option can be useful in setups that might pass unknown methods to WebClient.apiCall

The same guards against absolute URLs could definitely be added to application code, prefixing all .apiCall requests with the default API URL as you suggest, but IMO this implementation might be forgotten in some places which makes me think having an SDK implementation is preferred 👁️‍🗨️

WilliamBergamin

I see how the changes could be useful for security use cases 🤔

I've added one more comment about breaking the requestUrl logic into a method to increase readability, let me know what you think

Co-authored-by: William Bergamin <[email protected]>

zimeg · 2025-03-14T21:28:16Z

@WilliamBergamin 🙏 Thanks for suggesting the improvement to code structures. I'm hoping this'll make later changes around this logic a bit quicker to understand!

Tests continue to pass with that change too, but please let me know if other changes are requested!

WilliamBergamin

Nice 💯 this looks good

… above

packages/web-api/src/WebClient.ts

zimeg · 2025-03-19T00:04:09Z

@WilliamBergamin Thanks so much calling out the changes of this PR 🙏

I did a bit more testing and found one thing strange with http URLs fixed b154b6b. AFAICT this change only updates the request URL shown in logs since axios handles actual request logic, which is why the tests were passing 📺 ✨

Now is seeming like an alright time to merge this! I'll open a release PR for @slack/[email protected] but wait until tomorrow to release and make adjacent updates if this is all alright. Please let me know if other changes are needed before that though! 🔍

feat(web-api): add configs to toggle absolute url usage in dynamic ap…

2d14cfb

…i calls

zimeg added semver:minor enhancement M-T: A feature request for new functionality pkg:web-api applies to `@slack/web-api` labels Mar 13, 2025

zimeg added this to the [email protected] milestone Mar 13, 2025

zimeg requested review from hello-ashleyintech and WilliamBergamin March 13, 2025 03:39

zimeg self-assigned this Mar 13, 2025

zimeg requested a review from technically-tracy March 13, 2025 03:41

zimeg commented Mar 13, 2025

View reviewed changes

style: lint a ternary operator onto a single conditional line

a8bf6f5

WilliamBergamin requested changes Mar 13, 2025

View reviewed changes

WilliamBergamin reviewed Mar 14, 2025

View reviewed changes

zimeg and others added 2 commits March 14, 2025 14:08

chore: merge w main

5693e04

refactor: derive the request url in a standalone private function

5f98b56

Co-authored-by: William Bergamin <[email protected]>

zimeg requested a review from WilliamBergamin March 14, 2025 21:22

WilliamBergamin approved these changes Mar 17, 2025

View reviewed changes

zimeg added 3 commits March 18, 2025 14:53

chore: merge w main

88a5482

style: order constructor option initialization as shown in parameters…

92acdd4

… above

fix: check for absolute url prefixes with separate protocol strings

b154b6b

zimeg commented Mar 18, 2025

View reviewed changes

packages/web-api/src/WebClient.ts Show resolved Hide resolved

zimeg merged commit 6d62c40 into main Mar 19, 2025
59 checks passed

zimeg deleted the feat-web-api-allow-absolute-urls-option branch March 19, 2025 00:16

zimeg mentioned this pull request Mar 27, 2025

fix: require a custom 'api' url to send to instead of absolute urls as a 'method' slackapi/slack-github-action#420

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(web-api): add configs to toggle absolute url usage in dynamic api calls #2176

feat(web-api): add configs to toggle absolute url usage in dynamic api calls #2176

zimeg commented Mar 13, 2025

codecov bot commented Mar 13, 2025 •

edited

Loading

zimeg left a comment

zimeg Mar 13, 2025

zimeg Mar 13, 2025

zimeg Mar 13, 2025

WilliamBergamin left a comment

WilliamBergamin Mar 13, 2025 •

edited

Loading

zimeg Mar 13, 2025

WilliamBergamin Mar 14, 2025

zimeg Mar 14, 2025

zimeg commented Mar 13, 2025

WilliamBergamin left a comment

zimeg commented Mar 14, 2025

WilliamBergamin left a comment

zimeg commented Mar 19, 2025

feat(web-api): add configs to toggle absolute url usage in dynamic api calls #2176

feat(web-api): add configs to toggle absolute url usage in dynamic api calls #2176

Conversation

zimeg commented Mar 13, 2025

Summary

Preview

Reviewers

Requirements

codecov bot commented Mar 13, 2025 • edited Loading

Codecov Report

zimeg left a comment

Choose a reason for hiding this comment

zimeg Mar 13, 2025

Choose a reason for hiding this comment

zimeg Mar 13, 2025

Choose a reason for hiding this comment

zimeg Mar 13, 2025

Choose a reason for hiding this comment

WilliamBergamin left a comment

Choose a reason for hiding this comment

WilliamBergamin Mar 13, 2025 • edited Loading

Choose a reason for hiding this comment

zimeg Mar 13, 2025

Choose a reason for hiding this comment

WilliamBergamin Mar 14, 2025

Choose a reason for hiding this comment

zimeg Mar 14, 2025

Choose a reason for hiding this comment

zimeg commented Mar 13, 2025

WilliamBergamin left a comment

Choose a reason for hiding this comment

zimeg commented Mar 14, 2025

WilliamBergamin left a comment

Choose a reason for hiding this comment

zimeg commented Mar 19, 2025

codecov bot commented Mar 13, 2025 •

edited

Loading

WilliamBergamin Mar 13, 2025 •

edited

Loading