Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions .changeset/safe-user-agent.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@slack/web-api": patch
---

Fix user-agent header to URI-encode characters outside the Latin-1 range, preventing errors when `process.title` contains non-ASCII characters
86 changes: 86 additions & 0 deletions packages/web-api/src/instrument.test.ts
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🎹 praise: Wonderful additions to coverages and cases here! Thanks!

Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import assert from 'node:assert/strict';
import { createRequire } from 'node:module';
import { afterEach, describe, it } from 'node:test';

const require = createRequire(import.meta.url);

function isLatin1Safe(s: string): boolean {
return Buffer.from(s, 'latin1').toString('latin1') === s;
}

describe('instrument', () => {
const originalDescriptor = Object.getOwnPropertyDescriptor(process, 'title');
const modulePath = require.resolve('./instrument.ts');

afterEach(() => {
// Restore the original process.title property
if (originalDescriptor) {
Object.defineProperty(process, 'title', originalDescriptor);
}
// Clear module cache so next require gets a fresh evaluation
delete require.cache[modulePath];
});

function mockProcessTitle(title: string): void {
Object.defineProperty(process, 'title', {
get: () => title,
configurable: true,
});
}

/**
* Returns a fresh import of the instrument module. Since `baseUserAgent` is computed at module
* load time, we clear the require cache and re-require to pick up a mocked `process.title`.
*/
function freshImport(): typeof import('./instrument') {
delete require.cache[modulePath];
return require(modulePath);
}

describe('getUserAgent', () => {
it('should contain the package name', () => {
const { getUserAgent } = freshImport();
const ua = getUserAgent();
assert.ok(ua.includes('@slack:web-api'), `User-Agent should contain @slack:web-api: ${ua}`);
});

it('should include an ASCII process.title in the user agent', () => {
mockProcessTitle('node');
const { getUserAgent } = freshImport();
const ua = getUserAgent();
assert.ok(ua.includes('node/'), `User-Agent should contain node/: ${ua}`);
assert.ok(isLatin1Safe(ua));
});

it('should include other ASCII process.title in the user agent', () => {
mockProcessTitle('openclaw-gateway');
const { getUserAgent } = freshImport();
const ua = getUserAgent();
assert.ok(ua.includes('openclaw-gateway/'), `User-Agent should contain openclaw-gateway/: ${ua}`);
assert.ok(isLatin1Safe(ua));
});

it('should return a Latin-1 safe user agent when process.title contains non-Latin-1 characters', () => {
const notLatin1SafeTitle = '管理者: Windows PowerShell';
assert.strictEqual(isLatin1Safe(notLatin1SafeTitle), false);

mockProcessTitle(notLatin1SafeTitle);
const { getUserAgent } = freshImport();
const ua = getUserAgent();
assert.ok(isLatin1Safe(ua), `User-Agent contains non-Latin-1 characters: ${ua}`);
assert.ok(!ua.includes(notLatin1SafeTitle), 'User-Agent should not contain raw non-ASCII characters');
assert.ok(
ua.includes('%E7%AE%A1%E7%90%86%E8%80%85: Windows PowerShell'),
'User-Agent should percent-encode only non-Latin-1 characters',
);
});

it('should preserve Latin-1 characters in process.title', () => {
mockProcessTitle('café');
const { getUserAgent } = freshImport();
const ua = getUserAgent();
assert.ok(ua.includes('café/'), `User-Agent should preserve Latin-1 characters: ${ua}`);
assert.ok(isLatin1Safe(ua));
});
});
});
16 changes: 15 additions & 1 deletion packages/web-api/src/instrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@ function replaceSlashes(s: string): string {
return s.replace('/', ':');
}

const MAX_LATIN1_CODE = 0xff;

/**
* Ensures a string is safe for use in HTTP headers by URI-encoding characters outside the Latin-1 (ISO-8859-1) range.
* Latin-1 characters (code points 0x00–0xFF) are preserved as-is; all others are percent-encoded via encodeURIComponent.
*/
function toLatin1Safe(s: string): string {
let result = '';
for (const char of s) {
result += char.charCodeAt(0) <= MAX_LATIN1_CODE ? char : encodeURIComponent(char);
}
return result;
}

// TODO: for the deno build (see the `npm run build:deno` npm run script), we could replace the `os-browserify` npm
// module shim with our own shim leveraging the deno beta compatibility layer for node's `os` module (for more info
// see https://deno.land/std@0.116.0/node/os.ts). At the time of writing this TODO (2021/11/25), this required deno
Expand All @@ -18,7 +32,7 @@ function replaceSlashes(s: string): string {
// based code will report "browser/undefined" from a deno runtime.
const baseUserAgent =
`${replaceSlashes(packageJson.name)}/${packageJson.version} ` +
`${basename(process.title)}/${process.version.replace('v', '')} ` +
`${toLatin1Safe(basename(process.title))}/${process.version.replace('v', '')} ` +
`${os.platform()}/${os.release()}`;

const appMetadata: { [key: string]: string } = {};
Expand Down