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. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds an optional anti-cheat mode that opens and switches a Linux virtual terminal to graphics mode with signal-safe cleanup, extends CLI with Changes
Sequence DiagramssequenceDiagram
participant App as Application
participant DevTTY as /dev/tty0
participant Kernel as VT Kernel
participant Handlers as Signal Handlers
App->>DevTTY: open("/dev/tty0")
App->>DevTTY: ioctl(VT_OPENQRY) → find free VT
DevTTY-->>App: free VT N
App->>Kernel: ioctl(VT_ACTIVATE / VT_WAITACTIVE)
Kernel-->>App: VT activated
App->>Kernel: ioctl(KDSETMODE, KD_GRAPHICS)
App->>Handlers: register atexit + signal handlers
Note over App,Handlers: running in anti-cheat graphics VT
Handlers->>Kernel: restore KD_TEXT and reactivate prior VT on exit/signal
Kernel-->>Handlers: restored
sequenceDiagram
participant Main as Main Process
participant Env as Environment
participant PKExec as pkexec
participant Child as Elevated Process
Main->>Main: check settings (fullscreen & always-on-top)
alt both enabled and no --anti-cheat
Main->>Env: read SEB_PASSWORD
alt SEB_PASSWORD set
Env-->>Main: password (marked used)
else
Main->>Main: prompt user for password (mark used if provided)
end
Main->>Main: rewrite argv to include --anti-cheat
Main->>PKExec: spawn detached (optionally "env SEB_PASSWORD=...") with rewritten args
PKExec->>Child: start elevated process
Child-->>Main: started (success) / failed
Main->>Main: exit(0) on success or exit(1) on failure
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.cpp`:
- Around line 247-252: Replace the current environment-argument transport of the
SEB password with the previous one-shot temp-file flow: stop setting
SEB_PASSWORD via pkexec env and instead create a QTemporaryFile (like the prior
QTemporaryFile* tempFile approach), write the password into it, pass only
SEB_PASSWORD_FILE=<tempfile path> to the relaunched child, and ensure the child
code that reads the password (the lambda used by seb::loadSettingsFromResource
and any other re-launch paths referenced around lines 293-296) reads from that
file and deletes/unlinks the temporary file immediately after reading; update
locations that currently rely on qgetenv("SEB_PASSWORD") to read and clear the
temp-file mechanism instead.
- Around line 52-93: Change setup_barebones_vt() to return bool and ensure it
performs full cleanup on any failure: when opening g_tty0_fd or g_new_tty_fd, if
ioctl VT_OPENQRY/VT_ACTIVATE/VT_WAITACTIVE or KDSETMODE fails, close any opened
fds (g_new_tty_fd and g_tty0_fd as needed), restore original VT if already
changed (using VT_ACTIVATE back to g_original_vt), and call cleanup_vt_and_exit
only on atexit success path; return false on error and true only when VT switch
and KDSETMODE(KD_GRAPHICS) both succeed so the caller can abort anti-cheat
startup when false (update callers to check the bool and stop startup when
false).
| const seb::ResourceLoadResult loaded = seb::loadSettingsFromResource(resource, [&userPassword, &usedPassword](bool hashed) { | ||
| if (qEnvironmentVariableIsSet("SEB_PASSWORD")) { | ||
| userPassword = QString::fromUtf8(qgetenv("SEB_PASSWORD")); | ||
| usedPassword = true; | ||
| return userPassword; | ||
| } |
There was a problem hiding this comment.
Don't pass the SEB password through pkexec env.
The pkexec env SEB_PASSWORD=... handoff exposes the password in the detached process command line and then keeps it in the child environment for the rest of the session. Please switch this back to a one-shot transport such as the previous SEB_PASSWORD_FILE temp-file flow instead of argv/environment transport.
Based on learnings: in src/main.cpp, the prior anticheat re-launch block intentionally used QTemporaryFile* tempFile with SEB_PASSWORD_FILE, and the child deleted that file after reading it.
Also applies to: 293-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.cpp` around lines 247 - 252, Replace the current
environment-argument transport of the SEB password with the previous one-shot
temp-file flow: stop setting SEB_PASSWORD via pkexec env and instead create a
QTemporaryFile (like the prior QTemporaryFile* tempFile approach), write the
password into it, pass only SEB_PASSWORD_FILE=<tempfile path> to the relaunched
child, and ensure the child code that reads the password (the lambda used by
seb::loadSettingsFromResource and any other re-launch paths referenced around
lines 293-296) reads from that file and deletes/unlinks the temporary file
immediately after reading; update locations that currently rely on
qgetenv("SEB_PASSWORD") to read and clear the temp-file mechanism instead.
There was a problem hiding this comment.
The password isnt that secret...
There was a problem hiding this comment.
@incomplete-tree, understood — if the SEB password is not considered sensitive in your deployment context, the environment-variable transport is simpler. The main residual risk worth being aware of is that environment variables of a running process can be read by other processes owned by the same user (e.g. via /proc/<pid>/environ), and they persist for the lifetime of the child process rather than being consumed immediately. But if that's acceptable for your threat model, no objection.
✏️ Learnings added
Learnt from: incomplete-tree
Repo: Jvr2022/seb-linux PR: 5
File: src/main.cpp:247-252
Timestamp: 2026-03-22T13:58:54.431Z
Learning: In `src/main.cpp` of the `seb-linux` project (C++/Qt), the SEB password is intentionally passed to the pkexec-relaunched anti-cheat child process via the `SEB_PASSWORD` environment variable (using `pkexec env SEB_PASSWORD=...`). The project author considers this acceptable — do not flag environment-variable password transport as a security issue in this codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: incomplete-tree
Repo: Jvr2022/seb-linux PR: 4
File: src/main.cpp:316-338
Timestamp: 2026-03-22T13:04:35.013Z
Learning: In `src/main.cpp` of the `seb-linux` project (C++/Qt), the `QTemporaryFile* tempFile` created in the anticheat re-launch block (around the `startDetached()` call) is intentionally not deleted on the success path. The parent process exits immediately (`return 0`) and the child process (re-launched with `--anti-cheat` via `pkexec`) is responsible for deleting the file via `file.remove()` inside the `SEB_PASSWORD_FILE` password callback. This is by design; do not flag it as a leak.
Learnt from: incomplete-tree
Repo: Jvr2022/seb-linux PR: 3
File: src/seb_session.cpp:39-79
Timestamp: 2026-03-19T18:39:47.748Z
Learning: In `src/seb_session.cpp` of the `seb-linux` project (C++/Qt), the `getCachedSebVersion()` function intentionally performs a blocking HTTP fetch (with a 5 s timeout) to `https://api.github.com/repos/SafeExamBrowser/seb-win-refactoring/releases/latest` during startup and advertises the remote tag in the `SEB/x.y.z` User-Agent token, rather than using the locally shipped application version. This behaviour is by design; do not flag it as an issue.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cbaaace4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pkexecArgs << QStringLiteral("env"); | ||
| pkexecArgs << (QStringLiteral("SEB_PASSWORD=") + userPassword); |
There was a problem hiding this comment.
When an encrypted .seb file triggers the anti-cheat relaunch, this builds pkexec env SEB_PASSWORD=..., which copies the plaintext config/admin password into the detached process' argument vector. That value is observable through ps or /proc/*/cmdline while the polkit prompt is open, so local users can recover a secret that was originally entered in a password dialog.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main.cpp (1)
211-212: Minor formatting inconsistency.Line 211 has trailing whitespace, and line 212 has an extra leading space before
const QIcon. Consider normalizing the indentation.🧹 Proposed formatting fix
- - const QIcon appIcon(QStringLiteral(":/assets/icons/safe-exam-browser.png")); + const QIcon appIcon(QStringLiteral(":/assets/icons/safe-exam-browser.png"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cpp` around lines 211 - 212, Normalize indentation around the QIcon declaration: remove the trailing whitespace on the previous line and remove the extra leading space before the `const QIcon appIcon(QStringLiteral(":/assets/icons/safe-exam-browser.png"));` statement so it aligns with surrounding code; target the `appIcon` declaration / `QStringLiteral` usage to locate the line and ensure consistent indentation across the block.
🤖 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/main.cpp`:
- Around line 211-212: Normalize indentation around the QIcon declaration:
remove the trailing whitespace on the previous line and remove the extra leading
space before the `const QIcon
appIcon(QStringLiteral(":/assets/icons/safe-exam-browser.png"));` statement so
it aligns with surrounding code; target the `appIcon` declaration /
`QStringLiteral` usage to locate the line and ensure consistent indentation
across the block.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main.cpp (1)
84-108:⚠️ Potential issue | 🟠 MajorThe VT rollback path is still incomplete.
If
VT_GETSTATEfails,g_original_vtstays unset and cleanup falls back to VT 1. IfVT_ACTIVATEsucceeds beforeVT_WAITACTIVEfails, this branch closes both descriptors without switching back. Because the caller immediately_exit(1)s on failure, the user can be left on the wrong console. MakeVT_GETSTATEmandatory and always reactivateg_original_vtbefore returningfalsefrom the switch path.Suggested fix
struct vt_stat vts; - if (ioctl(g_tty0_fd, VT_GETSTATE, &vts) == 0) { - g_original_vt = vts.v_active; + if (ioctl(g_tty0_fd, VT_GETSTATE, &vts) < 0) { + qWarning() << "Could not determine the current VT."; + close(g_new_tty_fd); + g_new_tty_fd = -1; + close(g_tty0_fd); + g_tty0_fd = -1; + return false; } + g_original_vt = vts.v_active; if (ioctl(g_tty0_fd, VT_ACTIVATE, free_vt) < 0 || ioctl(g_tty0_fd, VT_WAITACTIVE, free_vt) < 0) { qWarning() << "Could not switch to VT" << free_vt; + ioctl(g_tty0_fd, VT_ACTIVATE, g_original_vt); + ioctl(g_tty0_fd, VT_WAITACTIVE, g_original_vt); close(g_new_tty_fd); g_new_tty_fd = -1; close(g_tty0_fd);
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.cpp`:
- Around line 315-322: The pkexec argument ordering is wrong: when usedPassword
is true you prepend "env" and VAR=... so "--keep-cwd" ends up being interpreted
by env instead of pkexec; modify pkexecArgs construction (pkexecArgs,
usedPassword, userPassword, QCoreApplication::applicationFilePath(), args) so
that QStringLiteral("--keep-cwd") is pushed onto pkexecArgs before the
conditional that prepends "env" and the SEB_PASSWORD assignment, ensuring
--keep-cwd is treated as a pkexec option and not passed to env.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10409d26-3abc-4781-8e70-3c52ccf04290
📒 Files selected for processing (3)
packaging/arch/PKGBUILDscripts/build-release.shsrc/main.cpp
✅ Files skipped from review due to trivial changes (2)
- packaging/arch/PKGBUILD
- scripts/build-release.sh
2 participants
Summary by CodeRabbit
New Features
Behavior Changes
Chores