(fixes #61) Issue R: Refactor encoding handling to enforce base64 usage for key packages and welcomes#98
Conversation
WalkthroughEnforces Base64-only content encoding: welcome and key-package content now require an explicit Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
❌ Coverage: 86.3% → 86.13% (-0.17%) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/mdk-core/src/welcomes.rs (1)
291-295: Missing encoding tag currently doesn’t create a failedProcessedWelcomeentryRequiring
ContentEncoding::from_tags(...)?is correct for enforcing explicit encoding tags, but note this earlyErr(Error::Welcome("Missing required encoding tag"...))path does not record aProcessedWelcome, unlike the decode/parse error paths below.If you want
is_welcome_processed/dedupe behavior to treat "no encoding tag" the same as other permanent failures, consider writing a failedProcessedWelcomehere as well.crates/mdk-core/src/util.rs (1)
83-105:ContentEncoding::from_tagsOption contract looks good; consider distinguishing invalid valuesThe new
Option<Self>behavior plus docs (“no encoding tag → None, callers must reject”) lines up with howparse_key_packageandpreview_welcomenow enforce explicit encoding tags, and the tests confirm the basic cases.One minor follow‑up you might consider: unknown encoding values (
["encoding", "foo"]) are currently indistinguishable from “no encoding tag” (both yieldNone). If better error messages would help debugging, a future refinement could be to return aResult<Option<Self>, String>or a dedicated error for “invalid encoding value”, while keepingNonestrictly for “tag absent”.Also applies to: 217-239
crates/mdk-core/src/lib.rs (1)
44-51: MdkConfig simplification and builder wiring are consistentUsing an empty
MdkConfigwithnew()/Defaultand threading it throughMdkBuilder::with_configand theMDKstruct gives you a stable extension point while decoupling it from encoding concerns. The examples reflect the new API correctly.If you expect
MdkConfigto remain empty for a while, you might add a brief doc note that it’s currently a placeholder for future configuration knobs, just to set caller expectations.Also applies to: 67-70, 90-105, 210-215
crates/mdk-core/src/key_packages.rs (2)
187-191: parse_key_package correctly enforces explicit encoding tagUsing
ContentEncoding::from_tags(event.tags.iter())here and erroring when it returnsNoneenforces the MIP‑00/MIP‑02 requirement for an explicit encoding tag and lines up with the new base64‑only emission.Small nuance: because
from_tagsreturnsNonefor both “no tag” and “unrecognized value”, callers will always see “Missing required encoding tag” even if an invalid"encoding"value was present. If you care about distinguishing those two cases for diagnostics, you might later extendfrom_tags/this error to surface an “invalid encoding value” variant.
668-669: Base64 creation/parsing tests look solid; consider adding an explicit hex‑tag parse case
- Updating
test_key_package_creation_and_parsingto callparse_serialized_key_package(..., ContentEncoding::Base64)and the new teststest_key_package_base64_encoding/test_key_package_always_base64_encoding/test_key_package_parsing_supports_both_formatsgives good coverage that:
- created key packages are always valid base64,
- an
["encoding","base64"]tag is present, and- the low‑level parser still handles hex when explicitly requested.
test_parse_key_package_with_legacy_tagsnow includes an explicit["encoding","base64"]tag so thatparse_key_packagecontinues to accept legacy ciphersuite/extension tags, which is important for backcompat.Two small follow‑ups you might consider:
- Add a
parse_key_packagetest for a hex‑encoded event with an["encoding","hex"]tag, to exercise the full tag‑driven path (not justparse_serialized_key_package) for legacy content.- Rename locals like
key_package_hexin tests that now hold base64 strings to something neutral (key_package_strorkey_package_base64) for clarity.Overall the behavior under test looks correct.
Also applies to: 1923-1924, 2096-2127, 2131-2163, 2167-2198
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/mdk-core/src/groups.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/lib.rscrates/mdk-core/src/util.rscrates/mdk-core/src/welcomes.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)
**/*.rs: All trait bounds should be written inwhereclauses rather than inline with type parameters in Rust
UseSelfinstead of the explicit type name when referring to the type in impl blocks
DeriveDebug,Clone,Copy,PartialEq,Eq, andHashfor public types when possible, in that order
DeriveDefaultfor public types when there is a reasonable default value
Always use full paths for logging withtracing::<op>!(...)instead of importing and using<op>!(...)
Prefer.to_string()or.to_owned()for&strtoStringconversion instead of.into()orString::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import thefmtmodule rather than using full paths when implementing traits fromcore::fmtorstd::fmt
Use relativeself::imports for sub-modules rather than bare module names
Place allusestatements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoidif let ... { } else { }constructs; usematchinstead when both branches are non-empty
Useif let ... { }without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g.,x.rs) withmod x;declaration rather than inlinemod x { .. }blocks, except fortestsandbenchesmodules
Use inlinemod tests { #[cfg(test)] ... }for test modules rather than creating separatetests.rsfiles
Use inlinemod benches { #[cfg(bench)] ... }for benchmark modules rather than creating separatebenches.rsfiles
Files:
crates/mdk-core/src/welcomes.rscrates/mdk-core/src/groups.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/util.rscrates/mdk-core/src/lib.rs
🧬 Code graph analysis (1)
crates/mdk-core/src/welcomes.rs (1)
crates/mdk-core/src/util.rs (1)
from_tags(93-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint (stable, all-features)
- GitHub Check: Test Coverage
🔇 Additional comments (1)
crates/mdk-core/src/groups.rs (1)
1344-1364: Welcome rumors now consistently base64‑encoded with explicit encoding tagHard‑coding
ContentEncoding::Base64here and tagging["encoding", "base64"]aligns with the newContentEncoding::from_tagscontract and the overall base64‑only story; the change looks correct and security‑motivated.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mdk-core/src/key_packages.rs (1)
2131-2164: Remove duplicate test.
test_key_package_always_base64_encodingappears to be a duplicate oftest_key_package_base64_encoding(lines 2095-2128). Both tests verify the same behavior with identical logic. Consider removing this duplicate to reduce maintenance burden.
♻️ Duplicate comments (2)
crates/mdk-core/src/key_packages.rs (1)
23-39: Doc comment partially updated; complete the transition to base64-only.Lines 23-24 correctly state "always base64", but the Returns section (lines 31-39) still says "An encoded string (hex or base64)" and lists the encoding tag as
"hex" or "base64". Update these to reflect base64-only per the new behavior.🔎 Suggested fix
/// # Returns /// /// A tuple containing: - /// * An encoded string (hex or base64) containing the serialized key package + /// * A base64-encoded string containing the serialized key package /// * A fixed array of 7 tags for the Nostr event, in order: /// - `mls_protocol_version` - MLS protocol version (e.g., "1.0") /// - `mls_ciphersuite` - Ciphersuite identifier (e.g., "0x0001") /// - `mls_extensions` - Required MLS extensions /// - `relays` - Relay URLs for distribution /// - `protected` - Marks the event as protected /// - `client` - Client identifier and version - /// - `encoding` - The encoding format tag ("hex" or "base64") + /// - `encoding` - The encoding format tag (always "base64")Based on learnings, ...
crates/mdk-core/src/welcomes.rs (1)
435-441: Hoist base64 imports to module level and fix size calculation.Multiple tests inline-declare
use nostr::base64::Engineanduse nostr::base64::engine::general_purpose::STANDARD as BASE64inside function bodies. Per Rust coding guidelines, allusestatements must be at the top of the file/module. Additionally, intest_large_group_welcome_size_limits(lines 960-964), the size calculation still useshex_size / 2logic with comments referring to hex encoding, but content is now base64—decode viaBASE64.decode(...)and usedecoded.len()instead.🔎 Suggested fix
Move imports to the top of the
testsmodule:#[cfg(test)] mod tests { use super::*; use crate::test_util::*; use crate::tests::create_test_mdk; use nostr::{Keys, Kind, TagKind}; + use nostr::base64::Engine; + use nostr::base64::engine::general_purpose::STANDARD as BASE64;Fix size calculation in
test_large_group_welcome_size_limits(lines 960-977):let welcome = &group_result.welcome_rumors[0]; - let welcome_content_bytes = welcome.content.as_bytes(); - let hex_size = welcome_content_bytes.len(); - let binary_size = hex_size / 2; // Hex encoding doubles the size + let decoded_bytes = BASE64 + .decode(&welcome.content) + .expect("Welcome content should be valid base64"); + let binary_size = decoded_bytes.len(); let size_kb = binary_size as f64 / 1024.0;Also update comments at lines 1006-1007 that still reference "hex-encoded MLS messages" to say "base64-encoded".
Based on learnings, ...
🧹 Nitpick comments (1)
crates/mdk-core/src/key_packages.rs (1)
659-669: Consider renamingkey_package_strto reflect base64 encoding.Variable names like
key_package_strat lines 659, 668, 943, 950, 1875, 1880 are generic. Since key packages are now always base64-encoded, consider renaming tokey_package_b64orkey_package_base64for clarity and consistency with the encoding model.Based on learnings, ...
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/mdk-core/src/key_packages.rscrates/mdk-core/src/welcomes.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)
**/*.rs: All trait bounds should be written inwhereclauses rather than inline with type parameters in Rust
UseSelfinstead of the explicit type name when referring to the type in impl blocks
DeriveDebug,Clone,Copy,PartialEq,Eq, andHashfor public types when possible, in that order
DeriveDefaultfor public types when there is a reasonable default value
Always use full paths for logging withtracing::<op>!(...)instead of importing and using<op>!(...)
Prefer.to_string()or.to_owned()for&strtoStringconversion instead of.into()orString::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import thefmtmodule rather than using full paths when implementing traits fromcore::fmtorstd::fmt
Use relativeself::imports for sub-modules rather than bare module names
Place allusestatements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoidif let ... { } else { }constructs; usematchinstead when both branches are non-empty
Useif let ... { }without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g.,x.rs) withmod x;declaration rather than inlinemod x { .. }blocks, except fortestsandbenchesmodules
Use inlinemod tests { #[cfg(test)] ... }for test modules rather than creating separatetests.rsfiles
Use inlinemod benches { #[cfg(bench)] ... }for benchmark modules rather than creating separatebenches.rsfiles
Files:
crates/mdk-core/src/key_packages.rscrates/mdk-core/src/welcomes.rs
🧬 Code graph analysis (1)
crates/mdk-core/src/welcomes.rs (1)
crates/mdk-core/src/util.rs (1)
from_tags(93-105)
🪛 GitHub Actions: CI
crates/mdk-core/src/key_packages.rs
[error] 1853-1853: cannot find value key_package_str in this scope. Hint: a local variable with a similar name exists: key_package_hex. (E0425)
[error] 1853-1853: mdk-core could not be compiled (lib test) due to the above error.
🪛 GitHub Actions: Coverage
crates/mdk-core/src/key_packages.rs
[error] 1853-1853: cannot find value key_package_str in this scope. help: a local variable with a similar name exists: key_package_hex
[error] 1853-1853: E0425
🔇 Additional comments (6)
crates/mdk-core/src/welcomes.rs (2)
291-317: LGTM! Security-focused encoding validation.The explicit encoding tag requirement correctly prevents downgrade attacks and parsing ambiguity per MIP-00/MIP-02. The error path properly saves a Failed ProcessedWelcome with failure_reason, logs the error, and returns an appropriate error. This is consistent with the security-centric design noted in the relevant code snippets.
857-862: LGTM! Proper test isolation.Preserving the encoding tag ensures this test exercises the unknown KeyPackage scenario rather than the missing encoding tag path. The comment clearly documents the intent.
crates/mdk-core/src/key_packages.rs (4)
71-73: LGTM! Correctly enforces base64 encoding.Hardcoding
ContentEncoding::Base64with an explicit tag prevents downgrade attacks and parsing ambiguity per MIP-00/MIP-02. The security comment is clear and appropriate.
187-190: LGTM! Proper encoding tag validation.Requiring an explicit encoding tag via
ContentEncoding::from_tags(...).ok_or_else(...)correctly enforces MIP-00/MIP-02 security requirements and provides a clear error message when the tag is missing.
2095-2128: LGTM! Comprehensive base64 encoding test.This test thoroughly validates that key packages are created with base64 encoding, have the correct encoding tag, and can be parsed successfully. Good coverage of the new encoding behavior.
2166-2239: LGTM! Good backward compatibility tests.These tests properly verify that while key packages are always created with base64 encoding, the parsing logic still supports hex-encoded events with explicit encoding tags for backward compatibility. This is important for the migration period.
❌ Coverage: 86.3% → 86.23% (-0.07%) |
❌ Coverage: 86.3% → 86.22% (-0.08%) |
erskingardner
left a comment
There was a problem hiding this comment.
If my comment about us removing the backwards compatability of older key package events that don't have an encoding tag is correct we probably just want to take a close look and decide if we want to remove all traces of code that handles hex encoding to clean up. If we're going to make a breaking release (based on other issues) then we can just do it.
To keep things working for users, we can have whitenoise check on launch for outdated key packages that might have been published without those encoding tags (which also sort of become moot) and delete them from relays and publish new key packages. That doesn't fix anything for other user's of MDK but I guess we could work directly with those that we know about to ensure when they upgrade they know what to do.
❌ Coverage: 86.3% → 86.17% (-0.13%) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/mdk-core/src/welcomes.rs (2)
435-441: Hoist base64 imports to module scope per Rust guidelines.Multiple test functions declare
use nostr::base64::Engineanduse nostr::base64::engine::general_purpose::STANDARD as BASE64inline. Per coding guidelines, allusestatements should be at the top of the file/module, never inside functions or methods.This is a duplicate of a previous review comment.
🔎 Proposed fix
Move imports to the top of the
testsmodule (around line 394):#[cfg(test)] mod tests { use super::*; use crate::test_util::*; use crate::tests::create_test_mdk; use nostr::{Keys, Kind, TagKind}; + use nostr::base64::Engine; + use nostr::base64::engine::general_purpose::STANDARD as BASE64;Then remove the inline declarations at lines 436-437, 530-531, 631-632, 777-778, 972-973.
Also applies to: 530-531, 631-632, 777-778, 972-973
Based on past review comments and coding guidelines.
962-964: Fix base64 size calculation to use actual decoded length.The size calculation uses
(encoded_size * 3) / 4as an approximation for base64 overhead. For accurate binary size measurement, decode the content and use the actual length instead of an estimate.This is a duplicate of a previous review comment.
🔎 Proposed fix
let welcome = &group_result.welcome_rumors[0]; - let welcome_content_bytes = welcome.content.as_bytes(); - let encoded_size = welcome_content_bytes.len(); - let binary_size = (encoded_size * 3) / 4; // Base64 encoding is ~33% larger + let decoded_bytes = BASE64 + .decode(&welcome.content) + .expect("Welcome content should be valid base64"); + let binary_size = decoded_bytes.len(); let size_kb = binary_size as f64 / 1024.0;Based on past review comments.
🧹 Nitpick comments (1)
crates/mdk-core/src/key_packages.rs (1)
656-658: Consider renaming variable to reflect base64 encoding.The variable
key_package_strcould be renamed tokey_package_b64or similar to make it clearer that it contains base64-encoded content, not hex. This is mentioned in past review comments but remains optional given the code already works correctly.Also applies to: 665-665, 940-940, 947-947, 1793-1795, 1811-1811, 1843-1843, 1865-1867, 1870-1870
Based on past review comments.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/src/key_packages.rscrates/mdk-core/src/util.rscrates/mdk-core/src/welcomes.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)
**/*.rs: All trait bounds should be written inwhereclauses rather than inline with type parameters in Rust
UseSelfinstead of the explicit type name when referring to the type in impl blocks
DeriveDebug,Clone,Copy,PartialEq,Eq, andHashfor public types when possible, in that order
DeriveDefaultfor public types when there is a reasonable default value
Always use full paths for logging withtracing::<op>!(...)instead of importing and using<op>!(...)
Prefer.to_string()or.to_owned()for&strtoStringconversion instead of.into()orString::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import thefmtmodule rather than using full paths when implementing traits fromcore::fmtorstd::fmt
Use relativeself::imports for sub-modules rather than bare module names
Place allusestatements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoidif let ... { } else { }constructs; usematchinstead when both branches are non-empty
Useif let ... { }without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g.,x.rs) withmod x;declaration rather than inlinemod x { .. }blocks, except fortestsandbenchesmodules
Use inlinemod tests { #[cfg(test)] ... }for test modules rather than creating separatetests.rsfiles
Use inlinemod benches { #[cfg(bench)] ... }for benchmark modules rather than creating separatebenches.rsfiles
Files:
crates/mdk-core/src/welcomes.rscrates/mdk-core/src/key_packages.rscrates/mdk-core/src/util.rs
🧬 Code graph analysis (1)
crates/mdk-core/src/welcomes.rs (1)
crates/mdk-core/src/util.rs (1)
from_tags(88-100)
🪛 markdownlint-cli2 (0.18.1)
crates/mdk-core/CHANGELOG.md
46-46: No space after hash on atx style heading
(MD018, no-missing-space-atx)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Coverage
🔇 Additional comments (8)
crates/mdk-core/src/welcomes.rs (1)
291-317: Excellent security-conscious error handling for missing encoding tags.The explicit validation of the encoding tag with detailed error paths (creating a Failed ProcessedWelcome entry, logging, and returning an error) aligns well with the MIP-00/MIP-02 requirements and prevents downgrade attacks.
crates/mdk-core/src/key_packages.rs (3)
23-39: Documentation accurately reflects base64-only encoding.The doc comments correctly state "always base64" and specify the explicit
["encoding", "base64"]tag requirement. This addresses security concerns about downgrade attacks and parsing ambiguity.
71-73: Excellent security comment enforcing base64-only encoding.The SECURITY comment clearly explains the rationale for always using base64 with an explicit encoding tag, referencing MIP-00/MIP-02 and mentioning prevention of downgrade attacks and parsing ambiguity.
184-187: Good use of explicit encoding validation.Using
ok_or_elsewith a clear error message when the encoding tag is missing follows the security requirements from MIP-00/MIP-02 and prevents silent failures.crates/mdk-core/src/util.rs (4)
51-58: ContentEncoding simplified correctly to base64-only.The enum correctly retains only the Base64 variant with proper derive order (Debug, Clone, Copy, PartialEq, Eq) per coding guidelines. The documentation clearly states only base64 is supported per MIP-00/MIP-02.
88-100: Excellent security-conscious change to require explicit encoding tags.Changing
from_tagsto returnOption<Self>with no default fallback is a critical security improvement. The comment on lines 98-99 clearly explains this enforces MIP-00/MIP-02 requirements. Callers must now handle the None case and reject events without valid encoding tags, preventing downgrade attacks.
177-178: Test correctly verifies hex is no longer supported.The test properly asserts that
from_tag_value("hex")returnsNone, confirming hex encoding has been removed as intended.
195-198: Tests correctly verify strict encoding tag requirements.Both tests properly assert that
from_tagsreturnsNonefor hex tags and empty tag lists, confirming the security requirement that encoding tags must be explicitly present and valid.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
crates/mdk-core/src/welcomes.rs (2)
958-959: Use actual decoded size instead of approximation formula.The binary size calculation uses an approximation formula
(encoded_size * 3) / 4rather than decoding the base64 content and measuring the actual decoded length. For a test validating size limits, using the actual decoded size would be more accurate.🔎 Calculate actual decoded size
- let welcome_content_bytes = welcome.content.as_bytes(); - let encoded_size = welcome_content_bytes.len(); - let binary_size = (encoded_size * 3) / 4; // Base64 encoding is ~33% larger + let decoded_bytes = BASE64 + .decode(&welcome.content) + .expect("Welcome content should be valid base64"); + let binary_size = decoded_bytes.len(); let size_kb = binary_size as f64 / 1024.0;Based on past review feedback.
436-437: Hoist base64 imports to the top of the tests module.Multiple test functions import
nostr::base64::EngineandBASE64inside their function bodies (lines 436-437, 526-527, 627-628, 773-774, 968-969). Per the Rust coding guidelines, allusestatements should be placed at the top of the file/module scope, not inside functions.🔎 Hoist imports to tests module scope
Add these imports at the top of the
testsmodule (after line 394):#[cfg(test)] mod tests { use super::*; use crate::test_util::*; use crate::tests::create_test_mdk; use nostr::{Keys, Kind, TagKind}; + use nostr::base64::Engine; + use nostr::base64::engine::general_purpose::STANDARD as BASE64;Then remove the inline declarations from each test function.
As per coding guidelines.
Also applies to: 526-527, 627-628, 773-774, 968-969
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/src/welcomes.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)
**/*.rs: All trait bounds should be written inwhereclauses rather than inline with type parameters in Rust
UseSelfinstead of the explicit type name when referring to the type in impl blocks
DeriveDebug,Clone,Copy,PartialEq,Eq, andHashfor public types when possible, in that order
DeriveDefaultfor public types when there is a reasonable default value
Always use full paths for logging withtracing::<op>!(...)instead of importing and using<op>!(...)
Prefer.to_string()or.to_owned()for&strtoStringconversion instead of.into()orString::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import thefmtmodule rather than using full paths when implementing traits fromcore::fmtorstd::fmt
Use relativeself::imports for sub-modules rather than bare module names
Place allusestatements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoidif let ... { } else { }constructs; usematchinstead when both branches are non-empty
Useif let ... { }without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g.,x.rs) withmod x;declaration rather than inlinemod x { .. }blocks, except fortestsandbenchesmodules
Use inlinemod tests { #[cfg(test)] ... }for test modules rather than creating separatetests.rsfiles
Use inlinemod benches { #[cfg(bench)] ... }for benchmark modules rather than creating separatebenches.rsfiles
**/*.rs: All trait bounds must be placed inwhereclauses, not inline with generic parameters in Rust code
UseSelfinstead of the type name when possible in Rust code
Derive macros must be ordered as:Debug,Clone,Copy,PartialEq,Eq,Hashin Rust code
Always usetracing::warn!(...)with explicit module path, never im...
Files:
crates/mdk-core/src/welcomes.rs
crates/*/CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
crates/*/CHANGELOG.md: Every change that modifies functionality must update the appropriate crate CHANGELOG.md file (mdk-core, mdk-storage-traits, mdk-memory-storage, or mdk-sqlite-storage)
Follow Keep a Changelog format with sections for Unreleased, Breaking changes, Changed, Added, Fixed, Removed, and Deprecated in CHANGELOG files
Always include a PR reference link in changelog entries using the format([#123](https://github.com/marmot-protocol/mdk/pull/123))
Files:
crates/mdk-core/CHANGELOG.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Never use bare URLs in markdown files. All URLs must be properly formatted: use
[link text](url)for inline links,<url>for standalone URLs,([#123](https://github.com/marmot-protocol/mdk/pull/123))for PR/issue references, and([abc1234](https://github.com/.../commit/abc1234...))for commit references
Files:
crates/mdk-core/CHANGELOG.md
🧠 Learnings (7)
📚 Learning: 2026-01-01T16:28:05.759Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-01T16:28:05.759Z
Learning: Applies to **/*.rs : When adding or modifying code related to encrypted media, ensure tests are run with the `mip04` feature using `cargo test --features mip04`
Applied to files:
crates/mdk-core/src/welcomes.rs
📚 Learning: 2026-01-01T16:28:05.759Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-01T16:28:05.759Z
Learning: Applies to crates/*/CHANGELOG.md : Every change that modifies functionality must update the appropriate crate CHANGELOG.md file (mdk-core, mdk-storage-traits, mdk-memory-storage, or mdk-sqlite-storage)
Applied to files:
crates/mdk-core/src/welcomes.rscrates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-01T16:28:05.759Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-01T16:28:05.759Z
Learning: Applies to **/*.rs : Be careful with cryptographic key material in MDK code, as it handles cryptographic operations with forward secrecy via MLS protocol
Applied to files:
crates/mdk-core/src/welcomes.rs
📚 Learning: 2026-01-01T16:28:05.759Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-01T16:28:05.759Z
Learning: Applies to crates/*/CHANGELOG.md : Follow Keep a Changelog format with sections for Unreleased, Breaking changes, Changed, Added, Fixed, Removed, and Deprecated in CHANGELOG files
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-01T16:28:05.759Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-01T16:28:05.759Z
Learning: Applies to crates/*/CHANGELOG.md : Always include a PR reference link in changelog entries using the format `([#123](https://github.com/marmot-protocol/mdk/pull/123))`
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-01T15:51:58.505Z
Learnt from: dannym-arx
Repo: marmot-protocol/mdk PR: 98
File: crates/mdk-core/CHANGELOG.md:46-46
Timestamp: 2026-01-01T15:51:58.505Z
Learning: In CHANGELOG.md files, keep the GitHub issue references (e.g., #61) as-is; do not convert '#61' (with no space) into an MD heading '# 61'. The hash without a space is intentional for GitHub auto-linking. This guideline applies to all CHANGELOG.md files in the repo.
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-01T16:28:05.759Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-01T16:28:05.759Z
Learning: Applies to **/*.md : Never use bare URLs in markdown files. All URLs must be properly formatted: use `[link text](url)` for inline links, `<url>` for standalone URLs, `([#123](https://github.com/marmot-protocol/mdk/pull/123))` for PR/issue references, and `([abc1234](https://github.com/.../commit/abc1234...))` for commit references
Applied to files:
crates/mdk-core/CHANGELOG.md
🧬 Code graph analysis (1)
crates/mdk-core/src/welcomes.rs (1)
crates/mdk-core/src/util.rs (1)
from_tags(88-100)
🪛 markdownlint-cli2 (0.18.1)
crates/mdk-core/CHANGELOG.md
43-43: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🔇 Additional comments (2)
crates/mdk-core/src/welcomes.rs (1)
291-317: LGTM - Proper enforcement of explicit encoding tag requirement.The validation logic correctly implements the security requirement to prevent downgrade attacks by requiring an explicit encoding tag. The error path properly creates a Failed ProcessedWelcome entry for audit trail and returns a clear error message.
crates/mdk-core/CHANGELOG.md (1)
30-36: LGTM - Clear breaking change documentation.The breaking changes are well-documented with security rationale and migration guidance for clients.
erskingardner
left a comment
There was a problem hiding this comment.
Looks like you have a minor conflict but this LGTM
…tent-enables-downgrade-and-interop-failures
❌ Coverage: 86.79% → 86.68% (-0.11%) |
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @crates/mdk-core/CHANGELOG.md:
- Around line 30-35: Update the breaking-change bullet in CHANGELOG.md to
include the required PR reference by appending
"([#98](https://github.com/marmot-protocol/mdk/pull/98))" to the main bullet
line that starts with "**Content Encoding**: Removed support for hex encoding in
key package and welcome event content"; ensure the PR link is added inline at
the end of that main bullet sentence and keep the rest of the sub-bullets
unchanged.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mdk-core/src/welcomes.rs (1)
119-130: Update encoding validation to reject unsupported encodings with a clear error.The
validate_welcome_eventfunction accepts both "hex" and "base64" as valid encoding values (line 122), butContentEncoding::from_tag_valueonly recognizes "base64". This creates misleading behavior:
- Validation passes when tag contains
["encoding", "hex"]preview_welcomecallsContentEncoding::from_tags, which returnsNonefor "hex"- Error message becomes "Missing required encoding tag" (line 397 in util.rs), even though the tag is present with an unsupported value
Update validation to reject "hex" early with a specific error like "hex encoding is not supported" instead of accepting it and failing later with a misleading message.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/src/groups.rscrates/mdk-core/src/welcomes.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)
**/*.rs: All trait bounds should be written inwhereclauses rather than inline with type parameters in Rust
UseSelfinstead of the explicit type name when referring to the type in impl blocks
DeriveDebug,Clone,Copy,PartialEq,Eq, andHashfor public types when possible, in that order
DeriveDefaultfor public types when there is a reasonable default value
Always use full paths for logging withtracing::<op>!(...)instead of importing and using<op>!(...)
Prefer.to_string()or.to_owned()for&strtoStringconversion instead of.into()orString::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import thefmtmodule rather than using full paths when implementing traits fromcore::fmtorstd::fmt
Use relativeself::imports for sub-modules rather than bare module names
Place allusestatements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoidif let ... { } else { }constructs; usematchinstead when both branches are non-empty
Useif let ... { }without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g.,x.rs) withmod x;declaration rather than inlinemod x { .. }blocks, except fortestsandbenchesmodules
Use inlinemod tests { #[cfg(test)] ... }for test modules rather than creating separatetests.rsfiles
Use inlinemod benches { #[cfg(bench)] ... }for benchmark modules rather than creating separatebenches.rsfiles
**/*.rs: All trait bounds in generics must be placed inwhereclauses, not inline
UseSelfinstead of the type name when possible
Derive order must be:Debug,Clone,Copy,PartialEq,Eq,Hash(in this order)
Always usetracing::warn!(...), never import and usewarn!(...)
Use.to_string()or.to_owned()for string ...
Files:
crates/mdk-core/src/welcomes.rscrates/mdk-core/src/groups.rs
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Never use bare URLs in markdown files; all URLs must be properly formatted using
[link text](url),<url>, or([#123](url))for PR/issue references
Files:
crates/mdk-core/CHANGELOG.md
crates/*/CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Every crate CHANGELOG entry must include a PR reference using the format
([#123](https://github.com/marmot-protocol/mdk/pull/123))
Files:
crates/mdk-core/CHANGELOG.md
🧠 Learnings (8)
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: When adding or modifying code related to encrypted media (mip04 feature), ensure tests are run with the `mip04` feature: `cargo test --features mip04`
Applied to files:
crates/mdk-core/src/welcomes.rs
📚 Learning: 2025-12-03T10:17:44.932Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: .cursor/rules/marmot.mdc:0-0
Timestamp: 2025-12-03T10:17:44.932Z
Learning: Ensure all code implementation is on spec with the Marmot Protocol specification
Applied to files:
crates/mdk-core/src/welcomes.rscrates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Applies to crates/*/CHANGELOG.md : Every crate CHANGELOG entry must include a PR reference using the format `([#123](https://github.com/marmot-protocol/mdk/pull/123))`
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-01T15:51:58.505Z
Learnt from: dannym-arx
Repo: marmot-protocol/mdk PR: 98
File: crates/mdk-core/CHANGELOG.md:46-46
Timestamp: 2026-01-01T15:51:58.505Z
Learning: In CHANGELOG.md files, keep the GitHub issue references (e.g., #61) as-is; do not convert '#61' (with no space) into an MD heading '# 61'. The hash without a space is intentional for GitHub auto-linking. This guideline applies to all CHANGELOG.md files in the repo.
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Applies to **/*.md : Never use bare URLs in markdown files; all URLs must be properly formatted using `[link text](url)`, `<url>`, or `([#123](url))` for PR/issue references
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Every change that modifies functionality must update the CHANGELOG in the affected crate
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: All code must follow the Marmot Protocol specification exactly; ask clarifying questions if any part of the spec is unclear
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2025-12-12T14:58:57.710Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: .cursor/rules/mls.mdc:0-0
Timestamp: 2025-12-12T14:58:57.710Z
Learning: This repository implements the Marmot protocol, which integrates MLS with the Nostr protocol. Reference https://github.com/marmot-protocol/marmot for protocol specifications and design decisions.
Applied to files:
crates/mdk-core/CHANGELOG.md
🧬 Code graph analysis (1)
crates/mdk-core/src/welcomes.rs (1)
crates/mdk-core/src/util.rs (1)
from_tags(88-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Coverage
🔇 Additional comments (8)
crates/mdk-core/src/groups.rs (1)
1346-1348: LGTM! Base64 enforcement aligns with MIP-00/MIP-02 security requirements.The hardcoded
ContentEncoding::Base64with the explicit encoding tag correctly prevents downgrade attacks and parsing ambiguity across clients.crates/mdk-core/src/welcomes.rs (7)
392-418: LGTM! Robust error handling for missing encoding tag.The implementation correctly:
- Requires explicit encoding tag per MIP-00/MIP-02
- Persists a Failed ProcessedWelcome for auditability before returning
- Uses structured logging with the appropriate target
495-497: LGTM! Imports correctly hoisted to module scope.This addresses the prior feedback about placing
usestatements at the module level rather than inside function bodies, per Rust coding guidelines.
538-541: LGTM! Test correctly validates base64-encoded welcome content.The test properly verifies that welcome content is valid base64 using the
BASE64engine imported at module scope.
1138-1142: LGTM! Test correctly preserves encoding tag to isolate the error path under test.Adding the encoding tag ensures the test validates the intended behavior (unknown KeyPackage handling) rather than inadvertently triggering the missing encoding tag error.
1242-1256: LGTM! Size calculation correctly uses base64-decoded length.The fix properly computes the actual binary size by decoding the base64 content first, replacing the previous hex-based estimation (
len / 2).
1286-1287: LGTM! Comment correctly reflects MIP-02 tag requirements.The comment now accurately states "4 tags" (relays, e, client, encoding) per MIP-02.
1307-1316: LGTM! Test correctly validates invalid base64 content rejection.The test uses content with invalid base64 characters (
!!!) and verifies that processing fails appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/mdk-core/CHANGELOG.md (1)
30-40: Changelog entries have contradictory encoding documentation.The new breaking change entry (lines 30-35) correctly documents that hex encoding is removed and only base64 is now supported. However, the existing entries from PR #96 (lines 38, 54) still state that the encoding tag value can be "hex" or "base64".
This creates confusion for users reading the changelog. Consider updating lines 38 and 54 to reflect the current state (base64-only), or add a note clarifying that PR #98 supersedes the hex support mentioned in PR #96.
🔎 Suggested clarification
- **MIP-02 Welcome Event Validation**: Encoding tag is now required for all welcome events ([#96](https://github.com/marmot-protocol/mdk/pull/96)) - Welcome events must now include exactly 4 tags: `relays`, `e`, `client`, and `encoding` - - The `encoding` tag must have a value of either "hex" or "base64" + - The `encoding` tag must have a value of "base64" (hex support removed in [#98](https://github.com/marmot-protocol/mdk/pull/98))crates/mdk-core/src/welcomes.rs (1)
677-776: Tests 4-8 use invalid "hex" encoding and no longer test their intended validation cases.With the validation change at line 122 (only "base64" is accepted), tests 4-8 all fail at the encoding validation step instead of testing their specific cases (empty relays, invalid relay URL, etc.). The tests pass because they expect
InvalidWelcomeMessage, but they're testing the wrong thing.Update these tests to use
"base64"encoding so they actually exercise their intended validation paths.🔎 Proposed fix
// Test 4: Empty relays tag let mut tags4 = nostr::Tags::new(); tags4.push(nostr::Tag::relays(vec![])); // Empty relays tags4.push(nostr::Tag::event(EventId::all_zeros())); tags4.push(nostr::Tag::client("mdk".to_string())); - tags4.push(nostr::Tag::parse(&["encoding".to_string(), "hex".to_string()]).unwrap()); + tags4.push(nostr::Tag::parse(&["encoding".to_string(), "base64".to_string()]).unwrap()); // ... similar changes for tests 5-8 ... // Test 5: Invalid relay URL format - tags5.push(nostr::Tag::parse(&["encoding".to_string(), "hex".to_string()]).unwrap()); + tags5.push(nostr::Tag::parse(&["encoding".to_string(), "base64".to_string()]).unwrap()); // Test 6: Incomplete relay URL - tags6.push(nostr::Tag::parse(&["encoding".to_string(), "hex".to_string()]).unwrap()); + tags6.push(nostr::Tag::parse(&["encoding".to_string(), "base64".to_string()]).unwrap()); // Test 7: Empty e tag content - tags7.push(nostr::Tag::parse(&["encoding".to_string(), "hex".to_string()]).unwrap()); + tags7.push(nostr::Tag::parse(&["encoding".to_string(), "base64".to_string()]).unwrap()); // Test 8: Empty client tag content - tags8.push(nostr::Tag::parse(&["encoding".to_string(), "hex".to_string()]).unwrap()); + tags8.push(nostr::Tag::parse(&["encoding".to_string(), "base64".to_string()]).unwrap());
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/src/welcomes.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)
**/*.rs: All trait bounds should be written inwhereclauses rather than inline with type parameters in Rust
UseSelfinstead of the explicit type name when referring to the type in impl blocks
DeriveDebug,Clone,Copy,PartialEq,Eq, andHashfor public types when possible, in that order
DeriveDefaultfor public types when there is a reasonable default value
Always use full paths for logging withtracing::<op>!(...)instead of importing and using<op>!(...)
Prefer.to_string()or.to_owned()for&strtoStringconversion instead of.into()orString::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import thefmtmodule rather than using full paths when implementing traits fromcore::fmtorstd::fmt
Use relativeself::imports for sub-modules rather than bare module names
Place allusestatements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoidif let ... { } else { }constructs; usematchinstead when both branches are non-empty
Useif let ... { }without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g.,x.rs) withmod x;declaration rather than inlinemod x { .. }blocks, except fortestsandbenchesmodules
Use inlinemod tests { #[cfg(test)] ... }for test modules rather than creating separatetests.rsfiles
Use inlinemod benches { #[cfg(bench)] ... }for benchmark modules rather than creating separatebenches.rsfiles
**/*.rs: All trait bounds in generics must be placed inwhereclauses, not inline
UseSelfinstead of the type name when possible
Derive order must be:Debug,Clone,Copy,PartialEq,Eq,Hash(in this order)
Always usetracing::warn!(...), never import and usewarn!(...)
Use.to_string()or.to_owned()for string ...
Files:
crates/mdk-core/src/welcomes.rs
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Never use bare URLs in markdown files; all URLs must be properly formatted using
[link text](url),<url>, or([#123](url))for PR/issue references
Files:
crates/mdk-core/CHANGELOG.md
crates/*/CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Every crate CHANGELOG entry must include a PR reference using the format
([#123](https://github.com/marmot-protocol/mdk/pull/123))
Files:
crates/mdk-core/CHANGELOG.md
🧠 Learnings (8)
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: When adding or modifying code related to encrypted media (mip04 feature), ensure tests are run with the `mip04` feature: `cargo test --features mip04`
Applied to files:
crates/mdk-core/src/welcomes.rs
📚 Learning: 2025-12-03T10:17:44.932Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: .cursor/rules/marmot.mdc:0-0
Timestamp: 2025-12-03T10:17:44.932Z
Learning: Ensure all code implementation is on spec with the Marmot Protocol specification
Applied to files:
crates/mdk-core/src/welcomes.rscrates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Applies to crates/*/CHANGELOG.md : Every crate CHANGELOG entry must include a PR reference using the format `([#123](https://github.com/marmot-protocol/mdk/pull/123))`
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-01T15:51:58.505Z
Learnt from: dannym-arx
Repo: marmot-protocol/mdk PR: 98
File: crates/mdk-core/CHANGELOG.md:46-46
Timestamp: 2026-01-01T15:51:58.505Z
Learning: In CHANGELOG.md files, keep the GitHub issue references (e.g., #61) as-is; do not convert '#61' (with no space) into an MD heading '# 61'. The hash without a space is intentional for GitHub auto-linking. This guideline applies to all CHANGELOG.md files in the repo.
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Applies to **/*.md : Never use bare URLs in markdown files; all URLs must be properly formatted using `[link text](url)`, `<url>`, or `([#123](url))` for PR/issue references
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Every change that modifies functionality must update the CHANGELOG in the affected crate
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: All code must follow the Marmot Protocol specification exactly; ask clarifying questions if any part of the spec is unclear
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2025-12-12T14:58:57.710Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: .cursor/rules/mls.mdc:0-0
Timestamp: 2025-12-12T14:58:57.710Z
Learning: This repository implements the Marmot protocol, which integrates MLS with the Nostr protocol. Reference https://github.com/marmot-protocol/marmot for protocol specifications and design decisions.
Applied to files:
crates/mdk-core/CHANGELOG.md
🧬 Code graph analysis (1)
crates/mdk-core/src/welcomes.rs (1)
crates/mdk-core/src/util.rs (1)
from_tags(88-100)
🔇 Additional comments (11)
crates/mdk-core/CHANGELOG.md (1)
65-70: LGTM!The new entries for the security fix and removed hex encoding support have proper PR references and accurately document the changes.
crates/mdk-core/src/welcomes.rs (10)
119-130: LGTM!The validation correctly enforces base64-only encoding and returns an appropriate error for invalid values, aligning with the security policy to prevent downgrade attacks.
392-418: LGTM!The explicit encoding tag requirement is well-implemented with proper error handling:
- Creates a Failed ProcessedWelcome for audit trail
- Uses structured logging with full tracing path
- Returns descriptive error message
- Aligns with the
ContentEncoding::from_tagsreturningNonefor missing tags (perutil.rs)
495-497: LGTM!The base64 imports are properly hoisted to the tests module scope, addressing the previous review feedback and following the coding guidelines for import placement.
538-548: LGTM!The test correctly validates that welcome content is valid base64 with appropriate assertions and updated comments reflecting the base64-only policy.
1138-1143: LGTM!The test correctly preserves the encoding tag with "base64" value, with a helpful comment explaining the intent to avoid the missing encoding tag error path.
1242-1257: LGTM!The size calculation now correctly decodes the base64 content and measures the actual binary size. This addresses the previous review feedback about the incorrect hex-based size calculation.
1286-1288: LGTM!The comment now correctly states "4 tags" per MIP-02 requirements and mentions "base64-encoded MLS messages", addressing the previous review feedback.
1307-1316: LGTM!The test correctly validates that invalid base64 content is rejected, with appropriate error expectation and comment.
919-923: LGTM!Test correctly validates that welcome content is valid base64.
1059-1068: LGTM!Test correctly asserts that both welcome messages contain valid base64 content with appropriate comments.
…tent-enables-downgrade-and-interop-failures
❌ Coverage: 86.85% → 86.72% (-0.13%) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/mdk-core/src/key_packages.rs (1)
656-656: Rename test variables to reflect base64 content, not hex.Multiple test functions still use the identifier
key_package_hexorkey_package_strfor variables that now contain base64-encoded data. For consistency and clarity, rename these tokey_package_b64orkey_package_base64so readers immediately understand the encoding format.Examples of variables to rename
- Line 656:
key_package_str→key_package_b64- Line 940:
key_package_str→key_package_b64- Line 1107:
key_package_hex→key_package_b64- Line 1145:
key_package_hex→key_package_b64- Line 1190:
key_package_hex→key_package_b64- Line 1230:
key_package_hex→key_package_b64- Line 1793:
key_package_str→key_package_b64- Line 1811:
key_package_str→key_package_b64- Line 1843:
key_package_str→key_package_b64- Line 1865:
key_package_str→key_package_b64- Line 1870:
key_package_str→key_package_b64- Line 1892:
key_package_hex→key_package_b64Also applies to: 940-940, 1107-1107, 1145-1145, 1190-1190, 1230-1230, 1793-1793, 1811-1811, 1843-1843, 1865-1865, 1870-1870, 1892-1892
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/src/key_packages.rscrates/mdk-core/src/welcomes.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust.mdc)
**/*.rs: All trait bounds should be written inwhereclauses rather than inline with type parameters in Rust
UseSelfinstead of the explicit type name when referring to the type in impl blocks
DeriveDebug,Clone,Copy,PartialEq,Eq, andHashfor public types when possible, in that order
DeriveDefaultfor public types when there is a reasonable default value
Always use full paths for logging withtracing::<op>!(...)instead of importing and using<op>!(...)
Prefer.to_string()or.to_owned()for&strtoStringconversion instead of.into()orString::from()
Order imports in the following sequence: (1) core/alloc/std, (2) external crates, (3) current sub-modules with mod declarations, (4) internal crate modules
Import thefmtmodule rather than using full paths when implementing traits fromcore::fmtorstd::fmt
Use relativeself::imports for sub-modules rather than bare module names
Place allusestatements at the top of the file following the specified import order, never inside functions, methods, or other blocks
Avoidif let ... { } else { }constructs; usematchinstead when both branches are non-empty
Useif let ... { }without an else clause when a match arm would be intentionally empty
Define sub-modules using separate files (e.g.,x.rs) withmod x;declaration rather than inlinemod x { .. }blocks, except fortestsandbenchesmodules
Use inlinemod tests { #[cfg(test)] ... }for test modules rather than creating separatetests.rsfiles
Use inlinemod benches { #[cfg(bench)] ... }for benchmark modules rather than creating separatebenches.rsfiles
**/*.rs: All trait bounds in generics must be placed inwhereclauses, not inline
UseSelfinstead of the type name when possible
Derive order must be:Debug,Clone,Copy,PartialEq,Eq,Hash(in this order)
Always usetracing::warn!(...), never import and usewarn!(...)
Use.to_string()or.to_owned()for string ...
Files:
crates/mdk-core/src/key_packages.rscrates/mdk-core/src/welcomes.rs
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Never use bare URLs in markdown files; all URLs must be properly formatted using
[link text](url),<url>, or([#123](url))for PR/issue references
Files:
crates/mdk-core/CHANGELOG.md
crates/*/CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Every crate CHANGELOG entry must include a PR reference using the format
([#123](https://github.com/marmot-protocol/mdk/pull/123))
Files:
crates/mdk-core/CHANGELOG.md
🧠 Learnings (8)
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: When adding or modifying code related to encrypted media (mip04 feature), ensure tests are run with the `mip04` feature: `cargo test --features mip04`
Applied to files:
crates/mdk-core/src/key_packages.rscrates/mdk-core/src/welcomes.rs
📚 Learning: 2025-12-03T10:17:44.932Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: .cursor/rules/marmot.mdc:0-0
Timestamp: 2025-12-03T10:17:44.932Z
Learning: Ensure all code implementation is on spec with the Marmot Protocol specification
Applied to files:
crates/mdk-core/src/welcomes.rscrates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Applies to crates/*/CHANGELOG.md : Every crate CHANGELOG entry must include a PR reference using the format `([#123](https://github.com/marmot-protocol/mdk/pull/123))`
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-01T15:51:58.505Z
Learnt from: dannym-arx
Repo: marmot-protocol/mdk PR: 98
File: crates/mdk-core/CHANGELOG.md:46-46
Timestamp: 2026-01-01T15:51:58.505Z
Learning: In CHANGELOG.md files, keep the GitHub issue references (e.g., #61) as-is; do not convert '#61' (with no space) into an MD heading '# 61'. The hash without a space is intentional for GitHub auto-linking. This guideline applies to all CHANGELOG.md files in the repo.
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Applies to **/*.md : Never use bare URLs in markdown files; all URLs must be properly formatted using `[link text](url)`, `<url>`, or `([#123](url))` for PR/issue references
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: Every change that modifies functionality must update the CHANGELOG in the affected crate
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2026-01-03T08:39:39.799Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T08:39:39.799Z
Learning: All code must follow the Marmot Protocol specification exactly; ask clarifying questions if any part of the spec is unclear
Applied to files:
crates/mdk-core/CHANGELOG.md
📚 Learning: 2025-12-12T14:58:57.710Z
Learnt from: CR
Repo: marmot-protocol/mdk PR: 0
File: .cursor/rules/mls.mdc:0-0
Timestamp: 2025-12-12T14:58:57.710Z
Learning: This repository implements the Marmot protocol, which integrates MLS with the Nostr protocol. Reference https://github.com/marmot-protocol/marmot for protocol specifications and design decisions.
Applied to files:
crates/mdk-core/CHANGELOG.md
🧬 Code graph analysis (1)
crates/mdk-core/src/welcomes.rs (2)
crates/mdk-core/src/util.rs (1)
from_tags(88-100)crates/mdk-uniffi/src/lib.rs (1)
result(397-405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Coverage
🔇 Additional comments (3)
crates/mdk-core/src/welcomes.rs (2)
418-444: Well-structured error handling for missing encoding tag.The encoding tag validation properly creates a Failed ProcessedWelcome state, logs the error, and returns early. This prevents processing of ambiguous content and maintains security while preserving a failure audit trail in storage.
146-156: Encoding validation correctly enforces base64-only per MIP-02.The validation logic properly checks that the encoding tag value equals "base64" and rejects any other value (including "hex"), which aligns with the PR objectives to prevent downgrade attacks and parsing ambiguity.
crates/mdk-core/CHANGELOG.md (1)
30-35: Breaking changes clearly documented with security rationale.The changelog entry properly explains the breaking change (base64-only enforcement), its security benefits (preventing encoding ambiguity and downgrade attacks), and migration guidance for clients. The PR reference follows the required format.
Summary by CodeRabbit
Refactor
Security
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.