feat: add authorizationToken config option - #136
Conversation
Adds an optional `authorizationToken` that attributes a test to a registered customer, so usage can be accounted for rather than being anonymous. The token is opaque to the engine: consumers obtain it from their own backend and pass it in. It is attached as a `token` query-string parameter to the endpoints that bill against a customer — the download, upload and load-generator requests, the TURN credentials request, and the results-logging requests. The reachability, RPKI and NXDOMAIN probes are excluded, since they target unrelated hosts. Sent in the query string rather than a header to avoid a CORS preflight on the measurement endpoints. Beyond the extra round trip, a preflight reuses the TCP connection for the real request, which would suppress the server-time calibration in BandwidthEngine that depends on observing a fresh handshake. The token is never attached over plain HTTP, so that a token cannot be exposed in cleartext. Defaults to null, leaving existing behaviour unchanged.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Both inline comments posted. Now the summary. Review summaryThis PR adds an optional What it changes
AssessmentThe core design is sound. I verified the claim that sub-engines inherit the token: Findings (both posted inline, non-blocking)
Correctness/security notes (no change requested)
No blocking issues. The two inline comments are quality/robustness suggestions. |
Absolute URLs are now parsed without a base, so only genuinely relative ones consult window.location.origin. Every default API URL is absolute, so constructing an engine with a token no longer needs a DOM. Also trims the comments added in the previous commit.
|
/bonk |
There was a problem hiding this comment.
Review: authorizationToken config option
Adds an optional, opaque authorizationToken that is baked into every billed API URL (downloadApiUrl, uploadApiUrl, turnServerCredsApiUrl, logAimApiUrl, logMeasurementApiUrl) as a token query-string param, defaulting to null so existing behavior is unchanged.
Verification
I traced the token through to the actual network calls and confirm the sub-engines inherit it:
- Download/upload + load-generator:
BandwidthEngine.ts:373andLoadNetworkEngine/index.ts:123both donew URL(apiUrl, ...)thensearchParams.set(...)their own params on top, so the baked-intokensurvives. - Per-measurement log:
LoggingBandwidthEngine.ts:113POSTs to the tokenizedlogMeasurementApiUrl. (Note the unrelatedtokenfield in the log body at line 104 is the server-issued measurement token, not the auth token — no collision since they use different transports.) - AIM results log:
logFinalResults.ts:134fetches the tokenizedapiUrldirectly. - TURN creds:
PacketLossEngine.ts:77fetches the tokenized URL directly.
The HTTPS-only guard, the query-string-over-header rationale, and the fresh-object mutation invariant (tested at authorizationToken.test.ts:75) are all sound and well justified. Ran the added unit tests (25 passing), tsc --noEmit (clean). The design is solid.
Findings
One non-blocking maintainability observation posted inline. The two earlier inline comments (redundant merge at index.ts:749, window dependency) were already addressed — the window crash is fixed in ce4c00b (absolute URLs parse without a DOM, covered by a test), and the redundant merge is a pre-existing pattern the author reasonably declined to widen this PR to fix.
Operational contract (out of diff, worth tracking)
The PR body correctly flags it: the billed endpoints must not return 4xx on an unrecognized/expired token. BandwidthEngine throws on any non-OK response and retries before surfacing Connection failed, so a rejecting endpoint would break the whole test — including for older library versions that never send a token. This is a server-side requirement, not fixable here, but should be tracked as follow-up.
No blocking issues.
|
Review posted. SummaryI reviewed PR #136 ( What it changes: Adds an optional, opaque Verification I did (not just claims):
Findings:
No blocking issues; the design is sound. |
The URLs carrying the token were enumerated in three places, so a new measurement endpoint could silently ship unattributed. TOKEN_URL_KEYS is now the only list, iterated by the helper and asserted exhaustively by the tests, which import it rather than keeping their own copy.
Address review feedback on the authorizationToken option: - Rename the query param from `token` to `auth`. The measurement log POST body already carries a `token` field holding a server-issued per-measurement value, and both are sent to logMeasurementApiUrl. - Mask the token in the fetch error paths. The failing URL reaches the consumer's onError callback, which is commonly forwarded to third-party log sinks. Also catch the log POST so a rejection cannot print the URL. - Rename TOKEN_URL_KEYS to AUTHORIZABLE_URLS and TokenizableUrls to AuthorizableUrls to convey eligibility rather than parsing. - Expose the merged config to subclasses via a protected getter, so SpeedTestEngine no longer rebuilds it just to read two fields.
Agreed with the API side. Still avoids colliding with the `token` field in the measurement log POST body.
|
/bonk |
There was a problem hiding this comment.
Review: authorizationToken config option
Optional, opaque authorizationToken (default null) baked into every billed API URL as a jwt query-string param — download, upload, TURN creds, and both logging endpoints. Reachability/RPKI/NXDOMAIN probes and turnServerUri are excluded.
Verification I ran (not just re-reading prior claims)
- Tests: installed deps,
pnpm test:unit→ 121/121 pass.pnpm lint→ clean. - Token survives to the wire:
BandwidthEngine.ts:374andLoadNetworkEngine/index.tsrebuild the URL and.set()their own params on top of the baked-injwt, so it isn't dropped. AIM (logFinalResults.ts:134) and TURN (PacketLossEngine.ts:77) fetch the tokenized URL directly. - No param collision: the
jwtquery param is distinct from the server-issuedtokenfield in the log POST body (LoggingBandwidthEngine.ts:104) — which is exactly why the rename tojwtin5853f9fwas necessary. Good call. - Redaction:
BandwidthEngine'sconsole.warn/onConnectionErrornow redact viaredactAuthorizationToken(BandwidthEngine.ts:557,566);LoggingBandwidthEngineswallows the best-effort log rejection so the tokenized URL can't surface in an unhandled rejection.logFinalResultsandPacketLossEnginecredential-failure paths pass the error, not the URL, so no leak there. - Prior review items are genuinely resolved in-branch: the
window-at-merge crash is fixed (ce4c00b, absolute URLs parse without a DOM, covered by a test assertingtypeof window === 'undefined'); the earlier "redundant second merge" inSpeedTestEngineis now gone — it readsthis.configvia a newprotected get config()getter (index.ts:143) instead of re-merging.
Findings
One real issue: the user-facing docs still say the param is named token, but the code sends jwt (authorization.ts:8). A consumer coordinating the server-side param name from the README/JSDoc would wire up the wrong name. Two inline comments below (defaultConfig.ts and README.md). Non-blocking but should be fixed before merge since the whole point of the jwt rename is the server-side contract.
Operational contract (out of diff, worth tracking)
As the PR body flags: billed endpoints must not 4xx on an unrecognized/expired token, or BandwidthEngine's retry-then-fail path (MAX_RETRIES = 20, no backoff) breaks the whole phase and reports a misleading "Connection failed". Server-side follow-up.
Design is sound; only the stale doc naming is worth a change here.
|
Reviewed PR #136 and posted a review with two inline comments. What it changesAdds an optional, opaque Verification I ran
Finding (non-blocking, but fix before merge)The code sends the param as
Since the PR explicitly requires the server-side param name to match, a consumer following these docs would wire up the wrong name. Both flagged inline. No correctness/security bugs in the diff. The 4xx-on-bad-token operational risk the PR body flags is a genuine server-side contract requirement but outside this change. |
The JSDoc and README still documented the param as `token`, which a consumer would wire up server-side. Both predate the rename to `jwt` in 5853f9f.
2 participants
Adds an optional
authorizationTokenthat attributes a test to a registered customer. It's opaque to the engine — consumers get it from their own backend:Defaults to
null, so existing behaviour is unchanged.Sent as a
jwtquery-string param on the download, upload, TURN-credential and results-logging requests (listed inAUTHORIZABLE_URLS). Never sent over plain HTTP. Reachability/RPKI probes are excluded — different hosts.Query string rather than an
Authorizationheader: a header adds a CORS preflight, and the warmed connection stopsBandwidthEngine's server-time calibration from seeing a fresh TCP handshake.Named
jwtrather thantokenbecause the measurement log POST body already has atokenfield carrying a server-issued per-measurement value, and both go tologMeasurementApiUrl. Requires the matching server-side param name.The token is masked before it reaches any log or callback. The failing URL is passed to the consumer's
onError, and consumers routinely forward those payloads to third-party log sinks.Assumes the download/upload endpoints don't reject on the token — an expired one should fall back to unattributed. A 4xx wouldn't halt a test, but it degrades badly:
BandwidthEnginetreats every non-OK response as a transient fault and retries 20 times with no backoff, so each affected phase burns 21 requests and reports a misleading "Connection failed" before moving on. Older versions that never send a token would be affected the same way. Worth fixing separately.The red
buildjob is a pre-existing e2e failure onmain(567aead), unrelated to this change.