Skip to content

feat: downstream x-request-id mirror + webhook HTTP fallback fix#1151

Open
gclapperton wants to merge 1 commit into
masterfrom
feat/request-id-slice-5
Open

feat: downstream x-request-id mirror + webhook HTTP fallback fix#1151
gclapperton wants to merge 1 commit into
masterfrom
feat/request-id-slice-5

Conversation

@gclapperton
Copy link
Copy Markdown
Contributor

@gclapperton gclapperton commented May 22, 2026

Context

This PR is part of a multi-PR effort to guarantee every request handled by broker carries a stable, traceable request ID end-to-end.

This PR extends that guarantee outward to the customer system and close a gap in the webhook fallback path.

What problem does this solve?

Gap 1 — Downstream tracing. When broker fires an outbound HTTP request to a customer system (GitHub, GitLab, Jira, etc.), it sends snyk-request-id but never the more standard x-request-id. Tracing systems and proxies on the customer side that key off x-request-id cannot correlate the request back to anything in Snyk's traces.

Gap 2 — Webhook HTTP fallback. The normal webhook flow (SCM → broker-client → WebSocket → broker-server → Snyk API) is fully covered by the other two PRs But when the WebSocket is unavailable, the broker-client falls back to a direct HTTP call to the Snyk API. In that fallback path, whatever snyk-request-id the Snyk API chose to echo could overwrite the UUID the broker had already assigned — breaking the invariant that the broker's ID is the correlation handle for the full round trip.

@snyk-io
Copy link
Copy Markdown
Contributor

snyk-io Bot commented May 22, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@gclapperton gclapperton force-pushed the feat/request-id-slice-5 branch from 487bfd4 to e2425a4 Compare May 22, 2026 19:17
@gclapperton gclapperton changed the title feat: downstream x-request-id mirror + webhook HTTP fallback fix (slices 3 & 5) feat: downstream x-request-id mirror + webhook HTTP fallback fix May 22, 2026
@gclapperton gclapperton force-pushed the feat/request-id-slice-5 branch from e2425a4 to 7e93b9d Compare May 22, 2026 20:37
…quest-id

- Pin snyk-request-id on HTTP fallback, WS, and streaming response paths
  so the broker's middleware-validated UUID is always authoritative
- Mirror snyk-request-id onto x-request-id for outbound requests to
  customer systems (downstream tracing compatibility)
- Make req.requestId the authoritative source in HybridClientRequestHandler
  constructor, replacing the previous ||= guard
- Mask broker token in requestId synthesis debug log
@gclapperton gclapperton force-pushed the feat/request-id-slice-5 branch from cfe0fe5 to dea3854 Compare May 22, 2026 20:48
@gclapperton gclapperton marked this pull request as ready for review May 22, 2026 20:51
@gclapperton gclapperton requested review from a team as code owners May 22, 2026 20:51
@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Partial Token Masking Risk 🟡 [minor]

The logic in setRequestIdHeader uses req.url.replaceAll(brokerToken, maskToken(brokerToken)). If a brokerToken happens to be a short or common string that appears elsewhere in the URL path or query parameters (e.g., a short UUID segment matching a project ID), replaceAll will mask those legitimate segments as well. While maskToken helps identify it as a token, it may make debugging difficult by obscuring non-sensitive parts of the URL.

? req.url.replaceAll(brokerToken, maskToken(brokerToken))
📚 Repository Context Analyzed

This review considered 14 relevant code sections from 10 files (average relevance: 0.97)

? req.url.replaceAll(brokerToken, maskToken(brokerToken))
: req.url;
logger.debug(
{ method: req.method, url: req.url },
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

introduced in the last PR

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.

1 participant