feat: Enforce api keys #479
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces a comprehensive API key permission system for the OpenZeppelin Relayer. It adds a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as Handler<br/>(with `@require_permissions`)
participant Macro as Macro<br/>(Code Gen)
participant Auth as validate_api_key_permissions()
participant Repo as API Key Repo
Client->>Handler: POST /api/...
Handler->>Macro: Execute injected permission checks
Macro->>Auth: validate_api_key_permissions(request, permissions)
Auth->>Auth: Extract Bearer token from header
Auth->>Repo: Get API key by value
Repo-->>Auth: ApiKeyRepoModel with PermissionGrants
Auth->>Auth: Match required permissions<br/>against grants (wildcard logic)
alt Permission matches
Auth-->>Macro: Ok(())
Macro-->>Handler: Continue to handler logic
Handler-->>Client: Response
else Permission denied
Auth-->>Macro: Err(Forbidden)
Macro-->>Client: 403 Forbidden
end
sequenceDiagram
participant Handler as Scoped Handler<br/>@require_permissions([("resource:write", "resource_id")])
participant Macro as Macro (Code Gen)
participant Auth as validate_api_key_permissions_scoped()
participant Repo as API Key Repo
Handler->>Macro: Extract resource_id from path/body
Macro->>Auth: validate_api_key_permissions_scoped(request, action,<br/>resource_id)
Auth->>Repo: Get API key by Bearer token
Repo-->>Auth: Return PermissionGrants
Auth->>Auth: Check if any grant allows action<br/>for specific resource_id
alt Grant scope == Global
Auth->>Auth: Allow (global access)
else Grant scope == Ids[resource_id]
Auth->>Auth: Allow (scoped access)
else No matching grant
Auth-->>Handler: Err(Forbidden)
end
Auth-->>Handler: Ok(())
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
Comment |
src/api/routes/plugin.rs
Outdated
| use relayer_macros::require_permissions; | ||
|
|
||
| /// List plugins | ||
| #[require_permissions(["plugins:get:all"])] |
There was a problem hiding this comment.
example of regular permission (no parameters)
src/api/routes/plugin.rs
Outdated
| } | ||
|
|
||
| /// Calls a plugin method. | ||
| #[require_permissions(["plugins:execute:{plugin_id}"])] |
There was a problem hiding this comment.
Example of scoped permission, (plugin_id parameter), an API key that passes this authorization must contain a permission of the form:
plugins:execute:abc-abc-abc-abc AND the plugin_id passed as function parameter must be: abc-abc-abc-abc
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
Thanks for your work!
In general i like the idea of macro at route level.
In the future for some advanced cases we would need standard methods to invoke to check permissions logic but for most cases macros would do the job.
I think we should just see how permissions should be defined.
I have added my two cents in the comments. In general i think permissions should be simple and we can define scope outside of permission string and use in the logic to check. This would eliminate need to define multiple permissions, * and for specific id and would keep permission system simpler.
macros/src/tests.rs
Outdated
| fn test_macro_detects_template_parameters() { | ||
| // This is a compile-time test - if the macro compiles, it works | ||
| let tokens = quote::quote! { | ||
| #[require_permissions(["relayers:get:{relayer_id}"])] |
There was a problem hiding this comment.
can it work with multiple permissions
For example required permissions are to get specific relayer or to be able to get any relayer?
There was a problem hiding this comment.
It works with multiple ones, but in this specific case relayers:get:{relayer_id} should match both a specific or all relayers (using wildcards).
- If API key contains
relayers:get:abc-abc-abc-abcpermission, then it passes. - If API key contains
relayers:get:*permission, then also passes.
I think there might be endpoints where we have more than one as you said, e.g. if you want to enforce both signing:execute:{relayer_id} + transactions:execute:{relayer_id} in the same endpoint:
#[require_permissions(["signing:execute:{relayer_id}", "transactions:execute:{relayer_id}"])]should work
src/api/routes/api_keys.rs
Outdated
| } | ||
|
|
||
| /// Get API key permissions | ||
| #[require_permissions(["api_keys:get:{api_key_id}"])] |
There was a problem hiding this comment.
one api key can have api_keys:get:some_id
some key can have access to all. is that api_keys:get:all?
if we want OR condition are we defining both permissions like:
#[require_permissions(["api_keys:get:{api_key_id}", "api_keys:get:all"])]
There was a problem hiding this comment.
I think in case you want to have some user accessing all the api keys you will use a wildcard.
similar to what I mentioned here
The :all suffix in permissions corresponds to the permission to list, rather to access all (this is in cases where listing does not include all the information of each entry). But I see it creates some confusion, maybe replacing ..:get:all by ..:list:all in the action makes more sense
src/api/routes/plugin.rs
Outdated
| } | ||
|
|
||
| /// Calls a plugin method. | ||
| #[require_permissions(["plugins:execute:{plugin_id}"])] |
There was a problem hiding this comment.
In terms of plugins, only execute plugin permissions are needed?
We do not check resources used in plugin logic?
For example plugin sends transaction via some relayer. Does relayer permission needs to be there or just plugin perm is enough?
There was a problem hiding this comment.
Yes this was the most complicated part, the challenge there is that we don't really control the plugin interactions at "routes" level, Maybe doing some extra validation inside the relayer-api in /plugins works
macros/src/tests.rs
Outdated
| fn test_macro_detects_template_parameters() { | ||
| // This is a compile-time test - if the macro compiles, it works | ||
| let tokens = quote::quote! { | ||
| #[require_permissions(["relayers:get:{relayer_id}"])] |
There was a problem hiding this comment.
What if we keep permissions simple like
notifications:list
notifications:read
notifications:create
notifications:update
notifications:delete
and then define scope for permissions which can be either global or list of ids.
There was a problem hiding this comment.
This way we are keeping permissions simple, for example relayers:read, and later in the logic we check scope of that permission which can be either global or list of items.
That way we do not need to tie permissions with specific entities.
for example api route can just be docorated with simple permission relayers:read. If api key has that permission our logic will check for permission scope which is defined at api key create phase.
Macro fingerprint would needed to be slightly modified, for example
#[require_permissions(["relayers:read"], "relayer_id")]
permission struct example:
{
"action": "relayers:read",
"resource": { "ids": ["*"] },
}
WDYT?
There was a problem hiding this comment.
Agree! that makes it simpler :)
|
Hi @zeljkoX I simplified the implementation by supporting macro with simple permissions + scope as defined in the PR description. Now the remaining challenge is how to propagate the API key permissions to plugins - when invoking plugins, we must pass the api key and somehow limit execution based on permissions, but I believe this is something we can add in the future, for now we can assume that plugin access represents being a super-user of relayers (adding a warning about this in documentation). Also I have added an extension to openapi route docs to reflect the required API key permissions in each endpoint using |
| name: "Test API Key".to_string(), | ||
| allowed_origins: vec!["*".to_string()], | ||
| permissions: vec!["relayer:all:execute".to_string()], | ||
| permissions: vec![PermissionGrant::global("relayers:execute")], |
There was a problem hiding this comment.
Improved the permissions definition, instead of using string literals we will use PermissionGrant struct
| } | ||
|
|
||
| /// Cancels a specific transaction by its ID. | ||
| #[require_permissions([("transactions:delete", "path.relayer_id")])] |
There was a problem hiding this comment.
note that for path parameters we can use dotted syntax
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/repositories/api_key/api_key_redis.rs (1)
163-188: Race: duplicate API key values slip through on concurrent createsLine [169] checks uniqueness with
GET, but the subsequentpipe.set(&value_index_key, &entity.id)runs without any NX guard. Two requests creating the same value can both observe a missing index and race intoEXEC, leaving two stored API keys while the index ends up pointing to whichever write finished last. That violates the uniqueness guarantee.Use an atomic reservation (e.g.,
SETNX) before writing the rest of the data and roll it back if the pipeline fails, or wrap everything in a Lua script so the uniqueness check and writes execute as one unit.Apply something along these lines:
- // Check if value already exists - let value_exists: Option<String> = conn - .get(&value_index_key) - .await - .map_err(|e| self.map_redis_error(e, "create_api_key_value_check"))?; - - if value_exists.is_some() { - return Err(RepositoryError::ConstraintViolation( - "API Key with this value already exists".to_string(), - )); - } + let reserved = conn + .set_nx(&value_index_key, &entity.id) + .await + .map_err(|e| self.map_redis_error(e, "create_api_key_value_reserve"))?; + + if !reserved { + return Err(RepositoryError::ConstraintViolation( + "API Key with this value already exists".to_string(), + )); + } ... - pipe.set(&value_index_key, &entity.id); - - pipe.exec_async(&mut conn) - .await - .map_err(|e| self.map_redis_error(e, "create_api_key"))?; + if let Err(err) = pipe + .exec_async(&mut conn) + .await + .map_err(|e| self.map_redis_error(e, "create_api_key")) + { + let _ = conn.del(&value_index_key).await; + return Err(err); + }
♻️ Duplicate comments (1)
macros/src/tests.rs (1)
25-38: Same issue as above: parsing-only test and legacy format.Update to the tuple/global form and move to
trybuildso the attribute actually expands and type-checks.
🧹 Nitpick comments (7)
src/utils/encryption.rs (1)
130-136: Improved error handling with try_from.The use of
try_fromprovides better error messages than the previous explicit length check. However, this shares the same verbosity concern as the key construction pattern (lines 88-90).src/api/routes/mod.rs (1)
55-75: Test validates macro compilation but not permission logic.The test correctly verifies that the
require_permissionsmacro compiles and that the macro-annotated endpoint returns a server error when no app state is provided. This is expected behavior since the permission validation would fail without a configured API key repository.Consider adding integration tests that:
- Set up proper app state with a mock API key repository
- Test successful permission validation with valid API keys
- Test permission denial with insufficient permissions
This would provide more comprehensive coverage of the permission enforcement system.
src/utils/mocks.rs (1)
299-301: Provide non-admin mock API keys to improve auth test coverage.
PermissionGrant::global("*:*")is fine for happy paths, but add helpers for:
- global read-only (e.g.,
relayers:read)- scoped keys (action + specific IDs)
Example helper (add near other mock fns):
pub fn create_scoped_api_key(id: &str, action: &str, ids: &[&str]) -> ApiKeyRepoModel { ApiKeyRepoModel { id: id.to_string(), name: format!("mock-{}", id), value: SecretString::new(format!("value-{}", id)), permissions: vec![PermissionGrant::scoped(action.to_string(), ids.iter().map(|s| s.to_string()).collect())], created_at: Utc::now().to_string(), } }This lets tests assert 200 vs 403 across relayers/plugins cleanly.
src/api/routes/docs/plugin_docs.rs (1)
21-27: Docs align with the new permission model; add a temporary caveat.Until API-key permission propagation into plugins is implemented, note that plugin execution may act with elevated privileges. Add a one-line warning in the endpoint description or as an
x-noteextension to prevent surprises.Would you like a short wording snippet for the caveat?
openapi.json (2)
5-5: Nice: comprehensive auth/permission overview ininfo.description.Consider adding a brief “Plugin permission propagation” note stating that, for now, plugins may execute with admin-like privileges even when the calling API key is scoped. This keeps docs consistent with current behavior.
110-115: Consistentx-required-permissionsacross endpoints — good coverage.
- Scopes and
scopeParameterlook correct and consistent with route params.- Minor:
/api/v1/relayers/{relayer_id}/transactions/has a trailing slash unlike siblings. Consider normalizing or ensuring the router mirrors this exactly to avoid client confusion.If helpful, I can provide a script to diff route param names against
scopeParametervalues to prevent drift.Also applies to: 210-215, 313-319, 420-426, 532-538, 695-701, 811-816, 911-916, 1029-1035, 1131-1137, 1258-1264, 1377-1383, 1517-1523, 1646-1652, 1775-1781, 1904-1910, 2023-2029, 2152-2158, 2291-2297, 2420-2426, 2539-2545, 2667-2673, 2803-2809, 2929-2935, 3030-3035, 3131-3135, 3233-3239, 3340-3346, 3452-3458, 3484-3494, 3523-3533, 3584-3590, 7877-7879
src/constants/permissions.rs (1)
53-58: Avoid per-call allocation when matching scoped IDs
ids.contains(&requested_id.to_string())clones therequested_idon every check. Switching toids.iter().any(|candidate| candidate == requested_id)keeps the logic identical without allocating.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.gitignore(1 hunks)Cargo.toml(2 hunks)README.md(1 hunks)macros/Cargo.toml(1 hunks)macros/src/lib.rs(1 hunks)macros/src/tests.rs(1 hunks)openapi.json(34 hunks)src/api/controllers/api_key.rs(8 hunks)src/api/routes/api_keys.rs(1 hunks)src/api/routes/docs/notification_docs.rs(5 hunks)src/api/routes/docs/plugin_docs.rs(2 hunks)src/api/routes/docs/relayer_docs.rs(18 hunks)src/api/routes/docs/signer_docs.rs(5 hunks)src/api/routes/metrics.rs(9 hunks)src/api/routes/mod.rs(1 hunks)src/api/routes/notification.rs(1 hunks)src/api/routes/plugin.rs(1 hunks)src/api/routes/relayer.rs(4 hunks)src/api/routes/signer.rs(1 hunks)src/bootstrap/config_processor.rs(2 hunks)src/constants/mod.rs(1 hunks)src/constants/permissions.rs(1 hunks)src/models/api_key/repository.rs(3 hunks)src/models/api_key/request.rs(1 hunks)src/models/api_key/response.rs(1 hunks)src/models/mod.rs(1 hunks)src/models/permission.rs(1 hunks)src/openapi.rs(1 hunks)src/repositories/api_key/api_key_in_memory.rs(12 hunks)src/repositories/api_key/api_key_redis.rs(16 hunks)src/repositories/api_key/mod.rs(19 hunks)src/services/turnkey/mod.rs(2 hunks)src/utils/auth.rs(4 hunks)src/utils/encryption.rs(4 hunks)src/utils/mocks.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T11:19:51.397Z
Learnt from: LuisUrrutia
Repo: OpenZeppelin/openzeppelin-relayer PR: 427
File: src/constants/network_tags.rs:5-6
Timestamp: 2025-08-28T11:19:51.397Z
Learning: In the OpenZeppelin relayer codebase, when deprecating constants that are still used internally (like OPTIMISM_TAG in LACKS_MEMPOOL_TAGS), use doc comments instead of #[deprecated] attribute to avoid clippy failures during the transition period.
Applied to files:
src/openapi.rs
🧬 Code graph analysis (18)
macros/src/tests.rs (1)
macros/src/lib.rs (1)
require_permissions(74-215)
macros/src/lib.rs (2)
src/utils/auth.rs (2)
validate_api_key_permissions(54-94)validate_api_key_permissions_scoped(109-150)src/models/app_state.rs (1)
api_key_repository(190-192)
src/api/routes/plugin.rs (2)
macros/src/lib.rs (1)
require_permissions(74-215)src/api/controllers/plugin.rs (1)
list_plugins(127-154)
src/utils/auth.rs (1)
src/constants/permissions.rs (2)
has_permission_grant(114-118)has_permission_grant_for_id(131-139)
src/constants/permissions.rs (1)
src/models/permission.rs (2)
ids(48-53)global(38-45)
src/api/routes/signer.rs (2)
macros/src/lib.rs (1)
require_permissions(74-215)src/api/controllers/signer.rs (5)
list_signers(35-62)get_signer(74-93)create_signer(112-137)update_signer(155-174)delete_signer(192-231)
src/api/controllers/api_key.rs (4)
src/models/api_key/repository.rs (1)
try_from(37-45)src/models/api_key/response.rs (2)
try_from(15-22)try_from(37-45)src/utils/mocks.rs (1)
create_mock_app_state(179-278)src/api/routes/api_keys.rs (1)
delete_api_key(46-52)
src/api/routes/notification.rs (1)
macros/src/lib.rs (1)
require_permissions(74-215)
src/api/routes/mod.rs (1)
macros/src/lib.rs (1)
require_permissions(74-215)
src/api/routes/api_keys.rs (2)
macros/src/lib.rs (1)
require_permissions(74-215)src/api/controllers/api_key.rs (4)
list_api_keys(73-106)get_api_key_permissions(115-136)create_api_key(37-59)delete_api_key(145-176)
src/models/api_key/repository.rs (2)
src/models/permission.rs (1)
global(38-45)src/models/api_key/response.rs (2)
try_from(15-22)try_from(37-45)
src/api/routes/metrics.rs (2)
src/metrics/mod.rs (2)
update_system_metrics(111-159)gather_metrics(102-108)macros/src/lib.rs (1)
require_permissions(74-215)
src/repositories/api_key/api_key_in_memory.rs (4)
src/repositories/api_key/api_key_redis.rs (6)
get_by_value(272-300)list_permissions(302-314)new(27-41)test_get_by_value_existing(691-706)test_get_by_value_non_existing(710-721)test_get_by_value_empty_store(725-731)src/repositories/api_key/mod.rs (4)
get_by_value(41-41)get_by_value(87-92)list_permissions(48-51)list_permissions(101-109)src/models/permission.rs (1)
global(38-45)src/models/api_key/repository.rs (1)
new(23-31)
src/bootstrap/config_processor.rs (1)
src/models/permission.rs (1)
global(38-45)
src/models/api_key/response.rs (1)
src/models/api_key/repository.rs (1)
try_from(37-45)
src/api/routes/relayer.rs (3)
macros/src/lib.rs (1)
require_permissions(74-215)src/api/controllers/relayer.rs (3)
get_relayer(87-107)send_transaction(417-437)get_transaction_by_nonce(492-526)src/models/permission.rs (1)
global(38-45)
src/repositories/api_key/mod.rs (4)
src/repositories/api_key/api_key_in_memory.rs (5)
get_by_value(66-75)create(55-59)count(102-105)list_permissions(107-119)new(36-40)src/repositories/api_key/api_key_redis.rs (6)
get_by_value(272-300)create(135-188)count(372-382)list_permissions(302-314)new(27-41)create_test_api_key(454-462)src/models/api_key/repository.rs (1)
new(23-31)src/models/permission.rs (1)
global(38-45)
src/repositories/api_key/api_key_redis.rs (3)
src/models/permission.rs (2)
fmt(70-77)global(38-45)src/repositories/api_key/api_key_in_memory.rs (3)
new(36-40)get_by_value(66-75)list_permissions(107-119)src/repositories/api_key/mod.rs (5)
get_by_value(41-41)get_by_value(87-92)list_permissions(48-51)list_permissions(101-109)create_test_api_key(165-178)
🪛 Gitleaks (8.28.0)
README.md
[high] 512-514: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
⏰ 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). (6)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: Analyze (rust)
🔇 Additional comments (17)
src/utils/encryption.rs (4)
67-68: LGTM: Iterator-based key construction.The migration from slice-based to iterator-based construction is correctly implemented. Using
from_iterwithiter().copied()maintains the same functional behavior.
99-104: LGTM: Nonce construction and encryption API updated.The iterator-based nonce construction and passing a reference to
encryptare consistent with the overall API migration pattern.
141-141: LGTM: Decryption API updated consistently.Passing
&nonceas a reference is consistent with the encryption API changes.
1-195: Changes are intentional and directly support the API key system—no verification needed.The encryption utility directly encrypts API keys. Evidence:
- Test case (line 281): encrypts "Sensitive API key: sk-1234567890abcdef"
macros/src/lib.rs: proc macro for API key permission checking integrates with this encryption infrastructure- The API migrations are dependency-driven updates (aes-gcm, zeroize versions in Cargo.toml)
These changes are essential for the PR's API key permission system, not unintentionally bundled.
Likely an incorrect or invalid review comment.
.gitignore (1)
46-47: LGTM!The addition of AGENTS.md to the gitignore is appropriate for excluding generated or temporary AI agent documentation from version control.
src/constants/mod.rs (1)
44-45: LGTM!The permissions module addition follows the established pattern in this file and appropriately exposes the new permission checking utilities to the rest of the codebase.
Cargo.toml (2)
1-2: LGTM!The workspace configuration correctly establishes a multi-crate structure for the procedural macro, following Cargo best practices.
22-22: LGTM!The relayer-macros dependency is correctly configured as a path dependency pointing to the local macros crate.
macros/Cargo.toml (1)
1-12: LGTM!The proc-macro crate manifest is correctly configured with the standard dependencies (proc-macro2, quote, syn) needed for procedural macro development. The
proc-macro = truesetting and syn's "full" features are appropriate for this use case.src/models/mod.rs (1)
56-57: LGTM!The permission module addition follows the established pattern and appropriately exposes the new PermissionGrant types that underpin the API key permission system.
src/models/api_key/request.rs (1)
1-8: Verify backward compatibility impact.The change from
Vec<String>toVec<PermissionGrant>provides better type safety and structure. However, this is a breaking change to the API contract. Ensure that:
- API versioning or migration strategy is in place
- SDK and client documentation are updated
- Any existing API keys are migrated appropriately
Based on the PR objectives, this appears intentional, but confirming the migration path would be helpful. The PR mentions "Impact on openzeppelin-relayer-sdk: none" which suggests clients can handle this transparently.
README.md (1)
395-526: Excellent documentation addition!The API Key Permissions System documentation is comprehensive and well-structured, covering:
- Permission structure and format
- Wildcard support
- Permission hierarchy
- Practical examples
- API usage instructions
The curl example at lines 512-524 correctly uses a placeholder (
YOUR_API_KEY) for demonstration purposes.Note: The static analysis hint about the curl command is a false positive since this is documentation with a placeholder value, not actual credentials.
src/utils/mocks.rs (1)
19-19: Good: type-safe permissions in mocks.Importing and using
PermissionGrantaligns mocks with the new model.src/api/routes/docs/plugin_docs.rs (1)
130-135: LGTM: global read permission for listing plugins documented.Matches the intended model.
src/models/api_key/response.rs (1)
16-44: Response models align with structured permissionsBoth
TryFrom<ApiKeyRepoModel>impls now surfacePermissionGrantdata directly, keeping list responses scoped and only returning the secret value for create. Looks consistent with the new permission model.src/models/permission.rs (1)
36-102: PermissionGrant API is well-scopedHelper constructors plus
is_global/get_idsgive the rest of the codebase a clean surface, and serde flattening matches the documented JSON shape.src/models/api_key/repository.rs (1)
23-45: Repository model updates preserve invariants
ApiKeyRepoModel::newand theTryFrom<ApiKeyRequest>impl both still generate server-side IDs/secrets while adoptingPermissionGrant, so downstream consumers keep the same guarantees.
macros/src/lib.rs
Outdated
| // Combine all permission checks | ||
| let combined_checks = quote! { | ||
| #(#permission_checks)* | ||
| }; | ||
|
|
||
| // Parse as a statement and prepend to function body | ||
| let permission_check_stmt: syn::Stmt = syn::parse2(combined_checks).unwrap(); | ||
| input_fn.block.stmts.insert(0, permission_check_stmt); |
There was a problem hiding this comment.
Fix macro parsing when multiple permission blocks
syn::parse2::<Stmt>(#combined_checks) only succeeds when there is exactly one generated statement. As soon as the attribute contains more than one scoped permission (e.g. [("relayers:read", "relayer_id"), ("transactions:execute", "relayer_id")]), #(#permission_checks)* expands to consecutive blocks, parse2 stops at the first, and then panics because of the trailing tokens. That turns a perfectly valid macro invocation into a compile-time crash. Please wrap the injected statements in a single block (or parse each permission_check individually) before inserting them.
- let combined_checks = quote! {
- #(#permission_checks)*
- };
- let permission_check_stmt: syn::Stmt = syn::parse2(combined_checks).unwrap();
+ let combined_checks = quote!({
+ #(#permission_checks)*
+ });
+ let permission_check_stmt: syn::Stmt = syn::parse2(combined_checks).unwrap();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Combine all permission checks | |
| let combined_checks = quote! { | |
| #(#permission_checks)* | |
| }; | |
| // Parse as a statement and prepend to function body | |
| let permission_check_stmt: syn::Stmt = syn::parse2(combined_checks).unwrap(); | |
| input_fn.block.stmts.insert(0, permission_check_stmt); | |
| // Combine all permission checks | |
| let combined_checks = quote!({ | |
| #(#permission_checks)* | |
| }); | |
| // Parse as a statement and prepend to function body | |
| let permission_check_stmt: syn::Stmt = syn::parse2(combined_checks).unwrap(); | |
| input_fn.block.stmts.insert(0, permission_check_stmt); |
🤖 Prompt for AI Agents
In macros/src/lib.rs around lines 201 to 208, the code currently builds
combined_checks with #(#permission_checks)* and calls syn::parse2::<syn::Stmt>,
which fails when there are multiple generated statements because parse2 stops at
the first token; change the injection so it becomes a single statement: either
wrap the generated statements in one block (e.g. quote! { {
#(#permission_checks)* } } ) and parse that as a single syn::Stmt (a Block)
before inserting, or parse each permission_check individually and push them
one-by-one into input_fn.block.stmts; implement the block-wrapping approach for
the simplest fix.
macros/src/tests.rs
Outdated
| fn test_macro_detects_template_parameters() { | ||
| // This is a compile-time test - if the macro compiles, it works | ||
| let tokens = quote::quote! { | ||
| #[require_permissions(["relayers:get:{relayer_id}"])] | ||
| async fn test_function( | ||
| relayer_id: actix_web::web::Path<String>, | ||
| raw_request: actix_web::HttpRequest, | ||
| data: actix_web::web::ThinData<String>, | ||
| ) -> actix_web::HttpResponse { | ||
| actix_web::HttpResponse::Ok().finish() | ||
| } | ||
| }; | ||
|
|
||
| // If we can parse this without panicking, the macro structure is correct | ||
| let parsed = syn::parse2::<syn::ItemFn>(tokens); | ||
| assert!(parsed.is_ok()); | ||
| } |
There was a problem hiding this comment.
Tests don’t exercise macro expansion and use outdated permission syntax.
- These tests only parse tokens with syn and don’t expand
require_permissions, so they’ll pass even if the macro is broken. - They also use legacy
relayers:get:{relayer_id}format; current macro expects("action", "param")tuples and supports dotted paths.
Switch to UI/compilation tests with trybuild, and update to the new tuple/dotted-path API. Example:
-#[require_permissions(["relayers:get:{relayer_id}"])]
+#[require_permissions([("relayers:read", "relayer_id")])]And add trybuild cases that:
- compile:
#[require_permissions(["relayers:read"])](global) - compile:
#[require_permissions([("transactions:read","path.relayer_id")])] - fail with clear error when the base param is missing.
Committable suggestion skipped: line range outside the PR's diff.
macros/src/tests.rs
Outdated
| fn test_parameter_extraction_logic() { | ||
| let permission = "relayers:get:{relayer_id}"; | ||
| let mut required_params = std::collections::HashSet::new(); | ||
|
|
||
| // Extract parameter names from {param_name} patterns (same logic as in macro) | ||
| let chars = permission.chars(); | ||
| let mut inside_braces = false; | ||
| let mut current_param = String::new(); | ||
|
|
||
| for ch in chars { | ||
| if ch == '{' { | ||
| inside_braces = true; | ||
| current_param.clear(); | ||
| } else if ch == '}' && inside_braces { | ||
| if !current_param.is_empty() { | ||
| required_params.insert(current_param.clone()); | ||
| } | ||
| inside_braces = false; | ||
| } else if inside_braces { | ||
| current_param.push(ch); | ||
| } | ||
| } | ||
|
|
||
| assert_eq!(required_params.len(), 1); | ||
| assert!(required_params.contains("relayer_id")); | ||
| } |
There was a problem hiding this comment.
Brace-based parameter extraction test is obsolete.
The macro no longer inspects {param}; it parses scoped tuples and supports dotted paths via split('.'). Replace this with tests that validate:
- dotted path works:
("transactions:read","path.relayer_id") - missing base parameter triggers the macro’s panic with your custom message (via
trybuildUI test).
src/api/routes/mod.rs
Outdated
| } | ||
|
|
||
| // Mock endpoint with require_permissions macro using const | ||
| #[require_permissions(["relayers:get:all"])] |
There was a problem hiding this comment.
Fix permission string to match documented format.
The permission string "relayers:get:all" doesn't align with the documented action format shown in README.md. According to the documentation, actions should follow the pattern {resource}:{operation} where operations are: read, create, update, delete, execute, or * for wildcards.
Apply this diff to use the documented format:
- #[require_permissions(["relayers:get:all"])]
+ #[require_permissions(["relayers:read"])]Alternatively, if the intent is to test wildcard permissions, use:
- #[require_permissions(["relayers:get:all"])]
+ #[require_permissions(["relayers:*"])]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[require_permissions(["relayers:get:all"])] | |
| #[require_permissions(["relayers:read"])] |
🤖 Prompt for AI Agents
In src/api/routes/mod.rs around line 44, the permission string
#[require_permissions(["relayers:get:all"])] uses a nonconforming action format;
replace it with the documented resource:operation format, e.g.
#[require_permissions(["relayers:read"])] for read access, or
#[require_permissions(["relayers:*"])] if the intent is to allow all
actions/wildcard access.
| permissions: vec![PermissionGrant::global("*:*")], | ||
| created_at: chrono::Utc::now().to_rfc3339(), | ||
| allowed_origins: vec!["*".to_string()], | ||
| }; | ||
| api_key_repo.create(test_api_key).await.unwrap(); |
There was a problem hiding this comment.
Update test requests to include the API key header
With #[require_permissions(...)] in place, every route now calls validate_api_key_permissions before hitting the controller. Because the test requests in test_routes_are_registered still go out without an API key, the new guard will stop them with an Unauthorized/Forbidden response, so the current INTERNAL_SERVER_ERROR assertions will start failing. Please inject the seeded key (test-value) into each request (or adjust the expected status accordingly). For example:
- let req = test::TestRequest::get().uri("/relayers").to_request();
+ let req = test::TestRequest::get()
+ .uri("/relayers")
+ .insert_header(("x-api-key", "test-value"))
+ .to_request();Repeat the header insertion for the rest of the requests in this test so the route-registration checks remain valid.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/api/routes/relayer.rs around lines 394 to 397, the test_requests in
test_routes_are_registered are missing the seeded API key header and are being
rejected by the new #[require_permissions(...)] guard; update each request in
this test to include the API key header (e.g., add header "x-api-key" with value
"test-value") before sending, and repeat this header insertion for every other
request in that test so the route-registration assertions keep returning the
expected status (or alternatively adjust the expected status assertions to match
Unauthorized/Forbidden if you intentionally want no key).
src/utils/auth.rs
Outdated
| info!("Token: {}", token); | ||
| return true; | ||
| } | ||
| false | ||
| } | ||
|
|
||
| /// Helper to validate API key and check permissions for global-scoped actions | ||
| /// |
There was a problem hiding this comment.
Restore API key equality check and remove secret logging.
Right now any request with a syntactically valid bearer header sails through because we never compare the token to _expected_key, and we also emit the full API key to INFO logs. That’s a critical security regression: callers can bypass the guard entirely and the log line leaks the shared secret. Please reinstate the equality check and stop logging the token.
@@
- if token.is_empty() || token.contains(' ') {
- return false;
- }
-
- info!("Token: {}", token);
- return true;
+ if token.is_empty() || token.contains(' ') {
+ return false;
+ }
+
+ if token != _expected_key.to_str() {
+ return false;
+ }
+
+ true(Also drop the now-unused use tracing::info; import.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| info!("Token: {}", token); | |
| return true; | |
| } | |
| false | |
| } | |
| /// Helper to validate API key and check permissions for global-scoped actions | |
| /// | |
| if token.is_empty() || token.contains(' ') { | |
| return false; | |
| } | |
| if token != _expected_key.to_str() { | |
| return false; | |
| } | |
| true | |
| } | |
| false | |
| } |
🤖 Prompt for AI Agents
In src/utils/auth.rs around lines 36 to 43, restore the missing equality check
against _expected_key and remove the insecure logging of the bearer token:
replace the unconditional success path with a comparison that returns true only
when token == _expected_key (or constant-time equality if available), eliminate
the info!("Token: {}", token) line so the secret is not logged, and remove the
now-unused use tracing::info; import to avoid dead code warnings.
| let key_array = <&[u8; 32]>::try_from(key_bytes.as_slice()) | ||
| .map_err(|_| EncryptionError::InvalidKeyLength(key_bytes.len()))?; | ||
| Ok(Key::<Aes256Gcm>::from_iter(key_array.iter().copied())) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Consider simplifying the key construction pattern.
The combination of try_from followed by from_iter with iter().copied() works correctly but appears verbose. If the aes_gcm crate supports it, consider using a more direct construction method.
However, if this pattern is dictated by the crate's API changes, the current implementation is acceptable.
🏁 Script executed:
#!/bin/bash
# Check the aes_gcm crate version and available Key construction methods
rg -n "aes_gcm.*=" Cargo.tomlLength of output: 0
🏁 Script executed:
#!/bin/bash
# Locate and examine Cargo.toml for dependencies
find . -name "Cargo.toml" -type f | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat {}'Length of output: 3886
🏁 Script executed:
#!/bin/bash
# Check the encryption.rs file imports and the key loading function
fd -e rs "encryption.rs" | xargs -I {} sh -c 'echo "=== {} ===" && cat {}'Length of output: 31514
🏁 Script executed:
#!/bin/bash
# Search for aes_gcm usage in the codebase
rg -n "aes_gcm|aes-gcm" -A 2 -B 2Length of output: 1242
🌐 Web query:
aes-gcm 0.10 KeyInit trait from_iter construction methods documentation
💡 Result:
Summary — aes-gcm 0.10 / KeyInit
- The KeyInit trait (from the aead crate, re-exported by aes-gcm) provides the standard initialization API used by AES‑GCM types: notably new(...) to construct a cipher from a key and generate_key(...) to create a key from an RNG. The aes-gcm crate shows usage via Key::from_slice / Key::::from_slice and generating keys with OsRng. [1][2]
Example usage patterns shown in the aes-gcm docs:
- generate a random key: let key = Aes256Gcm::generate_key(OsRng);
- build a Key from bytes: let key = Key::::from_slice(bytes);
- construct the cipher: let cipher = Aes256Gcm::new(&key);
(See docs for full signatures and examples.) [1]
Sources
- aes-gcm crate documentation (examples: Key::from_slice, Aes256Gcm::new, generate_key). [1]
- aead crate documentation (KeyInit trait is defined there; aes-gcm re-exports it). [2]
Simplify key construction using from_slice() after validation.
After the length check at line 85, the try_from followed by from_iter is redundant. The KeyInit trait (from the aead crate) provides Key::<Aes256Gcm>::from_slice() as a simpler construction method. Since the key is already validated as 32 bytes, simplify lines 88-90 to:
Ok(Key::<Aes256Gcm>::from_slice(&key_bytes).into())Apply the same simplification at line 76 in new_with_key().
🤖 Prompt for AI Agents
In src/utils/encryption.rs around lines 76 and 88-90, the key construction uses
a redundant try_from + from_iter pattern after validating the key length;
replace those with the KeyInit::from_slice path by calling
Key::<Aes256Gcm>::from_slice(&key_bytes).into() (i.e., after validating
key_bytes.len() == 32, return Key::<Aes256Gcm>::from_slice(&key_bytes).into()),
applying the same change at both locations to simplify and remove the
unnecessary conversion.
There was a problem hiding this comment.
from_slice is outdated, generates a clippy warn
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
aes-gcm 0.10 from_slice deprecated clippy warning
💡 Result:
I can help — please paste the exact Clippy warning text and the code snippet that triggers it (the aes-gcm version from Cargo.toml if different from 0.10). With the exact warning and code I’ll show the minimal fix and explain why it happened.
🌐 Web query:
aes-gcm Key from_slice vs from_iter KeyInit trait
💡 Result:
Short answer: use Key::from_slice when you already have a &[u8] or [u8;N] of the exact key length and want a borrowed GenericArray/Key (zero-copy borrow/checked length). KeyInit::from_iter (or the KeyInit/FromIterator-style constructors) is the generic trait method for building a key from an iterator of bytes — it consumes an iterator, collects/copies bytes into the key type and is useful in generic code or when you only have an iterator rather than a contiguous slice.
Details and sources:
- aes-gcm examples show Key::from_slice used to get a Key from a byte slice (requires exact length). [1]
- The generic FromIterator/from_iter API explains constructing types from an iterator (what KeyInit/from_iter uses under the hood). [2]
References:
[1] aes-gcm docs (Key::from_slice usage).
[2] Rust core::iter::FromIterator (from_iter).
@MCarlomagno, thank you for clarifying! You're right—if from_slice() triggers clippy warnings in your environment, the current implementation using from_iter is the appropriate approach. The pattern you've implemented (validating length with try_from, then constructing via from_iter) correctly avoids those warnings while ensuring type safety.
The current implementation is good as-is. I'll note this for future reviews.
✏️ Learnings added
Learnt from: MCarlomagno
Repo: OpenZeppelin/openzeppelin-relayer PR: 479
File: src/utils/encryption.rs:88-90
Timestamp: 2025-11-05T12:25:41.692Z
Learning: In the openzeppelin-relayer codebase with aes-gcm 0.10, avoid suggesting Key::from_slice() for aes_gcm::Key construction as it generates clippy warnings. The preferred pattern is to use Key::from_iter() with an iterator, e.g., `Key::<Aes256Gcm>::from_iter(key_array.iter().copied())` after validating the key length.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/utils/auth.rs (1)
18-38: Restore strict API key validation and stop logging secrets.The updated
check_authorization_headerno longer compares the presented token with the expectedSecretString, so any well-formed header now passes. Coupled with logging the raw token at info level, this leaks credentials and lets unauthorized callers straight through. We must reintroduce the equality check and drop the log.-pub fn check_authorization_header(req: &ServiceRequest, _expected_key: &SecretString) -> bool { +pub fn check_authorization_header(req: &ServiceRequest, expected_key: &SecretString) -> bool { @@ - let prefix_len = AUTHORIZATION_HEADER_VALUE_PREFIX.len(); - let token = &key[prefix_len..]; - - if token.is_empty() || token.contains(' ') { + let prefix_len = AUTHORIZATION_HEADER_VALUE_PREFIX.len(); + let token = &key[prefix_len..]; + + if token.is_empty() || token.contains(' ') { return false; } - info!("Token: {}", token); - return true; + let matches_expected = expected_key.as_str(|expected| expected == token); + if !matches_expected { + return false; + } + + true } false }src/repositories/api_key/api_key_redis.rs (1)
151-185: Race condition allows duplicate API key secrets
createdoes aGETon the value index and then unconditionally callsSETinside the pipeline. Two concurrent requests with the same secret will both pass the preflight check, and whichever executes last will overwrite the hash entry. You now end up with two different API key records pointing at the same secret, which is a serious integrity issue for permission enforcement.Please make the claim of the index atomic—e.g. use
SET ... NX(orSETNX) forvalue_index_keyand abort if it returns false, and make sure to clean the index back up if the subsequent pipeline fails, or move the whole sequence into a Lua script. That guarantees only the first request wins and prevents duplicate secrets.(dragonflydb.io)- let value_exists: Option<String> = conn - .get(&value_index_key) - .await - .map_err(|e| self.map_redis_error(e, "create_api_key_value_check"))?; - - if value_exists.is_some() { - return Err(RepositoryError::ConstraintViolation( - "API Key with this value already exists".to_string(), - )); - } + let value_claimed = conn + .set_nx(&value_index_key, &entity.id) + .await + .map_err(|e| self.map_redis_error(e, "create_api_key_value_claim"))?; + + if !value_claimed { + return Err(RepositoryError::ConstraintViolation( + "API Key with this value already exists".to_string(), + )); + } // Use atomic pipeline for consistency let mut pipe = redis::pipe(); pipe.atomic(); pipe.set(&key, json); pipe.sadd(&list_key, &entity.id); - pipe.set(&value_index_key, &entity.id); - - pipe.exec_async(&mut conn) - .await - .map_err(|e| self.map_redis_error(e, "create_api_key"))?; + if let Err(err) = pipe + .exec_async(&mut conn) + .await + .map_err(|e| self.map_redis_error(e, "create_api_key")) + { + let _: Result<(), _> = conn.del(&value_index_key).await; + return Err(err); + }src/api/routes/relayer.rs (1)
394-402: Tests will now fail without providing an API keyBecause the new
#[require_permissions]guard runs before the route body, everyTestRequestintest_routes_are_registeredis rejected with 401/403 unless it includes the API key header. The assertions currently expectingStatusCode::INTERNAL_SERVER_ERRORwill therefore fail. Please inject the same API key value you stored inApiKeyRepoModelinto each request header (or adjust the expectations) so the tests continue to verify that the routes are registered.
🧹 Nitpick comments (2)
macros/src/tests.rs (1)
8-67: Update macro tests to match the tuple-based permission API.The runtime macro now distinguishes only between global string permissions and
(action, param)tuples, yet these fixtures still rely on the deprecated{relayer_id}syntax. They continue to compile because we merely parse the tokens, but they never exercise the new tuple path or the dotted-field extraction we just introduced, so the suite now gives a false sense of safety. Please rewrite the cases to use the tuple form (e.g.#[require_permissions([("relayers:read", "relayer_id")])]) and adjust the parameter-extraction test to mirror the currentsplit('.')logic rather than{param}parsing.- #[require_permissions(["relayers:get:{relayer_id}"])] + #[require_permissions([("relayers:read", "relayer_id")])] ... - #[require_permissions(["relayers:list:all"])] + #[require_permissions(["relayers:read"])]That will ensure the tests cover the actual supported surface instead of stale syntax.
src/api/routes/mod.rs (1)
55-75: Exercise the permission guard rather than the missing-data path.
test_require_permissions_macro_compilescurrently “passes” becauseweb::ThinData<DefaultAppState>cannot be extracted (we never register it), so Actix returns a 500 before the macro-inserted guard even runs. That doesn’t prove the permission code compiles or behaves correctly. Please initialize the app with aThinDataholding a mockDefaultAppState(e.g., viacreate_mock_app_stateor a minimal in-memory repo setup), hit the route without the required permission to assert the expected 401/403, and then add the permission to assert success. That way the test actually covers the permission flow instead of a missing-data panic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.gitignore(1 hunks)Cargo.toml(2 hunks)README.md(1 hunks)macros/Cargo.toml(1 hunks)macros/src/lib.rs(1 hunks)macros/src/tests.rs(1 hunks)openapi.json(34 hunks)src/api/controllers/api_key.rs(8 hunks)src/api/routes/api_keys.rs(1 hunks)src/api/routes/docs/notification_docs.rs(5 hunks)src/api/routes/docs/plugin_docs.rs(2 hunks)src/api/routes/docs/relayer_docs.rs(18 hunks)src/api/routes/docs/signer_docs.rs(5 hunks)src/api/routes/metrics.rs(9 hunks)src/api/routes/mod.rs(1 hunks)src/api/routes/notification.rs(1 hunks)src/api/routes/plugin.rs(1 hunks)src/api/routes/relayer.rs(4 hunks)src/api/routes/signer.rs(1 hunks)src/bootstrap/config_processor.rs(2 hunks)src/constants/mod.rs(1 hunks)src/constants/permissions.rs(1 hunks)src/models/api_key/repository.rs(3 hunks)src/models/api_key/request.rs(1 hunks)src/models/api_key/response.rs(1 hunks)src/models/mod.rs(1 hunks)src/models/permission.rs(1 hunks)src/openapi.rs(1 hunks)src/repositories/api_key/api_key_in_memory.rs(12 hunks)src/repositories/api_key/api_key_redis.rs(16 hunks)src/repositories/api_key/mod.rs(19 hunks)src/services/turnkey/mod.rs(2 hunks)src/utils/auth.rs(4 hunks)src/utils/encryption.rs(4 hunks)src/utils/mocks.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T11:19:51.397Z
Learnt from: LuisUrrutia
Repo: OpenZeppelin/openzeppelin-relayer PR: 427
File: src/constants/network_tags.rs:5-6
Timestamp: 2025-08-28T11:19:51.397Z
Learning: In the OpenZeppelin relayer codebase, when deprecating constants that are still used internally (like OPTIMISM_TAG in LACKS_MEMPOOL_TAGS), use doc comments instead of #[deprecated] attribute to avoid clippy failures during the transition period.
Applied to files:
src/openapi.rs
🧬 Code graph analysis (18)
macros/src/tests.rs (1)
macros/src/lib.rs (1)
require_permissions(74-215)
src/constants/permissions.rs (1)
src/models/permission.rs (2)
ids(48-53)global(38-45)
macros/src/lib.rs (2)
src/utils/auth.rs (2)
validate_api_key_permissions(54-94)validate_api_key_permissions_scoped(109-150)src/models/app_state.rs (1)
api_key_repository(190-192)
src/utils/auth.rs (2)
src/constants/permissions.rs (2)
has_permission_grant(114-118)has_permission_grant_for_id(131-139)src/models/secret_string.rs (1)
new(30-36)
src/models/api_key/repository.rs (2)
src/models/permission.rs (1)
global(38-45)src/models/api_key/response.rs (2)
try_from(15-22)try_from(37-45)
src/api/routes/metrics.rs (2)
src/metrics/mod.rs (2)
update_system_metrics(111-159)gather_metrics(102-108)macros/src/lib.rs (1)
require_permissions(74-215)
src/repositories/api_key/api_key_redis.rs (4)
src/models/permission.rs (2)
fmt(70-77)global(38-45)src/repositories/api_key/api_key_in_memory.rs (3)
new(36-40)get_by_value(66-75)list_permissions(107-119)src/models/api_key/repository.rs (1)
new(23-31)src/repositories/api_key/mod.rs (4)
get_by_value(41-41)get_by_value(87-92)list_permissions(48-51)list_permissions(101-109)
src/api/routes/signer.rs (2)
macros/src/lib.rs (1)
require_permissions(74-215)src/api/controllers/signer.rs (5)
list_signers(35-62)get_signer(74-93)create_signer(112-137)update_signer(155-174)delete_signer(192-231)
src/api/routes/mod.rs (1)
macros/src/lib.rs (1)
require_permissions(74-215)
src/repositories/api_key/mod.rs (4)
src/repositories/api_key/api_key_in_memory.rs (5)
get_by_value(66-75)create(55-59)count(102-105)list_permissions(107-119)new(36-40)src/repositories/api_key/api_key_redis.rs (6)
get_by_value(272-300)create(135-188)count(372-382)list_permissions(302-314)new(27-41)create_test_api_key(454-462)src/models/api_key/repository.rs (1)
new(23-31)src/models/permission.rs (1)
global(38-45)
src/models/api_key/response.rs (1)
src/models/api_key/repository.rs (1)
try_from(37-45)
src/repositories/api_key/api_key_in_memory.rs (4)
src/repositories/api_key/api_key_redis.rs (3)
get_by_value(272-300)list_permissions(302-314)new(27-41)src/repositories/api_key/mod.rs (4)
get_by_value(41-41)get_by_value(87-92)list_permissions(48-51)list_permissions(101-109)src/models/permission.rs (1)
global(38-45)src/models/api_key/repository.rs (1)
new(23-31)
src/api/controllers/api_key.rs (4)
src/models/api_key/repository.rs (1)
try_from(37-45)src/models/api_key/response.rs (2)
try_from(15-22)try_from(37-45)src/utils/mocks.rs (1)
create_mock_app_state(179-278)src/api/routes/api_keys.rs (1)
delete_api_key(46-52)
src/api/routes/plugin.rs (1)
macros/src/lib.rs (1)
require_permissions(74-215)
src/api/routes/api_keys.rs (2)
macros/src/lib.rs (1)
require_permissions(74-215)src/api/controllers/api_key.rs (4)
list_api_keys(73-106)get_api_key_permissions(115-136)create_api_key(37-59)delete_api_key(145-176)
src/bootstrap/config_processor.rs (1)
src/models/permission.rs (1)
global(38-45)
src/api/routes/notification.rs (2)
macros/src/lib.rs (1)
require_permissions(74-215)src/api/controllers/notification.rs (5)
list_notifications(36-64)get_notification(76-98)create_notification(110-137)update_notification(150-182)delete_notification(200-245)
src/api/routes/relayer.rs (3)
macros/src/lib.rs (1)
require_permissions(74-215)src/api/controllers/relayer.rs (11)
list_relayers(47-75)get_relayer(87-107)send_transaction(417-437)get_transaction_by_id(450-479)get_transaction_by_nonce(492-526)list_transactions(539-573)cancel_transaction(620-639)replace_transaction(653-679)sign_data(692-719)sign_typed_data(732-755)sign_transaction(805-829)src/models/permission.rs (1)
global(38-45)
🪛 Gitleaks (8.28.0)
README.md
[high] 512-514: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (11)
src/utils/encryption.rs (3)
67-68: LGTM! Key and nonce construction correctly adapted to updated API.The migration from
from_slicetofrom_iterwith iterator-based construction is correct. The use oftry_fromfor slice-to-array conversion (lines 88-90, 130-136) provides proper validation before construction, and fixed-size arrays guarantee correct element counts.These changes appear to adapt to
aes_gcmcrate API updates. While mechanically correct, they seem unrelated to the PR's stated objective (API key permissions). Consider clarifying whether this is an incidental dependency update bundled with the PR.Also applies to: 88-90, 99-99, 130-136
104-104: LGTM! Reference passing aligns with updated API.The change to pass
&nonceby reference instead of by value is correct and required by the updatedaes_gcmAPI.Also applies to: 141-141
1-196: The review comment is based on an incorrect premise and should be dismissed.The encryption.rs changes are not unrelated to the PR objectives. This PR implements a comprehensive API key permissions system ("feat: Enforce API key permissions") across multiple interconnected components:
- Permission models & constants:
src/constants/permissions.rs,src/models/permission.rs- Authentication logic:
src/utils/auth.rs(242+ lines of new/modified code)- API key repositories & controllers: Extensive updates to manage API keys with permissions
- Cryptographic support:
src/utils/encryption.rs(25-line migration to aes-gcm 0.10 API)The aes-gcm version update is not a "bundled unrelated dependency"—it's an integral part of the feature that likely supports encryption of sensitive API key data and related cryptographic operations. The changes to encryption.rs (migrating from
from_slice()tofrom_iter()patterns) are necessary adaptations to the updated aes-gcm library API.All modifications are logically cohesive within a single feature branch implementing API key permission enforcement.
Likely an incorrect or invalid review comment.
.gitignore (1)
46-47: LGTM: AGENTS.md added to ignore list.Clean addition to exclude a generated or agent-related file from version control.
src/constants/mod.rs (1)
44-45: LGTM: Permissions module integration.The new permissions module is properly declared and re-exported, following the established pattern in this file.
Cargo.toml (1)
1-2: LGTM: Workspace configuration for procedural macro crate.The workspace setup correctly includes the new
macroscrate, enabling therequire_permissionsprocedural macro to be built and imported as part of the project.Also applies to: 22-22
src/models/mod.rs (1)
56-57: LGTM: Permission module added to public API.The permission module is properly integrated and re-exported, making the
PermissionGranttype and related constructs available throughout the codebase.macros/Cargo.toml (1)
1-12: LGTM: Procedural macro crate properly configured.The crate manifest is correctly set up with
proc-macro = trueand includes the standard dependencies (syn,quote,proc-macro2) needed for building procedural macros.README.md (1)
395-533: Excellent documentation of the permission system.The comprehensive API Key Permissions System section clearly explains the permission structure, action formats, wildcard support, hierarchy, and provides practical examples. The security warning about plugin permissions is particularly important and well-positioned.
Regarding the static analysis warning on Lines 512-514: This is a false positive. The curl example uses
YOUR_API_KEYas a placeholder text for documentation purposes, not an actual secret.src/api/routes/docs/notification_docs.rs (1)
18-23: LGTM: Consistent permission metadata across notification endpoints.The OpenAPI extensions follow a logical pattern:
- Collection operations (
listNotifications,createNotification) use global scope- Resource-specific operations (
getNotification,updateNotification,deleteNotification) use scoped permissions tied tonotification_idThis design aligns well with the permission system's intent to provide fine-grained access control.
Also applies to: 78-84, 148-153, 215-221, 286-292
src/models/api_key/request.rs (1)
1-8: Migration to typed permission model.The change from
Vec<String>toVec<PermissionGrant>represents a breaking change to the API request structure, but this aligns with the PR's objective to introduce a typed, fine-grained permission system. The migration provides better type safety and enables compile-time validation of permission structures.Note: The removal of
allowed_originsmentioned in the summary is not visible in this diff but should be verified in the broader context of the PR changes.
macros/src/lib.rs
Outdated
| // Combine all permission checks | ||
| let combined_checks = quote! { | ||
| #(#permission_checks)* | ||
| }; | ||
|
|
||
| // Parse as a statement and prepend to function body | ||
| let permission_check_stmt: syn::Stmt = syn::parse2(combined_checks).unwrap(); | ||
| input_fn.block.stmts.insert(0, permission_check_stmt); |
There was a problem hiding this comment.
Fix multi-permission statement parsing.
Combining every generated permission check into a single token stream and feeding it to syn::parse2::<Stmt> only works when there is exactly one check. As soon as a caller adds a second permission (for example a global + scoped pair), parse2 rejects the extra tokens and .unwrap() panics, so the macro refuses to compile precisely in the multi-permission scenario we document. Please parse each snippet individually and prepend them in order.
Apply this diff to retain the intended behaviour:
- let combined_checks = quote! {
- #(#permission_checks)*
- };
-
- // Parse as a statement and prepend to function body
- let permission_check_stmt: syn::Stmt = syn::parse2(combined_checks).unwrap();
- input_fn.block.stmts.insert(0, permission_check_stmt);
+ let permission_statements: Vec<syn::Stmt> = permission_checks
+ .into_iter()
+ .map(|check| syn::parse2::<syn::Stmt>(check).expect("Failed to parse permission check"))
+ .collect();
+
+ for stmt in permission_statements.into_iter().rev() {
+ input_fn.block.stmts.insert(0, stmt);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Combine all permission checks | |
| let combined_checks = quote! { | |
| #(#permission_checks)* | |
| }; | |
| // Parse as a statement and prepend to function body | |
| let permission_check_stmt: syn::Stmt = syn::parse2(combined_checks).unwrap(); | |
| input_fn.block.stmts.insert(0, permission_check_stmt); | |
| let permission_statements: Vec<syn::Stmt> = permission_checks | |
| .into_iter() | |
| .map(|check| syn::parse2::<syn::Stmt>(check).expect("Failed to parse permission check")) | |
| .collect(); | |
| for stmt in permission_statements.into_iter().rev() { | |
| input_fn.block.stmts.insert(0, stmt); | |
| } |
🤖 Prompt for AI Agents
In macros/src/lib.rs around lines 201 to 208, the code currently concatenates
all generated permission check token streams and parses them once as a single
syn::Stmt, which panics when there are multiple checks; instead iterate over
each permission_check token stream, parse each one individually with
syn::parse2::<syn::Stmt>(), and insert each parsed statement at the front of
input_fn.block.stmts in such a way that the original permission_checks order is
preserved (e.g., insert parsed statements in reverse iteration so the first
check ends up at index 0). Ensure you handle the parse result the same way the
project prefers (currently using unwrap() is acceptable here if consistent).
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (22.1%) is below the target coverage (90.0%). You can increase the patch coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## main #479 +/- ##
========================================
- Coverage 93.1% 0.4% -92.8%
========================================
Files 241 233 -8
Lines 87849 24652 -63197
========================================
- Hits 81833 102 -81731
- Misses 6016 24550 +18534
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
Thanks for great work!
Left some nit comments!
Regarding plugins, I think we should pass api key to plugins and to rust logic that exposes api to plugins and verify permissions there as it is entry to the system like rest api.
| use quote::quote; | ||
| use syn::{parse_macro_input, Expr, ExprArray, ItemFn}; | ||
|
|
||
| #[cfg(test)] |
| // Get the API key to check its value | ||
| if let Ok(Some(api_key)) = state.api_key_repository.get_by_id(&api_key_id).await { | ||
| let api_key_value = api_key.value.as_str(|s| s.to_string()); | ||
| if api_key_value == env_api_key { |
There was a problem hiding this comment.
Should we also check if it is a global api key (stored in repo model?), and prevent deletions?
There was a problem hiding this comment.
Maybe to check super admin permissions(":") and prevent deletion just in case and handle this case later if needed.
| } | ||
|
|
||
| /// Delete API key | ||
| #[require_permissions([("api_keys:delete", "api_key_id")])] |
There was a problem hiding this comment.
should api key be able to delete itself always? Maybe we can inject that permission by default when creating api keys if not already.
| @@ -0,0 +1,194 @@ | |||
| #[cfg(test)] | |||
There was a problem hiding this comment.
I think we need tests with different api key repo permissions against route required permission.
Right now for macro and routes I do not see those tests.
There was a problem hiding this comment.
Tests should also cover malicious attempts like when url contains query string with relayer id for which api key has permissions but relayer id from path is different one.
| AKR: ApiKeyRepositoryTrait + Send + Sync + 'static, | ||
| { | ||
| let api_key_model = ApiKeyRepoModel::new( | ||
| "default".to_string(), |
There was a problem hiding this comment.
delete api key logic might check this string and prevent deletion.
Use case:
- redis mode is used
- api key got stored
- user restarted instance and provided different value for API_KEY env var.
Current validation would allow deletion in this case as it compares env var.
Not sure how much valid is above case but anyway, it could produce side effects
| // Check permissions with AND logic - ALL permissions must be satisfied | ||
| for required_action in required_actions { | ||
| if !has_permission_grant(&api_key.permissions, required_action) { | ||
| let required_list = required_actions.join(" AND "); |
There was a problem hiding this comment.
Why this joining is needed?
| // Check permissions for the specific resource ID | ||
| for required_action in required_actions { | ||
| if !has_permission_grant_for_id(&api_key.permissions, required_action, resource_id) { | ||
| let required_list = required_actions.join(" AND "); |
| pub name: String, | ||
| pub permissions: Vec<String>, | ||
| pub allowed_origins: Option<Vec<String>>, | ||
| pub permissions: Vec<PermissionGrant>, |
There was a problem hiding this comment.
I think that now it is allowed to create api key with empty permissions.
We should prevent it by adding validations.
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| #[derive(Serialize, Deserialize)] | ||
| pub struct ApiKeyRequest { |
There was a problem hiding this comment.
I think we need to narrow down api key cration logic to prevent use cases like:
- user with scoped api key permissiona + api key create permission can create an api key with higher permissions than it has and take over system.
I guess we can do something like: prevent api key creation that has higher permissions than api key used for creating a new api key.
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| #[derive(Serialize, Deserialize)] | ||
| pub struct ApiKeyRequest { |
There was a problem hiding this comment.
Also maybe we should inject defaults permissions, like api key should be always able to delete itself.
Summary
This PR introduces an API key permissions system with a new proc macro
require_permissionsthat enforces access control on all API endpoints.Examples
Scopes
Permissions use a structured
PermissionGrantmodel with two scopes:Examples
The permission system supports wildcards for flexible access control:
*:*- Super admin (full system access)relayers:*- All actions on relayers resourcetransactions:*- All actions on transactions resourceTesting Process
Start the server with multiple relayers configured (e.g.,
sepolia-exampleandsolana-example).Create an API key with restricted permissions to only access
sepolia-example:Response includes the API key value.
3. Test Authorized Access
Access the permitted relayer (should succeed):
curl -X GET http://localhost:8080/api/v1/relayers/sepolia-example \ -H "Authorization: Bearer SCOPED_API_KEY_VALUE"Try to access a different relayer (should fail with 403 Forbidden):
curl -X GET http://localhost:8080/api/v1/relayers/solana-example \ -H "Authorization: Bearer SCOPED_API_KEY_VALUE"Expected response: { "error": "Insufficient permissions. Required: relayers:read for resource 'solana-example'" }Create an API key with global read access:
This key can access any relayer but cannot perform write operations.
Create a super admin API key:
This key has full system access equivalent to the environment API key.
Impact in
openzeppelin-relayer-sdkNo impact :) the setup will work exactly as the setup using the default key:
Checklist
Summary by CodeRabbit
New Features
Documentation