Feat/mail accessibility - #2
Merged
Merged
Conversation
Reviewer's GuideAdds HTTP-visible job state inspection for the mail queue, introduces first-boot auth/bootstrap helpers (API key + JWT secret), enhances observability and safety around Postgres/logging, and updates API types/routes/OpenAPI, config, and tests accordingly. Sequence diagram for GET /mail/jobs/{id} job state lookupsequenceDiagram
actor Client
participant Router as AxumRouter
participant MailRoutes as get_job_state
participant AppStateArc as AppState
participant QueueHandle as QueueHandle
participant Storage as PostgresStorage_MailJob
Client->>Router: HTTP GET /mail/jobs/{id}
Router->>MailRoutes: dispatch get_job_state(id)
MailRoutes->>AppStateArc: clone queue handle
MailRoutes->>QueueHandle: fetch(id_str)
QueueHandle->>QueueHandle: parse TaskId from id_str
alt invalid TaskId
QueueHandle-->>MailRoutes: Err QueueError_InvalidId
MailRoutes-->>Client: 400 BadRequest
else valid TaskId
QueueHandle->>Storage: fetch_by_id(task_id)
alt storage error
Storage-->>QueueHandle: Err
QueueHandle-->>MailRoutes: Err QueueError_Fetch_or_Sqlx
MailRoutes-->>Client: 500 Internal
else job not found
Storage-->>QueueHandle: Ok None
QueueHandle-->>MailRoutes: Ok None
MailRoutes-->>Client: 404 NotFound
else job found
Storage-->>QueueHandle: Ok Request_MailJob_SqlContext
QueueHandle->>QueueHandle: snapshot_from_request
QueueHandle-->>MailRoutes: Ok Some JobSnapshot
MailRoutes->>MailRoutes: convert JobSnapshot into JobStateResponse
MailRoutes-->>Client: 200 OK JSON JobStateResponse
end
end
Sequence diagram for first-boot auth bootstrap on startupsequenceDiagram
participant Main as main
participant Config as AppConfig
participant Bootstrap as maybe_bootstrap_auth
participant BootstrapMod as AuthBootstrapModule
Main->>Config: AppConfig::load()
Config-->>Main: cfg
Main->>Bootstrap: maybe_bootstrap_auth(cfg_mut)
alt bootstrap disabled or api_keys not empty
Bootstrap-->>Main: return
else bootstrap enabled and no api_keys
Bootstrap->>BootstrapMod: generate_bootstrap_key(BOOTSTRAP_KEY_ID)
alt key generation fails
BootstrapMod-->>Bootstrap: Err ApiKeyError
Bootstrap-->>Main: log error and return
else key generation ok
BootstrapMod-->>Bootstrap: BootstrapKey
alt jwt_secret is default
Bootstrap->>BootstrapMod: generate_jwt_secret()
BootstrapMod-->>Bootstrap: new_secret
Bootstrap->>Bootstrap: set cfg.auth.jwt_secret
else jwt_secret already set
Bootstrap->>Bootstrap: keep existing jwt_secret
end
Bootstrap->>Bootstrap: insert api_keys[key.id] = key.hash
Bootstrap->>BootstrapMod: print_bootstrap_banner(key, maybe_secret)
BootstrapMod-->>Bootstrap: done
Bootstrap-->>Main: return
end
end
Main->>Main: continue startup with configured auth
Class diagram for updated queue, API job state, and auth bootstrap typesclassDiagram
class MailJob {
+uuid_Uuid id
+MailJobKind kind
}
class QueueError {
+Migrate String
+Push String
+InvalidId String
+Fetch String
+Worker String
+Sqlx sqlx_Error
}
class JobSnapshot {
+String task_id
+uuid_Uuid mail_id
+String status
+usize attempts
+i32 max_attempts
+Option_String last_error
+DateTime_Utc run_at
+Option_i64 lock_at
+Option_i64 done_at
}
class QueueHandle {
-PostgresStorage_MailJob storage
+push(job MailJob) Result_String_QueueError
+fetch(task_id &str) Result_Option_JobSnapshot__QueueError
}
class QueueRuntime {
+start(app_cfg AppConfig, deps WorkerDeps) Result_QueueRuntime_QueueError
}
class EnqueuedResponse {
+String job_id
+String status
}
class JobStateResponse {
+String task_id
+uuid_Uuid mail_id
+String status
+usize attempts
+i32 max_attempts
+Option_String last_error
+DateTime_Utc run_at
+Option_i64 lock_at
+Option_i64 done_at
}
class AuthConfig {
+String jwt_secret
+String jwt_issuer
+i64 jwt_ttl_secs
+HashMap_String_String api_keys
+bool bootstrap
}
class BootstrapKey {
+String id
+String plaintext
+String hash
}
class AuthBootstrapModule {
+generate_bootstrap_key(id String) Result_BootstrapKey_ApiKeyError
+generate_jwt_secret() String
+print_bootstrap_banner(key BootstrapKey, jwt_secret_generated Option_str)
}
class MailApiMain {
+maybe_bootstrap_auth(cfg AppConfig_mut)
+init_tracing(cfg AppConfig)
}
QueueHandle --> MailJob : enqueues
QueueHandle --> JobSnapshot : returns
QueueHandle --> QueueError : error_type
JobStateResponse ..> JobSnapshot : From_impl
EnqueuedResponse --> JobStateResponse : references_task_id
MailApiMain --> AuthConfig : reads_writes
MailApiMain --> AuthBootstrapModule : calls
AuthBootstrapModule --> BootstrapKey : returns
QueueRuntime --> QueueHandle : creates
QueueRuntime --> QueueError : error_type
AuthConfig --> BootstrapKey : stores_hash_via_api_keys
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The job status is currently represented as a plain
StringacrossJobSnapshot,JobStateResponse, and the API; consider introducing a smallenum(withSerialize/ToSchema) to avoid typos and make status handling more type-safe end-to-end. - The
DEFAULT_JWT_SECRETconstant inmailify-apimust stay in sync with the defaultjwt_secretvalue inAppConfig; consider centralizing this string in one crate or reusing the config default to avoid subtle bootstrap bugs if one changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The job status is currently represented as a plain `String` across `JobSnapshot`, `JobStateResponse`, and the API; consider introducing a small `enum` (with `Serialize`/`ToSchema`) to avoid typos and make status handling more type-safe end-to-end.
- The `DEFAULT_JWT_SECRET` constant in `mailify-api` must stay in sync with the default `jwt_secret` value in `AppConfig`; consider centralizing this string in one crate or reusing the config default to avoid subtle bootstrap bugs if one changes.
## Individual Comments
### Comment 1
<location path="crates/mailify-api/src/main.rs" line_range="95-98" />
<code_context>
+ return;
+ }
+
+ let key = match generate_bootstrap_key(BOOTSTRAP_KEY_ID) {
+ Ok(k) => k,
+ Err(e) => {
+ error!(error = %e, "failed to generate bootstrap api key");
+ return;
+ }
</code_context>
<issue_to_address>
**🚨 issue (security):** Bootstrap auth failure is only logged, leaving the server running without any configured API key.
In `maybe_bootstrap_auth`, if `generate_bootstrap_key` fails, we log and return but still start with an empty `api_keys` map even though `auth.bootstrap` is enabled. This undermines the expectation that a secured instance always has at least one key. Instead, treat this as a hard startup failure (e.g., return an error from `main` / abort) when bootstrap is requested but key generation fails, rather than continuing without auth material.
</issue_to_address>
### Comment 2
<location path="crates/mailify-queue/src/worker.rs" line_range="286-295" />
<code_context>
}
+
+/// Strip password from a `postgres://user:pass@host/db` URL so it is safe to log.
+fn redact_db_url(url: &str) -> String {
+ match (url.find("://"), url.find('@')) {
+ (Some(scheme_end), Some(at)) if scheme_end + 3 < at => {
+ let (prefix, rest) = url.split_at(scheme_end + 3);
+ let (creds, host) = rest.split_at(at - (scheme_end + 3));
+ let user = creds.split(':').next().unwrap_or("");
+ if user.is_empty() {
+ format!("{prefix}***{host}")
+ } else {
+ format!("{prefix}{user}:***{host}")
+ }
+ }
+ _ => url.to_string(),
+ }
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `redact_db_url` treats `postgres://user@host/db` as if it had a password, which may be misleading in logs.
For URLs like `postgres://user@localhost/db` (user but no password), `creds` is just `"user"`, so we log `postgres://user:***@localhost/db`, implying a password existed when it didn’t. That’s misleading for operators reading logs. Consider checking for `':'` in `creds` and only keeping the username when a password is present; otherwise, redact the whole credential segment as `***@host`.
Suggested implementation:
```rust
/// Strip password from a `postgres://user:pass@host/db` URL so it is safe to log.
///
/// If the URL has credentials without a password (e.g. `postgres://user@host/db`),
/// the entire credential segment is redacted as `***@host` to avoid implying a
/// password was present.
fn redact_db_url(url: &str) -> String {
match (url.find("://"), url.find('@')) {
(Some(scheme_end), Some(at)) if scheme_end + 3 < at => {
let (prefix, rest) = url.split_at(scheme_end + 3);
let (creds, host) = rest.split_at(at - (scheme_end + 3));
let has_password = creds.contains(':');
if has_password {
let user = creds.splitn(2, ':').next().unwrap_or("");
if user.is_empty() {
// No explicit user, just redact the whole credential segment.
format!("{prefix}***{host}")
} else {
// Keep username, redact only the password.
format!("{prefix}{user}:***{host}")
}
} else {
// Credentials without password: redact entire credential segment.
format!("{prefix}***{host}")
}
}
_ => url.to_string(),
}
}
```
Consider adding unit tests for `redact_db_url` covering cases like:
- `postgres://user:pass@localhost/db` → `postgres://user:***@localhost/db`
- `postgres://user@localhost/db` → `postgres://***@localhost/db`
- `postgres://@localhost/db` → `postgres://***@localhost/db`
- `postgres://localhost/db` (no creds) → unchanged.
These tests can live in the same file under a `#[cfg(test)]` module or in an appropriate test module for this crate.
</issue_to_address>
### Comment 3
<location path="crates/mailify-auth/src/bootstrap.rs" line_range="31-38" />
<code_context>
+ random_token(48)
+}
+
+fn random_token(byte_len: usize) -> String {
+ const ALPHABET: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
+ let mut bytes = vec![0u8; byte_len];
+ OsRng.fill_bytes(&mut bytes);
+ bytes
+ .into_iter()
+ .map(|b| ALPHABET[(b as usize) % ALPHABET.len()] as char)
+ .collect()
+}
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** `random_token` introduces modulo bias when mapping bytes into the allowed alphabet.
The mapping `ALPHABET[(b as usize) % ALPHABET.len()]` creates a non-uniform distribution because 256 is not divisible by `ALPHABET.len()`. Since these tokens are used for API keys / JWT secrets, consider using rejection sampling to ensure uniformity, e.g. only accept bytes `< ALPHABET.len() * (256 / ALPHABET.len())` and redraw otherwise. This removes the modulo bias with modest added complexity.
Suggested implementation:
```rust
fn random_token(len: usize) -> String {
const ALPHABET: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
// Rejection sampling to avoid modulo bias:
// Draw from a large uniform range and only keep values that fall within
// a multiple of ALPHABET.len(), then map with `% ALPHABET.len()`.
let mut rng = OsRng;
let alphabet_len = ALPHABET.len() as u32;
let zone = (u32::MAX / alphabet_len) * alphabet_len;
let mut out = String::with_capacity(len);
while out.len() < len {
let v = rng.next_u32();
if v < zone {
let idx = (v % alphabet_len) as usize;
out.push(ALPHABET[idx] as char);
}
}
out
}
```
None required, assuming `random_token` is only called via `generate_jwt_secret()` and similar call sites that do not depend on the parameter name change from `byte_len` to `len`. If there are direct callers relying on that parameter name (e.g. via macros), adjust the argument name accordingly, though in normal function calls this is not an issue in Rust.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+95
to
+98
| let key = match generate_bootstrap_key(BOOTSTRAP_KEY_ID) { | ||
| Ok(k) => k, | ||
| Err(e) => { | ||
| error!(error = %e, "failed to generate bootstrap api key"); |
There was a problem hiding this comment.
🚨 issue (security): Bootstrap auth failure is only logged, leaving the server running without any configured API key.
In maybe_bootstrap_auth, if generate_bootstrap_key fails, we log and return but still start with an empty api_keys map even though auth.bootstrap is enabled. This undermines the expectation that a secured instance always has at least one key. Instead, treat this as a hard startup failure (e.g., return an error from main / abort) when bootstrap is requested but key generation fails, rather than continuing without auth material.
Comment on lines
+286
to
+295
| fn redact_db_url(url: &str) -> String { | ||
| match (url.find("://"), url.find('@')) { | ||
| (Some(scheme_end), Some(at)) if scheme_end + 3 < at => { | ||
| let (prefix, rest) = url.split_at(scheme_end + 3); | ||
| let (creds, host) = rest.split_at(at - (scheme_end + 3)); | ||
| let user = creds.split(':').next().unwrap_or(""); | ||
| if user.is_empty() { | ||
| format!("{prefix}***{host}") | ||
| } else { | ||
| format!("{prefix}{user}:***{host}") |
There was a problem hiding this comment.
suggestion (bug_risk): redact_db_url treats postgres://user@host/db as if it had a password, which may be misleading in logs.
For URLs like postgres://user@localhost/db (user but no password), creds is just "user", so we log postgres://user:***@localhost/db, implying a password existed when it didn’t. That’s misleading for operators reading logs. Consider checking for ':' in creds and only keeping the username when a password is present; otherwise, redact the whole credential segment as ***@host.
Suggested implementation:
/// Strip password from a `postgres://user:pass@host/db` URL so it is safe to log.
///
/// If the URL has credentials without a password (e.g. `postgres://user@host/db`),
/// the entire credential segment is redacted as `***@host` to avoid implying a
/// password was present.
fn redact_db_url(url: &str) -> String {
match (url.find("://"), url.find('@')) {
(Some(scheme_end), Some(at)) if scheme_end + 3 < at => {
let (prefix, rest) = url.split_at(scheme_end + 3);
let (creds, host) = rest.split_at(at - (scheme_end + 3));
let has_password = creds.contains(':');
if has_password {
let user = creds.splitn(2, ':').next().unwrap_or("");
if user.is_empty() {
// No explicit user, just redact the whole credential segment.
format!("{prefix}***{host}")
} else {
// Keep username, redact only the password.
format!("{prefix}{user}:***{host}")
}
} else {
// Credentials without password: redact entire credential segment.
format!("{prefix}***{host}")
}
}
_ => url.to_string(),
}
}Consider adding unit tests for redact_db_url covering cases like:
postgres://user:pass@localhost/db→postgres://user:***@localhost/dbpostgres://user@localhost/db→postgres://***@localhost/dbpostgres://@localhost/db→postgres://***@localhost/dbpostgres://localhost/db(no creds) → unchanged.
These tests can live in the same file under a#[cfg(test)]module or in an appropriate test module for this crate.
Comment on lines
+31
to
+38
| fn random_token(byte_len: usize) -> String { | ||
| const ALPHABET: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"; | ||
| let mut bytes = vec![0u8; byte_len]; | ||
| OsRng.fill_bytes(&mut bytes); | ||
| bytes | ||
| .into_iter() | ||
| .map(|b| ALPHABET[(b as usize) % ALPHABET.len()] as char) | ||
| .collect() |
There was a problem hiding this comment.
🚨 suggestion (security): random_token introduces modulo bias when mapping bytes into the allowed alphabet.
The mapping ALPHABET[(b as usize) % ALPHABET.len()] creates a non-uniform distribution because 256 is not divisible by ALPHABET.len(). Since these tokens are used for API keys / JWT secrets, consider using rejection sampling to ensure uniformity, e.g. only accept bytes < ALPHABET.len() * (256 / ALPHABET.len()) and redraw otherwise. This removes the modulo bias with modest added complexity.
Suggested implementation:
fn random_token(len: usize) -> String {
const ALPHABET: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
// Rejection sampling to avoid modulo bias:
// Draw from a large uniform range and only keep values that fall within
// a multiple of ALPHABET.len(), then map with `% ALPHABET.len()`.
let mut rng = OsRng;
let alphabet_len = ALPHABET.len() as u32;
let zone = (u32::MAX / alphabet_len) * alphabet_len;
let mut out = String::with_capacity(len);
while out.len() < len {
let v = rng.next_u32();
if v < zone {
let idx = (v % alphabet_len) as usize;
out.push(ALPHABET[idx] as char);
}
}
out
}None required, assuming random_token is only called via generate_jwt_secret() and similar call sites that do not depend on the parameter name change from byte_len to len. If there are direct callers relying on that parameter name (e.g. via macros), adjust the argument name accordingly, though in normal function calls this is not an issue in Rust.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Expose mail queue job state over HTTP and improve bootstrap, logging, and database connectivity behavior.
New Features:
Enhancements:
Documentation:
Tests: