Skip to content

Commit

Permalink
[server] Fix handling of multiple session cookies with the same name (g…
Browse files Browse the repository at this point in the history 
…itpod-io#19456)

* [server] Fix handling of multiple session cookies with the same name

* [public-api-server] Fix handling of multiple session cookies with the same name

* Ensure exact same behavior as before in jwtSessionConvertor
  • Loading branch information
@geropl
geropl authored Feb 22, 2024
1 parent 5af55a7 commit 96df901
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 32 deletions.
25 changes: 19 additions & 6 deletions components/public-api-server/pkg/auth/middleware.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,20 @@ func (i *Interceptor) tokenFromHeaders(ctx context.Context, headers http.Header)
}

// Extract the JWT token from Cookies
cookie, err := cookieFromString(rawCookie, i.sessionCfg.Cookie.Name)
if err != nil {
cookies := cookiesFromString(rawCookie, i.sessionCfg.Cookie.Name)
if len(cookies) == 0 {
return Token{}, connect.NewError(connect.CodeUnauthenticated, fmt.Errorf("No cookie credentials present on request."))
}

_, err = VerifySessionJWT(cookie.Value, i.verifier, i.sessionCfg.Issuer)
if err != nil {
var cookie *http.Cookie
for _, c := range cookies {
_, err := VerifySessionJWT(c.Value, i.verifier, i.sessionCfg.Issuer)
if err == nil {
cookie = c
break
}
}
if cookie == nil {
return Token{}, connect.NewError(connect.CodeUnauthenticated, fmt.Errorf("JWT session could not be verified."))
}

Expand All @@ -98,12 +105,18 @@ func NewClientInterceptor(accessToken string) connect.Interceptor {
}
}

func cookieFromString(rawCookieHeader, name string) (*http.Cookie, error) {
func cookiesFromString(rawCookieHeader, name string) []*http.Cookie {
// To access the cookie as an http.Cookie, we sadly have to construct a request with the appropriate header such
// that we can then extract the cookie.
header := http.Header{}
header.Add("Cookie", rawCookieHeader)
req := http.Request{Header: header}

return req.Cookie(name)
var cookies []*http.Cookie
for _, c := range req.Cookies() {
if c.Name == name {
cookies = append(cookies, c)
}
}
return cookies
}
43 changes: 43 additions & 0 deletions components/server/src/session-handler.spec.db.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { v4 } from "uuid";
import { SessionHandler } from "./session-handler";
import { createTestContainer } from "./test/service-testing-container-module";
import { UserService } from "./user/user-service";
import { fail } from "assert";

describe("SessionHandler", () => {
let container: Container;
Expand Down Expand Up @@ -102,6 +103,48 @@ describe("SessionHandler", () => {
expect(user).to.be.undefined;
});
});

describe("verifyJWTCookie", () => {
it("should return undefined for an empty cookie", async () => {
const claims = await sessionHandler.verifyJWTCookie("");
expect(claims).to.be.undefined;
});
it("should return undefined for an invalid cookie", async () => {
const claims = await sessionHandler.verifyJWTCookie("invalid");
expect(claims).to.be.undefined;
});
it("should return claims for a valid JWT with correct 'sub' claim", async () => {
const cookie = await sessionHandler.createJWTSessionCookie(existingUser.id);
const claims = await sessionHandler.verifyJWTCookie(`${cookie.name}=${cookie.value}`);
expect(claims?.sub).to.be.equal(existingUser.id);
});
it("should return undefined for a valid JWT with incorrect 'sub' claim", async () => {
const unexisingUserId = v4();
const cookie = await sessionHandler.createJWTSessionCookie(unexisingUserId);
const claims = await sessionHandler.verifyJWTCookie(`${cookie.name}=${cookie.value}`);
expect(claims).to.not.be.undefined;
expect(claims?.sub).to.be.equal(unexisingUserId);
});
it("should return claims for the first valid JWT with correct 'sub' claim", async () => {
const validCookie = await sessionHandler.createJWTSessionCookie(existingUser.id);
const claims = await sessionHandler.verifyJWTCookie(
`${validCookie.name}=invalid_value_1; ${validCookie.name}=${validCookie.value}; ${validCookie.name}=invalid_value_2;`,
);
expect(claims?.sub).to.be.equal(existingUser.id);
});
it("should throw if there are only invalid JWTs", async () => {
const validCookie = await sessionHandler.createJWTSessionCookie(existingUser.id);
try {
await sessionHandler.verifyJWTCookie(
`${validCookie.name}=invalid_value_1; ${validCookie.name}=invalid_value_2;`,
);
fail("Expected an error to be thrown");
} catch (err) {
expect(err).to.not.be.undefined;
}
});
});

describe("createJWTSessionCookie", () => {
it("should create a valid JWT token with correct attributes and cookie options", async () => {
const maxAge = 7 * 24 * 60 * 60;
Expand Down
104 changes: 78 additions & 26 deletions components/server/src/session-handler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,19 @@ export class SessionHandler {
}

const cookies = parseCookieHeader(req.headers.cookie || "");
const jwtToken = cookies[getJWTCookieName(this.config)];
if (!jwtToken) {
const jwtTokens = cookies[getJWTCookieName(this.config)];

let decoded: JwtPayload | undefined = undefined;
try {
// will throw if the token is expired
decoded = await this.verifyFirstValidJwt(jwtTokens);
} catch (err) {
res.status(401);
res.send("JWT Session is invalid");
return;
}

if (!decoded) {
const cookie = await this.createJWTSessionCookie(user.id);

res.cookie(cookie.name, cookie.value, cookie.opts);
Expand All @@ -45,15 +56,12 @@ export class SessionHandler {
res.status(200);
res.send("New JWT cookie issued.");
} else {
try {
// will throw if the token is expired
const decoded = await this.authJWT.verify(jwtToken);
const issuedAtMs = (decoded.iat || 0) * 1000;
const now = new Date();

const issuedAtMs = (decoded.iat || 0) * 1000;
const now = new Date();

// Was the token issued more than threshold ago?
if (issuedAtMs + SessionHandler.JWT_REFRESH_THRESHOLD < now.getTime()) {
// Was the token issued more than threshold ago?
if (issuedAtMs + SessionHandler.JWT_REFRESH_THRESHOLD < now.getTime()) {
try {
// issue a new one, to refresh it
const cookie = await this.createJWTSessionCookie(user.id);
res.cookie(cookie.name, cookie.value, cookie.opts);
Expand All @@ -62,15 +70,15 @@ export class SessionHandler {
res.status(200);
res.send("Refreshed JWT cookie issued.");
return;
} catch (err) {
res.status(401);
res.send("JWT Session can't be signed");
return;
}

res.status(200);
res.send("User session already has a valid JWT session.");
} catch (err) {
res.status(401);
res.send("JWT Session is invalid");
return;
}

res.status(200);
res.send("User session already has a valid JWT session.");
}
};
}
Expand Down Expand Up @@ -124,16 +132,55 @@ export class SessionHandler {
}
}

/**
* Parses the given cookie string, and looks out for cookies with our session JWT cookie name (getJWTCookieName).
* It iterates over the found values, and returns the first valid session cookie it finds.
* Edge cases:
* - If there is no cookie, undefined is returned
* - If there is no valid cookie, throws the error of the first verification attempt.
* @param cookie
* @returns
*/
async verifyJWTCookie(cookie: string): Promise<JwtPayload | undefined> {
const cookies = parseCookieHeader(cookie);
const jwtToken = cookies[getJWTCookieName(this.config)];
if (!jwtToken) {
const cookieValues = cookies[getJWTCookieName(this.config)];

return this.verifyFirstValidJwt(cookieValues);
}

/**
* Returns the first valid session token it finds.
* Edge cases:
* - If there is no token, undefined is returned
* - If there is no valid token, throws the error of the first verification attempt.
* @param tokenCandidates to verify
* @returns
*/
private async verifyFirstValidJwt(tokenCandidates: string[] | undefined): Promise<JwtPayload | undefined> {
if (!tokenCandidates || tokenCandidates.length === 0) {
log.debug("No JWT session present on request");
return undefined;
}
const claims = await this.authJWT.verify(jwtToken);
log.debug("JWT Session token verified", { claims });
return claims;

let firstVerifyError: any;
for (const jwtToken of tokenCandidates) {
try {
const claims = await this.authJWT.verify(jwtToken);
log.debug("JWT Session token verified", { claims });
return claims;
} catch (err) {
if (!firstVerifyError) {
firstVerifyError = err;
}
log.debug("Found invalid JWT session token, skipping.", err);
}
}
if (firstVerifyError) {
throw firstVerifyError;
}

// Just here to please the compiler, this should never happen
return undefined;
}

public async createJWTSessionCookie(
Expand Down Expand Up @@ -178,13 +225,18 @@ function getJWTCookieDomain(config: Config): string {
return config.hostUrl.url.hostname;
}

function parseCookieHeader(cookie: string): { [key: string]: string } {
return cookie
function parseCookieHeader(c: string): { [key: string]: string[] } {
return c
.split("; ")
.map((keypair) => keypair.split("="))
.reduce<{ [key: string]: string }>((aggregator, vals) => {
.reduce<{ [key: string]: string[] }>((aggregator, vals) => {
const [key, value] = vals;
aggregator[key] = value;
let l = aggregator[key];
if (!l) {
l = [];
aggregator[key] = l;
}
l.push(value);
return aggregator;
}, {});
}

0 comments on commit 96df901

Please sign in to comment.