Skip to content

Commit ef67e0e

Browse files
annejanclaude
andauthored
test: cover SSH_AUTH_SOCK override soft-validation via Util helper (#1469)
The Settings dialog warning logic added in #1439 (configdialog.cpp on_accepted) had no unit tests. Extract the validation rules into a pure helper so they're testable without a Qt event loop or QMessageBox spy. Production change (mechanical, no semantic diff): - New Util::sshAuthSockOverrideStatus(QString) returning an enum: Valid / DoesNotExist / NotReadable / NotUnixDomainSocket. Same three checks as before (exists, readable, S_ISSOCK on Unix), now reusable and unit-testable. - ConfigDialog::on_accepted switches the three inline if/else branches to a switch over the enum. Translation strings stay in ConfigDialog (Util has no tr() callers). - Dropped now-unused #include <QFileInfo> and #include <sys/stat.h> from configdialog.cpp. Tests (tst_util.cpp): - sshAuthSockOverrideStatusDoesNotExist — non-existent path - sshAuthSockOverrideStatusRegularFileRejected — regular file (Unix) - sshAuthSockOverrideStatusNotReadable — chmod 0 (Unix; skipped under root since euid 0 ignores mode bits) - sshAuthSockOverrideStatusValid — bind(2) a real Unix domain socket and assert Valid POSIX socket(2)/bind(2) instead of QLocalServer to avoid pulling QtNetwork into the util test target. Unix-only tests SKIP on Windows where the production code also skips the socket check (ssh-agent uses a named pipe there). Build clean, 124/124 util tests pass (was 120, +4). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 18a14de commit ef67e0e

4 files changed

Lines changed: 182 additions & 17 deletions

File tree

src/configdialog.cpp

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
#include <QClipboard>
1212
#include <QDir>
1313
#include <QFileDialog>
14-
#include <QFileInfo>
1514
#include <QMessageBox>
1615
#include <QPushButton>
1716
#include <QSystemTrayIcon>
@@ -21,10 +20,6 @@
2120
#include <windows.h>
2221
#endif
2322

24-
#ifdef Q_OS_UNIX
25-
#include <sys/stat.h>
26-
#endif
27-
2823
#ifdef QT_DEBUG
2924
#include "debughelper.h"
3025
#endif
@@ -258,21 +253,19 @@ void ConfigDialog::on_accepted() {
258253
const QString sshAuthSockOverride = ui->sshAuthSockOverride->text().trimmed();
259254
if (!sshAuthSockOverride.isEmpty()) {
260255
QString reason;
261-
QFileInfo fi(sshAuthSockOverride);
262-
if (!fi.exists()) {
256+
switch (Util::sshAuthSockOverrideStatus(sshAuthSockOverride)) {
257+
case Util::SshAuthSockOverrideStatus::Valid:
258+
break;
259+
case Util::SshAuthSockOverrideStatus::DoesNotExist:
263260
reason = tr("The path does not exist.");
264-
} else if (!fi.isReadable()) {
261+
break;
262+
case Util::SshAuthSockOverrideStatus::NotReadable:
265263
reason = tr("The path is not readable.");
264+
break;
265+
case Util::SshAuthSockOverrideStatus::NotUnixDomainSocket:
266+
reason = tr("The path is not a Unix domain socket.");
267+
break;
266268
}
267-
#ifdef Q_OS_UNIX
268-
if (reason.isEmpty()) {
269-
struct stat st;
270-
if (::stat(sshAuthSockOverride.toLocal8Bit().constData(), &st) != 0 ||
271-
!S_ISSOCK(st.st_mode)) {
272-
reason = tr("The path is not a Unix domain socket.");
273-
}
274-
}
275-
#endif
276269
if (!reason.isEmpty()) {
277270
QMessageBox::warning(
278271
this, tr("Potentially invalid SSH_AUTH_SOCK override"),

src/util.cpp

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#ifdef Q_OS_WIN
2828
#include <windows.h>
2929
#else
30+
#include <sys/stat.h>
3031
#include <sys/time.h>
3132
#endif
3233
#include "qtpasssettings.h"
@@ -285,6 +286,29 @@ auto Util::isPathInStore(const QString &storeRoot, const QString &candidate)
285286
resolved.startsWith(rootCanon + QLatin1Char('/'));
286287
}
287288

289+
auto Util::sshAuthSockOverrideStatus(const QString &path)
290+
-> SshAuthSockOverrideStatus {
291+
const QFileInfo fi(path);
292+
if (!fi.exists()) {
293+
return SshAuthSockOverrideStatus::DoesNotExist;
294+
}
295+
if (!fi.isReadable()) {
296+
return SshAuthSockOverrideStatus::NotReadable;
297+
}
298+
#ifdef Q_OS_UNIX
299+
// S_ISSOCK isn't exposed by QFileInfo; go through stat(2). On Windows
300+
// ssh-agent uses a named pipe (\\.\pipe\openssh-ssh-agent.<sid>) and the
301+
// socket check would always fail, so skip it there — exists + readable is
302+
// the best we can do.
303+
struct stat st{};
304+
if (::stat(path.toLocal8Bit().constData(), &st) != 0 ||
305+
!S_ISSOCK(st.st_mode)) {
306+
return SshAuthSockOverrideStatus::NotUnixDomainSocket;
307+
}
308+
#endif
309+
return SshAuthSockOverrideStatus::Valid;
310+
}
311+
288312
/**
289313
* @brief Finds the absolute path of a binary by searching the PATH environment
290314
* variable.

src/util.h

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,34 @@ class StoreModel;
1919
*/
2020
class Util {
2121
public:
22+
/**
23+
* @brief Classify a non-empty SSH_AUTH_SOCK override path supplied by the
24+
* user.
25+
*
26+
* Empty / unset input is handled by the caller (no warning when the field
27+
* is blank). For a non-empty path, this returns one of:
28+
*
29+
* - `Valid` — exists, readable, and on Unix is a Unix domain socket.
30+
* - `DoesNotExist` — `QFileInfo::exists()` returned false.
31+
* - `NotReadable` — exists but `QFileInfo::isReadable()` returned false.
32+
* - `NotUnixDomainSocket` — Unix-only: `stat()` says it's not `S_ISSOCK`.
33+
*
34+
* On Windows the socket check is skipped — ssh-agent uses a named pipe.
35+
*/
36+
enum class SshAuthSockOverrideStatus {
37+
Valid,
38+
DoesNotExist,
39+
NotReadable,
40+
NotUnixDomainSocket,
41+
};
42+
/**
43+
* @brief Validate an SSH_AUTH_SOCK override path. Caller is expected to
44+
* skip validation when the input is empty / whitespace-only.
45+
* @param path Non-empty candidate path.
46+
* @return Classification per SshAuthSockOverrideStatus.
47+
*/
48+
static auto sshAuthSockOverrideStatus(const QString &path)
49+
-> SshAuthSockOverrideStatus;
2250
/**
2351
* @brief Locate an executable by searching the process PATH and (on Windows)
2452
* falling back to WSL.

tests/auto/util/tst_util.cpp

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,12 @@
1111
#include <QUuid>
1212
#include <QtTest>
1313
#ifndef Q_OS_WIN
14+
#include <sys/socket.h>
1415
#include <sys/stat.h>
16+
#include <sys/un.h>
17+
#include <unistd.h>
18+
19+
#include <cstring>
1520
#endif
1621

1722
#include "../../../src/enums.h"
@@ -226,6 +231,11 @@ private Q_SLOTS:
226231
void isPathInStoreRejectsEmptyArgs();
227232
// .gpg-id permission hardening (security)
228233
void writeGpgIdFileSetsOwnerOnlyPerms();
234+
// SSH_AUTH_SOCK override soft-validation (Settings dialog warning)
235+
void sshAuthSockOverrideStatusDoesNotExist();
236+
void sshAuthSockOverrideStatusRegularFileRejected();
237+
void sshAuthSockOverrideStatusNotReadable();
238+
void sshAuthSockOverrideStatusValid();
229239
};
230240

231241
/**
@@ -2327,5 +2337,115 @@ void tst_util::writeGpgIdFileSetsOwnerOnlyPerms() {
23272337
#endif
23282338
}
23292339

2340+
// ---------------------------------------------------------------------------
2341+
// Util::sshAuthSockOverrideStatus tests
2342+
//
2343+
// Backs the Settings dialog soft-warning logic added in #1439. The dialog
2344+
// shows a non-blocking warning when the user-typed override path is bogus
2345+
// (missing, unreadable, or not a Unix domain socket) but still saves the
2346+
// value as entered. These tests drive the pure validation function directly
2347+
// so we don't need a Qt event loop / QMessageBox spy.
2348+
// ---------------------------------------------------------------------------
2349+
2350+
/**
2351+
* @brief A non-existent path is classified DoesNotExist.
2352+
*/
2353+
void tst_util::sshAuthSockOverrideStatusDoesNotExist() {
2354+
QTemporaryDir tmp;
2355+
QVERIFY2(tmp.isValid(), "tmp dir should be valid");
2356+
QCOMPARE(Util::sshAuthSockOverrideStatus(tmp.path() + "/no-such-socket"),
2357+
Util::SshAuthSockOverrideStatus::DoesNotExist);
2358+
}
2359+
2360+
/**
2361+
* @brief A regular file that exists but isn't a Unix domain socket is
2362+
* rejected on Unix. Skipped on Windows where the socket check is a
2363+
* no-op (ssh-agent uses a named pipe there).
2364+
*/
2365+
void tst_util::sshAuthSockOverrideStatusRegularFileRejected() {
2366+
#ifdef Q_OS_WIN
2367+
QSKIP("socket-type check is Unix-only");
2368+
#else
2369+
QTemporaryDir tmp;
2370+
QVERIFY2(tmp.isValid(), "tmp dir should be valid");
2371+
const QString filePath = tmp.path() + "/regular-file";
2372+
QFile f(filePath);
2373+
QVERIFY2(f.open(QIODevice::WriteOnly), "should be able to create a file");
2374+
f.close();
2375+
QCOMPARE(Util::sshAuthSockOverrideStatus(filePath),
2376+
Util::SshAuthSockOverrideStatus::NotUnixDomainSocket);
2377+
#endif
2378+
}
2379+
2380+
/**
2381+
* @brief A file that exists but has no read permission is classified
2382+
* NotReadable. Skipped on Windows because Qt's permission bits
2383+
* don't round-trip the same way.
2384+
*/
2385+
void tst_util::sshAuthSockOverrideStatusNotReadable() {
2386+
#ifdef Q_OS_WIN
2387+
QSKIP("Unix permission bits don't round-trip on Windows");
2388+
#else
2389+
// Root user on Linux ignores read-mode bits, so the test would
2390+
// falsely return Valid. Skip in that case.
2391+
if (::geteuid() == 0) {
2392+
QSKIP("root sees all files as readable; skip");
2393+
}
2394+
QTemporaryDir tmp;
2395+
QVERIFY2(tmp.isValid(), "tmp dir should be valid");
2396+
const QString filePath = tmp.path() + "/unreadable";
2397+
QFile f(filePath);
2398+
QVERIFY2(f.open(QIODevice::WriteOnly), "should be able to create a file");
2399+
f.close();
2400+
QVERIFY2(QFile::setPermissions(filePath, QFile::Permissions{}),
2401+
"should be able to chmod 0 on the file");
2402+
QCOMPARE(Util::sshAuthSockOverrideStatus(filePath),
2403+
Util::SshAuthSockOverrideStatus::NotReadable);
2404+
// Restore something so QTemporaryDir can clean up.
2405+
QFile::setPermissions(filePath, QFile::ReadOwner | QFile::WriteOwner);
2406+
#endif
2407+
}
2408+
2409+
/**
2410+
* @brief A real Unix domain socket (bind(2)) is classified Valid.
2411+
* Unix-only.
2412+
*/
2413+
void tst_util::sshAuthSockOverrideStatusValid() {
2414+
#ifdef Q_OS_WIN
2415+
QSKIP("socket creation API differs on Windows");
2416+
#else
2417+
QTemporaryDir tmp;
2418+
QVERIFY2(tmp.isValid(), "tmp dir should be valid");
2419+
const QString sockPath = tmp.path() + "/agent.sock";
2420+
const QByteArray sockBytes = sockPath.toLocal8Bit();
2421+
2422+
// sun_path is typically 108 bytes on Linux; QTemporaryDir gives us a
2423+
// short /tmp/qttest_XXXXXX prefix, so this should fit easily.
2424+
QVERIFY2(sockBytes.size() < static_cast<int>(sizeof(sockaddr_un{}.sun_path)),
2425+
"socket path too long for sockaddr_un");
2426+
2427+
const int fd = ::socket(AF_UNIX, SOCK_STREAM, 0);
2428+
QVERIFY2(fd >= 0, "socket(AF_UNIX) failed");
2429+
2430+
sockaddr_un addr{};
2431+
addr.sun_family = AF_UNIX;
2432+
std::memcpy(addr.sun_path, sockBytes.constData(),
2433+
static_cast<size_t>(sockBytes.size()));
2434+
2435+
const int br = ::bind(fd, reinterpret_cast<sockaddr *>(&addr), sizeof(addr));
2436+
if (br != 0) {
2437+
::close(fd);
2438+
QFAIL("bind(AF_UNIX) failed");
2439+
}
2440+
2441+
QCOMPARE(Util::sshAuthSockOverrideStatus(sockPath),
2442+
Util::SshAuthSockOverrideStatus::Valid);
2443+
2444+
::close(fd);
2445+
// QTemporaryDir will rm -rf the directory on destruction, removing the
2446+
// socket file along with it.
2447+
#endif
2448+
}
2449+
23302450
QTEST_MAIN(tst_util)
23312451
#include "tst_util.moc"

0 commit comments

Comments
 (0)