Skip to content

DX-2769: reject empty id in single-resource endpoints#202

Merged
CahidArda merged 5 commits into
mainfrom
DX-2769
Jun 25, 2026
Merged

DX-2769: reject empty id in single-resource endpoints#202
CahidArda merged 5 commits into
mainfrom
DX-2769

Conversation

@CahidArda

Copy link
Copy Markdown
Collaborator

Summary

Same class of bug as the qstash-js fix. An empty single-id string was either
joined into the path as a trailing slash (path-collapse, e.g. v2/waiters/) or
wrapped into a bulk filter (dlqIds= / workflowRunIds=) with an empty value,
both of which the server resolves to a collection/bulk operation.

This adds an assertNonEmptyId guard so an empty id throws before any request.

Changes

  • Add assertNonEmptyId(id, label) helper in src/client/utils.ts
  • Guard single-id entry points:
    • client.cancel (string form)
    • makeNotifyRequest (eventId), makeGetWaitersRequest (eventId)
    • dlq: delete, resume, restart (string forms), retryFailureFunction

Testing

  • 7 new guard tests using mockQStashServer with receivesRequest: false.
  • tsc and eslint clean.

@linear-code

linear-code Bot commented Jun 17, 2026

Copy link
Copy Markdown

DX-2769

…201)

* feat: report failing step name via Upstash-Error-Step-Name header

When a step throws during execution, attach the step name to the error and
surface it on the 500 error response via the Upstash-Error-Step-Name header
so Workflow Logs can show which step is being retried.

* feat: expose stepName on failed next-step logs

The server now reports the failing step name (from the Upstash-Error-Step-Name
header) on the "next" step log group. Surface it as an optional stepName field
in the logs response type.

* fix: change requestcatcher to httpstatus

* fix: update mock server URLs to use requestcatcher

* feat: expose labels array on DLQ messages

DLQMessage only surfaced the first label via the deprecated `label`
field, unlike WorkflowRunLog which already exposes the full `labels`
array. Add `labels: string[]` to DLQMessage (and PublicDLQMessage) and
mark `label` deprecated, matching the run-log type.

Adds a mocked test asserting both fields round-trip through dlq.list()
and a live test confirming a run triggered with label: [a, b] surfaces
label === a and labels === [a, b] on its DLQ message.

* fix: bump version

* fix: urls in the tests

* fix: harden step-name error annotation and header sanitization

- attachStepNameToError no longer throws on non-extensible/frozen errors,
  so it can never mask the original failure
- sanitize the step name (strip control chars like CR/LF) before putting it
  in the Upstash-Error-Step-Name header so an invalid value can't break the
  500 response

* style: apply prettier formatting fixes to test files

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a defensive guard to prevent empty single-resource identifiers from being sent to the Workflow/QStash API, avoiding cases where "" becomes a trailing slash path segment or an empty bulk filter value that the server can interpret as a collection/bulk operation.

Changes:

  • Introduce assertNonEmptyId(id, label) in src/client/utils.ts and apply it to several single-id entry points.
  • Add guards to client.cancel (string form), notify/getWaiters eventId requests, and DLQ single-id operations.
  • Add tests intended to verify that no HTTP request is sent when ids are empty.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/client/utils.ts Adds assertNonEmptyId and uses it in notify/getWaiters request builders.
src/client/index.ts Guards cancel(string) with assertNonEmptyId.
src/client/index.test.ts Adds tests for empty-id guards on cancel/notify/getWaiters.
src/client/dlq.ts Guards DLQ single-id operations with assertNonEmptyId.
src/client/dlq.test.ts Adds tests for empty-id guards on DLQ operations.
Comments suppressed due to low confidence (7)

src/client/dlq.ts:186

  • resume still allows empty ids via (1) the deprecated { dlqId } overload and (2) WorkflowDLQActionFilters with dlqIds: "" / dlqIds: [""]. Those cases can still produce dlqIds= and be interpreted as a bulk operation. Validate all dlqIds in both legacy and new code paths.
        path: ["v2", "workflows", "dlq", "resume"],
        query: { dlqIds },
        method: "POST",
        headers: buildResumeRestartHeaders({ flowControl, retries }),
      });

src/client/dlq.ts:268

  • restart still allows empty ids via the deprecated { dlqId } overload and via WorkflowDLQActionFilters with empty dlqIds values ("" / [""]). Validate all dlqIds before sending the request.
        path: ["v2", "workflows", "dlq", "restart"],
        query: { dlqIds },
        method: "POST",
        headers: buildResumeRestartHeaders({ flowControl, retries }),
      });

src/client/dlq.ts:328

  • Only the string overload is validated. delete([""]) or delete({ dlqIds: "" | [""] }) can still send dlqIds= which may be interpreted server-side as a bulk operation. Validate all dlqIds in the normalized filters object.
   *
   * ```ts
   * let cursor: string | undefined;
   * do {
   *   const result = await client.dlq.delete({ all: true, count: 100, cursor });

src/client/dlq.test.ts:263

  • The execute callback doesn’t return/await the .rejects assertion, so the test may pass without asserting the rejection.
        execute: async () => {
          const result = await client.dlq.resume(dlqIds);
          expect(result).toEqual({ cursor: undefined, workflowRuns: responses });

src/client/dlq.test.ts:518

  • The execute callback doesn’t return/await the .rejects assertion, so the test may pass without actually validating the rejection.
        execute: async () => {
          const result = await client.dlq.restart(dlqIds);
          expect(result).toEqual({ cursor: undefined, workflowRuns: responses });

src/client/dlq.test.ts:713

  • The execute callback doesn’t return/await the .rejects assertion, so the rejection may not be asserted reliably.
    });
  });

  describe("retryFailureFunction", () => {
    test("should retry failure function of a DLQ message", async () => {

src/client/dlq.test.ts:778

  • The execute callback doesn’t return/await the .rejects assertion, so the test may pass without asserting the rejection.
        execute: async () => {
          const result = await client.dlq.delete(dlqIds);
          expect(result.deleted).toBe(deleted);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/client/index.test.ts Outdated
Comment thread src/client/index.test.ts Outdated
Comment thread src/client/index.test.ts Outdated
Comment thread src/client/utils.ts Outdated
Comment thread src/client/index.ts Outdated
requestcatcher.com was failing live deliveries with TLS certificate
errors. Replace it with the IANA-reserved example.com (valid cert,
stable) and centralize the host into a single MOCK_DESTINATION_HOST
constant in test-utils, referenced everywhere instead of repeating
the literal. Examples define a local MOCK_DESTINATION_URL since they
can't import test-utils.

Also switch the triggerWorkflowDelete test from spyOn (which made a
real network call) to the local mockQStashServer utility, removing the
last external request dependency.
# Conflicts:
#	src/client/index.test.ts
#	src/middleware/middleware.test.ts
#	src/serve/multi-region-integration.test.ts
#	src/serve/serve-many.test.ts
#	src/serve/serve.test.ts
#	src/test-utils.ts
#	src/workflow-requests.test.ts
- Validate every id in array/legacy forms of cancel() and DLQ
  resume/restart/delete via a shared toNonEmptyIdArray helper, not just
  the single-string overload, so cancel([""]) / delete([""]) fail fast
  instead of sending a bulk filter the server treats as a collection op.
- assertNonEmptyId: use a falsy guard instead of id.length so a plain-JS
  caller passing undefined/null gets a consistent QstashError rather than
  a TypeError from reading .length.
- makeNotifyRequest: treat workflowRunId with an explicit undefined check
  and validate it when provided (an empty string no longer silently
  changes the request path).
- await the .rejects assertions inside mockQStashServer execute callbacks
  so the rejection is actually asserted.
- add tests covering an empty string inside an id array for cancel and
  DLQ delete.
@CahidArda CahidArda merged commit f601f2c into main Jun 25, 2026
41 of 46 checks passed
@CahidArda CahidArda deleted the DX-2769 branch June 25, 2026 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants