Feat/secrets #773
Feat/secrets #773
Conversation
sauraww
force-pushed
the
feat/secrets
branch
3 times, most recently
from
8a384d0 to
64cfe56
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request introduces a complete secrets management system with AES-256-GCM encryption. It adds secrets CRUD operations, workspace encryption key management, master key rotation capabilities, and integrates encryption infrastructure throughout the application via parameter propagation. Includes database schema changes, frontend components, and updated AppState to store encryption keys. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Handler as Secrets Handler
participant DB as Database
participant Encryption as Encryption Utils
participant KMS as KMS/Env
Note over Client,KMS: Secret Creation Flow
Client->>Handler: POST /secrets {name, value, description, change_reason}
Handler->>KMS: (via AppState) Retrieve workspace encryption key
KMS-->>Handler: master_key
Handler->>Encryption: encrypt_secret(value, workspace_key)
Encryption-->>Handler: encrypted_value (base64)
Handler->>DB: Insert Secret {name, encrypted_value, metadata...}
DB-->>Handler: Success
Handler->>Client: SecretResponse {name, value: plaintext, ...} (unmasked on create)
Note over Client,KMS: Secret Retrieval & Masking
Client->>Handler: GET /secrets/{name}
Handler->>DB: Fetch Secret by name
DB-->>Handler: Secret {encrypted_value, key_version, ...}
Handler->>Encryption: decrypt_secret(encrypted_value, workspace_key)
Encryption-->>Handler: plaintext_value
Handler->>Client: SecretResponse {name, value: MASKED, ...}
sequenceDiagram
participant Client
participant Handler as Key Rotation Handler
participant DB as Database
participant Encryption as Encryption Utils
participant KMS as KMS/Env
Note over Client,KMS: Workspace Key Rotation
Client->>Handler: POST /secrets/rotate-workspace-key {change_reason}
Handler->>KMS: Retrieve old & new master keys
KMS-->>Handler: old_master_key, new_master_key
Handler->>DB: Start transaction
Handler->>DB: Fetch workspace encryption keys (old encrypted)
DB-->>Handler: encrypted_workspace_key, previous_key
Handler->>Encryption: decrypt_workspace_key(encrypted, old_master_key)
Encryption-->>Handler: workspace_key_plaintext
Handler->>Encryption: Generate new workspace key
Encryption-->>Handler: new_workspace_key
Handler->>Encryption: encrypt_workspace_key(new_key, new_master_key)
Encryption-->>Handler: new_encrypted_key
Handler->>DB: Fetch all secrets for workspace
DB-->>Handler: List<Secret>
loop Each Secret
Handler->>Encryption: decrypt_secret(secret.encrypted_value, workspace_key)
Encryption-->>Handler: plaintext
Handler->>Encryption: encrypt_secret(plaintext, new_workspace_key)
Encryption-->>Handler: new_encrypted_value
Handler->>DB: Update Secret {encrypted_value, key_version++}
end
Handler->>DB: Update Workspace {encryption_key, previous_encryption_key, key_rotation_at}
DB-->>Handler: Commit transaction
Handler->>Client: KeyRotationStatus {success: true, secrets_re_encrypted: N, timestamp}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The PR introduces substantial new functionality (secrets management with encryption) affecting 30+ files across multiple crates with heterogeneous changes. While master_key parameter threading follows a repetitive pattern (lower density), the new encryption module and key rotation logic are intricate. The addition of database schema changes, frontend implementation, and type definitions across multiple layers requires careful review, though many changes follow consistent conventions. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Fix all issues with AI Agents 🤖
In @crates/context_aware_config/src/api/functions/helpers.rs:
- Around line 91-102: The generated JS in generate_secrets_template (and
similarly generate_vars_template) directly interpolates keys/values and thus can
be broken or injected if strings contain quotes, backslashes or newlines; change
these functions to produce a proper JSON string via serde_json (e.g., build a
serde_json::Map or a HashMap of the key/value pairs and call
serde_json::to_string) and then embed that serialized JSON into the JS template
(e.g., format!("const SECRETS = {};", json_string)) so all keys and values are
correctly escaped.
In @crates/frontend/src/pages/function.rs:
- Around line 240-244: In Mode::Editor update the existing Tip component so its
message/code_snippet/example match the new pattern that includes secrets; locate
the Tip inside the Mode::Editor match arm in function.rs and change its message
to reference both variables and secrets, update code_snippet to
"VARS.{VARIABLE_NAME} or SECRETS.{SECRET_NAME}" and example to "VARS.FOO_VAR or
SECRETS.API_KEY" to be consistent with the other Tip.
In @crates/frontend/src/pages/secret.rs:
- Around line 78-81: The code calls use_navigate() inside an async spawn_local
block which can break Leptos' reactive context; hoist the hook to the component
level by moving let navigate = use_navigate(); out of the async closure (keep
the redirect_url construction/use of format! and the navigate(&redirect_url,
Default::default()) call), then capture/clone that navigate variable into the
spawn_local closure so the async block uses the outer navigate instead of
calling use_navigate() internally.
In @crates/frontend/src/pages/secrets_list.rs:
- Around line 185-187: The map closure uses json!(secret).as_object().unwrap()
which can panic if serialization isn't an object; change this to safely handle
non-object results by using as_object().cloned().unwrap_or_default() or
otherwise checking the Option and returning a default/empty map or logging an
error before proceeding; update the code around the map closure that sets
ele_map (the variable created from json!(secret)) so it never calls unwrap on
as_object() and instead falls back to a safe default.
In @crates/service_utils/src/encryption.rs:
- Around line 51-57: The error currently includes the actual key material by
interpolating `key` into the `EncryptionError::InvalidKey` message when checking
`key_bytes.len() != 32`; remove any secret value from logs and change the
message to only state the expected vs actual length (or refer to a non-secret
identifier), e.g. use a message like "Invalid key length: expected 32 bytes, got
X" that uses `key_bytes.len()` but does not include `key`; update the
`EncryptionError::InvalidKey` invocation accordingly and ensure no other
branches log the raw key.
- Around line 218-234: Update the KMS decrypt API and all call sites to return
and handle Result instead of panicking: change the function signature
crate::aws::kms::decrypt(...) -> Result<String, EncryptionError> and replace any
internal unwrap_or_else(|_| panic!()) and other panics with proper Err(...)
returns; then in this module (the match arm that sets decrypted_master_key and
decrypted_previous_master_key), call crate::aws::kms::decrypt(...).await? (or
propagate errors by mapping kms errors into EncryptionError via .map_err(...)?),
so both decrypted_master_key and decrypted_previous_master_key are obtained as
Results and any failure is returned as an EncryptionError rather than causing a
panic. Ensure all other call sites of crate::aws::kms::decrypt are updated
similarly to handle Result.
- Around line 201-217: The DEV/TEST branch of get_master_encryption_keys
currently generates new random keys via generate_encryption_key when
MASTER_ENCRYPTION_KEY or PREVIOUS_MASTER_ENCRYPTION_KEY are not set, risking
data loss on restart; modify this branch to detect when a key was generated (vs
present in env), and (1) emit a clear warning via the service logger that a
transient key is being used and that it will invalidate existing encrypted data,
and (2) optionally fail startup if encrypted secrets are detected (or make this
behavior configurable) instead of silently generating keys; ensure you reference
get_master_encryption_keys, generate_encryption_key, MASTER_ENCRYPTION_KEY,
PREVIOUS_MASTER_ENCRYPTION_KEY and AppEnv::DEV|AppEnv::TEST when implementing
the checks and logging.
In @crates/superposition_types/migrations/2025-12-03-020509_secrets/down.sql:
- Around line 1-4: Down migration currently only drops columns from
superposition.workspaces and omits removing the table and indexes created in the
up migration; add a statement to drop the created table (public.secrets) so the
down migration fully reverts changes — specifically, in the down.sql include a
DROP TABLE IF EXISTS public.secrets; (this will also remove its dependent
indexes idx_secrets_created_at, idx_secrets_last_modified_at, and
idx_secrets_key_version).
In @crates/superposition_types/src/database/models.rs:
- Around line 283-285: The workspace struct fields encryption_key and
previous_encryption_key currently use plain String and should be treated as
secrets: change their types to a secret/zeroizing wrapper (e.g.,
secrecy::SecretString or zeroize::Zeroizing<String>), ensure they are not
Debug/Display/Serialize by adding or deriving appropriate skips (or implementing
custom serialization/Debug) and avoid including them in logs; update any code
paths using encryption_key/previous_encryption_key (and related key_rotation_at
handling) to access the inner secret via the wrapper's API so keys are zeroized
on drop and never accidentally serialized or printed.
In @crates/superposition_types/src/database/models/others.rs:
- Around line 262-272: The create_secret handler currently returns the plaintext
client-provided value (value: req.value) in its API response; change the
response construction in the create_secret function to mask the secret by
setting value to MASKED_VALUE.to_string() instead of echoing req.value so the
endpoint matches get_secret/list_secrets and avoids exposing plaintext in
logs/traces.
In @crates/superposition_types/src/database/superposition_schema.rs:
- Around line 64-66: The create_workspace path currently persists the plaintext
workspace key; change it to load the master key (from app state or env where
other code reads it), call encrypt_workspace_key(generated_key, master_key) and
store the resulting ciphertext in the encryption_key column (not the plaintext),
mirroring the key rotation flow; update the create_workspace implementation (and
any helpers it calls) to use the same master-key loading logic used by the
rotation code so the stored value is always the encrypted key.
In @tests/src/variables.test.ts:
- Line 394: The tests reference a non-existent enum variant
FunctionTypes.VALIDATION; revert this to the correct enum variant
FunctionTypes.VALUE_VALIDATION in tests (e.g., the occurrence inside
variables.test.ts) or alternatively add a VALIDATION alias to the FunctionTypes
enum if that rename was intended; ensure all test references are
consistent—update variables.test.ts to use FunctionTypes.VALUE_VALIDATION or
update the enum to include VALIDATION and keep VALUE_VALIDATION usages aligned
across other tests (functions.test.ts, dimension.test.ts,
default_config.test.ts).
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/src/variables.test.ts (1)
356-433: Use correct request type and provide required fields.The test is using
ValidateFunctionRequestwhich does not exist in the SDK. The correct type isValueValidationFunctionRequest. Additionally, the SDK expectskeyandvalueto be required fields (not empty strings), andtypeandenvironmentare also required by theValueValidationFunctionRequestinterface.Update the request to use the correct type and structure:
const testCommand = new TestCommand({ workspace_id: ENV.workspace_id, org_id: ENV.org_id, function_name: functionName, stage: "draft", request: { - ValidateFunctionRequest: { + ValueValidationFunctionRequest: { key: "", value: "" }, }, });As a secondary concern, the function signature accepts
keyandvalueparameters but the implementation ignores them entirely, only accessingVARS.API_KEY. Consider either using these parameters in the function or removing them if they are intentionally unused for this test type.crates/context_aware_config/src/api/config/handlers.rs (1)
798-807: Address clippytoo_many_argumentswarning flagged by CI.The
get_resolved_configfunction has 8 parameters, triggering thetoo_many_argumentslint. Consider bundling related parameters into a struct.🔎 Suggested approach: Create a config resolution context struct
// Define near the top of the file or in a types module struct ResolveConfigContext { db_conn: DbConnection, dimension_params: DimensionQuery<QueryMap>, query_filters: superposition_query::Query<ResolveConfigQuery>, workspace_context: WorkspaceContext, app_state: Data<AppState>, } // Then update the handler signature: #[routes] #[get("/resolve")] #[post("/resolve")] async fn get_resolved_config( req: HttpRequest, body: Option<Json<ContextPayload>>, merge_strategy: Header<MergeStrategy>, ctx: ResolveConfigContext, // Requires implementing FromRequest ) -> superposition::Result<HttpResponse> { // ... }Alternatively, you can suppress the lint with
#[allow(clippy::too_many_arguments)]if refactoring is deferred.
🧹 Nitpick comments (21)
crates/service_utils/Cargo.toml (1)
11-11: Addaes-gcmandrandto workspace dependencies for consistency and to resolve version mismatches.Currently,
aes-gcm(0.10) andrand(0.8) are declared directly, while other dependencies likebase64,chrono, andserdeuseworkspace = true. Additionally,randhas inconsistent versions across crates—0.8 here and 0.9.1 insuperposition_core. Moving both to workspace dependencies will ensure consistent versions and follow the established pattern.crates/superposition_types/src/database/models/others.rs (1)
221-253: Consider reducing code duplication between SecretName and VariableName.The
SecretNametype (lines 221-253) is nearly identical toVariableName(lines 169-201), with identical validation logic,TryFrom, andFromimplementations. This duplication could make maintenance harder if validation rules need to change.However, keeping them as separate types provides strong type safety and prevents accidentally mixing secrets and variables at the type level. If you prefer to maintain this separation, the current approach is acceptable.
Alternatively, consider extracting the common validation pattern into a macro or generic wrapper to reduce duplication while maintaining type distinctions.
💡 Example macro approach to reduce duplication
macro_rules! validated_string_wrapper { ($name:ident, $validation:expr) => { #[derive(Debug, Serialize, Deserialize, AsRef, Deref, DerefMut, Clone, PartialEq, Into)] #[serde(try_from = "String")] #[cfg_attr( feature = "diesel_derives", derive(AsExpression, FromSqlRow, TextFromSql, TextToSql) )] #[cfg_attr(feature = "diesel_derives", diesel(sql_type = Text))] pub struct $name(pub String); impl $name { pub fn validate(name: String) -> Result<Self, String> { let name = name.trim(); $validation .match_regex(name) .map(|_| Self(name.to_string())) } } impl TryFrom<String> for $name { type Error = String; fn try_from(value: String) -> Result<Self, Self::Error> { Self::validate(value) } } impl From<&$name> for String { fn from(value: &$name) -> Self { value.0.clone() } } }; } // Usage: validated_string_wrapper!(VariableName, RegexEnum::VariableName); validated_string_wrapper!(SecretName, RegexEnum::VariableName);crates/superposition/src/app_state.rs (1)
32-33: Consider validating master key parameters.The
master_keyandprevious_master_keyparameters are accepted as&strand converted directly toStringwithout any validation. Consider adding checks to ensure:
- Keys are not empty strings
- Keys meet minimum length/format requirements (if applicable)
- Keys are properly encoded (e.g., base64) if they're binary data
This would help catch configuration errors early rather than failing later during cryptographic operations.
💡 Example validation approach
pub async fn get( app_env: AppEnv, port: u16, kms_client: &Option<aws_sdk_kms::Client>, service_prefix: String, base: &str, master_key: &str, previous_master_key: &str, ) -> AppState { // Validate keys are not empty if master_key.is_empty() { panic!("master_key cannot be empty"); } if previous_master_key.is_empty() { log::warn!("previous_master_key is empty, key rotation may not work correctly"); } // Rest of the function...crates/service_utils/src/service/types.rs (1)
55-57: Usesecrecy::Secret<String>for cryptographic key storage to prevent accidental exposure and enable automatic zeroization.While
master_keyandprevious_master_keyare stored as plainStringtypes, the primary risk is that strings are not automatically zeroized from memory when dropped. AlthoughAppStatedoes not currently deriveDebug(mitigating one exposure vector), keys stored as plaintext remain vulnerable to:
- Memory dumps that could capture the plaintext key
- Uncontrolled zeroization of key material on deallocation
Consider migrating to
secrecy::Secret<String>to guarantee zeroization and prevent accidental exposure through Display/Debug implementations.crates/frontend/src/pages/workspace.rs (1)
306-320: Consider conditionally rendering the modal only when visible.The modal is currently always rendered in the DOM with visibility controlled by the
visibleprop. For better performance and cleaner DOM, consider conditional rendering:🔎 Proposed optimization
- {move || { - view! { - <KeyRotationModal - visible=show_key_rotation_modal_rs.get() - on_close=move |_| show_key_rotation_modal_ws.set(false) - on_success=move |_| { - logging::log!("Key rotation completed successfully"); - workspace_resource.refetch(); - } - workspace_name=key_rotation_workspace_rs.get() - org_id=org_id.get().0 - /> - } - }} + {move || { + if show_key_rotation_modal_rs.get() { + view! { + <KeyRotationModal + visible=true + on_close=move |_| show_key_rotation_modal_ws.set(false) + on_success=move |_| { + logging::log!("Key rotation completed successfully"); + workspace_resource.refetch(); + } + workspace_name=key_rotation_workspace_rs.get() + org_id=org_id.get().0 + /> + } + .into_view() + } else { + ().into_view() + } + }}crates/frontend/src/pages/secret.rs (1)
19-36: Consider masking or providing copy-to-clipboard for secret values.The secret value is displayed in plain text. Depending on security requirements, consider:
- Masking the value by default with a reveal toggle
- Providing a copy-to-clipboard button instead of direct display
- Adding a warning about sensitive data visibility
This is common practice in secrets management UIs to prevent shoulder-surfing.
crates/context_aware_config/src/api/functions/handlers.rs (1)
48-48: InconsistentAppStateimport style.Line 48 uses
Data<AppState>with the type imported at line 8, but line 279 uses the fully qualified pathData<service_utils::service::types::AppState>. Consider using the importedAppStateconsistently for better readability.🔎 Proposed fix
async fn test( params: Path<TestParam>, request: Json<FunctionExecutionRequest>, db_conn: DbConnection, schema_name: SchemaName, - app_state: Data<service_utils::service::types::AppState>, + app_state: Data<AppState>, ) -> superposition::Result<Json<FunctionExecutionResponse>> {Also applies to: 279-279
crates/frontend/src/pages/secrets_list.rs (1)
165-169: Silent error swallowing may hide API failures from users.Using
unwrap_or_default()silently discards any errors from the secrets fetch. Users won't know if the API call failed vs. if there are genuinely no secrets. Consider logging the error or showing a user-facing error state.crates/frontend/src/components/key_rotation_modal.rs (2)
155-157: Non-reactive value binding may cause stale display.Using
get_untracked()for thevalueprop means theChangeFormwon't reactively update ifchange_reason_rschanges. If the form needs to reflect the current signal value, usechange_reason_rs.get()or pass the signal directly.🔎 Proposed fix
<ChangeForm title="Reason for Key Rotation" placeholder="e.g., Scheduled quarterly rotation, Security incident response, etc." - value=change_reason_rs.get_untracked() + value=change_reason_rs.get() on_change=move |new_reason| { change_reason_ws.set(new_reason) } />
25-31: Consider validating change reason before allowing confirmation.The rotation can proceed with an empty
change_reason. If the backend requires a non-empty reason (viaChangeReason::try_from), the error will only appear after the user clicks "Rotate Key Now". Adding client-side validation or disabling the button when empty would improve UX.crates/frontend/src/pages/secrets_list/filter.rs (2)
200-206: Unusual deserialization pattern for comma-separated values.Using
serde_json::from_value(Value::String(names))to parse comma-separated input relies on a customDeserializeimplementation forCommaSeparatedStringQParams. This works but is non-obvious. Consider adding a brief comment explaining this pattern, or use a more explicit parsing approach.
163-163: Buffer not synced with external filter changes.
filters_buffer_rwsis initialized once withget_untracked(). Iffilters_rwschanges externally (e.g., via URL query parameter updates), the drawer will show stale values when reopened. Consider syncing the buffer when the drawer opens.crates/superposition_types/migrations/2025-12-03-020509_secrets/up.sql (1)
18-20: Consider ifidx_secrets_key_versionindex is needed.The
key_versionindex is useful if you frequently query secrets by version (e.g., during key rotation). However, if key_version is primarily used for decryption logic after fetching by name, this index may add write overhead without significant read benefit.crates/superposition_types/src/api/secrets.rs (1)
98-106: Minor: Inconsistent DateTime type reference.Line 104 uses
chrono::DateTime<chrono::Utc>while other structs use the importedDateTime<Utc>(e.g., lines 79-80, 94). Consider using the imported types consistently.Suggested fix
#[derive(Debug, Clone, Serialize, Deserialize)] pub struct MasterKeyRotationStatus { pub success: bool, pub workspaces_rotated: i64, pub total_workspaces: i64, pub total_secrets_re_encrypted: i64, - pub rotation_timestamp: chrono::DateTime<chrono::Utc>, + pub rotation_timestamp: DateTime<Utc>, pub message: String, }crates/context_aware_config/src/api/secrets/handlers.rs (2)
108-124: Consider extracting the Secret-to-SecretResponse mapping to reduce duplication.The mapping logic from
SecrettoSecretResponsewith masked value is duplicated in two places withinlist_secrets. Consider extracting this to a helper function or using aFrom/Intoimplementation.Suggested refactor
fn secret_to_masked_response(s: Secret) -> SecretResponse { SecretResponse { name: s.name.0, value: MASKED_VALUE.to_string(), key_version: s.key_version, description: s.description, change_reason: s.change_reason, created_at: s.created_at, last_modified_at: s.last_modified_at, created_by: s.created_by, last_modified_by: s.last_modified_by, } }Also applies to: 153-166
327-339: Redundant update before delete.The update to
last_modified_atandlast_modified_by(lines 327-334) is immediately followed by a delete (lines 336-339). Since the record is being deleted, this update has no lasting effect. If this is for audit logging purposes via the trigger, consider adding a comment to clarify the intent.If audit trigger needs the update, add a clarifying comment:
+ // Update last_modified fields before delete to capture the deleting user in audit log diesel::update(secrets) .filter(name.eq(&secret_name)) .set(( last_modified_at.eq(chrono::Utc::now()), last_modified_by.eq(user.get_email()), )) .schema_name(&schema_name) .execute(&mut conn)?;crates/context_aware_config/src/api/secrets/key_rotation.rs (1)
85-87: Consider pagination for large secret counts.Loading all secrets into memory at once could cause memory pressure for workspaces with many secrets. For a key rotation operation, this is likely acceptable since it's an infrequent admin operation, but consider documenting this limitation or adding a warning for large workspaces.
crates/context_aware_config/src/api/default_config/handlers.rs (1)
314-348: Consider moving the import outside the function.The
use crate::api::context::helpers::validate_value_with_function;import on line 331 is inside the function body. While this works, it's unconventional. Consider moving it to the top of the file with other imports unless there's a specific reason (e.g., avoiding circular dependencies).smithy/models/secret.smithy (1)
231-242: Consider adding authorization/access control annotation for key rotation.
RotateWorkspaceKeyis a critical security operation that re-encrypts all workspace secrets. Consider adding an explicit access control annotation or documenting the required permission level.🔎 Example: Add authorization documentation
@documentation("Rotates the workspace encryption key. Generates a new encryption key, re-encrypts all secrets with the new key, and stores the old key as previous_encryption_key for graceful migration. This is a critical operation that should be done during low-traffic periods.") +@documentation("Requires workspace admin or higher privileges.") @http(method: "POST", uri: "/secrets/rotate-key") @tags(["Secrets"]) operation RotateWorkspaceKey {crates/frontend/src/components/secret_form.rs (2)
283-309: Change summary may not provide meaningful diff for updates.When updating a secret, both
old_valuesandnew_valuesshow masked values (sec.valuewhich is already masked, and"••••••••"). This provides no useful visual diff for the user since both values appear identical or similarly masked.Consider showing the description or other metadata changes instead, or clarifying in the UI that the value will be replaced (without showing actual values).
🔎 Alternative: Show description diff or clearer messaging
ChangeType::Update(update_request) => { ( Map::from_iter( vec![ - ("Value".to_string(), Value::String(sec.value.clone())), + ("Description".to_string(), Value::String(sec.description.clone())), + ("Value".to_string(), Value::String("[current encrypted value]".to_string())), ], ), Map::from_iter( vec![ - ( - "Value".to_string(), - Value::String("••••••••".to_string()), - ), + ("Description".to_string(), Value::String(update_request.description.clone().map(|d| d.to_string()).unwrap_or_else(|| sec.description.clone()))), + ("Value".to_string(), Value::String("[will be updated]".to_string())), ], ), ) }
268-279: Consider moving Effect outside of the render closure.The
Effect::newinside the Suspense fallback works but can lead to the effect running multiple times. Consider restructuring to separate the side effect from the render logic.🔎 Alternative: Use create_effect at component level
// Move this outside the view! macro, after create_local_resource create_effect(move |_| { if let Some(Ok(_)) = secret.get() { disabled_rws.set(false); } else if let Some(Err(e)) = secret.get() { logging::error!("Error fetching secret: {}", e); } });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
crates/context_aware_config/src/api.rscrates/context_aware_config/src/api/config/handlers.rscrates/context_aware_config/src/api/context/handlers.rscrates/context_aware_config/src/api/context/helpers.rscrates/context_aware_config/src/api/context/operations.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/handlers.rscrates/context_aware_config/src/api/functions/handlers.rscrates/context_aware_config/src/api/functions/helpers.rscrates/context_aware_config/src/api/secrets.rscrates/context_aware_config/src/api/secrets/handlers.rscrates/context_aware_config/src/api/secrets/key_rotation.rscrates/context_aware_config/src/api/type_templates/handlers.rscrates/context_aware_config/src/helpers.rscrates/context_aware_config/src/validation_functions.rscrates/frontend/src/api.rscrates/frontend/src/app.rscrates/frontend/src/components.rscrates/frontend/src/components/key_rotation_modal.rscrates/frontend/src/components/secret_form.rscrates/frontend/src/components/side_nav.rscrates/frontend/src/pages.rscrates/frontend/src/pages/function.rscrates/frontend/src/pages/secret.rscrates/frontend/src/pages/secrets_list.rscrates/frontend/src/pages/secrets_list/filter.rscrates/frontend/src/pages/workspace.rscrates/service_utils/Cargo.tomlcrates/service_utils/src/encryption.rscrates/service_utils/src/lib.rscrates/service_utils/src/middlewares/tenant.rscrates/service_utils/src/service/types.rscrates/superposition/src/app_state.rscrates/superposition/src/main.rscrates/superposition/src/webhooks/handlers.rscrates/superposition/src/workspace/handlers.rscrates/superposition_types/migrations/2025-12-03-020509_secrets/down.sqlcrates/superposition_types/migrations/2025-12-03-020509_secrets/up.sqlcrates/superposition_types/src/api.rscrates/superposition_types/src/api/secrets.rscrates/superposition_types/src/database/models.rscrates/superposition_types/src/database/models/others.rscrates/superposition_types/src/database/schema.rscrates/superposition_types/src/database/superposition_schema.rssmithy/models/main.smithysmithy/models/secret.smithysuperposition.sqltests/src/variables.test.tsworkspace_template.sql
🧰 Additional context used
🧬 Code graph analysis (25)
crates/frontend/src/pages.rs (2)
crates/frontend/src/pages/secret.rs (1)
secret(46-176)crates/frontend/src/pages/secrets_list.rs (1)
secrets_list(138-264)
crates/context_aware_config/src/api.rs (1)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
secrets(241-244)
crates/superposition_types/src/database/models/others.rs (2)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
secrets(241-244)crates/context_aware_config/src/api/variables/handlers.rs (1)
variables(135-138)
crates/context_aware_config/src/api/secrets.rs (2)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
endpoints(24-33)crates/superposition/src/workspace/handlers.rs (1)
endpoints(61-68)
crates/superposition_types/src/api.rs (1)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
secrets(241-244)
crates/context_aware_config/src/api/functions/handlers.rs (4)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(489-513)crates/service_utils/src/middlewares/tenant.rs (1)
req(78-78)crates/service_utils/src/service/types.rs (5)
req(84-84)req(110-110)req(130-130)req(154-154)req(173-173)crates/context_aware_config/src/validation_functions.rs (2)
execute_fn(192-252)stdout(230-230)
crates/context_aware_config/src/api/context/operations.rs (1)
crates/context_aware_config/src/api/context/helpers.rs (2)
create_ctx_from_put_req(367-416)validate_override_with_functions(230-275)
crates/context_aware_config/src/api/secrets/handlers.rs (2)
crates/service_utils/src/encryption.rs (2)
decrypt_workspace_key(194-199)encrypt_secret(46-75)crates/context_aware_config/src/helpers.rs (1)
workspaces(260-262)
crates/superposition/src/webhooks/handlers.rs (3)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(489-513)crates/service_utils/src/middlewares/tenant.rs (1)
req(78-78)crates/service_utils/src/service/types.rs (5)
req(84-84)req(110-110)req(130-130)req(154-154)req(173-173)
crates/frontend/src/pages/secrets_list.rs (4)
crates/frontend/src/components/table/types.rs (2)
default_column_formatter(53-57)default_with_sort(93-102)crates/frontend/src/query_updater.rs (2)
use_param_updater(17-63)use_signal_from_query(75-89)crates/superposition_types/src/lib.rs (1)
flip(233-238)crates/superposition_types/src/custom_query.rs (1)
reset_page(253-259)
crates/frontend/src/app.rs (2)
crates/frontend/src/pages/secret.rs (1)
secret(46-176)crates/frontend/src/pages/secrets_list.rs (1)
secrets_list(138-264)
crates/context_aware_config/src/api/context/handlers.rs (3)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(489-513)crates/service_utils/src/middlewares/tenant.rs (1)
req(78-78)crates/context_aware_config/src/api/context/helpers.rs (1)
validate_ctx(492-535)
crates/service_utils/src/encryption.rs (1)
crates/service_utils/src/aws/kms.rs (1)
decrypt(5-27)
crates/context_aware_config/src/api/secrets/key_rotation.rs (1)
crates/service_utils/src/encryption.rs (5)
decrypt_with_fallback(116-137)decrypt_workspace_key(194-199)encrypt_secret(46-75)encrypt_workspace_key(187-192)generate_encryption_key(40-44)
crates/context_aware_config/src/api/type_templates/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(489-513)
crates/context_aware_config/src/api/dimension/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(489-513)
crates/frontend/src/pages/secrets_list/filter.rs (4)
crates/frontend/src/components/badge.rs (1)
badge(12-57)crates/frontend/src/components/button.rs (1)
button(13-61)crates/frontend/src/components/drawer.rs (2)
drawer(53-88)close_drawer(17-24)crates/frontend/src/utils.rs (1)
f(401-401)
crates/superposition/src/workspace/handlers.rs (1)
crates/service_utils/src/encryption.rs (1)
generate_encryption_key(40-44)
crates/frontend/src/pages/workspace.rs (1)
crates/frontend/src/components/key_rotation_modal.rs (1)
key_rotation_modal(11-199)
crates/context_aware_config/src/validation_functions.rs (1)
crates/context_aware_config/src/api/functions/helpers.rs (2)
functions(29-32)inject_secrets_and_variables_into_code(163-191)
crates/context_aware_config/src/api/default_config/handlers.rs (4)
crates/context_aware_config/src/api/functions/helpers.rs (2)
functions(29-32)get_published_function_code(35-46)crates/context_aware_config/src/helpers.rs (7)
functions(395-399)validate_change_reason(489-513)state(283-285)state(287-289)state(297-299)state(302-304)put_config_in_redis(267-307)crates/context_aware_config/src/api/context/validations.rs (4)
e(49-49)e(169-169)e(186-186)e(203-203)crates/context_aware_config/src/api/context/helpers.rs (1)
validate_value_with_function(310-346)
crates/context_aware_config/src/api/context/helpers.rs (1)
crates/context_aware_config/src/validation_functions.rs (1)
execute_fn(192-252)
crates/superposition_types/src/database/schema.rs (1)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
secrets(241-244)
crates/superposition/src/main.rs (3)
crates/service_utils/src/encryption.rs (1)
get_master_encryption_keys(201-234)crates/context_aware_config/src/api/secrets/handlers.rs (2)
secrets(241-244)endpoints(24-33)crates/context_aware_config/src/api/secrets.rs (1)
endpoints(6-8)
crates/context_aware_config/src/api/config/handlers.rs (3)
crates/context_aware_config/src/api/dimension/handlers.rs (4)
get(243-262)conn(156-157)conn(355-356)conn(523-523)crates/superposition/src/app_state.rs (1)
get(26-139)crates/context_aware_config/src/helpers.rs (1)
evaluate_remote_cohorts(444-487)
🪛 GitHub Actions: CI Checks on PRs
crates/context_aware_config/src/api/config/handlers.rs
[error] 798-807: clippy: too_many_arguments. The function get_resolved_config has too many arguments (8) for this lint level. Consider refactoring to reduce the number of parameters. See clippy warning: 'too_many_arguments'.
| pub fn generate_secrets_template(secrets: &[(String, String)]) -> String { | ||
| if secrets.is_empty() { | ||
| return String::new(); | ||
| } | ||
|
|
||
| let json_entries: Vec<String> = secrets | ||
| .iter() | ||
| .map(|(k, v)| format!("\"{}\":\"{}\"", k, v)) | ||
| .collect(); | ||
|
|
||
| format!("const SECRETS = {{\n{}\n}};", json_entries.join(",\n")) | ||
| } |
There was a problem hiding this comment.
Potential injection vulnerability: secret values are not escaped.
If a secret value contains ", \, or newlines, the generated JavaScript will be malformed or could lead to code injection. The same issue exists in generate_vars_template (line 78-89).
Consider using serde_json::to_string for proper JSON escaping:
🔎 Proposed fix using proper JSON serialization
pub fn generate_secrets_template(secrets: &[(String, String)]) -> String {
if secrets.is_empty() {
return String::new();
}
- let json_entries: Vec<String> = secrets
- .iter()
- .map(|(k, v)| format!("\"{}\":\"{}\"", k, v))
- .collect();
-
- format!("const SECRETS = {{\n{}\n}};", json_entries.join(",\n"))
+ let map: serde_json::Map<String, serde_json::Value> = secrets
+ .iter()
+ .map(|(k, v)| (k.clone(), serde_json::Value::String(v.clone())))
+ .collect();
+ let json = serde_json::to_string_pretty(&map).unwrap_or_default();
+ format!("const SECRETS = {};", json)
}🤖 Prompt for AI Agents
In @crates/context_aware_config/src/api/functions/helpers.rs around lines
91-102, The generated JS in generate_secrets_template (and similarly
generate_vars_template) directly interpolates keys/values and thus can be broken
or injected if strings contain quotes, backslashes or newlines; change these
functions to produce a proper JSON string via serde_json (e.g., build a
serde_json::Map or a HashMap of the key/value pairs and call
serde_json::to_string) and then embed that serialized JSON into the JS template
(e.g., format!("const SECRETS = {};", json_string)) so all keys and values are
correctly escaped.
| let navigate = use_navigate(); | ||
| let redirect_url = | ||
| format!("/admin/{}/{}/secrets", org.get().0, workspace.get().0,); | ||
| navigate(&redirect_url, Default::default()); |
There was a problem hiding this comment.
use_navigate() called inside async block may cause issues.
In Leptos, use_navigate() should typically be called at the component level, not inside an async spawn_local block. The hook may not have access to the correct reactive context when called asynchronously.
🔎 Proposed fix: hoist use_navigate to component level
#[component]
pub fn secret() -> impl IntoView {
let workspace = use_context::<Signal<Tenant>>().unwrap();
let org = use_context::<Signal<OrganisationId>>().unwrap();
let params = use_params_map();
+ let navigate = use_navigate();
let secret_name = Signal::derive(move || {
params.with(|p| p.get("secret_name").cloned().unwrap_or_default())
});
// ... rest of component ...
let on_delete_confirm = move |_| {
delete_inprogress_rws.set(true);
let sec_name = secret_name.get_untracked();
+ let nav = navigate.clone();
spawn_local(async move {
let result = secrets::delete(
sec_name,
&workspace.get_untracked().0,
&org.get_untracked().0,
)
.await;
delete_inprogress_rws.set(false);
match result {
Ok(_) => {
logging::log!("Secret deleted successfully");
- let navigate = use_navigate();
let redirect_url =
format!("/admin/{}/{}/secrets", org.get().0, workspace.get().0,);
- navigate(&redirect_url, Default::default());
+ nav(&redirect_url, Default::default());
enqueue_alert(
String::from("Secret deleted successfully"),
AlertType::Success,
5000,
);
}
// ...
}
});
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @crates/frontend/src/pages/secret.rs around lines 78-81, The code calls
use_navigate() inside an async spawn_local block which can break Leptos'
reactive context; hoist the hook to the component level by moving let navigate =
use_navigate(); out of the async closure (keep the redirect_url construction/use
of format! and the navigate(&redirect_url, Default::default()) call), then
capture/clone that navigate variable into the spawn_local closure so the async
block uses the outer navigate instead of calling use_navigate() internally.
| .map(|secret| { | ||
| let mut ele_map = json!(secret).as_object().unwrap().to_owned(); | ||
| ele_map |
There was a problem hiding this comment.
Potential panic if serialization returns non-object.
json!(secret).as_object().unwrap() will panic if the serialization somehow doesn't produce an object. While unlikely for a well-defined struct, defensive coding with unwrap_or_default() or proper error handling would be safer.
🔎 Proposed fix
.map(|secret| {
- let mut ele_map = json!(secret).as_object().unwrap().to_owned();
+ let mut ele_map = json!(secret)
+ .as_object()
+ .cloned()
+ .unwrap_or_default();
ele_map🤖 Prompt for AI Agents
In @crates/frontend/src/pages/secrets_list.rs around lines 185-187, The map
closure uses json!(secret).as_object().unwrap() which can panic if serialization
isn't an object; change this to safely handle non-object results by using
as_object().cloned().unwrap_or_default() or otherwise checking the Option and
returning a default/empty map or logging an error before proceeding; update the
code around the map closure that sets ele_map (the variable created from
json!(secret)) so it never calls unwrap on as_object() and instead falls back to
a safe default.
| if key_bytes.len() != 32 { | ||
| return Err(EncryptionError::InvalidKey(format!( | ||
| "Key {} must be 32 bytes, got {}", | ||
| key, | ||
| key_bytes.len() | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Security: Avoid logging the key value in error messages.
Line 53-54 includes the actual key value in the error message. This could leak sensitive key material to logs if the key is partially valid base64 but wrong length.
Suggested fix
if key_bytes.len() != 32 {
return Err(EncryptionError::InvalidKey(format!(
- "Key {} must be 32 bytes, got {}",
- key,
+ "Key must be 32 bytes, got {}",
key_bytes.len()
)));
}📝 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.
| if key_bytes.len() != 32 { | |
| return Err(EncryptionError::InvalidKey(format!( | |
| "Key {} must be 32 bytes, got {}", | |
| key, | |
| key_bytes.len() | |
| ))); | |
| } | |
| if key_bytes.len() != 32 { | |
| return Err(EncryptionError::InvalidKey(format!( | |
| "Key must be 32 bytes, got {}", | |
| key_bytes.len() | |
| ))); | |
| } |
🤖 Prompt for AI Agents
In @crates/service_utils/src/encryption.rs around lines 51-57, The error
currently includes the actual key material by interpolating `key` into the
`EncryptionError::InvalidKey` message when checking `key_bytes.len() != 32`;
remove any secret value from logs and change the message to only state the
expected vs actual length (or refer to a non-secret identifier), e.g. use a
message like "Invalid key length: expected 32 bytes, got X" that uses
`key_bytes.len()` but does not include `key`; update the
`EncryptionError::InvalidKey` invocation accordingly and ensure no other
branches log the raw key.
| ALTER TABLE superposition.workspaces | ||
| DROP COLUMN IF EXISTS key_rotation_at, | ||
| DROP COLUMN IF EXISTS previous_encryption_key, | ||
| DROP COLUMN IF EXISTS encryption_key; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat crates/superposition_types/migrations/2025-12-03-020509_secrets/up.sqlRepository: juspay/superposition
Length of output: 927
Down migration is incomplete: missing DROP TABLE IF EXISTS public.secrets and dependent indexes.
The up.sql migration creates the public.secrets table along with three indexes (idx_secrets_created_at, idx_secrets_last_modified_at, idx_secrets_key_version). The down.sql only drops the three columns from the superposition.workspaces table but does not drop the public.secrets table or its indexes. This means running the migration up followed by down will not fully restore the database to its original state. Add the following to down.sql:
DROP TABLE IF EXISTS public.secrets;This will also automatically drop the dependent indexes.
🤖 Prompt for AI Agents
In @crates/superposition_types/migrations/2025-12-03-020509_secrets/down.sql
around lines 1-4, Down migration currently only drops columns from
superposition.workspaces and omits removing the table and indexes created in the
up migration; add a statement to drop the created table (public.secrets) so the
down migration fully reverts changes — specifically, in the down.sql include a
DROP TABLE IF EXISTS public.secrets; (this will also remove its dependent
indexes idx_secrets_created_at, idx_secrets_last_modified_at, and
idx_secrets_key_version).
| pub encryption_key: Option<String>, | ||
| pub previous_encryption_key: Option<String>, | ||
| pub key_rotation_at: Option<DateTime<Utc>>, |
There was a problem hiding this comment.
Apply the same security considerations to workspace encryption keys.
Similar to the concern raised for master_key in AppState, the workspace-level encryption_key and previous_encryption_key fields store cryptographic keys as plain String types. These have the same security implications:
- Keys persist in memory without zeroization
- Potential exposure through Debug traits or serialization
- Risk of inclusion in logs or error messages
If these keys are sensitive (which encryption keys typically are), consider using a secure secret type or ensuring they are never logged/serialized in debug contexts.
🤖 Prompt for AI Agents
In @crates/superposition_types/src/database/models.rs around lines 283-285, The
workspace struct fields encryption_key and previous_encryption_key currently use
plain String and should be treated as secrets: change their types to a
secret/zeroizing wrapper (e.g., secrecy::SecretString or
zeroize::Zeroizing<String>), ensure they are not Debug/Display/Serialize by
adding or deriving appropriate skips (or implementing custom
serialization/Debug) and avoid including them in logs; update any code paths
using encryption_key/previous_encryption_key (and related key_rotation_at
handling) to access the inner secret via the wrapper's API so keys are zeroized
on drop and never accidentally serialized or printed.
| pub struct Secret { | ||
| pub name: SecretName, | ||
| pub encrypted_value: String, | ||
| pub key_version: i32, | ||
| pub description: Description, | ||
| pub change_reason: ChangeReason, | ||
| pub created_at: DateTime<Utc>, | ||
| pub last_modified_at: DateTime<Utc>, | ||
| pub created_by: String, | ||
| pub last_modified_by: String, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the Secret struct definition and nearby code
echo "=== Secret struct definition ==="
cat -n crates/superposition_types/src/database/models/others.rs | sed -n '260,280p'
echo -e "\n=== Checking Secret struct instantiation patterns ==="
rg -n "Secret\s*\{" --type=rust -A5 | head -60
echo -e "\n=== Checking for encryption-related functions ==="
rg -n "fn.*encrypt|fn.*decrypt" --type=rust | head -30Repository: juspay/superposition
Length of output: 3182
🏁 Script executed:
#!/bin/bash
# Look for where Secret values are created from requests
echo "=== Checking how Secret is created from API requests ==="
rg -n "Secret\s*\{|Secret::|from_request|from_body" --type=rust -B2 -A8 | head -100
echo -e "\n=== Checking for potential plaintext exposure ==="
rg -n "encrypted_value" --type=rust -B2 -A2 | head -80Repository: juspay/superposition
Length of output: 13234
🏁 Script executed:
#!/bin/bash
# Search for database insert/update patterns
echo "=== Checking database operations with Secret ==="
rg -n "insert.*Secret|update.*Secret|save.*Secret" --type=rust -B2 -A4 | head -80
echo -e "\n=== Checking API response serialization ==="
rg -n "Secret.*serialize|serde.*Secret|json.*Secret" --type=rust -B1 -A3 | head -60Repository: juspay/superposition
Length of output: 762
🏁 Script executed:
#!/bin/bash
# Examine the create_secret handler in detail
echo "=== create_secret handler implementation ==="
rg -n "fn create_secret|pub async fn create" crates/context_aware_config/src/api/secrets/handlers.rs -A30 | head -80
echo -e "\n=== Checking how Secret is inserted into database ==="
rg -n "diesel::insert_into|\.insert(" crates/context_aware_config/src/api/secrets/handlers.rs -B3 -A3
echo -e "\n=== Looking at CreateSecretRequest structure ==="
rg -n "struct CreateSecretRequest|pub struct.*Secret.*Request" crates/superposition_types/src/api/secrets.rs -A10 | head -40Repository: juspay/superposition
Length of output: 2141
🏁 Script executed:
#!/bin/bash
# Check where plaintext arrives in the request
echo "=== Checking CreateSecretRequest and how it's used ==="
cat -n crates/superposition_types/src/api/secrets.rs | grep -A15 "CreateSecretRequest"
echo -e "\n=== Full create_secret handler ==="
sed -n '180,240p' crates/context_aware_config/src/api/secrets/handlers.rsRepository: juspay/superposition
Length of output: 2678
🏁 Script executed:
#!/bin/bash
# Check API response serialization
echo "=== Checking what Secret endpoints return ==="
rg -n "fn create_secret|fn get_secret|fn list_secret" crates/context_aware_config/src/api/secrets/handlers.rs -A1 | head -20
echo -e "\n=== Checking the return types and serialization ==="
rg -n "Ok\(.*Secret\)" crates/context_aware_config/src/api/secrets/handlers.rs -B2 -A2 | head -60
echo -e "\n=== Checking if Secret derives Serialize ==="
rg -n "derive.*Serialize|#\[serde" crates/superposition_types/src/database/models/others.rs -B2 -A5 | head -40Repository: juspay/superposition
Length of output: 1714
🏁 Script executed:
#!/bin/bash
# Find SecretResponse definition
echo "=== SecretResponse structure ==="
rg -n "struct SecretResponse|pub struct SecretResponse" --type=rust -A12
echo -e "\n=== Full create_secret return ==="
sed -n '218,230p' crates/context_aware_config/src/api/secrets/handlers.rs
echo -e "\n=== Check get_secret and update_secret handlers ==="
sed -n '232,290p' crates/context_aware_config/src/api/secrets/handlers.rsRepository: juspay/superposition
Length of output: 3605
🏁 Script executed:
#!/bin/bash
# Check if plaintext is ever logged
echo "=== Searching for logging of secret values ==="
rg -n "log::|tracing::|debug!|info!|warn!" crates/context_aware_config/src/api/secrets/handlers.rs -B1 -A1
echo -e "\n=== Checking encryption function implementation ==="
sed -n '46,75p' crates/service_utils/src/encryption.rs
echo -e "\n=== Checking for plaintext secret exposure in logs ==="
rg -n "req\.value|encrypted_secret_value.*log|plaintext.*log" crates/context_aware_config/src/api/secrets/ --type=rustRepository: juspay/superposition
Length of output: 1965
🏁 Script executed:
#!/bin/bash
# Check list_secrets endpoint
echo "=== list_secrets handler ==="
sed -n '80,130p' crates/context_aware_config/src/api/secrets/handlers.rsRepository: juspay/superposition
Length of output: 1958
Fix plaintext secret exposure in create_secret API response.
The create_secret endpoint returns the plaintext secret value (value: req.value) in its response, which is inconsistent with get_secret that masks the value using MASKED_VALUE. While the plaintext was sent by the client in the request, echoing it back in the API response creates unnecessary exposure in API logs, network traces, and browser history.
The encryption flow is correctly implemented at the API boundary (plaintext is encrypted immediately before storing), and decryption is properly restricted to internal operations. However, the response should mask the secret value like get_secret and list_secrets do.
Update line 220 in crates/context_aware_config/src/api/secrets/handlers.rs from value: req.value to value: MASKED_VALUE.to_string().
🤖 Prompt for AI Agents
In @crates/superposition_types/src/database/models/others.rs around lines
262-272, The create_secret handler currently returns the plaintext
client-provided value (value: req.value) in its API response; change the
response construction in the create_secret function to mask the secret by
setting value to MASKED_VALUE.to_string() instead of echoing req.value so the
endpoint matches get_secret/list_secrets and avoids exposing plaintext in
logs/traces.
| encryption_key -> Nullable<Text>, | ||
| previous_encryption_key -> Nullable<Text>, | ||
| key_rotation_at -> Nullable<Timestamptz>, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify workspace keys are encrypted before storage and decrypted on retrieval
# Search for encryption of workspace keys before DB insertion
echo "=== Checking workspace key encryption before storage ==="
rg -nP -C5 'encryption_key.*encrypt|encrypt.*encryption_key' --type rust
# Search for decryption of workspace keys after DB retrieval
echo -e "\n=== Checking workspace key decryption after retrieval ==="
rg -nP -C5 'encryption_key.*decrypt|decrypt.*encryption_key' --type rust
# Verify master key is loaded from environment, not database
echo -e "\n=== Checking master key source (should be env, not DB) ==="
rg -nP -C3 'master_key.*env|MASTER_KEY|master.*key.*=.*env' --type rustRepository: juspay/superposition
Length of output: 12937
🏁 Script executed:
#!/bin/bash
# Check where encrypt_workspace_key is called and how workspace keys are stored
echo "=== Finding encrypt_workspace_key function definition ==="
rg -nA10 'fn encrypt_workspace_key' --type rust
echo -e "\n=== Checking workspace creation/insert flow ==="
rg -nB5 -A10 'insert.*workspace|create.*workspace' crates/superposition/src/workspace/handlers.rs
echo -e "\n=== Looking for where encryption_key is encrypted in workspace creation ==="
rg -nB10 'workspace.encryption_key\s*=' crates/superposition/src/workspace/handlers.rs | head -30
echo -e "\n=== Check if master key is used during workspace creation ==="
rg -nB5 -A5 'fn create_workspace' crates/superposition/src/workspace/handlers.rs | head -40Repository: juspay/superposition
Length of output: 3752
🏁 Script executed:
#!/bin/bash
# Check if master key is available and used in workspace creation
echo "=== Check full create_workspace function signature and imports ==="
rg -B30 'async fn create_workspace' crates/superposition/src/workspace/handlers.rs | head -50
echo -e "\n=== Check if master key is loaded/passed in create_workspace ==="
rg -A50 'async fn create_workspace' crates/superposition/src/workspace/handlers.rs | grep -i 'master\|encrypt' | head -20
echo -e "\n=== Verify workspace creation inserts plaintext key (not encrypted) ==="
sed -n '87,147p' crates/superposition/src/workspace/handlers.rs
echo -e "\n=== Compare: Check how master key is obtained in key_rotation.rs ==="
rg -B10 'rotate_workspace_encryption_key_internal' crates/context_aware_config/src/api/secrets/key_rotation.rs | grep -A10 'fn rotate_workspace'Repository: juspay/superposition
Length of output: 4332
Fix critical security flaw: Workspace encryption keys are stored in plaintext in the database during workspace creation.
The create_workspace function generates an encryption key and stores it directly in the database without encrypting it with the master key (lines 102, 130). This exposes the workspace key material in the database, defeating the purpose of the master key encryption scheme.
In contrast, the key rotation logic properly encrypts keys before storage using encrypt_workspace_key(key, master_key). To maintain consistent security:
- Load the master key in
create_workspace(available via app state or environment variable) - Encrypt the generated key with the master key before storing in the database
- Ensure the encrypted key is stored in the
encryption_keyfield, not the plaintext value
This brings workspace creation in line with the key rotation implementation and ensures workspace key material is protected at rest.
🤖 Prompt for AI Agents
In @crates/superposition_types/src/database/superposition_schema.rs around lines
64-66, The create_workspace path currently persists the plaintext workspace key;
change it to load the master key (from app state or env where other code reads
it), call encrypt_workspace_key(generated_key, master_key) and store the
resulting ciphertext in the encryption_key column (not the plaintext), mirroring
the key rotation flow; update the create_workspace implementation (and any
helpers it calls) to use the same master-key loading logic used by the rotation
code so the stored value is always the encrypted key.
tests/src/variables.test.ts
Outdated
| change_reason: "Testing variable integration", | ||
| runtime_version: "1", | ||
| function_type: FunctionTypes.VALUE_VALIDATION, | ||
| function_type: FunctionTypes.VALIDATION, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'VALUE_VALIDATION' --type=ts --type=js -g '!node_modules' -g '!dist' -g '!build'Repository: juspay/superposition
Length of output: 2225
🏁 Script executed:
# Search for FunctionTypes enum definition and all VALIDATION references
rg -n 'FunctionTypes\s*=' -A 20 --type=ts --type=js -g '!node_modules' -g '!dist' -g '!build'Repository: juspay/superposition
Length of output: 2351
🏁 Script executed:
# Examine the test file around the changed lines
sed -n '385,420p' tests/src/variables.test.tsRepository: juspay/superposition
Length of output: 1523
🏁 Script executed:
# Search for other uses of FunctionTypes.VALIDATION to see if enum change was applied consistently
rg -n 'FunctionTypes\.VALIDATION' --type=ts --type=js -g '!node_modules' -g '!dist' -g '!build'Repository: juspay/superposition
Length of output: 152
Revert the enum variant - FunctionTypes.VALIDATION does not exist.
The enum variant FunctionTypes.VALIDATION referenced at line 394 does not exist in the FunctionTypes enum definition. The actual enum contains VALUE_VALIDATION, not VALIDATION. This change will cause a type error at runtime or during compilation.
Additionally, multiple test files still reference VALUE_VALIDATION:
- tests/src/functions.test.ts (3 times)
- tests/src/dimension.test.ts (1 time)
- tests/src/default_config.test.ts (2 times)
Either revert this change to use FunctionTypes.VALUE_VALIDATION or update the enum definition to include the VALIDATION variant if that's the intended refactoring.
🤖 Prompt for AI Agents
In @tests/src/variables.test.ts around line 394, The tests reference a
non-existent enum variant FunctionTypes.VALIDATION; revert this to the correct
enum variant FunctionTypes.VALUE_VALIDATION in tests (e.g., the occurrence
inside variables.test.ts) or alternatively add a VALIDATION alias to the
FunctionTypes enum if that rename was intended; ensure all test references are
consistent—update variables.test.ts to use FunctionTypes.VALUE_VALIDATION or
update the enum to include VALIDATION and keep VALUE_VALIDATION usages aligned
across other tests (functions.test.ts, dimension.test.ts,
default_config.test.ts).
Implement the foundational layer for workspace-level secrets management with AES-256-GCM encryption and support for encryption key rotation. Database Changes: - Add secrets table to workspace_template.sql with key_version tracking - Add encryption_key, previous_encryption_key, and key_rotation_at columns to workspaces table - Create migration files for workspace encryption fields Encryption: - Implement AES-256-GCM encryption module with key generation - Add encrypt_secret() and decrypt_secret() functions - Add decrypt_with_fallback() for graceful key rotation support - Include comprehensive unit tests for encryption operations Data Models: - Add Secret model with encrypted_value and key_version fields - Add SecretName type with validation - Update Workspace model with encryption key management fields - Update database schema definitions for secrets table API Types: - Add secrets API module with request/response types - Implement SecretFilters, CreateSecretRequest, UpdateSecretRequest - Add RotateKeyRequest and KeyRotationStatus for key rotation - Add SortOn enum for secrets listing Dependencies: - Add aes-gcm 0.10 for AES-256-GCM encryption - Add rand 0.8 for cryptographic random number generation Documentation: - Create comprehensive implementation plan in plans/secrets.md - Add implementation summary in plans/secrets-implementation-summary.md This commit establishes the foundation for Phase 2 implementation which will include API handlers, Smithy models, and frontend components. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Fix all issues with AI Agents 🤖
In @crates/context_aware_config/src/api/config/handlers.rs:
- Around line 630-634: Remove the duplicated attribute by keeping a single
#[allow(clippy::too_many_arguments)] on the handler annotated with #[authorized]
#[routes] #[get("/resolve")]; locate the attribute pair surrounding that resolve
handler and delete the extra #[allow(clippy::too_many_arguments)] so only one
remains, and run a quick search to ensure the same attribute isn't accidentally
duplicated elsewhere in the same module.
In @crates/context_aware_config/src/api/secrets/handlers.rs:
- Around line 218-228: The create_secret handler currently returns the plaintext
secret in SecretResponse.value which will be captured by RequestResponseLogger
at TRACE; change the handler (create_secret) to not echo the raw secret (e.g.,
return value: "[REDACTED]" or omit the field) and/or add a sanitizing path-aware
filter to the RequestResponseLogger middleware so it redacts the "value" field
for secret endpoints (or disables body logging for "/secrets"); update related
types/serializers if needed (SecretResponse) and add a brief inline
comment/documentation noting that trace-level logging will not include plaintext
secret values.
In @crates/context_aware_config/src/api/secrets/key_rotation.rs:
- Around line 231-317: The rotate_master_key handler lacks an authorization
guard; before loading workspaces or starting the DB transaction in
rotate_master_key, verify the invoking User is a system administrator (e.g.,
call a method like user.is_admin() or user.has_role("admin") /
user.is_system_admin()) and if not return an appropriate HTTP Forbidden/AppError
response and log the denied attempt; place this check at the top of
rotate_master_key (using the User argument) so no DB reads or key operations
occur for unauthorized callers.
- Around line 176-229: The rotate_encryption_key handler (function
rotate_encryption_key) is missing an authorization check; add the appropriate
authorization attribute or explicit guard so only admins/privileged users can
invoke it (e.g. add #[authorized] above the function or call the existing
authorization helper such as authorize_workspace_admin(&user, &workspace) at the
top of rotate_encryption_key before performing the rotation), returning a proper
error on unauthorized access; also audit other sensitive handlers in this module
for the same pattern and apply the same attribute or helper where needed.
- Around line 264-266: Replace the local derivation of schema_name (currently
using format!("{}_{}", workspace.organisation_id, workspace.workspace_name))
with the precomputed value on the Workspace struct: use
workspace.workspace_schema_name (borrow or clone as needed) for schema_name to
ensure consistency; update the assignment of schema_name and any downstream
usages to rely on that field instead of recomputing it.
In @crates/frontend/src/api.rs:
- Around line 1038-1044: The secrets API call is using the header key "x-tenant"
which is inconsistent with the rest of the frontend (which uses "x-workspace");
update the header creation in the secrets-related request to use "x-workspace"
instead of "x-tenant" by changing the tuple passed to construct_request_headers
(the call shown with request(... construct_request_headers(&[("x-tenant",
tenant), ("x-org-id", org_id)])?)). Search within the secrets module for other
uses of "x-tenant" and replace them with "x-workspace" so all calls (including
those invoking request and construct_request_headers) are standardized while
keeping the same variable (tenant) value name if desired.
In @crates/frontend/src/components/secret_form.rs:
- Line 23: The import crate::types::Tenant is missing because Tenant is not
defined; either remove Tenant from the use list in secret_form.rs (and
corresponding imports in pages/secret.rs and pages/secrets_list.rs) or add a
proper Tenant type to the types module; to fix, decide whether Tenant should be
an alias (e.g., type Tenant = ...), a struct, or an enum, then implement that
definition in the types module with the expected fields/traits used by
SecretForm/Secret pages (or remove all references and update SecretForm methods
that reference Tenant), and update the use statements to import the correct
symbol (e.g., OrganisationId and Tenant) so compilation succeeds.
- Around line 235-240: The code is manually dereferencing tuple fields with .0
when calling create_local_resource; change the tuple to use workspace.get() and
org.get() directly (i.e., (secret_name.clone(), workspace.get(), org.get())) so
the Workspace and OrganisationId Deref coercion supplies &str to secrets::get();
update the closure passed to create_local_resource accordingly (affecting the
call that constructs secret via create_local_resource and the inner async
closure invoking secrets::get).
In @crates/frontend/src/components/side_nav.rs:
- Around line 61-66: The AppRoute entry uses an undefined variable `tenant` in
the key and path; replace both `{tenant}` occurrences with the function
parameter `workspace` so the key and path use `{workspace}` (update the AppRoute
block where key and path are formatted) to resolve the compilation error.
In @crates/frontend/src/pages/secret.rs:
- Around line 57-62: The closure passed to create_blocking_resource for
secret_resource may capture differently sized types; explicitly annotate the
async closure parameters as String to avoid borrowing/size issues—change the
async move |(sec_name, tenant, org_id)| to accept (sec_name: String, tenant:
String, org_id: String) and then call secrets::get(&sec_name, &tenant,
&org_id).await.ok() so the types are owned Strings inside the async block.
- Line 17: The import line brings in a non-existent type `Tenant` from
`crate::types`; remove `Tenant` from the use statement (`use
crate::types::{OrganisationId, Tenant};` -> `use crate::types::OrganisationId;`)
and update any references to `Tenant` in this file (e.g., in functions, structs,
or signatures) to use the correct existing type from `crate::types` (such as
`OrganisationId` or `TenantId` if that exists) or define the missing type in
`crate::types` if `Tenant` is actually required.
In @crates/frontend/src/pages/secrets_list.rs:
- Line 32: The import of `Tenant` from `crate::types` in the secrets_list module
is unresolved; open the `crate::types` module to confirm the actual symbol name
(e.g., `Workspace`, `TenantId`, or a differently named struct/type) and either
replace `Tenant` with the correct symbol or remove it if unused. Update the `use
crate::types::{OrganisationId, Tenant};` line to the correct binding (for
example `use crate::types::{OrganisationId, Workspace};` or `... TenantId }`)
and, if the desired type truly belongs elsewhere, add or export the correct type
from the `types` module so `secrets_list.rs` can import it. Ensure any code in
`secrets_list.rs` that referenced `Tenant` is renamed to the correct type name
(`Workspace`/`TenantId`) to match the fix.
In @crates/frontend/src/pages/workspace.rs:
- Around line 309-318: The callbacks passed to KeyRotationModal (on_close and
on_success) use a single-argument closure signature (move |_|) but the component
expects Callback<()> (no-arg closures); change both to zero-argument closures
(e.g., use move || { ... }) so the signatures match, keeping the bodies that
call show_key_rotation_modal_ws.set(false), logging::log!("Key rotation
completed successfully"), and workspace_resource.refetch() unchanged.
In @crates/service_utils/Cargo.toml:
- Line 11: Update the vulnerable dependency entries in Cargo.toml: change the
aes-gcm dependency from "0.10" to "0.10.3" (or a later 0.10.x) and bump rand
from "0.8" to "0.9.2"; after editing the aes-gcm and rand dependency lines, run
cargo update to lock the new versions and ensure no transitive constraints
revert them.
In @crates/superposition_types/migrations/2025-12-03-020509_secrets/up.sql:
- Around line 6-16: The secrets table lacks workspace scoping: update the CREATE
TABLE for public.secrets (the secrets table definition) to add workspace_id and
organisation_id columns, make the primary key composite (name, workspace_id,
organisation_id) instead of just name, and add a foreign key constraint
referencing the workspaces table (organisation_id, workspace_name) to enforce
workspace/organisation relationship; ensure the new columns are NOT NULL and
adjust defaults/lengths to match workspaces schema and any code in
key_rotation.rs that assumes schema-based scoping.
In @crates/superposition/src/app_state.rs:
- Around line 135-137: The AppState fields master_key and previous_master_key
are stored as plain Strings; change them to a secure secret type (e.g.,
secrecy::SecretString or zeroize::Zeroizing<String>) in the AppState struct and
all constructors/clone sites so the secret is zeroized on drop and not
accidentally logged; update any code that accesses these values (including
encryption.rs where format!("Key {} ...", key) is used) to call the secret
access method (e.g., secret.expose_secret() or deref the Zeroizing wrapper) and
remove/avoid Debug/Serialize derives or direct formatting of the secret to
prevent accidental printing; ensure kms_client.clone() usage remains unchanged
and update function signatures and tests that accept
master_key/previous_master_key to the new secure type.
In @crates/superposition/src/main.rs:
- Around line 262-266: Add a new Resource variant and wire it into the secrets
scope: add a Secrets variant to the Resource enum (e.g., Resource::Secrets) in
the service types definition, then update the scope("/secrets") registration to
include .app_data(Resource::Secrets) alongside the existing
.wrap(OrgWorkspaceMiddlewareFactory::new(true, true)) and
.service(secrets::endpoints()) calls; this ensures the AuthZHandler sees the
correct resource context for the secrets endpoints.
In @crates/superposition/src/workspace/handlers.rs:
- Around line 129-131: The workspace encryption keys are stored as plain
Strings; change the types of encryption_key and previous_encryption_key to
secrecy::SecretString (Option<SecretString> for previous_encryption_key) and
construct them via SecretString::new(...) when assigning (e.g., where you
currently set encryption_key: Some(encryption_key), use encryption_key:
Some(SecretString::new(encryption_key))). Update any imports (use
secrecy::SecretString) and adjust code that reads or logs these fields to call
expose_secret() only when strictly necessary, avoiding Debug/serialization to
prevent accidental leakage and ensuring secret types zeroize on drop.
In @smithy/models/secret.smithy:
- Around line 26-29: The Secret resource incorrectly lists RotateWorkspaceKey in
its operations (alongside CreateSecret); remove RotateWorkspaceKey from the
Secret resource's operations array and instead declare RotateWorkspaceKey as a
top-level standalone operation (not bound to the Secret resource), ensuring its
input/output types remain unchanged (so its output KeyRotationStatus stays
separate from Secret properties) and that Secret operations keep only
instance-scoped operations like CreateSecret; also update any references or
smithy docs that assumed RotateWorkspaceKey was a Secret-bound operation.
- Around line 38-39: SecretResponse's value field is not bound to the resource
model (resource defines encrypted_value) and causes validation errors; update
the model so API representation maps to a resource property: either add a
resource property named value (with appropriate traits such as @documentation,
@sensitive if available) to the Secret resource, or add a @property trait on
SecretResponse.value that binds it to the resource property encrypted_value (or
rename encrypted_value to value consistently), ensuring SecretResponse, the
resource definition, and any operations reference the same property name and
traits to separate encrypted storage from the API representation.
- Around line 12-13: The model defines a storage-only field encrypted_value but
all operations use value, so fix the mismatch by either marking encrypted_value
as internal-only (add the @notProperty trait to the encrypted_value property) or
by promoting the API field (add a properties entry for value: String and update
the resource/operation shapes to reference value instead of encrypted_value);
alternatively use a Smithy mixin/trait to map storage schema to API schema so
encrypted_value remains internal while value is exposed — update any operation
input/output shapes that currently reference value/encrypted_value to match the
chosen approach (look for encrypted_value and value in the resource properties
and operation shapes to apply the change).
In @superposition.sql:
- Line 150: Remove the redundant standalone COMMIT statement at the end of this
SQL file: since db_init.sql already wraps this script in a transaction
(BEGIN...COMMIT) and the application uses Diesel's conn.transaction() for
migrations/workspace setup, delete the final "COMMIT;" so the script does not
issue an explicit commit inside the outer transaction context.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
workspace_template.sql (1)
885-903: Misleading comment - references secrets but code handles default_configs.The comment on line 885 says "Add audit trigger for the secrets table" but the following code block (lines 886-903) adds a foreign key constraint for
default_configs.value_compute_function_name. The secrets audit trigger appears to be missing from this template.🔎 Proposed fix
Either:
- Remove/correct the misleading comment, or
- Add the actual secrets audit trigger:
-- Add audit trigger for the secrets table +DO $$ BEGIN + CREATE TRIGGER secrets_audit AFTER INSERT OR DELETE OR UPDATE ON {replaceme}.secrets FOR EACH ROW EXECUTE FUNCTION {replaceme}.event_logger(); +EXCEPTION + WHEN duplicate_object THEN null; +END $$; + +-- Add FK constraint for default_configs value_compute_function_name DO $$ BEGIN IF EXISTS ( SELECT 1 FROM information_schema.columnscrates/context_aware_config/src/api/default_config/handlers.rs (1)
197-225: Address clippy warning: unnecessary borrow.The clippy warning at line 223 correctly identifies that
&schema_nameshould beschema_namesinceschema_nameis already a reference (&SchemaName).🔎 Fix for needless borrow
validate_change_reason( &req.change_reason, &mut conn, - &schema_name, + schema_name, &state.master_key, )?;
♻️ Duplicate comments (8)
crates/superposition_types/migrations/2025-12-03-020509_secrets/down.sql (1)
1-4: Down migration is incomplete: missing DROP TABLE for public.secrets.As noted in the previous review, this down migration only drops the three workspace columns but does not drop the
public.secretstable (and its indexes) created in the up migration. This leaves the database in an inconsistent state after running down.🔎 Proposed fix
ALTER TABLE superposition.workspaces DROP COLUMN IF EXISTS key_rotation_at, DROP COLUMN IF EXISTS previous_encryption_key, DROP COLUMN IF EXISTS encryption_key; + +DROP TABLE IF EXISTS public.secrets;Note: Dropping the table will automatically remove its dependent indexes (idx_secrets_created_at, idx_secrets_last_modified_at, idx_secrets_key_version).
crates/superposition_types/src/database/models.rs (1)
283-285: Acknowledge past review: Use secure secret types for encryption keys.As noted in the previous review, storing
encryption_keyandprevious_encryption_keyas plainStringtypes has security implications. This concern applies consistently across the codebase (also raised formaster_keyinAppStateand workspace creation).Consider implementing a comprehensive solution using secure secret types (e.g.,
secrecy::SecretString) across all encryption key storage locations to ensure cryptographic material is properly protected.Let me verify all locations where these encryption keys are used to assess the scope of the required changes:
#!/bin/bash # Search for usage of encryption_key, previous_encryption_key, and master_key fields # to understand the full scope of the security concern echo "=== Searching for encryption_key usage ===" rg -n "encryption_key" --type rust -C 2 echo -e "\n=== Searching for previous_encryption_key usage ===" rg -n "previous_encryption_key" --type rust -C 2 echo -e "\n=== Searching for master_key usage ===" rg -n "master_key" --type rust -C 2crates/frontend/src/pages/secrets_list.rs (1)
185-187: Potential panic if serialization returns non-object.
json!(secret).as_object().unwrap()will panic if the serialization doesn't produce an object. Use defensive coding withunwrap_or_default().🔎 Proposed fix (from previous review)
.map(|secret| { - let mut ele_map = json!(secret).as_object().unwrap().to_owned(); + let mut ele_map = json!(secret) + .as_object() + .cloned() + .unwrap_or_default(); ele_mapcrates/frontend/src/pages/secret.rs (1)
64-94:use_navigate()called inside async block may cause issues.In Leptos,
use_navigate()should be called at the component level, not inside an asyncspawn_localblock. The hook may not have access to the correct reactive context when called asynchronously.🔎 Proposed fix: hoist use_navigate to component level
#[component] pub fn secret() -> impl IntoView { let workspace = use_context::<Signal<Tenant>>().unwrap(); let org = use_context::<Signal<OrganisationId>>().unwrap(); let params = use_params_map(); + let navigate = use_navigate(); let secret_name = Signal::derive(move || { params.with(|p| p.get("secret_name").cloned().unwrap_or_default()) }); // ... in on_delete_confirm ... let on_delete_confirm = move |_| { delete_inprogress_rws.set(true); let sec_name = secret_name.get_untracked(); + let nav = navigate.clone(); spawn_local(async move { // ... match result { Ok(_) => { logging::log!("Secret deleted successfully"); - let navigate = use_navigate(); let redirect_url = format!("/admin/{}/{}/secrets", org.get().0, workspace.get().0,); - navigate(&redirect_url, Default::default()); + nav(&redirect_url, Default::default());crates/context_aware_config/src/api/functions/helpers.rs (1)
75-86: Potential injection vulnerability: secret values are not escaped.This issue was flagged in a previous review. If a secret value contains
",\, or newlines, the generated JavaScript will be malformed or could lead to code injection. The same issue exists ingenerate_vars_template.Consider using
serde_json::to_stringfor proper JSON escaping:🔎 Proposed fix using proper JSON serialization
pub fn generate_secrets_template(secrets: &[(String, String)]) -> String { if secrets.is_empty() { return String::new(); } - let json_entries: Vec<String> = secrets - .iter() - .map(|(k, v)| format!("\"{}\":\"{}\"", k, v)) - .collect(); - - format!("const SECRETS = {{\n{}\n}};", json_entries.join(",\n")) + let map: serde_json::Map<String, serde_json::Value> = secrets + .iter() + .map(|(k, v)| (k.clone(), serde_json::Value::String(v.clone()))) + .collect(); + let json = serde_json::to_string_pretty(&map).unwrap_or_default(); + format!("const SECRETS = {};", json) }crates/service_utils/src/encryption.rs (3)
51-57: Security: Avoid logging the key value in error messages.This issue was flagged in a previous review. Line 53-54 includes the actual key value in the error message, which could leak sensitive key material to logs.
🔎 Suggested fix
if key_bytes.len() != 32 { return Err(EncryptionError::InvalidKey(format!( - "Key {} must be 32 bytes, got {}", - key, + "Key must be 32 bytes, got {}", key_bytes.len() ))); }
201-216: Operational risk: Random key generation in DEV/TEST may cause data loss.This issue was flagged in a previous review. If
MASTER_ENCRYPTION_KEYis not set in DEV/TEST environments, a random key is generated on each application restart, making previously encrypted secrets unrecoverable.Consider adding a warning log when using a generated key.
218-231: KMS decrypt function panics instead of returning errors.This issue was flagged in a previous review. The
crate::aws::kms::decryptfunction (per the relevant snippet) usespanic!()on failures, which will crash the service rather than propagating errors gracefully.Consider refactoring the KMS decrypt to return
Result<String, EncryptionError>and handling errors here.
🟡 Minor comments (3)
superposition.sql-150-150 (1)
150-150: Remove redundant COMMIT statement.The standalone
COMMITat the end is unnecessary. The initialization script (db_init.sql) that actually executes this file wraps all statements in an explicit transaction (BEGIN...COMMITat lines 61–1736). Additionally, the application uses Diesel ORM for migrations and workspace schema setup, which handles transaction boundaries automatically viaconn.transaction(). Explicit COMMIT statements are redundant in these contexts.crates/frontend/src/api.rs-1038-1044 (1)
1038-1044: Fix inconsistent header usage in secrets module: usex-workspaceinstead ofx-tenant.The secrets module uses
x-tenantheader while all other frontend API calls (variables, experiments, etc.) usex-workspace. Although the backend accepts both headers via fallback logic inget_workspace_id(), the inconsistency makes the code harder to maintain. Standardize onx-workspaceto match the rest of the codebase.crates/frontend/src/components/secret_form.rs-235-240 (1)
235-240: Remove manual.0dereferencing; useworkspace.get()andorg.get()directly.The tuple should be
(secret_name.clone(), workspace.get(), org.get()). BothWorkspaceandOrganisationIdimplementDeref, allowing automatic coercion to&strwhen passed tosecrets::get(). The manual.0access is unnecessary.
🧹 Nitpick comments (12)
crates/service_utils/src/service/types.rs (1)
56-58: AppState fields added for encryption key management.The additions properly extend AppState to support the new secrets management infrastructure. The
kms_clientis appropriately optional since KMS may not always be configured.Consider secure memory handling for sensitive keys.
The
master_keyandprevious_master_keyare stored as plainStringtypes in memory. While this may be acceptable for the application's threat model, consider whether additional protections are needed:
- Use of secure memory/zeroing on drop for key material
- Time-limited key retention in memory
- Audit logging of key access
crates/superposition_types/src/database/schema.rs (1)
767-779: Consider addingsecretstoallow_tables_to_appear_in_same_query!.The new
secretstable definition looks correct. However, it's not included in theallow_tables_to_appear_in_same_query!macro (lines 781-834), unlike other tables such asvariables. If secrets will ever need to be joined with other tables in queries, this could cause compilation errors.🔎 Proposed fix
Add
secretsto the allow_tables_to_appear_in_same_query! macro:diesel::allow_tables_to_appear_in_same_query!( config_versions, contexts, default_configs, dimensions, // ... other tables ... variables, + secrets, );crates/context_aware_config/src/api/secrets/handlers.rs (1)
278-293: Consider incrementing key_version on value update.When updating a secret's value (lines 278-293), the
key_versionfield is not incremented. This could make it difficult to track which version of the secret was used at different points in time. Consider incrementingkey_versionwhen the value changes to maintain an audit trail.However, if versioning isn't a requirement for this secrets implementation, this is acceptable.
</comment_end -->
crates/frontend/src/pages/secrets_list/filter.rs (1)
200-206: Consider simplifying comma-separated string parsing.Lines 201-206 use
serde_json::from_value(Value::String(names))to parse comma-separated strings intoCommaSeparatedStringQParams. This roundtrip throughserde_json::Valueis indirect.If
CommaSeparatedStringQParamssupports it, consider usingFromStror a direct constructor method for clearer intent.</comment_end -->
crates/superposition_types/src/database/models/others.rs (1)
221-253: Consider consolidating SecretName and VariableName validation.
SecretNameandVariableName(lines 178-201) have identical validation logic, both usingRegexEnum::VariableName. While keeping them as separate types provides type safety, the duplicated validation code could be refactored into a shared helper function.💡 Suggested refactoring approach
Create a shared validation helper:
fn validate_name_with_regex(name: String) -> Result<String, String> { let name = name.trim(); RegexEnum::VariableName .match_regex(name) .map(|_| name.to_string()) }Then use it in both implementations:
impl SecretName { pub fn validate(name: String) -> Result<Self, String> { validate_name_with_regex(name).map(Self) } } impl VariableName { pub fn validate(name: String) -> Result<Self, String> { validate_name_with_regex(name).map(Self) } }crates/superposition_types/migrations/2025-12-03-020509_secrets/up.sql (1)
1-20: Missing down migration for rollback.This migration lacks a corresponding
down.sqlfile. Without it, database rollbacks become difficult and error-prone, especially in production environments where migration failures need to be reversed cleanly.Create a
down.sqlfile with:DROP INDEX IF EXISTS public.idx_secrets_key_version; DROP INDEX IF EXISTS public.idx_secrets_last_modified_at; DROP INDEX IF EXISTS public.idx_secrets_created_at; DROP TABLE IF EXISTS public.secrets; ALTER TABLE superposition.workspaces DROP COLUMN IF EXISTS key_rotation_at, DROP COLUMN IF EXISTS previous_encryption_key, DROP COLUMN IF EXISTS encryption_key;crates/context_aware_config/src/api/secrets/key_rotation.rs (1)
113-145: Consider batching for large-scale secret re-encryption.The current implementation re-encrypts all workspace secrets in a single transaction. For workspaces with hundreds or thousands of secrets, this could:
- Hold database locks for an extended period
- Risk transaction timeouts
- Consume significant memory
For production deployments with large secret counts, consider implementing batched re-encryption:
const BATCH_SIZE: usize = 100; for batch in all_secrets.chunks(BATCH_SIZE) { // Re-encrypt batch // Commit batch update }This would improve performance and reduce lock contention, though it complicates transaction handling and error recovery.
crates/superposition_types/src/api/secrets.rs (1)
98-106: Minor inconsistency in DateTime import usage.Line 104 uses fully qualified
chrono::DateTime<chrono::Utc>while line 94 uses importedDateTime<Utc>. Consider using consistent import style.🔎 Suggested fix
pub struct MasterKeyRotationStatus { pub success: bool, pub workspaces_rotated: i64, pub total_workspaces: i64, pub total_secrets_re_encrypted: i64, - pub rotation_timestamp: chrono::DateTime<chrono::Utc>, + pub rotation_timestamp: DateTime<Utc>, pub message: String, }crates/service_utils/src/encryption.rs (1)
139-185: Good test coverage for encryption basics.Tests cover:
- Key generation produces correct length
- Round-trip encrypt/decrypt
- Wrong key fails decryption
- Fallback decryption with old key
Consider adding tests for edge cases like empty plaintext and special characters.
crates/frontend/src/components/secret_form.rs (3)
64-125: Consider the complexity of the two-phase update flow.The update logic requires two user interactions: first submit validates and stores the payload, second submit (via confirmation dialog) executes the update. While this provides good UX for confirmation, the state management with
update_request_rwsadds complexity.Consider documenting this flow with inline comments to help future maintainers understand the edit vs. create branching and the UpdatePrecheck → Response state transition.
283-309: UX consideration: Users cannot verify their new secret value.When updating a secret, the confirmation dialog shows the old value (fetched and masked) but displays the new value as
"••••••••". While this is secure, users cannot verify what they're about to save before confirming.This may be intentional for security, but consider whether showing a masked representation prevents users from catching typos in their secret values during the confirmation step.
272-279: Error handling could be more robust.When secret fetching fails (line 276-278), the error is logged but the confirm button remains disabled. However, there's no user-facing error message in the dialog itself, forcing users to check browser console.
Consider adding an error state to display a message within the ChangeLogPopup when the fetch fails.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
crates/context_aware_config/src/api.rscrates/context_aware_config/src/api/config/handlers.rscrates/context_aware_config/src/api/config/helpers.rscrates/context_aware_config/src/api/context/handlers.rscrates/context_aware_config/src/api/context/helpers.rscrates/context_aware_config/src/api/context/operations.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/handlers.rscrates/context_aware_config/src/api/functions/handlers.rscrates/context_aware_config/src/api/functions/helpers.rscrates/context_aware_config/src/api/secrets.rscrates/context_aware_config/src/api/secrets/handlers.rscrates/context_aware_config/src/api/secrets/key_rotation.rscrates/context_aware_config/src/api/type_templates/handlers.rscrates/context_aware_config/src/helpers.rscrates/context_aware_config/src/validation_functions.rscrates/frontend/src/api.rscrates/frontend/src/app.rscrates/frontend/src/components.rscrates/frontend/src/components/key_rotation_modal.rscrates/frontend/src/components/secret_form.rscrates/frontend/src/components/side_nav.rscrates/frontend/src/pages.rscrates/frontend/src/pages/function.rscrates/frontend/src/pages/secret.rscrates/frontend/src/pages/secrets_list.rscrates/frontend/src/pages/secrets_list/filter.rscrates/frontend/src/pages/workspace.rscrates/frontend/src/utils.rscrates/service_utils/Cargo.tomlcrates/service_utils/src/encryption.rscrates/service_utils/src/lib.rscrates/service_utils/src/middlewares/workspace_context.rscrates/service_utils/src/service/types.rscrates/superposition/src/app_state.rscrates/superposition/src/main.rscrates/superposition/src/resolve/handlers.rscrates/superposition/src/webhooks/handlers.rscrates/superposition/src/workspace/handlers.rscrates/superposition_types/migrations/2025-12-03-020509_secrets/down.sqlcrates/superposition_types/migrations/2025-12-03-020509_secrets/up.sqlcrates/superposition_types/src/api.rscrates/superposition_types/src/api/secrets.rscrates/superposition_types/src/database/models.rscrates/superposition_types/src/database/models/others.rscrates/superposition_types/src/database/schema.rscrates/superposition_types/src/database/superposition_schema.rssmithy/models/main.smithysmithy/models/secret.smithysuperposition.sqlworkspace_template.sql
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-03T13:25:40.584Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 816
File: crates/frontend/src/pages/webhook.rs:136-137
Timestamp: 2026-01-03T13:25:40.584Z
Learning: In the superposition codebase (Rust frontend), the `Workspace` and `OrganisationId` newtype wrappers implement `Deref`, which allows `&Workspace` and `&OrganisationId` to be automatically coerced to `&str` when passed to functions expecting `&str` parameters. Manual `.0` dereferencing is not needed.
Applied to files:
crates/service_utils/src/middlewares/workspace_context.rs
📚 Learning: 2026-01-03T13:27:14.072Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 816
File: crates/frontend/src/pages/type_template.rs:82-87
Timestamp: 2026-01-03T13:27:14.072Z
Learning: In the frontend crate, both `Workspace` and `OrganisationId` types implement `Deref` trait (via `#[derive(Deref)]`), allowing automatic coercion from `&Workspace` to `&str` and `&OrganisationId` to `&str`. When passing these types to functions expecting `&str`, use `&workspace` or `&org` directly instead of `&workspace.0` or `&org.0`.
Applied to files:
crates/service_utils/src/middlewares/workspace_context.rs
📚 Learning: 2026-01-02T20:59:01.233Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 543
File: crates/service_utils/src/middlewares/auth_z.rs:141-152
Timestamp: 2026-01-02T20:59:01.233Z
Learning: In crates/service_utils/src/middlewares/auth_z.rs, the AuthZHandler::init function is intentionally designed to panic on startup if AUTH_Z_PROVIDER environment variable is missing or set to an unknown value. This fail-fast behavior is expected and preferred for this critical authorization configuration.
Applied to files:
crates/superposition/src/main.rs
🧬 Code graph analysis (26)
crates/context_aware_config/src/api/type_templates/handlers.rs (2)
crates/superposition/src/app_state.rs (1)
get(26-139)crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(507-535)
crates/frontend/src/pages/secret.rs (4)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
secrets(241-244)crates/frontend/src/components/drawer.rs (1)
drawer(53-88)crates/frontend/src/components/secret_form.rs (1)
secret_form(45-216)crates/frontend/src/api.rs (23)
None(61-61)None(87-87)None(109-109)None(141-141)None(163-163)None(249-249)None(272-272)None(299-299)None(324-324)None(343-343)None(368-368)None(394-394)None(414-414)None(433-433)None(446-446)None(627-627)get(152-172)get(952-972)get(1078-1095)delete(238-258)delete(997-1017)delete(1117-1134)delete(1287-1307)
crates/frontend/src/components.rs (2)
crates/frontend/src/components/key_rotation_modal.rs (1)
key_rotation_modal(11-199)crates/frontend/src/components/secret_form.rs (1)
secret_form(45-216)
crates/frontend/src/pages.rs (2)
crates/frontend/src/pages/secret.rs (1)
secret(46-176)crates/frontend/src/pages/secrets_list.rs (1)
secrets_list(138-264)
crates/superposition_types/src/api.rs (1)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
secrets(241-244)
crates/superposition/src/webhooks/handlers.rs (2)
crates/service_utils/src/middlewares/workspace_context.rs (1)
req(81-81)crates/service_utils/src/service/types.rs (5)
req(113-113)req(139-139)req(165-165)req(189-189)req(208-208)
crates/context_aware_config/src/validation_functions.rs (1)
crates/context_aware_config/src/api/functions/helpers.rs (3)
functions(29-32)functions(53-57)inject_secrets_and_variables_into_code(116-217)
crates/superposition/src/workspace/handlers.rs (1)
crates/service_utils/src/encryption.rs (1)
generate_encryption_key(40-44)
crates/frontend/src/app.rs (2)
crates/frontend/src/pages/secret.rs (1)
secret(46-176)crates/frontend/src/pages/secrets_list.rs (1)
secrets_list(138-264)
crates/context_aware_config/src/api/secrets.rs (2)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
endpoints(24-33)crates/superposition/src/workspace/handlers.rs (1)
endpoints(62-69)
crates/frontend/src/pages/workspace.rs (1)
crates/frontend/src/components/key_rotation_modal.rs (1)
key_rotation_modal(11-199)
crates/frontend/src/pages/secrets_list.rs (4)
crates/frontend/src/components/table/types.rs (2)
default_column_formatter(53-57)default_with_sort(93-102)crates/frontend/src/query_updater.rs (2)
use_param_updater(17-63)use_signal_from_query(75-89)crates/superposition_types/src/lib.rs (1)
flip(235-240)crates/superposition_types/src/custom_query.rs (1)
reset_page(253-259)
crates/superposition_types/src/database/schema.rs (1)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
secrets(241-244)
crates/superposition_types/src/database/models/others.rs (3)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
secrets(241-244)crates/context_aware_config/src/api/variables/handlers.rs (1)
variables(139-142)crates/superposition_types/src/database/models.rs (7)
try_from(75-86)try_from(161-172)try_from(396-401)from(68-70)from(154-156)from(176-178)from(389-391)
crates/context_aware_config/src/api/functions/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(507-535)
crates/frontend/src/pages/secrets_list/filter.rs (1)
crates/superposition_types/src/custom_query.rs (1)
reset_page(253-259)
crates/frontend/src/components/key_rotation_modal.rs (7)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
secrets(241-244)crates/frontend/src/components/button.rs (1)
button(13-61)crates/frontend/src/components/change_form.rs (1)
change_form(6-34)crates/frontend/src/providers/alert_provider.rs (2)
alert_provider(72-82)enqueue_alert(54-69)crates/frontend/src/api.rs (4)
rotate_key(1136-1157)get(152-172)get(952-972)get(1078-1095)crates/context_aware_config/src/api/default_config/handlers.rs (2)
e(116-116)e(241-241)crates/frontend/src/utils.rs (3)
value(78-79)value(106-107)value(135-135)
crates/frontend/src/pages/function.rs (1)
crates/frontend/src/components/tip.rs (1)
tip(4-60)
crates/context_aware_config/src/api/dimension/handlers.rs (1)
crates/context_aware_config/src/helpers.rs (1)
validate_change_reason(507-535)
crates/context_aware_config/src/api/functions/helpers.rs (1)
crates/service_utils/src/encryption.rs (2)
decrypt_with_fallback(116-137)decrypt_workspace_key(194-199)
crates/superposition/src/main.rs (3)
crates/service_utils/src/encryption.rs (1)
get_master_encryption_keys(201-234)crates/superposition/src/app_state.rs (1)
get(26-139)crates/context_aware_config/src/api/secrets/handlers.rs (2)
secrets(241-244)endpoints(24-33)
crates/context_aware_config/src/api/config/handlers.rs (1)
crates/superposition/src/app_state.rs (1)
get(26-139)
crates/context_aware_config/src/api/context/operations.rs (1)
crates/context_aware_config/src/api/context/helpers.rs (2)
create_ctx_from_put_req(383-432)validate_override_with_functions(237-285)
crates/context_aware_config/src/api/context/helpers.rs (1)
crates/context_aware_config/src/validation_functions.rs (1)
execute_fn(176-236)
crates/service_utils/src/encryption.rs (2)
crates/superposition/src/app_state.rs (2)
get_from_env_or_default(36-36)get_from_env_or_default(113-113)crates/service_utils/src/aws/kms.rs (1)
decrypt(5-27)
crates/context_aware_config/src/api.rs (1)
crates/context_aware_config/src/api/secrets/handlers.rs (1)
secrets(241-244)
🪛 GitHub Actions: CI Checks on PRs
crates/frontend/src/pages/secret.rs
[error] 17-17: unresolved import crate::types::Tenant from use crate::types::{OrganisationId, Tenant};.
[error] 59-59: doesn't have a size known at compile-time. The type of this tuple contains a str which is unsized; ensure all local variables have a statically known size.
crates/frontend/src/components.rs
[error] 1-1: Cargo build failed for frontend crate: wasm32-unknown-unknown target. See logs for details.
crates/frontend/src/pages.rs
[error] 1-1: Cargo build failed for frontend crate: wasm32-unknown-unknown target. See logs for details.
crates/frontend/src/app.rs
[error] 1-1: Cargo build failed for frontend crate: wasm32-unknown-unknown target. See logs for details.
crates/frontend/src/pages/workspace.rs
[error] 1-1: Cargo build failed for frontend crate: wasm32-unknown-unknown target. See logs for details.
crates/frontend/src/components/side_nav.rs
[error] 62-62: cannot find value tenant in this scope.
[error] 63-63: cannot find value tenant in this scope.
crates/frontend/src/pages/secrets_list.rs
[error] 32-32: unresolved import crate::types::Tenant from use crate::types::{OrganisationId, Tenant};.
[error] 165-165: doesn't have a size known at compile-time. The type of this tuple contains a str which is unsized; ensure all local variables have a statically known size.
crates/frontend/src/utils.rs
[error] 1-1: Cargo build failed for frontend crate: wasm32-unknown-unknown target. See logs for details.
crates/service_utils/Cargo.toml
[error] Build failed: could not compile frontend (lib) due to multiple unresolved imports and unsized-type issues in frontend crates
crates/frontend/src/pages/secrets_list/filter.rs
[error] 1-1: Cargo build failed for frontend crate: wasm32-unknown-unknown target. See logs for details.
crates/frontend/src/components/key_rotation_modal.rs
[error] 1-1: Cargo build failed for frontend crate: wasm32-unknown-unknown target. See logs for details.
crates/frontend/src/pages/function.rs
[error] 1-1: Cargo build failed for frontend crate: wasm32-unknown-unknown target. See logs for details.
crates/frontend/src/components/secret_form.rs
[error] 23-23: unresolved import crate::types::Tenant in types::{OrganisationId, Tenant}. There is no Tenant in types.
[error] 237-237: doesn't have a size known at compile-time. The type of this tuple/closure contains a str which is unsized; ensure all local variables have a statically known size.
[error] 59-59: doesn't have a size known at compile-time. The type of this tuple contains a str which is unsized; ensure all local variables have a statically known size.
crates/context_aware_config/src/api/default_config/handlers.rs
[warning] 223-223: clippy: needless_borrow: change &schema_name to schema_name (potential improvement; not a failing error)
crates/frontend/src/api.rs
[error] 1-1: Cargo build failed for frontend crate: wasm32-unknown-unknown target. See logs for details.
smithy/models/secret.smithy
[error] 6-6: ResourceOperationInputOutput: The property 'encrypted_value' is not used in the input or output of create or an instance operation.
[error] 39-39: ResourceOperationInputOutput: SecretResponse$value: 'value' does not target a property or identifier for resource 'Secret'. If it is an identifier, apply resourceIdentifier trait; if a property, apply property trait.
[warning] 66-66: MemberShouldReferenceResource: SecretUnmaskedResponse$name references resources but does not reference them in @references (Secret, Variable, Webhook).
[error] 70-70: ResourceOperationInputOutput: SecretUnmaskedResponse$value: 'value' is not a valid member for Secret (not a property or identifier).
[error] 108-108: ResourceOperationInputOutput: RotateWorkspaceKey$success: 'success' is not a valid property or identifier for resource 'Secret'.
[error] 112-112: ResourceOperationInputOutput: RotateWorkspaceKey$secrets_re_encrypted: 'secrets_re_encrypted' is not a valid property or identifier for resource 'Secret'.
[error] 116-116: ResourceOperationInputOutput: RotateWorkspaceKey$rotation_timestamp: 'rotation_timestamp' is not a valid property or identifier for resource 'Secret'.
[error] 120-120: ResourceOperationInputOutput: RotateWorkspaceKey$message: 'message' is not a valid property or identifier for resource 'Secret'.
[warning] 129-129: MemberShouldReferenceResource: CreateSecretInput$name references resources (Variable, Webhook) but they are not included in @references.
[error] 133-133: ResourceOperationInputOutput: CreateSecretInput$value: 'value' is not a valid member for Secret (not a property or identifier).
[warning] 153-153: MemberShouldReferenceResource: UpdateSecretInput$name references resources (Variable, Webhook) but they are not included in @references.
[error] 156-156: ResourceOperationInputOutput: UpdateSecretInput$value: 'value' is not a valid member for Secret (not a property or identifier).
[warning] 176-176: MemberShouldReferenceResource: ListSecretsInput$name references resources (Variable, Webhook) but they are not included in @references.
[warning] 211-211: MemberShouldReferenceResource: GetSecretInput$name references resources (Variable, Webhook) but they are not included in @references.
[warning] 225-225: MemberShouldReferenceResource: DeleteSecretInput$name references resources (Variable, Webhook) but they are not included in @references.
[error] 234-234: ResourceIdentifierBinding: RotateWorkspaceKey: Missing required input identifier bindings (name, org_id, workspace_id) for binding to Secret; current bindings do not match expected (org_id, workspace_id).
[error] 234-234: ResourceIdentifierBinding: RotateWorkspaceKey: Operation cannot form a valid instance operation bound to Secret due to misbound identifiers.
| #[allow(clippy::too_many_arguments)] | ||
| #[authorized] | ||
| #[routes] | ||
| #[allow(clippy::too_many_arguments)] | ||
| #[get("/resolve")] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate #[allow(clippy::too_many_arguments)] attribute.
Line 630 and line 633 both have the same allow attribute. This appears to be an accidental duplication.
🔎 Suggested fix
#[allow(clippy::too_many_arguments)]
#[authorized]
#[routes]
-#[allow(clippy::too_many_arguments)]
#[get("/resolve")]
#[post("/resolve")]📝 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.
| #[allow(clippy::too_many_arguments)] | |
| #[authorized] | |
| #[routes] | |
| #[allow(clippy::too_many_arguments)] | |
| #[get("/resolve")] | |
| #[allow(clippy::too_many_arguments)] | |
| #[authorized] | |
| #[routes] | |
| #[get("/resolve")] |
🤖 Prompt for AI Agents
In @crates/context_aware_config/src/api/config/handlers.rs around lines 630-634,
Remove the duplicated attribute by keeping a single
#[allow(clippy::too_many_arguments)] on the handler annotated with #[authorized]
#[routes] #[get("/resolve")]; locate the attribute pair surrounding that resolve
handler and delete the extra #[allow(clippy::too_many_arguments)] so only one
remains, and run a quick search to ensure the same attribute isn't accidentally
duplicated elsewhere in the same module.
| Ok(Json(SecretResponse { | ||
| name: created_secret.name.0, | ||
| value: req.value, | ||
| key_version: created_secret.key_version, | ||
| description: created_secret.description, | ||
| change_reason: created_secret.change_reason, | ||
| created_at: created_secret.created_at, | ||
| last_modified_at: created_secret.last_modified_at, | ||
| created_by: created_secret.created_by, | ||
| last_modified_by: created_secret.last_modified_by, | ||
| })) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for response logging middleware and configuration
rg -n "Logger|ResponseLogger|middleware.*log|log.*middleware" crates/context_aware_config/src/Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check for logging configuration and setup
rg -n "env_logger|tracing|slog|log::|filter.*log" crates/context_aware_config/src/Repository: juspay/superposition
Length of output: 15649
🏁 Script executed:
# Check the main application setup and middleware configuration
fd "main.rs|lib.rs" crates/context_aware_config/src/ -x cat -n {}Repository: juspay/superposition
Length of output: 187
🏁 Script executed:
# Search for the handlers.rs file to understand the endpoint registration
rg -n "secrets.*routes|endpoints|configure" crates/context_aware_config/src/api/secrets/Repository: juspay/superposition
Length of output: 588
🏁 Script executed:
# Search for app/service setup and middleware configuration
rg -n "actix|App|HttpServer|service\(|configure" crates/context_aware_config/src/main.rs crates/context_aware_config/src/lib.rsRepository: juspay/superposition
Length of output: 143
🏁 Script executed:
# Check the handlers.rs file around the create_secret handler
sed -n '195,230p' crates/context_aware_config/src/api/secrets/handlers.rsRepository: juspay/superposition
Length of output: 1228
🏁 Script executed:
# Look for any logging configuration or filter setup
rg -n "RUST_LOG|env_logger|tracing|LogLevel|log.*filter" crates/Repository: juspay/superposition
Length of output: 20327
🏁 Script executed:
# Check context_aware_config Cargo.toml for logging dependencies
cat crates/context_aware_config/Cargo.toml | grep -A 5 -B 5 "log\|tracing"Repository: juspay/superposition
Length of output: 835
🏁 Script executed:
# Find lib.rs or app setup in context_aware_config
find crates/context_aware_config/src -name "lib.rs" -o -name "mod.rs" | head -5Repository: juspay/superposition
Length of output: 102
🏁 Script executed:
# Check for any app/server setup that might have middleware
rg -n "HttpServer|App::new|middleware|service\(" crates/context_aware_config/src/ -A 3Repository: juspay/superposition
Length of output: 6874
🏁 Script executed:
# Look at the entire handlers.rs to understand the endpoint structure
wc -l crates/context_aware_config/src/api/secrets/handlers.rsRepository: juspay/superposition
Length of output: 123
🏁 Script executed:
# Check lib.rs for app configuration and middleware setup
cat crates/context_aware_config/src/lib.rsRepository: juspay/superposition
Length of output: 159
🏁 Script executed:
# Get the first 80 lines of handlers.rs to see the full structure and decorators
head -80 crates/context_aware_config/src/api/secrets/handlers.rsRepository: juspay/superposition
Length of output: 2556
🏁 Script executed:
# Search for tracing-actix-web which was mentioned in CHANGELOG
rg -n "tracing-actix-web|TracingLogger" crates/context_aware_config/Repository: juspay/superposition
Length of output: 192
🏁 Script executed:
# Check if tracing-actix-web is in Cargo.toml
rg -n "tracing-actix-web" crates/context_aware_config/Cargo.toml crates/service_utils/Cargo.tomlRepository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Search for where the app is set up - likely in superposition crate
find crates/superposition -name "*.rs" | xargs grep -l "HttpServer\|App::new" | head -5Repository: juspay/superposition
Length of output: 96
🏁 Script executed:
# Look for any middleware configuration in superposition
rg -n "middleware\|Logger\|middleware::" crates/superposition/src/Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check service_utils for app configuration
rg -n "app_factory|middleware|configure" crates/service_utils/src/Repository: juspay/superposition
Length of output: 894
🏁 Script executed:
# Check main.rs for app configuration
head -150 crates/superposition/src/main.rsRepository: juspay/superposition
Length of output: 5251
🏁 Script executed:
# List the available middlewares
ls -la crates/service_utils/src/middlewares/Repository: juspay/superposition
Length of output: 552
🏁 Script executed:
# Check if there's any response logging middleware defined
rg -n "ResponseLogger|response.*log|log.*response" crates/service_utils/src/middlewares/Repository: juspay/superposition
Length of output: 1176
🏁 Script executed:
# Examine the RequestResponseLogger middleware to see what it logs
cat crates/service_utils/src/middlewares/request_response_logging.rsRepository: juspay/superposition
Length of output: 7823
🏁 Script executed:
# Continue reading main.rs to see if RequestResponseLogger is added to the middleware chain
sed -n '100,200p' crates/superposition/src/main.rsRepository: juspay/superposition
Length of output: 4380
🏁 Script executed:
# Check if there are any filters or exclusions for the RequestResponseLogger
rg -n "RequestResponseLogger|request_response_logging" crates/superposition/src/main.rsRepository: juspay/superposition
Length of output: 212
🏁 Script executed:
# Look for any path-based middleware configuration or guards
rg -n "\.wrap\(|middleware::" crates/superposition/src/main.rsRepository: juspay/superposition
Length of output: 1899
Response logging will expose plaintext secrets at trace level.
The create_secret handler returns the plaintext value field in the response (line 220). The RequestResponseLogger middleware (applied at line 294 of main.rs) logs complete response bodies at TRACE level without filtering sensitive fields. If RUST_LOG includes trace-level logging, this plaintext secret value will be logged.
The middleware is conditionally applied only when trace logging is explicitly enabled, but this provides insufficient protection because:
- Trace logging is easily enabled for debugging in development or staging environments
- No filtering or masking is applied even at trace level
- The code contains no documentation of this security trade-off
Consider:
- Adding a sanitizing filter in the
RequestResponseLoggermiddleware to redact sensitive fields (e.g.,valuein secret endpoints) - Documenting that trace-level logging for
/secretsendpoints exposes plaintext secret values - Using a configuration flag to disable response body logging for specific paths
🤖 Prompt for AI Agents
In @crates/context_aware_config/src/api/secrets/handlers.rs around lines
218-228, The create_secret handler currently returns the plaintext secret in
SecretResponse.value which will be captured by RequestResponseLogger at TRACE;
change the handler (create_secret) to not echo the raw secret (e.g., return
value: "[REDACTED]" or omit the field) and/or add a sanitizing path-aware filter
to the RequestResponseLogger middleware so it redacts the "value" field for
secret endpoints (or disables body logging for "/secrets"); update related
types/serializers if needed (SecretResponse) and add a brief inline
comment/documentation noting that trace-level logging will not include plaintext
secret values.
| #[post("/rotate-workspace-key")] | ||
| pub async fn rotate_encryption_key( | ||
| user: User, | ||
| db_conn: DbConnection, | ||
| workspace_id: service_utils::service::types::WorkspaceId, | ||
| org_id: service_utils::service::types::OrganisationId, | ||
| schema_name: service_utils::service::types::SchemaName, | ||
| app_state: Data<AppState>, | ||
| ) -> superposition::Result<Json<KeyRotationStatus>> { | ||
| use superposition_types::database::superposition_schema::superposition::workspaces::dsl as ws; | ||
|
|
||
| let DbConnection(mut conn) = db_conn; | ||
|
|
||
| let master_key = app_state.master_key.clone(); | ||
|
|
||
| let workspace: Workspace = ws::workspaces | ||
| .filter(ws::workspace_name.eq(&workspace_id.0)) | ||
| .filter(ws::organisation_id.eq(&org_id.0)) | ||
| .first(&mut conn)?; | ||
|
|
||
| let is_initial_setup = workspace.encryption_key.is_none(); | ||
| let user_email = user.get_email(); | ||
| let schema_name_str = schema_name.0.clone(); | ||
|
|
||
| let result = conn.transaction::<KeyRotationStatus, superposition::AppError, _>(|conn| { | ||
| let (secrets_re_encrypted, rotation_timestamp) = rotate_workspace_encryption_key_internal( | ||
| conn, | ||
| &workspace, | ||
| &schema_name_str, | ||
| &master_key, | ||
| &master_key, // Same master key for both encrypt/decrypt in workspace rotation | ||
| &user_email, | ||
| )?; | ||
|
|
||
| Ok(KeyRotationStatus { | ||
| success: true, | ||
| secrets_re_encrypted, | ||
| rotation_timestamp, | ||
| message: if is_initial_setup { | ||
| format!( | ||
| "Successfully set up initial encryption key and encrypted {} secret(s)", | ||
| secrets_re_encrypted | ||
| ) | ||
| } else { | ||
| format!( | ||
| "Successfully rotated encryption key and re-encrypted {} secret(s)", | ||
| secrets_re_encrypted | ||
| ) | ||
| }, | ||
| }) | ||
| })?; | ||
|
|
||
| Ok(Json(result)) | ||
| } |
There was a problem hiding this comment.
Critical: Missing authorization check on key rotation endpoint.
The rotate_encryption_key endpoint lacks an #[authorized] attribute or equivalent authorization check. This allows any authenticated user to rotate workspace encryption keys, which is a highly privileged operation that should be restricted to administrators.
Add authorization:
+#[authorized(permission = "WORKSPACE_ADMIN")]
#[post("/rotate-workspace-key")]
pub async fn rotate_encryption_key(Verify similar authorization is in place for other sensitive endpoints in this module.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @crates/context_aware_config/src/api/secrets/key_rotation.rs around lines
176-229, The rotate_encryption_key handler (function rotate_encryption_key) is
missing an authorization check; add the appropriate authorization attribute or
explicit guard so only admins/privileged users can invoke it (e.g. add
#[authorized] above the function or call the existing authorization helper such
as authorize_workspace_admin(&user, &workspace) at the top of
rotate_encryption_key before performing the rotation), returning a proper error
on unauthorized access; also audit other sensitive handlers in this module for
the same pattern and apply the same attribute or helper where needed.
| let result = conn.transaction::<KeyRotationStatus, superposition::AppError, _>(|conn| { | ||
| let (secrets_re_encrypted, rotation_timestamp) = rotate_workspace_encryption_key_internal( | ||
| conn, | ||
| &workspace, | ||
| &schema_name_str, | ||
| &master_key, | ||
| &master_key, // Same master key for both encrypt/decrypt in workspace rotation | ||
| &user_email, | ||
| )?; |
There was a problem hiding this comment.
Logic error: Using same key for old and new master key in workspace rotation.
Line 206 passes &master_key for both the new and old master key parameters. This means the function decrypts the existing workspace key with master_key and then re-encrypts it with the same master_key, which is effectively a no-op for the workspace key itself (though secrets are still re-encrypted with a new workspace key).
For workspace-only rotation (not master key rotation), this might be intentional—but the function name and behavior suggest the workspace key should be rotated to a newly generated key, not re-encrypted with the same master key.
Clarify the intended behavior:
- If this endpoint is meant to rotate the workspace key only (not the master key), the current logic is correct
- If this should support optional master key changes, it needs a different parameter
The confusion arises because rotate_workspace_encryption_key_internal has separate new_master_key and old_master_key parameters, but this endpoint provides the same value for both.
| #[post("/rotate-master-key")] | ||
| pub async fn rotate_master_key( | ||
| user: User, | ||
| db_conn: DbConnection, | ||
| app_state: Data<AppState>, | ||
| ) -> superposition::Result<Json<MasterKeyRotationStatus>> { | ||
| use superposition_types::database::superposition_schema::superposition::workspaces::dsl as ws; | ||
|
|
||
| let DbConnection(mut conn) = db_conn; | ||
|
|
||
| let new_master_key = app_state.master_key.clone(); | ||
| let previous_master_key = app_state.previous_master_key.clone(); | ||
|
|
||
| let all_workspaces: Vec<Workspace> = ws::workspaces.load(&mut conn)?; | ||
|
|
||
| let total_workspaces = all_workspaces.len() as i64; | ||
| let user_email = user.get_email(); | ||
|
|
||
| let result = conn.transaction::<MasterKeyRotationStatus, superposition::AppError, _>(|conn| { | ||
| let mut workspaces_rotated = 0i64; | ||
| let mut total_secrets_re_encrypted = 0i64; | ||
|
|
||
| for workspace in &all_workspaces { | ||
| // Skip workspaces without encryption keys configured | ||
| if workspace.encryption_key.is_none() { | ||
| log::info!( | ||
| "Skipping workspace {}/{} - no encryption key configured", | ||
| workspace.organisation_id, | ||
| workspace.workspace_name | ||
| ); | ||
| continue; | ||
| } | ||
|
|
||
| // Derive schema name from workspace (adjust based on your schema naming convention) | ||
| let schema_name = format!("{}_{}", workspace.organisation_id, workspace.workspace_name); | ||
|
|
||
| match rotate_workspace_encryption_key_internal( | ||
| conn, | ||
| workspace, | ||
| &schema_name, | ||
| &new_master_key, | ||
| &previous_master_key, | ||
| &user_email, | ||
| ) { | ||
| Ok((secrets_count, _)) => { | ||
| workspaces_rotated += 1; | ||
| total_secrets_re_encrypted += secrets_count; | ||
| } | ||
| Err(e) => { | ||
| let workspace_identifier = format!( | ||
| "{}/{}", | ||
| workspace.organisation_id, workspace.workspace_name | ||
| ); | ||
| log::error!( | ||
| "Failed to rotate keys for workspace {}: {}", | ||
| workspace_identifier, | ||
| e | ||
| ); | ||
| // Return error to rollback transaction on any failure | ||
| return Err(e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let rotation_time = chrono::Utc::now(); | ||
|
|
||
| log::info!( | ||
| "Successfully rotated master key. Rotated {} workspaces, re-encrypted {} secrets.", | ||
| workspaces_rotated, | ||
| total_secrets_re_encrypted | ||
| ); | ||
|
|
||
| Ok(MasterKeyRotationStatus { | ||
| success: true, | ||
| workspaces_rotated, | ||
| total_workspaces, | ||
| total_secrets_re_encrypted, | ||
| rotation_timestamp: rotation_time, | ||
| message: format!( | ||
| "Successfully rotated master key for {} workspace(s) and re-encrypted {} secret(s)", | ||
| workspaces_rotated, total_secrets_re_encrypted | ||
| ), | ||
| }) | ||
| })?; | ||
|
|
||
| Ok(Json(result)) | ||
| } |
There was a problem hiding this comment.
Critical: Missing authorization check on master key rotation endpoint.
The rotate_master_key endpoint lacks authorization checks. Master key rotation is an extremely sensitive operation that affects all workspaces and should be restricted to system administrators only.
Add strict authorization:
+#[authorized(permission = "SYSTEM_ADMIN")]
#[post("/rotate-master-key")]
pub async fn rotate_master_key(Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @crates/context_aware_config/src/api/secrets/key_rotation.rs around lines
231-317, The rotate_master_key handler lacks an authorization guard; before
loading workspaces or starting the DB transaction in rotate_master_key, verify
the invoking User is a system administrator (e.g., call a method like
user.is_admin() or user.has_role("admin") / user.is_system_admin()) and if not
return an appropriate HTTP Forbidden/AppError response and log the denied
attempt; place this check at the top of rotate_master_key (using the User
argument) so no DB reads or key operations occur for unauthorized callers.
| .service( | ||
| scope("/secrets") | ||
| .wrap(OrgWorkspaceMiddlewareFactory::new(true, true)) | ||
| .service(secrets::endpoints()) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Resource enum variants
echo "=== Resource enum definition ==="
rg -n "enum Resource" -A 30 crates/service_utils/src/service/types.rs
echo ""
echo "=== Resource usage in main.rs ==="
rg -n "Resource::" crates/superposition/src/main.rs | head -30
echo ""
echo "=== app_data usage for scopes in main.rs ==="
rg -n "\.app_data\(Resource::" crates/superposition/src/main.rs
echo ""
echo "=== Context around /secrets scope ==="
sed -n '255,275p' crates/superposition/src/main.rs
echo ""
echo "=== Context around /context scope ==="
rg -n "/context" crates/superposition/src/main.rs -B 2 -A 5 | head -30Repository: juspay/superposition
Length of output: 4539
Add missing Resource app_data for /secrets scope and create Resource::Secrets enum variant.
The /secrets scope is missing the .app_data(Resource::...) pattern used consistently by all other service scopes (/context, /dimension, /variables, /webhook, /audit-log, /config, /experiment, /function, etc.). Additionally, the Resource enum lacks a Secrets variant. This gap may prevent proper authorization and resource-level access control if AuthZHandler relies on the Resource context.
Add Secrets to the Resource enum in crates/service_utils/src/service/types.rs and include .app_data(Resource::Secrets) in the /secrets scope definition.
🤖 Prompt for AI Agents
In @crates/superposition/src/main.rs around lines 262-266, Add a new Resource
variant and wire it into the secrets scope: add a Secrets variant to the
Resource enum (e.g., Resource::Secrets) in the service types definition, then
update the scope("/secrets") registration to include
.app_data(Resource::Secrets) alongside the existing
.wrap(OrgWorkspaceMiddlewareFactory::new(true, true)) and
.service(secrets::endpoints()) calls; this ensures the AuthZHandler sees the
correct resource context for the secrets endpoints.
| encryption_key: Some(encryption_key), | ||
| previous_encryption_key: None, | ||
| key_rotation_at: None, |
There was a problem hiding this comment.
Apply secure secret types to workspace encryption keys.
Similar to the concern with master_key in AppState, the workspace-level encryption_key is stored as a plain String type, which has the same security implications:
- Keys persist in memory without zeroization
- Potential exposure through Debug traits, serialization, or logging
- Risk of keys appearing in error messages
Consider using a secure secret type (e.g., secrecy::SecretString) for encryption_key and previous_encryption_key fields to ensure cryptographic material is properly protected throughout its lifecycle.
🤖 Prompt for AI Agents
In @crates/superposition/src/workspace/handlers.rs around lines 129-131, The
workspace encryption keys are stored as plain Strings; change the types of
encryption_key and previous_encryption_key to secrecy::SecretString
(Option<SecretString> for previous_encryption_key) and construct them via
SecretString::new(...) when assigning (e.g., where you currently set
encryption_key: Some(encryption_key), use encryption_key:
Some(SecretString::new(encryption_key))). Update any imports (use
secrecy::SecretString) and adjust code that reads or logs these fields to call
expose_secret() only when strictly necessary, avoiding Debug/serialization to
prevent accidental leakage and ensuring secret types zeroize on drop.
| properties: { | ||
| encrypted_value: String |
There was a problem hiding this comment.
Resolve the property modeling inconsistency.
The resource defines encrypted_value as a property, but no operation uses it in inputs/outputs. Instead, operations use a value field, causing multiple validation errors throughout the model.
This mismatch between storage representation (encrypted_value) and API representation (value) needs resolution. Consider:
- Marking
encrypted_valuewith@notPropertysince it's internal-only - Adding
valueas a proper property if it should be part of the resource model - Using Smithy mixins or traits to separate storage schema from API schema
🤖 Prompt for AI Agents
In @smithy/models/secret.smithy around lines 12-13, The model defines a
storage-only field encrypted_value but all operations use value, so fix the
mismatch by either marking encrypted_value as internal-only (add the
@notProperty trait to the encrypted_value property) or by promoting the API
field (add a properties entry for value: String and update the
resource/operation shapes to reference value instead of encrypted_value);
alternatively use a Smithy mixin/trait to map storage schema to API schema so
encrypted_value remains internal while value is exposed — update any operation
input/output shapes that currently reference value/encrypted_value to match the
chosen approach (look for encrypted_value and value in the resource properties
and operation shapes to apply the change).
| operations: [ | ||
| CreateSecret | ||
| RotateWorkspaceKey | ||
| ] |
There was a problem hiding this comment.
RotateWorkspaceKey should not be bound to the Secret resource.
The RotateWorkspaceKey operation is workspace-scoped (operates on all secrets, not a specific secret), but it's bound to the Secret resource via the operations list (line 28). This causes validation errors because:
- The operation doesn't include the
nameidentifier required for Secret instance operations - Its output
KeyRotationStatuscontains fields that aren't Secret properties
Solution: Move RotateWorkspaceKey to a standalone operation outside the Secret resource definition. It's a workspace-level operation, similar to how you might define workspace settings or admin operations.
Also applies to: 104-121
🤖 Prompt for AI Agents
In @smithy/models/secret.smithy around lines 26-29, The Secret resource
incorrectly lists RotateWorkspaceKey in its operations (alongside CreateSecret);
remove RotateWorkspaceKey from the Secret resource's operations array and
instead declare RotateWorkspaceKey as a top-level standalone operation (not
bound to the Secret resource), ensuring its input/output types remain unchanged
(so its output KeyRotationStatus stays separate from Secret properties) and that
Secret operations keep only instance-scoped operations like CreateSecret; also
update any references or smithy docs that assumed RotateWorkspaceKey was a
Secret-bound operation.
| @documentation("Masked value shown as '••••••••' for security. Only shown unmasked during creation.") | ||
| value: String |
There was a problem hiding this comment.
The value field requires proper resource modeling.
The value field in SecretResponse doesn't reference any Secret resource property or identifier, causing validation errors. This stems from the resource defining encrypted_value while operations use value.
To fix this, either:
- Add
valueas a resource property with appropriate traits - Use
@propertytrait to explicitly bind the field to a resource property - Restructure the model to properly separate encrypted storage from API representation
🧰 Tools
🪛 GitHub Actions: CI Checks on PRs
[error] 39-39: ResourceOperationInputOutput: SecretResponse$value: 'value' does not target a property or identifier for resource 'Secret'. If it is an identifier, apply resourceIdentifier trait; if a property, apply property trait.
🤖 Prompt for AI Agents
In @smithy/models/secret.smithy around lines 38-39, SecretResponse's value field
is not bound to the resource model (resource defines encrypted_value) and causes
validation errors; update the model so API representation maps to a resource
property: either add a resource property named value (with appropriate traits
such as @documentation, @sensitive if available) to the Secret resource, or add
a @property trait on SecretResponse.value that binds it to the resource property
encrypted_value (or rename encrypted_value to value consistently), ensuring
SecretResponse, the resource definition, and any operations reference the same
property name and traits to separate encrypted storage from the API
representation.
Problem
Describe the problem you are trying to solve here
Solution
Provide a brief summary of your solution so that reviewers can understand your code
Environment variable changes
What ENVs need to be added or changed
Pre-deployment activity
Things needed to be done before deploying this change (if any)
Post-deployment activity
Things needed to be done after deploying this change (if any)
API changes
Possible Issues in the future
Describe any possible issues that could occur because of this change
Summary by CodeRabbit
New Features
SECRETS.{SECRET_NAME}syntaxUI Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.