Conversation
| * [Optional] A flag to enable or disable redaction for query parameters. | ||
| * @defaultValue true | ||
| */ | ||
| redactQueryParams?: boolean; |
There was a problem hiding this comment.
I have added an opt in/out option indiviually for username:password and query params. Previously we just had a redactUrls which would completely not redact anything in the url, if set to False
Do we need this level of flexibility?
There was a problem hiding this comment.
This seems like too many configs (with long names) which are ( probably ) going to be rarely used.
Suggestion: Introduce a new constant enum and lest change the redactUrls to use that
redactUrls?: boolean | RedactUrlOptionWith values like (names are suggestions only so feel free to change them)
- true / false (does what it does today - nothing or use defaults with merged urls)
- AllMerge (same as true)
- AllReplace
- UrlOnly
- UsernamePasswordOnly
- etc
Or use a bitwise value logic for the values
0x00-0x0f (Url)
0x10 add (to Skip Username/Passwword etc)
There was a problem hiding this comment.
Quick question, when the user choose replace/append for query params, the username and password should be redacted as per logic right?
There was a problem hiding this comment.
Pull request overview
This PR updates the URL field redaction configuration in AppInsightsCore by separating control of credential redaction vs query-parameter redaction, and expands unit test coverage for the new configuration behavior.
Changes:
- Introduces separate query-parameter redaction configuration (
redactQueryParamsboolean + append/replace query param lists). - Updates
fieldRedaction()/ query-parameter redaction selection logic to respect the new configuration shape. - Adjusts and adds unit tests covering disabled redaction, credential-only redaction, and custom query param modes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| shared/AppInsightsCore/Tests/Unit/src/ai/ApplicationInsightsCore.Tests.ts | Adds/updates FieldRedaction unit tests for new query-param config modes and flag interactions. |
| shared/AppInsightsCore/Tests/Unit/src/ai/AppInsightsCommon.tests.ts | Updates dataSanitizeUrl test config usage and adds coverage for “replace default sensitive params” behavior. |
| shared/AppInsightsCore/src/utils/EnvUtils.ts | Updates query-param redaction selection and splits credential vs query redaction behavior in fieldRedaction(). |
| shared/AppInsightsCore/src/interfaces/ai/IConfiguration.ts | Changes the public config surface for URL redaction (new boolean + new append/replace arrays). |
Comments suppressed due to low confidence (1)
shared/AppInsightsCore/Tests/Unit/src/ai/ApplicationInsightsCore.Tests.ts:2264
- Test name/assert message say this covers an "empty redactQueryParams array", but the test now uses
let config = {} as IConfiguration;(no empty array / no query-param config at all). Either pass an explicit empty list (for the new config shape) or rename the test/message so it matches what’s being exercised.
this.testCase({
name: "FieldRedaction: should handle empty redactQueryParams array",
test: () => {
let config = {} as IConfiguration;
// Should still redact default parameters
const url = "https://example.com/path?Signature=secret&custom_param=value";
const redactedLocation = fieldRedaction(url, config);
Assert.equal(redactedLocation, "https://example.com/path?Signature=REDACTED&custom_param=value",
"URL with default sensitive parameters should still be redacted with empty custom array");
You can also share your feedback on Copilot code review. Take the survey.
shared/AppInsightsCore/Tests/Unit/src/ai/ApplicationInsightsCore.Tests.ts
Show resolved
Hide resolved
|
Will document all the changes once decide all the config changes. |
71e3d85 to
b4d41fd
Compare
No description provided.