Skip to content

fix: redact output of credential tools #500

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

Merged
merged 4 commits into from
Jun 14, 2024

Conversation

g-linville
Copy link
Member

@g-linville g-linville commented Jun 14, 2024

for #483

This creates a little wrapper type around the monitor, to be used only for credential calls, so that we do not print the plaintext output to the console.

Update: this instead excludes the output of credential tools from the CallFinish Event entirely, so the Monitor never has a chance to see it.

@ibuildthecloud
Copy link
Contributor

ibuildthecloud commented Jun 14, 2024

There might be a different way by just looking at the tool category in the monitor when it's printing. This is effectively what the TUI does, we don't print output for tool category "context", "provider" or "credential". For the CLI logs I would still like provider and context, but it make sense maybe to not print the credential? I'm not 100% why we care though.

@ibuildthecloud
Copy link
Contributor

Oh looking at this a big more, I think we probably need to drop the output in the place where we write the event. The event should not have the clear text of a password in it. Regardless if it's printed. But I'm still lost on how the event even has the clear text... Soo many layers. What madness have we created.

@g-linville
Copy link
Member Author

Oh looking at this a big more, I think we probably need to drop the output in the place where we write the event. The event should not have the clear text of a password in it. Regardless if it's printed. But I'm still lost on how the event even has the clear text... Soo many layers. What madness have we created.

@ibuildthecloud I can figure this out. I will probably have to plumb some gross thing through all the layers to get it to work, but as you said, that's the nature of the madness.

@g-linville g-linville marked this pull request as draft June 14, 2024 14:09
Signed-off-by: Grant Linville <[email protected]>
@g-linville g-linville force-pushed the fix-cred-tool-output branch from 4ca869f to bf402f9 Compare June 14, 2024 14:55
@g-linville g-linville marked this pull request as ready for review June 14, 2024 15:09
@ibuildthecloud ibuildthecloud self-requested a review June 14, 2024 16:28
Signed-off-by: Grant Linville <[email protected]>
@g-linville g-linville merged commit f8d3b44 into gptscript-ai:main Jun 14, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants