Skip to content

channels: tighten slack-channel-factory mock to be name-aware #94

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.

Sign up for GitHub

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
mcheemaa merged 1 commit into from
Apr 25, 2026
Merged

channels: tighten slack-channel-factory mock to be name-aware #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
71 changes: 57 additions & 14 deletions src/channels/__tests__/slack-channel-factory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,46 @@ const HTTP_IDENTITY = {
},
};

// Cross-repo invariant (audit Finding 1, dated 2026-04-25): the names
// "slack_bot_token" and "slack_gateway_signing_secret" must appear in
// phantomd's internal/secrets/types.go AllowedSecretNames map. Any drift
// between phantom and phantomd breaks SLACK_TRANSPORT=http boot with HTTP
// 404. This map is the SINGLE source of truth for the http-mode tests
// below; makeSecretFetcher() throws fail-loud on any name not listed
// here, so a future production-side rename that misses one repo will
// fail this test suite immediately instead of silently shipping a 404.
const SECRET_RESPONSES: Record<string, string> = {
slack_bot_token: "xoxb-from-metadata",
slack_gateway_signing_secret: "0123456789abcdef".repeat(4),
};

/**
* Build a name-aware secret fetcher mock. Returns the canned value for any
* name listed in SECRET_RESPONSES; throws an Error mentioning the offending
* name for any other input. The optional `tape` argument records every call
* for assertions on call ordering / call count without re-instantiating.
*
* Tests must NEVER substitute a permissive `() => Promise.resolve("...")`
* fetcher in place of this helper for the http path: the audit caught a
* production drift that a permissive mock would have hidden.
*/
function makeSecretFetcher(tape?: string[]): { get(name: string): Promise<string> } {
return {
get(name: string) {
tape?.push(name);
if (!(name in SECRET_RESPONSES)) {
const allowed = Object.keys(SECRET_RESPONSES).join(", ");
return Promise.reject(
new Error(
`unexpected secret name in test: ${name}. Allowed in this fixture: ${allowed}. This is the audit Finding 1 fail-loud guard; if production code requests a new name, add it to SECRET_RESPONSES AND to phantomd's internal/secrets/types.go AllowedSecretNames in the same change.`,
),
);
}
return Promise.resolve(SECRET_RESPONSES[name] as string);
},
};
}

describe("readSlackTransportFromEnv", () => {
test("returns 'socket' when SLACK_TRANSPORT is unset", () => {
expect(readSlackTransportFromEnv({} as NodeJS.ProcessEnv)).toBe("socket");
Expand Down Expand Up @@ -132,7 +167,7 @@ describe("createSlackChannel", () => {

test("transport=http with slack identity and metadata secrets returns a SlackHttpChannel", async () => {
const idFetcher = { get: () => Promise.resolve(HTTP_IDENTITY) };
const secFetcher = { get: (name: string) => Promise.resolve(SECRET_RESPONSES[name] ?? "") };
const secFetcher = makeSecretFetcher();
const ch = await createSlackChannel({
transport: "http",
channelsConfig: null,
Expand All @@ -151,33 +186,28 @@ describe("createSlackChannel", () => {
test("transport=http fetches both required secrets in parallel", async () => {
const requested: string[] = [];
const idFetcher = { get: () => Promise.resolve(HTTP_IDENTITY) };
const secFetcher = {
get: (name: string) => {
requested.push(name);
return Promise.resolve(SECRET_RESPONSES[name] ?? "");
},
};
const secFetcher = makeSecretFetcher(requested);
await createSlackChannel({
transport: "http",
channelsConfig: null,
port: 3100,
identityFetcher: idFetcher,
secretsFetcher: secFetcher,
});
// Audit F1 contract: production must request these EXACT names. Any
// future rename on either side without the matching edit on the other
// side will leave one of these assertions failing or trip the
// makeSecretFetcher fail-loud guard.
expect(requested).toContain("slack_bot_token");
expect(requested).toContain("slack_gateway_signing_secret");
expect(requested).toHaveLength(2);
});

test("transport=http never reads bot_token or app_token from channels.yaml", async () => {
// Even when channels.yaml has socket creds, the http path uses metadata.
const idFetcher = { get: () => Promise.resolve(HTTP_IDENTITY) };
const secretCalls: string[] = [];
const secFetcher = {
get: (name: string) => {
secretCalls.push(name);
return Promise.resolve(SECRET_RESPONSES[name] ?? "");
},
};
const secFetcher = makeSecretFetcher(secretCalls);
const ch = await createSlackChannel({
transport: "http",
channelsConfig: SOCKET_CONFIG,
Expand All @@ -197,7 +227,7 @@ describe("createSlackChannel", () => {
// factory wires the documented default base URL when no custom URL is
// passed. The fetcher class is responsible for the URL it constructs.
const idFetcher = { get: () => Promise.resolve(HTTP_IDENTITY) };
const secFetcher = { get: (name: string) => Promise.resolve(SECRET_RESPONSES[name] ?? "") };
const secFetcher = makeSecretFetcher();
// Pin the contract: passing through metadataBaseUrl propagates.
const ch = await createSlackChannel({
transport: "http",
Expand All @@ -209,4 +239,17 @@ describe("createSlackChannel", () => {
});
expect(ch).toBeInstanceOf(SlackHttpChannel);
});

test("makeSecretFetcher fails-loud when production asks for an unknown name", async () => {
// This is the audit Finding 1 regression guard. If the production code
// in slack-channel-factory.ts ever drifts to ask for a different name
// (for example reverting "slack_gateway_signing_secret" to the legacy
// "slack_signing_secret"), this fixture will throw and the test suite
// will fail-loud. The error message points at the cross-repo allowlist.
const fetcher = makeSecretFetcher();
await expect(fetcher.get("slack_signing_secret")).rejects.toThrow(
/unexpected secret name in test: slack_signing_secret/,
);
await expect(fetcher.get("totally_made_up")).rejects.toThrow(/AllowedSecretNames/);
});
});
8 changes: 8 additions & 0 deletions src/channels/slack-channel-factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ export async function createSlackChannel(input: CreateSlackChannelInput): Promis
"Run the OAuth flow via phantom-control or revert to SLACK_TRANSPORT=socket.",
);
}
// Cross-repo invariant (audit Finding 1, dated 2026-04-25): the names
// "slack_bot_token" and "slack_gateway_signing_secret" must appear in
// phantomd's internal/secrets/types.go AllowedSecretNames map. Drift on
// either side breaks SLACK_TRANSPORT=http boot with HTTP 404 (the
// gateway maps ErrInvalidName to 404 to avoid name enumeration). The
// regression is pinned by slack-channel-factory.test.ts's name-aware
// mock, which throws on any unexpected name; phantomd pins the same
// contract via TestIsAllowedName_AcceptsSlackGatewaySigningSecret.
const [botToken, signingSecret] = await Promise.all([
secFetcher.get("slack_bot_token"),
secFetcher.get("slack_gateway_signing_secret"),
Expand Down
Loading