Skip to content

Conversation

@jskjw157
Copy link

@jskjw157 jskjw157 commented Jan 20, 2026

Summary

Add integration tests for Prompts, Resources, and Tools over Streamable HTTP transport as outlined in #183.

Changes

  • Infrastructure: Enable STREAMABLE_HTTP transport in test base
  • Prompts: Add functional and security integration tests
  • Resources: Add functional and security integration tests
  • Tools: Add functional and security integration tests

Related Issue

Fixes partially #183, #332

@jskjw157 jskjw157 force-pushed the test/streamable-http-integration branch from e71221c to 56cd2d7 Compare January 20, 2026 09:31
Copy link
Contributor

@kpavlov kpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @jskjw157!
Could we take a look at the test scenarios and see, if we can move any use cases that aren’t specific to the protocol to the base class?
Also, could we tidy up those long TODO comments?
Let’s give another go, this time with a focus on making the tests easier to maintain.

@kpavlov kpavlov added the tests label Jan 20, 2026
@jskjw157
Copy link
Author

Fixed in 97f1fa9

Copy link
Contributor

@kpavlov kpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase onto main and squash the commits so we maintain a clear commit history.

@kpavlov kpavlov force-pushed the test/streamable-http-integration branch from 97f1fa9 to 921bed3 Compare January 22, 2026 07:19
@jskjw157 jskjw157 force-pushed the test/streamable-http-integration branch 2 times, most recently from c2a5d9f to 8e9557c Compare January 22, 2026 13:46
@jskjw157
Copy link
Author

jskjw157 commented Jan 22, 2026

Done! Rebased onto main and squashed the commits.

Are there any other issues or features you'd recommend I work on next?

- Add StreamableHTTP transport support to KotlinTestBase
- Add Prompt/Resource/Tool integration tests for StreamableHTTP
- Add security integration tests (transport-neutral, STDIO-based)
- Add StreamableHttpTestUtils and MockAuthorizationWrapper utilities
- Unify mcp-session-id header casing across test files
@kpavlov kpavlov force-pushed the test/streamable-http-integration branch from 8e9557c to e253b80 Compare January 26, 2026 06:47
Copy link
Contributor

@kpavlov kpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @jskjw157!

While this PR does add some useful integration tests for the Streamable HTTP protocol, the security tests could use a bit more thought.

To start, I’d suggest moving the security tests to a separate PR and keeping this one focused on adding Streamable HTTP transport support to the existing tests (we can remove the ‘Security tests’ tag).

The access control system isn’t quite ready to go on the server yet, but we’re running some tests to check it out a bit early. Also, the list resource tests should make sure that resources are only shown to users who have the right to see them. Maybe we could replace MockAuthorizationWrapper with something more like a production-code solution, such as a chain of generic (resource+principal)-aware predicates. You might want to have a look at Spring Security, where you can find examples of how these ideas are put into action.

TL;DR - Let's remove security tests from this PR and keep it focused on adding Streamable HTTP protocol tests.

private var stdioClientOutput: Sink? = null

// StreamableHTTP-specific fields
private var streamableHttpServerTransport: StreamableHttpServerTransport? = null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a heads-up: this should probably be in the streamable test subclass, but since we already have transport-specific fields here, let’s tackle it separately.

Comment on lines +27 to +28
private val secretPromptName = "secret-prompt"
private val restrictedPromptName = "restricted-prompt"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any difference between "secret" and "restricted" in this scenario

Comment on lines +131 to +132
assertTrue(promptNames.contains(secretPromptName), "Secret prompt should be listed")
assertTrue(promptNames.contains(restrictedPromptName), "Restricted prompt should be listed")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not expect the server to return restricted resources in a list, unless access to the resources is granted. But the current server implementation does not support that, unfortunately. These 2 checks should be removed / commented

Comment on lines +136 to +141
fun testListPromptsDenied() {
runBlocking {
val result = client.listPrompts()
assertNotNull(result, "List prompts should succeed")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does not redundant

Comment on lines +184 to +222
@Test
fun testGetPromptPartialAccess(): Unit = runBlocking {
val publicResult = client.getPrompt(
GetPromptRequest(
GetPromptRequestParams(
name = publicPromptName,
arguments = emptyMap(),
),
),
)
assertNotNull(publicResult, "Public prompt should be accessible")

val restrictedResult = client.getPrompt(
GetPromptRequest(
GetPromptRequestParams(
name = restrictedPromptName,
arguments = emptyMap(),
),
),
)
assertNotNull(restrictedResult, "Restricted prompt should be accessible")

val exception = assertThrows<McpException> {
runBlocking {
client.getPrompt(
GetPromptRequest(
GetPromptRequestParams(
name = secretPromptName,
arguments = emptyMap(),
),
),
)
}
}

withClue("Secret prompt should be denied") {
exception.message?.lowercase()?.contains("access denied") shouldBe true
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should have been a test per use case, not a single test

* )
*
* server.addPrompt(...) { request ->
* if (!authWrapper.isAllowed("prompts", "get", mapOf("name" to request.params.name))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this API be even better?

authWrapper.isAllowed(…) // throws exception

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend having a look at Spring Security and implementing something like ResourceAuthorizer – a predicate that accepts the currently authenticated user and the resources and returns an authentication decision: ACCEPTED/DENIED/UNKNOWN. For test simplicity, it could return just a boolean.

Comment on lines +86 to +125
// TODO: Future utilities for advanced testing scenarios

// TODO: Creates a custom HTTP client with specific headers for testing.
//
// This would be useful for testing:
// - Invalid session IDs
// - Custom Host/Origin headers for DNS rebinding tests
// - Missing required headers
//
// Implementation requires creating a Ktor HttpClient with custom configuration.
// fun createCustomHttpClient(customHeaders: Map<String, String>): HttpClient

// TODO: Sends a direct HTTP request to the server for low-level testing.
//
// This would be useful for testing:
// - Malformed requests
// - Invalid JSON
// - HTTP-level errors (404, 403, 400, etc.)
//
// Implementation requires direct Ktor client usage outside of MCP client abstraction.
// suspend fun sendDirectHttpRequest(url: String, method: HttpMethod, headers: Map<String, String>, body: String?): HttpResponse

// TODO: Creates a StreamableHTTP client with a pre-set session ID.
//
// This would be useful for testing:
// - Session resumption
// - Invalid session ID handling
// - Session expiration scenarios
//
// Implementation requires ability to inject session ID into client transport.
// fun createClientWithSessionId(url: String, sessionId: String): Client

// TODO: Asserts that a request was rejected with a specific HTTP status code.
//
// This would be useful for testing:
// - Authorization failures (403)
// - Invalid requests (400)
// - Not found errors (404)
//
// Implementation requires catching and analyzing transport-level errors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s take out the comment to keep the code tidy.

import kotlin.test.assertNotNull
import kotlin.test.assertTrue

abstract class AbstractResourceSecurityIntegrationTest : KotlinTestBase() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments to AbstractPromptSecurityIntegrationTest

import kotlin.test.assertNotNull
import kotlin.test.assertTrue

abstract class AbstractToolSecurityIntegrationTest : KotlinTestBase() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments to AbstractPromptSecurityIntegrationTest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants