feat: query filtering as validator module#1270
Open
Dodecahedr0x wants to merge 26 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new magicblock-query-filtering crate (auth, JWT tokens, permission PDA/types, transaction admission and redaction, attestation/quote) and integrates it across the validator: new aml_risk and query_filtering configs and consts; AML wiring into chainlink and MagicValidator; Aperture HTTP dispatcher token auth, auth endpoints, and authenticated_user injection; permission-aware account ensures and handler-level filtering/redaction; integration tests, Make/test-runner updates, and CI artifact sharding for a query_filtering test shard. Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-aperture/src/requests/http/get_signatures_for_address.rs`:
- Around line 60-65: The permission-ensuring path currently swallows errors:
change ensure_permission_accounts to return a Result (propagating the Result
from self.chainlink.ensure_accounts) instead of returning (), and update its
callers (including the handler in get_signatures_for_address.rs) to propagate
the error (or convert it into an immediate denied/fail response) so that
failures to populate permission PDAs do not default to
PermissionEntry::Unrestricted; specifically modify ensure_permission_accounts to
return Result<(), ErrorType>, have it return Err when
chainlink.ensure_accounts(...) fails, and update the call site in
get_signatures_for_address (and any other callers) to handle the Result
(propagate or return a permission-denied/failure) rather than discarding it.
In `@magicblock-aperture/src/requests/http/get_token_accounts_by_owner.rs`:
- Around line 70-74: The call to self.ensure_permission_accounts(...) is
discarding its Result; change the invocation in get_token_accounts_by_owner
(same pattern as in get_signatures_for_address.rs) to propagate errors by using
the try operator after awaiting it (i.e., await?); ensure the calling function's
signature returns a compatible Result so the propagated error type matches or is
converted as needed.
In `@magicblock-aperture/src/server/http/dispatch.rs`:
- Around line 460-481: Both json_response and quote_json_response currently call
.expect(...) on Response::builder().body(...), which can panic; change them to
use a non-panicking fallback (e.g., .unwrap_or_else(|build_err| { build and
return a safe 500 JSON Response with an "error" field describing the builder
error }) so request handling never panics. Specifically, in json_response
replace the .expect(...) with an unwrap_or_else that returns a
Response::builder().status(StatusCode::INTERNAL_SERVER_ERROR).header("content-type","application/json").body(JsonBody(serde_json::to_vec(&serde_json::json!({"error":
format!("response build failed: {}",
build_err)})).unwrap_or_default())).expect-free fallback; and in
quote_json_response apply the same unwrap_or_else to the Ok branch's
builder.body() so serialization errors still go through the json_response error
path and builder failures return the safe 500 JSON Response.
In `@magicblock-api/src/magic_validator.rs`:
- Around line 170-172: The current check logs an error when
config.query_filtering.jwt_secret equals consts::DEFAULT_JWT_SECRET; change this
to a hard failure during startup by returning or panicking instead of only
logging: locate the conditional that compares config.query_filtering.jwt_secret
to consts::DEFAULT_JWT_SECRET and replace the error!() call with logic that
aborts initialization (e.g., return Err(...) from the validator function or call
panic!() if initialization context requires it), ensuring the startup fails when
the default JWT secret is detected and include a clear message referencing the
default secret check.
In `@magicblock-config/src/consts.rs`:
- Around line 94-102: The startup must hard-fail when query filtering is enabled
but the JWT secret equals the placeholder DEFAULT_JWT_SECRET: change the
startup/config validation to return an error or abort initialization (instead of
only logging) when config.query_filtering.enabled is true and
config.query_filtering.jwt_secret == DEFAULT_JWT_SECRET; ensure this check runs
before constructing QueryFilteringService::new(...) so the service is never
created with the insecure secret, and include a clear error message referencing
DEFAULT_JWT_SECRET and config.query_filtering.jwt_secret in the failure path.
In `@magicblock-query-filtering/src/auth/service.rs`:
- Around line 99-110: The challenge timestamp check currently only rejects past
timestamps older than self.challenge_ttl_seconds and accepts arbitrarily future
timestamps, enabling replay; update the validation around
timestamp/challenge_time/now so that you also reject timestamps that are too far
in the future (e.g., if challenge_time > now + allowed_skew_seconds return an
appropriate AuthError) and clamp or treat small negative signed_duration_since
values as zero/skew (i.e., compute skew =
now.signed_duration_since(challenge_time), if skew < 0 then use min(skew.abs(),
allowed_skew_seconds) or reject when skew < -allowed_skew_seconds) before
comparing against self.challenge_ttl_seconds to enforce an upper bound on
forward-dated challenges.
In `@magicblock-query-filtering/src/auth/token.rs`:
- Around line 117-122: The test test_verify_invalid_expiry currently just
generates a fresh token with AuthTokenGenerator::new(...), calls generate and
verify, and therefore duplicates the valid/expired test; fix it by either (a)
implementing a real invalid-expiry scenario: construct or produce a token whose
"exp" claim is in the past (e.g., produce a token with an expired timestamp or
manipulate the payload so the exp is earlier than now) and assert that
AuthTokenGenerator::verify(&token) returns an Err (or panics) instead of Ok, or
(b) remove the duplicate test, or (c) simply rename test_verify_invalid_expiry
to reflect that it currently asserts a valid token's pubkey (it calls generate,
verify, and checks claims.pubkey). Ensure you modify the test function named
test_verify_invalid_expiry and refer to AuthTokenGenerator::new, generate,
verify, and claims.pubkey when making the change.
- Around line 99-104: The test test_verify_expired_token currently generates a
fresh token that never expires; change it to produce an expired token so verify
fails: either construct a Claims instance with an exp timestamp in the past and
sign it via AuthTokenGenerator::generate_from_claims (or the equivalent signing
method), or instantiate AuthTokenGenerator with a negative/zero lifetime (e.g.,
-1 or 0 seconds) before calling generate so the token is already expired; then
call generator.verify(&token) and assert that it returns the expected error for
expired tokens instead of unwrapping success. Ensure you reference
AuthTokenGenerator, Claims, generate (or the signing helper), and verify when
updating the test.
In `@magicblock-table-mania/src/derive_keypair.rs`:
- Line 32: Replace the panic-prone unwrap on seed.try_into() in
derive_keypair.rs: change the line creating SecretKey (let secret: SecretKey =
seed.try_into().unwrap();) to use explicit error handling or a documented
expect; either propagate a Result/Error from the function when try_into() fails
or use expect("seed must be 32 bytes as derived from hash[0..32]") to document
the invariant—refer to the variables/functions seed and SecretKey and the
try_into() call when making this change.
In `@test-integration/programs/flexi-counter/src/instruction.rs`:
- Around line 132-143: The UpdatePermission doc comment lists 6 accounts
(including "magic program" and "ephemeral vault") but the builder function
create_update_permission_ix constructs only 5 metas ending with the system
program; update the doc to match the actual account metas or update
create_update_permission_ix to include the missing accounts. Specifically,
either edit the UpdatePermission enum variant docblock to list the same five
accounts used by create_update_permission_ix, or modify
create_update_permission_ix to append the magic program and ephemeral vault
metas (and their signer/write flags) so the docs and builder align; reference
UpdatePermission and create_update_permission_ix when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bffc6edf-be39-4af7-8d5a-ef1499c32d56
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/programs/acl/acl.sois excluded by!**/*.so
📒 Files selected for processing (58)
.github/workflows/ci-test-integration.ymlCargo.tomlconfig.example.tomlmagicblock-aml/src/lib.rsmagicblock-aperture/Cargo.tomlmagicblock-aperture/src/lib.rsmagicblock-aperture/src/requests/http/get_account_info.rsmagicblock-aperture/src/requests/http/get_balance.rsmagicblock-aperture/src/requests/http/get_multiple_accounts.rsmagicblock-aperture/src/requests/http/get_program_accounts.rsmagicblock-aperture/src/requests/http/get_signatures_for_address.rsmagicblock-aperture/src/requests/http/get_token_account_balance.rsmagicblock-aperture/src/requests/http/get_token_accounts_by_delegate.rsmagicblock-aperture/src/requests/http/get_token_accounts_by_owner.rsmagicblock-aperture/src/requests/http/get_transaction.rsmagicblock-aperture/src/requests/http/mod.rsmagicblock-aperture/src/requests/http/send_transaction.rsmagicblock-aperture/src/requests/http/simulate_transaction.rsmagicblock-aperture/src/requests/mod.rsmagicblock-aperture/src/server/http/dispatch.rsmagicblock-aperture/src/server/http/mod.rsmagicblock-aperture/tests/setup.rsmagicblock-api/Cargo.tomlmagicblock-api/src/errors.rsmagicblock-api/src/magic_validator.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-config/src/config/aml_risk.rsmagicblock-config/src/config/chain.rsmagicblock-config/src/config/mod.rsmagicblock-config/src/config/query_filtering.rsmagicblock-config/src/consts.rsmagicblock-config/src/lib.rsmagicblock-config/src/tests.rsmagicblock-query-filtering/Cargo.tomlmagicblock-query-filtering/src/auth/mod.rsmagicblock-query-filtering/src/auth/service.rsmagicblock-query-filtering/src/auth/token.rsmagicblock-query-filtering/src/lib.rsmagicblock-query-filtering/src/quote/mod.rsmagicblock-query-filtering/src/quote/tdx_guest_native.rsmagicblock-query-filtering/src/service.rsmagicblock-query-filtering/src/transaction.rsmagicblock-query-filtering/src/types.rsmagicblock-table-mania/src/derive_keypair.rstest-integration/Cargo.tomltest-integration/Makefiletest-integration/configs/query-filtering-conf.devnet.tomltest-integration/configs/query-filtering-conf.ephem.tomltest-integration/programs/flexi-counter/src/instruction.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-query-filtering/Cargo.tomltest-integration/test-query-filtering/src/lib.rstest-integration/test-query-filtering/tests/account_visibility.rstest-integration/test-query-filtering/tests/auth.rstest-integration/test-query-filtering/tests/program_accounts.rstest-integration/test-query-filtering/tests/signatures.rstest-integration/test-query-filtering/tests/transactions.rstest-integration/test-runner/bin/run_tests.rs
💤 Files with no reviewable changes (1)
- magicblock-config/src/config/chain.rs
bmuddha
reviewed
Jun 2, 2026
Collaborator
bmuddha
left a comment
bmuddha
left a comment
There was a problem hiding this comment.
It seems the feature is quite tightly integrated into the validator and not as optional as the PR description would suggest. I wonder it could be designed to be compiled out via feature gating, although judging by current design it's spread out quite extensively. But may be you have better ideas/understanding in that regard.
Contributor
Author
Yes, I used "optional" in the sense that it can be disabled via config and should have a negligible impact when disabled. One of the goal was precisely to integrate it into the validator to improve filtering perfs. I can try to refactor it a bit for feature gating but I'm afraid it will be verbose. |
Contributor
Author
|
@bmuddha pushed a commit to feature gate the filtering but I'd happily reverse it as it makes the code less readable imo. Also, this is a tentative PR to integrate the filtering, it is not necessary to merge it and we can keep the existing service separated |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-query-filtering/src/quote/mod.rs`:
- Around line 73-85: The init_attested_key function currently races: multiple
callers can all call spawn_blocking(init_attested_key_blocking) before ATTESTED
is set, causing duplicate expensive TDX work; change initialization to serialize
the blocking call by introducing an async-aware guard (e.g., a
tokio::sync::OnceCell or a Mutex) around ATTESTED so only the first caller runs
init_attested_key_blocking via spawn_blocking and others await that result, then
store the result into ATTESTED with get_or_init; update init_attested_key to
check the async guard first, run spawn_blocking only once, and return the stored
&'static AttestedKeyMaterial.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87cb2cc2-ad93-4e22-8189-ebd4e161c922
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
magicblock-aperture/Cargo.tomlmagicblock-aperture/src/lib.rsmagicblock-aperture/src/requests/http/get_account_info.rsmagicblock-aperture/src/requests/http/get_balance.rsmagicblock-aperture/src/requests/http/get_multiple_accounts.rsmagicblock-aperture/src/requests/http/get_program_accounts.rsmagicblock-aperture/src/requests/http/get_signatures_for_address.rsmagicblock-aperture/src/requests/http/get_token_account_balance.rsmagicblock-aperture/src/requests/http/get_token_accounts_by_delegate.rsmagicblock-aperture/src/requests/http/get_token_accounts_by_owner.rsmagicblock-aperture/src/requests/http/get_transaction.rsmagicblock-aperture/src/requests/http/mod.rsmagicblock-aperture/src/requests/http/send_transaction.rsmagicblock-aperture/src/requests/http/simulate_transaction.rsmagicblock-aperture/src/server/http/dispatch.rsmagicblock-aperture/src/server/http/mod.rsmagicblock-aperture/tests/setup.rsmagicblock-api/Cargo.tomlmagicblock-api/src/errors.rsmagicblock-api/src/magic_validator.rsmagicblock-query-filtering/Cargo.tomlmagicblock-query-filtering/src/quote/mod.rsmagicblock-query-filtering/src/service.rsmagicblock-validator/Cargo.tomltest-integration/test-tools/src/validator.rs
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
The query filtering used on TEE nodes is currently a separate service in a private repo. This PR reproduces the same behavior as an optional module of the validator. The benefits are:
Breaking Changes
Summary by CodeRabbit
New Features
Behavior Changes
Tests