fix: add separate kronos dispatch token#1089
Conversation
|
Review changes with Changed Files
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a dedicated Kronos dispatch token, retrieves and stores it in application state, uses it for dispatcher secrets, and authenticates dispatch webhooks through a new dispatch-user context. ChangesKronos dispatch token flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Kronos as Kronos dispatcher
participant AuthNMiddleware
participant AppState
participant dispatch_handler
Kronos->>AuthNMiddleware: Send /dispatch/webhook with Internal authorization
AuthNMiddleware->>AppState: Compare token with kronos_dispatch_token
AuthNMiddleware->>dispatch_handler: Insert DispatchUser and forward request
dispatch_handler->>dispatch_handler: Validate DispatchUserContext
dispatch_handler-->>Kronos: Process webhook or return unauthorized
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
mahatoankitkumar
force-pushed
the
feat/kronos-implementation
branch
from
8b53e71 to
002bcdc
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated authentication path for Kronos webhook dispatch callbacks by adding a scoped KRONOS_DISPATCH_TOKEN, wiring it through app state and Kronos dispatcher provisioning, and updating the dispatch endpoint to require a dispatch-specific caller context instead of the broader internal-caller context.
Changes:
- Added
KRONOS_DISPATCH_TOKENretrieval and plumbed it throughAppState, dispatcher setup, and workspace lifecycle hooks. - Extended auth middleware + types with a
DispatchUsermarker/context and updated the dispatch handler to require it. - Bumped
kronos-worker/kronos-commongit revisions and updated.env.exampledocumentation.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/superposition/src/workspace/handlers.rs | Uses the new dispatch token when provisioning/updating Kronos dispatcher config per workspace. |
| crates/superposition/src/dispatch/handlers.rs | Requires DispatchUserContext for /dispatch/webhook and updates the unauthorized message. |
| crates/superposition/src/app_state.rs | Loads KRONOS_DISPATCH_TOKEN, stores it in AppState, and uses it when setting up dispatcher in service mode. |
| crates/superposition_types/src/lib.rs | Adds DispatchUser marker and DispatchUserContext extractor. |
| crates/service_utils/src/service/types.rs | Extends AppState with kronos_dispatch_token. |
| crates/service_utils/src/middlewares/auth_n.rs | Adds auth middleware branch to recognize the dispatch token and mark requests as DispatchUser. |
| crates/service_utils/src/kronos_dispatch.rs | Updates dispatcher secret provisioning to use the dispatch token and clarifies header/token semantics. |
| crates/service_utils/src/db/utils.rs | Adds get_kronos_dispatch_token env/KMS retrieval helper. |
| Cargo.toml | Bumps kronos-worker / kronos-common git revs. |
| Cargo.lock | Locks updated Kronos git revs and transitive dependency changes. |
| .env.example | Documents KRONOS_DISPATCH_TOKEN. |
Comments suppressed due to low confidence (1)
crates/superposition/src/app_state.rs:43
SUPERPOSITION_HOSTno longer has the servicebasesuffix applied. When the API is deployed behind a non-emptyservice_prefix(sobaseis like/foo), the Kronos dispatcher URL will be registered as<host>/dispatch/webhookinstead of<host><base>/dispatch/webhook, breaking callbacks..env.examplealso documentsSUPERPOSITION_HOSTwithout the base suffix, so the previous behavior seems intentional.
let cac_host =
get_from_env_or_default::<String>("CAC_HOST", format!("http://localhost:{port}"))
+ base;
let superposition_host =
std::env::var("SUPERPOSITION_HOST").unwrap_or_else(|_| cac_host.clone());
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let superposition_token = get_superposition_token(kms_client, &app_env).await; | ||
| let kronos_dispatch_token = get_kronos_dispatch_token(kms_client, &app_env).await; | ||
|
|
| // Kronos dispatch callbacks: scoped token, marks the request | ||
| // as DispatchUser only (no InternalUser, so no authz bypass). | ||
| (Some("Internal"), Some(token)) | ||
| if token == state.kronos_dispatch_token => | ||
| { | ||
| request | ||
| .headers() | ||
| .get("x-user") | ||
| .and_then(|auth| auth.to_str().ok()) | ||
| .and_then(|user_str| { | ||
| serde_json::from_str::<User>(user_str).ok() | ||
| }) | ||
| .map(|user| { | ||
| request | ||
| .extensions_mut() | ||
| .insert::<DispatchUser>(DispatchUser); | ||
| Ok(user) | ||
| }) | ||
| } |
mahatoankitkumar
changed the title
| // Kronos dispatch callbacks: scoped token, marks the request | ||
| // as DispatchUser only (no InternalUser, so no authz bypass). | ||
| (Some("Internal"), Some(token)) | ||
| if token == state.kronos_dispatch_token => | ||
| { | ||
| request | ||
| .headers() | ||
| .get("x-user") | ||
| .and_then(|auth| auth.to_str().ok()) | ||
| .and_then(|user_str| { | ||
| serde_json::from_str::<User>(user_str).ok() | ||
| }) | ||
| .map(|user| { | ||
| request | ||
| .extensions_mut() | ||
| .insert::<DispatchUser>(DispatchUser); | ||
| Ok(user) | ||
| }) | ||
| } |
There was a problem hiding this comment.
All of this could have been written in the branch above. Can we reduce the DRY ?
mahatoankitkumar
force-pushed
the
feat/kronos-implementation
branch
from
002bcdc to
8471724
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/service_utils/src/kronos_dispatch.rs (2)
89-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
DISPATCHER_SECRET_NAMEis now misleading — consider a clarifying comment.The constant
"superposition-internal-token"is used as the Kronos secret key name, but it now stores the dispatch token (not thesuperposition_token). Renaming would break existing Kronos workspace secrets, so at minimum add a comment noting that despite the name, the value stored is the scopeddispatch_token.📝 Suggested comment
pub const DISPATCHER_SECRET_NAME: &str = "superposition-internal-token"; +// NOTE: Despite the name, this secret stores the scoped KRONOS_DISPATCH_TOKEN +// (not SUPERPOSITION_TOKEN). The name is retained for backward compatibility +// with existing Kronos workspace secrets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/service_utils/src/kronos_dispatch.rs` around lines 89 - 99, Add a clarifying comment next to DISPATCHER_SECRET_NAME stating that, despite its legacy name, the Kronos secret stores the scoped dispatch_token rather than superposition_token; retain the constant name to preserve compatibility with existing workspace secrets.
85-115: 🩺 Stability & Availability | 🔵 TrivialExisting library-mode workspaces may need dispatcher secret re-provisioning.
In library mode (
kronos_workspace.is_none()),setup_dispatcheris only called on workspace creation or migration. Existing workspaces that were set up before this PR still have the oldsuperposition_tokenstored as the dispatcher secret in Kronos. After deployment, the auth middleware will match the incoming old token againstsuperposition_tokenfirst (insertingInternalUser), soDispatchUserContextwill fail and the dispatch handler will reject the webhook.In service mode this is not an issue — the shared workspace dispatcher is re-provisioned on every startup. For library mode, consider whether a one-time migration or re-provisioning step is needed for existing workspaces.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/service_utils/src/kronos_dispatch.rs` around lines 85 - 115, Ensure library-mode startup re-provisions the dispatcher secret for existing workspaces, not only during workspace creation or migration. Update the library-mode initialization path to invoke setup_dispatcher with the current dispatch token for every existing workspace, while preserving the existing service-mode behavior and avoiding duplicate setup where appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/service_utils/src/middlewares/auth_n.rs`:
- Around line 112-123: Add startup validation in app_state.rs where
superposition_token and kronos_dispatch_token are retrieved: assert or otherwise
fail initialization when both configured token values are identical, while
allowing distinct values and unset configuration as appropriate. Locate the
application state initialization logic and ensure the validation runs before the
service starts.
- Around line 112-114: Reject blank KRONOS_DISPATCH_TOKEN values before storing
the token in AppState: update the get_from_env_or_default handling used to
initialize AppState so empty or whitespace-only values are treated as
missing/invalid and cannot satisfy the Internal dispatch webhook match in the
auth middleware.
---
Nitpick comments:
In `@crates/service_utils/src/kronos_dispatch.rs`:
- Around line 89-99: Add a clarifying comment next to DISPATCHER_SECRET_NAME
stating that, despite its legacy name, the Kronos secret stores the scoped
dispatch_token rather than superposition_token; retain the constant name to
preserve compatibility with existing workspace secrets.
- Around line 85-115: Ensure library-mode startup re-provisions the dispatcher
secret for existing workspaces, not only during workspace creation or migration.
Update the library-mode initialization path to invoke setup_dispatcher with the
current dispatch token for every existing workspace, while preserving the
existing service-mode behavior and avoiding duplicate setup where appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6141ed65-a3f3-4110-9d60-49230e421b3b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.env.exampleCargo.tomlcrates/service_utils/src/db/utils.rscrates/service_utils/src/kronos_dispatch.rscrates/service_utils/src/middlewares/auth_n.rscrates/service_utils/src/service/types.rscrates/superposition/src/app_state.rscrates/superposition/src/dispatch/handlers.rscrates/superposition/src/workspace/handlers.rscrates/superposition_types/src/lib.rs
| (Some("Internal"), Some(token)) | ||
| if token == state.kronos_dispatch_token | ||
| && request.path().ends_with("/dispatch/webhook") => |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect get_kronos_dispatch_token to verify it rejects empty/missing tokens.
ast-grep outline crates/service_utils/src/db/utils.rs --match 'get_kronos_dispatch_token' --view expandedRepository: juspay/superposition
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation of get_kronos_dispatch_token and nearby helpers.
sed -n '1,220p' crates/service_utils/src/db/utils.rs
echo
echo '--- references ---'
rg -n "kronos_dispatch_token|get_kronos_dispatch_token" crates/service_utils/src -SRepository: juspay/superposition
Length of output: 4043
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "pub fn get_from_env_or_default|fn get_from_env_or_default|pub async fn decrypt|fn decrypt" crates/service_utils/src -SRepository: juspay/superposition
Length of output: 820
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper implementations that could return an empty token.
rg -n "get_from_env_or_default|decrypt" crates/service_utils/src -S
echo
echo '--- helpers ---'
sed -n '1,220p' crates/service_utils/src/helpers.rs
echo
echo '--- kms ---'
sed -n '1,220p' crates/service_utils/src/aws/kms.rsRepository: juspay/superposition
Length of output: 14516
Reject blank KRONOS_DISPATCH_TOKEN values
get_from_env_or_default accepts "", so a blank KRONOS_DISPATCH_TOKEN in DEV/TEST becomes a valid dispatch token and lets Internal through to /dispatch/webhook without a secret. Reject empty values before storing it in AppState.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/service_utils/src/middlewares/auth_n.rs` around lines 112 - 114,
Reject blank KRONOS_DISPATCH_TOKEN values before storing the token in AppState:
update the get_from_env_or_default handling used to initialize AppState so empty
or whitespace-only values are treated as missing/invalid and cannot satisfy the
Internal dispatch webhook match in the auth middleware.
| (Some("Internal"), Some(token)) | ||
| if token == state.kronos_dispatch_token | ||
| && request.path().ends_with("/dispatch/webhook") => | ||
| { | ||
| request | ||
| .extensions_mut() | ||
| .insert::<DispatchUser>(DispatchUser); | ||
| Some(Ok(User::new( | ||
| "[email protected]".to_string(), | ||
| "kronos-dispatcher".to_string(), | ||
| ))) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add startup validation that kronos_dispatch_token differs from superposition_token
If both tokens are set to the same value, the first match arm (line 96) always wins because Rust evaluates match guards in order. This breaks the dispatch endpoint in both scenarios:
- With
x-userheader:InternalUseris inserted instead ofDispatchUser, soDispatchUserContextreturnsfalseand the handler rejects the request. - Without
x-userheader: The first arm's body returnsNone(theand_thenchain fails on missingx-user). The match expression evaluates toNone— it does not fall through to the second arm — so the dispatch token is never recognized and fallback auth is used.
A startup panic in app_state.rs (where both tokens are retrieved) is the simplest safeguard:
🔒 Suggested validation in app_state.rs
let superposition_token = get_superposition_token(kms_client, &app_env).await;
let kronos_dispatch_token = get_kronos_dispatch_token(kms_client, &app_env).await;
+
+if superposition_token == kronos_dispatch_token {
+ panic!(
+ "KRONOS_DISPATCH_TOKEN must differ from SUPERPOSITION_TOKEN; \
+ identical values break dispatch endpoint authentication"
+ );
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/service_utils/src/middlewares/auth_n.rs` around lines 112 - 123, Add
startup validation in app_state.rs where superposition_token and
kronos_dispatch_token are retrieved: assert or otherwise fail initialization
when both configured token values are identical, while allowing distinct values
and unset configuration as appropriate. Locate the application state
initialization logic and ensure the validation runs before the service starts.
| Ok(user) | ||
| }) | ||
| } | ||
| (Some("Internal"), Some(token)) |
There was a problem hiding this comment.
Don't use internal for kronos, since the callback is external. Basic, bearer, etc should be more appropriate
| if !*dispatch_caller { | ||
| return Err(unexpected_error!( | ||
| "Unauthorized: only internal callers may use this endpoint" | ||
| "Unauthorized: only the Kronos dispatcher may use this endpoint" | ||
| )); | ||
| } |
There was a problem hiding this comment.
should we gate webhooks to only be called by kronos? What if the end user has their own system to do this - we should re-think this block
There was a problem hiding this comment.
okay gating it for users other than kronos and auth users.
There was a problem hiding this comment.
You mean unauthenticated users? They would already be gated right?
| /// Marker for callers authenticated with the Kronos dispatch token. Unlike | ||
| /// `InternalUser`, this grants no authz bypass — it only unlocks the webhook | ||
| /// dispatch endpoint. | ||
| pub struct DispatchUser; | ||
|
|
||
| #[derive(Deref, Default)] | ||
| pub struct DispatchUserContext(bool); | ||
|
|
||
| #[cfg(feature = "server")] | ||
| impl FromRequest for DispatchUserContext { | ||
| type Error = actix_web::error::Error; | ||
| type Future = Ready<Result<Self, Self::Error>>; | ||
|
|
||
| fn from_request(req: &HttpRequest, _: &mut Payload) -> Self::Future { | ||
| ready(Ok(Self(req.extensions().get::<DispatchUser>().is_some()))) | ||
| } | ||
| } |
There was a problem hiding this comment.
You won't need all this if kronos uses bearer and is just treated like any other system
There was a problem hiding this comment.
bearer can collide with user tokens
There was a problem hiding this comment.
Basic or anything else. Point being no need to treat kronos as a special entity. It flows through the same auth flow as all human and service entities
mahatoankitkumar
force-pushed
the
feat/kronos-implementation
branch
2 times, most recently
from
8a94aec to
dbcd15f
Compare
| } | ||
| } | ||
|
|
||
| pub async fn get_kronos_dispatch_token( |
There was a problem hiding this comment.
nitpick: do we need this function? Could just be inlined
| KRONOS_ENCRYPTION_KEY="5d5e2e704c866bddeb6dd7ef1ea69f39a6eb3934f60e3f13f67c605b513ced9e" | ||
| KRONOS_DB_POOL_SIZE="1" No newline at end of file | ||
| KRONOS_DB_POOL_SIZE="1" | ||
|
|
| // Callers: the Kronos dispatcher (DispatchUser — bypasses casbin, see auth_z) | ||
| // or any user authorized for the "dispatch" action on Webhook in this workspace. |
There was a problem hiding this comment.
We don't need this comment. This is not special case.
| } | ||
| } | ||
|
|
||
| pub struct DispatchUser; |
There was a problem hiding this comment.
auth_n.rs
mahatoankitkumar
force-pushed
the
feat/kronos-implementation
branch
from
dbcd15f to
ebb2c23
Compare
This pull request introduces a scoped authentication mechanism for Kronos webhook dispatch callbacks, separating it from the existing internal token. It creates a new
KRONOS_DISPATCH_TOKENenvironment variable and associated code paths, ensuring that only the dispatch endpoint can be accessed with this token, without granting broader internal access. The changes also update dependency versions and improve documentation and code clarity around authentication.Authentication and Security Enhancements:
KRONOS_DISPATCH_TOKENenvironment variable and retrieval logic inget_kronos_dispatch_token, used exclusively for Kronos webhook dispatch callbacks, with clear separation from the broaderSUPERPOSITION_TOKEN(crates/service_utils/src/db/utils.rs,.env.example) [1] [2].DispatchUser, granting access only to the dispatch endpoint and not to internal APIs (crates/service_utils/src/middlewares/auth_n.rs,crates/service_utils/src/service/types.rs,crates/superposition_types/src/lib.rs) [1] [2] [3] [4].DispatchUserContextinstead ofInternalUserContext, ensuring only properly authenticated requests can trigger dispatch (crates/superposition/src/dispatch/handlers.rs) [1] [2].Dependency and Configuration Updates:
kronos-workerandkronos-commondependencies to a newer commit, ensuring compatibility with the latest Kronos features (Cargo.toml).Code and Documentation Improvements:
crates/service_utils/src/kronos_dispatch.rs,crates/superposition/src/app_state.rs,crates/superposition/src/workspace/handlers.rs) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10].These changes collectively enhance security by scoping the Kronos dispatch token, reduce the risk of privilege escalation, and clarify the authentication flow for dispatch callbacks.
Summary by CodeRabbit