feat: allow metadata headers for device flow requests#283
Conversation
Endpoint audit❌ 40 off-contract call(s) — not on the agent-server · classifiers: cloud
❌ Not on agent-server (gated, 40)⛔ (no known backend) — served by no backend we can see (15)
|
| return fetch(`${normalizeHost(host)}${path}`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': contentType }, | ||
| headers: { ...headers, 'Content-Type': contentType }, |
There was a problem hiding this comment.
The PR description states that it "keeps the endpoint-managed Content-Type authoritative," but the current spread operation doesn't actually guarantee that behavior.
While HTTP header names are case-insensitive, JavaScript object keys are not. As a result, if a caller provides a lowercase content-type, the merged object ends up containing two distinct keys. Unfortunately, fetch doesn't resolve this using a last-write-wins approach. Instead, the Headers constructor appends the values together.
Verified against Node's undici:
const h = { ...{ 'content-type': 'text/plain', 'X-Foo': 'bar' }, 'Content-Type': 'application/json' };
[...new Headers(h)]
// => [['content-type', 'text/plain, application/json'], ['x-foo', 'bar']]Instead of the SDK's value taking precedence, the request is sent with a malformed Content-Type. The server rejects the request, and the user receives an opaque Unexpected response from server: 415 error that provides no indication the issue originated from the header they supplied.
One possible approach is to construct a Headers instance first, since it normalizes header names, and then use set, which correctly overrides any existing value:
const requestHeaders = new Headers(headers);
requestHeaders.set('Content-Type', contentType);
return fetch(`${normalizeHost(host)}${path}`, {
method: 'POST',
headers: requestHeaders,
body: requestBody,
signal,
});One small follow-up is that both new tests would need to be updated, as they currently assert against a plain object literal. With this change, they'd instead need to compare the resulting Headers entries.
I agree this is likely an uncommon code path—a caller passing content-type through a metadata headers API isn't something most users would do. That said, I think it's still worth addressing before merging because Content-Type is the only header the SDK itself manages, the PR explicitly states that it remains authoritative, and this behavior isn't currently covered by tests, which likely explains why it wasn't caught.
Thank you very much! 🙏
|
🚀 Released in v1.34.0. |
2 participants
What changed
startDeviceFlowandPollDeviceTokenOptions.CloudClient.startDeviceFlow.Content-Typeauthoritative.Why
Consumers need to identify their client at Cloud ingress without copying the SDK's OAuth device-flow and RFC 8628 polling implementation. The current helpers hard-code request headers and expose no transport extension point.
This is backward-compatible: existing calls require no changes.
Validation
npm run lint(passes with pre-existing warnings)npm run buildnpm run test:coverage -- --runInBand(290 tests)npm run format:check