fix: send the authorization token in a header, not the URL - #139
Merged
Conversation
Request URLs are routinely persisted in server-side logs, traces and error reports, so a bearer credential must not travel in one. The token now rides an `Authorization: Bearer` header on the measurement, TURN credential and logging requests. Resolved per target URL rather than once per engine, so the existing "never over plain HTTP" guard still holds for configs that mix schemes. The public config is unchanged: consumers still set `authorizationToken`. The URL-redaction helpers are gone, since no URL carries the token anymore.
|
@andre-j3sus Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
Collaborator
Author
|
/bonk |
Collaborator
Author
|
/bonk |
Collaborator
Author
|
/bonk |
andre-j3sus
marked this pull request as ready for review
August 18, 2026 17:33
Collaborator
Author
|
/bonk |
Collaborator
Author
|
/bonk |
Collaborator
|
Can we add a "allowInsecureAuthorizationToken: false" to allow http for local development? |
The HTTPS-only guard makes the token untestable against a local `http://` server. This adds an explicit opt-out, defaulting to false, rather than auto-trusting loopback: the dev server is reachable over non-loopback hostnames too, and an explicit flag is greppable in a consumer's config where implicit detection would not be. Warns once when it actually takes effect — a run issues hundreds of requests, so the warning is latched rather than per-request. The token and this flag now travel together as one `AuthorizationOptions` through the engines, so the next transport change stays one parameter wide instead of adding a second one to every call site. Also fixes type errors in two test files that neither eslint (src only) nor tsdown (src only) typechecks.
Collaborator
Author
|
Added in 1ffd5b2 — I went with the explicit flag rather than auto-trusting loopback, for two reasons. The dev server is reachable over non-loopback hostnames ( Two things still needed on the server side before this is testable end to end:
|
The loaded-jitter getters return `null` with fewer than 2 samples, but the guards only excluded `undefined`, so a `null` reached a numeric matcher and threw `actual value must be number or bigint, received "object"`. Constrained CI runners hit this; local runs collect enough samples and never did. Guarding on `typeof === 'number'` can't let a null through to a numeric matcher, and covers the `undefined` case the guards were already there for.
devandrepascoa
previously approved these changes
Aug 19, 2026
Addresses MR review on #139. - `authorizationEnabled` ('header' | boolean, default undefined) opts in to sending the token, which is otherwise withheld even when set. `true` is documented as a permanent alias for 'header' rather than "the default transport", so adding a transport later cannot silently change what an existing config does. `false` is accepted so disabling can be explicit. - Latch the insecure-transport warning per options object rather than per module, so a second run in the same page warns again. This needs the options object to be stable, so it is now built once in the constructor instead of per getter access. Kept out of `AuthorizationOptions` via a WeakSet, since that type is public and a mutable internal flag does not belong in it. - Destructure `authorization` explicitly in LoggingBandwidthEngine and pass it back to `super`, rather than reaching into the rest spread. - Return headers as a plain object. Note this is what surfaced the weak assertion in the "never puts the token in the URL" test: it only passed because a Headers instance serialises to `{}`, hiding the token from JSON.stringify rather than proving anything about the URL. - Treat a whitespace-only token as absent, and trim padding instead of sending `Bearer `. - Omit `credentials` rather than setting it to undefined, in all three places. - Mark the whole feature experimental: `@experimental` in the JSDoc (which reaches the generated .d.ts) and 🧪 in the README, with a note that these options may change or be removed in any release.
LoadNetworkEngine saturates the same __down/__up endpoints as a real measurement, but its fetch was the one measurement call site not wrapped in withAuthorizationHeader, so that traffic went out unauthenticated. This was a regression from the header move. The old design rewrote the config URLs — downloadApiUrl and uploadApiUrl were both in AUTHORIZABLE_URLS — so anything consuming those URLs inherited the token for free, this engine included. Attaching per fetch site means every site has to be enumerated, and this one was missed. Only reachable via packetLossUnderLoad (or packetLoss with loadDown/loadUp), which the default measurement list does not use, so no shipped config was affected. It is still the heaviest traffic in a test when it does run, which is the last thing that should be silently unattributed. Covered by a unit test that instantiates the engine, since the e2e suite skips packet loss for CORS reasons and would never have caught it. ReachabilityEngine remains deliberately unwrapped: the probe hosts are unrelated endpoints. Also passes authorization to super explicitly in PacketLossUnderLoadEngine rather than through the rest spread, matching LoggingBandwidthEngine, and fixes two comments: an obsolete rationale on the log fetch (it referred to the token being in the URL, which it no longer is) and a withAuthorizationHeader docblock that had grown into design justification belonging in the PR.
Collaborator
Author
|
/bonk |
devandrepascoa
approved these changes
Aug 20, 2026
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
2 participants
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.
The optional
authorizationTokenwas appended to request URLs as ajwtqueryparameter. Request URLs are routinely persisted well outside the request itself
— server-side request logs, tracing span tags, error-reporting breadcrumbs — so
a bearer credential travelling in one leaks into every system that records a
URL.
This moves the token to an
Authorization: Bearerheader on the requests thatneed it: the download/upload measurement requests, the load-generating
requests that run alongside them, the TURN credential fetch, and the two
logging endpoints.
Because the token is now withheld from any endpoint that isn't known to be
HTTPS, a new
allowInsecureAuthorizationTokenoption (defaultfalse) optsback in for local development against an
http://server. It warns once whenit takes effect.
A token alone is not enough to send one: the new
authorizationEnabledoption(
'header' | boolean, defaultundefined) has to opt in.trueis a permanentalias for
'header'rather than "whatever the default transport is", so atransport added later cannot silently change what an existing config does.
All three options are marked experimental —
@experimentalin the JSDoc, whichreaches the published
.d.ts, and 🧪 in the README. NoteauthorizationTokenalready shipped in v1.13.0, so this is a retroactive relabel rather than a new
option being introduced as unstable.