Skip to content
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.

Sign up for GitHub

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

WebAPI: Allow to specify session cookie name #18384

Merged
merged 1 commit into from
Jan 17, 2023
Merged

WebAPI: Allow to specify session cookie name #18384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/base/preferences.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,16 @@ void Preferences::setWebUISessionTimeout(const int timeout)
setValue(u"Preferences/WebUI/SessionTimeout"_qs, timeout);
}

QString Preferences::getWebAPISessionCookieName() const
{
return value<QString>(u"WebAPI/SessionCookieName"_qs);
}

void Preferences::setWebAPISessionCookieName(const QString &cookieName)
{
setValue(u"WebAPI/SessionCookieName"_qs, cookieName);
}

bool Preferences::isWebUiClickjackingProtectionEnabled() const
{
return value(u"Preferences/WebUI/ClickjackingProtection"_qs, true);
Expand Down
2 changes: 2 additions & 0 deletions src/base/preferences.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ class Preferences : public QObject
void setWebUIBanDuration(std::chrono::seconds duration);
int getWebUISessionTimeout() const;
void setWebUISessionTimeout(int timeout);
QString getWebAPISessionCookieName() const;
void setWebAPISessionCookieName(const QString &cookieName);

// WebUI security
bool isWebUiClickjackingProtectionEnabled() const;
Expand Down
29 changes: 24 additions & 5 deletions src/webui/webapplication.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
#include "base/logger.h"
#include "base/preferences.h"
#include "base/types.h"
#include "base/utils/bytearray.h"
#include "base/utils/fs.h"
#include "base/utils/misc.h"
#include "base/utils/random.h"
Expand All @@ -63,7 +62,7 @@
#include "api/transfercontroller.h"

const int MAX_ALLOWED_FILESIZE = 10 * 1024 * 1024;
const auto C_SID = QByteArrayLiteral("SID"); // name of session id cookie
const QString DEFAULT_SESSION_COOKIE_NAME = u"SID"_qs;

const QString WWW_FOLDER = u":/www"_qs;
const QString PUBLIC_FOLDER = u"/public"_qs;
Expand Down Expand Up @@ -129,6 +128,18 @@ namespace

return languages.join(u'\n');
}

bool isValidCookieName(const QString &cookieName)
{
if (cookieName.isEmpty() || (cookieName.size() > 128))
return false;

const QRegularExpression invalidNameRegex {u"[^a-zA-Z0-9_\\-]"_qs};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very general remark and I don't know if this has been discussed already.
For regex we should consider switching to raw string literals. It will improve regex readability by dropping the need for backslash escapes.
However, I don't know if raw string literals can mix with user-defined literals (_qs).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this has been discussed already.
For regex we should consider switching to raw string literals.

We didn't discuss such a requirement earlier.

It will improve regex readability by dropping the need for backslash escapes.

Personally, I don't care about it in trivial cases like this. In complex regexps, it really makes sense.
However, if you see the point of making this a requirement for regular expressions, I don't mind. But in this case, it would be good to start by converting all existing instances, so that later some newly-minted contributors do not poke their finger there when they are pointed out to such a requirement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not keen into make it a hard requirement. You should treat it more like a suggestion IMO.
Do you think it should be a requirement?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it should be a requirement?

No.

if (invalidNameRegex.match(cookieName).hasMatch())
return false;

return true;
}
}

WebApplication::WebApplication(IApplication *app, QObject *parent)
Expand All @@ -141,6 +152,14 @@ WebApplication::WebApplication(IApplication *app, QObject *parent)

configure();
connect(Preferences::instance(), &Preferences::changed, this, &WebApplication::configure);

m_sessionCookieName = Preferences::instance()->getWebAPISessionCookieName();
if (!isValidCookieName(m_sessionCookieName))
{
LogMsg(tr("Unacceptable session cookie name is specified: '%1'. Default one is used.")
.arg(m_sessionCookieName), Log::WARNING);
m_sessionCookieName = DEFAULT_SESSION_COOKIE_NAME;
}
}

WebApplication::~WebApplication()
Expand Down Expand Up @@ -563,7 +582,7 @@ void WebApplication::sessionInitialize()
{
Q_ASSERT(!m_currentSession);

const QString sessionId {parseCookie(m_request.headers.value(u"cookie"_qs)).value(QString::fromLatin1(C_SID))};
const QString sessionId {parseCookie(m_request.headers.value(u"cookie"_qs)).value(m_sessionCookieName)};

// TODO: Additional session check

Expand Down Expand Up @@ -649,7 +668,7 @@ void WebApplication::sessionStart()
m_currentSession->registerAPIController<TransferController>(u"transfer"_qs);
m_sessions[m_currentSession->id()] = m_currentSession;

QNetworkCookie cookie(C_SID, m_currentSession->id().toUtf8());
QNetworkCookie cookie {m_sessionCookieName.toLatin1(), m_currentSession->id().toUtf8()};
cookie.setHttpOnly(true);
cookie.setSecure(m_isSecureCookieEnabled && m_isHttpsEnabled);
cookie.setPath(u"/"_qs);
Expand All @@ -663,7 +682,7 @@ void WebApplication::sessionEnd()
{
Q_ASSERT(m_currentSession);

QNetworkCookie cookie(C_SID);
QNetworkCookie cookie {m_sessionCookieName.toLatin1()};
cookie.setPath(u"/"_qs);
cookie.setExpirationDate(QDateTime::currentDateTime().addDays(-1));

Expand Down
1 change: 1 addition & 0 deletions src/webui/webapplication.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ class WebApplication final
bool m_isAuthSubnetWhitelistEnabled;
QVector<Utils::Net::Subnet> m_authSubnetWhitelist;
int m_sessionTimeout;
QString m_sessionCookieName;

// security related
QStringList m_domainList;
Expand Down