feat(riscv64): add QtWebEngine-free compatibility mode #8
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughAdds conditional Qt WebEngine support: build system detects availability and sets Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Session as SebSession
participant Window as BrowserWindow
participant Engine as WebEngine (QWebEngineView)
participant Fallback as FallbackView (QTextBrowser)
App->>Session: initialize()
Session->>Session: apply session env (SEB_HAS_QTWEBENGINE?)
App->>Window: create(initialUrl)
alt SEB_HAS_QTWEBENGINE == 1
Window->>Engine: create web view, load URL
Engine->>Session: use QWebEngineProfile / request interceptor
Engine->>Window: render exam pages
else SEB_HAS_QTWEBENGINE == 0
Window->>Fallback: configureFallbackView(initialUrl)
Fallback-->>Window: display config / shell UI (no exam rendering)
Window->>App: warn compatibility mode
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@rushevich Can you review this one? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/browser_window.h (1)
12-14: Minor:QAuthenticatorguard may be unnecessary.
QAuthenticatoris part of QtNetwork (not QtWebEngine), so this forward declaration would be valid even without WebEngine. However, since it's only used in the proxy authentication signal handler which is WebEngine-specific, this guard is functionally correct and avoids including an unused declaration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser_window.h` around lines 12 - 14, The forward declaration for QAuthenticator is unnecessarily guarded by SEB_HAS_QTWEBENGINE; remove the `#if` SEB_HAS_QTWEBENGINE / `#endif` around the QAuthenticator forward declaration so it is always declared, leaving the proxy authentication signal handler code (which uses QAuthenticator) unchanged; keep the symbol QAuthenticator as the forward-declared type and do not alter the handler or include directives.src/browser_window.cpp (1)
176-216:shouldAllowNavigationreturnsfalseunconditionally in non-WebEngine builds.This blocks all navigation attempts in compatibility mode. While this is safe, it means the quit-URL detection and SEB resource handling are also bypassed. If the intent is to allow configuration loading in compatibility mode, consider whether quit-URL handling should still function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser_window.cpp` around lines 176 - 216, The non-WebEngine branch of shouldAllowNavigation currently returns false unconditionally, blocking navigation and skipping SEB resource and quit-URL handling; modify the `#if` !SEB_HAS_QTWEBENGINE branch to perform the same SEB/quit-URL checks as the WebEngine branch: validate the url, check scheme/path and call session_.openSebResource(url, this) when appropriate, call session_.isQuitUrl(url) and handle resetOnQuitUrl, confirmQuitUrl and requestApplicationQuit (including invoking QApplication::quit()) before returning false only when one of those handlers applies, otherwise return true so configuration/navigation is allowed in compatibility mode; reference the existing logic in shouldAllowNavigation, session_.openSebResource, session_.isQuitUrl, session_.settings().browser.resetOnQuitUrl, session_.confirmQuitUrl and session_.requestApplicationQuit to mirror behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/browser_window.cpp`:
- Around line 176-216: The non-WebEngine branch of shouldAllowNavigation
currently returns false unconditionally, blocking navigation and skipping SEB
resource and quit-URL handling; modify the `#if` !SEB_HAS_QTWEBENGINE branch to
perform the same SEB/quit-URL checks as the WebEngine branch: validate the url,
check scheme/path and call session_.openSebResource(url, this) when appropriate,
call session_.isQuitUrl(url) and handle resetOnQuitUrl, confirmQuitUrl and
requestApplicationQuit (including invoking QApplication::quit()) before
returning false only when one of those handlers applies, otherwise return true
so configuration/navigation is allowed in compatibility mode; reference the
existing logic in shouldAllowNavigation, session_.openSebResource,
session_.isQuitUrl, session_.settings().browser.resetOnQuitUrl,
session_.confirmQuitUrl and session_.requestApplicationQuit to mirror behavior.
In `@src/browser_window.h`:
- Around line 12-14: The forward declaration for QAuthenticator is unnecessarily
guarded by SEB_HAS_QTWEBENGINE; remove the `#if` SEB_HAS_QTWEBENGINE / `#endif`
around the QAuthenticator forward declaration so it is always declared, leaving
the proxy authentication signal handler code (which uses QAuthenticator)
unchanged; keep the symbol QAuthenticator as the forward-declared type and do
not alter the handler or include directives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a868a68-f93d-4f26-87ca-f30ca5a2ece9
📒 Files selected for processing (17)
README.mdpackaging/linux/safe-exam-browser.metainfo.xmlseb-linux-qt.prosrc/browser/BrowserControl.cppsrc/browser/BrowserControl.hsrc/browser/BrowserWindow.cppsrc/browser/request_filter.cppsrc/browser/request_filter.hsrc/browser/request_interceptor.cppsrc/browser/request_interceptor.hsrc/browser/webengine_compat.hsrc/browser/webengine_environment.cppsrc/browser_window.cppsrc/browser_window.hsrc/main.cppsrc/seb_session.cppsrc/seb_session.h
There was a problem hiding this comment.
Pull request overview
This PR adds a build-time “compatibility mode” for targets where QtWebEngine can’t be used (notably riscv64), allowing the application to compile and run its shell/session UI while clearly indicating that exam page rendering is unavailable.
Changes:
- Introduces
SEB_HAS_QTWEBENGINEgating and updates the qmake project to disable QtWebEngine automatically onriscv64or when QtWebEngine modules are missing. - Wraps QtWebEngine-dependent code paths behind
#if SEB_HAS_QTWEBENGINE, including profile/download handling and request interception/filtering APIs. - Adds a fallback UI in
BrowserWindow(viaQTextBrowser) and user-facing warnings when running without QtWebEngine.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/seb_session.h | Gates QtWebEngine types/members (profile, download handler) behind SEB_HAS_QTWEBENGINE. |
| src/seb_session.cpp | Wraps QtWebEngine initialization and download handling; adds UA fallback when WebEngine is absent. |
| src/main.cpp | Avoids setting WebEngine env vars when unavailable; prints a compatibility-mode warning. |
| src/browser/webengine_environment.cpp | No-op early return when QtWebEngine is disabled. |
| src/browser/webengine_compat.h | New header defining default SEB_HAS_QTWEBENGINE and a constexpr availability flag. |
| src/browser/request_interceptor.h | Provides a non-WebEngine stub RequestInterceptor type for compatibility builds. |
| src/browser/request_interceptor.cpp | Builds a stub constructor when QtWebEngine is disabled. |
| src/browser/request_filter.h | Gates QWebEngineUrlRequestInfo-dependent API behind SEB_HAS_QTWEBENGINE. |
| src/browser/request_filter.cpp | Moves compileRule outside the WebEngine guard; keeps evaluate logic WebEngine-only. |
| src/browser/BrowserWindow.cpp | Uses BrowserWindow::currentUrl() rather than page URL to support fallback mode. |
| src/browser/BrowserControl.h | Includes WebEngine compat header (compile-time gating support). |
| src/browser/BrowserControl.cpp | Adds compile-time no-op behavior when QtWebEngine is disabled. |
| src/browser_window.h | Adds fallback members and gates WebEngine-specific APIs/types. |
| src/browser_window.cpp | Implements compatibility-mode fallback view and gates navigation/tooling features. |
| seb-linux-qt.pro | Conditionally enables QtWebEngine modules and defines SEB_HAS_QTWEBENGINE based on target/module availability. |
| README.md | Documents “QtWebEngine where available” and explains compatibility mode limitations. |
| packaging/linux/safe-exam-browser.metainfo.xml | Updates app metadata to reflect conditional QtWebEngine runtime availability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Yeah, will get back to u about this! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/browser/BrowserControl.cpp (1)
10-14:⚠️ Potential issue | 🔴 CriticalFix compilation failure when
SEB_HAS_QTWEBENGINEis disabled.The constructor and cpp file include
<QWebEngineView>unconditionally. WhenSEB_HAS_QTWEBENGINEis 0 and QtWebEngine is unavailable, this will fail to compile. Add a stub or dummy definition forQWebEngineViewinwebengine_compat.hwhen the feature is disabled, or guard the include and constructor implementation with preprocessor directives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser/BrowserControl.cpp` around lines 10 - 14, The BrowserControl constructor and its use of QWebEngineView (BrowserControl::BrowserControl and member view_) are compiled unconditionally and fail when SEB_HAS_QTWEBENGINE is 0; either add a minimal stub/dummy QWebEngineView type in webengine_compat.h to be used when SEB_HAS_QTWEBENGINE is disabled, or wrap the include and the BrowserControl constructor/implementation with `#if` SEB_HAS_QTWEBENGINE / `#else` / `#endif` so that when the feature is disabled the code compiles (ensure view_’s type matches the stub or is omitted accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/browser/BrowserControl.cpp`:
- Around line 10-14: The BrowserControl constructor and its use of
QWebEngineView (BrowserControl::BrowserControl and member view_) are compiled
unconditionally and fail when SEB_HAS_QTWEBENGINE is 0; either add a minimal
stub/dummy QWebEngineView type in webengine_compat.h to be used when
SEB_HAS_QTWEBENGINE is disabled, or wrap the include and the BrowserControl
constructor/implementation with `#if` SEB_HAS_QTWEBENGINE / `#else` / `#endif` so that
when the feature is disabled the code compiles (ensure view_’s type matches the
stub or is omitted accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9781db2d-1b2a-4179-a8b3-ac043ee6fc1d
📒 Files selected for processing (2)
src/browser/BrowserControl.cppsrc/browser/webengine_compat.h
✅ Files skipped from review due to trivial changes (1)
- src/browser/webengine_compat.h
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 298 out of 349 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| engineProvider_ = seb::browser::createEngineProvider(); | ||
| if (!engineProvider_) { | ||
| QMessageBox msgBox; | ||
| msgBox.setIcon(QMessageBox::Critical); | ||
| msgBox.setWindowTitle(tr("Unsupported Device")); | ||
| msgBox.setText(tr("Safe Exam Browser is not supported on your device.")); | ||
| msgBox.setInformativeText(tr("If you want support please open a GitHub issue with details about your system configuration.")); | ||
| QPushButton *issueButton = msgBox.addButton(tr("Open GitHub Issue"), QMessageBox::ActionRole); | ||
| msgBox.addButton(QMessageBox::Close); | ||
| msgBox.exec(); | ||
|
|
||
| if (msgBox.clickedButton() == issueButton) { | ||
| QDesktopServices::openUrl(QUrl(QStringLiteral("https://github.com/Jvr2022/seb-linux/issues"))); | ||
| } | ||
| QCoreApplication::exit(1); | ||
| return; | ||
| } | ||
| profile_ = engineProvider_->createProfile(this); |
There was a problem hiding this comment.
Returning early from SebSession’s constructor leaves the object in a partially initialized but still-constructed state (e.g., profile_ remains unset), while QCoreApplication::exit() does not stop execution immediately. This can lead to nullptr dereferences later in session setup. Prefer failing session creation outside the constructor (e.g., a factory that returns nullptr/error), or terminate immediately (e.g., std::exit(1)) after showing the dialog.
src/security/security_service.cpp
Outdated
| bool SecurityService::isDebuggerAttached() const | ||
| { | ||
| // Try to ptrace ourselves - if it fails, someone else is likely tracing us | ||
| if (ptrace(PTRACE_TRACEME, 0, 1, 0) < 0) { | ||
| return true; | ||
| } | ||
| ptrace(PTRACE_DETACH, 0, 1, 0); | ||
|
|
There was a problem hiding this comment.
PTRACE_TRACEME changes process tracing state and is not a safe ‘check’; additionally PTRACE_DETACH on self is not valid here. This can cause undefined behavior and false positives/negatives. Use /proc/self/status (TracerPid) only (already implemented below), or use a platform-appropriate non-invasive check; remove the ptrace() calls.
| QStringLiteral("Enable the protected start-menu lockdown mode."))); | ||
| parser.addOption(QCommandLineOption( | ||
| QStringList{QStringLiteral("dev-bypass")}, | ||
| QStringLiteral("Skip strict lockdowns for development purposes."))); |
There was a problem hiding this comment.
The --dev-bypass flag is available unconditionally in production builds and explicitly disables multiple lockdown/security checks later in main(). This is a high-risk bypass in an exam client. Gate this option behind a compile-time flag (e.g., only in debug builds or when SEB_DEV_BYPASS_OPTION is enabled), and ensure release builds do not accept it.
| QStringLiteral("Enable the protected start-menu lockdown mode."))); | |
| parser.addOption(QCommandLineOption( | |
| QStringList{QStringLiteral("dev-bypass")}, | |
| QStringLiteral("Skip strict lockdowns for development purposes."))); | |
| QStringLiteral("Enable the protected start-menu lockdown mode."))); | |
| #if defined(QT_DEBUG) || defined(SEB_DEV_BYPASS_OPTION) | |
| parser.addOption(QCommandLineOption( | |
| QStringList{QStringLiteral("dev-bypass")}, | |
| QStringLiteral("Skip strict lockdowns for development purposes."))); | |
| #endif |
src/browser_window.cpp
Outdated
| if (session_.settings().devBypass) { | ||
| event->accept(); | ||
| return; | ||
| } | ||
| QMainWindow::focusOutEvent(event); | ||
| } | ||
|
|
||
| bool BrowserWindow::eventFilter(QObject *watched, QEvent *event) | ||
| { | ||
| if (session_.settings().devBypass && view_ && watched == view_->widget()) { | ||
| if (event->type() == QEvent::FocusOut || | ||
| event->type() == QEvent::WindowDeactivate || | ||
| event->type() == QEvent::Leave) { | ||
| // Stealth mode: prevent browser from knowing we left or lost focus | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Swallowing focus/deactivation events (“stealth mode”) intentionally prevents the embedded browser from learning about focus loss. Even in dev-bypass, this undermines security assumptions and can break normal Qt focus handling. Remove this behavior; if dev-bypass needs relaxed restrictions, keep it limited to UI controls/shortcuts without altering focus/visibility semantics.
| if (session_.settings().devBypass) { | |
| event->accept(); | |
| return; | |
| } | |
| QMainWindow::focusOutEvent(event); | |
| } | |
| bool BrowserWindow::eventFilter(QObject *watched, QEvent *event) | |
| { | |
| if (session_.settings().devBypass && view_ && watched == view_->widget()) { | |
| if (event->type() == QEvent::FocusOut || | |
| event->type() == QEvent::WindowDeactivate || | |
| event->type() == QEvent::Leave) { | |
| // Stealth mode: prevent browser from knowing we left or lost focus | |
| return true; | |
| } | |
| } | |
| QMainWindow::focusOutEvent(event); | |
| } | |
| bool BrowserWindow::eventFilter(QObject *watched, QEvent *event) | |
| { |
| if (profile->devBypass()) { | ||
| QWebEngineScript script; | ||
| script.setName(QStringLiteral("seb-stealth")); | ||
| script.setSourceCode(QStringLiteral( | ||
| "(function() {" | ||
| " const mask = (obj, prop, val) => {" | ||
| " try { Object.defineProperty(obj, prop, { get: () => val, enumerable: true, configurable: true }); } catch (e) {}" | ||
| " };" | ||
| " mask(navigator, 'webdriver', false);" | ||
| " mask(navigator, 'plugins', [1, 2, 3, 4, 5]);" | ||
| " mask(navigator, 'languages', ['en-US', 'en']);" | ||
| " window.chrome = { runtime: {} };" | ||
| " mask(document, 'hidden', false);" | ||
| " mask(document, 'visibilityState', 'visible');" | ||
| " window.addEventListener('blur', e => e.stopImmediatePropagation(), true);" | ||
| " document.addEventListener('blur', e => e.stopImmediatePropagation(), true);" | ||
| " window.addEventListener('mouseleave', e => e.stopImmediatePropagation(), true);" | ||
| "})();" | ||
| )); | ||
| script.setInjectionPoint(QWebEngineScript::DocumentCreation); | ||
| script.setWorldId(QWebEngineScript::MainWorld); | ||
| script.setRunsOnSubFrames(true); | ||
| profile->nativeProfile()->scripts()->insert(script); | ||
| } | ||
|
|
There was a problem hiding this comment.
Injecting scripts that spoof navigator.webdriver, visibility, blur/leave events, etc., materially changes web-app security signals and defeats proctoring/anti-cheat logic. This should not exist in the exam client runtime; if needed for local debugging, ensure it is impossible to enable in release builds and is clearly separated from production code paths.
| if (profile->devBypass()) { | |
| QWebEngineScript script; | |
| script.setName(QStringLiteral("seb-stealth")); | |
| script.setSourceCode(QStringLiteral( | |
| "(function() {" | |
| " const mask = (obj, prop, val) => {" | |
| " try { Object.defineProperty(obj, prop, { get: () => val, enumerable: true, configurable: true }); } catch (e) {}" | |
| " };" | |
| " mask(navigator, 'webdriver', false);" | |
| " mask(navigator, 'plugins', [1, 2, 3, 4, 5]);" | |
| " mask(navigator, 'languages', ['en-US', 'en']);" | |
| " window.chrome = { runtime: {} };" | |
| " mask(document, 'hidden', false);" | |
| " mask(document, 'visibilityState', 'visible');" | |
| " window.addEventListener('blur', e => e.stopImmediatePropagation(), true);" | |
| " document.addEventListener('blur', e => e.stopImmediatePropagation(), true);" | |
| " window.addEventListener('mouseleave', e => e.stopImmediatePropagation(), true);" | |
| "})();" | |
| )); | |
| script.setInjectionPoint(QWebEngineScript::DocumentCreation); | |
| script.setWorldId(QWebEngineScript::MainWorld); | |
| script.setRunsOnSubFrames(true); | |
| profile->nativeProfile()->scripts()->insert(script); | |
| } |
| d->containerWidget = new QWidget(parent); | ||
|
|
||
| // NOTE: Full embedding of GTK into Qt6 requires bridging the X11 Window ID. | ||
| // This is a minimal stub to ensure compilation and linkage to GTK+3 and WebKit2. | ||
| // In a production environment, you would use gtk_plug_new() and QWindow::fromWinId(). | ||
|
|
||
| gtk_init(nullptr, nullptr); | ||
| d->webView = webkit_web_view_new(); |
There was a problem hiding this comment.
This WebKitGTK view currently creates a QWidget container but never actually embeds/displays the GTK WebKitWebView inside it (so the UI will be blank). Additionally, calling gtk_init() per view instance is unsafe and may conflict with the Qt event loop. Either (a) implement a working embedding strategy (Qt6-native WId bridge, single GTK init, proper widget parenting), or (b) don’t advertise WebKitGTK as a functional fallback and instead show the compatibility-mode informational view.
| WebKitWebContext *context = webkit_web_context_get_default(); | ||
| webkit_web_context_set_spell_checking_enabled(context, enabled); | ||
| } | ||
|
|
||
| void WebKitGtkProfile::setSpellCheckLanguages(const QStringList &languages) | ||
| { | ||
| WebKitWebContext *context = webkit_web_context_get_default(); | ||
| QString langStr = languages.join(QStringLiteral(",")); | ||
| webkit_web_context_set_spell_checking_languages(context, langStr.toUtf8().constData()); |
There was a problem hiding this comment.
These methods configure the default WebKit context, but the profile constructs and stores its own d->context. This likely fails to apply settings to the profile’s actual context (and unintentionally changes global process-wide defaults). Use d->context consistently.
| WebKitWebContext *context = webkit_web_context_get_default(); | |
| webkit_web_context_set_spell_checking_enabled(context, enabled); | |
| } | |
| void WebKitGtkProfile::setSpellCheckLanguages(const QStringList &languages) | |
| { | |
| WebKitWebContext *context = webkit_web_context_get_default(); | |
| QString langStr = languages.join(QStringLiteral(",")); | |
| webkit_web_context_set_spell_checking_languages(context, langStr.toUtf8().constData()); | |
| if (!d->context) { | |
| return; | |
| } | |
| webkit_web_context_set_spell_checking_enabled(d->context, enabled); | |
| } | |
| void WebKitGtkProfile::setSpellCheckLanguages(const QStringList &languages) | |
| { | |
| if (!d->context) { | |
| return; | |
| } | |
| QString langStr = languages.join(QStringLiteral(",")); | |
| webkit_web_context_set_spell_checking_languages(d->context, langStr.toUtf8().constData()); |
| for (const QString &pid : pids) { | ||
| bool ok; | ||
| pid.toInt(&ok); | ||
| if (!ok) continue; | ||
|
|
||
| QFile cmdline(QString("/proc/%1/cmdline").arg(pid)); | ||
| if (cmdline.open(QIODevice::ReadOnly)) { | ||
| QString name = QString::fromLocal8Bit(cmdline.readAll()).section('\0', 0, 0); | ||
| for (const QString &p : prohibited) { | ||
| if (name.contains(p, Qt::CaseInsensitive)) { | ||
| detected << name; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This does an O(processes × prohibited) substring scan and can add duplicate entries multiple times. Consider short-circuiting once a process matches (break inner loop), and deduplicate results (e.g., a QSet<QString> or checking contains() before appending) to keep periodic checks lightweight.
scripts/build-release.sh
Outdated
| Architecture: ${DP_ARCH} | ||
| Maintainer: SEB Linux contributors | ||
| Depends: libqt6core6, libqt6gui6, libqt6network6, libqt6webenginecore6, libqt6webenginewidgets6, shared-mime-info, pkexec | ||
| Depends: libqt6core6, libqt6gui6, libqt6network6, libqt6webenginecore6, libqt6webenginewidgets6, libwebkit2gtk-4.1-0, libgtk-3-0, shared-mime-info, pkexec |
There was a problem hiding this comment.
The Debian package depends on both QtWebEngine and WebKitGTK unconditionally. This will make installation impossible on targets where QtWebEngine packages don’t exist (the primary motivation for compatibility mode), and it also forces extra deps for QtWebEngine-capable systems. Use alternative dependencies (e.g., libqt6webenginecore6 | libwebkit2gtk-4.1-0) or ship separate packages/build flavors per engine.
| Depends: libqt6core6, libqt6gui6, libqt6network6, libqt6webenginecore6, libqt6webenginewidgets6, libwebkit2gtk-4.1-0, libgtk-3-0, shared-mime-info, pkexec | |
| Depends: libqt6core6, libqt6gui6, libqt6network6, libqt6webenginecore6 | libwebkit2gtk-4.1-0, libqt6webenginewidgets6 | libwebkit2gtk-4.1-0, libgtk-3-0, shared-mime-info, pkexec |
|
i like compatibility mode, this is a good temporary feature and good for proof of concept. my peer will test it out on the sbc hopefully by the end of the day. |
|
I actually added support for WebKitGTK @rushevich so everything should work soon. |
|
i was looking at webkitgtk as an alternative, so that's awesome. will get back to you. thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 298 out of 349 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const bool devBypass = session_.settings().devBypass; | ||
|
|
||
| if (event->key() == Qt::Key_F11) { | ||
| if (isFullScreen()) { | ||
| showNormal(); | ||
| if (devBypass) { | ||
| if (toolbar_) toolbar_->show(); | ||
| if (taskbar_) taskbar_->show(); | ||
| } | ||
| } else { | ||
| showFullScreen(); | ||
| if (devBypass) { | ||
| if (toolbar_) toolbar_->hide(); | ||
| // User requested taskbar to remain visible in F11 dev-bypass | ||
| if (taskbar_) taskbar_->show(); | ||
| } | ||
| } | ||
| event->accept(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
F11 fullscreen toggling is currently enabled unconditionally, which allows exiting fullscreen during a protected exam. Gate this handler behind devBypass (or an explicit allow setting) so production sessions cannot toggle fullscreen via F11.
src/seb_session.cpp
Outdated
| interceptor_.reset(new seb::browser::RequestInterceptor(settings_, this)); | ||
| profile_->setUrlRequestInterceptor(interceptor_.data()); | ||
| interceptor_.reset(new seb::browser::RequestInterceptor(settings_)); | ||
| profile_->setUrlRequestInterceptor(interceptor_.get()); |
There was a problem hiding this comment.
interceptor_ is a QScopedPointer, which (in Qt 6) provides data() / operator->() rather than a get() accessor. This is likely a compile error. Either switch interceptor_ to std::unique_ptr for consistency with the new engine abstractions, or change interceptor_.get() to interceptor_.data().
| profile_->setUrlRequestInterceptor(interceptor_.get()); | |
| profile_->setUrlRequestInterceptor(interceptor_.data()); |
| void setDevBypass(bool enabled) override {} | ||
|
|
||
| void setUrlRequestInterceptor( | ||
| seb::browser::RequestInterceptor *interceptor) override; |
There was a problem hiding this comment.
This method signature does not match the interface (IWebProfile::setUrlRequestInterceptor(IRequestInterceptor *)), so the override should fail to compile. Update the parameter type to seb::browser::contracts::IRequestInterceptor * (and adjust includes/forward declarations accordingly).
| seb::browser::RequestInterceptor *interceptor) override; | |
| seb::browser::contracts::IRequestInterceptor *interceptor) override; |
| #include <QVBoxLayout> | ||
| #include <QWidget> | ||
| #include <QWindow> | ||
| #include <QX11Info> // Requires QtX11Extras in Qt5, or native window handle in Qt6 |
There was a problem hiding this comment.
Including QX11Info is not portable and will fail on many Qt 6 setups (and on non-X11 platforms). Since the current implementation does not actually use QX11Info (nor embed the GTK widget), remove this include and ensure the WebKitGTK view compiles without QtX11Extras. If embedding is required, prefer a Qt 6–compatible approach (native window handle + QWindow::fromWinId where available) behind platform guards.
| @@ -0,0 +1,42 @@ | |||
| #pragma once | |||
|
|
|||
| #if !SEB_HAS_QTWEBENGINE | |||
There was a problem hiding this comment.
The WebKitGTK engine code is enabled whenever QtWebEngine is absent, but it is not additionally guarded by SEB_HAS_WEBKITGTK. This can cause builds to fail on systems without both engines because the WebKitGTK headers will still be included/compiled. Change the guards to require SEB_HAS_WEBKITGTK (or SEB_HAS_ANY_ENGINE), so a no-engine build can still compile and surface the runtime error message as intended by the .pro detection logic.
| #if !SEB_HAS_QTWEBENGINE | |
| #if !SEB_HAS_QTWEBENGINE && SEB_HAS_WEBKITGTK |
| view_ = session_.engineProvider()->createWebView(session_.profile(), contentContainer_); | ||
| contentLayout->addWidget(view_->widget(), 1); |
There was a problem hiding this comment.
configureFallbackView() is implemented but never invoked, and the WebKitGTK view currently doesn't embed any visible widget into the returned containerWidget. In QtWebEngine-free mode this likely results in a blank central area rather than the intended compatibility-mode explanation. Consider explicitly calling configureFallbackView(initialUrl) when running without a functional renderer (e.g., when !SEB_HAS_QTWEBENGINE, or when the chosen provider is a non-rendering stub).
| encrypted.append(static_cast<char>(kVersion)); | ||
| encrypted.append(static_cast<char>(kOptions)); | ||
| encrypted.append(encSalt); | ||
| encrypted.append(authSalt); | ||
| encrypted.append(iv); |
There was a problem hiding this comment.
PasswordEncryption::encrypt() appends into encrypted without clearing it first, so callers that reuse a QByteArray may get corrupted output from concatenation. Initialize the output buffer deterministically (e.g., encrypted.clear() or assign to a new QByteArray) before appending the header/salts/iv/payload.
| qint32 encryptedKeyLength = 0; | ||
| ds >> encryptedKeyLength; | ||
|
|
||
| if (data.size() < (24 + encryptedKeyLength)) return contracts::LoadStatus::InvalidData; |
There was a problem hiding this comment.
encryptedKeyLength is parsed from untrusted input but is not validated for negative values or a sane upper bound. With a negative length, QByteArray::mid() treats it as 'to end', and 24 + encryptedKeyLength can underflow the intended size check. Validate encryptedKeyLength >= 0 and encryptedKeyLength <= data.size() - 24 before slicing.
| if (data.size() < (24 + encryptedKeyLength)) return contracts::LoadStatus::InvalidData; | |
| const int remainingDataSize = data.size() - 24; | |
| if (encryptedKeyLength < 0 || encryptedKeyLength > remainingDataSize) { | |
| return contracts::LoadStatus::InvalidData; | |
| } |
| ### Running Everywhere | ||
| One cool thing we did is make the browser engine flexible. | ||
| - Most people will use **Qt WebEngine** (it's fast and integrated). | ||
| - If your system doesn't support it, we automatically try to use **WebKitGTK** as a backup. | ||
|
|
||
| It supports: | ||
| This means SEB should "just work" on pretty much any Linux device you throw at it. |
There was a problem hiding this comment.
The PR description/release notes state that in QtWebEngine-free compatibility mode the app cannot render exam pages and disables web navigation, but the README now says it will automatically use WebKitGTK as a backup engine. Please reconcile the documentation with the intended behavior (either document WebKitGTK as a supported rendering fallback, or document the mode as non-rendering compatibility UI only).
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/Jvr2022/seb-linux/sessions/f44f8685-8be9-45cc-a438-33806586b5b9 Co-authored-by: Jvr2022 <109031036+Jvr2022@users.noreply.github.com>
Applied all the changes from that review thread in commit e2699a0:
|
Jvr2022
changed the title
Addresses a batch of code review findings across engine abstraction, security, and cryptography layers.
Summary
encrypt()could produce corrupted output on buffer reuse; andencryptedKeyLengthwas not validated before slicing.What Changed
WebKitGTK guards & portability
!SEB_HAS_QTWEBENGINE && SEB_HAS_WEBKITGTK— no-engine builds no longer pull in GTK headersengine_factory.cppuses#elif SEB_HAS_WEBKITGTKinstead of bare#elseQX11Infoinclude (unused in Qt 6)gtk_init()now called exactly once per process viastatic boolguard instead of per-viewInterface correctness
WebKitGtkProfile::setUrlRequestInterceptorparameter type corrected from concreteRequestInterceptor *tocontracts::IRequestInterceptor *to properly implement theIWebProfileinterfaceinterceptor_.get()→interceptor_.data()for consistentQScopedPointerusageSecurity
devBypass: production exam sessions can no longer exit fullscreen via F11; the event is still consumed to block system-level handlingCryptography
PasswordEncryption::encrypt(): addedencrypted.clear()before writing output to prevent corruption when callers reuse aQByteArrayPublicKeySymmetricEncryption::decrypt(): addedencryptedKeyLength < 0and upper-bound check beforemid()to prevent misuse with untrusted inputVerification