Skip to content
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

iOS public logging #408

Merged
merged 3 commits into from
Nov 15, 2024
Merged

Conversation

rocketraman
Copy link
Contributor

Change the implementation of native method darwin_log_with_type to use %{public}s rather than %s.

Resolves #400

Copy link
Contributor

@KevinSchildhorn KevinSchildhorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change it to add a function for public logging, or add a parameter?
We'd like the public logging to be an opt-in so that by default it wont be public.

@rocketraman
Copy link
Contributor Author

rocketraman commented Nov 14, 2024

@KevinSchildhorn I'm not sure that approach really makes sense in the context of Kermit. The reason is because when os_log is used directly, the default is for static strings to be displayed as public, and for dynamic strings to be private. In the context of Kermit, because we're delegating to os_log, everything is considered dynamic.

Users wouldn't expect the behavior of everything-private-by-default. They would expect logs to be public, just as they are on other platforms, and would elide dynamic data that should not be logged before sending it to Kermit. They would need to do this for the other platforms they are running on in any case. Therefore, I think it makes sense to use %{public}s by default, as that would offer consistency across platforms, and follow the principle of least surprise for users.

All that being said, I've done the implementation of opt-in public logging anyway. However, I can remove that second commit, or alternatively reverse the default, if you agree with my argument above.

Note also that this change will now conflict with #409. After that PR is merged, I can rebase this one.

@KevinSchildhorn
Copy link
Contributor

Thank you for adding the opt-in for public logging. We understand your argument and the expected behavior, however we don't want to change the default implementation silently at this time. There may be existing users of Kermit that have sensitive data in their logs, and we don't want to change their logs that they expect are private to suddenly be public.

@rocketraman
Copy link
Contributor Author

Thank you for adding the opt-in for public logging. We understand your argument and the expected behavior, however we don't want to change the default implementation silently at this time. There may be existing users of Kermit that have sensitive data in their logs, and we don't want to change their logs that they expect are private to suddenly be public.

Sounds good. I see you're already merged the conflicting changes into this branch. Were there any other changes needed here?

Copy link
Contributor

@KevinSchildhorn KevinSchildhorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good to go now, thank you for the PR and your understanding.

@KevinSchildhorn KevinSchildhorn merged commit 5f5fc07 into touchlab:main Nov 15, 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.

OSLogWriter logs doesn't show up in console after process is detached from XCode
2 participants