fix(cache): let a cache mask its keys in the log - #160
Conversation
github-actions
Bot
added
the
needs-cla-review
|
🔎 Maintainer heads-up: automated triage flagged this PR as potentially material, so it may need a signed CLA in addition to the DCO sign-off. Strong signals
This is advisory only — the bot does not decide. Please judge against the CLA criteria (material, product-critical, patent-sensitive, corporate contributor, broad commercial use). Note that thresholds can be gamed by splitting PRs, so use your judgement.
|
5a8db68 to
078dc8f
Compare
cosmin-staicu
changed the title
078dc8f to
cccc722
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Several Warning, Error, Debug, and Trace paths still log unmasked keys when masking is enabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds configurable cache-key masking to reduce sensitive-key exposure in logs.
Changes:
- Introduces
LogKeysand deferredLoggedKeymasking. - Applies masking across multilayer, Redis, event-publisher, and distributed-cache logging.
- Adds configuration documentation and tests.
File summaries
| File | Description |
|---|---|
tests/UiPath.Caching.Tests/Redis/RedisHashCacheTests.cs |
Tests Redis hash-cache miss logging. |
tests/UiPath.Caching.Tests/Redis/RedisCacheTests.cs |
Tests Redis cache miss logging. |
tests/UiPath.Caching.Tests/LoggedKeyTests.cs |
Tests masking and prefix detection. |
tests/UiPath.Caching.Tests/Distributed/UiPathDistributedCacheTests.cs |
Verifies adapter warning masking. |
tests/UiPath.Caching.Tests/Config/KeyLoggingTests.cs |
Adds container-level logging tests. |
src/UiPath.Caching/Redis/RedisHashCache.cs |
Masks Redis hash-cache keys. |
src/UiPath.Caching/Redis/RedisCacheOptions.cs |
Adds Redis logging options. |
src/UiPath.Caching/Redis/RedisCacheBase.cs |
Centralizes Redis-key rendering. |
src/UiPath.Caching/Redis/RedisCache.cs |
Masks Redis cache log arguments. |
src/UiPath.Caching/PublicAPI.Unshipped.txt |
Records new public properties. |
src/UiPath.Caching/MultilayerHashCache.cs |
Masks multilayer hash-cache keys. |
src/UiPath.Caching/MultilayerCacheBase.cs |
Centralizes multilayer masking. |
src/UiPath.Caching/MultilayerCache.cs |
Masks single and joined keys. |
src/UiPath.Caching/LoggedKey.cs |
Implements masked key rendering. |
src/UiPath.Caching/InMemoryRedisCacheOptions.cs |
Adds key-logging configuration. |
src/UiPath.Caching/InMemoryCacheOptions.cs |
Adds key-logging configuration. |
src/UiPath.Caching/ICacheOptions.cs |
Defines the LogKeys API. |
src/UiPath.Caching/Distributed/UiPathDistributedCache.cs |
Masks adapter-owned messages. |
src/UiPath.Caching/Config/DistributedCacheCollectionExtensions.cs |
Disables raw logging for adapter tiers. |
src/UiPath.Caching/CacheEventPublisher.cs |
Masks event-publisher keys. |
samples/UiPath.Caching.Sample/appsettings.all.json |
Demonstrates LogKeys settings. |
docs/reference/settings.md |
Documents masking behavior. |
CHANGELOG.md |
Records the security fix. |
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// Whether log lines show the cache key in full. Off, every site that would log a key keeps the prefix | ||
| /// the key strategy composed and masks the caller's key to its first three characters | ||
| /// (<c>myapp:s:cos****</c>). Turn it off where keys can be secrets — session ids, API keys — which is what | ||
| /// <c>AddDistributedCache</c> does for its own provider. | ||
| /// </summary> | ||
| public bool LogKeys { get => true; set => _ = value; } |
| if (!await StoreAsync(cacheKey, fields, ttl, token).ConfigureAwait(false)) | ||
| { | ||
| LogWriteNotApplied(key); | ||
| LogWriteNotApplied(LoggedKey.Mask(key)); |
| private protected string? RedisKeyLogPrefix { get; set; } | ||
|
|
||
| // The physical key is the Redis prefix followed by the cache key, which carries its own prefix; without the first, nothing is known to be safe. | ||
| private protected LoggedKey Logged(RedisKey key) => new(key.ToString(), RedisKeyLogPrefix is null ? null : RedisKeyLogPrefix + _cacheKeyLogPrefix, LogKeys); |
cccc722 to
4e56e5b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Several warning, error, and broadcast paths still log raw consumer keys despite LogKeys being disabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Balanced
| _keyLogPrefix = (multiLayerCacheOptions as IKeyLogPrefixSource)?.KeyLogPrefix | ||
| ?? LoggedKey.PrefixOf(multiLayerCacheOptions.CacheKeyStrategy ?? new DefaultCacheKeyStrategy()); | ||
| _eventPublisher = new CacheEventPublisher(cacheName, _topicProvider, cacheEventFactory, logger, multiLayerCacheOptions.LogKeys, _keyLogPrefix); |
|
4e56e5b to
cc32af4
Compare
ICacheOptions.MaskKeys (default false) and MaskedKeyPrefixes on the three options classes. On, a log line that names a key keeps the prefix the key strategy composed and masks the caller's key to its first three characters (myapp:s:session:cos****). Only a key starting with a listed prefix is masked, and a value that is plainly an identifier, a number or a GUID, is left alone: identifiers are not secrets. Each layer learns its own prefix by composing a probe key through its own strategy, and the Redis layer keeps that in front of the cache key it judges. Masking runs only when the line is written: the generated log methods take a LoggedKey, which renders in ToString, so a disabled level costs nothing and a raw key cannot reach them by accident. AddDistributedCache masks the key in its own two messages regardless, because IDistributedCache keys belong to the consumer and can be secrets. Closes #129 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017fwLrS3Sbcen8v6iRkUaFB Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
cc32af4 to
a368758
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Raw-key failure logs and sharded Redis key strategies can still expose sensitive cache keys.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 3
- Review effort level: Balanced
| var composed = (string?)strategy.GetRedisKey(new CacheKey(Probe, CacheKeyCasing.Sensitive)); | ||
| return composed is not null && composed.Length > Probe.Length && composed.EndsWith(Probe, StringComparison.OrdinalIgnoreCase) | ||
| ? composed[..^Probe.Length] | ||
| : null; |
| _keyMasking = KeyMasking.For(multiLayerCacheOptions); | ||
| _eventPublisher = new CacheEventPublisher(cacheName, _topicProvider, cacheEventFactory, logger, _keyMasking); |
| /// and shows only its first three characters (<c>myapp:s:session:cos****</c>). <c>AddDistributedCache</c> | ||
| /// turns it on for its own provider, with its own prefix listed. | ||
| /// </summary> | ||
| public bool MaskKeys { get => false; set => _ = value; } |
|
Closing in favour of a redesign rather than iterating this one further. Review here converged on a structural problem: the masking policy lives on per-tier options, so every component that logs a key needs those options threaded to it. The memory setter, the rehydration coordinator and the broadcast change token all log keys today and none of them can see the switch, which is why keys still reach logs with masking on. Patching each one spreads the same plumbing further. The replacement is a policy seam. An IKeyMaskingPolicy answers one question, whether a key is secret, and is resolved from the container, so any component can consume it without carrying options. Rendering stays in the library and splices the masked value into whatever the key strategy composed, so it works for a prefix, a suffix or a hash tag rather than assuming the library's part comes first. Masking off is a null implementation, matching every other seam here. The distributed adapter keeps its guarantee through a per-tier policy rather than by forcing a flag on its private tiers. Also carried over: the caller's key and the composed key are both passed to the renderer, which removes the probe that discovers the composed prefix today; identifier-shaped keys stay readable; and masking is deferred to log formatting so a disabled level costs nothing. Branch fix/log-keys-redaction stays as the record of this attempt. New work starts from main. Closes #129 is carried to the replacement. |
Closes #129.
Problem
Cache keys are written to logs, several of them at
Warning. For the application's own caches that is usually harmless. For theIDistributedCacheadapter it is not: those keys are the consumer's and can be secrets, such as the ASP.NET Core session id. A Redis blip raised the inner-cache warning paths and wrote session identifiers to the log (CWE-532).Change
ICacheOptions.LogKeys, defaulttrue, onInMemoryCacheOptions,InMemoryRedisCacheOptionsandRedisCacheOptions. The interface member has a default implementation, so hand-writtenICacheOptionsimplementers keep compiling.****:d:ses****at the cache layer,myapp:dh:d:ses****at the Redis layer. Each layer learns its prefix by composing a probe key through its own strategy; a custom strategy whose output does not end with the key yields no prefix, and the whole value is masked, since nothing in it is known to be safe. The Redis layer adds the cache-key prefix on top of its own. The adapter hands its ownd:prefix to the tier copies it configures, because it neutralizes their key strategy.LoggedKey, an internal struct whoseToStringdoes the masking. TheLoggerMessagegenerator only callsToStringwhen the level is enabled, so a masked Trace site on a hot path costs nothing when Trace is off.CacheKeyorRedisKeynow takes aLoggedKeyand is namedLog…Core; a same-namedLog…(CacheKey)wrapper beside it applies the cache's switch. The 100-odd call sites are untouched, and a new call site cannot bypass the switch: a raw key does not convert toLoggedKey. Applied mechanically toMultilayerCache(22 methods),MultilayerHashCache(16),RedisCache(5),RedisHashCache(3) andCacheEventPublisher(1); the four joined-keys sites go through the same helper. Not done viaCacheKey.ToString(), which the issue rules out because the implicit string conversion is used to compose physical keys.LogKeys = falseon the private copies of its tier's options, next to the existingCacheKeyStrategyneutralization, and masks the key in its own two messages.Tests
LoggedKeyTests: verbatim, prefix kept and key masked, case-insensitive prefix match, whole-value masking without a matching prefix, short keys, prefix learning for the Redis and cache-key strategies including one that does not keep the key as a tail, join.KeyLoggingTests: real caches through the container with a Trace-level capturing logger. With the switch off, no line contains the key and lines showsec****; with it on, the reverse. Same for the hash cache. The distributed adapter on theInMemorytier logsd:Sec****even with the tier's own switch left at its default.RedisCacheTestsandRedisHashCacheTests: the miss line follows the switch, using the mocked logger.UiPathDistributedCacheTests: the failed-write warning asserts the masked key and the absence of the raw one.Docs:
LogKeysrows in the three provider tables, the distributed-cache note rewritten from "keys appear in logs" to what now happens,appsettings.all.jsongains the setting, changelog under Fixed.🤖 Generated with Claude Code
https://claude.ai/code/session_01V9jRarnMLeZRZ5rYySRhkh