🚀 feat: Add support for redacting values#3759
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds configurable redaction of sensitive values across multiple middlewares (cache, csrf, idempotency, limiter, cors). Introduces per-manager Changes
Sequence Diagram(s)sequenceDiagram
participant Init as Middleware Init
participant Manager as Storage/Manager
participant Mask as maskKey / logKey
participant Storage as Storage Backend
Note over Init,Manager: Initialization passes redact flag
Init->>Manager: newManager(storage, redactEnabled)
Manager->>Mask: set redact behavior
Note over Manager,Storage: runtime operations (Get/Set/Del)
Manager->>Storage: perform storage op (uses actual key)
Storage-->>Manager: success / error
alt storage error or unexpected type
Manager->>Mask: maskKey(key)
Mask-->>Manager: "[redacted]" or original
Manager->>Init: log/error with maskedKey
else success
Manager->>Init: return result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3759 +/- ##
=======================================
Coverage 91.44% 91.44%
=======================================
Files 113 113
Lines 11824 11865 +41
=======================================
+ Hits 10812 10850 +38
- Misses 745 748 +3
Partials 267 267
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
middleware/idempotency/idempotency.go (1)
109-109: Redacted unlock log: consider adding a non-reversible fingerprint for correlation.Optional: include a short HMAC/SHA-256 fingerprint (or even len(key)) behind a config flag to correlate repeated failures without exposing the key.
middleware/cache/manager.go (1)
37-37: Avoid drift: centralize the placeholder.Multiple middleware packages define the same redactedKey. Consider a tiny internal shared helper (e.g., middleware/internal/redact.Placeholder()) to keep a single source of truth and enable future upgrades (e.g., hashed tokens).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
middleware/cache/manager.go(9 hunks)middleware/idempotency/idempotency.go(2 hunks)middleware/limiter/manager.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/limiter/manager.gomiddleware/idempotency/idempotency.gomiddleware/cache/manager.go
🧬 Code graph analysis (1)
middleware/idempotency/idempotency.go (1)
log/fiberlog.go (1)
Errorf(49-51)
⏰ 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: repeated
- GitHub Check: Compare
🔇 Additional comments (3)
middleware/idempotency/idempotency.go (1)
24-24: Consistent key redaction placeholder.Good addition; aligns with cache/limiter changes and avoids leaking raw keys.
middleware/limiter/manager.go (1)
29-29: LGTM: limiter uses the shared redaction pattern consistently.Matches cache/idempotency behavior; no control-flow changes.
Also applies to: 68-68, 74-74, 88-88, 100-100, 104-104
middleware/cache/manager.go (1)
86-86: Approve — redaction verified across middleware packagesrg scan returned no matches for format strings that log raw
keyin middleware/**; redaction is consistently applied.
There was a problem hiding this comment.
Pull Request Overview
This PR implements key redaction across multiple middleware components to prevent sensitive information from being exposed in error messages and logs.
- Adds configurable key redaction with
RedactKeysfield to cache, CSRF, CORS, and idempotency middlewares - Replaces direct key references in error messages with redacted placeholders (
[redacted]) by default - Maintains backward compatibility by allowing users to disable redaction when needed
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/limiter/manager.go | Replaces key values with redacted constant in error messages |
| middleware/idempotency/idempotency.go | Adds RedactKeys configuration and maskKey function for log messages |
| middleware/idempotency/config.go | Adds RedactKeys configuration field |
| middleware/csrf/storage_manager.go | Updates constructor to accept redactKeys parameter and adds logKey method |
| middleware/csrf/csrf_test.go | Updates test calls to include redactKeys parameter |
| middleware/csrf/csrf.go | Adds RedactKeys configuration and maskValue function for panic messages |
| middleware/csrf/config.go | Adds RedactKeys configuration field |
| middleware/cors/cors_test.go | Adds test for redaction behavior in panic messages |
| middleware/cors/cors.go | Adds RedactKeys configuration and maskValue function for panic messages |
| middleware/cors/config.go | Adds RedactKeys configuration field |
| middleware/cache/manager_test.go | Updates test calls and adds logKey method test |
| middleware/cache/manager.go | Updates constructor and adds logKey method for error message redaction |
| middleware/cache/config.go | Adds RedactKeys configuration field |
| middleware/cache/cache.go | Adds RedactKeys configuration and maskKey function for error messages |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable security feature by adding support for redacting sensitive keys in logs and error messages across several middlewares. The implementation is mostly solid, with new configuration options and logic to mask keys when enabled. I've pointed out a few areas for improvement:
- In
middleware/cors/cors.go, there's a small piece of redundant logic for initializing theredactKeysflag that can be simplified. - Across
middleware/cache/manager.go,middleware/csrf/storage_manager.go, andmiddleware/idempotency/idempotency.go, the format specifier in error messages was changed from%qto%s. I recommend reverting this to%qto ensure keys are always properly quoted in logs for better readability and safety.
Overall, this is a great enhancement for security and logging hygiene. Addressing these minor points will make the implementation even more robust.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
middleware/cors/cors_test.go (1)
346-346: Minor inconsistency in test mounting pattern.This test now mounts the CORS middleware with a specific route prefix
"/"unlike most other tests that mount it directly on the app. While this doesn't affect functionality, it's inconsistent with the testing pattern used elsewhere in the file.Consider maintaining consistency with other tests by mounting without the route prefix:
- app.Use("/", New(Config{ + app.Use(New(Config{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
docs/middleware/cache.md(3 hunks)docs/middleware/cors.md(3 hunks)docs/middleware/csrf.md(2 hunks)docs/middleware/idempotency.md(2 hunks)docs/whats_new.md(5 hunks)middleware/cache/cache.go(7 hunks)middleware/cache/config.go(2 hunks)middleware/cache/manager.go(9 hunks)middleware/cache/manager_test.go(2 hunks)middleware/cors/config.go(2 hunks)middleware/cors/cors.go(3 hunks)middleware/cors/cors_test.go(1 hunks)middleware/csrf/config.go(2 hunks)middleware/csrf/csrf.go(2 hunks)middleware/csrf/csrf_test.go(2 hunks)middleware/csrf/storage_manager.go(3 hunks)middleware/idempotency/config.go(3 hunks)middleware/idempotency/idempotency.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- middleware/cache/manager.go
- middleware/idempotency/idempotency.go
🧰 Additional context used
📓 Path-based instructions (2)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/middleware/idempotency.mddocs/middleware/cors.mddocs/middleware/csrf.mddocs/middleware/cache.mddocs/whats_new.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/csrf/config.gomiddleware/cors/config.gomiddleware/cache/config.gomiddleware/cache/cache.gomiddleware/csrf/csrf.gomiddleware/cors/cors.gomiddleware/idempotency/config.gomiddleware/csrf/storage_manager.gomiddleware/csrf/csrf_test.gomiddleware/cache/manager_test.gomiddleware/cors/cors_test.go
🧠 Learnings (4)
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
PR: gofiber/fiber#3625
File: middleware/session/config.go:57-58
Timestamp: 2025-07-27T17:28:53.403Z
Learning: In the session middleware `Config` struct, the `Extractor` field uses function closures (like `FromCookie(key)`), making it impossible to introspect extractor parameters at runtime for validation purposes without complex reflection techniques.
Applied to files:
docs/middleware/csrf.md
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Applied to files:
docs/middleware/csrf.mdmiddleware/csrf/csrf_test.go
📚 Learning: 2024-09-25T16:17:00.969Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-09-25T16:17:00.969Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
Applied to files:
docs/middleware/csrf.mdmiddleware/csrf/csrf_test.go
📚 Learning: 2024-07-01T03:44:03.672Z
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Applied to files:
docs/whats_new.md
🧬 Code graph analysis (5)
middleware/cache/cache.go (2)
internal/storage/memory/memory.go (1)
Storage(15-20)storage_interface.go (1)
Storage(10-46)
middleware/idempotency/config.go (2)
internal/memory/memory.go (1)
Storage(13-16)internal/storage/memory/memory.go (1)
Storage(15-20)
middleware/csrf/storage_manager.go (2)
storage_interface.go (1)
Storage(10-46)middleware/csrf/csrf.go (1)
New(47-214)
middleware/csrf/csrf_test.go (1)
middleware/csrf/config.go (1)
Config(16-126)
middleware/cors/cors_test.go (2)
middleware/cors/cors.go (1)
New(16-200)middleware/cors/config.go (1)
Config(8-79)
🪛 GitHub Check: lint
middleware/csrf/storage_manager.go
[failure] 27-27:
struct-tag: tag on not-exported field redactKeys (revive)
🪛 GitHub Actions: golangci-lint
middleware/csrf/storage_manager.go
[error] 27-27: golangci-lint: struct-tag: tag on not-exported field redactKeys (revive)
⏰ 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: Compare
- GitHub Check: repeated
🔇 Additional comments (40)
docs/middleware/csrf.md (1)
45-47: LGTM - Clear documentation of the new RedactKeys feature.The comment and documentation appropriately describe the new RedactKeys configuration option for hiding sensitive tokens and storage keys in logs and error messages. The inline comment style is consistent with the codebase.
docs/middleware/cache.md (3)
107-108: LGTM - Clear feature documentation.The documentation clearly explains the purpose of RedactKeys for masking cache keys in logs and error messages when dealing with sensitive identifiers.
118-118: LGTM - Configuration table documentation is accurate.The table entry correctly documents the RedactKeys option with proper type, description, and default value consistent with the implementation in the config files.
135-135: LGTM - Default config example is accurate.The RedactKeys field is correctly documented with its default value of false, maintaining consistency with the actual configuration implementation.
middleware/idempotency/config.go (2)
51-54: LGTM - Well-documented configuration field.The RedactKeys field is properly documented with clear comments explaining its purpose and default value. The documentation follows the established pattern used in other middleware.
79-81: LGTM - Consistent default configuration.The ConfigDefault properly initializes RedactKeys to false, maintaining consistency with other middleware implementations and preserving backward compatibility.
docs/middleware/idempotency.md (3)
71-72: LGTM - Clear introductory documentation.The documentation appropriately introduces the RedactKeys feature, explaining when it should be enabled for hiding sensitive idempotency keys from logs and error messages.
80-80: LGTM - Accurate configuration table entry.The table entry correctly documents RedactKeys with proper type (bool), clear description, and accurate default value (false).
109-109: LGTM - Consistent default config documentation.The ConfigDefault example correctly shows RedactKeys: false, maintaining consistency with the actual implementation.
middleware/csrf/config.go (2)
100-104: LGTM - Well-documented RedactKeys field.The field is properly documented with clear comments explaining its purpose for redacting CSRF tokens and storage keys in logs and errors. The documentation follows the established pattern and includes the correct default value.
139-139: LGTM - Consistent default configuration.The ConfigDefault properly initializes RedactKeys to false, maintaining backward compatibility and consistency with other middleware implementations.
middleware/cors/config.go (2)
59-63: LGTM - Clear documentation of RedactKeys field.The field is well-documented with appropriate comments explaining its purpose for redacting configuration values and origins in logs and panics. The documentation is consistent with the pattern used in other middleware.
86-86: LGTM - Proper default configuration.The ConfigDefault correctly initializes RedactKeys to false, maintaining consistency with other middleware implementations and ensuring backward compatibility.
middleware/cors/cors_test.go (1)
322-338: LGTM - Comprehensive test for RedactKeys functionality.The test properly verifies that the RedactKeys configuration controls redaction behavior in panic messages. It tests both the enabled (redacted) and disabled (unredacted) states, ensuring the feature works as intended.
docs/middleware/cors.md (3)
13-13: LGTM - Clear introduction of RedactKeys feature.The documentation appropriately introduces the RedactKeys feature in the context of origin validation and security, explaining when it should be used for sensitive data.
121-121: LGTM - Accurate configuration table entry.The table entry correctly documents the RedactKeys option with proper type, clear description, and accurate default value.
137-137: LGTM - Consistent default config documentation.The ConfigDefault example correctly shows RedactKeys: false, maintaining consistency with the actual implementation.
middleware/cache/config.go (2)
65-68: LGTM! Well documented configuration addition.The
RedactKeysfield is properly documented with clear purpose and defaults. The implementation follows the established pattern for boolean configuration fields.
89-89: LGTM! Consistent default initialization.The default value for
RedactKeysis correctly set tofalse, maintaining backward compatibility.middleware/csrf/storage_manager.go (3)
30-38: LGTM! Clean constructor signature update.The constructor now properly accepts the redaction flag and stores it for use by the
logKeymethod. This provides a clear separation of concerns.
75-75: LGTM! Consistent key redaction in error messages.The error messages now use
logKey()to redact storage keys when redaction is enabled. This prevents sensitive key information from being exposed in logs and error messages.Also applies to: 89-89
98-103: LGTM! Simple and effective key redaction logic.The
logKeymethod provides a clean way to conditionally redact keys based on the configuration flag. The implementation is straightforward and consistent with the pattern used across other middleware.docs/whats_new.md (1)
1164-1164: LGTM! Consistent documentation of RedactKeys feature.The documentation accurately describes the new
RedactKeysconfiguration option across multiple middleware components (Cache, CORS, CSRF, and Idempotency). The consistent messaging about defaulting tofalseand the purpose of masking sensitive keys in logs and error messages is well done.Also applies to: 1187-1187, 1201-1201, 1205-1205, 1225-1225, 1263-1263
middleware/csrf/csrf.go (3)
51-58: LGTM! Clean redaction helper implementation.The
maskValuehelper function provides a clean way to conditionally redact values based on the configuration. The logic is simple and consistent with the pattern used across other middleware.
66-66: LGTM! Updated constructor call for redaction support.The storage manager constructor call is properly updated to pass the
redactKeysflag, enabling key redaction in storage operations.
79-79: LGTM! Consistent origin masking in error messages.The panic messages for invalid trusted origins now use
maskValue()to redact potentially sensitive hostname information when redaction is enabled. This prevents leaking internal or sensitive hostnames in error messages.Also applies to: 87-87
middleware/cache/manager_test.go (2)
15-15: LGTM! Test updated for new constructor signature.The test correctly uses the new two-argument constructor signature with redaction enabled.
34-42: LGTM! Comprehensive test for key redaction.The new test
Test_manager_logKeyproperly verifies both redacted and non-redacted behavior by testing thelogKeymethod with different constructor configurations. This ensures the redaction feature works correctly.middleware/csrf/csrf_test.go (2)
735-735: LGTM! Tests updated for new constructor signature.The test setup correctly uses the new two-argument
newStorageManagerconstructor signature with redaction enabled.Also applies to: 743-743
752-760: LGTM! Comprehensive test for storage manager key redaction.The new test
Test_storageManager_logKeyproperly verifies both redacted and non-redacted behavior by testing thelogKeymethod with different constructor configurations. This ensures the redaction feature works correctly in the CSRF storage manager.middleware/cors/cors.go (3)
13-13: LGTM! Consistent redaction constant.The
redactedValueconstant follows the same naming and value pattern used across other middleware components for key redaction.
30-40: LGTM! Clean redaction configuration and helper.The redaction flag is properly extracted from the configuration and the
maskValuehelper provides a clean way to conditionally redact values. The logic is consistent with the pattern used across other middleware.
68-68: LGTM! Origin redaction in error messages.The panic messages for invalid origin formats now use
maskValue()to redact potentially sensitive hostname information when redaction is enabled. This prevents leaking sensitive origins in error messages while maintaining the same functionality.Also applies to: 76-76
middleware/cache/cache.go (7)
74-81: LGTM! Clean redaction configuration.The redaction flag is properly extracted from the configuration and the
maskKeyhelper provides a clean way to conditionally redact cache keys. The implementation is consistent with the pattern used across other middleware.
96-96: LGTM! Manager constructor updated for redaction.The cache manager constructor call is properly updated to pass the
redactKeysflag, enabling key redaction in cache operations.
170-170: Use %s format specifier for consistency.The format specifier
%qis used with a potentially redacted key. SincemaskKey(key)may return[redacted]which doesn't need quotes, this could result in confusing output like"[redacted]". Use%sinstead for consistency with other error messages.- return fmt.Errorf("cache: failed to delete expired key %q: %w", maskKey(key), err) + return fmt.Errorf("cache: failed to delete expired key %s: %w", maskKey(key), err)
186-186: LGTM! Consistent function signature update.The
cacheBodyFetchErrorfunction is correctly called with themaskKeyfunction as the first parameter, enabling key redaction in the error message.
267-267: Use %s format specifier for consistency.Similar to the previous comment, the format specifier
%qis used with a potentially redacted key. Use%sinstead to avoid unnecessary quotes around[redacted].- return fmt.Errorf("cache: failed to delete key %q while evicting: %w", maskKey(keyToRemove), err) + return fmt.Errorf("cache: failed to delete key %s while evicting: %w", maskKey(keyToRemove), err)
335-335: Use %s format specifier for consistency.Similar to the previous comments, the format specifier
%qis used with a potentially redacted key. Use%sinstead to avoid unnecessary quotes around[redacted].- cleanupErr = errors.Join(cleanupErr, fmt.Errorf("cache: failed to delete raw key %q after store error: %w", maskKey(rawKey), err)) + cleanupErr = errors.Join(cleanupErr, fmt.Errorf("cache: failed to delete raw key %s after store error: %w", maskKey(rawKey), err))
397-399: Use %s format specifier for consistency.The format specifier
%qis used with a potentially redacted key. Sincemask(key)may return[redacted]which doesn't need quotes, use%sinstead for consistency with other error messages.- return fmt.Errorf("cache: no cached body for key %q: %w", mask(key), err) + return fmt.Errorf("cache: no cached body for key %s: %w", mask(key), err)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable security and privacy feature by adding configurable redaction for sensitive values in logs and error messages across the Cache, CSRF, CORS, and Idempotency middlewares. The implementation is consistent and well-documented for these middlewares.
My review focuses on an inconsistency found in the limiter middleware, where redaction is applied unconditionally, unlike the other middlewares. I've also suggested a minor improvement to the logging format for consistency.
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 (1)
middleware/limiter/config.go (1)
1-130: Rename remaining DisableRedactedValues → DisableValueRedaction in limiter package and docsSearch/replace remaining occurrences; update the Config field/comment and default, change calls to newManager to use !cfg.DisableValueRedaction, and update tests/docs.
- middleware/limiter/config.go — rename field + update ConfigDefault
- middleware/limiter/limiter_fixed.go — newManager(cfg.Storage, !cfg.DisableValueRedaction)
- middleware/limiter/limiter_sliding.go — newManager(cfg.Storage, !cfg.DisableValueRedaction)
- middleware/limiter/limiter_test.go — update test config usages
- docs/middleware/limiter.md — update text and config table
- docs/whats_new.md — update mentions
🧹 Nitpick comments (8)
docs/whats_new.md (1)
1397-1398: Unify naming: use DisableValueRedaction (not DisableRedactedValues) for limiter to match other middlewares.Keeps public API consistent with Cache/CORS/CSRF/Idempotency.
- Limiter now redacts request keys in error paths by default. A new `DisableRedactedValues` boolean (default `false`) lets you reveal the raw limiter key if diagnostics require it. + Limiter now redacts request keys in error paths by default. A new `DisableValueRedaction` boolean (default `false`) lets you reveal the raw limiter key if diagnostics require it.middleware/limiter/limiter_fixed.go (1)
24-24: Rename field for consistency + tiny readability tweak.Prefer DisableValueRedaction to align with other middlewares; name the negation for clarity.
- manager := newManager(cfg.Storage, !cfg.DisableRedactedValues) + // Redact by default unless explicitly disabled. + redactEnabled := !cfg.DisableValueRedaction + manager := newManager(cfg.Storage, redactEnabled)middleware/limiter/limiter_sliding.go (1)
25-25: Consistent field name + readability.Match other middleware naming and make intent explicit.
- manager := newManager(cfg.Storage, !cfg.DisableRedactedValues) + redactEnabled := !cfg.DisableValueRedaction + manager := newManager(cfg.Storage, redactEnabled)docs/middleware/limiter.md (3)
9-10: Rename to DisableValueRedaction to match rest of the project.-Limiter redacts request keys in error paths by default so storage identifiers and rate-limit keys don't leak into logs. Set `DisableRedactedValues` to `true` when you explicitly need the raw key for troubleshooting. +Limiter redacts request keys in error paths by default so storage identifiers and rate-limit keys don't leak into logs. Set `DisableValueRedaction` to `true` when you explicitly need the raw key for troubleshooting.
111-112: Config table: use DisableValueRedaction.-| DisableRedactedValues | `bool` | Disables redaction of limiter keys in error messages and logs. | false | +| DisableValueRedaction | `bool` | Disables redaction of limiter keys in error messages and logs. | false |
137-138: Default config: use DisableValueRedaction.- DisableRedactedValues: false, + DisableValueRedaction: false,middleware/limiter/config.go (2)
71-75: Public API naming consistency: DisableValueRedaction (not DisableRedactedValues).Aligns with Cache/CORS/CSRF/Idempotency flags.
- // DisableRedactedValues turns off masking limiter keys in logs and error messages when set to true. + // DisableValueRedaction turns off masking limiter keys in logs and error messages when set to true. // // Default: false - DisableRedactedValues bool + DisableValueRedaction bool
90-91: Default config field rename to match above.- DisableRedactedValues: false, + DisableValueRedaction: false,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/middleware/limiter.md(3 hunks)docs/whats_new.md(6 hunks)middleware/limiter/config.go(2 hunks)middleware/limiter/limiter_fixed.go(1 hunks)middleware/limiter/limiter_sliding.go(1 hunks)middleware/limiter/limiter_test.go(2 hunks)middleware/limiter/manager.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- middleware/limiter/limiter_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/limiter/manager.go
🧰 Additional context used
📓 Path-based instructions (2)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/middleware/limiter.mddocs/whats_new.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/limiter/config.gomiddleware/limiter/limiter_fixed.gomiddleware/limiter/limiter_sliding.go
🧠 Learnings (1)
📚 Learning: 2024-07-01T03:44:03.672Z
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Applied to files:
docs/whats_new.md
🧬 Code graph analysis (1)
middleware/limiter/limiter_fixed.go (3)
internal/memory/memory.go (1)
Storage(13-16)internal/storage/memory/memory.go (1)
Storage(15-20)storage_interface.go (1)
Storage(10-46)
⏰ 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). (3)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
🔇 Additional comments (6)
docs/whats_new.md (6)
1164-1165: LGTM – clear cache key redaction note.
1187-1188: LGTM – concise CORS origin redaction doc.
1201-1202: LGTM – CSRF token/key redaction doc reads well.
1204-1206: LGTM – Idempotency redaction defaults documented.
2227-2227: LGTM – CSRF migration toggle mention.
2264-2266: LGTM – Idempotency migration toggle mention.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable security feature by adding configurable redaction for sensitive values (like keys and tokens) in logs and error messages across several middlewares: Cache, CSRF, CORS, Idempotency, and Limiter. The implementation is solid and includes corresponding documentation and test updates. My main feedback is to improve API consistency by renaming the configuration flag in the Limiter middleware to match the name used in all other updated middlewares.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
middleware/limiter/limiter_sliding.go (1)
25-25: Redaction flag propagation looks correct; tiny readability nitConsider naming the boolean before passing it to avoid the negation-at-call-site read.
- manager := newManager(cfg.Storage, !cfg.DisableValueRedaction) + redactKeys := !cfg.DisableValueRedaction + manager := newManager(cfg.Storage, redactKeys)Also, please confirm that manager.get/set wrap storage errors with a redacted key when redaction is enabled so callers don’t need to include the key in their fmt.Errorf wrappers.
docs/whats_new.md (1)
1164-1165: Add a brief caution about disabling redaction + clarify scopeA short admonition helps prevent accidental PII/secret leakage and clarifies that “values” includes keys/tokens/origin strings.
-Cache keys are now redacted in logs and error messages by default, and a `DisableValueRedaction` boolean (default `false`) lets you opt out when you need the raw value for troubleshooting. +Cache keys are now redacted in logs and error messages by default, and a `DisableValueRedaction` boolean (default `false`) lets you opt out when you need the raw value for troubleshooting. + +:::caution +Only disable value redaction temporarily in trusted environments. It may expose secrets, tokens, storage keys, or user‑supplied data (e.g., Origin) in logs. +:::
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/middleware/limiter.md(3 hunks)docs/whats_new.md(6 hunks)middleware/limiter/config.go(2 hunks)middleware/limiter/limiter_fixed.go(1 hunks)middleware/limiter/limiter_sliding.go(1 hunks)middleware/limiter/limiter_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- middleware/limiter/limiter_test.go
- middleware/limiter/limiter_fixed.go
- middleware/limiter/config.go
- docs/middleware/limiter.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/limiter/limiter_sliding.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/whats_new.md
🧠 Learnings (1)
📚 Learning: 2024-07-01T03:44:03.672Z
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Applied to files:
docs/whats_new.md
🧬 Code graph analysis (1)
middleware/limiter/limiter_sliding.go (3)
internal/memory/memory.go (1)
Storage(13-16)internal/storage/memory/memory.go (1)
Storage(15-20)storage_interface.go (1)
Storage(10-46)
⏰ 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). (6)
- GitHub Check: Compare
- GitHub Check: Analyse
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, macos-latest)
🔇 Additional comments (6)
docs/whats_new.md (6)
1187-1188: LGTM – consistent with the new redaction behaviorWording and default align with other middleware sections.
1201-1202: LGTM – CSRF docs match the flag and defaultClear and consistent.
1203-1206: LGTM – Idempotency section addition reads wellMatches the unified redaction toggle.
1397-1398: LGTM – Limiter notes are accurateDefault and scope are clear.
2227-2228: LGTM – Migration note mirrors overviewGood placement in migration guidance.
2264-2266: LGTM – Idempotency migration noteConsistent with the rest of the doc.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable security enhancement by adding configurable value redaction to the Cache, CSRF, CORS, and Idempotency middlewares. The implementation is consistent across all affected middlewares, using a DisableValueRedaction flag to control the behavior. The changes are well-documented and include corresponding tests, which is great. I have one minor suggestion regarding log message formatting to ensure consistency across the codebase.
Summary