fix(auth): support __Host-/__Secure- prefixed session cookies and proxy-relative paths (issue #635) (#648)
- CookieSerialization: add SESSION_COOKIE_NAMES (__Host-, __Secure-, bare) and isSessionCookieName(); isSessionCookie now matches any variant. - PersistentCookieJar.getSessionCookieValue(): iterate SESSION_COOKIE_NAMES, returning the strictest prefixed match when present. - HermesWsClient.refreshWsTicketIfNeeded(): drop the bare-name session-cookie preflight in gated mode — the server may hold a valid refresh cookie under a prefixed name; the authenticated ticket endpoint is the authority. Ticket URL already derived from endpoint (issue #634). - HermesWsClient: stop logging WS close/failure reason strings and exception messages (class name only) to avoid leaking server context / URLs. - ProfileScopeInterceptor: match profile-scoped paths via endpoint-relative request path so reverse-proxy prefixes (e.g. /hermes/api/...) are honored; wrap endpoint read in runCatching. Verified: ktlintCheck, testDebugUnitTest, lintDebug all green locally. Co-authored-by: M57 <Hy4ri@users.noreply.github.com>
This commit is contained in:
1 parent
86c780c10b
commit
c8a5055a84
7 files changed
+225
-15
No files matched your search
@@ -86,8 +86,25 @@ fun sessionCookieFromValue(value: String): Cookie? = wrapSessionCookie(value)
|
|||||||
|
|
||||||
const val SESSION_COOKIE_NAME = "hermes_session_at"
|
const val SESSION_COOKIE_NAME = "hermes_session_at"
|
||||||
|
|
||||||
|
/**
|
||||||
|
* All names the dashboard may use for its session cookie, strictest variant
|
||||||
|
* first. Over HTTPS the server emits a [`__Host-`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#cookie_prefixes)
|
||||||
|
* (root-facing HTTPS) or `__Secure-` (HTTPS reverse-proxy with a path prefix)
|
||||||
|
* prefixed cookie; the bare name is the legacy HTTP form. Ordering matches the
|
||||||
|
* server's own fallback precedence so the most-constrained variant wins.
|
||||||
|
*/
|
||||||
|
val SESSION_COOKIE_NAMES: List<String> =
|
||||||
|
listOf(
|
||||||
|
"__Host-$SESSION_COOKIE_NAME",
|
||||||
|
"__Secure-$SESSION_COOKIE_NAME",
|
||||||
|
SESSION_COOKIE_NAME,
|
||||||
|
)
|
||||||
|
|
||||||
|
/** True when [name] is any recognized dashboard session-cookie name. */
|
||||||
|
fun isSessionCookieName(name: String): Boolean = name in SESSION_COOKIE_NAMES
|
||||||
|
|
||||||
/** True when [cookie] is the dashboard session cookie. */
|
/** True when [cookie] is the dashboard session cookie. */
|
||||||
fun isSessionCookie(cookie: Cookie): Boolean = cookie.name == SESSION_COOKIE_NAME
|
fun isSessionCookie(cookie: Cookie): Boolean = isSessionCookieName(cookie.name)
|
||||||
|
|
||||||
/** Parse a raw `Set-Cookie` header value (sans the `hermes_session_at=` key). */
|
/** Parse a raw `Set-Cookie` header value (sans the `hermes_session_at=` key). */
|
||||||
fun Cookie.Companion.parseRaw(
|
fun Cookie.Companion.parseRaw(
|
||||||
|
|||||||
@@ -74,8 +74,14 @@ class PersistentCookieJar(
|
|||||||
return null
|
return null
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Convenience: value of the `hermes_session_at` session cookie. */
|
/**
|
||||||
fun getSessionCookieValue(): String? = getCookie(SESSION_COOKIE_NAME)?.value
|
* Convenience: value of the dashboard session cookie.
|
||||||
|
*
|
||||||
|
* Iterates [SESSION_COOKIE_NAMES] (strictest `__Host-` / `__Secure-`
|
||||||
|
* prefix first) and returns the first match. Over HTTPS the server may
|
||||||
|
* store the cookie under a prefixed name the bare-name lookup would miss.
|
||||||
|
*/
|
||||||
|
fun getSessionCookieValue(): String? = SESSION_COOKIE_NAMES.firstNotNullOfOrNull { name -> getCookie(name)?.value }
|
||||||
|
|
||||||
override fun saveFromResponse(
|
override fun saveFromResponse(
|
||||||
url: HttpUrl,
|
url: HttpUrl,
|
||||||
|
|||||||
@@ -62,7 +62,15 @@ object ProfileScopeInterceptor : Interceptor {
|
|||||||
return chain.proceed(request) // explicit param wins
|
return chain.proceed(request) // explicit param wins
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!isProfileScopedPath(url.encodedPath)) {
|
// Derive the path relative to the endpoint's proxy prefix so profile
|
||||||
|
// scoping works when the dashboard is served behind a reverse proxy
|
||||||
|
// (e.g. `/hermes/api/status` → relative `/api/status`). A transient
|
||||||
|
// DataStore read failure mid-intercept must not crash every request.
|
||||||
|
val relativePath =
|
||||||
|
runCatching { AuthManager.endpoint().relativeRequestPath(url) }
|
||||||
|
.getOrDefault(url.encodedPath)
|
||||||
|
|
||||||
|
if (!isProfileScopedPath(relativePath)) {
|
||||||
return chain.proceed(request)
|
return chain.proceed(request)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -253,13 +253,6 @@ object HermesWsClient {
|
|||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
|
|
||||||
val sessionCookie = AuthManager.getSessionCookie()
|
|
||||||
if (sessionCookie.isNullOrBlank()) {
|
|
||||||
Log.w(TAG, "WS ticket refresh aborted: no session cookie found in gated mode")
|
|
||||||
_connectionStatus.value = ConnectionStatus.AUTH_EXPIRED
|
|
||||||
return false
|
|
||||||
}
|
|
||||||
|
|
||||||
try {
|
try {
|
||||||
val client = OkHttpProvider.probe
|
val client = OkHttpProvider.probe
|
||||||
val request =
|
val request =
|
||||||
@@ -294,7 +287,7 @@ object HermesWsClient {
|
|||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
} catch (e: Exception) {
|
} catch (e: Exception) {
|
||||||
Log.w(TAG, "WS ticket refresh failed: ${e.message}")
|
Log.w(TAG, "WS ticket refresh failed: ${e.javaClass.simpleName}")
|
||||||
_connectionStatus.value = ConnectionStatus.AUTH_EXPIRED
|
_connectionStatus.value = ConnectionStatus.AUTH_EXPIRED
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
@@ -539,7 +532,8 @@ object HermesWsClient {
|
|||||||
code: Int,
|
code: Int,
|
||||||
reason: String,
|
reason: String,
|
||||||
) {
|
) {
|
||||||
Log.d(TAG, "WebSocket closing: $code $reason")
|
// Do NOT log [reason] — it may carry server-side context.
|
||||||
|
Log.d(TAG, "WebSocket closing: $code")
|
||||||
webSocket.close(code, reason)
|
webSocket.close(code, reason)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -548,7 +542,9 @@ object HermesWsClient {
|
|||||||
code: Int,
|
code: Int,
|
||||||
reason: String,
|
reason: String,
|
||||||
) {
|
) {
|
||||||
Log.i(TAG, "WebSocket closed: $code $reason")
|
// Do NOT log [reason] — it may carry server-side context. The
|
||||||
|
// reason is still inspected internally to detect auth failures.
|
||||||
|
Log.i(TAG, "WebSocket closed: $code")
|
||||||
connected.set(false)
|
connected.set(false)
|
||||||
stopHealthTracking()
|
stopHealthTracking()
|
||||||
if (code == 4001 || code == 4401 ||
|
if (code == 4001 || code == 4401 ||
|
||||||
@@ -567,7 +563,10 @@ object HermesWsClient {
|
|||||||
t: Throwable,
|
t: Throwable,
|
||||||
response: Response?,
|
response: Response?,
|
||||||
) {
|
) {
|
||||||
Log.e(TAG, "WebSocket failure: ${t.message}", t)
|
// Log the exception class only — [Throwable.message] can leak URLs
|
||||||
|
// or headers. The message is still inspected internally for auth
|
||||||
|
// detection.
|
||||||
|
Log.e(TAG, "WebSocket failure: ${t.javaClass.simpleName}", t)
|
||||||
connected.set(false)
|
connected.set(false)
|
||||||
stopHealthTracking()
|
stopHealthTracking()
|
||||||
val code = response?.code ?: 0
|
val code = response?.code ?: 0
|
||||||
|
|||||||
@@ -1,10 +1,12 @@
|
|||||||
package com.m57.hermescontrol.data.remote
|
package com.m57.hermescontrol.data.remote
|
||||||
|
|
||||||
import kotlinx.coroutines.test.runTest
|
import kotlinx.coroutines.test.runTest
|
||||||
|
import okhttp3.Cookie
|
||||||
import okhttp3.HttpUrl.Companion.toHttpUrl
|
import okhttp3.HttpUrl.Companion.toHttpUrl
|
||||||
import org.junit.After
|
import org.junit.After
|
||||||
import org.junit.Assert.assertEquals
|
import org.junit.Assert.assertEquals
|
||||||
import org.junit.Assert.assertNull
|
import org.junit.Assert.assertNull
|
||||||
|
import org.junit.Assert.assertTrue
|
||||||
import org.junit.Test
|
import org.junit.Test
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -28,11 +30,62 @@ class CookieManagerTest {
|
|||||||
return ServerEndpoint.fromLegacy(host, 9119)
|
return ServerEndpoint.fromLegacy(host, 9119)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private fun httpsEp(
|
||||||
|
host: String,
|
||||||
|
path: String = "/",
|
||||||
|
): ServerEndpoint =
|
||||||
|
ServerEndpoint.parse(
|
||||||
|
"https://$host:9119$path",
|
||||||
|
CleartextPolicy.ALLOW_WITH_WARNING,
|
||||||
|
)
|
||||||
|
|
||||||
@After
|
@After
|
||||||
fun tearDown() {
|
fun tearDown() {
|
||||||
CookieManager.resetForTest()
|
CookieManager.resetForTest()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun getSessionCookie_recognizesHostPrefixedCookie() =
|
||||||
|
runTest {
|
||||||
|
val jar = buildFakePersistentCookieJar()
|
||||||
|
jar.useStore(PersistentCookieJar.DEFAULT_SERVER_ID)
|
||||||
|
CookieManager.setJarForTest(jar)
|
||||||
|
// Server stored a __Host- prefixed cookie (HTTPS deployment).
|
||||||
|
jar.saveFromResponse(
|
||||||
|
"https://dash.local/".toHttpUrl(),
|
||||||
|
listOf(
|
||||||
|
Cookie
|
||||||
|
.Builder()
|
||||||
|
.name("__Host-hermes_session_at")
|
||||||
|
.value("prefixed-value")
|
||||||
|
.expiresAt(System.currentTimeMillis() + 10L * 365 * 24 * 60 * 60 * 1000)
|
||||||
|
.hostOnlyDomain("dash.local")
|
||||||
|
.path("/")
|
||||||
|
.secure()
|
||||||
|
.httpOnly()
|
||||||
|
.build(),
|
||||||
|
),
|
||||||
|
)
|
||||||
|
|
||||||
|
assertEquals("prefixed-value", CookieManager.getSessionCookie())
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun setSessionCookie_httpsMarksSecureAndHonorsPath() =
|
||||||
|
runTest {
|
||||||
|
val jar = fakeJar()
|
||||||
|
// Proxy-prefixed HTTPS endpoint.
|
||||||
|
CookieManager.setSessionCookie("sess-secure", httpsEp("dash.local", "/hermes/"))
|
||||||
|
|
||||||
|
val cookies =
|
||||||
|
jar.loadForRequest(
|
||||||
|
"https://dash.local/hermes/api/status".toHttpUrl(),
|
||||||
|
)
|
||||||
|
assertEquals(1, cookies.size)
|
||||||
|
assertTrue("HTTPS session cookie must be Secure", cookies[0].secure)
|
||||||
|
assertEquals("/hermes/", cookies[0].path)
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun setThenGetSessionCookie_roundTrips() {
|
fun setThenGetSessionCookie_roundTrips() {
|
||||||
fakeJar()
|
fakeJar()
|
||||||
|
|||||||
@@ -179,4 +179,60 @@ class PersistentCookieJarTest {
|
|||||||
assertTrue(jar.loadForRequest("http://empty.local/".toHttpUrl()).isEmpty())
|
assertTrue(jar.loadForRequest("http://empty.local/".toHttpUrl()).isEmpty())
|
||||||
assertNull(jar.getSessionCookieValue())
|
assertNull(jar.getSessionCookieValue())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun getSessionCookieValue_findsPrefixedHostCookie() =
|
||||||
|
runTest {
|
||||||
|
val jar = makeJar()
|
||||||
|
jar.useStore("prefixed-scope")
|
||||||
|
val url = "https://dash.local/".toHttpUrl()
|
||||||
|
val prefixed =
|
||||||
|
Cookie
|
||||||
|
.Builder()
|
||||||
|
.name("__Host-hermes_session_at")
|
||||||
|
.value("host-prefixed-value")
|
||||||
|
.expiresAt(System.currentTimeMillis() + 10L * 365 * 24 * 60 * 60 * 1000)
|
||||||
|
.hostOnlyDomain("dash.local")
|
||||||
|
.path("/")
|
||||||
|
.secure()
|
||||||
|
.httpOnly()
|
||||||
|
.build()
|
||||||
|
jar.saveFromResponse(url, listOf(prefixed))
|
||||||
|
|
||||||
|
assertEquals("host-prefixed-value", jar.getSessionCookieValue())
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun getSessionCookieValue_prefersStrictestVariant() =
|
||||||
|
runTest {
|
||||||
|
val jar = makeJar()
|
||||||
|
jar.useStore("multi-scope")
|
||||||
|
val url = "https://dash.local/".toHttpUrl()
|
||||||
|
// Both a bare and a __Secure- prefixed cookie present; the strictest
|
||||||
|
// (__Host- / __Secure- first in SESSION_COOKIE_NAMES) must win.
|
||||||
|
val bare =
|
||||||
|
Cookie
|
||||||
|
.Builder()
|
||||||
|
.name(SESSION_COOKIE_NAME)
|
||||||
|
.value("bare-value")
|
||||||
|
.expiresAt(System.currentTimeMillis() + 10L * 365 * 24 * 60 * 60 * 1000)
|
||||||
|
.hostOnlyDomain("dash.local")
|
||||||
|
.path("/")
|
||||||
|
.httpOnly()
|
||||||
|
.build()
|
||||||
|
val secure =
|
||||||
|
Cookie
|
||||||
|
.Builder()
|
||||||
|
.name("__Secure-hermes_session_at")
|
||||||
|
.value("secure-value")
|
||||||
|
.expiresAt(System.currentTimeMillis() + 10L * 365 * 24 * 60 * 60 * 1000)
|
||||||
|
.hostOnlyDomain("dash.local")
|
||||||
|
.path("/")
|
||||||
|
.secure()
|
||||||
|
.httpOnly()
|
||||||
|
.build()
|
||||||
|
jar.saveFromResponse(url, listOf(bare, secure))
|
||||||
|
|
||||||
|
assertEquals("secure-value", jar.getSessionCookieValue())
|
||||||
|
}
|
||||||
}
|
}
|
||||||
@@ -2,9 +2,12 @@ package com.m57.hermescontrol.data.ws
|
|||||||
|
|
||||||
import android.util.Log
|
import android.util.Log
|
||||||
import com.m57.hermescontrol.data.local.AuthManager
|
import com.m57.hermescontrol.data.local.AuthManager
|
||||||
|
import com.m57.hermescontrol.data.remote.CleartextPolicy
|
||||||
import com.m57.hermescontrol.data.remote.CookieManager
|
import com.m57.hermescontrol.data.remote.CookieManager
|
||||||
|
import com.m57.hermescontrol.data.remote.ServerEndpoint
|
||||||
import com.m57.hermescontrol.data.remote.buildFakePersistentCookieJar
|
import com.m57.hermescontrol.data.remote.buildFakePersistentCookieJar
|
||||||
import io.mockk.every
|
import io.mockk.every
|
||||||
|
import io.mockk.mockk
|
||||||
import io.mockk.mockkObject
|
import io.mockk.mockkObject
|
||||||
import io.mockk.mockkStatic
|
import io.mockk.mockkStatic
|
||||||
import io.mockk.unmockkAll
|
import io.mockk.unmockkAll
|
||||||
@@ -45,6 +48,13 @@ class HermesWsClientTest {
|
|||||||
every { AuthManager.wsUrl() } returns mockWebServer.url("/").toString().replace("http://", "ws://")
|
every { AuthManager.wsUrl() } returns mockWebServer.url("/").toString().replace("http://", "ws://")
|
||||||
every { AuthManager.isAutoReconnect() } returns false
|
every { AuthManager.isAutoReconnect() } returns false
|
||||||
every { AuthManager.getSessionCookie() } returns null
|
every { AuthManager.getSessionCookie() } returns null
|
||||||
|
// Non-gated by default (token mode) so the gated ticket path is exercised
|
||||||
|
// only by the explicit gated-mode test below.
|
||||||
|
every { AuthManager.serverStore } returns
|
||||||
|
mockk<com.m57.hermescontrol.data.config.ServerStore>().also {
|
||||||
|
every { it.getLatestState() } returns
|
||||||
|
com.m57.hermescontrol.data.config.ServerStoreState()
|
||||||
|
}
|
||||||
|
|
||||||
// Issue #470: clients are built through OkHttpProvider, which now
|
// Issue #470: clients are built through OkHttpProvider, which now
|
||||||
// resolves the shared CookieManager.cookieJar. Inject a fake jar so
|
// resolves the shared CookieManager.cookieJar. Inject a fake jar so
|
||||||
@@ -475,4 +485,65 @@ class HermesWsClientTest {
|
|||||||
assertEquals(ConnectionStatus.DISCONNECTED, HermesWsClient.connectionStatus.value)
|
assertEquals(ConnectionStatus.DISCONNECTED, HermesWsClient.connectionStatus.value)
|
||||||
assertFalse(HermesWsClient.isConnected)
|
assertFalse(HermesWsClient.isConnected)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ── Issue #635: gated-mode WS ticket fetch must not be blocked by a
|
||||||
|
// missing bare-name session cookie (HTTPS deployments prefix it with
|
||||||
|
// __Host- / __Secure-). ────────────────────────────────────────────────
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun testGatedMode_attemptsTicketFetchWithoutBareCookie() {
|
||||||
|
// Force gated mode (ws auth via ticket, not loopback token).
|
||||||
|
every { AuthManager.serverStore } returns
|
||||||
|
mockk<com.m57.hermescontrol.data.config.ServerStore>().also {
|
||||||
|
every { it.getLatestState() } returns
|
||||||
|
com.m57.hermescontrol.data.config.ServerStoreState(wsAuthParam = "ticket")
|
||||||
|
}
|
||||||
|
// No bare-name session cookie present (the prefixed one is server-side).
|
||||||
|
every { AuthManager.getSessionCookie() } returns null
|
||||||
|
// setToken is exercised by the ticket refresh; stub it (AuthManager is
|
||||||
|
// a mocked object, so unstubbed calls throw).
|
||||||
|
every { AuthManager.setToken(any()) } returns Unit
|
||||||
|
|
||||||
|
// Separate server for the ticket endpoint so its queue can't interleave
|
||||||
|
// with the WebSocket upgrade on the main mockWebServer.
|
||||||
|
val ticketServer = MockWebServer()
|
||||||
|
ticketServer.start()
|
||||||
|
every { AuthManager.endpointForBuild() } returns
|
||||||
|
ServerEndpoint.parse(
|
||||||
|
ticketServer.url("/").toString(),
|
||||||
|
CleartextPolicy.ALLOW_WITH_WARNING,
|
||||||
|
)
|
||||||
|
ticketServer.enqueue(
|
||||||
|
MockResponse()
|
||||||
|
.setResponseCode(200)
|
||||||
|
.setBody("""{"ticket":"refreshed-ticket"}"""),
|
||||||
|
)
|
||||||
|
|
||||||
|
val connectLatch = CountDownLatch(1)
|
||||||
|
mockWebServer.enqueue(
|
||||||
|
MockResponse().withWebSocketUpgrade(
|
||||||
|
object : WebSocketListener() {
|
||||||
|
override fun onOpen(
|
||||||
|
ws: WebSocket,
|
||||||
|
response: okhttp3.Response,
|
||||||
|
) {
|
||||||
|
connectLatch.countDown()
|
||||||
|
}
|
||||||
|
},
|
||||||
|
),
|
||||||
|
)
|
||||||
|
|
||||||
|
HermesWsClient.connect()
|
||||||
|
|
||||||
|
// Before the fix, a null bare cookie short-circuited to AUTH_EXPIRED and
|
||||||
|
// the ticket endpoint was NEVER called. After the fix it is attempted,
|
||||||
|
// so the connection reaches CONNECTED.
|
||||||
|
assertTrue(
|
||||||
|
"Gated WS ticket fetch should be attempted even without a bare cookie",
|
||||||
|
connectLatch.await(5, TimeUnit.SECONDS),
|
||||||
|
)
|
||||||
|
assertEquals(ConnectionStatus.CONNECTED, HermesWsClient.connectionStatus.value)
|
||||||
|
|
||||||
|
ticketServer.shutdown()
|
||||||
|
}
|
||||||
}
|
}
|
||||||
Reference in new issue
Block a user