Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
🤖 Augment PR SummarySummary: This PR makes MCP authentication denials more actionable by advertising RFC 9728 Protected Resource Metadata (PRM) via the MCP endpoint’s OAuth challenge. Changes:
Technical Notes: Only routes that can actually publish PRM add the 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| {"requestSchema", std::string_view{mcp_request_schema}}, | ||
| {"responseSchema", std::string_view{mcp_response_schema}}}; | ||
| {"responseSchema", std::string_view{mcp_response_schema}}, | ||
| {"metadataPath", std::string_view{mcp_metadata_path}}}; |
There was a problem hiding this comment.
mcp_metadata_path is a temporary std::string but is stored into URITemplateRouter::Argument as a std::string_view; URITemplateRouter keeps argument values as string_view and requires the referenced storage to outlive the router. This can dangle and later produce an invalid metadataPath when ActionMCP_v1 reads route arguments (potentially breaking the WWW-Authenticate resource_metadata construction).
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/e2e/html/hurl/mcp-prm.community.hurl">
<violation number="1" location="test/e2e/html/hurl/mcp-prm.community.hurl:115">
P3: The leading comment describes a "denial carrying no pointer," but the very next block is a successful ping (HTTP 200, `result: {}`), and every denial in this file (including the trailing-slash GET below) carries a `Link: ...rel="describedby"` header. The comment doesn't match the code it precedes, so it reads as a stale copy from another edition and misleads future readers; consider moving/rephrasing it or dropping it.</violation>
</file>
<file name="src/index/generators.h">
<violation number="1" location="src/index/generators.h:1030">
P1: Protected MCP endpoints under a non-root base path advertise a doubled base path, so OAuth clients cannot fetch `resource_metadata` after a 401. Compose this URL from the origin and RFC 9728 well-known path, or pass an argument that is relative to `server_uri()` without re-adding `configuration.base_path`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| auth_callback_arguments); | ||
|
|
||
| const auto mcp_metadata_path{ | ||
| configuration.base_path + |
There was a problem hiding this comment.
P1: Protected MCP endpoints under a non-root base path advertise a doubled base path, so OAuth clients cannot fetch resource_metadata after a 401. Compose this URL from the origin and RFC 9728 well-known path, or pass an argument that is relative to server_uri() without re-adding configuration.base_path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/index/generators.h, line 1030:
<comment>Protected MCP endpoints under a non-root base path advertise a doubled base path, so OAuth clients cannot fetch `resource_metadata` after a 401. Compose this URL from the origin and RFC 9728 well-known path, or pass an argument that is relative to `server_uri()` without re-adding `configuration.base_path`.</comment>
<file context>
@@ -1026,9 +1026,13 @@ struct GENERATE_URITEMPLATE_ROUTES {
auth_callback_arguments);
+ const auto mcp_metadata_path{
+ configuration.base_path +
+ std::string{sourcemeta::one::ENDPOINT_MCP_PRM}};
const sourcemeta::core::URITemplateRouter::Argument mcp_arguments[] = {
</file context>
| jsonpath "$.properties.authorization_servers.items.format" == "uri" | ||
| jsonpath "$.properties.bearer_methods_supported.const" includes "header" | ||
|
|
||
| # Where no document is published the denial carries no pointer either, since |
There was a problem hiding this comment.
P3: The leading comment describes a "denial carrying no pointer," but the very next block is a successful ping (HTTP 200, result: {}), and every denial in this file (including the trailing-slash GET below) carries a Link: ...rel="describedby" header. The comment doesn't match the code it precedes, so it reads as a stale copy from another edition and misleads future readers; consider moving/rephrasing it or dropping it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/e2e/html/hurl/mcp-prm.community.hurl, line 115:
<comment>The leading comment describes a "denial carrying no pointer," but the very next block is a successful ping (HTTP 200, `result: {}`), and every denial in this file (including the trailing-slash GET below) carries a `Link: ...rel="describedby"` header. The comment doesn't match the code it precedes, so it reads as a stale copy from another edition and misleads future readers; consider moving/rephrasing it or dropping it.</comment>
<file context>
@@ -87,10 +98,67 @@ jsonpath "$.valid" == true
+jsonpath "$.properties.authorization_servers.items.format" == "uri"
+jsonpath "$.properties.bearer_methods_supported.const" includes "header"
+
+# Where no document is published the denial carries no pointer either, since
+# there would be nothing at the other end of it
+POST {{base}}/self/v1/mcp
</file context>
There was a problem hiding this comment.
2 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/index/generators.h">
<violation number="1" location="src/index/generators.h:847">
P1: Protected MCP challenges advertise a doubled base path for deployments mounted below `/`. For `https://host/v1/catalog`, this produces `https://host/v1/catalog/v1/catalog/.well-known/...`, so clients cannot fetch protected-resource metadata; construct the challenge URL from the origin plus this path, or pass a path not already prefixed.</violation>
</file>
<file name="src/http/include/sourcemeta/one/http_helpers.h">
<violation number="1" location="src/http/include/sourcemeta/one/http_helpers.h:194">
P1: Production builds can emit injected response headers when a future/configuration-derived challenge contains CR/LF, because the only validation is debug-only `assert`. Retain the default Bearer challenge when the added value is invalid rather than appending it verbatim.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const auto mcp_response_schema{configuration.base_path + | ||
| "/self/v1/schemas/mcp/response"}; | ||
| const auto mcp_metadata_path{ | ||
| configuration.base_path + |
There was a problem hiding this comment.
P1: Protected MCP challenges advertise a doubled base path for deployments mounted below /. For https://host/v1/catalog, this produces https://host/v1/catalog/v1/catalog/.well-known/..., so clients cannot fetch protected-resource metadata; construct the challenge URL from the origin plus this path, or pass a path not already prefixed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/index/generators.h, line 847:
<comment>Protected MCP challenges advertise a doubled base path for deployments mounted below `/`. For `https://host/v1/catalog`, this produces `https://host/v1/catalog/v1/catalog/.well-known/...`, so clients cannot fetch protected-resource metadata; construct the challenge URL from the origin plus this path, or pass a path not already prefixed.</comment>
<file context>
@@ -843,6 +843,9 @@ struct GENERATE_URITEMPLATE_ROUTES {
const auto mcp_response_schema{configuration.base_path +
"/self/v1/schemas/mcp/response"};
+ const auto mcp_metadata_path{
+ configuration.base_path +
+ std::string{sourcemeta::one::ENDPOINT_MCP_PRM}};
const auto mcp_protected_resource_metadata_response_schema{
</file context>
There was a problem hiding this comment.
This is a known limitation, though worth explaining in the docs maybe
| if (challenge.empty()) { | ||
| response.write_header("WWW-Authenticate", "Bearer realm=\"registry\""); | ||
| } else { | ||
| std::string value{"Bearer realm=\"registry\", "}; | ||
| value.append(challenge); | ||
| response.write_header("WWW-Authenticate", value); | ||
| } |
There was a problem hiding this comment.
P1: Production builds can emit injected response headers when a future/configuration-derived challenge contains CR/LF, because the only validation is debug-only assert. Retain the default Bearer challenge when the added value is invalid rather than appending it verbatim.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/http/include/sourcemeta/one/http_helpers.h, line 194:
<comment>Production builds can emit injected response headers when a future/configuration-derived challenge contains CR/LF, because the only validation is debug-only `assert`. Retain the default Bearer challenge when the added value is invalid rather than appending it verbatim.</comment>
<file context>
@@ -189,7 +191,13 @@ inline auto json_error(const HTTPRequest &request, HTTPResponse &response,
// https://datatracker.ietf.org/doc/html/rfc6750#section-3
if (status == sourcemeta::core::HTTP_STATUS_UNAUTHORIZED) {
- response.write_header("WWW-Authenticate", "Bearer realm=\"registry\"");
+ if (challenge.empty()) {
+ response.write_header("WWW-Authenticate", "Bearer realm=\"registry\"");
+ } else {
</file context>
| if (challenge.empty()) { | |
| response.write_header("WWW-Authenticate", "Bearer realm=\"registry\""); | |
| } else { | |
| std::string value{"Bearer realm=\"registry\", "}; | |
| value.append(challenge); | |
| response.write_header("WWW-Authenticate", value); | |
| } | |
| if (challenge.empty() || | |
| challenge.find_first_of("\r\n") != std::string_view::npos) { | |
| response.write_header("WWW-Authenticate", "Bearer realm=\"registry\""); | |
| } else { | |
| std::string value{"Bearer realm=\"registry\", "}; | |
| value.append(challenge); | |
| response.write_header("WWW-Authenticate", value); | |
| } |
There was a problem hiding this comment.
1 issue found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="enterprise/e2e/auth-closed/hurl/mcp-prm.all.hurl">
<violation number="1" location="enterprise/e2e/auth-closed/hurl/mcp-prm.all.hurl:84">
P2: This 401 response is produced with no Origin header on the request, but the server only emits `Access-Control-Expose-Headers` when an `Origin` is present (see json_error in src/http/include/sourcemeta/one/http_helpers.h). The new assertion `header "Access-Control-Expose-Headers" contains "WWW-Authenticate"` therefore cannot match and the test will fail. Send an `Origin` (matching the instance's allowed origin) on this request, as the OPTIONS case does, so the CORS header the assertion depends on is actually emitted.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| {{credentialed_body}} | ||
| ``` | ||
| HTTP 200 | ||
| [Asserts] |
There was a problem hiding this comment.
P2: This 401 response is produced with no Origin header on the request, but the server only emits Access-Control-Expose-Headers when an Origin is present (see json_error in src/http/include/sourcemeta/one/http_helpers.h). The new assertion header "Access-Control-Expose-Headers" contains "WWW-Authenticate" therefore cannot match and the test will fail. Send an Origin (matching the instance's allowed origin) on this request, as the OPTIONS case does, so the CORS header the assertion depends on is actually emitted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/e2e/auth-closed/hurl/mcp-prm.all.hurl, line 84:
<comment>This 401 response is produced with no Origin header on the request, but the server only emits `Access-Control-Expose-Headers` when an `Origin` is present (see json_error in src/http/include/sourcemeta/one/http_helpers.h). The new assertion `header "Access-Control-Expose-Headers" contains "WWW-Authenticate"` therefore cannot match and the test will fail. Send an `Origin` (matching the instance's allowed origin) on this request, as the OPTIONS case does, so the CORS header the assertion depends on is actually emitted.</comment>
<file context>
@@ -65,23 +66,47 @@ GET {{base}}/.well-known/oauth-protected-resource/self/v1/mcp
+{{credentialed_body}}
+```
+HTTP 200
+[Asserts]
+jsonpath "$.valid" == true
+
</file context>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark Index (enterprise)
Details
| Benchmark suite | Current: 349516f | Previous: 4677568 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
334 ms |
398 ms |
0.84 |
Add one schema (100 existing) |
94 ms |
108 ms |
0.87 |
Add one schema (1000 existing) |
159 ms |
152 ms |
1.05 |
Add one schema (10000 existing) |
1057 ms |
706 ms |
1.50 |
Update one schema (1 existing) |
82 ms |
99 ms |
0.83 |
Update one schema (101 existing) |
94 ms |
105 ms |
0.90 |
Update one schema (1001 existing) |
156 ms |
156 ms |
1 |
Update one schema (10001 existing) |
1068 ms |
719 ms |
1.49 |
Cached rebuild (1 existing) |
8 ms |
6 ms |
1.33 |
Cached rebuild (101 existing) |
10 ms |
8 ms |
1.25 |
Cached rebuild (1001 existing) |
35 ms |
24 ms |
1.46 |
Cached rebuild (10001 existing) |
297 ms |
208 ms |
1.43 |
Index 100 schemas |
735 ms |
643 ms |
1.14 |
Index 1000 schemas |
1600 ms |
1366 ms |
1.17 |
Index 10000 schemas |
12783 ms |
11661 ms |
1.10 |
Index 10000 schemas (custom meta-schema) |
15914 ms |
14517 ms |
1.10 |
Index 10000 schemas ($ref fan-out) |
14906 ms |
14079 ms |
1.06 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Benchmark Index (community)
Details
| Benchmark suite | Current: 349516f | Previous: 4677568 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
327 ms |
314 ms |
1.04 |
Add one schema (100 existing) |
30 ms |
33 ms |
0.91 |
Add one schema (1000 existing) |
94 ms |
104 ms |
0.90 |
Add one schema (10000 existing) |
796 ms |
874 ms |
0.91 |
Update one schema (1 existing) |
21 ms |
22 ms |
0.95 |
Update one schema (101 existing) |
29 ms |
34 ms |
0.85 |
Update one schema (1001 existing) |
95 ms |
104 ms |
0.91 |
Update one schema (10001 existing) |
791 ms |
878 ms |
0.90 |
Cached rebuild (1 existing) |
8 ms |
8 ms |
1 |
Cached rebuild (101 existing) |
10 ms |
12 ms |
0.83 |
Cached rebuild (1001 existing) |
37 ms |
42 ms |
0.88 |
Cached rebuild (10001 existing) |
320 ms |
361 ms |
0.89 |
Index 100 schemas |
595 ms |
590 ms |
1.01 |
Index 1000 schemas |
1346 ms |
1477 ms |
0.91 |
Index 10000 schemas |
13699 ms |
13733 ms |
1.00 |
Index 10000 schemas (custom meta-schema) |
16664 ms |
16433 ms |
1.01 |
Index 10000 schemas ($ref fan-out) |
16608 ms |
16445 ms |
1.01 |
This comment was automatically generated by workflow using github-action-benchmark.
1 participant
Signed-off-by: Juan Cruz Viotti jv@jviotti.com