-
Notifications
You must be signed in to change notification settings - Fork 56
fix(bwrap): surface allowLocalNetwork settings the Bubblewrap backend cannot honor #750
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
+140
β10
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Your reading of the logger plumbing is correct, but the conclusion overstates what this PR changed β and the fix you propose is a different, much larger PR.
logger.log_line("WARNING: ...")is the established mechanism for preflight policy warnings in this codebase. The closest analogue is Bubblewrap's own cooperative-proxy warning atsrc/core/wxc_common/src/config_parser.rs:918, which tells the caller thatdefaultPolicy: "block"is not actually enforced for raw-socket clients β a security-relevant no-op warning with byte-for-byte identical visibility characteristics. Same forlearningMode(config_parser.rs:714), which warns that AppContainer restrictions are not enforced at all, and the non-existent-path warning atconfig_parser.rs:367.So the buffered-logger limitation you describe is real, but it is a property of the logging architecture that predates this change and applies uniformly to every preflight warning mxc emits. This PR moves
allowLocalNetworkfrom not read at all to diagnosable through the same channel every other policy warning uses. That is the whole intent, and it is strictly better than the status quo.What you are asking for β "propagate preflight warnings through an API/response channel that the CLI and SDK surfaces consume" β means adding a warnings field to
ScriptResponse(src/core/wxc_common/src/models.rs:748, which has no such field today) and threading it throughlxc-exec/wxc-exec,mxc-sdk,mxc_ffi'sMxcRunResult, the C# SDK, and the TypeScript SDK. That is a cross-cutting API change affecting every backend and all three language bindings. Doing it here would bury a scoped Bubblewrap fix inside an SDK-surface redesign, and it should be decided on its own merits with maintainer input.Not resolving this thread β leaving it open deliberately so a maintainer can weigh in on whether the warnings channel is wanted as a follow-up. Happy to file it as a separate issue if that is the preference.