-
Notifications
You must be signed in to change notification settings - Fork 820
feat: improve extension installer + updater #278
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
Conversation
shadowfax92
commented
Jan 1, 2026
- feat: new extension installer + bundle support
- feat: support bundle extension download in cli
- chore: update release yaml to include new bundle_extensions module
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.
15 files reviewed, 3 comments
| + ManifestLocation::kExternalPref, // CRX location (bundled) | ||
| + ManifestLocation::kExternalPrefDownload, // Download location (remote) |
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.
logic: Using both kExternalPref and kExternalPrefDownload as constructor arguments appears incorrect for ExternalProviderImpl. The constructor typically takes creation_flags and install_flags, not two ManifestLocation values. Are you sure the ExternalProviderImpl constructor accepts two ManifestLocation parameters in this order?
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/chromium_patches/chrome/browser/extensions/external_provider_impl.cc
Line: 44:45
Comment:
**logic:** Using both kExternalPref and kExternalPrefDownload as constructor arguments appears incorrect for ExternalProviderImpl. The constructor typically takes creation_flags and install_flags, not two ManifestLocation values. Are you sure the ExternalProviderImpl constructor accepts two ManifestLocation parameters in this order?
How can I resolve this? If you propose a fix, please make it concise.| + for (const auto [id, _] : last_config_) { | ||
| + extension_ids_.insert(id); | ||
| + } |
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.
logic: Extension IDs from remote config are added without validation - potential security risk
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/chromium_patches/chrome/browser/browseros/extensions/browseros_extension_maintainer.cc
Line: 133:135
Comment:
**logic:** Extension IDs from remote config are added without validation - potential security risk
How can I resolve this? If you propose a fix, please make it concise.| + base::FilePath crx_path = | ||
| + bundled_path.Append(base::FilePath::FromUTF8Unsafe(*crx_file)); |
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.
logic: Using FromUTF8Unsafe with external input could allow directory traversal attacks. Consider validating that crx_file contains only a filename without path separators.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/chromium_patches/chrome/browser/browseros/extensions/browseros_extension_installer.cc
Line: 144:145
Comment:
**logic:** Using `FromUTF8Unsafe` with external input could allow directory traversal attacks. Consider validating that `crx_file` contains only a filename without path separators.
How can I resolve this? If you propose a fix, please make it concise.