diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b4c5b833f..b4ca697732 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #3795, Clarify `Accept: vnd.pgrst.object` error message - @steve-chavez - #3779, Always log the schema cache load time - @steve-chavez - #3706, Fix insert with `missing=default` uses default value of domain instead of column - @taimoorzaeem + - #3788, Fix jwt cache does not remove expired entries - @taimoorzaeem ### Changed diff --git a/src/PostgREST/Auth.hs b/src/PostgREST/Auth.hs index c8c3b720cb..bbef5f8228 100644 --- a/src/PostgREST/Auth.hs +++ b/src/PostgREST/Auth.hs @@ -181,10 +181,33 @@ getJWTFromCache appState token maxLifetime parseJwt utc = do authResult <- maybe parseJwt (pure . Right) checkCache case (authResult,checkCache) of - (Right res, Nothing) -> C.insert' (getJwtCache appState) (getTimeSpec res maxLifetime utc) token res + -- From comment: + -- https://github.com/PostgREST/postgrest/pull/3801#discussion_r1857987914 + -- + -- We purge expired cache entries on a cache miss + -- The reasoning is that: + -- + -- 1. We expect it to be rare (otherwise there is no point of the cache) + -- 2. It makes sure the cache is not growing (as inserting new entries + -- does garbage collection) + -- 3. Since this is time expiration based cache there is no real risk of + -- starvation - sooner or later we are going to have a cache miss. + + (Right res, Nothing) -> do -- cache miss + + let timeSpec = getTimeSpec res maxLifetime utc + + -- purge expired cache entries + C.purgeExpired jwtCache + + -- insert new cache entry + C.insert' jwtCache timeSpec token res + _ -> pure () return authResult + where + jwtCache = getJwtCache appState -- Used to extract JWT exp claim and add to JWT Cache getTimeSpec :: AuthResult -> Int -> UTCTime -> Maybe TimeSpec diff --git a/test/io/test_io.py b/test/io/test_io.py index e61e074cbc..7d06ffcaa6 100644 --- a/test/io/test_io.py +++ b/test/io/test_io.py @@ -1648,3 +1648,51 @@ def test_schema_cache_startup_load_with_in_db_config(defaultenv, metapostgrest): response = metapostgrest.session.post("/rpc/reset_db_schemas_config") assert response.text == "" assert response.status_code == 204 + + +def test_jwt_cache_purges_expired_entries(defaultenv): + "test expired cache entries are purged on cache miss" + + # The verification of actual cache size reduction is done manually, see https://github.com/PostgREST/postgrest/pull/3801#issuecomment-2620776041 + # This test is written for code coverage of purgeExpired function + + relativeSeconds = lambda sec: int( + (datetime.now(timezone.utc) + timedelta(seconds=sec)).timestamp() + ) + + headers = lambda sec: jwtauthheader( + {"role": "postgrest_test_author", "exp": relativeSeconds(sec)}, + SECRET, + ) + + env = { + **defaultenv, + "PGRST_JWT_CACHE_MAX_LIFETIME": "86400", + "PGRST_JWT_SECRET": SECRET, + "PGRST_DB_CONFIG": "false", + } + + with run(env=env) as postgrest: + + # Generate two unique JWT tokens + # The 1 second sleep is needed for it generate a unique token + hdrs1 = headers(5) + postgrest.session.get("/authors_only", headers=hdrs1) + + time.sleep(1) + + hdrs2 = headers(5) + postgrest.session.get("/authors_only", headers=hdrs2) + + # Wait 5 seconds for the tokens to expire + time.sleep(5) + + hdrs3 = headers(5) + + # Make another request which should cause a cache miss and so + # the purgeExpired function will be triggered. + # + # This should remove the 2 expired tokens but adds another to cache + response = postgrest.session.get("/authors_only", headers=hdrs3) + + assert response.status_code == 200