Skip to content

feat: add getter for sha256Hash for modules#2300

Merged
SkArchon merged 4 commits intomainfrom
milinda/eng-8395-accessing-sha256hash-custom-modules
Oct 29, 2025
Merged

feat: add getter for sha256Hash for modules#2300
SkArchon merged 4 commits intomainfrom
milinda/eng-8395-accessing-sha256hash-custom-modules

Conversation

@SkArchon
Copy link
Copy Markdown
Contributor

@SkArchon SkArchon commented Oct 24, 2025

This PR adds the following

  • A way for modules (which are outside of the core package) to access the computed sha256Hash
  • The Sha256Hash is computed only when it is required, for example when the access log has added the "operation_sha256" context field or an expression uses the sha256Hash, if none of these are true the getter alone would not be useful, thus we add a way for the user to force the computation of the hash from modules. This has to be called from the "RouterOnRequest" hook.

Summary by CodeRabbit

  • New Features

    • Added a SHA-256 verification module integrated into request handling
    • Added ability to force SHA-256 computation for operations
    • Exposed operation SHA-256 hashes for downstream use
  • Tests

    • Added tests validating hash computation, consistency across runs, and force-on behavior

Checklist

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 24, 2025

Walkthrough

Adds a new SHA-256 verifier router module and tests, extends RequestContext and OperationContext with force-hash and hash-access APIs, and updates PreHandler to consider the new force-SHA256 flag when deciding to compute operation SHA-256.

Changes

Cohort / File(s) Summary
SHA-256 Verifier Module
router-tests/modules/sha256-verifier/module.go
New module implementing SHA-256 verification with middleware and router hooks; introduces ResultContainer and Sha256VerifierModule plus Middleware, RouterOnRequest, and Module methods and interface guards.
Module Tests
router-tests/modules/sha256_verifier_test.go
New tests validating Sha256VerifierModule behavior: force-disabled vs force-enabled hashing, hash consistency across requests, and deterministic hashes for identical queries.
Context Core changes
router/core/context.go
Added RequestContext methods SetCustomFieldValueRenderer and SetForceSha256Compute; added internal forceSha256Compute bool to requestContext with SetForceSha256Compute implementation; added OperationContext.Sha256Hash() string and corresponding operationContext accessor.
PreHandler update
router/core/graphql_prehandler.go
Updated PreHandler.shouldComputeOperationSha256 signature to accept (*OperationKit, *requestContext) and use reqCtx.forceSha256Compute alongside existing flags; updated call sites to pass the request context.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Heterogeneous changes: new module + tests, public API additions, and core request handling logic.
  • Areas needing extra attention:
    • Correctness and concurrency-safety of the forceSha256Compute flag usage.
    • Interface/backwards-compatibility implications of added RequestContext and OperationContext methods.
    • Call-site updates to shouldComputeOperationSha256 across the prehandler path and tests ensuring expected behavior.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "feat: add getter for sha256Hash for modules" directly aligns with the primary objective and main changes. The PR introduces a Sha256Hash() getter method on the OperationContext interface, enables modules to access the computed SHA-256 hash, and provides infrastructure (SetForceSha256Compute) and an example implementation (Sha256VerifierModule) to support this access. The title accurately captures the essential change—exposing the sha256Hash for external module consumption—while remaining concise and avoiding vague terminology.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3c32bab and ddd497e.

📒 Files selected for processing (1)
  • router/core/graphql_prehandler.go (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/graphql_prehandler.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/graphql_prehandler.go
🧬 Code graph analysis (1)
router/core/graphql_prehandler.go (1)
router/core/operation_processor.go (1)
  • OperationKit (171-179)
⏰ 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: integration_test (./events)
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: image_scan (nonroot)
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: image_scan
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
🔇 Additional comments (3)
router/core/graphql_prehandler.go (3)

526-536: Good: early, conditional SHA‑256 compute with request‑level override

The compute timing and context propagation into telemetry and expression context look correct. No changes requested.


539-555: <!-- [scratchpad_start]
[task review_file_1/1 verification_3/3 test_context]
Determine if websocket hash mismatch tests run with safelist enabled.

[observations]

  • websocket_test.go has tests with Safelist enabled at lines 2166-2194 and 2195+
  • websocket_test.go tests at lines 2321-2327 and 2360-2366 check hash mismatch errors
  • Question: Are the hash mismatch tests (2321-2327) within a safelist-enabled test context?

[pending]

  • Extract full context of websocket test around line 2321 to see parent test setup
  • Verify if hash mismatch test runs in safelist context or plain APQ context

[actions]

  • Examine broader context of websocket_test.go around line 2321
  • Check if that test function has safelist/logUnknown enabled in its setup

442-446: Force flag wiring looks good; consider nil‑guard or passing a plain bool

Current usage dereferences reqCtx without a nil‑guard. Either add a defensive check or narrow the method API to accept a bool to reduce coupling.

Option A (defensive guard):

-func (h *PreHandler) shouldComputeOperationSha256(operationKit *OperationKit, reqCtx *requestContext) bool {
+func (h *PreHandler) shouldComputeOperationSha256(operationKit *OperationKit, reqCtx *requestContext) bool {
   // If forced, always compute the hash
-  if h.computeOperationSha256 || reqCtx.forceSha256Compute {
+  if h.computeOperationSha256 || (reqCtx != nil && reqCtx.forceSha256Compute) {
     return true
   }

Option B (narrow API):

-func (h *PreHandler) shouldComputeOperationSha256(operationKit *OperationKit, reqCtx *requestContext) bool {
+func (h *PreHandler) shouldComputeOperationSha256(operationKit *OperationKit, force bool) bool {
   // If forced, always compute the hash
-  if h.computeOperationSha256 || reqCtx.forceSha256Compute {
+  if h.computeOperationSha256 || force {
     return true
   }

And call site:

- if h.shouldComputeOperationSha256(operationKit, requestContext) {
+ if h.shouldComputeOperationSha256(operationKit, requestContext != nil && requestContext.forceSha256Compute) {

Also please confirm the RouterOnRequest hook runs before handleOperation so the force flag is visible.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 24, 2025

Router-nonroot image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-748eafe9458e4aec9da66896b113d3fde1900dfb-nonroot

@SkArchon
Copy link
Copy Markdown
Contributor Author

@coderabbitai generate docstrings

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 27, 2025

✅ Actions performed

Initiated docstring generation; will generate only if new commits exist.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 27, 2025

Caution

Docstrings generation - FAILED

No docstrings were generated.

@SkArchon SkArchon merged commit 83e0ed6 into main Oct 29, 2025
37 of 38 checks passed
@SkArchon SkArchon deleted the milinda/eng-8395-accessing-sha256hash-custom-modules branch October 29, 2025 08:36
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