Repository navigation
Send a proper User-Agent instead of okhttp's default - #735
Conversation
SDK requests went out as `okhttp/5.4.0`, which makes them indistinguishable from any other OkHttp client and leaves a server nothing to attribute a request to. This surfaced when Automattic systems could not identify the WordPress/Jetpack Android app's Gravatar traffic while investigating an edge rule on api.gravatar.com. `buildOkHttpClient` is the single funnel for every API client in the SDK, so setting the header in `SdkVersionInterceptor` covers both the core services and the Quick Editor's internal ones, with no change to any call path. It is only set when absent, so a consumer that passes its own client keeps its own User-Agent, and OkHttp's BridgeInterceptor then leaves the header alone. `Gravatar.appIdentifier()` lets an app state the name it is already known by server-side, which `context()` cannot do well: the launcher label is localized and usually suffixed per build type, so one app reaches the backend under several names. `context()` now also reads the package version. The Quick Editor's image download uses DownloadManager rather than OkHttp, so the interceptor cannot reach it and it sets the header itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📲 You can test the changes from this Pull Request in Gravatar Demo by scanning the QR code below to install the corresponding build.
|
Two fixes from reviewing the User-Agent change.
Reject identifiers a header cannot carry. `appVersion` was interpolated
into the User-Agent raw, while `appName` one line above was trimmed and
whitespace-dashed. A version holding anything outside tab and printable
ASCII — `appIdentifier("wp-android", "1.0-β")`, or an `android:versionName`
that aapt2 happily accepts — made OkHttp reject the header from inside
the interceptor, failing every request with an opaque `ErrorType.Unknown`
and rethrowing on the dispatcher thread. `appIdentifier` now fails on the
call that supplied the bad value instead; whitespace stays legal, since a
header value may contain it. `context()` only drops an unusable
`versionName` and logs, rather than throwing: the caller did not choose
that string, and the name alone still identifies the app.
Make `appIdentifier` take precedence, as its KDoc already claimed. Both
it and `context()` were unconditional writes to the same fields, so the
order decided the winner, and the only documented initialization ends in
`.context(appContext)`. An app naming itself in `Application.onCreate`
silently reverted to the localized, build-type-suffixed launcher label
the next time anything called `context()` — exactly what the identifier
exists to avoid. `context()` now leaves an explicit identifier alone in
either order.
Also documents `appIdentifier` in the get-started guide, which only
showed `context()`, along with the "set it before the first request"
requirement that lived solely in the pull request description.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The composed User-Agent was only observable with a proxy, so there was no way to check what an app actually sends short of intercepting its traffic. `Gravatar.userAgent` is public for exactly this, so the demo now displays it above the tabs, with a button for each way of naming the app. Pressing `context()` after `appIdentifier()` leaves the header alone, which makes the precedence rule visible in a running app rather than only in a unit test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| Logger.w(LOG_TAG, "Ignoring context(): appIdentifier() already named this app.") | ||
| return this | ||
| } | ||
| GravatarSdkContainer.instance.appName = getApplicationName(context) |
There was a problem hiding this comment.
context() saves the launcher label as appName without checking it. appIdentifier() checks with isHeaderSafe, and so does the version path, but the name from context() is never checked.
If the label has non-ASCII characters (for example グラバタ or Café), OkHttp throws when it sets the header, so every SDK request fails. A non-breaking space (Jetpack\u00A0Debug) does the same, because the \s regex doesn't replace it.
This already happens on trunk through X-Source, but this PR adds User-Agent as a second way to hit it. The KDoc in GravatarSdkContainer also says context() drops unsafe values, but that's only true for the version.
Could we make the label header-safe before saving it, or skip it the same way we skip a bad versionName? I checked this with a unit test: context() with グラバタ and then a request through SdkVersionInterceptor throws IllegalArgumentException.
There was a problem hiding this comment.
Good catch! I've aligned and set the same check and flow for both now: 5227b7b
| @@ -24,6 +25,7 @@ internal class ImageDownloader( | |||
| .addRequestHeader("X-Platform", "Android") | |||
| .addRequestHeader("X-SDK-Version", BuildConfig.SDK_VERSION) | |||
| .addRequestHeader("X-Source", appName) | |||
There was a problem hiding this comment.
This request now gets User-Agent from Gravatar.userAgent, which uses the name from appIdentifier(). But X-Source still uses context.appName, which is the launcher label. So one request can carry two different app names.
Example: the app calls appIdentifier("wp-android", "27.6"). API calls send X-Source: wp-android, but the download sends X-Source: Jetpack Debug together with User-Agent: wp-android/27.6 ….
Could we use the same name for both headers? One option is to expose the SDK's app name from gravatar and only fall back to the launcher label when it's not set.
| val userAgent: String | ||
| get() = buildString { | ||
| appName?.let { name -> | ||
| append(name.trim().replace(WHITESPACE, "-")) |
There was a problem hiding this comment.
Two problems with how the app name goes into the User-Agent:
-
Characters that aren't allowed. We only replace whitespace. Characters like
/,(,)and;stay in, and they aren't allowed in a User-Agent product token. Example: a labelA/B Testerwith version2.0givesA/B-Tester/2.0 GravatarAndroid/…. A parser will read the product asAand the version asB-Tester/2.0, so server rules keyed on the app name won't match. -
Blank name. If the launcher label is empty or only spaces, the User-Agent becomes
/27.6 GravatarAndroid/…, which has no app name before the slash. We also send an emptyX-Sourceheader.appIdentifier()already rejects a blank name, butcontext()doesn't.
Could we replace every character that isn't a valid token character, and treat a name that ends up empty as "no name"? Then the User-Agent would be just GravatarAndroid/… and no X-Source would be sent.
There was a problem hiding this comment.
This is related to comment 1, so also fixed here: 5227b7b
| // OkHttp's BridgeInterceptor runs after this one and also only fills a missing header, | ||
| // so setting it here is what stops the default `okhttp/<version>` from going out. | ||
| if (request.header(USER_AGENT) == null) { | ||
| header(USER_AGENT, GravatarSdkContainer.instance.userAgent) |
There was a problem hiding this comment.
This sets the User-Agent only for the OkHttp clients the SDK builds for API calls. Images in gravatar-ui (Avatar, SocialMedia) and gravatar-quickeditor (SelectableAvatar, AltTextPage) load through Coil's default ImageLoader, which still sends okhttp/4.12.0.
So avatar and social icon requests to gravatar.com won't carry the new User-Agent, even though the PR description and docs say "every SDK request". Is that on purpose? If yes, could we update the docs to say API requests only? If not, we could pass an imageLoader that adds the header to those AsyncImage calls.
There was a problem hiding this comment.
I've opted to include the user-agent here too: 926c7ee
The app name reached the wire two different ways: the User-Agent replaced whitespace in the launcher label, while X-Source sent the label verbatim. So `A/B Tester` became the product `A` to any parser, a blank label produced a leading `/27.6`, and a non-ASCII label made OkHttp throw from inside the interceptor and fail every request the SDK made. Reduce the label to an RFC 9110 product token once, on write, and send that token in both headers. A label with nothing a token can hold leaves X-Source off and the User-Agent as the SDK token alone, rather than sending something unusable or nothing at all. `appIdentifier` now rejects a name that is not already a token instead of quietly rewriting one the caller chose, suggesting the token it would have made. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ImageDownloader` read the launcher label straight off the `Context` for
`X-Source` while taking `User-Agent` from the SDK, so a download made with
`appIdentifier("wp-android", "27.6")` set went out as `X-Source: Jetpack Debug`
alongside `User-Agent: wp-android/27.6 …` — one request naming the app two
ways, and a label `DownloadManager` could not send would have broken the
download the way it used to break API calls.
Expose the name the SDK sends as `Gravatar.appName`, next to the User-Agent it
already exposes, and have the downloader ask for that instead of deriving a
second name of its own. The `Context.appName` extension stays for the OAuth
page, which wants the localized label precisely because it shows it to a person.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Avatars and social icons load through Coil, whose ImageLoader builds its own OkHttp client that the SDK's interceptors never see. So the requests this SDK makes most of — one per avatar rendered — were still reaching gravatar.com as `okhttp/<version>`, the anonymous default the User-Agent exists to replace, while the docs claimed every SDK request carried it. Set the header on the ImageRequest instead of supplying an ImageLoader of our own, so the host app's cache, interceptors and dispatchers are left alone and the only thing the SDK adds is the name. The helper is duplicated in both UI modules because `internal` does not cross a module boundary and the core module has no business depending on Coil. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `@throws` on `appIdentifier` only mentioned values a header cannot carry, but the name is now held to the stricter bar of a product token, so `My App` is rejected too — and nothing told a reader that, or that a launcher label `context()` derives gets reduced before it is sent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@AdamGrzybkowski I think the PR is ready for another go :) |
AdamGrzybkowski
left a comment
There was a problem hiding this comment.
Thanks @adalpari, all four comments are fixed. 🚀
You can test this PR in the app using the snapshot CI published:
2.5.1-128-g8efa9e0a-SNAPSHOT
Add the snapshots repository to settings.gradle.kts:
maven {
url = uri("https://central.sonatype.com/repository/maven-snapshots/")
content { includeGroup("com.gravatar") }
}
TL;DR
This PR is adding a mechanism to set the
user-agentthe sdk is using for its requests. Right now, it's using the OkHttp default one, and we need to inject the proper UA.Description
Why this is needed
SDK requests currently go out with OkHttp's default
User-Agent: okhttp/<version>— the SDK declares 4.12.0, but consumers routinely resolve higher, so what reaches the backend is whatever their build landed on. That string identifies the HTTP library and nothing else, so on the server side every Gravatar SDK request looks identical to every other OkHttp client on the internet — there is no way to tell which app made a call, which version it was running, or even that it came from our SDK at all.That has three concrete costs:
On point 3: the services do accept an
OkHttpClientvia constructor, but the Quick Editor never exposes that.QuickEditorContainerisinternalwith aprivate constructor, itsinit()takes only aContext, itsokHttpClientis aprivate valbuilt inline, and it constructsProfileService()with no client at all.GravatarQuickEditor.show()andGravatarQuickEditorParamshave no client parameter either. So a consumer using the Quick Editor has no injection point anywhere, and short of rebuilding the profile and avatar-picker UI against the raw services, cannot set a header on these requests.This surfaced concretely while Automattic systems were investigating an edge rule on
api.gravatar.comthat returned403to the WordPress/Jetpack Android app for roughly ten months. They asked us to send an identifying User-Agent so the rule could be keyed on something stable — and we found there was no way to do it from the app at all.Where the header is set.
GravatarSdkContainer.buildOkHttpClientis the single place the SDK constructs anOkHttpClientfor its API calls, and it always addsSdkVersionInterceptor. Setting the header there covers the core services and the Quick Editor's internalProfileService()/AvatarService(), with no change to any call path and nothing for consumers to opt into.It is only set when absent, so a consumer that passes its own client keeps its own
User-Agent. OkHttp'sBridgeInterceptorruns afterwards and also only fills a missing header, so this is what stops the default from going out.Identifying the app.
Gravatar.appIdentifier(name, version)lets an app state the name it is already known by server-side. The name has to be aUser-Agentproduct token — no whitespace, no/, unaccented ASCII — and anything else throws at the call rather than failing requests later, so the string a server keys a rule on is exactly the string you passed.context()can't do that well — the launcher label is localized and usually suffixed per build type (JetpackvsJetpack Debug), so one app reaches the backend under several names and carries no version.context()still works and now also readsversionNamefromPackageManager, so existing consumers get a better UA for free.Resulting header:
User-AgentGravatar.appIdentifier("wp-android", "27.6")wp-android/27.6 GravatarAndroid/2.5.0Gravatar.context(context)Jetpack-Debug/27.6 GravatarAndroid/2.5.0GravatarAndroid/2.5.0How to use it from an Android client
Set the identifier once, early —
Application.onCreate()is the natural place — and every subsequent SDK request carries it. It is a plain setter on a singleton, so it must run before the first request, but it does not have to run before SDK initialization and is cheap enough not to matter on the startup path.That produces
User-Agent: wp-android/27.6 GravatarAndroid/2.5.0on every request the SDK makes, including the ones the Quick Editor makes internally.Pick
namedeliberately: it should be the identifier your backend already uses for the app, stable across releases and build types, so one rule covers you everywhere. Prefer it overcontext(), which derives the name from the launcher label — that is localized and usually suffixed per build type, so the same app arrives asJetpackandJetpack Debugdepending on the build.If you have no such identifier and just want attribution,
context()still works and now fills the version too:Calling neither is fine — you get
GravatarAndroid/2.5.0, which at least identifies the SDK. And if you already pass your ownOkHttpClienttoProfileService/AvatarServicewith aUser-Agentof your own, the SDK leaves it alone, so nothing changes for you.Gravatar.userAgentexposes the composed string if you want to reuse it on requests you make yourself.Requests the interceptor can't reach. Two paths don't go through an SDK-built
OkHttpClient, and both set the header themselves:ImageDownloaderdownloads throughDownloadManager. It setsUser-Agentalongside theX-Platform/X-SDK-Version/X-Sourceit already sent — andX-Sourcenow comes fromGravatar.appNamerather than being re-derived from the launcher label, so a download can no longer name the app one way there and another in theUser-Agent.ImageLoaderbelongs to the host app and builds its own OkHttp client. Those requests — one per avatar rendered, so plausibly the bulk of what the SDK sends — now carry the header on theImageRequestitself. Setting it there rather than supplying anImageLoaderof our own leaves the host's cache, interceptors and dispatchers untouched.One name, on every header. The app name is reduced once to an RFC 9110 product token and that token is what goes out, in both
X-Sourceand theUser-Agent. This is a behaviour change forX-Source, which previously sent the launcher label verbatim: a label containing whitespace or punctuation now arrives tokenized (Jetpack (Debug)asJetpack-Debug), and a label a token cannot represent at all — one written entirely in kana — is omitted rather than sent. Worth a look from anyone who consumes that header. It also fixes a latent crash: a non-ASCII label made OkHttp throw from inside the interceptor and fail every request the SDK made, which was already reachable ontrunkthroughX-Source.Gravatar.appNameexposes the name the SDK sends, next toGravatar.userAgent, so a consumer — or the Quick Editor's downloader — identifies the app the same way instead of deriving a second name of its own. It is null when there is no usable name.gravatar/api/gravatar.apiwas regenerated withapiDump. The change is purely additive (appIdentifier,userAgent,appName), so it stays binary compatible.Testing Steps
Automated:
50 new tests across
SdkVersionInterceptorTest,GravatarSdkContainerTest,GravatarTest,ImageDownloaderTestand aGravatarImageRequestTestper UI module; none removed. Screenshot baselines are unchanged.Each guard is mutation-checked — reverting it fails the tests that cover it:
User-Agentset only when absentgiven a request that already has a User-Agent …GravatarSdkContainerTestcasesappIdentifiername validationGravatarTestcasesX-Sourcefrom the tokenSdkVersionInterceptorTestcasesX-Sourceon downloadsImageDownloaderTest, withexpected:<wp-android> but was:<com.gravatar.quickeditor.test>User-Agenton image loadsGravatarImageRequestTestcases per moduleThe non-ASCII case reproduces the original crash exactly: without the guard,
SdkVersionInterceptorTestfails withjava.lang.IllegalArgumentException: Unexpected char 0x30b0 at 0 in X-Source value: グラバタ.Manual, from a consuming app:
Gravatar.appIdentifier("your-app", "1.0")before opening the Quick Editor.api.gravatar.com(mitmproxy, or a logging interceptor on a client you pass in).User-Agent: your-app/1.0 GravatarAndroid/<version>on/v3/me/profileand/v3/me/avatars.DownloadManagerrequest.AvatarorLargeProfileand confirm the avatar image request carries it too, rather than OkHttp's default.The demo app shows the composed
User-Agentin a bar above the tabs, with buttons forappIdentifier()andcontext(), if you want to see it change without a proxy.Notes for reviewers
Two judgement calls worth a second opinion:
Build.MODEL/Build.VERSION.RELEASE. Thegravatarmodule has no Robolectric and noreturnDefaultValues, so those read asnullin unit tests — it would have either put"null"in the string or forced new test infrastructure into a module that has none.X-Platform: Androidalready carries the platform. Happy to add it if you'd rather have it.gravatar-uiandgravatar-quickeditor.internaldoesn't cross a module boundary and the core module has no business depending on Coil, so the alternative was making it public API. Six lines each felt like the cheaper of the two.appIdentifierhas no@JvmOverloads, matching the existingapiKeyandcontext, neither of which is annotated. Worth adding across all three if Java consumers matter, but that felt like a separate change.🤖 Generated with Claude Code