Conversation
Contributor
WalkthroughComputes operation SHA‑256 pre-parse when configured, enforces persistedQuery Sha256Hash matches query body, conditionally populates persisted-query extensions, expands persisted-operation fetch conditions, logs or rejects unknown persisted operations per config, and adds tests plus persisted-operation test data. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
dkorittki
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
router-tests/websocket_test.go (2)
2073-2236: Great coverage of persisted-operation paths over WebSockets. Add two cases to harden hash+query behavior when safelist/log_unknown are enabled.
- Add tests for:
- Provided hash + query + safelist enabled + mismatch → error.
- Provided hash + query + safelist enabled + match → success.
This guards against regressions when both fields are present under enforcement modes.I can draft these two subtests mirroring the existing patterns—want me to push patches?
2136-2169: Avoid magic SHA-256 constants in assertions; compute at runtime for stability.Precompute the expected hash from the query to reduce brittleness if whitespace or formatting changes.
Apply this diff within the test (add hex import at top if needed):
@@ - require.Len(t, logEntries, 1) - requestContext := logEntries[0].ContextMap() - require.Contains(t, requestContext["query"], "subscription { currentTime { unixTime } }") - require.Equal(t, "6e94d99132b544a0d7522696a7d35643d56a26c7b8c2e0df29e2b9935636628c", requestContext["sha256Hash"]) + require.Len(t, logEntries, 1) + requestContext := logEntries[0].ContextMap() + query := "subscription { currentTime { unixTime } }" + require.Contains(t, requestContext["query"], query) + sum := sha256.Sum256([]byte(query)) + expected := fmt.Sprintf("%x", sum[:]) + require.Equal(t, expected, requestContext["sha256Hash"])router/core/websocket.go (2)
864-873: Log more context on unknown persisted operations (client name) and avoid nil/empty query logs.Add client name to the warning and guard empty queries to reduce noisy entries.
Apply this diff:
- h.logger.Warn("Unknown persisted operation found", zap.String("query", operationKit.parsedOperation.Request.Query), zap.String("sha256Hash", poNotFoundErr.Sha256Hash)) + fields := []zap.Field{zap.String("sha256Hash", poNotFoundErr.Sha256Hash), zap.String("client_name", h.clientInfo.Name)} + if q := operationKit.parsedOperation.Request.Query; q != "" { + fields = append(fields, zap.String("query", q)) + } + h.logger.Warn("Unknown persisted operation found", fields...)
1245-1257: Nil-safety in shouldComputeOperationSha256.Accessing PersistedQuery.HasHash() assumes a non-nil receiver. Prefer explicit nil check.
Apply this diff:
-func (h *WebSocketConnectionHandler) shouldComputeOperationSha256(operationKit *OperationKit) bool { - hasPersistedHash := operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.HasHash() +func (h *WebSocketConnectionHandler) shouldComputeOperationSha256(operationKit *OperationKit) bool { + pq := operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery + hasPersistedHash := pq != nil && pq.Sha256Hash != ""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
router-tests/testenv/testdata/cdn/organization/graph/operations/my-client/8ad544bda5b2ad7a59481e31fb6fa62705fd072b20fdaadba4f3908d01f2c132.json(1 hunks)router-tests/websocket_test.go(2 hunks)router/core/websocket.go(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: endigma
PR: wundergraph/cosmo#2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
PR: wundergraph/cosmo#2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
router-tests/testenv/testdata/cdn/organization/graph/operations/my-client/8ad544bda5b2ad7a59481e31fb6fa62705fd072b20fdaadba4f3908d01f2c132.json
📚 Learning: 2025-08-28T09:18:10.121Z
Learnt from: endigma
PR: wundergraph/cosmo#2141
File: router-tests/http_subscriptions_test.go:100-108
Timestamp: 2025-08-28T09:18:10.121Z
Learning: In router-tests/http_subscriptions_test.go heartbeat tests, the message ordering should remain strict with data messages followed by heartbeat messages, as the timing is deterministic and known by design in the Cosmo router implementation.
Applied to files:
router-tests/websocket_test.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
PR: wundergraph/cosmo#2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
Applied to files:
router/core/websocket.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
PR: wundergraph/cosmo#2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
router/core/websocket.go
🧬 Code graph analysis (2)
router-tests/websocket_test.go (3)
router-tests/testenv/testenv.go (7)
Run(107-124)Config(286-342)Environment(1729-1765)WSWriteJSON(2785-2826)WebSocketMessage(2281-2285)WSReadJSON(2699-2740)LogObservationConfig(388-391)router/pkg/config/config.go (3)
Config(987-1061)PersistedOperationsConfig(842-848)SafelistConfiguration(850-852)router/core/router.go (2)
Option(172-172)WithPersistedOperationsConfig(2044-2048)
router/core/websocket.go (2)
router/core/operation_processor.go (3)
GraphQLRequestExtensions(188-190)GraphQLRequestExtensionsPersistedQuery(192-195)OperationKit(171-179)router/internal/persistedoperation/client.go (1)
PersistentOperationNotFoundError(18-21)
🔇 Additional comments (3)
router-tests/testenv/testdata/cdn/organization/graph/operations/my-client/8ad544bda5b2ad7a59481e31fb6fa62705fd072b20fdaadba4f3908d01f2c132.json (1)
1-4: Fixture LGTM.Hash and body align with subscription used in tests.
router-tests/websocket_test.go (1)
20-20: Import add is appropriate.zapcore is used for log observation level; no issues.
router/core/websocket.go (1)
32-33: New import is correct.Used for typed error matching of unknown persisted operation.
endigma
requested changes
Sep 19, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
router-tests/websocket_test.go (2)
2103-2130: Tidy up the subscription lifecycle in the testAfter asserting the first "next", send a "complete" and assert the "complete" frame (consistent with other tests) instead of closing the socket immediately.
Apply:
- require.NoError(t, conn.Close()) + require.NoError(t, testenv.WSWriteJSON(t, conn, &testenv.WebSocketMessage{ID: "1", Type: "complete"})) + var complete testenv.WebSocketMessage + require.NoError(t, testenv.WSReadJSON(t, conn, &complete)) + require.Equal(t, "complete", complete.Type) + require.NoError(t, conn.Close())
2132-2171: Strengthen the log assertion with client nameSince the server includes client scoping in persisted-op lookups, assert the client as well once logged. Recommend logging clientName from the error and asserting it here.
Test change (after server change below):
- require.Len(t, logEntries, 1) + require.Len(t, logEntries, 1) requestContext := logEntries[0].ContextMap() require.Contains(t, requestContext["query"], "subscription { currentTime { unixTime } }") require.Equal(t, "6e94d99132b544a0d7522696a7d35643d56a26c7b8c2e0df29e2b9935636628c", requestContext["sha256Hash"]) + require.Equal(t, "my-client", requestContext["clientName"])router/core/websocket.go (2)
840-860: Don’t overwrite client-provided persistedQuery after validationYou correctly validate before mutation—nice. However, you still overwrite PersistedQuery even when the client provided one. Preserve client-provided fields (e.g., version) and only synthesize when missing.
Apply:
- if h.operationBlocker.safelistEnabled || h.operationBlocker.logUnknownOperationsEnabled { - // Set the request hash to the parsed hash, to see if it matches a persisted operation - operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery = &GraphQLRequestExtensionsPersistedQuery{ - Sha256Hash: operationKit.parsedOperation.Sha256Hash, - } - } + if (h.operationBlocker.safelistEnabled || h.operationBlocker.logUnknownOperationsEnabled) && + !operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.HasHash() { + // Synthesize only when missing + operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery = &GraphQLRequestExtensionsPersistedQuery{ + Sha256Hash: operationKit.parsedOperation.Sha256Hash, + } + }
861-874: Include clientName in “unknown persisted operation” logThis aids triage across multi-tenant clients and matches the persisted-op error context.
Apply:
- h.logger.Warn("Unknown persisted operation found", zap.String("query", operationKit.parsedOperation.Request.Query), zap.String("sha256Hash", poNotFoundErr.Sha256Hash)) + h.logger.Warn("Unknown persisted operation found", + zap.String("query", operationKit.parsedOperation.Request.Query), + zap.String("sha256Hash", poNotFoundErr.Sha256Hash), + zap.String("clientName", poNotFoundErr.ClientName))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router-tests/websocket_test.go(2 hunks)router/core/websocket.go(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: endigma
PR: wundergraph/cosmo#2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Learnt from: endigma
PR: wundergraph/cosmo#2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
📚 Learning: 2025-08-28T09:18:10.121Z
Learnt from: endigma
PR: wundergraph/cosmo#2141
File: router-tests/http_subscriptions_test.go:100-108
Timestamp: 2025-08-28T09:18:10.121Z
Learning: In router-tests/http_subscriptions_test.go heartbeat tests, the message ordering should remain strict with data messages followed by heartbeat messages, as the timing is deterministic and known by design in the Cosmo router implementation.
Applied to files:
router-tests/websocket_test.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
PR: wundergraph/cosmo#2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
Applied to files:
router/core/websocket.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
PR: wundergraph/cosmo#2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
router/core/websocket.go
🧬 Code graph analysis (2)
router-tests/websocket_test.go (3)
router-tests/testenv/testenv.go (7)
Run(107-124)Config(286-342)Environment(1729-1765)WSWriteJSON(2785-2826)WebSocketMessage(2281-2285)WSReadJSON(2699-2740)LogObservationConfig(388-391)router/pkg/config/config.go (3)
Config(987-1061)PersistedOperationsConfig(842-848)SafelistConfiguration(850-852)router/core/router.go (2)
Option(172-172)WithPersistedOperationsConfig(2044-2048)
router/core/websocket.go (2)
router/core/operation_processor.go (3)
GraphQLRequestExtensions(188-190)GraphQLRequestExtensionsPersistedQuery(192-195)OperationKit(171-179)router/internal/persistedoperation/client.go (1)
PersistentOperationNotFoundError(18-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-router
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
🔇 Additional comments (6)
router-tests/websocket_test.go (4)
20-21: LGTM: import zapcore for log observationImport is correct and used for LogObservation level in new tests.
2074-2101: Good coverage: rejection when safelist is enabledTest asserts the precise hash and client scoping in the error payload; mirrors router behavior.
2173-2204: LGTM: matching hash with bodyThis validates the pre-parse hash check for WebSockets; aligns with prehandler semantics.
2206-2236: LGTM: mismatched hash is correctly blockedError message assertion via Contains makes the test resilient.
router/core/websocket.go (2)
32-33: LGTM: import persistedoperation error typeNeeded for typed handling of not-found; correct package path.
1245-1257: LGTM: hash-computation gate mirrors HTTP prehandler intentConditions cover: (1) body+hash for validation and (2) no hash but safelist/logUnknown enabled for lookup. Good.
Contributor
Author
|
@endigma can have a look again? implemented your suggestions |
endigma
requested changes
Sep 24, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
router-tests/websocket_test.go (2)
2074-2101: Avoid hardcoding the expected SHA-256; compute it to prevent driftHardcoding the hash couples the test to query whitespace/formatting. Compute it from the query to keep the test resilient.
Apply this diff:
- require.JSONEq(t, `[{"message":"operation '9a41d21da2823195ad42c11d51e9ad3345824abdabf567b3615a235843a1fcc7' for client 'my-client' not found"}]`, - string(res.Payload)) + expectedHash := fmt.Sprintf("%x", sha256.Sum256([]byte(`subscription { employeeUpdated(employeeID: 1) { id } }`))) + require.JSONEq(t, + fmt.Sprintf(`[{"message":"operation '%s' for client 'my-client' not found"}]`, expectedHash), + string(res.Payload), + )
2132-2171: Make log assertions type-safe to avoid brittle interface comparisonsContextMap values are interface-typed. Assert types before Contains/Equal to avoid surprises if encoders change.
Apply this diff:
- requestContext := logEntries[0].ContextMap() - require.Contains(t, requestContext["query"], "subscription { currentTime { unixTime timeStamp } }") - require.Equal(t, "8ad544bda5b2ad7a59481e31fb6fa62705fd072b20fdaadba4f3908d01f2c132", requestContext["sha256Hash"]) + requestContext := logEntries[0].ContextMap() + q, okQ := requestContext["query"].(string) + h, okH := requestContext["sha256Hash"].(string) + require.True(t, okQ && okH, "unexpected log field types for query/sha256Hash") + require.Contains(t, q, "subscription { currentTime { unixTime timeStamp } }") + require.Equal(t, "8ad544bda5b2ad7a59481e31fb6fa62705fd072b20fdaadba4f3908d01f2c132", h)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router-tests/websocket_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: endigma
PR: wundergraph/cosmo#2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Learnt from: endigma
PR: wundergraph/cosmo#2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
📚 Learning: 2025-08-28T09:18:10.121Z
Learnt from: endigma
PR: wundergraph/cosmo#2141
File: router-tests/http_subscriptions_test.go:100-108
Timestamp: 2025-08-28T09:18:10.121Z
Learning: In router-tests/http_subscriptions_test.go heartbeat tests, the message ordering should remain strict with data messages followed by heartbeat messages, as the timing is deterministic and known by design in the Cosmo router implementation.
Applied to files:
router-tests/websocket_test.go
🧬 Code graph analysis (1)
router-tests/websocket_test.go (2)
router-tests/testenv/testenv.go (7)
Run(107-124)Config(286-342)Environment(1729-1765)WSWriteJSON(2785-2826)WebSocketMessage(2281-2285)WSReadJSON(2699-2740)LogObservationConfig(388-391)router/pkg/config/config.go (4)
Config(995-1069)PersistedOperationsConfig(850-856)SafelistConfiguration(858-860)AutomaticPersistedQueriesConfig(862-866)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
router-tests/websocket_test.go (4)
20-21: Import looks correctzapcore is used for LogObservation level; no issues.
2103-2130: Known-hash safelist pass case: LGTMCovers the hash-only path with safelist enabled and uses the versioned persistedQuery extension as intended.
2173-2204: Matching hash enforcement: LGTMValidates query+hash acceptance; aligns with the stricter pre-parse hash validation.
2206-2236: Mismatched hash rejection: LGTMGood coverage for the breaking behavior: incorrect sha256Hash blocks the request even without safelisting.
Consider adding a companion test where query + hash match but the hash is not in the safelist (safelist disabled, log_unknown true) to assert it passes and logs. Want a patch for that?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
router-tests/websocket_test.go (3)
2074-2101: Compute expected sha256 at test-time (avoid brittle hard-coding)Derive the hash from the exact query string to reduce fragility if spacing changes.
Apply this diff:
- conn := xEnv.InitGraphQLWebSocketConnection(nil, nil, []byte(`{"graphql-client-name": "my-client"}`)) - err := testenv.WSWriteJSON(t, conn, testenv.WebSocketMessage{ + conn := xEnv.InitGraphQLWebSocketConnection(nil, nil, []byte(`{"graphql-client-name": "my-client"}`)) + query := "subscription { employeeUpdated(employeeID: 1) { id } }" + sum := sha256.Sum256([]byte(query)) + expectedHash := fmt.Sprintf("%x", sum) + err := testenv.WSWriteJSON(t, conn, testenv.WebSocketMessage{ ID: "1", Type: "subscribe", - Payload: []byte(`{"query":"subscription { employeeUpdated(employeeID: 1) { id } }"}`), + Payload: fmt.Appendf(nil, `{"query":"%s"}`, query), }) require.NoError(t, err) var res testenv.WebSocketMessage err = testenv.WSReadJSON(t, conn, &res) require.NoError(t, err) require.Equal(t, "error", res.Type) require.Equal(t, "1", res.ID) - require.JSONEq(t, `[{"message":"operation '9a41d21da2823195ad42c11d51e9ad3345824abdabf567b3615a235843a1fcc7' for client 'my-client' not found"}]`, - string(res.Payload)) + require.JSONEq(t, fmt.Sprintf(`[{"message":"operation '%s' for client 'my-client' not found"}]`, expectedHash), string(res.Payload))
2132-2171: Derive logged sha256 dynamically instead of hard-codingThis avoids coupling to an exact whitespace format of the query.
Apply this diff:
- conn := xEnv.InitGraphQLWebSocketConnection(nil, nil, []byte(`{"graphql-client-name": "my-client"}`)) - err := testenv.WSWriteJSON(t, conn, testenv.WebSocketMessage{ + conn := xEnv.InitGraphQLWebSocketConnection(nil, nil, []byte(`{"graphql-client-name": "my-client"}`)) + query := "subscription { currentTime { unixTime timeStamp } }" + sum := sha256.Sum256([]byte(query)) + expectedHash := fmt.Sprintf("%x", sum) + err := testenv.WSWriteJSON(t, conn, testenv.WebSocketMessage{ ID: "1", Type: "subscribe", - Payload: []byte(`{"query":"subscription { currentTime { unixTime timeStamp } }"}`), + Payload: fmt.Appendf(nil, `{"query":"%s"}`, query), }) require.NoError(t, err) @@ - require.Contains(t, string(res.Payload), `"data"`) + require.Contains(t, string(res.Payload), `"data"`) @@ - requestContext := logEntries[0].ContextMap() - require.Contains(t, requestContext["query"], "subscription { currentTime { unixTime timeStamp } }") - require.Equal(t, "8ad544bda5b2ad7a59481e31fb6fa62705fd072b20fdaadba4f3908d01f2c132", requestContext["sha256Hash"]) + requestContext := logEntries[0].ContextMap() + require.Contains(t, requestContext["query"], query) + require.Equal(t, expectedHash, requestContext["sha256Hash"])
2173-2204: Compute matching hash from the query instead of hard-codingKeeps the test resilient if the query body changes slightly.
Apply this diff:
- err := testenv.WSWriteJSON(t, conn, testenv.WebSocketMessage{ + query := "subscription { currentTime { unixTime } }" + sum := sha256.Sum256([]byte(query)) + hash := fmt.Sprintf("%x", sum) + err := testenv.WSWriteJSON(t, conn, testenv.WebSocketMessage{ ID: "1", Type: "subscribe", Payload: []byte(`{ - "query": "subscription { currentTime { unixTime } }", + "query": "` + query + `", "extensions": { "persistedQuery": { "version": 1, - "sha256Hash": "6e94d99132b544a0d7522696a7d35643d56a26c7b8c2e0df29e2b9935636628c" + "sha256Hash": "` + hash + `" } } }`), })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router-tests/websocket_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: endigma
PR: wundergraph/cosmo#2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Learnt from: endigma
PR: wundergraph/cosmo#2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
PR: wundergraph/cosmo#2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
router-tests/websocket_test.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
PR: wundergraph/cosmo#2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
Applied to files:
router-tests/websocket_test.go
📚 Learning: 2025-09-24T12:54:00.740Z
Learnt from: endigma
PR: wundergraph/cosmo#2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.740Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router-tests/websocket_test.go
📚 Learning: 2025-09-24T12:54:00.740Z
Learnt from: endigma
PR: wundergraph/cosmo#2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.740Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.25 minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router-tests/websocket_test.go
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
PR: wundergraph/cosmo#2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router-tests/websocket_test.go
📚 Learning: 2025-08-28T09:18:10.121Z
Learnt from: endigma
PR: wundergraph/cosmo#2141
File: router-tests/http_subscriptions_test.go:100-108
Timestamp: 2025-08-28T09:18:10.121Z
Learning: In router-tests/http_subscriptions_test.go heartbeat tests, the message ordering should remain strict with data messages followed by heartbeat messages, as the timing is deterministic and known by design in the Cosmo router implementation.
Applied to files:
router-tests/websocket_test.go
🧬 Code graph analysis (1)
router-tests/websocket_test.go (3)
router-tests/testenv/testenv.go (7)
Run(107-124)Config(286-342)Environment(1729-1765)WSWriteJSON(2785-2826)WebSocketMessage(2281-2285)WSReadJSON(2699-2740)LogObservationConfig(388-391)router/pkg/config/config.go (4)
Config(995-1069)PersistedOperationsConfig(850-856)SafelistConfiguration(858-860)AutomaticPersistedQueriesConfig(862-866)router/core/router.go (2)
Option(172-172)WithPersistedOperationsConfig(2044-2048)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
router-tests/websocket_test.go (4)
20-21: Import zapcore is appropriateNeeded for LogObservation LogLevel in new tests.
2103-2130: Good: sending only the hash (no query) with safelist enabledPrevents cache-poisoning vectors and aligns with persisted-ops expectations.
2206-2236: LGTM: mismatched hash is correctly rejectedValidates the breaking-change behavior: mismatch blocks regardless of safelist.
2238-2302: Excellent cache‑poisoning prevention test
- Sends mismatched query+hash, expects rejection.
- Then sends only the hash and verifies the correct persisted op is served.
- fmt.Appendf usage is fine given the project’s Go 1.25 toolchain.
…ith-persisted-operation
…ith-persisted-operation
devsergiy
approved these changes
Sep 26, 2025
…ith-persisted-operation
…ith-persisted-operation
dkorittki
deleted the
dominik/eng-7506-subscription-registration-with-persisted-operation
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Checklist
Motivation and Context
Users are confused that safelisting does not work for subscriptions. After some digging I found that it does work but only on HTTP2 streams, not websocket connections. This is because for websockets, we haven't implemented safelisting, although persisted operations are in fact supported. This PR adds safelisting in a similar fashion to how its done for HTTP connections in prehandler, including hash calculation.
Now, when safelisting is enabled and you provide a query whoms hash is unknown, you get this error:
Query with payload whoms hash is unknown
{ "id": "sub-1", "type": "subscribe", "payload": { "query": "subscription { currentTime { unixTime timeStamp } }" } }Query which references the unknown hash directly
{ "id": "sub-1", "type": "subscribe", "payload": { "extensions": { "persistedQuery": { "version": 1, "sha256Hash": "9a41d21da2823195ad42c11d51e9ad3345824abdabf567b3615a235843a1fcc7" } } } }It would also be the same for an operation containing both the query body and hash extension.
Feedback on websocket:
[{"message":"operation '9a41d21da2823195ad42c11d51e9ad3345824abdabf567b3615a235843a1fcc7' for client 'my-client' not found"}]Note: The subscription is still registered, even if we faced this error. That's due to the error handling of websocket parsing. Not sure if thats a good or bad thing tbh.
I also added support for
persisted_operations.log_unknownin a similar fashion to the HTTP prehandler: log it bu if safelisting is disabled let it pass.For safelisting to work I had to add hash calculation in front of it, similar to how its done in the HTTP prehandler.
Alongside that I added a verification of provided hash vs calculated hash. Thats actually kind of a breaking change.
No matter the safelist settings, these queries will now get blocked (look at
sha256Hash). Before this PR it would have passed.