mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-04 11:01:16 +01:00
fix: report Slack callback health correctly behind Cloud proxies (#13694)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Slack connections turn messages into governed agent runs and return replies to Slack. > - Remote runs must replace an incompatible sandbox runner with the controller's packaged binary. > - The fallback used a package-relative path that does not match the vendored server layout. > - Slack callback health also compared the internal proxy address with the public callback address. > - Master now contains the runner fallback fix; this pull request fixes callback health and extends missing-runner regression coverage. ## Linked Issues or Issue Description Refs #13677, #13680, #13691, and #13686. **What happened?** A packaged controller stopped a Slack-triggered remote run with `runner_remote_artifact_unavailable` when the sandbox runner needed replacement. Working Slack callbacks also showed a stale URL warning behind the Cloud gateway. **Expected behavior** The controller stages its packaged runner when needed. Callback health uses the observed public address and still detects real address changes. **Steps to reproduce** 1. Run a packaged server with a sandbox that has an older runner or no runner. 2. Send a Slack mention to an agent that uses that sandbox. 3. Route signed Slack callbacks through a claimed Cloud gateway that rewrites the upstream host. 4. Check the run and the Slack callback health panel. **Paperclip version or commit** Reproduced on master at `aeef493f4a7603b7b1254421b80fb00212982390`. The fix branch also includes #13691. **Deployment mode** Packaged server with a Cloud gateway and a Daytona sandbox. ## What Changed - Extend the controller-owned runner fallback tests with a missing sandbox binary case; retain the fix now merged in #13686. - Prefer dedicated gateway diagnostic headers that survive provider rewrites of standard forwarded headers. Use validated host hints only for callback-health evidence on claimed Cloud instances after provider acceptance. Preserve request bodies, routing, authentication, and configured callback URLs. - Cover current, stale, and missing sandbox runners, all Slack callback surfaces, rejected callbacks, malformed proxy hints, real host and port changes, and the existing self-hosted behavior. - Document the artifact lookup and callback-health boundaries. ## Verification - Native session executor and binary resolver suites: 379 tests passed after merging current master (`45c99a0d0`). - Targeted callback integration suite with disposable PostgreSQL: 4 tests passed before rebase. - Full `pnpm -r typecheck` and `pnpm build` passed after merging current master. The full local suite passed 12,668 tests; one suite failed to start its disposable PostgreSQL. Rerunning that suite alone passed all 31 tests. - The built server resolver selected the executable under `server/dist/vendor/paperclip-runner/bin/`. - Final callback regression: all 4 targeted integration tests pass, covering provider header rewrites, default ports, uppercase/trailing-dot hosts, and ignored self-hosted hints. - Live staging proof of the runner fix: a previously failed Slack thread recovered, a new mention received its requested response, and the account-connect command succeeded. Unsigned callbacks returned 401. - Live browser and Slack acceptance passed: generated callback URLs, account linking, a new mention, an interactive question and answer, and all three callback-health indicators. The connector was activated through the onboarding UI. - All 54 latest-head checks pass, two optional checks are skipped, and Greptile is 5/5 with no unresolved threads. ## Risks - Runner fallback must select a binary for the remote platform. This preserves explicit remote artifact overrides and the existing capability checks. - Proxy headers are not identity proof. They are used only for diagnostics on claimed Cloud instances after the provider accepts the request. Self-hosted instances ignore them. Wrong public hosts and ports still warn. - No database migration or new public API contract. ## Model Used OpenAI GPT-6 via Codex. Used reasoning, repository tools, code execution, and browser/native-app testing. Exact model variant and context window were not exposed by the session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
45c99a0d06
commit
b193077582
5 files changed
+123
-6
No files matched your search
@@ -53,6 +53,10 @@ runner instead; it can still reuse the image's compatible provider CLI. Explicit
|
||||
remote runner artifacts undergo the same checks. This metadata gate does not
|
||||
change the PRP version or invalidate existing durable session manifests.
|
||||
|
||||
The replacement artifact uses the controller's server-owned binary resolver,
|
||||
including the vendored binary in packaged deployments. It must not depend on a
|
||||
source checkout or a local Rust build directory being present on the server.
|
||||
|
||||
For a controller deployed before this capability check, operators can set
|
||||
`PAPERCLIP_RUNNER_REMOTE_BINARY_PATH` in the **server process environment** to a
|
||||
current runner binary built for the sandbox's OS and architecture. This bypasses
|
||||
|
||||
@@ -221,6 +221,13 @@ HTTPS termination at a trusted deployment boundary must not create false URL-dri
|
||||
warnings for the internal HTTP hop; continue detecting genuine host, port, and
|
||||
path changes. Keep callback verification evidence separate for distinct surfaces.
|
||||
|
||||
On a claimed Cloud instance, callback health can use the public host forwarded
|
||||
by the gateway after the provider authenticates the request. These hints are
|
||||
diagnostic evidence only: they must not affect authentication, routing, or the
|
||||
configured callback URL. Self-hosted instances continue using the request URL.
|
||||
Preserve gateway observations in dedicated diagnostic headers when another
|
||||
provider proxy replaces standard forwarded headers before reaching the tenant.
|
||||
|
||||
## Apply and verify
|
||||
|
||||
For a requested redesign, identify the relevant principles and fix the concrete
|
||||
|
||||
@@ -12976,6 +12976,8 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => {
|
||||
const body = JSON.stringify({ type: "url_verification", challenge: "proxy-check" });
|
||||
const request = signedSlackWebhookRequest({ url: `http://paperclip.example${path}`, contentType: "application/json", body });
|
||||
request.headers.set("x-forwarded-host", "untrusted.example");
|
||||
request.headers.set("x-paperclip-cloud-forwarded-host", "untrusted.example");
|
||||
request.headers.set("x-paperclip-cloud-forwarded-proto", "https");
|
||||
request.headers.set("x-forwarded-proto", "https");
|
||||
await service.handleWebhook(endpoint.publicId, "slack", request);
|
||||
await expect(service.get(endpoint.id)).resolves.toMatchObject({ setup: { callbacksNeedUpdate: false, callbackSurfaces: { events: { status: "current" } } } });
|
||||
@@ -12987,6 +12989,81 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("records the public Cloud proxy host for authenticated Slack callback health", async () => {
|
||||
const fixture = await seedCompany();
|
||||
const { endpoint, service } = await configuredSlackEndpoint(fixture);
|
||||
const canonicalOrigin = vi.spyOn(cloudRuntimeIdentity, "runtimeCanonicalOrigin").mockReturnValue("https://paperclip.example");
|
||||
const path = `/api/chat-webhooks/${endpoint.publicId}/slack`;
|
||||
const cases = [
|
||||
["events", "application/json", JSON.stringify({ type: "url_verification", challenge: "cloud-proxy-check" })],
|
||||
["interactivity", "application/x-www-form-urlencoded", new URLSearchParams({ payload: JSON.stringify({ type: "block_actions", team: { id: "T-PAPERCLIP" } }) }).toString()],
|
||||
["slashCommands", "application/x-www-form-urlencoded", new URLSearchParams({ command: "/maya-paperclip", team_id: "T-PAPERCLIP" }).toString()],
|
||||
] as const;
|
||||
try {
|
||||
for (const prefix of ["x-forwarded", "x-paperclip-cloud-forwarded"]) {
|
||||
for (const [surface, contentType, body] of cases) {
|
||||
const request = signedSlackWebhookRequest({ url: `http://tenant.internal:3100${path}`, contentType, body });
|
||||
request.headers.set("x-forwarded-host", "tenant.up.railway.app");
|
||||
request.headers.set(`${prefix}-host`, "paperclip.example");
|
||||
request.headers.set(`${prefix}-proto`, "https");
|
||||
expect((await service.handleWebhook(endpoint.publicId, "slack", request)).ok).toBe(true);
|
||||
await expect(service.get(endpoint.id)).resolves.toMatchObject({ setup: {
|
||||
callbacksNeedUpdate: false, callbackSurfaces: { [surface]: { status: "current" } },
|
||||
} });
|
||||
// The proxy hint is evidence, not a rewrite to the adapter request.
|
||||
expect(request.url).toBe(`http://tenant.internal:3100${path}`);
|
||||
}
|
||||
}
|
||||
canonicalOrigin.mockReturnValue("https://moved.example");
|
||||
await expect(service.get(endpoint.id)).resolves.toMatchObject({ setup: {
|
||||
callbacksNeedUpdate: true, callbackSurfaces: { events: { status: "stale" }, interactivity: { status: "stale" }, slashCommands: { status: "stale" } },
|
||||
} });
|
||||
} finally {
|
||||
await service.shutdown();
|
||||
canonicalOrigin.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it("does not let rejected callbacks or malformed proxy hints establish healthy Slack callbacks", async () => {
|
||||
const fixture = await seedCompany();
|
||||
const { endpoint, runtime, service } = await configuredSlackEndpoint(fixture);
|
||||
const providerRuntime = runtime.endpoints.get(endpoint.id)!;
|
||||
const canonicalOrigin = vi.spyOn(cloudRuntimeIdentity, "runtimeCanonicalOrigin").mockReturnValue("https://paperclip.example");
|
||||
const path = `/api/chat-webhooks/${endpoint.publicId}/slack`;
|
||||
const body = JSON.stringify({ type: "url_verification", challenge: "proxy-hint-check" });
|
||||
const send = async (host: string, protocol = "https", signed = true) => {
|
||||
const request = signedSlackWebhookRequest({ url: `http://tenant.internal:3100${path}`, contentType: "application/json", body });
|
||||
request.headers.set("x-paperclip-cloud-forwarded-host", host);
|
||||
request.headers.set("x-paperclip-cloud-forwarded-proto", protocol);
|
||||
if (!signed) request.headers.delete("x-slack-signature");
|
||||
// The fake provider runtime supplies the adapter's authentication result.
|
||||
providerRuntime.webhookResponse = new Response(signed ? "accepted" : "rejected", { status: signed ? 202 : 401 });
|
||||
return service.handleWebhook(endpoint.publicId, "slack", request);
|
||||
};
|
||||
try {
|
||||
// A real callback on an old public hostname must still warn.
|
||||
await send("old.example");
|
||||
await expect(service.get(endpoint.id)).resolves.toMatchObject({ setup: { callbacksNeedUpdate: true } });
|
||||
expect((await send("paperclip.example", "https", false)).status).toBe(401);
|
||||
await expect(service.get(endpoint.id)).resolves.toMatchObject({ setup: { callbacksNeedUpdate: true } });
|
||||
for (const host of ["paperclip.example, proxy.example", "paperclip.example/path", "user@paperclip.example", "paperclip.example?query=1", "paperclip.example#fragment", "paperclip.example:8443"]) {
|
||||
await send(host);
|
||||
await expect(service.get(endpoint.id)).resolves.toMatchObject({ setup: { callbacksNeedUpdate: true } });
|
||||
}
|
||||
await send("paperclip.example", "javascript");
|
||||
await expect(service.get(endpoint.id)).resolves.toMatchObject({ setup: { callbacksNeedUpdate: true } });
|
||||
await send("paperclip.example");
|
||||
await expect(service.get(endpoint.id)).resolves.toMatchObject({ setup: { callbacksNeedUpdate: false } });
|
||||
for (const host of ["paperclip.example:443", "PAPERCLIP.EXAMPLE:443", "paperclip.example.", "paperclip.example.:443"]) {
|
||||
await send(host);
|
||||
await expect(service.get(endpoint.id)).resolves.toMatchObject({ setup: { callbacksNeedUpdate: false } });
|
||||
}
|
||||
} finally {
|
||||
await service.shutdown();
|
||||
canonicalOrigin.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it("tracks Slack callback surfaces independently and reports public URL drift", async () => {
|
||||
const fixture = await seedCompany();
|
||||
const { endpoint, runtime, service } =
|
||||
|
||||
@@ -513,6 +513,7 @@ function canonicalCallbackUrl(value: string): string | null {
|
||||
try {
|
||||
const url = new URL(value);
|
||||
if (url.protocol !== "https:" && url.protocol !== "http:") return null;
|
||||
url.hostname = url.hostname.replace(/\.$/, "");
|
||||
url.username = "";
|
||||
url.password = "";
|
||||
url.search = "";
|
||||
@@ -544,10 +545,36 @@ type SlackCallbackInspection = {
|
||||
isUrlVerification: boolean;
|
||||
};
|
||||
|
||||
// Cloud records the public request host in dedicated headers so provider edges
|
||||
// cannot replace it with their upstream host. Only use callback-health evidence on a
|
||||
// claimed Cloud instance, after provider authentication succeeded. It must not
|
||||
// change the Request passed to the adapter, signature verification, routing,
|
||||
// board identity, or the configured URL used to generate callbacks.
|
||||
function slackCallbackObservationUrl(request: Request): string | null {
|
||||
const directUrl = canonicalCallbackUrl(request.url);
|
||||
if (!directUrl || !runtimeCanonicalOrigin()) return directUrl;
|
||||
const hasCloudEvidence = request.headers.has("x-paperclip-cloud-forwarded-host");
|
||||
const host = request.headers.get(hasCloudEvidence ? "x-paperclip-cloud-forwarded-host" : "x-forwarded-host")?.trim();
|
||||
const protocol = request.headers.get(hasCloudEvidence ? "x-paperclip-cloud-forwarded-proto" : "x-forwarded-proto")?.trim();
|
||||
if (!host || /[\s/@\\?#,]/.test(host) || (protocol !== "https" && protocol !== "http")) return directUrl;
|
||||
try {
|
||||
const origin = new URL(`${protocol}://${host}`);
|
||||
// Accept one authority only, never credentials, paths, queries, or lists.
|
||||
if (!origin.host || origin.username || origin.password || origin.pathname !== "/" || origin.search || origin.hash) return directUrl;
|
||||
const observed = new URL(directUrl);
|
||||
observed.protocol = origin.protocol;
|
||||
observed.host = origin.host;
|
||||
observed.port = origin.port;
|
||||
return canonicalCallbackUrl(observed.toString());
|
||||
} catch {
|
||||
return directUrl;
|
||||
}
|
||||
}
|
||||
|
||||
async function inspectSlackCallback(
|
||||
request: Request,
|
||||
): Promise<SlackCallbackInspection | null> {
|
||||
const url = canonicalCallbackUrl(request.url);
|
||||
const url = slackCallbackObservationUrl(request);
|
||||
if (!url) return null;
|
||||
const contentType = request.headers.get("content-type")?.toLowerCase() ?? "";
|
||||
let body: string;
|
||||
|
||||
@@ -292,6 +292,7 @@ import {
|
||||
} from "./native-session-executor.js";
|
||||
|
||||
beforeEach(() => {
|
||||
state.resolveRunnerBinary.mockReset().mockReturnValue("/tmp/paperclip-runnerd");
|
||||
state.resolveCurrentWakeCommentsBinding.mockReset().mockResolvedValue(null);
|
||||
state.assertCurrentWakeCommentsRead.mockReset().mockResolvedValue(undefined);
|
||||
});
|
||||
@@ -10059,11 +10060,12 @@ describe("runnerd provider runtime wiring", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it.each([false, true])("uses shared Codex and replaces only a stale runner image (stale=%s)", async (staleRunner) => {
|
||||
it.each(["current", "stale", "missing"])("uses shared Codex and the server-owned replacement artifact (image=%s)", async (image) => {
|
||||
const needsReplacement = image !== "current";
|
||||
// The mocked remote executes metadata probes; artifact staging only needs bytes.
|
||||
// Keep this regression independent of a locally compiled Rust runner binary.
|
||||
const controllerArtifact = join(isolatedStateDirectory, "paperclip-runnerd");
|
||||
if (staleRunner) {
|
||||
if (needsReplacement) {
|
||||
await writeFile(controllerArtifact, "fixture runner artifact");
|
||||
state.resolveRunnerBinary.mockReturnValueOnce(controllerArtifact);
|
||||
}
|
||||
@@ -10078,7 +10080,7 @@ describe("runnerd provider runtime wiring", () => {
|
||||
binaryName: "paperclip-runnerd",
|
||||
packageName: "@paperclipai/paperclip-runner",
|
||||
binaryContractVersion: 2,
|
||||
durableSessionCapabilities: staleRunner && command.command === "/usr/local/bin/paperclip-runnerd"
|
||||
durableSessionCapabilities: image === "stale" && command.command === "/usr/local/bin/paperclip-runnerd"
|
||||
? undefined
|
||||
: ["unlimited_runtime", "connection_lease_renewal"],
|
||||
prpTransportModes: ["listen_ws"],
|
||||
@@ -10095,7 +10097,7 @@ describe("runnerd provider runtime wiring", () => {
|
||||
} else if (script === "uname -s; uname -m") {
|
||||
stdout = `${process.platform === "darwin" ? "Darwin" : "Linux"}\n${process.arch === "arm64" ? "arm64" : "x86_64"}\n`;
|
||||
} else if (script.includes("command -v paperclip-runnerd")) {
|
||||
stdout = "/usr/local/bin/paperclip-runnerd\n";
|
||||
stdout = image === "missing" ? "" : "/usr/local/bin/paperclip-runnerd\n";
|
||||
} else if (script.includes("command -v codex")) {
|
||||
stdout = script.includes("/opt/paperclip-runner/bin/codex")
|
||||
? "/opt/paperclip-runner/bin/codex\n"
|
||||
@@ -10140,7 +10142,7 @@ describe("runnerd provider runtime wiring", () => {
|
||||
await expect(transport.controlPlaneRegistration({})).rejects.toThrow(
|
||||
"reached-preinstalled-codex-verification",
|
||||
);
|
||||
if (staleRunner) {
|
||||
if (needsReplacement) {
|
||||
expect(transport.runnerBinary).toBe(controllerArtifact);
|
||||
expect(syncIn).toHaveBeenCalledTimes(1);
|
||||
expect(syncIn).toHaveBeenCalledWith([expect.objectContaining({
|
||||
|
||||
Reference in new issue
Block a user