Rework DPoPSigner, fixing nonce tracking and other errors by ThisIsMissEm · Pull Request #50 · ChimeHQ/OAuthenticator

ThisIsMissEm · 2026-02-21T23:59:08Z

This completely reworks the DPoP Nonce handling, such that you have one DPoPSigner still, however, it is request-origin aware, and tracks DPoP Nonce's per origin.

The DPoPSigner internally uses a method to convert URLResponseProvider's URLResponse to a HTTPUrlResponse in order to simplify the logic in the DPoPSigner code.

The DPoPSigner.JWTParameters no longer contains a issuer as this isn't in the DPoP JWT Parameters, and would leak the user's authorization server to a third-party.

We now correctly handle errors and retries: If the Authorization Server (issuer) returns a OAuth Error Response we will only retry if the error is use_dpop_nonce per RFC 9449. This means that all other errors from the Authorization Server do not trigger a retry.

If we are requesting against a Resource Server (not a Authorization Server), and we get a WWW-Authenticate header with a value like DPoP error="use_dpop_nonce", then we will retry the request with the server-supplied DPoP-Nonce. All other errors from the Resource Server will not be retried.

No retries will happen if the server does not return a DPoP-Nonce header, or if the new DPoP-Nonce header is the same as the previous DPoP Nonce that we had for that origin, since the retry would just fail again. (That is it's only possible to retry on DPoP Nonce failure if the response contains a new DPoP-Nonce value.

The DPoP htu is now correctly the request URL without query parameters or hash fragments, per https://datatracker.ietf.org/doc/html/rfc9449#section-4.2-4.6 (This is the requestEndpoint in DPoPSigner.JWTParameters). This could have been causing issues previously.

Test Coverage

I've significantly increased the test coverage of the DPoPSigner, going from one test to an entire suite containing 16 test cases (5 are in a parameterized test case).

This test coverage includes verifying that different origins don't clobber each other's DPoP Nonce values, and asserting that the retry logic works correctly.

Before Merging

Test in an actual application to verify no regressions
Clean up commit history
Rebase after Run swift linux tests in docker container #52 is merged – that PR fixes the failing linux tests.

ThisIsMissEm · 2026-02-22T00:16:56Z

-
 	public func response(
 		isolation: isolated (any Actor),
 		for request: URLRequest,


We don't currently have handling for this, due to the FIXME below, however, the request can include a Authorization: DPoP access-token header, this is the case when making a request with an OAuth Session. We do not encounter this in the tests currently.

If we see that we have an Authorization: DPoP access-token header, then we need to take the access-token value and get the sha256 of the first five characters of that value, and use that as the new Authorization header, so:

token = "e8db0489-6851-4f1d-bafa-b3914b37eb9c" tokenHash = "jxjZyne7ZIPxhQeKKN-_qgN40uiaMa8izmRqIRqCGow" request = { Authorization: "DPoP e8db0489-6851-4f1d-bafa-b3914b37eb9c" } // `buildProof` method receives: // - not: `e8db0489-6851-4f1d-bafa-b3914b37eb9c` // - but: `jxjZyne7ZIPxhQeKKN-_qgN40uiaMa8izmRqIRqCGow` // // Which is: base64Url(SHA256("e8db0489-6851-4f1d-bafa-b3914b37eb9c")) request = { Authorization: "DPoP JWT('jxjZyne7ZIPxhQeKKN-_qgN40uiaMa8izmRqIRqCGow')" }

This prevents leaking the Access Token to an untrusted party through the DPoP ath parameter for a request with an Authorization header. However, as we're not using swift-crypto here, we actually need to use the tokenHash supplied from a external source, which is what the FIXME is about.

The current implementation would just completely discard the existing Authorization header.

We could provide a SHA256 method to DPoP Signer which would allow the DPoP Signer to calculate the tokenHash. It would just be pkce.hashFunction however in the current code, and that's probably just as messy, and we should prefer using swift crypto instead.

Source: https://github.com/bluesky-social/atproto/blob/4e96e2c7b7cc0231607d3065c95704069c4ca2a2/packages/oauth/oauth-client/src/fetch-dpop.ts#L54

I think a function to abstract crypto needs, along with a conditionally-compiled import of CryptoKit + default implementation would actually work great for stuff like this. I think I've done that in other places too.

ThisIsMissEm · 2026-02-22T00:31:40Z

 	}

-	@discardableResult
-	public func setNonce(from response: URLResponse) -> Bool {


I'm not sure where this method was ever called. I couldn't find any references to it in the code.

it's public API, so question is if this is valid public API; why would a client call it?

Well, that's the thing, a client shouldn't ever be mutating the nonce cache. It's an internal detail to the OAuth Session / OAuth Client.

@mattmassicotte why does this API exist? I can't find any references to it anywhere.

I cannot recall, haha.

I'm fine removing public API here, because it sounds like this should not be used anyways. And in that case, introducing breakage seems like it makes sense to me.

ThisIsMissEm · 2026-02-22T00:47:20Z


 			let (data, _) = try await params.responseProvider(request)

 			let tokenResponse = try JSONDecoder().decode(TokenResponse.self, from: data)


We'd use OAuthErrorResponse here too, to actually decode Authorization Server errors, and throw those as a well formed Error. This would give the client details as to why the token exchange request failed.

We've actually a fix for this in a different branch, and I'll try to get that upstreamed to you.

ThisIsMissEm · 2026-02-22T01:04:52Z

+	}
 }

 extension DPoPSigner {


Technically, the DPoPSigner should be negotiating with the authorization server as to supported key algorithms, as to not use a incorrect key with the authorization server: https://github.com/bluesky-social/atproto/blob/4e96e2c7b7cc0231607d3065c95704069c4ca2a2/packages/oauth/oauth-client/src/fetch-dpop.ts#L226

However, in AT Protocol, we have this lovely part of the spec:

The ES256 (NIST "P-256") cryptographic algorithm must be supported by all clients and servers for DPoP JWT signing. The set of algorithms recommended for use is expected to evolve over time. Clients and Servers may implement additional algorithms and declare them in metadata documents to facilitate cryptographic evolution and negotiation.

From: https://atproto.com/specs/oauth#demonstrating-proof-of-possession-d-po-p

So because we're using ES256 keys in our DPoPKey implementation, this "mismatch" between supported algorithms and the key's algorithm, we never hit this issue. Yay, shortcuts?

mattmassicotte · 2026-02-23T13:01:50Z

Wow! I'm delighted to see you looking at this. I don't really understand this area very deeply, so I'm sure you'll find lots of stuff to improve.

While I have no looked closely at all, one thing does give me pause. Changing the type of URLResponseProvider is going to make it hard for even me to adapt to this, and my own usage is very simple.

I will admit that I do not actually even know if there are other subclasses of URLResponse besides HTTPURLResponse. However, that is how Foundation is arranged and we cannot change it.

I wonder if this could be achieved in a less-invasive way by building a small wrapper function with this type and shape?

public typealias HTTPURLResponseProvider = @Sendable (URLRequest) async throws -> (Data, HTTPURLResponse)

ThisIsMissEm · 2026-02-24T00:59:56Z

@mattmassicotte okay, now I know why you responded to this, because it caught me by surprise! I'd actually intended to open against the germ-network repo, but uhh.. github's UI got me. I guess it's fine you saw this before it was ready.

ThisIsMissEm · 2026-02-24T02:45:18Z

+		var originComponents = URLComponents()
+		originComponents.scheme = scheme
+		originComponents.host = host
+		originComponents.path = self.relativePath


Should this actually be self.path? https://www.avanderlee.com/swift/url-components/#working-with-url-components

Yes it should! Not sure how that stuck in there.

Weirdly self.path isn't available on all platforms OAuthenticator supports, I think?

mattmassicotte · 2026-02-24T16:02:17Z

@ThisIsMissEm ahh. Ha! well that's fine. I've just been letting you do your thing, but if you need/want any feedback from me at any point I'd be happy to provide it.

ThisIsMissEm · 2026-02-24T16:19:54Z

@mattmassicotte we're just putting it through it's paces in practice in an application, and the other Germ team members found some issues, and I'm going to be debugging them with them. Interestingly enough these bugs did not show up in test coverage.

ThisIsMissEm · 2026-03-03T00:03:23Z

We've now tested this PR in an actual application, to verify it's working as expected. There was an issue discovered due to how the code figured out whether or not it was a request against an authorization server or not, which triggers different error handling to detect DPoP Nonce errors.

I've now fixed that issue.

ThisIsMissEm · 2026-03-03T00:18:30Z


 		let storedLogin = Login(
-			accessToken: Token(value: "EXPIRE SOON", expiry: Date().addingTimeInterval(5)),
+			accessToken: Token(value: "EXPIRE SOON", expiry: Date().addingTimeInterval(15)),


See b81a754

This was an issue due to timing.

I struggled a lot with figuring out a reliable way to do this, and I'm not 100% sure it actually is fixable with only delays. However, I don't think that actually matters overall and I'm very happy to kick this can down the road.

This also ensures we clearly signal to the DPoP request method whether we are talking to an authorization server or a resource server (since these give different OAuth Error responses). The test coverage has increased dramatically here.

ThisIsMissEm · 2026-03-06T15:43:00Z

@mattmassicotte this should now be ready for review + merge.

(We've tested in https://github.com/germ-network/ATProtoLiteClient/compare/test/oauthenticator-dpop-changes )

mattmassicotte · 2026-03-16T14:25:22Z

+#endif
+
+// Only sets the port if the port is not the default port for http or https requests:
+internal func omitWebDefaultPort(components: inout URLComponents, port: Int?, scheme: String) {


This is purely stylistic, but I think\ an extension on URLComponents with a mutating member is a nice way to do exactly this that keeps the functionality scoped to the type it can operation on.

mattmassicotte · 2026-03-16T14:28:16Z

I trust you on the implementation here. But I don't fully get the use of a cache, particularly one that is owned by a value type. Can you help me understand that better?

germ-mark · 2026-03-16T15:59:53Z

The reference type NSCache is a property of the class DPoPSigner which is then held as a property within an Authenticator object?

Because the nonces are issued on a rolling time window for an Oauth Session (mapping 1:1 to an Authenticator object) (and valid concurrently within that window), it seems correct to have stateful storage (but doesn't need to be persisted because of the short time window) within Authenticator of the nonce (+origin) sliding window. I think Emelia reached for the NSCache as a self-evicting data structure for the [Origin: Nonce] map.

ThisIsMissEm · 2026-03-16T17:11:58Z

Yeah, and because we can explicitly limit the cache size if you're interacting with many different resource server origins (e.g., multiple app views). Most apps will likely only have the Authorization Server and the PDS in here

mattmassicotte · 2026-03-18T17:41:03Z

The reference type NSCache is a property of the class DPoPSigner which is then held as a property within an Authenticator object?

That's what I expected to see, but it does not look to me like that's how it works. Am I missing it?

if you're interacting with many different resource server origins

This is also something I don't fully understand. How would this work? One Authenticator object that is shared across different domains?

ThisIsMissEm · 2026-03-18T19:11:27Z

The reference type NSCache is a property of the class DPoPSigner which is then held as a property within an Authenticator object?

That's what I expected to see, but it does not look to me like that's how it works. Am I missing it?

Yes, the DPoPSigner is a singleton across the OAuthenticator instance. You could definitely change the API to allow passing in a DPoP Cache from outside of the OAuthenticator instance, but there's not a huge value to that. Majority of the requests you make will be through requests to resource servers, not authorization servers.

So because the nonce cache (NSCache) is a private property of the DPoPSigner, you effectively have a singleton of that too per Authenticator. The code this was inspired by does actually pass a DPoP Cache into a signer, as to share it across multiple sessions and clients, but that's not the architecture of OAuthenticator.

if you're interacting with many different resource server origins

This is also something I don't fully understand. How would this work? One Authenticator object that is shared across different domains?

Yes, resource servers can and often are on different domains to authorization servers. That's also why there's that note about handling 401's for resource requests is currently incorrect.

mattmassicotte · 2026-03-18T19:22:13Z

Ok I understand, thank you!

NSCache has a long history and is a surprisingly complex type that doesn't always work as people expect given the name. It might make sense to (eventually) revisit this so library clients have a little more control. But this would be completely transparent API-wise and I don't think it will affect any of the behaviors here.

So let's just go for it!