-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hide input value from kbn-config-schema error messages #58843
Hide input value from kbn-config-schema error messages #58843
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alerting changes LGTM
…nfig-schema-value-in-error
@@ -40,7 +40,7 @@ test('properly parse the value if input is a string', () => { | |||
test('fails if string input cannot be parsed', () => { | |||
const type = schema.mapOf(schema.string(), schema.string()); | |||
expect(() => type.validate(`invalidjson`)).toThrowErrorMatchingInlineSnapshot( | |||
`"could not parse map value from [invalidjson]"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example and schema.literal, hostname/uri bother me the most. It's would be painful to debug such a message without a real output and could make life harder for our support team. There are just a few fields in the config that could be considered as sensitive, and most of them are defined within the platform. OTOH it's a good practice to have the most strict rules and loosen them by request, so OK for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and I expressed the same concerns in #58652 (comment). But I guess being consistent in this rule makes the more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this issue to today's weekly discussions just to be sure we all are on the same page here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on @pgayvallet!
It looks like ByteSizeValue and Duration are throwing errors with user input, and this can end up within the logs. The likelihood of us having sensitive/secret durations and byte sizes is pretty low, but in an effort to keep all of this consistent, something we should probably address. Were these intentionally excluded?
I think there's one more within
|
@kobelb hope that was the last one 😄 . PTAL |
…nfig-schema-value-in-error
…nfig-schema-value-in-error
…nfig-schema-value-in-error
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* use inline snapshots instead of snapshots * hide input value from error messages * update core snapshots * update xpack snapshots * fix ftr assertions * fix new snapshots * hide values for byte_size and duration * update new snapshots * remove another byte_size value reference * fix yet another value references in error messages * update xpack snapshots * update xpack ftr assertions
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
…#59565) * Hide input value from kbn-config-schema error messages (#58843) * use inline snapshots instead of snapshots * hide input value from error messages * update core snapshots * update xpack snapshots * fix ftr assertions * fix new snapshots * hide values for byte_size and duration * update new snapshots * remove another byte_size value reference * fix yet another value references in error messages * update xpack snapshots * update xpack ftr assertions * update snapshots
* master: (154 commits) Add an optional authentication mode for HTTP resources (elastic#58589) Implement embeddable drilldown menu options (elastic#59232) [Alerting] "Create alert" graph visualization design improvements (elastic#59399) Alerting update route throttle property is missing (elastic#59580) [SIEM] Adds 'Load prebuilt rules' Cypress test (elastic#59529) Show error if field is not found during filter rendering (elastic#59298) Navigate back to discover app during test, because the saved search from the preceding test has major performance problems when used with this test (elastic#59571) Check for alert dialog when doing a force logout (elastic#59329) ensure fs deletes are not cwd dependent (elastic#59570) Empty message for APM service map (elastic#59518) [Drilldowns] <ActionWizard/> Component (elastic#59032) [Reporting] Improve the page exit error messages (elastic#59351) Ensure logged out starting state for tests that need it (elastic#59322) Hide input value from kbn-config-schema error messages (elastic#58843) [ML] Transforms: Migrate client plugin to NP. (elastic#59443) [ML] Disable failing functional tests [SIEM] Update Timeline to use the latest euiFlyoutBody style (elastic#59524) Temporarily remove the project mappings for PR labels (elastic#59493) [Alerting] replace index threshold graph usage of watcher APIs with new API (elastic#59385) [ML] Show view series link in anomalies table for machine_learning_user role (elastic#59549) ...
Summary
Fix #58652
Note: first commit converts every config-schema's snapshots to inline snapshots (that needed to be done and greatly improve next commit's readability). Would be easier to review by ignoring it.
Checklist
For maintainers