fix(sns_aggregator): restrict reconfigure to controllers and clamp short intervals - #8034
Open
yhabib wants to merge 2 commits into
Open
fix(sns_aggregator): restrict reconfigure to controllers and clamp short intervals#8034yhabib wants to merge 2 commits into
yhabib wants to merge 2 commits into
Conversation
The reconfigure update method took a new Config from any principal, and the anonymous principal could call it. The method exists only in the development build, which the Dockerfile publishes as sns_aggregator_dev.wasm.gz. Add a controller check before the method changes any state. Raise an update interval below 100 ms to the minimum, so a 0 ms interval cannot drive a continuous data collection loop.
|
✅ No security or compliance issues detected. Reviewed everything up to 3d8cf11. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
🟢 Approved
The changes address the stated security risk with clear, centralized logic and targeted unit tests covering the new behavior.
Pull request overview
This PR hardens the SNS aggregator canister’s (dev-only) reconfigure update method by restricting it to canister controllers and preventing cycle-burning configurations via a minimum interval clamp, with accompanying unit tests and a security changelog entry.
Changes:
- Enforced a controller-only guard for
reconfigurevia a dedicatedauthmodule. - Centralized config persistence + interval clamping in
apply_config, applied duringsetup(coveringinit,post_upgrade, and stable-memory-restored configs). - Added unit tests (including test doubles for system APIs) and documented the security fix in the changelog.
File summaries
| File | Description |
|---|---|
| rs/sns_aggregator/src/lib.rs | Adds controller check for reconfigure and introduces apply_config to clamp intervals during setup. |
| rs/sns_aggregator/src/state.rs | Adds Config::MIN_INTERVAL_MS and raise_short_intervals helper for interval clamping. |
| rs/sns_aggregator/src/auth.rs | Implements controller-only authorization check with trap/panic behavior depending on build context. |
| rs/sns_aggregator/src/auth/test_api.rs | Provides test doubles for caller/controllers and trap behavior to enable unit testing outside a canister. |
| rs/sns_aggregator/src/tests.rs | Adds unit tests covering controller gating, reconfigure rejection behavior, and config clamping via apply_config. |
| rs/sns_aggregator/src/state/tests.rs | Adds unit tests for Config::raise_short_intervals. |
| CHANGELOG-Sns_Aggregator.md | Documents the controller restriction and minimum interval clamp under Unreleased/Security. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Motivation
The
reconfigureupdate method took a newConfigfrom any principal, including the anonymous principal. It exists only in the development build (sns_aggregator_dev.wasm.gz), but any test deployment built from that wasm was open to it. An attacker could halt data refresh or set a 0 ms interval and burn cycles until the canister freezes.Changes
reconfiguretouches any state.apply_config, which stores aConfigand raises any interval below 100 ms to that minimum.apply_configinsetup, soinit,post_upgrade, and a config left in stable memory all get the same clamp.reconfigureitself.CHANGELOG-Sns_Aggregator.md.Tests
cargo test --package sns_aggregator: 13 passed, 0 failed, 2 ignored (both ignored before this change).cargo test --workspace: all tests pass, 0 failed../scripts/lint-rs: no warning, run with--all-features../scripts/fmt-rs: no change.reconfiguretests fail. Removing the clamp made bothapply_configclamp tests fail. Restoring both brought all 13 tests back to passing.Todos
CHANGELOG-Sns_Aggregator.mdunderUnreleased/Security.