Skip to content

fix(express): forward query strings without deprecated url.parse - #10142

Open
danielpeng1 wants to merge 1 commit into
masterfrom
WCN-4315-bitgo-express-errors
Open

danielpeng1 wants to merge 1 commit into
masterfrom
WCN-4315-bitgo-express-errors

Conversation

@danielpeng1

Copy link
Copy Markdown
Contributor

Eliminate the DEP0169 url.parse() deprecation warning emitted by BitGo Express on proxied requests with query parameters.

  • createAPIPath in modules/express/src/clientRoutes.ts extracts the query string by slicing req.url from the first ? up to any #, replacing the legacy url.parse() call that triggered DEP0169.
  • The slice preserves query bytes exactly as the client sent them and matches legacy url.parse().search behavior, including stripping fragments; the unused url import is removed.
  • Affects only the catch-all proxy handlers that call createAPIPath (v1 REST passthrough, v2 user, v2 coin-specific); local typed routes and the coin modules' getAddressDetails url.parse calls are unchanged.

Tests

  • modules/express/test/unit/clientRoutes/proxiedQueryStrings.ts runs real requests through the express app and asserts the exact upstream request path via nock: no query, simple query, percent-encoded values (%20, %27, %2B), +, repeated keys, empty value, bare ?, and v2 user and v2 coin paths forwarded byte-for-byte.

Ticket: WCN-4315

@danielpeng1 danielpeng1 self-assigned this Nov 6, 2026
@linear-code

linear-code Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

WCN-4315

WCN-4316

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

⚠️ Compatibility checks are failing on Node.js 27-nightly (experimental, non-blocking). This does not block merge, but it signals an incompatibility to fix before this release becomes supported.

View run

@danielpeng1
danielpeng1 marked this pull request as ready for review November 6, 2026 21:02
@danielpeng1
danielpeng1 requested review from a team as code owners November 6, 2026 21:02

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@danielpeng1

Copy link
Copy Markdown
Contributor Author

@claude review

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment on lines +339 to +342
const queryStart = req.url.indexOf('?');
if (queryStart !== -1) {
const fragmentStart = req.url.indexOf('#', queryStart);
apiPath += fragmentStart === -1 ? req.url.slice(queryStart) : req.url.slice(queryStart, fragmentStart);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 (optional) Operators running Express from source or the Docker image will most likely still see the DEP0169 warning on authenticated proxied requests, so the PR's goal is not met. The same request still reaches legacy url.parse in setRequestQueryString (modules/sdk-api/src/api.ts:212) and calculateHMACSubject (modules/sdk-hmac/src/hmac.ts:45). Fix: remove url.parse along the whole proxied-request path, which means changing sdk-api and sdk-hmac. That is larger than this change, so either do it or narrow the PR's claim. Node prints DEP0169 only once per process, so moving the first call elsewhere does not remove the warning.

Why this was flagged

Trigger: an authenticated client (access token set) sends a proxied GET with a query, e.g. /api/v2/tbtc/wallet?allTokens=true, through handleV2CoinSpecificREST -> createAPIPath -> redirectRequest. In bitgoAPI.ts:599 _sendRequestWithHmac calls setRequestQueryString(req), which runs urlLib.parse(req.url) at api.ts:212. It then calls calculateRequestHeaders, which reaches urlLib.parse(urlPath) at sdk-hmac/src/hmac.ts:45, and verifyResponse parses the same way. Node only raises DEP0169 when the caller is not inside node_modules. The Dockerfile copies sdk-api and sdk-hmac to /var/modules/... (lines 59, 61), and the monorepo runs them from modules/..., which are the same setups where clientRoutes.ts emitted the warning on base. The warning is deduplicated per process, so operators still get exactly one DEP0169, just from a different file. The new unit test only checks upstream paths and never checks that the warning is gone.

Verification: Severity: nit. The warning was already there on the base. _sendRequestWithHmac calls setRequestQueryString(req) at bitgoAPI.ts:599, which always runs urlLib.parse(req.url) (sdk-api/src/api.ts:212). In the Docker image, sdk-api and sdk-hmac live in /var/modules, not node_modules, so these calls count as application code and the warning is still printed.

This branch has not been deployed

No deployments
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