-
Notifications
You must be signed in to change notification settings - Fork 42
Add a Checkbox to filter password field for output_project_options and output_user_options. #25
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
base: main
Are you sure you want to change the base?
Conversation
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'm just doing some manual sanity tests and I've left some small change suggestions, but overall looks good and it was a great idea to add this for security.
Assuming the sanity tests work well I'm also happy to get this merged and I can make the cleanup changes done in another PR. Let me know what you'd prefer. 😄
src/test/kotlin/net/portswigger/mcp/security/CredentialFilterTest.kt
Outdated
Show resolved
Hide resolved
src/test/kotlin/net/portswigger/mcp/security/CredentialFilterTest.kt
Outdated
Show resolved
Hide resolved
Just did some manual testing. Unfortunately at the moment there are a few issues with the static dataclass schema representation of the Burp project and user settings. I'm thinking that it may be easier to approach and maintain it from a dynamic JSON parsing approach so if changes are made to the Burp settings JSON schema we don't need to make updates. I'm happy to help out with implementing this if you get stuck. Unfortunately I won't be able to get to this until the end of the month though. |
I'm sorry for the delay, I didn’t catch all of them in the first pass. I’ve gone ahead and fixed the remaining issues on the test module as per your suggestions. |
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 that the static JSON serialization wasn't the best approach. I've committed a new version of the feature using a recursive approach, which should make it more maintainable in case of future changes to the JSON schemas. Feel free to take a look at the changes and let me know if any adjustments are needed.
This pull request fixes functions that leak the credentials setting of the Platform Authentication and socks proxy. The issue was reported in #22.
changes:
please review and let me know if there are any issues or improvements needed.