fix(sdk): make the Node SDK usable against platform credentials - #67
Merged
Conversation
The default OAuth client auth now sends client_secret_post, matching how self-service apps are registered (the old always-Basic behavior was rejected with invalid_client); client_secret_basic and none are selectable via tokenEndpointAuthMethod. Error parsing prefers the activation envelope's stable code over the human message while keeping OAuth error/error_description handling, and 429s expose retryAfterSeconds. verifyWebhookSignature enforces a five-minute freshness window (configurable) as replay protection. Also brings the SDK up to the documented API surface: revokeToken, clientCredentialsGrant, revokeActivation, getAppConfig, listActivationRequests, listAuthorizations, idempotencyKey on create, revoked/deniedReason on the status response, and request lifecycle controls (injectable fetch, timeouts, per-request AbortSignal, transport failures wrapped as BottleneckAuthError). Tests run against the built artifact in both module formats; the integration suite gains a key-set tripwire so the status payload and the SDK type cannot drift silently again.
…kflow CI now runs the SDK suite on Node 18.17/20/22 (the claimed engines range) and installs the packed tarball into throwaway ESM and CJS consumers, so the export map and file list are exercised, not just the source tree. The build moves to prepack so git installs and plain npm pack produce a working package, the CJS declaration file is wired into the require condition, and the tarball now ships the repository's Apache-2.0 license (package.json previously claimed MIT; never published under it). Publishing is a tag-triggered workflow (sdk-node-v*) using npm trusted publishing with provenance; the npm side still needs the trusted publisher configured before the first release.
… labeling The README's callback example now stores state and the PKCE verifier in the session and validates state before exchanging the code; the old example invited a copy-pasted login CSRF. The install section says plainly that the package is not on npm yet, and the endpoints the SDK deliberately does not wrap are listed. The Go and Python clients are labeled as unsupported examples rather than SDKs, and their two real defects are fixed so copying them works against real credentials: introspection sends client_secret_post instead of Basic auth, and webhook verification checks timestamp freshness.
… bad input - timeoutMs and the caller AbortSignal now apply to reading the response body, not just the headers; a stalled or dropped body times out or aborts instead of hanging - 2xx bodies that fail to parse throw BottleneckAuthError with code invalid_response; mid-body transport failures wrap as network_error - verifyWebhookSignature returns false on a missing or non-string signature instead of throwing (Node and Python; Go already handled it) - createActivationRequest rejects idempotency keys outside the 8-255 range the server otherwise ignores silently - a configured clientSecret requires clientId at construction - BottleneckAuthError instanceof holds across the parallel ESM and CJS builds via Symbol.hasInstance - injected fetch is never invoked as a method of the transport object - Go example introspect sends client_id alone for public clients - verify-pack resolves the SDK dir with fileURLToPath so checkout paths with spaces work - README: import randomUUID from node:crypto (global crypto is Node 19+), document the list-endpoint caps and invalid_response
The route used the generic unauthorized() helper, whose envelope has no code field, so the SDK surfaced the human sentence as BottleneckAuthError.code on the one endpoint that broke the documented branch-on-code contract. Emit the OAuth envelope instead: invalid_request for a missing bearer token, invalid_token for an invalid one.
The SDK rejects webhook timestamps older than 300s while retry delays run to 24h, so deliveries stay verifiable only because deliverOne stamps and signs at send time on every attempt. Nothing asserted that invariant, and freezing the timestamp across attempts would pass every existing test while breaking all late retries at SDK consumers.
Both list endpoints return at most the most recent rows (50 requests, 200 authorizations) with no pagination, and the docs suggested using them to recover ids and reconcile grants without mentioning it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An external SDK review found the default OAuth flow incompatible with credentials the platform actually issues: self-service apps are registered with token_endpoint_auth_method = client_secret_post, but the SDK always sent Basic auth, so the README's exchangeCode/refreshToken/introspect examples failed with invalid_client. This closes that and the rest of the review's actionable findings.
Node SDK (0.3.0, still unpublished):
Tests and packaging:
Docs and example clients:
Review attention: the webhook freshness default is a behavior change for any existing verifier caller (none known beyond our own tests, which now use fresh timestamps), and the 0.2.0 -> 0.3.0 breaking changes are catalogued in sdk/node/CHANGELOG.md.