Duansg commented on PR #4279:
URL: https://github.com/apache/hertzbeat/pull/4279#issuecomment-5242704615
Registered grant_types is validated but never enforced
oauth_register carefully validates request.grant_types — it requires
authorization_code and rejects anything outside authorization_code |
refresh_token — and then echoes the value back to the client in
additional_fields["grant_types"]. But the record it actually stores has no such
field:
struct RegisteredClient {
client_id: String,
client_secret: Option<String>,
redirect_uris: Vec<String>,
token_endpoint_auth_method: TokenEndpointAuthMethod,
registered_at: DateTime<Utc>,
expires_at: DateTime<Utc>,
refresh_expires_at: Option<DateTime<Utc>>,
// no grant_types
}
So neither side of token handling can consult it:
- issue_tokens_locked mints refresh_token: Some(refresh_token_value)
unconditionally and inserts it into refresh_tokens; there is no branch on what
the client registered.
- oauth_token dispatches purely on the request string — match
request.grant_type.as_str() { "refresh_token" =>
state.exchange_refresh_token(&request).await, .. } — without looking the client
up.
Net effect: a client that registered only authorization_code still receives
a refresh token, and can still redeem it via grant_type=refresh_token. The
constraint the server states back to the client in the registration response is
not one it enforces.
Scope. This is not a cross-client issue — exchange_refresh_token calls
validate_token_client and rejects on record.client_id != request.client_id, so
a refresh token can't be redeemed by another client. The problem is least
privilege and consistency with the registration response. It matters most for
public clients (token_endpoint_auth_method: none, so client_secret is None):
they end up holding a long-lived credential they never asked for and have no
client secret to protect.
Knock-on effect on the new registration back-pressure. issue_tokens_locked
ends with:
client.expires_at = refresh_expires_at;
client.refresh_expires_at = Some(refresh_expires_at);
and the MAX_CLIENTS reclaim path only considers a client reclaimable when
client.refresh_expires_at.is_none_or(|expires_at| expires_at <= now)
Because every client gets a refresh token, every client that completes one
code exchange becomes non-reclaimable, and its registration TTL is extended
from REGISTERED_CLIENT_IDLE_TTL_SECONDS to the refresh lifetime — including
clients that never requested refresh_token. The client-table limit added in
this PR is weakened by the same gap.
Suggested fix
1. Store the validated grant_types on RegisteredClient.
2. In issue_tokens_locked, only mint and persist a refresh token when the
client registered refresh_token; leave refresh_token out of the token response
otherwise, and don't extend expires_at / refresh_expires_at for those clients.
3. In oauth_token, return unauthorized_client for a grant_type the client
did not register, rather than dispatching on the request alone.
Minor, pre-existing (not introduced here): ClientRegistrationRequest comes
from the rmcp SDK, where grant_types is a required field with no
#[serde(default)]. RFC 7591 §2 says an omitted grant_types defaults to
["authorization_code"], so a spec-conformant minimal registration request fails
at deserialization before reaching your validation. Worth handling if you're
touching this path anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
