-
Notifications
You must be signed in to change notification settings - Fork 66
feat(auth): Add 403 Forbidden insufficient_scope support per MCP Auth Spec 2025-11-25 and RFC 6750 (Section 3.1)
#537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
97b3ada
9731df5
3188f91
085979c
57ca974
a6d1f6c
bbe2f8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| ### feat(auth): Add 403 Forbidden `insufficient_scope` support per MCP Auth Spec 2025-11-25 and RFC 6750 (Section 3.1) - @gocamille PR #537 | ||
|
|
||
| This adds HTTP 403 Forbidden responses with `error="insufficient_scope"` per [MCP Auth Spec 2025-11-25 Section 10: Error Handling](https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization#error-handling) and [RFC 6750 Section 3.1](https://www.rfc-editor.org/rfc/rfc6750.html#section-3.1). | ||
|
|
||
| **Changes:** | ||
| - `www_authenticate.rs`: Added `BearerError::InsufficientScope` enum and [error](crates/apollo-mcp-server/src/auth/www_authenticate.rs#L135-L149) field to `WWW-Authenticate` header | ||
| - `valid_token.rs`: Extract [scope](crates/apollo-mcp-server/src/auth/valid_token.rs#L27-L38)/[scp](crates/apollo-mcp-server/src/auth/valid_token.rs#L503-L509) claims from JWTs (handles both standard OAuth and Azure AD) | ||
| - `auth.rs`: Scope validation with fail-closed behaviour—valid tokens lacking required scopes get `403` | ||
| - `headers.rs`: Updated tests for new `ValidToken` struct | ||
|
|
||
| **Behavior:** | ||
| - **401 Unauthorized**: Missing or invalid token | ||
| - **403 Forbidden**: Valid token but insufficient scopes (includes `error="insufficient_scope"` in response) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,7 @@ mod www_authenticate; | |
| use protected_resource::ProtectedResource; | ||
| pub(crate) use valid_token::ValidToken; | ||
| use valid_token::ValidateToken; | ||
| use www_authenticate::WwwAuthenticate; | ||
| use www_authenticate::{BearerError, WwwAuthenticate}; | ||
|
|
||
| /// Errors that can occur when building a TLS-configured HTTP client | ||
| #[derive(Debug, thiserror::Error)] | ||
|
|
@@ -191,11 +191,15 @@ async fn oauth_validate( | |
| ) -> Result<Response, (StatusCode, TypedHeader<WwwAuthenticate>)> { | ||
| let auth_config = &auth_state.config; | ||
|
|
||
| // Consolidated unauthorized error for use with any fallible step in this process | ||
| let unauthorized_error = || { | ||
| let mut resource = auth_config.resource.clone(); | ||
| resource.set_path("/.well-known/oauth-protected-resource"); | ||
| // Helper to construct the resource metadata URL | ||
| let resource_metadata_url = || { | ||
| let mut url = auth_config.resource.clone(); | ||
| url.set_path("/.well-known/oauth-protected-resource"); | ||
| url | ||
| }; | ||
|
|
||
| // Unauthorized error for missing or invalid tokens | ||
| let unauthorized_error = || { | ||
| let scope = if auth_config.scopes.is_empty() { | ||
| None | ||
| } else { | ||
|
|
@@ -205,8 +209,21 @@ async fn oauth_validate( | |
| ( | ||
| StatusCode::UNAUTHORIZED, | ||
| TypedHeader(WwwAuthenticate::Bearer { | ||
| resource_metadata: resource, | ||
| resource_metadata: resource_metadata_url(), | ||
| scope, | ||
| error: None, | ||
| }), | ||
| ) | ||
| }; | ||
|
|
||
| // Forbidden error for valid tokens with insufficient scopes (RFC 6750 Section 3.1) | ||
| let forbidden_error = |required_scopes: &[String]| { | ||
| ( | ||
| StatusCode::FORBIDDEN, | ||
| TypedHeader(WwwAuthenticate::Bearer { | ||
| resource_metadata: resource_metadata_url(), | ||
| scope: Some(required_scopes.join(" ")), | ||
| error: Some(BearerError::InsufficientScope), | ||
| }), | ||
| ) | ||
| }; | ||
|
|
@@ -229,6 +246,27 @@ async fn oauth_validate( | |
| unauthorized_error() | ||
| })?; | ||
|
|
||
| // Check if token has required scopes (fail-closed: missing scope claim = insufficient) | ||
| if !auth_config.scopes.is_empty() { | ||
| let missing_scopes: Vec<_> = auth_config | ||
| .scopes | ||
| .iter() | ||
| .filter(|required| !valid_token.scopes.iter().any(|s| s == *required)) | ||
|
DaleSeo marked this conversation as resolved.
|
||
| .collect(); | ||
|
|
||
| if !missing_scopes.is_empty() { | ||
| tracing::warn!( | ||
| required = ?auth_config.scopes, | ||
| present = ?valid_token.scopes, | ||
| missing = ?missing_scopes, | ||
| "Token has insufficient scopes" | ||
| ); | ||
| tracing::Span::current().record("reason", "insufficient_scope"); | ||
| tracing::Span::current().record("status_code", StatusCode::FORBIDDEN.as_u16()); | ||
| return Err(forbidden_error(&auth_config.scopes)); | ||
| } | ||
| } | ||
|
Comment on lines
+249
to
+268
|
||
|
|
||
| // Insert new context to ensure that handlers only use our enforced token verification | ||
| // for propagation | ||
| request.extensions_mut().insert(valid_token); | ||
|
|
@@ -336,6 +374,78 @@ mod tests { | |
| assert!(!www_auth.contains("scope=")); | ||
| } | ||
|
|
||
| mod scope_validation { | ||
| use super::*; | ||
|
|
||
| fn scopes_are_sufficient(required: &[String], present: &[String]) -> bool { | ||
| required.iter().all(|req| present.contains(req)) | ||
| } | ||
|
|
||
| #[test] | ||
| fn insufficient_scopes_fails() { | ||
| let required = vec!["read".to_string(), "write".to_string()]; | ||
| let present = vec!["read".to_string()]; | ||
| assert!(!scopes_are_sufficient(&required, &present)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn all_required_scopes_succeeds() { | ||
| let required = vec!["read".to_string(), "write".to_string()]; | ||
| let present = vec!["read".to_string(), "write".to_string()]; | ||
| assert!(scopes_are_sufficient(&required, &present)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn no_scopes_when_required_fails() { | ||
| let required = vec!["read".to_string()]; | ||
| let present: Vec<String> = vec![]; | ||
| assert!(!scopes_are_sufficient(&required, &present)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn superset_of_scopes_succeeds() { | ||
| let required = vec!["read".to_string()]; | ||
| let present = vec!["read".to_string(), "write".to_string(), "admin".to_string()]; | ||
| assert!(scopes_are_sufficient(&required, &present)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn empty_required_scopes_always_succeeds() { | ||
| let required: Vec<String> = vec![]; | ||
| let present = vec!["read".to_string()]; | ||
| assert!(scopes_are_sufficient(&required, &present)); | ||
|
|
||
| let present_empty: Vec<String> = vec![]; | ||
| assert!(scopes_are_sufficient(&required, &present_empty)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn scope_order_does_not_matter() { | ||
| let required = vec!["write".to_string(), "read".to_string()]; | ||
| let present = vec!["read".to_string(), "write".to_string()]; | ||
| assert!(scopes_are_sufficient(&required, &present)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn forbidden_error_contains_insufficient_scope() { | ||
| let header = WwwAuthenticate::Bearer { | ||
| resource_metadata: Url::parse( | ||
| "https://test.com/.well-known/oauth-protected-resource", | ||
| ) | ||
| .unwrap(), | ||
| scope: Some("read write".to_string()), | ||
| error: Some(BearerError::InsufficientScope), | ||
| }; | ||
|
|
||
| let mut values = Vec::new(); | ||
| headers::Header::encode(&header, &mut values); | ||
| let encoded = values.first().unwrap().to_str().unwrap(); | ||
|
|
||
| assert!(encoded.contains(r#"error="insufficient_scope""#)); | ||
| assert!(encoded.contains(r#"scope="read write""#)); | ||
| } | ||
| } | ||
|
|
||
| mod tls_config { | ||
| use super::*; | ||
| use std::io::Write; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we assume that if users set the
transport.auth.scopesoption to an empty list, they want to skip scope validation completely?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (and the one below) is a really great question (and super nuanced @DaleSeo !) -- looking at the specs (both MCP and OAuth), this assumption is correct. So when
transport.auth.scopesis set to an empty list the following would happen:scopes_supportedfield in the protected resource metadata would be empty (which a client could see as "no specific scopes needed")This seems a reasonable default for those who want simple "authenticated or not" access control, but (and this is related to the comment below too) we may want to consider adding flexibility / complexity in a few ways:
scopes_supportedfromscopes_required— like you mentioned the MCP spec explicitly says scopes inWWW-Authenticate"may" matchscopes_supported, be a subset or superset of it, or an alternative collection, while right now we use one list for enforcement.What do you think of the current "all-or-nothing" approach? Should we enhance the flexibility of this to cover any of the above items?
(If we do stay with the current approach I would update the docs to document this clearly, and possibly add a warning when the scopes list is empty)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing your thoughts, @gocamille! This reminds me that in PR #535, we introduced the
allow_any_audienceflag instead of treatingaudiences: []as skipping validation. I think we should do the same for scopes, and it might be another case where a CORS-like configuration makes sense. Skipping scope validation reduces security, so I would suggest making it an explicit opt-out.