feat(notif): add signed unsubscribe url to digest#2371
Merged
Conversation
Contributor
seanaye
commented
Apr 6, 2026
seanaye
commented
- move preferences into notification crate
- add hmac url signing
- add service urls crate
- feed hmac key into digest formatter
- remove unused templates
- update infra to contain secret
- add get handler for presigned urls
- update token extractors
- split out internal logic
- create decode jwt extractor
- move file into own crate
- put macro user id in decodedjwt
- use decoded jwt extractor in notification device
- update all notification handlers
- add more tests
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds HMAC-signed unsubscribe URLs and SignedUrl utilities; introduces a decode_jwt extractor crate and axum extractors; threads an unsubscribe HMAC secret from Pulumi into the notification router state and digest worker; adds notification preference endpoints (including presigned disables); refactors digest formatter types; and removes legacy HTML templates. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@rust/cloud-storage/authentication_service/src/api/middleware/extract_tokens.rs`:
- Around line 17-28: The extractor branches for access_token and refresh_token
currently swallow the original rejection and always return a 400 with "no ...
token"; update the Err arms in extract_tokens (the access_token and
refresh_token matches) to propagate the original rejection instead of
fabricating "no token" — e.g. return Err(e.into_response()) or match on the
rejection to only map genuine "missing" cases to your custom 400, otherwise
forward the existing rejection from the extractor so malformed/invalid-token
errors are preserved.
In `@rust/cloud-storage/decode_jwt/Cargo.toml`:
- Around line 20-21: The dependency serde_urlencoded is declared directly in
this crate's [dev-dependencies]; to align with the workspace you should remove
the pinned serde_urlencoded = "0.7" here and instead add serde_urlencoded =
"0.7" (or the desired version spec) into the workspace's
[workspace.dependencies], then update this crate's Cargo.toml to reference the
workspace-managed dependency (i.e., no version here) so all crates use the
consistent workspace version; locate the entry for serde_urlencoded in this
crate's Cargo.toml and the workspace Cargo.toml and make the change.
In `@rust/cloud-storage/decode_jwt/src/lib.rs`:
- Around line 89-111: The local_auth branch panics by calling
MacroUserIdStr::parse_from_str(...).map(CowLike::into_owned).expect(...) and
using unwrap() when parsing LOCAL_ORG_ID; change these to propagate a
descriptive error instead of panicking: replace the expect and unwrap usage in
the block that builds and returns DecodedJwt/UserContext so that parse_from_str
and .parse() return Err(...) (with clear messages identifying which env var
failed) and the function returns that Err variant (rather than panicking),
preserving the same return type (Result<DecodedJwt, _>); ensure you reference
MacroUserIdStr::parse_from_str, CowLike::into_owned, DecodedJwt, and UserContext
when making the change.
In `@rust/cloud-storage/email_formatting/src/lib.rs`:
- Around line 59-63: The function new_from_digest_batch currently moves the
DigestBatch by destructuring it (e.g., in the pattern that uses user_id: _) then
later tries to read digest.user_id, causing a use-after-move compile error;
change the destructuring to borrow or clone fields you need (or destructure into
variables that keep ownership for later use) so digest remains available for
accessing digest.user_id, and replace the placeholder "/todo" path with the
actual endpoint constant or a function-generated path (update any call sites
that construct the URL) so the final email/link uses the correct route instead
of "/todo".
- Around line 88-94: The code sets contacts_url.set_path("/todo") which produces
invalid signed unsubscribe links; replace the placeholder with the real
unsubscribe endpoint used by the notification/contacts service (find the
notification service's unsubscribe route in its API docs/code) and update
contacts_url.set_path(...) accordingly so SignedUrl::new(contacts_url, sha)
points to a valid handler; check the surrounding variables
(env.contacts_service(), contacts_url, digest.user_id, sha, and SignedUrl::new)
to ensure the query param and signing still apply to the correct unsubscribe
path.
In `@rust/cloud-storage/macro_auth/src/headers.rs`:
- Line 17: Public extractor types (AccessTokenCookieExtractor,
RefreshTokenCookieExtractor, AccessTokenExtractor, RefreshTokenHeaderExtractor,
RefreshTokenExtractor) are missing doc comments; add concise Rust doc comments
(/// ...) above each pub struct to describe its purpose, what it extracts
(cookie vs header), authentication semantics, and what rejection/errors callers
can expect (e.g., missing or invalid token -> rejection type). Ensure each
comment is placed immediately above the corresponding type declaration (e.g.,
pub struct AccessTokenCookieExtractor(pub Cookie<'static>);) and keep
descriptions brief and consistent across all five extractors.
- Line 55: Fix the typo in the expect message used when extracting the
CookieJar: change the string "This extractor is infallibe" to "This extractor is
infallible" in the expression that creates `jar: CookieJar` (the
`parts.extract().await.expect(...)` call) in headers.rs so the error message is
spelled correctly.
- Line 29: Fix the typo in the expect message on the CookieJar extraction: in
headers.rs locate the line using parts.extract().await.expect(...) where the
variable jar: CookieJar is assigned and change the string "infallibe" to
"infallible" so the expect message reads correctly.
In `@rust/cloud-storage/macro_service_urls/Cargo.toml`:
- Around line 1-9: Add the crate-level attribute to enforce documentation by
adding #![deny(missing_docs)] at the top of the library root (lib.rs); open
lib.rs and insert the attribute as the first non-comment line so the compiler
will require docs for all public items (modules, structs, enums, functions)
exported by the macro_service_urls crate.
In `@rust/cloud-storage/macro_service_urls/src/lib.rs`:
- Around line 1-5: Add the crate-level documentation lint by inserting the
attribute to deny missing docs at the top of the crate root (lib.rs) and include
a short crate-level doc comment; specifically, add #![deny(missing_docs)] as the
very first item in lib.rs and a concise //! crate-level doc comment describing
the purpose of the macro_service_urls crate so the compiler enforces
documentation on all public items exported by this crate.
In `@rust/cloud-storage/notification_sandbox/src/main.rs`:
- Around line 373-387: The sandbox currently creates the HMAC key with a
hardcoded [0; 32] inside digest_to_sandbox, causing generated links to differ
from the service's UNSUBSCRIBE_HMAC_SECRET; modify digest_to_sandbox to read the
configured unsubscribe HMAC secret (same source used in
rust/cloud-storage/notification_service/src/main.rs or the shared config/env var
name UNSUBSCRIBE_HMAC_SECRET), derive the 32-byte key (e.g. from the secret
bytes or a decoded value), construct Hmac::<sha2::Sha256>::new_from_slice(...)
with that secret, and pass it into
EmailDigestNotification::new_from_digest_batch so sandbox links are signed with
the configured secret and validate against the local service.
In `@rust/cloud-storage/notification_service/src/api/mod.rs`:
- Around line 61-67: The unsubscribe handlers get_unsubscribes, unsubscribe_all,
unsubscribe_item, remove_unsubscribe_all, remove_unsubscribe_item, and
unsubscribe_email still expect Extension<UserContext> but the router around
ApiContext (in mod.rs where middleware is built with
macro_middleware::connection_drop_prevention_handler) no longer installs auth
middleware; either re-add the authentication layer to the ServiceBuilder used to
construct the Router so Extension<UserContext> is provided, or update each
handler signature to accept DecodedJwt (and map that to a UserContext inside the
handler) and remove the Extension<UserContext> dependency; locate the middleware
construction and the route registrations in mod.rs and apply one consistent fix
across all six handlers.
In `@rust/cloud-storage/notification/src/domain/models/signing.rs`:
- Around line 19-23: The new() method currently MACs u.as_str() verbatim while
verify() reconstructs the unsigned URL via query_pairs()/extend_pairs(), causing
inconsistent normalization; fix by extracting a shared canonicalization routine
(used by compute_mac or a new helper) that removes SIG_PARAM, iterates
query_pairs(), and serializes the URL in the normalized form, then call that
routine from both Signing::new() and Signing::verify() (and have compute_mac
accept the canonicalized string/Url), and add a regression test that signs a raw
pre-encoded query string and verifies it to ensure both paths use identical
canonicalization.
In `@rust/cloud-storage/notification/src/inbound/http/device.rs`:
- Around line 24-29: The tracing span for register_device currently exposes raw
PII by using fields(user_id=?macro_user_id); update the instrumentation to avoid
logging the raw macro_user_id from DecodedJwt by either removing the
fields(user_id=?macro_user_id) entry from the #[tracing::instrument(...)]
attribute or replace it with a non-PII representation (e.g., a hashed/redacted
value computed from macro_user_id before the span is created) and reference that
safe identifier in the attribute; ensure changes target the register_device
function and the DecodedJwt/macro_user_id usage so logs no longer contain the
raw email-like identifier.
In `@rust/cloud-storage/notification/src/inbound/http/mod.rs`:
- Around line 112-116: The GET route must not perform the unsubscribe mutation;
update routing so presigned_disable_notification_type only renders a
confirmation page (no state change) and move the actual mutation into an
explicit non-GET handler (e.g., change disable_notification_type to accept POST
or DELETE or add a new confirm_disable_notification_type POST/DELETE handler)
that performs the write after the user confirms; ensure the GET presigned
handler validates the signature but does not call the mutation and the
POST/DELETE handler verifies the signature/confirmation token before calling
preferences::disable_notification_type to mutate state.
In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs`:
- Around line 121-134: The handler rebuilds the absolute URL using
Environment::new_or_prod().notification_service(), but SignedUrl::verify expects
the exact absolute URL string that was MACed when signing (the contacts-service
base), so verification fails if hosts differ; change the verification code in
this handler to construct the absolute URL using the same shared base used by
the signer (the contacts-service unsubscribe base) instead of
env.notification_service() — use the same config/value (the shared unsubscribe
base used during signing) when joining original_uri.path_and_query() before
calling SignedUrl::verify(..., state.hmac_signing_key.clone()) so the MAC input
matches exactly.
- Around line 47-50: Add the tracing failure instrumentation to the
Result-returning handlers by annotating each function with
#[tracing::instrument(err)]; specifically add this attribute above
get_notification_type_preferences, disable_notification_type,
presigned_disable_notification_type, disable_notification_type_inner, and
enable_notification_type so any returned Err is recorded with context. Ensure
the attribute is applied to the async function signatures (before the fn
keyword) and that the tracing crate is in scope (use tracing::instrument or
fully qualify as shown) so compilation and standardized failure telemetry are
preserved.
- Around line 120-145: Change presigned_disable_notification_type to return
Result<Html<String>, (StatusCode, Html<String>)> and update every error branch
to include an appropriate StatusCode: return (StatusCode::BAD_REQUEST,
Html("Invalid link".to_string())) when to_verify fails,
(StatusCode::BAD_REQUEST, Html("Invalid signature".to_string())) when
SignedUrl::verify(to_verify, state.hmac_signing_key.clone()) fails, and convert
the .map_err on disable_notification_type_inner to produce
(StatusCode::INTERNAL_SERVER_ERROR, Html(message.to_string())) (or another
4xx/5xx as appropriate for the backend failure). Keep the success path returning
Html(...) as Ok(...). Reference the functions/symbols
presigned_disable_notification_type, SignedUrl::verify,
disable_notification_type_inner, state.hmac_signing_key, original_uri, and
notification_service_url when making the edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ccf3584-76bf-4e31-968b-b16842cc55e4
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (53)
infra/stacks/notification-service/Pulumi.dev.yamlinfra/stacks/notification-service/Pulumi.prod.yamlinfra/stacks/notification-service/index.tsrust/cloud-storage/Cargo.tomlrust/cloud-storage/authentication_service/src/api/middleware/extract_tokens.rsrust/cloud-storage/decode_jwt/Cargo.tomlrust/cloud-storage/decode_jwt/src/lib.rsrust/cloud-storage/decode_jwt/src/test.rsrust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rsrust/cloud-storage/email_formatting/Cargo.tomlrust/cloud-storage/email_formatting/src/lib.rsrust/cloud-storage/email_formatting/templates/digest.htmlrust/cloud-storage/github/Cargo.tomlrust/cloud-storage/macro_auth/Cargo.tomlrust/cloud-storage/macro_auth/src/headers.rsrust/cloud-storage/macro_middleware/Cargo.tomlrust/cloud-storage/macro_middleware/src/auth/attach_user.rsrust/cloud-storage/macro_middleware/src/auth/decode_jwt.rsrust/cloud-storage/macro_service_urls/Cargo.tomlrust/cloud-storage/macro_service_urls/src/lib.rsrust/cloud-storage/macro_service_urls/src/test.rsrust/cloud-storage/notification/Cargo.tomlrust/cloud-storage/notification/src/domain/models.rsrust/cloud-storage/notification/src/domain/models/signing.rsrust/cloud-storage/notification/src/domain/models/signing/test.rsrust/cloud-storage/notification/src/domain/ports.rsrust/cloud-storage/notification/src/domain/service/egress.rsrust/cloud-storage/notification/src/domain/service/test.rsrust/cloud-storage/notification/src/inbound/http/device.rsrust/cloud-storage/notification/src/inbound/http/mod.rsrust/cloud-storage/notification/src/inbound/http/preferences.rsrust/cloud-storage/notification/src/inbound/http/test.rsrust/cloud-storage/notification/src/inbound/worker.rsrust/cloud-storage/notification_sandbox/Cargo.tomlrust/cloud-storage/notification_sandbox/src/main.rsrust/cloud-storage/notification_service/Cargo.tomlrust/cloud-storage/notification_service/src/api/mod.rsrust/cloud-storage/notification_service/src/api/swagger.rsrust/cloud-storage/notification_service/src/api/user_notification.rsrust/cloud-storage/notification_service/src/config.rsrust/cloud-storage/notification_service/src/main.rsrust/cloud-storage/notification_service/src/openapi.rsrust/cloud-storage/notification_service/src/templates/channel_invite/_channel_invite_basic_template.htmlrust/cloud-storage/notification_service/src/templates/channel_invite/_channel_invite_message_template.htmlrust/cloud-storage/notification_service/src/templates/channel_invite/mod.rsrust/cloud-storage/notification_service/src/templates/channel_message/_channel_message_basic_template.htmlrust/cloud-storage/notification_service/src/templates/channel_message/_channel_message_named_template.htmlrust/cloud-storage/notification_service/src/templates/channel_message/mod.rsrust/cloud-storage/notification_service/src/templates/item_share/_item_share_template.htmlrust/cloud-storage/notification_service/src/templates/item_share/mod.rsrust/cloud-storage/notification_service/src/templates/mod.rsrust/cloud-storage/notification_service/src/templates/test.rsrust/cloud-storage/notification_service/src/templates/unread_message/_unread_message_template.html
💤 Files with no reviewable changes (12)
- rust/cloud-storage/notification_service/src/openapi.rs
- rust/cloud-storage/notification_service/src/templates/test.rs
- rust/cloud-storage/notification_service/src/templates/item_share/_item_share_template.html
- rust/cloud-storage/notification_service/src/templates/channel_invite/mod.rs
- rust/cloud-storage/notification_service/src/templates/channel_invite/_channel_invite_message_template.html
- rust/cloud-storage/notification_service/src/templates/channel_message/_channel_message_named_template.html
- rust/cloud-storage/notification_service/src/templates/channel_message/mod.rs
- rust/cloud-storage/notification_service/src/templates/channel_invite/_channel_invite_basic_template.html
- rust/cloud-storage/notification_service/src/templates/unread_message/_unread_message_template.html
- rust/cloud-storage/notification_service/src/templates/mod.rs
- rust/cloud-storage/notification_service/src/templates/item_share/mod.rs
- rust/cloud-storage/notification_service/src/templates/channel_message/_channel_message_basic_template.html
Comment on lines
+17
to
+28
| let access_token = match access_token { | ||
| Ok(access_token) => access_token.as_ref().to_string(), | ||
| Err(_e) => { | ||
| return Err((StatusCode::BAD_REQUEST, "no access token to refresh").into_response()); | ||
| } | ||
| }; | ||
|
|
||
| let refresh_token = match macro_auth::headers::extract_refresh_token_from_request_headers( | ||
| headers, | ||
| ) { | ||
| Ok(refresh_token) => refresh_token, | ||
| Err(e) => match e { | ||
| MacroAuthError::NoRefreshTokenProvided => { | ||
| tracing::error!(error=?e, "missing refresh token"); | ||
| return Err( | ||
| (StatusCode::BAD_REQUEST, "no refresh token to refresh").into_response() | ||
| ); | ||
| } | ||
| _ => { | ||
| tracing::error!(error=?e, "unable to extract refresh token from request headers"); | ||
| return Err((StatusCode::INTERNAL_SERVER_ERROR).into_response()); | ||
| } | ||
| }, | ||
| let refresh_token = match refresh_token { | ||
| Ok(refresh_token) => refresh_token.as_ref().to_string(), | ||
| Err(_e) => { | ||
| return Err((StatusCode::BAD_REQUEST, "no refresh token to refresh").into_response()); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't rewrite every extractor failure as “no token”.
access_token and refresh_token already carry a StatusCode rejection. These branches discard that signal and always return 400 Bad Request, so malformed/invalid tokens and real extractor failures now get reported as “no access/refresh token”. Preserve non-missing rejections instead of flattening them here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/authentication_service/src/api/middleware/extract_tokens.rs`
around lines 17 - 28, The extractor branches for access_token and refresh_token
currently swallow the original rejection and always return a 400 with "no ...
token"; update the Err arms in extract_tokens (the access_token and
refresh_token matches) to propagate the original rejection instead of
fabricating "no token" — e.g. return Err(e.into_response()) or match on the
rejection to only map genuine "missing" cases to your custom 400, otherwise
forward the existing rejection from the extractor so malformed/invalid-token
errors are preserved.
Comment on lines
+20
to
+21
| [dev-dependencies] | ||
| serde_urlencoded = "0.7" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using workspace dependency for serde_urlencoded.
The serde_urlencoded = "0.7" is pinned directly while other dependencies use workspace versions. If serde_urlencoded is used elsewhere in the workspace, consider adding it to [workspace.dependencies] for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/decode_jwt/Cargo.toml` around lines 20 - 21, The
dependency serde_urlencoded is declared directly in this crate's
[dev-dependencies]; to align with the workspace you should remove the pinned
serde_urlencoded = "0.7" here and instead add serde_urlencoded = "0.7" (or the
desired version spec) into the workspace's [workspace.dependencies], then update
this crate's Cargo.toml to reference the workspace-managed dependency (i.e., no
version here) so all crates use the consistent workspace version; locate the
entry for serde_urlencoded in this crate's Cargo.toml and the workspace
Cargo.toml and make the change.
Comment on lines
+59
to
+63
| pub fn new_from_digest_batch( | ||
| digest: DigestBatch, | ||
| env: Environment, | ||
| sha: Hmac<Sha256>, | ||
| ) -> Result<Self, Report> { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="rust/cloud-storage/email_formatting/src/lib.rs"
# Show the moved binding and later usage together.
nl -ba "$FILE" | sed -n '60,100p'
# Confirm both patterns exist in the same function.
rg -n --type=rust -C2 'let DigestBatch \{|digest\.user_id|append_pair\("id"' "$FILE"Repository: macro-inc/macro
Length of output: 101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="rust/cloud-storage/email_formatting/src/lib.rs"
echo "=== Lines 55-100 of the file ==="
cat -n "$FILE" | sed -n '55,100p'Repository: macro-inc/macro
Length of output: 1737
Compile blocker: digest is used after move.
DigestBatch is destructured at lines 64–68 with user_id: _ (discarding it), consuming the digest binding. Line 92 then attempts to access digest.user_id, which is invalid after the move and will fail compilation.
Proposed fix
let DigestBatch {
- user_id: _,
+ user_id,
notifications,
..
} = digest;
@@
let mut contacts_url = env.contacts_service();
contacts_url.set_path("/todo");
contacts_url
.query_pairs_mut()
- .append_pair("id", digest.user_id.as_ref())
+ .append_pair("id", user_id.as_ref())Additionally, the hardcoded /todo path at line 89 appears to be a placeholder and should be replaced with the actual endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/email_formatting/src/lib.rs` around lines 59 - 63, The
function new_from_digest_batch currently moves the DigestBatch by destructuring
it (e.g., in the pattern that uses user_id: _) then later tries to read
digest.user_id, causing a use-after-move compile error; change the destructuring
to borrow or clone fields you need (or destructure into variables that keep
ownership for later use) so digest remains available for accessing
digest.user_id, and replace the placeholder "/todo" path with the actual
endpoint constant or a function-generated path (update any call sites that
construct the URL) so the final email/link uses the correct route instead of
"/todo".
Comment on lines
+47
to
+50
| pub async fn get_notification_type_preferences<S: NotificationReader>( | ||
| State(state): State<NotificationRouterState<S>>, | ||
| decoded_jwt: DecodedJwt, | ||
| ) -> Result<Json<GetNotificationTypePreferencesResponse>, (StatusCode, Json<ErrorResponse<'static>>)> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n rust/cloud-storage/notification/src/inbound/http/preferences.rs | head -220Repository: macro-inc/macro
Length of output: 9100
🏁 Script executed:
grep -n "tracing::instrument" rust/cloud-storage/notification/src/inbound/http/preferences.rsRepository: macro-inc/macro
Length of output: 41
Add #[tracing::instrument(err)] to all Result-returning functions.
All five functions in this handler module return Result but none are annotated with #[tracing::instrument(err)]: get_notification_type_preferences (line 47), disable_notification_type (line 89), presigned_disable_notification_type (line 113), disable_notification_type_inner (line 149), and enable_notification_type (line 202). This leaves the preference/unsubscribe flow without the standard failure instrumentation this repo expects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs` around lines
47 - 50, Add the tracing failure instrumentation to the Result-returning
handlers by annotating each function with #[tracing::instrument(err)];
specifically add this attribute above get_notification_type_preferences,
disable_notification_type, presigned_disable_notification_type,
disable_notification_type_inner, and enable_notification_type so any returned
Err is recorded with context. Ensure the attribute is applied to the async
function signatures (before the fn keyword) and that the tracing crate is in
scope (use tracing::instrument or fully qualify as shown) so compilation and
standardized failure telemetry are preserved.
rust/cloud-storage/notification/src/inbound/http/preferences.rs
Outdated
Show resolved
Hide resolved
Comment on lines
+121
to
+134
| let env = Environment::new_or_prod(); | ||
| let notification_service_url = env.notification_service(); | ||
| let to_verify = notification_service_url.join( | ||
| original_uri | ||
| .path_and_query() | ||
| .map(PathAndQuery::as_str) | ||
| .unwrap_or("/"), | ||
| ); | ||
|
|
||
| let Ok(to_verify) = to_verify else { | ||
| return Err(Html("Invalid link".to_string())); | ||
| }; | ||
|
|
||
| let Some(_verified) = SignedUrl::verify(to_verify, state.hmac_signing_key.clone()) else { |
Contributor
There was a problem hiding this comment.
Sign and verify against the same absolute URL.
SignedUrl::verify MACs the full absolute URL string, but this handler always rebuilds that string with env.notification_service(). The signing side currently uses the contacts-service base, so valid digest unsubscribe links will be rejected as Invalid signature whenever those hosts differ. Please source the unsubscribe base URL from the same shared value on both the signing and verification paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs` around lines
121 - 134, The handler rebuilds the absolute URL using
Environment::new_or_prod().notification_service(), but SignedUrl::verify expects
the exact absolute URL string that was MACed when signing (the contacts-service
base), so verification fails if hosts differ; change the verification code in
this handler to construct the absolute URL using the same shared base used by
the signer (the contacts-service unsubscribe base) instead of
env.notification_service() — use the same config/value (the shared unsubscribe
base used during signing) when joining original_uri.path_and_query() before
calling SignedUrl::verify(..., state.hmac_signing_key.clone()) so the MAC input
matches exactly.
seanaye
force-pushed
the
seanaye/feat/hmac-unsubscribing
branch
from
b2696b9 to
e892407
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/decode_jwt/src/lib.rs`:
- Around line 55-77: Change the extractor so its Rejection is a single
DecodeJwtError instead of Either<DecodeJwtError, StatusCode>: update the impl
FromRequestParts<S> for DecodedJwt to use type Rejection = DecodeJwtError, and
when extracting (the parts.extract().await call that returns
(Result<AccessTokenExtractor, StatusCode>, Query<Params>)) map any extraction
failure into a DecodeJwtError variant (e.g., DecodeJwtError::InternalError or a
new Variant) before returning, then propagate Self::new(access_token, params,
&args).map_err(...) returning the unified DecodeJwtError; keep references to
DecodedJwt, FromRequestParts, JwtValidationArgs::from_ref, AccessTokenExtractor
and Self::new to locate the code to change.
In `@rust/cloud-storage/notification/src/inbound/http/mod.rs`:
- Line 316: The assignment uses decoded_jwt.macro_user_id.clone() where
MacroUserIdStr is Copy, so remove the unnecessary clone: set user_id to
decoded_jwt.macro_user_id directly (ensure the field type of user_id accepts
MacroUserIdStr); locate the occurrence in the code that builds the
struct/assignment (the user_id field using decoded_jwt.macro_user_id.clone())
and replace it with decoded_jwt.macro_user_id.
In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs`:
- Around line 155-169: The current unconventional `let true =
state.blockable_notification_typenames.contains(notification_event_type) else {
... }` should be replaced with a standard boolean check: use an `if` to test
membership on `state.blockable_notification_typenames` (or `if
!state.blockable_notification_typenames.contains(notification_event_type)` to
early-return) and keep the existing `Err((StatusCode::BAD_REQUEST,
Json(ErrorResponse { message: ... })))` payload unchanged; update the
conditional around `notification_event_type` so the logic and message remain the
same but uses a conventional `if`/`else` branch for readability.
In `@rust/cloud-storage/notification/src/inbound/http/test.rs`:
- Around line 557-576: Two tests, presigned_disable_succeeds_without_jwt and
presigned_disable_succeeds_with_valid_hmac, exercise the same successful
presigned-disable path; consolidate them by keeping a single success test
(preferably presigned_disable_succeeds_without_jwt which asserts the
"unsubscribed" body) and either remove the duplicate or change
presigned_disable_succeeds_with_valid_hmac to a distinct case (e.g., assert
HMAC-specific behavior like rejecting an invalid signature or ensuring the hmac
query param is required) using presigned_router() and signed_disable_uri() to
craft requests; update or remove the redundant test accordingly so each test has
a unique purpose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73c32707-b22b-4260-b773-8a48915592e5
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (10)
rust/cloud-storage/decode_jwt/src/lib.rsrust/cloud-storage/decode_jwt/src/test.rsrust/cloud-storage/macro_middleware/src/auth/decode_jwt.rsrust/cloud-storage/notification/Cargo.tomlrust/cloud-storage/notification/src/inbound/http/mod.rsrust/cloud-storage/notification/src/inbound/http/preferences.rsrust/cloud-storage/notification/src/inbound/http/test.rsrust/cloud-storage/notification_service/Cargo.tomlrust/cloud-storage/notification_service/src/api/mod.rsrust/cloud-storage/notification_service/src/api/user_notification.rs
Comment on lines
+155
to
+169
| let true = state | ||
| .blockable_notification_typenames | ||
| .contains(notification_event_type) | ||
| else { | ||
| return Err(( | ||
| StatusCode::BAD_REQUEST, | ||
| Json(ErrorResponse { | ||
| message: format!( | ||
| "Cannot block notification type {notification_event_type}. Expected one of {:?}", | ||
| state.blockable_notification_typenames | ||
| ) | ||
| .into(), | ||
| }), | ||
| )); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider more idiomatic pattern for boolean check.
The let true = ... else pattern is valid but unconventional. A standard if statement is more readable.
Suggested refactor
- // make sure the notification to block is one that matches the list
- let true = state
- .blockable_notification_typenames
- .contains(notification_event_type)
- else {
- return Err((
+ // make sure the notification to block is one that matches the list
+ if !state
+ .blockable_notification_typenames
+ .contains(notification_event_type)
+ {
+ return Err((
StatusCode::BAD_REQUEST,
Json(ErrorResponse {
message: format!(
"Cannot block notification type {notification_event_type}. Expected one of {:?}",
state.blockable_notification_typenames
)
.into(),
}),
));
- };
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs` around lines
155 - 169, The current unconventional `let true =
state.blockable_notification_typenames.contains(notification_event_type) else {
... }` should be replaced with a standard boolean check: use an `if` to test
membership on `state.blockable_notification_typenames` (or `if
!state.blockable_notification_typenames.contains(notification_event_type)` to
early-return) and keep the existing `Err((StatusCode::BAD_REQUEST,
Json(ErrorResponse { message: ... })))` payload unchanged; update the
conditional around `notification_event_type` so the logic and message remain the
same but uses a conventional `if`/`else` branch for readability.
Comment on lines
+557
to
+576
| #[tokio::test] | ||
| async fn presigned_disable_succeeds_without_jwt() { | ||
| let router = presigned_router(); | ||
| let uri = signed_disable_uri("test_type", "macro|user@example.com"); | ||
|
|
||
| let req = Request::builder() | ||
| .uri(&uri) | ||
| .method("GET") | ||
| .body(axum::body::Body::empty()) | ||
| .unwrap(); | ||
| let resp = router.oneshot(req).await.unwrap(); | ||
|
|
||
| assert_eq!(resp.status(), StatusCode::OK); | ||
| let body = resp.into_body().collect().await.unwrap().to_bytes(); | ||
| let text = String::from_utf8_lossy(&body); | ||
| assert!( | ||
| text.contains("unsubscribed"), | ||
| "expected success HTML, got: {text}" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating duplicate presigned URL tests.
presigned_disable_succeeds_without_jwt and presigned_disable_succeeds_with_valid_hmac test the same scenario—both send a validly signed request without a JWT and assert StatusCode::OK. The first test additionally checks the response body contains "unsubscribed". Consider removing the duplicate or differentiating their purposes.
Also applies to: 578-595
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/notification/src/inbound/http/test.rs` around lines 557 -
576, Two tests, presigned_disable_succeeds_without_jwt and
presigned_disable_succeeds_with_valid_hmac, exercise the same successful
presigned-disable path; consolidate them by keeping a single success test
(preferably presigned_disable_succeeds_without_jwt which asserts the
"unsubscribed" body) and either remove the duplicate or change
presigned_disable_succeeds_with_valid_hmac to a distinct case (e.g., assert
HMAC-specific behavior like rejecting an invalid signature or ensuring the hmac
query param is required) using presigned_router() and signed_disable_uri() to
craft requests; update or remove the redundant test accordingly so each test has
a unique purpose.
e892407 to
43920d7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
rust/cloud-storage/notification/src/inbound/http/mod.rs (1)
113-116:⚠️ Potential issue | 🟠 MajorDon't make the unsubscribe write happen on
GET.A valid signed email link can be fetched by scanners, previewers, or browser prefetch before the user clicks, which silently disables the notification type. Keep
GETread-only and move the mutation behind an explicitPOST/DELETEor a second confirmation step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/notification/src/inbound/http/mod.rs` around lines 113 - 116, The GET handler must not perform the unsubscribe write; change the routing so preferences::presigned_disable_notification_type (or a new read-only handler) only renders/returns a confirmation/preflight response and move the actual mutation into an explicit POST or DELETE handler (either repurpose preferences::disable_notification_type to be invoked only for POST/DELETE or add a new preferences::confirm_disable_notification_type) and update the route mapping so "/preferences/{notification_event_type}/disable" uses GET for read-only confirmation and POST/DELETE for the write that performs the disable.rust/cloud-storage/decode_jwt/src/lib.rs (1)
67-75:⚠️ Potential issue | 🟡 MinorExtract
Query<Params>separately to preserve client-error semantics.When
Query<Params>deserialization fails (malformed query string), axum rejects with400. But the current tuple extraction maps this to500, misclassifying a client error as a server error. Extract it as a separate parameter or useparts.extract()independently, as shown inmacro_middleware/src/auth/decode_jwt.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/decode_jwt/src/lib.rs` around lines 67 - 75, The tuple extraction currently maps any extract error (including Query<Params> deserialization failures) to a 500 via map_err(|_| Either::E2(...)); change the extraction to pull Query<Params> separately so client-side 400 rejections are preserved: call parts.extract().await for the AccessTokenExtractor and then parts.extract().await (or extract Query<Params> as its own let binding) instead of extracting the tuple, handle their Results independently and only map server errors to Either::E2 while returning Query deserialization errors unchanged; update the code around parts.extract(), AccessTokenExtractor, Query<Params>, and the Self::new(...).map_err(Either::E1) call accordingly so malformed queries still produce 400.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/decode_jwt/src/lib.rs`:
- Around line 217-218: The tracing error currently logs the rejected Macro user
id verbatim in DecodeJwtError::InvalidUserId (the tracing::error!(error=%id,
"invalid macro user id") call); change this to avoid logging PII by replacing
the verbatim id with either a generic message (e.g., "invalid macro user id
parse failure") or a redacted/hashed form of id (compute a stable hash or redact
local part) before logging so MacroUserIdStr / the InvalidUserId branch does not
emit raw email-like identifiers.
- Around line 89-91: The conditional that enables local-auth currently checks
both cfg!(feature = "local_auth") and std::env::var("LOCAL_USER_ID").is_ok(),
causing the local-auth branch in the code that constructs user_id (the block
using std::env::var("LOCAL_USER_ID").unwrap_or(...)) to be skipped when the env
var is missing; update the guard to only check cfg!(feature = "local_auth") so
the default identity is used when LOCAL_USER_ID is unset (i.e., remove the
std::env::var(...).is_ok() check while keeping the existing unwrap_or default
logic in the same block that sets user_id).
In `@rust/cloud-storage/notification/src/inbound/http/mod.rs`:
- Around line 99-120: The router() currently only registers /bulk and
/preferences, which leaves the existing handlers in this module for /item/bulk,
/item/{event_item_id} and /{notification_id} unreachable; update the
Router::new() construction in this module to re-add the missing routes (the
/item nest with its /bulk and /{event_item_id} routes and the top-level
/{notification_id} route) and wire them to the corresponding handler functions
already implemented in this file so those APIs stop returning 404.
---
Duplicate comments:
In `@rust/cloud-storage/decode_jwt/src/lib.rs`:
- Around line 67-75: The tuple extraction currently maps any extract error
(including Query<Params> deserialization failures) to a 500 via map_err(|_|
Either::E2(...)); change the extraction to pull Query<Params> separately so
client-side 400 rejections are preserved: call parts.extract().await for the
AccessTokenExtractor and then parts.extract().await (or extract Query<Params> as
its own let binding) instead of extracting the tuple, handle their Results
independently and only map server errors to Either::E2 while returning Query
deserialization errors unchanged; update the code around parts.extract(),
AccessTokenExtractor, Query<Params>, and the Self::new(...).map_err(Either::E1)
call accordingly so malformed queries still produce 400.
In `@rust/cloud-storage/notification/src/inbound/http/mod.rs`:
- Around line 113-116: The GET handler must not perform the unsubscribe write;
change the routing so preferences::presigned_disable_notification_type (or a new
read-only handler) only renders/returns a confirmation/preflight response and
move the actual mutation into an explicit POST or DELETE handler (either
repurpose preferences::disable_notification_type to be invoked only for
POST/DELETE or add a new preferences::confirm_disable_notification_type) and
update the route mapping so "/preferences/{notification_event_type}/disable"
uses GET for read-only confirmation and POST/DELETE for the write that performs
the disable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ff64fd9-7b95-4683-9df2-a5df46701343
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (6)
rust/cloud-storage/decode_jwt/src/lib.rsrust/cloud-storage/decode_jwt/src/test.rsrust/cloud-storage/macro_middleware/src/auth/decode_jwt.rsrust/cloud-storage/notification/Cargo.tomlrust/cloud-storage/notification/src/inbound/http/mod.rsrust/cloud-storage/notification/src/inbound/http/test.rs
Comment on lines
+217
to
+218
| DecodeJwtError::InvalidUserId(id) => { | ||
| tracing::error!(error=%id, "invalid macro user id"); |
Contributor
There was a problem hiding this comment.
Don't log the rejected Macro user id verbatim.
This identifier is email-based, so error=%id writes PII from attacker-controlled tokens into auth logs. Log a generic parse failure or a redacted/hash form instead.
Based on learnings, MacroUserIdStr::try_from_email(...) merely prepends macro| to an email string, so this identifier is email-based.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/decode_jwt/src/lib.rs` around lines 217 - 218, The tracing
error currently logs the rejected Macro user id verbatim in
DecodeJwtError::InvalidUserId (the tracing::error!(error=%id, "invalid macro
user id") call); change this to avoid logging PII by replacing the verbatim id
with either a generic message (e.g., "invalid macro user id parse failure") or a
redacted/hashed form of id (compute a stable hash or redact local part) before
logging so MacroUserIdStr / the InvalidUserId branch does not emit raw
email-like identifiers.
Comment on lines
+99
to
+120
| Router::new() | ||
| .nest( | ||
| "/bulk", | ||
| Router::new() | ||
| .route("/", delete(bulk_delete_notifications)) | ||
| .route("/seen", patch(bulk_mark_seen)) | ||
| .route("/done", patch(bulk_mark_done)) | ||
| .route("/undone", patch(bulk_mark_undone)), | ||
| ) | ||
| .route( | ||
| "/preferences", | ||
| get(preferences::get_notification_type_preferences), | ||
| ) | ||
| .route( | ||
| "/preferences/{notification_event_type}/disable", | ||
| put(preferences::disable_notification_type) | ||
| .get(preferences::presigned_disable_notification_type), | ||
| ) | ||
| .route( | ||
| "/preferences/{notification_event_type}/enable", | ||
| put(preferences::enable_notification_type), | ||
| ) |
Contributor
There was a problem hiding this comment.
Re-add the item and single-notification routes to router().
The builder now only wires /bulk and /preferences. That leaves the handlers below for /item/bulk, /item/{event_item_id}, and /{notification_id} unreachable, so those existing notification APIs will return 404 even though their handler implementations are still present in this module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/notification/src/inbound/http/mod.rs` around lines 99 -
120, The router() currently only registers /bulk and /preferences, which leaves
the existing handlers in this module for /item/bulk, /item/{event_item_id} and
/{notification_id} unreachable; update the Router::new() construction in this
module to re-add the missing routes (the /item nest with its /bulk and
/{event_item_id} routes and the top-level /{notification_id} route) and wire
them to the corresponding handler functions already implemented in this file so
those APIs stop returning 404.
221c010 to
eec9438
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_email.rs (1)
28-40:⚠️ Potential issue | 🟠 MajorAvoid logging email addresses (PII) in error messages.
Line 32 logs
email=?emailwhich writes the user's email address to logs. This is a compliance/privacy concern (GDPR/CCPA). Consider removing the email from the log or using a hashed/redacted form for debugging purposes.📝 Proposed fix
notification_db_client::unsubscribe::email::upsert_email_unsubscribe(&ctx.db, &email) .await .map_err(|e| { - tracing::error!(error=?e, email=?email, "unable to unsubscribe email"); + tracing::error!(error=?e, "unable to unsubscribe email"); ( StatusCode::INTERNAL_SERVER_ERROR, Json(ErrorResponse {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_email.rs` around lines 28 - 40, The tracing::error call in unsubscribe_email.rs currently includes the raw email (decoded_jwt.user_context.user_id) which exposes PII; change the error logging to avoid writing the plaintext email by computing and logging a non-reversible identifier (e.g., SHA256 hex or a redacted mask) before the call to notification_db_client::unsubscribe::email::upsert_email_unsubscribe, then replace email=?email in the tracing::error with the hashed/redacted identifier (or remove the email field entirely), ensuring no other logs or error responses include the raw email value.rust/cloud-storage/notification/src/inbound/http/device.rs (1)
49-56:⚠️ Potential issue | 🟠 MajorAuthorize device unregister against the caller.
DecodedJwtis still discarded here, and the service call only keys on{token, device_type}. Any authenticated user who learns another device token can unregister it. Threadmacro_user_idthroughunregister_deviceand delete by(user_id, token, device_type)instead.Suggested change
async fn unregister_device<S: NotificationReader>( State(state): State<NotificationRouterState<S>>, - _macro_user: DecodedJwt, + DecodedJwt { macro_user_id, .. }: DecodedJwt, Json(req): Json<DeviceRequest>, ) -> Result<Json<()>, (StatusCode, Json<ErrorResponse<'static>>)> { state .inner - .unregister_device(&req.token, &req.device_type) + .unregister_device(macro_user_id, &req.token, &req.device_type) .awaitThe
NotificationReadertrait and its implementations need the same parameter added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/notification/src/inbound/http/device.rs` around lines 49 - 56, Unregister handler currently drops DecodedJwt and calls state.inner.unregister_device(&req.token, &req.device_type), allowing any caller to delete any token; change the handler signature in unregister_device to extract the caller id from DecodedJwt (e.g., macro_user.id or macro_user_id) and pass it into the service call as state.inner.unregister_device(¯o_user_id, &req.token, &req.device_type). Update the NotificationReader trait method signature (unregister_device) to accept the user_id first and change all implementations accordingly to perform the deletion by (user_id, token, device_type).
♻️ Duplicate comments (5)
rust/cloud-storage/decode_jwt/src/lib.rs (2)
222-223:⚠️ Potential issue | 🟠 MajorAvoid logging PII in error messages.
The
error=%idlogs the user id verbatim, which contains an email address (format:macro|email@example.com). This writes PII to logs from potentially attacker-controlled tokens. Log a generic failure message or a redacted/hashed form instead.📝 Proposed fix
DecodeJwtError::InvalidUserId(id) => { - tracing::error!(error=%id, "invalid macro user id"); + tracing::error!("invalid macro user id format"); ( StatusCode::UNAUTHORIZED, Json(ErrorResponse {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/decode_jwt/src/lib.rs` around lines 222 - 223, In the DecodeJwtError::InvalidUserId(id) match arm (where tracing::error!(error=%id, "invalid macro user id") is called) avoid logging the raw `id` (PII); instead compute a non-PII representation (e.g., redact or hash the `id`) and log that representation or just a generic message; update the tracing::error call to use the redacted/hashed variable or drop the id altogether so no email/macro user id is written to logs.
89-91:⚠️ Potential issue | 🟡 MinorInconsistent
local_authguard logic.The guard checks
std::env::var("LOCAL_USER_ID").is_ok()but then line 91 usesunwrap_or("macro|orguser@org.com"). IfLOCAL_USER_IDmust be set to enter this branch, the default value can never be reached. Either remove theis_ok()check to allow the default, or remove theunwrap_orsince the variable is guaranteed to exist.📝 Proposed fix (remove the is_ok() check to allow default)
- if cfg!(feature = "local_auth") && std::env::var("LOCAL_USER_ID").is_ok() { + if cfg!(feature = "local_auth") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/decode_jwt/src/lib.rs` around lines 89 - 91, The cfg guard and env handling are inconsistent: keep the unwrap_or fallback but remove the existence check so the default can be used when LOCAL_USER_ID is unset; change the condition from `if cfg!(feature = "local_auth") && std::env::var("LOCAL_USER_ID").is_ok()` to just `if cfg!(feature = "local_auth")` and retain the `let user_id = std::env::var("LOCAL_USER_ID").unwrap_or("macro|orguser@org.com".to_string());` line so `user_id` gets the default when the env var is absent.rust/cloud-storage/notification/src/inbound/http/preferences.rs (1)
47-50:⚠️ Potential issue | 🟡 MinorAdd
#[tracing::instrument(err)]to the Result-returning preference handlers.These entry points all return
Result, but the module currently misses the repo-standard failure instrumentation.As per coding guidelines, "Use
#[tracing::instrument(err)]on functions returningResult."Also applies to: 89-95, 113-120, 154-158, 207-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs` around lines 47 - 50, The preference handler get_notification_type_preferences returns a Result but lacks failure tracing; add the repo-standard instrumentation by annotating it with #[tracing::instrument(err)] immediately above the function definition, and apply the same attribute to the other Result-returning preference handler functions in this module (the handlers corresponding to the ranges noted in the review) so failures get captured consistently by tracing.rust/cloud-storage/email_formatting/src/lib.rs (1)
64-68:⚠️ Potential issue | 🔴 CriticalFix the partial move of
digest.
notificationsis moved out at the destructuring, sodigest.user_idis no longer usable later. This won't compile.Suggested change
let DigestBatch { - user_id: _, + user_id, notifications, .. } = digest; @@ unsubscribe_url .query_pairs_mut() - .append_pair("id", digest.user_id.as_ref()) + .append_pair("id", user_id.as_ref()) .finish();Also applies to: 93-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/email_formatting/src/lib.rs` around lines 64 - 68, The destructuring "let DigestBatch { user_id: _, notifications, .. } = digest;" moves notifications out of digest and prevents later access to digest.user_id; fix by borrowing notifications instead of moving it (e.g., destructure from a reference or use "ref notifications" so notifications is a reference: "let DigestBatch { ref notifications, .. } = digest;" or "let DigestBatch { notifications, .. } = &digest;" ), and apply the same change at the other occurrence (lines ~93-97) so digest remains usable afterwards.rust/cloud-storage/notification/src/domain/models/signing.rs (1)
19-24: 🧹 Nitpick | 🔵 TrivialStrip any preexisting
sigbefore signing.
new()appendssigunconditionally. If a caller re-signs an already-signed URL, the result has twosigparams andverify()rejects it because it expects exactly one. FilteringSIG_PARAMout during canonicalization makes the API idempotent.Suggested change
fn canonicalize(u: &Url) -> Url { - let pairs: Vec<(Cow<'_, str>, Cow<'_, str>)> = u.query_pairs().collect(); + let pairs: Vec<(Cow<'_, str>, Cow<'_, str>)> = u + .query_pairs() + .filter(|(k, _)| k != SIG_PARAM) + .collect(); let mut canonical = u.clone(); canonical.query_pairs_mut().clear().extend_pairs(pairs).finish(); canonical }Also applies to: 27-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/notification/src/domain/models/signing.rs` around lines 19 - 24, The new() function currently appends SIG_PARAM without removing any existing sigs, causing duplicate sig parameters when re-signing; update canonicalize() (or perform a pre-sign filter in new()) to remove any existing query parameter named SIG_PARAM before computing the MAC so canonicalization is idempotent, then compute_mac(&canonical, mac) and append a single SIG_PARAM; reference the canonicalize, new, compute_mac, SIG_PARAM, and verify symbols to ensure canonicalize strips SIG_PARAM so verify() still sees exactly one signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/email_formatting/src/lib.rs`:
- Around line 93-97: The code currently appends the raw MacroUserIdStr
(digest.user_id) into the unsubscribe_url query (?id=...), exposing email-like
PII; instead produce an opaque identifier or signed token and put that in the
query. Replace the append_pair("id", digest.user_id.as_ref()) usage by creating
a derived non-email token (for example a short opaque signed token or stable
numeric id) from digest.user_id (do not use MacroUserIdStr::try_from_email
output directly) and append that token to the URL, then call
SignedUrl::new(unsubscribe_url, sha) as before; locate changes around
unsubscribe_url.query_pairs_mut().append_pair("id", ...) and SignedUrl::new to
implement this tokenization step.
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_item.rs`:
- Around line 26-31: The tracing span for the async handler function doesn't
record errors because the attribute lacks the err option; update the attribute
on the handler function (handler) from #[tracing::instrument(skip(ctx,
decoded_jwt))] to #[tracing::instrument(skip(ctx, decoded_jwt), err)] so
failures returned in Result<Response, Response> are attached to the span; locate
the attribute above pub async fn handler(...) and add the err flag.
In `@rust/cloud-storage/notification/src/inbound/http/device.rs`:
- Around line 24-29: The tracing spans on the device handlers don't capture
error context; update the tracing attributes to include err so failures are
recorded—e.g., change #[tracing::instrument(skip(state, macro_user_id, req))] on
register_device (and the sibling handler, e.g., unregister_device) to
#[tracing::instrument(skip(state, macro_user_id, req), err)] (preserving the
same skip list and generics) so the Result-returning functions record error
details in their spans.
In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs`:
- Around line 111-151: presigned_disable_notification_type currently performs a
state-changing unsubscribe on a GET, which must be changed to only verify the
signed URL and render a confirmation page on GET; move the actual call to
disable_notification_type_inner into a separate POST handler (e.g.,
presigned_disable_notification_type_confirm or reuse the same path with POST),
keep SignedUrl::verify(to_verify, state.hmac_signing_key.clone()) in the GET to
validate the link, render a safe confirmation HTML on GET, and have the POST
validate the same signed URL/params then call
disable_notification_type_inner(&state, params.id,
notification_event_type.as_str()).await and return the unsubscribe result;
ensure routes for GET vs POST are updated so only POST performs the mutation.
---
Outside diff comments:
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_email.rs`:
- Around line 28-40: The tracing::error call in unsubscribe_email.rs currently
includes the raw email (decoded_jwt.user_context.user_id) which exposes PII;
change the error logging to avoid writing the plaintext email by computing and
logging a non-reversible identifier (e.g., SHA256 hex or a redacted mask) before
the call to
notification_db_client::unsubscribe::email::upsert_email_unsubscribe, then
replace email=?email in the tracing::error with the hashed/redacted identifier
(or remove the email field entirely), ensuring no other logs or error responses
include the raw email value.
In `@rust/cloud-storage/notification/src/inbound/http/device.rs`:
- Around line 49-56: Unregister handler currently drops DecodedJwt and calls
state.inner.unregister_device(&req.token, &req.device_type), allowing any caller
to delete any token; change the handler signature in unregister_device to
extract the caller id from DecodedJwt (e.g., macro_user.id or macro_user_id) and
pass it into the service call as state.inner.unregister_device(¯o_user_id,
&req.token, &req.device_type). Update the NotificationReader trait method
signature (unregister_device) to accept the user_id first and change all
implementations accordingly to perform the deletion by (user_id, token,
device_type).
---
Duplicate comments:
In `@rust/cloud-storage/decode_jwt/src/lib.rs`:
- Around line 222-223: In the DecodeJwtError::InvalidUserId(id) match arm (where
tracing::error!(error=%id, "invalid macro user id") is called) avoid logging the
raw `id` (PII); instead compute a non-PII representation (e.g., redact or hash
the `id`) and log that representation or just a generic message; update the
tracing::error call to use the redacted/hashed variable or drop the id
altogether so no email/macro user id is written to logs.
- Around line 89-91: The cfg guard and env handling are inconsistent: keep the
unwrap_or fallback but remove the existence check so the default can be used
when LOCAL_USER_ID is unset; change the condition from `if cfg!(feature =
"local_auth") && std::env::var("LOCAL_USER_ID").is_ok()` to just `if
cfg!(feature = "local_auth")` and retain the `let user_id =
std::env::var("LOCAL_USER_ID").unwrap_or("macro|orguser@org.com".to_string());`
line so `user_id` gets the default when the env var is absent.
In `@rust/cloud-storage/email_formatting/src/lib.rs`:
- Around line 64-68: The destructuring "let DigestBatch { user_id: _,
notifications, .. } = digest;" moves notifications out of digest and prevents
later access to digest.user_id; fix by borrowing notifications instead of moving
it (e.g., destructure from a reference or use "ref notifications" so
notifications is a reference: "let DigestBatch { ref notifications, .. } =
digest;" or "let DigestBatch { notifications, .. } = &digest;" ), and apply the
same change at the other occurrence (lines ~93-97) so digest remains usable
afterwards.
In `@rust/cloud-storage/notification/src/domain/models/signing.rs`:
- Around line 19-24: The new() function currently appends SIG_PARAM without
removing any existing sigs, causing duplicate sig parameters when re-signing;
update canonicalize() (or perform a pre-sign filter in new()) to remove any
existing query parameter named SIG_PARAM before computing the MAC so
canonicalization is idempotent, then compute_mac(&canonical, mac) and append a
single SIG_PARAM; reference the canonicalize, new, compute_mac, SIG_PARAM, and
verify symbols to ensure canonicalize strips SIG_PARAM so verify() still sees
exactly one signature.
In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs`:
- Around line 47-50: The preference handler get_notification_type_preferences
returns a Result but lacks failure tracing; add the repo-standard
instrumentation by annotating it with #[tracing::instrument(err)] immediately
above the function definition, and apply the same attribute to the other
Result-returning preference handler functions in this module (the handlers
corresponding to the ranges noted in the review) so failures get captured
consistently by tracing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a94febc9-f472-41bc-bf57-5424e418faa5
📒 Files selected for processing (15)
rust/cloud-storage/decode_jwt/src/lib.rsrust/cloud-storage/email_formatting/src/lib.rsrust/cloud-storage/macro_auth/src/headers.rsrust/cloud-storage/macro_service_urls/src/lib.rsrust/cloud-storage/notification/src/domain/models/signing.rsrust/cloud-storage/notification/src/inbound/http/device.rsrust/cloud-storage/notification/src/inbound/http/preferences.rsrust/cloud-storage/notification/src/inbound/http/test.rsrust/cloud-storage/notification_sandbox/src/main.rsrust/cloud-storage/notification_service/src/api/unsubscribe/get_unsubscribes.rsrust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_all.rsrust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_item.rsrust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_all.rsrust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_email.rsrust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_item.rs
rust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_item.rs
Show resolved
Hide resolved
seanaye
force-pushed
the
seanaye/feat/hmac-unsubscribing
branch
from
eec9438 to
06bbbef
Compare
seanaye
force-pushed
the
seanaye/feat/hmac-unsubscribing
branch
from
995fbbd to
fc89116
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/cloud-storage/email_formatting/src/lib.rs (1)
64-68:⚠️ Potential issue | 🔴 CriticalFix use-after-move error:
digestis consumed by pattern destructuring but accessed later.The
DigestBatchpattern match at lines 64–68 consumesdigest. SinceDigestBatchcontains aVecfield and is notCopy, the entire binding is consumed. Line 95 then attempts to accessdigest.user_id, which is invalid.Additionally, this exposes PII: the
user_id(which contains email-based identifiers per the data type) is appended to a URL query parameter, making it visible in browser history, server logs, and access logs.Proposed fix
let DigestBatch { - user_id: _, + user_id, notifications, .. } = digest; @@ unsubscribe_url .query_pairs_mut() - .append_pair("id", digest.user_id.as_ref()) + .append_pair("id", user_id.as_ref()) .finish();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/email_formatting/src/lib.rs` around lines 64 - 68, The pattern match `let DigestBatch { user_id: _, notifications, .. } = digest;` moves `digest` and causes a use-after-move when `digest.user_id` is accessed later; change the destructuring to avoid moving `digest` (e.g., borrow the fields: `let DigestBatch { user_id, notifications, .. } = &digest;` or clone only `user_id` via `let user_id = digest.user_id.clone(); let notifications = digest.notifications;`) so subsequent accesses to `digest` succeed. Also remove the raw `user_id` from any URL query parameter creation (do not append `user_id` to the link); instead use a non-PII opaque token or server-side lookup (or include the identifier in a POST body) to avoid exposing email-based identifiers in logs and browser history. Ensure references to `DigestBatch`, `digest`, `user_id`, and `notifications` are updated accordingly.
♻️ Duplicate comments (7)
rust/cloud-storage/email_formatting/src/lib.rs (1)
93-97:⚠️ Potential issue | 🟠 MajorUser ID in unsubscribe URL exposes PII.
The
MacroUserIdStr(which contains email-like identifiers prefixed withmacro|) is appended directly to the URL query string. This leaks PII into browser history, server access logs, and link-scanner telemetry. Consider using an opaque identifier or encoding the user ID within the signed payload rather than exposing it in the URL.Based on learnings,
MacroUserIdStr::try_from_email(...)prepends"macro|"to the email string, so this identifier carries raw email PII.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/email_formatting/src/lib.rs` around lines 93 - 97, The unsubscribe URL currently leaks PII by appending digest.user_id (a MacroUserIdStr containing the raw email) as a query param; instead remove the append_pair("id", digest.user_id.as_ref()) call and embed an opaque identifier inside the SignedUrl payload (or replace it with a server-side opaque token/HMAC) so that SignedUrl::new receives a payload that authorizes the user without exposing their email; update the code that validates SignedUrl to extract/verify the opaque token and map it back to the user server-side, and refer to symbols unsubscribe_url, SignedUrl::new, MacroUserIdStr, digest.user_id, and MacroUserIdStr::try_from_email when making these changes.rust/cloud-storage/decode_jwt/src/lib.rs (2)
89-91:⚠️ Potential issue | 🟡 Minor
local_authdefault identity is unreachable.The condition
std::env::var("LOCAL_USER_ID").is_ok()means the block only executes when the env var is set. Theunwrap_ordefault on line 91 is therefore unreachable—ifLOCAL_USER_IDisn't set, the code falls through to real JWT validation instead of using the default"macro|orguser@org.com".If the intent is to always use local auth when the feature is enabled (with a fallback identity), remove the extra env var check:
♻️ Suggested fix
- if cfg!(feature = "local_auth") && std::env::var("LOCAL_USER_ID").is_ok() { + if cfg!(feature = "local_auth") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/decode_jwt/src/lib.rs` around lines 89 - 91, The code currently guards the local-auth branch with both cfg!(feature = "local_auth") and std::env::var("LOCAL_USER_ID").is_ok(), making the unwrap_or default in the user_id assignment unreachable; change the condition to only cfg!(feature = "local_auth") and then read the env var with a fallback (e.g. let user_id = std::env::var("LOCAL_USER_ID").unwrap_or("macro|orguser@org.com".to_string())); this ensures local auth is used when the feature is enabled and still falls back to the default identity when LOCAL_USER_ID is not set.
221-222:⚠️ Potential issue | 🟠 MajorDon't log the rejected Macro user ID verbatim—it contains PII.
The
MacroUserIdStris email-based (e.g.,macro|user@example.com). Loggingerror=%idwrites attacker-controlled PII into error logs. Log a generic message or a hashed/redacted form instead.🛡️ Proposed fix
DecodeJwtError::InvalidUserId(id) => { - tracing::error!(error=%id, "invalid macro user id"); + tracing::error!("invalid macro user id format"); ( StatusCode::UNAUTHORIZED,Based on learnings,
MacroUserIdStr::try_from_email(...)prepends"macro|"to the email string, so this identifier carries raw email PII.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/decode_jwt/src/lib.rs` around lines 221 - 222, The match arm handling DecodeJwtError::InvalidUserId(id) is logging the raw MacroUserIdStr (which contains email PII) via tracing::error!(error=%id, ...); change this to avoid logging verbatim PII by either logging a generic message (e.g., "invalid macro user id" without the id) or logging a deterministic redaction/hash of the id (compute a short hash/fingerprint of the MacroUserIdStr and log that) — update the code in the DecodeJwtError::InvalidUserId match arm to remove error=%id and instead log only the generic message or the hashed/redacted representation while keeping the same tracing::error call site.rust/cloud-storage/notification/src/inbound/http/device.rs (1)
24-25:⚠️ Potential issue | 🟡 MinorAdd
errto both handler spans.Both handlers return
Result, so failures are not attached to tracing spans right now.Suggested change
-#[tracing::instrument(skip(state, macro_user_id, req))] +#[tracing::instrument(err, skip(state, macro_user_id, req))] @@ -#[tracing::instrument(skip(state, _macro_user, req))] +#[tracing::instrument(err, skip(state, _macro_user, req))]As per coding guidelines, "Use
#[tracing::instrument(err)]on functions returningResult."Also applies to: 48-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/notification/src/inbound/http/device.rs` around lines 24 - 25, The tracing spans for the HTTP handlers are missing error attachment: update the attributes on the handler functions (e.g., register_device and the corresponding unregister_device) to include err in the tracing::instrument macro so failures are recorded (change #[tracing::instrument(skip(state, macro_user_id, req))] to #[tracing::instrument(skip(state, macro_user_id, req), err)] and likewise for the other handler).rust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_item.rs (1)
26-31:⚠️ Potential issue | 🟡 MinorInclude
errin the handler span.This handler returns
Result<Response, Response>, so adderrto retain failure context on the span.Suggested change
-#[tracing::instrument(skip(ctx, decoded_jwt))] +#[tracing::instrument(err, skip(ctx, decoded_jwt))]As per coding guidelines, "Use
#[tracing::instrument(err)]on functions returningResult."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_item.rs` around lines 26 - 31, The tracing span on the handler function doesn't capture errors; update the attribute on the handler (the async function named handler in remove_unsubscribe_item.rs) to include err in the instrument macro so failures are recorded (e.g., change #[tracing::instrument(skip(ctx, decoded_jwt))] to include err), keeping the existing skip list (State(ctx), decoded_jwt) intact; no other logic changes needed.rust/cloud-storage/notification/src/inbound/http/preferences.rs (2)
47-50: 🛠️ Refactor suggestion | 🟠 MajorAdd
#[tracing::instrument(err)]to these Result-returning handlers/helpers.This module adds five
Result-returning entry points with no standard failure instrumentation, so errors here lose the usual route-level context unless every caller logs them manually.As per coding guidelines, "Use
#[tracing::instrument(err)]on functions returningResult. Do NOT adderron functions returningOption,(), or non-Result types. Never includelevel = "info"."Also applies to: 89-95, 113-120, 152-156, 205-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs` around lines 47 - 50, Add the tracing instrumentation to all Result-returning request handlers in this module by annotating them with #[tracing::instrument(err)] (do not set level or use err on non-Result functions); specifically add #[tracing::instrument(err)] above get_notification_type_preferences and the other Result-returning handlers in this file (the functions at the other reported locations) so errors emitted from these functions include route-level context in traces.
111-149:⚠️ Potential issue | 🟠 MajorDon't perform the unsubscribe on GET.
Mail scanners and browser prefetchers routinely dereference email links. With the state change happening directly in this GET handler, a background fetch can disable a notification type without the user intentionally visiting the page. Render confirmation on GET and move the mutation to POST, or use the one-click unsubscribe flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs` around lines 111 - 149, The current presigned_disable_notification_type GET handler (presigned_disable_notification_type) performs a state-changing unsubscribe by calling disable_notification_type_inner; change this so GET only verifies the signed URL (using SignedUrl::verify and existing state.hmac_signing_key) and renders a confirmation page with a form that POSTs back to a new POST handler (e.g., presigned_disable_notification_type_post) or the same path with POST; move the call to disable_notification_type_inner into that POST handler, re-verify the signature there before mutating, and return the unsubscribe success/failure from the POST while keeping GET idempotent and safe for crawlers/prefetchers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/macro_service_urls/src/lib.rs`:
- Around line 40-44: The doc comment above the helper function url(s: &str) ->
Url is misleading because Url::parse is not const and parsing happens at
runtime; update the comment to state these are static string literals with
validated syntax at runtime (so unwrap is safe) rather than claiming
compile-time constants, and mention that Url::parse is expected to succeed for
these literals to clarify why unwrap() is used.
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/get_unsubscribes.rs`:
- Around line 24-28: The tracing span for the handler function is missing error
capture; update the #[tracing::instrument(...)] macro on the handler function
(the async fn handler(State(ctx): State<ApiContext>, decoded_jwt: DecodedJwt) ->
Result<Response, Response>) to include the err argument (e.g., add err to the
macro attributes along with skip(ctx, decoded_jwt)) so that errors returned as
the Err(Response) variant are recorded in spans.
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_all.rs`:
- Around line 24-28: The tracing span for the handler function is missing error
capture; update the attribute on the handler (the function named handler with
signature fn handler(State(ctx): State<ApiContext>, decoded_jwt: DecodedJwt) ->
Result<Response, Response>) to include err so failures are recorded (change
#[tracing::instrument(skip(ctx, decoded_jwt))] to
#[tracing::instrument(skip(ctx, decoded_jwt), err)]), keeping the same skips for
ctx and decoded_jwt.
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_all.rs`:
- Around line 23-27: The tracing span for the handler function doesn't capture
returned errors; update the attribute on the handler (the function named handler
with signature Result<Response, Response> and parameters State(ctx):
State<ApiContext>, decoded_jwt: DecodedJwt) to include err so the span records
the error — e.g. change #[tracing::instrument(skip(ctx, decoded_jwt))] to
#[tracing::instrument(skip(ctx, decoded_jwt), err)].
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_email.rs`:
- Around line 23-27: The tracing span on the handler isn't recording returned
errors; update the #[tracing::instrument(...)] on the handler function to
include err (e.g., #[tracing::instrument(skip(ctx, decoded_jwt), err)]) so that
failures returned by handler (Result<Response, Response>) are captured on the
span; modify the attribute above pub async fn handler(...) to include err while
keeping skip(ctx, decoded_jwt).
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_item.rs`:
- Around line 32-37: The tracing span for handler currently omits error
attachment; update the attribute on the handler function to include error
capture so failures get recorded in the span (e.g., change
#[tracing::instrument(skip(ctx, decoded_jwt))] on the handler function to
#[tracing::instrument(skip(ctx, decoded_jwt), err)]), leaving the same signature
Result<Response, Response> and keeping skip arguments unchanged.
In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs`:
- Around line 37-45: The utoipa path attributes in this module are missing an
explicit tag and are leaking the Rust module path into the OpenAPI spec; update
each #[utoipa::path(...)] annotation in this file (e.g., the one with
operation_id = "get_notification_type_preferences" and the other two path
annotations referenced) to include a stable tag like tag = "UserNotifications"
(or another agreed public-facing tag), ensuring all path attributes in this
module use the same explicit tag so the generated spec groups APIs under that
stable name rather than the internal module path.
In `@rust/cloud-storage/notification/src/inbound/http/test.rs`:
- Around line 518-555: The test builds signed URLs using a hardcoded
NOTIFICATION_BASE_URL which can mismatch the handler's verification base; change
signed_disable_uri to derive the base from
Environment::new_or_prod().notification_service() (or replace
NOTIFICATION_BASE_URL with that value) so the SignedUrl is created from the
exact same base the handler verifies against (refer to signed_disable_uri,
NOTIFICATION_BASE_URL, and Environment::new_or_prod().notification_service());
ensure the resulting URL is parsed and signed the same way and you still return
the path+query for the request URI.
---
Outside diff comments:
In `@rust/cloud-storage/email_formatting/src/lib.rs`:
- Around line 64-68: The pattern match `let DigestBatch { user_id: _,
notifications, .. } = digest;` moves `digest` and causes a use-after-move when
`digest.user_id` is accessed later; change the destructuring to avoid moving
`digest` (e.g., borrow the fields: `let DigestBatch { user_id, notifications, ..
} = &digest;` or clone only `user_id` via `let user_id = digest.user_id.clone();
let notifications = digest.notifications;`) so subsequent accesses to `digest`
succeed. Also remove the raw `user_id` from any URL query parameter creation (do
not append `user_id` to the link); instead use a non-PII opaque token or
server-side lookup (or include the identifier in a POST body) to avoid exposing
email-based identifiers in logs and browser history. Ensure references to
`DigestBatch`, `digest`, `user_id`, and `notifications` are updated accordingly.
---
Duplicate comments:
In `@rust/cloud-storage/decode_jwt/src/lib.rs`:
- Around line 89-91: The code currently guards the local-auth branch with both
cfg!(feature = "local_auth") and std::env::var("LOCAL_USER_ID").is_ok(), making
the unwrap_or default in the user_id assignment unreachable; change the
condition to only cfg!(feature = "local_auth") and then read the env var with a
fallback (e.g. let user_id =
std::env::var("LOCAL_USER_ID").unwrap_or("macro|orguser@org.com".to_string()));
this ensures local auth is used when the feature is enabled and still falls back
to the default identity when LOCAL_USER_ID is not set.
- Around line 221-222: The match arm handling DecodeJwtError::InvalidUserId(id)
is logging the raw MacroUserIdStr (which contains email PII) via
tracing::error!(error=%id, ...); change this to avoid logging verbatim PII by
either logging a generic message (e.g., "invalid macro user id" without the id)
or logging a deterministic redaction/hash of the id (compute a short
hash/fingerprint of the MacroUserIdStr and log that) — update the code in the
DecodeJwtError::InvalidUserId match arm to remove error=%id and instead log only
the generic message or the hashed/redacted representation while keeping the same
tracing::error call site.
In `@rust/cloud-storage/email_formatting/src/lib.rs`:
- Around line 93-97: The unsubscribe URL currently leaks PII by appending
digest.user_id (a MacroUserIdStr containing the raw email) as a query param;
instead remove the append_pair("id", digest.user_id.as_ref()) call and embed an
opaque identifier inside the SignedUrl payload (or replace it with a server-side
opaque token/HMAC) so that SignedUrl::new receives a payload that authorizes the
user without exposing their email; update the code that validates SignedUrl to
extract/verify the opaque token and map it back to the user server-side, and
refer to symbols unsubscribe_url, SignedUrl::new, MacroUserIdStr,
digest.user_id, and MacroUserIdStr::try_from_email when making these changes.
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_item.rs`:
- Around line 26-31: The tracing span on the handler function doesn't capture
errors; update the attribute on the handler (the async function named handler in
remove_unsubscribe_item.rs) to include err in the instrument macro so failures
are recorded (e.g., change #[tracing::instrument(skip(ctx, decoded_jwt))] to
include err), keeping the existing skip list (State(ctx), decoded_jwt) intact;
no other logic changes needed.
In `@rust/cloud-storage/notification/src/inbound/http/device.rs`:
- Around line 24-25: The tracing spans for the HTTP handlers are missing error
attachment: update the attributes on the handler functions (e.g.,
register_device and the corresponding unregister_device) to include err in the
tracing::instrument macro so failures are recorded (change
#[tracing::instrument(skip(state, macro_user_id, req))] to
#[tracing::instrument(skip(state, macro_user_id, req), err)] and likewise for
the other handler).
In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs`:
- Around line 47-50: Add the tracing instrumentation to all Result-returning
request handlers in this module by annotating them with
#[tracing::instrument(err)] (do not set level or use err on non-Result
functions); specifically add #[tracing::instrument(err)] above
get_notification_type_preferences and the other Result-returning handlers in
this file (the functions at the other reported locations) so errors emitted from
these functions include route-level context in traces.
- Around line 111-149: The current presigned_disable_notification_type GET
handler (presigned_disable_notification_type) performs a state-changing
unsubscribe by calling disable_notification_type_inner; change this so GET only
verifies the signed URL (using SignedUrl::verify and existing
state.hmac_signing_key) and renders a confirmation page with a form that POSTs
back to a new POST handler (e.g., presigned_disable_notification_type_post) or
the same path with POST; move the call to disable_notification_type_inner into
that POST handler, re-verify the signature there before mutating, and return the
unsubscribe success/failure from the POST while keeping GET idempotent and safe
for crawlers/prefetchers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5da79338-e333-45ed-b57e-06f05b544bde
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (20)
.env-local.enc.env-localdev.encjs/app/packages/service-clients/service-notification/openapi.jsonrust/cloud-storage/decode_jwt/Cargo.tomlrust/cloud-storage/decode_jwt/src/lib.rsrust/cloud-storage/decode_jwt/src/test.rsrust/cloud-storage/email_formatting/src/lib.rsrust/cloud-storage/macro_auth/src/headers.rsrust/cloud-storage/macro_service_urls/src/lib.rsrust/cloud-storage/notification/src/domain/models/signing.rsrust/cloud-storage/notification/src/inbound/http/device.rsrust/cloud-storage/notification/src/inbound/http/preferences.rsrust/cloud-storage/notification/src/inbound/http/test.rsrust/cloud-storage/notification_sandbox/src/main.rsrust/cloud-storage/notification_service/src/api/unsubscribe/get_unsubscribes.rsrust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_all.rsrust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_item.rsrust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_all.rsrust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_email.rsrust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_item.rs
Comment on lines
+40
to
+44
| /// Helper to parse a static URL string. All URLs here are compile-time constants | ||
| /// so parsing can never fail. | ||
| fn url(s: &str) -> Url { | ||
| Url::parse(s).unwrap() | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the doc comment—these aren't compile-time constants.
Url::parse is not a const fn, so parsing occurs at runtime. The URLs are static string literals with valid syntax, making the unwrap safe in practice, but the current wording could mislead readers into thinking there's compile-time validation.
📝 Suggested clarification
-/// Helper to parse a static URL string. All URLs here are compile-time constants
-/// so parsing can never fail.
+/// Helper to parse a static URL string. All URLs here are hardcoded valid
+/// literals, so parsing cannot fail in practice.
fn url(s: &str) -> Url {
Url::parse(s).unwrap()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/macro_service_urls/src/lib.rs` around lines 40 - 44, The
doc comment above the helper function url(s: &str) -> Url is misleading because
Url::parse is not const and parsing happens at runtime; update the comment to
state these are static string literals with validated syntax at runtime (so
unwrap is safe) rather than claiming compile-time constants, and mention that
Url::parse is expected to succeed for these literals to clarify why unwrap() is
used.
Comment on lines
+24
to
28
| #[tracing::instrument(skip(ctx, decoded_jwt))] | ||
| pub async fn handler( | ||
| State(ctx): State<ApiContext>, | ||
| user_context: Extension<UserContext>, | ||
| decoded_jwt: DecodedJwt, | ||
| ) -> Result<Response, Response> { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n rust/cloud-storage/notification_service/src/api/unsubscribe/get_unsubscribes.rs | head -35Repository: macro-inc/macro
Length of output: 1304
Add err to the handler span.
The handler function returns Result<Response, Response>, requiring the err attribute in the tracing instrument macro to capture error context in spans.
Suggested change
-#[tracing::instrument(skip(ctx, decoded_jwt))]
+#[tracing::instrument(err, skip(ctx, decoded_jwt))]📝 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.
Suggested change
| #[tracing::instrument(skip(ctx, decoded_jwt))] | |
| pub async fn handler( | |
| State(ctx): State<ApiContext>, | |
| user_context: Extension<UserContext>, | |
| decoded_jwt: DecodedJwt, | |
| ) -> Result<Response, Response> { | |
| #[tracing::instrument(err, skip(ctx, decoded_jwt))] | |
| pub async fn handler( | |
| State(ctx): State<ApiContext>, | |
| decoded_jwt: DecodedJwt, | |
| ) -> Result<Response, Response> { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/get_unsubscribes.rs`
around lines 24 - 28, The tracing span for the handler function is missing error
capture; update the #[tracing::instrument(...)] macro on the handler function
(the async fn handler(State(ctx): State<ApiContext>, decoded_jwt: DecodedJwt) ->
Result<Response, Response>) to include the err argument (e.g., add err to the
macro attributes along with skip(ctx, decoded_jwt)) so that errors returned as
the Err(Response) variant are recorded in spans.
Comment on lines
+24
to
28
| #[tracing::instrument(skip(ctx, decoded_jwt))] | ||
| pub async fn handler( | ||
| State(ctx): State<ApiContext>, | ||
| user_context: Extension<UserContext>, | ||
| decoded_jwt: DecodedJwt, | ||
| ) -> Result<Response, Response> { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "remove_unsubscribe_all.rs"Repository: macro-inc/macro
Length of output: 144
🏁 Script executed:
sed -n '20,35p' rust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_all.rs | cat -nRepository: macro-inc/macro
Length of output: 707
Add err to the tracing::instrument attribute.
The handler function returns Result<Response, Response>, so err must be included to capture failures in the tracing context.
Suggested change
-#[tracing::instrument(skip(ctx, decoded_jwt))]
+#[tracing::instrument(err, skip(ctx, decoded_jwt))]📝 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.
Suggested change
| #[tracing::instrument(skip(ctx, decoded_jwt))] | |
| pub async fn handler( | |
| State(ctx): State<ApiContext>, | |
| user_context: Extension<UserContext>, | |
| decoded_jwt: DecodedJwt, | |
| ) -> Result<Response, Response> { | |
| #[tracing::instrument(err, skip(ctx, decoded_jwt))] | |
| pub async fn handler( | |
| State(ctx): State<ApiContext>, | |
| decoded_jwt: DecodedJwt, | |
| ) -> Result<Response, Response> { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/remove_unsubscribe_all.rs`
around lines 24 - 28, The tracing span for the handler function is missing error
capture; update the attribute on the handler (the function named handler with
signature fn handler(State(ctx): State<ApiContext>, decoded_jwt: DecodedJwt) ->
Result<Response, Response>) to include err so failures are recorded (change
#[tracing::instrument(skip(ctx, decoded_jwt))] to
#[tracing::instrument(skip(ctx, decoded_jwt), err)]), keeping the same skips for
ctx and decoded_jwt.
Comment on lines
+23
to
27
| #[tracing::instrument(skip(ctx, decoded_jwt))] | ||
| pub async fn handler( | ||
| State(ctx): State<ApiContext>, | ||
| user_context: Extension<UserContext>, | ||
| decoded_jwt: DecodedJwt, | ||
| ) -> Result<Response, Response> { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_all.rs | head -40Repository: macro-inc/macro
Length of output: 1500
Add err to tracing instrumentation.
The handler returns Result<Response, Response>, so include err on the span.
Suggested change
-#[tracing::instrument(skip(ctx, decoded_jwt))]
+#[tracing::instrument(err, skip(ctx, decoded_jwt))]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_all.rs`
around lines 23 - 27, The tracing span for the handler function doesn't capture
returned errors; update the attribute on the handler (the function named handler
with signature Result<Response, Response> and parameters State(ctx):
State<ApiContext>, decoded_jwt: DecodedJwt) to include err so the span records
the error — e.g. change #[tracing::instrument(skip(ctx, decoded_jwt))] to
#[tracing::instrument(skip(ctx, decoded_jwt), err)].
Comment on lines
+23
to
27
| #[tracing::instrument(skip(ctx, decoded_jwt))] | ||
| pub async fn handler( | ||
| State(ctx): State<ApiContext>, | ||
| user_context: Extension<UserContext>, | ||
| decoded_jwt: DecodedJwt, | ||
| ) -> Result<Response, Response> { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_email.rs" | sed -n '20,35p'Repository: macro-inc/macro
Length of output: 788
Include err in tracing instrumentation.
This handler returns Result<Response, Response> and should capture failures on the span.
Suggested change
-#[tracing::instrument(skip(ctx, decoded_jwt))]
+#[tracing::instrument(err, skip(ctx, decoded_jwt))]📝 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.
Suggested change
| #[tracing::instrument(skip(ctx, decoded_jwt))] | |
| pub async fn handler( | |
| State(ctx): State<ApiContext>, | |
| user_context: Extension<UserContext>, | |
| decoded_jwt: DecodedJwt, | |
| ) -> Result<Response, Response> { | |
| #[tracing::instrument(err, skip(ctx, decoded_jwt))] | |
| pub async fn handler( | |
| State(ctx): State<ApiContext>, | |
| decoded_jwt: DecodedJwt, | |
| ) -> Result<Response, Response> { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_email.rs`
around lines 23 - 27, The tracing span on the handler isn't recording returned
errors; update the #[tracing::instrument(...)] on the handler function to
include err (e.g., #[tracing::instrument(skip(ctx, decoded_jwt), err)]) so that
failures returned by handler (Result<Response, Response>) are captured on the
span; modify the attribute above pub async fn handler(...) to include err while
keeping skip(ctx, decoded_jwt).
Comment on lines
+32
to
37
| #[tracing::instrument(skip(ctx, decoded_jwt))] | ||
| pub async fn handler( | ||
| State(ctx): State<ApiContext>, | ||
| user_context: Extension<UserContext>, | ||
| decoded_jwt: DecodedJwt, | ||
| Path(UnsubscribeItemPathParams { item_type, item_id }): Path<UnsubscribeItemPathParams>, | ||
| ) -> Result<Response, Response> { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "unsubscribe_item.rs" | grep -E "notification_service"Repository: macro-inc/macro
Length of output: 140
🏁 Script executed:
cat -n rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_item.rs | sed -n '30,40p'Repository: macro-inc/macro
Length of output: 550
Add err to the handler span.
The function returns Result<Response, Response>, so errors should be attached to tracing spans.
Suggested change
-#[tracing::instrument(skip(ctx, decoded_jwt))]
+#[tracing::instrument(err, skip(ctx, decoded_jwt))]📝 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.
Suggested change
| #[tracing::instrument(skip(ctx, decoded_jwt))] | |
| pub async fn handler( | |
| State(ctx): State<ApiContext>, | |
| user_context: Extension<UserContext>, | |
| decoded_jwt: DecodedJwt, | |
| Path(UnsubscribeItemPathParams { item_type, item_id }): Path<UnsubscribeItemPathParams>, | |
| ) -> Result<Response, Response> { | |
| #[tracing::instrument(err, skip(ctx, decoded_jwt))] | |
| pub async fn handler( | |
| State(ctx): State<ApiContext>, | |
| decoded_jwt: DecodedJwt, | |
| Path(UnsubscribeItemPathParams { item_type, item_id }): Path<UnsubscribeItemPathParams>, | |
| ) -> Result<Response, Response> { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/notification_service/src/api/unsubscribe/unsubscribe_item.rs`
around lines 32 - 37, The tracing span for handler currently omits error
attachment; update the attribute on the handler function to include error
capture so failures get recorded in the span (e.g., change
#[tracing::instrument(skip(ctx, decoded_jwt))] on the handler function to
#[tracing::instrument(skip(ctx, decoded_jwt), err)]), leaving the same signature
Result<Response, Response> and keeping skip arguments unchanged.
Comment on lines
+37
to
+45
| #[utoipa::path( | ||
| get, | ||
| operation_id = "get_notification_type_preferences", | ||
| path = "/v1/user_notifications/preferences", | ||
| responses( | ||
| (status = 200, body = GetNotificationTypePreferencesResponse), | ||
| (status = 401, body = ErrorResponse), | ||
| (status = 500, body = ErrorResponse), | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pin a stable OpenAPI tag on these #[utoipa::path] declarations.
Without an explicit tag, utoipa is already emitting notification_crate::inbound::http::preferences into the generated spec. That leaks an internal Rust module path into the public API docs and can rename downstream JS client groupings.
Suggested fix
#[utoipa::path(
get,
+ tag = "user_notification",
operation_id = "get_notification_type_preferences",
path = "/v1/user_notifications/preferences",
@@
#[utoipa::path(
put,
+ tag = "user_notification",
operation_id = "disable_notification_type",
path = "/v1/user_notifications/preferences/{notification_event_type}/disable",
@@
#[utoipa::path(
put,
+ tag = "user_notification",
operation_id = "enable_notification_type",
path = "/v1/user_notifications/preferences/{notification_event_type}/enable",Also applies to: 75-87, 192-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/notification/src/inbound/http/preferences.rs` around lines
37 - 45, The utoipa path attributes in this module are missing an explicit tag
and are leaking the Rust module path into the OpenAPI spec; update each
#[utoipa::path(...)] annotation in this file (e.g., the one with operation_id =
"get_notification_type_preferences" and the other two path annotations
referenced) to include a stable tag like tag = "UserNotifications" (or another
agreed public-facing tag), ensuring all path attributes in this module use the
same explicit tag so the generated spec groups APIs under that stable name
rather than the internal module path.
Comment on lines
+518
to
+555
| /// The base URL that `Environment::new_or_prod()` (Production) resolves to. | ||
| const NOTIFICATION_BASE_URL: &str = "https://notifications.macro.com"; | ||
|
|
||
| fn presigned_router() -> Router { | ||
| let hmac_key = Hmac::<Sha256>::new_from_slice(HMAC_KEY).unwrap(); | ||
| let state = NotificationRouterState::new( | ||
| PresignedTestService, | ||
| &BLOCKABLE, | ||
| hmac_key, | ||
| JwtValidationArgs::new_testing(), | ||
| ); | ||
|
|
||
| Router::new() | ||
| .nest( | ||
| "/user_notifications", | ||
| super::router::<PresignedTestService, serde_json::Value>(), | ||
| ) | ||
| .with_state(state) | ||
| } | ||
|
|
||
| /// Build a presigned disable URL path+query for use as a request URI. | ||
| /// | ||
| /// Signs the full absolute URL (`https://notifications.macro.com/...`) and | ||
| /// returns only the path+query portion (e.g. `/user_notifications/preferences/...?id=...&sig=...`). | ||
| fn signed_disable_uri(notification_type: &str, user_id: &str) -> String { | ||
| let hmac_key = Hmac::<Sha256>::new_from_slice(HMAC_KEY).unwrap(); | ||
| let mut unsigned = url::Url::parse(&format!( | ||
| "{NOTIFICATION_BASE_URL}/user_notifications/preferences/{notification_type}/disable" | ||
| )) | ||
| .unwrap(); | ||
| // Use query_pairs_mut so the encoding matches what SignedUrl::verify expects | ||
| // (application/x-www-form-urlencoded round-trip). | ||
| unsigned.query_pairs_mut().append_pair("id", user_id); | ||
| let signed = SignedUrl::new(unsigned, hmac_key); | ||
| let signed_url = signed.as_ref(); | ||
| // Return path + query for use as request URI | ||
| format!("{}?{}", signed_url.path(), signed_url.query().unwrap()) | ||
| } |
Contributor
There was a problem hiding this comment.
Build the signed test URL from the same base the handler verifies against.
presigned_disable_notification_type verifies against Environment::new_or_prod().notification_service(). If the test process resolves Develop or Local, the signature produced here becomes invalid and these tests fail for the wrong reason.
Suggested fix
+use macro_env::Environment;
+use macro_service_urls::EnvExtMacroServiceUrls;
@@
-/// The base URL that `Environment::new_or_prod()` (Production) resolves to.
-const NOTIFICATION_BASE_URL: &str = "https://notifications.macro.com";
-
/// Build a presigned disable URL path+query for use as a request URI.
///
/// Signs the full absolute URL (`https://notifications.macro.com/...`) and
/// returns only the path+query portion (e.g. `/user_notifications/preferences/...?id=...&sig=...`).
fn signed_disable_uri(notification_type: &str, user_id: &str) -> String {
let hmac_key = Hmac::<Sha256>::new_from_slice(HMAC_KEY).unwrap();
- let mut unsigned = url::Url::parse(&format!(
- "{NOTIFICATION_BASE_URL}/user_notifications/preferences/{notification_type}/disable"
- ))
- .unwrap();
+ let mut unsigned = Environment::new_or_prod().notification_service();
+ unsigned.set_path(&format!(
+ "/user_notifications/preferences/{notification_type}/disable"
+ ));
// Use query_pairs_mut so the encoding matches what SignedUrl::verify expects
// (application/x-www-form-urlencoded round-trip).
unsigned.query_pairs_mut().append_pair("id", user_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/notification/src/inbound/http/test.rs` around lines 518 -
555, The test builds signed URLs using a hardcoded NOTIFICATION_BASE_URL which
can mismatch the handler's verification base; change signed_disable_uri to
derive the base from Environment::new_or_prod().notification_service() (or
replace NOTIFICATION_BASE_URL with that value) so the SignedUrl is created from
the exact same base the handler verifies against (refer to signed_disable_uri,
NOTIFICATION_BASE_URL, and Environment::new_or_prod().notification_service());
ensure the resulting URL is parsed and signed the same way and you still return
the path+query for the request URI.
fc89116 to
b716d5a
Compare
whutchinson98
requested changes
Apr 6, 2026
| notification-service:macro_cache_secret_key: macro-cache-dev | ||
| notification-service:authentication_service_internal_api_key: authentication-service-internal-api-key-dev | ||
| notification-service:macro_db_secret_key: macro-db-dev | ||
| notification-service:unsubscribe_hmac_secret: |
Member
There was a problem hiding this comment.
its better practice for us to store these secrets in aws secrets manager
| notification-service:macro_cache_secret_key: macro-cache-prod | ||
| notification-service:authentication_service_internal_api_key: authentication-service-internal-api-key-prod | ||
| notification-service:macro_db_secret_key: macro-db-prod | ||
| notification-service:unsubscribe_hmac_secret: |
seanaye
force-pushed
the
seanaye/feat/hmac-unsubscribing
branch
2 times, most recently
from
a09456c to
426fc8a
Compare
seanaye
force-pushed
the
seanaye/feat/hmac-unsubscribing
branch
from
426fc8a to
e5f4044
Compare
whutchinson98
approved these changes
Apr 7, 2026
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.