Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
efcfcad
feat(xmldsig): add enveloped signature transform and pipeline executor
polaz Mar 16, 2026
d7865b0
test(xmldsig): add integration tests for transform pipeline
polaz Mar 16, 2026
50ccb15
fix(xmldsig): replace expect() with error propagation, validate Inclu…
polaz Mar 16, 2026
d7f3edf
refactor(xmldsig): replace NodeId with Node in transform API for cros…
polaz Mar 16, 2026
4a2a4b1
docs(xmldsig): add explicit URL to XMLDSig spec reference in rustdoc
polaz Mar 16, 2026
662c929
fix(xmldsig): validate Transform namespace and require PrefixList att…
polaz Mar 16, 2026
f04bc62
refactor(xmldsig): remove doc param from transform API, rename error …
polaz Mar 16, 2026
1ad6bdf
feat(ci): add Rust code review instructions for automated PR analysis
polaz Mar 16, 2026
01097ec
refactor(xmldsig): restrict apply_transform visibility, use correct e…
polaz Mar 17, 2026
5bc19d6
test(xmldsig): use two real Signature elements in nested-signatures test
polaz Mar 17, 2026
277f3b4
refactor(xmldsig): mark TransformError non_exhaustive, simplify ptr::…
polaz Mar 17, 2026
9a8064b
docs(xmldsig): add non_exhaustive rationale, add code-review instruct…
polaz Mar 17, 2026
cf13f5d
Merge remote-tracking branch 'origin/main' into feat/#13-enveloped-tr…
polaz Mar 17, 2026
f19c79b
fix(xmldsig): validate Transforms element and reject unexpected children
polaz Mar 17, 2026
2423b0e
refactor(xmldsig): use #[expect] over #[allow] in test module, trim d…
polaz Mar 17, 2026
fc508b9
refactor(xmldsig): use expect() for hardcoded C14N URI invariant
polaz Mar 17, 2026
70f6f18
docs(ci): add non_exhaustive and expect_used to DO NOT Flag exclusions
polaz Mar 17, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .github/instructions/code-review.instructions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
applyTo: "**/*.rs"
---

# Code Review Instructions for xml-sec

## Scope Rules (CRITICAL)

- **Review ONLY code within the PR's diff.** Do not suggest inline fixes for unchanged lines.
- For issues in code **outside the diff**, suggest creating a **separate issue** instead of proposing code changes. Example: "Consider opening an issue to add namespace validation here — this is outside this PR's scope."
- **Read the PR description carefully.** If the PR body has an "out of scope" section listing items handled by other PRs, do NOT flag those items.

## Rust Standards

- `unsafe` blocks require `// SAFETY:` comments explaining the invariant
- Prefer `#[expect(lint)]` over `#[allow(lint)]` — `#[expect]` warns when suppression becomes unnecessary
- Use `TryFrom`/`TryInto` for fallible conversions; `as` casts need `#[expect(clippy::cast_possible_truncation)]` with reason
- No `unwrap()` / `expect()` on I/O paths — use `Result` propagation
- `expect()` is acceptable for programmer invariants (lock poisoning) with `#[expect(clippy::expect_used, reason = "...")]`
- Code must pass `cargo clippy --all-features -- -D warnings`

## Testing

- Corruption/validation tests: tamper the relevant field (e.g., digest value, signature bytes, base64 encoding) and assert the error
- Use same serialization as production (e.g., canonical XML output, not shortcuts)
- Test naming: `fn <what>_<condition>_<expected>()`
80 changes: 80 additions & 0 deletions .github/instructions/rust.instructions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
---
applyTo: "**/*.rs"
---

# Rust Code Review Instructions

## Review Priority (HIGH -> LOW)

Focus review effort on real bugs, not cosmetics. Stop after finding issues in higher tiers -- do not pad reviews with low-priority nitpicks.

### Tier 1 -- Logic Bugs and Correctness (MUST flag)
- Data corruption: wrong algorithm, incorrect ordering, dropped or duplicated data
- Off-by-one in boundaries, index lookups, or range operations
- Checksum/hash mismatches: computing over wrong byte range, verifying against stale value
- TOCTOU: checking state then acting on it without holding a lock or atomic operation
- Missing validation: unchecked index, unvalidated input from network/disk/external source
- Resource leaks: unclosed file handles, missing cleanup on error paths
- Concurrency: data races, lock ordering violations, missing synchronization
- Error swallowing: `let _ = fallible_call()` silently dropping errors that affect correctness
- Integer overflow/truncation on sizes, offsets, lengths, or security-critical values

### Tier 2 -- Safety and Crash Recovery (MUST flag)
- `unsafe` without `// SAFETY:` invariant explanation
- `unwrap()`/`expect()` on I/O, network, or deserialization paths (must use `Result` propagation)
- Crash safety: write ordering that leaves data unrecoverable after power loss
- Sensitive data (keys, passwords, tokens) not wrapped in `Zeroize`/`Zeroizing`
- Constant-time comparison not used for cryptographic MACs/checksums
- Hardcoded secrets, credentials, or private URLs

### Tier 3 -- API Design and Robustness (flag if clear improvement)
- Public API missing `#[must_use]` on builder-style methods or non-`Result` types callers might discard
- `pub` visibility where `pub(crate)` suffices
- Missing `Send + Sync` bounds on types used across threads
- `Clone` on large types where a reference would work
- Fallible operations returning `()` instead of `Result`

### Tier 4 -- Style (ONLY flag if misleading or confusing)
- Variable/function names that actively mislead about behavior
- Dead code (unused functions, unreachable branches)

## DO NOT Flag (Explicit Exclusions)

These are not actionable review findings. Do not raise them:

- **Comment wording vs code behavior**: If a comment says "flush when full" but the threshold uses `>=` not `>`, the intent is clear. Do not suggest rewording comments to match exact operators or implementation details. Comments describe intent, not repeat the code.
- **Comment precision**: "returns the key" when it technically returns `Result<Key>` -- the comment conveys meaning, not type signature.
- **Magic numbers with context**: `4` in `assert_eq!(header.len(), 4, "expected u32 checksum")` -- the assertion message provides context. Do not suggest a named constant when the value is used once in a test with an explanatory message.
- **Domain constants**: Specific numeric values for block sizes (e.g., `4096`), key sizes, protocol version numbers, port numbers, or configuration defaults are domain constants, not magic numbers, when used with surrounding context.
- **Minor naming preferences**: `lvl` vs `level`, `opts` vs `options`, `enc_part` vs `encrypted_part` -- these are team style, not bugs.
- **Import ordering**: Import grouping or ordering style. Unused imports are NOT cosmetic -- they cause `clippy -D warnings` failures and must be removed.
- **Test code style**: Tests prioritize readability and explicitness over DRY. Repeated setup code in tests is acceptable.
- **`#[allow(clippy::...)]` with justification comment**: When `#[allow]` has an adjacent comment explaining why it is used instead of `#[expect]`, do not suggest switching. Common reason: the lint does not fire on the current code but the suppression is kept defensively. `#[expect]` would fail the build with "unfulfilled expectation" in that case.
- **`#[allow(clippy::...)]` in existing/unchanged code**: Do not flag `#[allow]` suppressions in unchanged lines. New code should use `#[expect]` when the lint actually fires.
- **Temporary directory strategies in existing code**: Existing tests using manual temp paths are not a finding. New tests should prefer `tempfile::tempdir()`.
- **Semver concerns on pre-1.0 crates**: Adding required methods to public traits, changing public API signatures, or restructuring modules in a crate with version `0.x.y` is expected and does not require a default implementation or sealed trait pattern. Semver breakage only matters at `>= 1.0.0`.
- **Test infrastructure scripts (shell)**: Shell scripts in `tests/` that set up Docker containers, KDCs, databases, or other test infrastructure are not production code. Do not flag hardcoded test credentials (they are overridable via env vars), redundant principal creation (required by the specific technology), or non-production patterns in these scripts.
- **Previously resolved review threads**: If the same suggestion was already raised and resolved in a prior review round on this PR, do not re-raise it. Check the resolved threads before flagging.

## Scope Rules

- **Review ONLY code within the PR's diff.** Do not suggest inline fixes for unchanged lines.
- For issues **outside the diff**, suggest opening a separate issue.
- **Read the PR description.** If it lists known limitations or deferred items, do not re-flag them.

## Rust-Specific Standards

- Prefer `#[expect(lint)]` over `#[allow(lint)]` when the lint actually fires -- `#[expect]` warns when suppression becomes unnecessary. Use `#[allow(lint)]` with a justification comment when the lint does not fire but defensive suppression is desired.
- `TryFrom`/`TryInto` for fallible conversions; `as` casts need justification
- No `unwrap()` / `expect()` on I/O paths -- use `?` propagation
- `expect()` is acceptable for programmer invariants (e.g., lock poisoning, `const` construction) with reason
- Code must pass `cargo clippy --all-features -- -D warnings`
- RFC/spec compliance comments (e.g., "RFC 4120 section 7.5.1") are documentation -- preserve them

## Testing Standards

- Test naming: `fn <what>_<condition>_<expected>()` or `fn test_<scenario>()`
- Use `tempfile::tempdir()` for test directories -- ensures cleanup even on panic
- Integration tests that require infrastructure use `#[ignore = "reason"]`
- Prefer `assert_eq!` with message over bare `assert!` for better failure output
- Hardcoded values in tests are fine when accompanied by explanatory comments or assertion messages
2 changes: 2 additions & 0 deletions src/xmldsig/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
//! - ID attribute resolution with configurable attribute names
//! - Node set types for the transform pipeline

pub mod transforms;
pub mod types;
pub mod uri;

pub use transforms::{execute_transforms, parse_transforms, Transform};
pub use types::{NodeSet, TransformData, TransformError};
Loading
Loading