Repository navigation
Fixes 36216: Check bot and personal access tokens against a per-server credential cache - #36217
Conversation
harshach
added
safe to test
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
✅ SSO Login Nightly — PASSED View workflow run · Commit
Totals: 94 passed · 0 failed · 0 flaky · 0 skipped · duration 18m 23s |
A subject that a deep read leaves out keeps its cache entry, marked unconfirmed at the read's stamp. No check is answered from that entry. Its next check reads the subject and answers by that read, or 503 while the subject does not convert, with the usual backoff. A matching fingerprint does not move an unconfirmed entry's stamp, and its record is offered to no decrypt reuse. Installs stay newer-wins by stamp. A read stamped before the deep read cannot replace the entry and a read stamped after it can, and the deep read leaves an entry that a later read installed as it is. A subject left out does not fail the deep pass. The retry after min(deepVerifyInterval, 1 min) is scheduled only when a batch's reconfirmation or deep read throws.
The two sweep-thread tests count only the threads named om-credential-verifier that appear after a snapshot taken before their verifier starts, so a verifier that another test in the JVM still runs does not count. shippedWithBlock replaces only the credentialVerification block's own lines, up to the next top-level key, so a key that follows the block in conf/openmetadata.yaml stays in the parsed config.
The sweepOnce and deepReadQuietly Javadocs, the deep-verify warning, the CredentialSource.read Javadoc and the decision record say that a subject the deep read leaves out is unconfirmed, that its checks read it again and answer 503 until a read converts it, and that the deep pass is retried after min(deepVerifyInterval, 1 min) only when a batch's reconfirmation or deep read throws.
Consequences now say that from the next deep pass on, a bot whose secret the secrets manager cannot decrypt answers 503 until a read decrypts it, and that its checks inside the backoff window count as cause=backoff.
…inute The warning window ran from each failure and lasted the 500 ms backoff, so a principal left out at every backoff window was logged with a stack trace on every read. The window now starts at the warning and lasts one minute, and a read that converts the principal still ends it.
The decision record now counts the sweep decrypts of a bot whose stored rows changed, says that a hanging decrypt holds a request-read thread, and gives the warning rate. The CredentialSource and CheckDecision Javadocs say that only the deep verify's read unconfirms a subject.
…xactly ReadBackoffTest covers a warning stamped before the open window that finishes later, and a window opened just before the ticker wraps. The decision record says that a decrypt that hangs during the deep read holds the sweep thread, and that the warning rate is per principal.
Four changes to the credential verifier, from review: - A check of a token that the principal's last full read lacks, while that read is younger than missRefreshMinAge, now answers 503 with Retry-After (cause=deferred) instead of 401. A token created on another server is then accepted on the retry. The check still answers 401 when the entry shows the token withdrawn: a read lacked it although a check presented it before that read began, or an earlier read held it and the read began after that one finished. A counter taken around each read's statement orders the reads, and an entry keeps at most 8 withdrawn digests. - Each sweep pass reconfirms every batch first, then continues the deep verify at 10 principals per read. It starts another read only while the pass has run less than maxStaleness minus twice sweepInterval, so slow decrypts delay the deep verify instead of the reconfirmations. - An interrupt while a check waits for its read answers 503 under cause=interrupted instead of counting as a read timeout. - JwtFilter.checkValidationsForToken runs the stored-credential check on the token it is given, from that token's own claims. The decision record states each rule. test_patchBotAuthMechanism_isIgnored now waits past a 503 for the 401.
Four changes from the review of 057db89: - An entry lists its withdrawn digests newest first, starting with the one its own read shows withdrawn. A check whose read finds more than 8 tokens gone then still answers 401 for the token it presented, not 503. - SocketAddressFilter.validatePrefixedTokenRequest verifies the signature with verifySignatureOnly and leaves the stored-credential check to checkValidationsForToken. A WebSocket handshake or an MCP HTTP request now counts one check in om.credential.checks instead of two. - A test covers a deep verify whose first read throws while the later passes succeed. It is retried a minute after it began. - The decision record and the sweep Javadoc include the reconfirm time in the deep-verify timing, and say when the next deep pass begins after a long one. The record also covers a withdrawn token that comes back when a deleted principal is restored.
The message of bd9aeb7 says an MCP HTTP request now counts one check. The transport does check each request once, but a tool or prompt call is checked again by its callback, through getCatalogSecurityContext, so it counts two checks instead of three. The decision record now says so. The record and the sweep Javadoc also count nudge drains on the sweep thread in the deep-verify timing, and name the deep budget rather than its default. The Javadoc now says when the next deep pass begins after a long one.
The decision record and the sweep Javadoc gave the age between two reconfirmations as a sum that left out the next pass's own drain. They now say why the deep budget is maxStaleness minus twice sweepInterval, which conditions keep entries within maxStaleness, and that checks of an entry past it read the database. A prompt call's callback checks the token in the prompt's Authorization argument, as it did before this branch. The record now says so instead of grouping prompt calls with tool calls. The record also says a deep read covers at most 10 principals, and that the next deep pass begins with the first sweep pass that starts once its interval has passed.
The decision record and the sweep Javadoc listed conditions that keep entries within maxStaleness, but one nudge drain still holding the sweep thread when the next pass is due could age an entry past it with every condition met. They now give an entry's age at its next reconfirmation as the sum of the work between the two, say what the deep budget alone keeps within maxStaleness, and say that the next check of an older entry reads it again. They also say that a sweep pass makes deep reads only while a deep pass has principals left. The sweepInterval validation message no longer says that the half-of-maxStaleness rule absorbs a missed sweep, which holds only while passes are short. Exactly half stays accepted.
The decision record said a slow decrypt delays only the deep pass. A sweep or nudge re-read that cannot reuse the last decrypt, as for a bot whose stored secret changed, also decrypts on the sweep thread, so a slow decrypt there delays reconfirmations too. The record now names all three places, and says the deep budget limits how many deep reads a pass starts, not how long one takes. The age breakdown in the record and the sweep Javadoc is an upper bound for an entry the pass reconfirmed. An entry the pass did not reconfirm, such as one in a failed batch, can be older. Both now say so.
From the review of ee62509: - A sweep or nudge re-read reuses a decrypt only from a confirmed entry whose read saw the same stored mechanism and updatedAt, so any change to a bot's user row decrypts again, not only a changed secret. - While the secrets manager cannot decrypt a bot's secret, the bot answers 503 from its next deep read, or sooner once its user row changes. - A failed batch leaves to age out only the entries it did not reconfirm. Its matched entries are reconfirmed before the re-read of the changed ones. - The age bound covers an entry that still holds the stamp its batch took in the pass. - A sweep pass starts sweepInterval after the previous one ends, not every sweepInterval. - The DEEP_CHUNK_SIZE Javadoc says how far past the budget a pass's deep reads can end. Only the decision record, Javadoc comments and a configuration comment change.
From the review of c26d72d: - While the secrets manager cannot decrypt a bot's secret, every read that must decrypt it fails, so any check of the bot that cannot be answered from memory answers 503. That includes a server's first check of a bot it holds no entry for. The record now says so instead of listing when the 503s start, a list that left that case out. - A sweep pass is due sweepInterval after the previous one ends. The first runs at once, and a nudge drain can delay one. - Decrypt reuse is keyed on the stored mechanism and updatedAt, not on the whole user row. - An entry is too old to answer from once its last read or reconfirmation is older than maxStaleness, whatever the gauge shows. - A deep read takes fewer subjects when batchSize is smaller. The reconfirmAll Javadoc, the failed-batch log message, the CredentialRecord Javadoc and the sweepInterval comment in openmetadata.yaml get the matching corrections.
From the review of 0ccf370: - The reconfirmAll Javadoc said a failing batch leaves the other batches' entries reconfirmed. Another batch can hold an entry whose re-read cannot convert it, so the Javadoc now says only that a failing batch does not stop the rest from reconfirming. - The decision record no longer says that writes to a bot user through the API change its updatedAt, since some writes keep it. The decrypt-reuse rule beside it already names the stored mechanism and updatedAt. - While the secrets manager cannot decrypt a bot's secret, a check that waits on a read that must decrypt, or on the backoff after one, answers 503. A check that joins a nudge read able to reuse the last decrypt can still get an answer, so the record no longer says that every check it cannot answer from memory answers 503. - A stale entry's checks need a read and answer 503 when the verifier cannot confirm within loadTimeout, in the record and in openmetadata.yaml. - The sweepInterval comment in openmetadata.yaml now says a pass is due that long after the previous one ends, which the message of 0ccf370 said it already did.
Two JwtFilterTest cases from main call the seven-argument test constructor, which this branch extends with the verifier supplier.
…erifier docs - The verifier's decision record says MCP access tokens are stored and that checkValidationsForToken looks them up, and that prompt callbacks, like tool callbacks, check the token in the request's Authorization header (#36531). - Amend the per-request cleanup record: the personal access token loader it names is gone, and the verifier reads outside the request entity cache. - OmCredential.NotStored and the checkValidationsForToken Javadoc name the MCP access token lookup and the rule that a provider token never signs in to a bot. - Every minted token gets its jti in one place; the MCP branch adds only client_id. - MintedTokensAreRecognizedTest mints the MCP access token with a client id, as issueAccessToken does, and describes the self-service case as a token minted before generateToken became bot-only. - McpServerUnconfirmedTokenTest sends the prompt caller's token in the header.
…allback repeats - The checkValidationsForToken Javadoc says the rule that a provider token never signs in to a bot account belongs to the legacy claim flow, where the code applies it. - The decision record says an MCP tool or prompt callback repeats the stored-credential check, and that classify reads no client_id, which is why an MCP access token is NotStored like a login token. - MintedTokensAreRecognizedTest names what it runs: the stored-credential check through validateJwtAndGetClaims, and reads back only what is kept for the verifier.
Since #36156, POST /users/generateToken mints tokens for bot users only, and main's test_generateToken_forOwnRegularAccount_forbidden covers that rule. UserResourceIT.test_selfGeneratedToken_lifecycle called it for a regular user, so it now gets 403, and MintedTokensAreRecognizedTest had a row for the same call. Both are removed. test_deleteBotUser_rejectsBotToken and test_deleteUser_rejectsPersonalAccessToken still cover account deletion.
#36143 authenticates WebSocket upgrades in the upgrade creator, which the servlet filter chain never reaches. A CredentialUnavailableException thrown there escaped the creator, and Jetty answered the upgrade with 500. The upgrade now answers 503 with Retry-After, as the servlet path and the REST and MCP entry points do, and stops the negotiation. authenticateUpgrade becomes package-private and static, so OpenMetadataApplicationPushFeedUpgradeTest can register it on an embedded Jetty server and open the upgrade with the JDK's WebSocket client. The authenticate Javadoc now lists CredentialUnavailableException.
The JDK's HttpClient.close() waits for an open handshake, so a server that accepted the connection and never answered would hold the test, and the build, until the server closed it. connectTimeout fails the handshake after 10 s instead, and the 20 s get bound stays above it. The per-server verifier's decision record names the test in its Guard.
OpenMetadataApplicationPushFeedUpgradeTest now also opens upgrades that authenticate rejects with a HandshakeRejectedException or an AuthenticationException, and checks the status the client receives. With the 503 case, each rejection that authenticateUpgrade answers is tested at the upgrade layer.
8608c29 to
bbc2a23
Compare
Code Review ✅ Approved 4 closed / 4 findings🔴 High risk · Reworking bot and personal-access-token authorization around cached database credentials could wrongly accept revoked tokens or reject valid requests during outages. Adds a per-server credential cache so bot JWTs and personal access tokens are checked against database-stored credentials, with sweep-based convergence across servers. Resolved the verifier decision-record gaps, the waitForPropagation REST contract documentation, the unrecorded PERSONAL_ACCESS provider-token rejection, and repeated deep-pass decrypts of an unconvertible bot principal. No open issues remain. ✅ 4 closed✅ Quality: Verifier limits and backoff defaults are not in the decision record
✅ Quality: waitForPropagation REST contract is not in the verifier decision record
✅ Quality: Rejecting provider tokens that claim PERSONAL_ACCESS is unrecorded
✅ Performance: One unconvertible principal makes every deep pass re-decrypt all bots each minute
VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
Describe your changes:
Fixes #36216
Bot JWTs and personal access tokens are now checked against a per-server, in-memory cache of the credentials the database stores for them. Each server confirms its cached principals against the database in sweep passes, each due 2 s after the previous one ends, with one indexed statement per 500 principals. A committed credential change therefore reaches every server within
credentialVerification.maxStaleness(5 s by default). That holds with or without Redis, and for every writer: the REST API,openmetadata-opsand direct SQL. A cached check makes no Redis or database round trip. When a server cannot confirm a token, for example during a database outage, the request gets503withRetry-After. Session and identity-provider tokens are unchanged.The commits build it from the bottom up:
user_tokensindex migration and the database credential source.CredentialVerifier, with its cache, single-flight reads, sweep and deep verify, and theCredentialsholder that repositories notify after commit.503mappings, and removal of the per-request token cache.jtion every minted token, and removal of the unused impersonation token generator.Type of change:
High-level design:
Cache.
CredentialVerifier, one per server, keeps the SHA-256 digests of the tokens the database stores for each principal. Each entry is stamped with the monotonic time its read was sent. A check is answered from memory while that stamp is withinmaxStaleness. Otherwise the check reads on a bounded pool (maxConcurrentRequestReads, 16) and waits at mostloadTimeout(2 s). Reads of one principal are single-flight. Caffeine bounds the cache atmaxSubjects(4,000) and drops a principal that presents no token foridleExpiry(15 min). A token that the principal's last full read lacks gets a new read once that read ismissRefreshMinAge(500 ms) old. Before then the check answers503withRetry-After, because the token may have been created on another server after the read. It answers401instead when the entry shows the token withdrawn: a read lacked it although a check presented it before that read began, or an earlier read held it and the read began after that one finished.Sweep. A sweep pass is due
sweepInterval(2 s) after the previous one ends. One fingerprint statement perbatchSize(500) cached principals confirms the batch, and only principals whose fingerprint moved are re-read. EverydeepVerifyInterval(10 min), the sweep re-reads every cached principal in full and forces each decrypt. That covers a secret edited inside an external secrets manager. Each pass drains nudges and reconfirms every batch first. While a deep verify has principals left, the pass then makes at least one of its reads, of at most 10 principals each, and starts another only while the pass is within its deep budget (maxStalenessminus twicesweepInterval, 1 s by default). The rest carries over to later passes, so the deep verify's decrypts delay the deep verify, not the next reconfirmations, while one of its reads takes less thansweepInterval. A principal whose stored credentials the deep read cannot convert, for example while the secrets manager cannot decrypt a bot's secret, is unconfirmed from then on. Its checks read it again and answer503until a read converts it, and the sweep does not confirm it by fingerprint. The new indexidx_user_tokens_user_type (userId, tokenType, token)serves both statements, and makes the fingerprint statement index-only on Postgres.Writes. Repositories call
Credentials.changedafter commit, so the writing server sees its own write on the next request. With Redis, the write is also published, and the other servers re-read at once. Correctness depends on neither, because the sweep sees every committed change, including changes made outside the server.Entry points. REST and the MCP tool and prompt callbacks go through
JwtFilter.authenticate. The WebSocket handshake and the MCP HTTP transport verify the signature withJwtFilter.verifySignatureOnlyand then callcheckValidationsForToken. Each path verifies the signature, classifies the token withOmCredential.classifyas stored, not stored or unrecognized, and callsrequirefor the stored kinds. The stored-credential check of an MCP tool or prompt call runs twice, once at the transport and once in its callback, both times on the token in the request'sAuthorizationheader.OmCredential.classifydoes not readclient_id, so an MCP access token is not stored, like a login token, andcheckValidationsForTokenlooks it up inoauth_access_tokens, the lookup that #36498 added. An impersonated request is checked against the presenting bot's own token. A token is rejected as unrecognized when OpenMetadata signed it with a claim combination the server never mints, or when another issuer signed it and it claimsisBot=trueortokenType=PERSONAL_ACCESS. Probes that grant no access, such as the startup bot-token check, call onlyverifySignatureOnly.Database unavailable. A check that cannot be confirmed answers
503withRetry-Afteron REST, on the WebSocket upgrade and on the MCP transport. On MCP callbacks it answers JSON-RPC error-32003. A principal whose check read produced no record is not read again for a check formissRefreshMinAge(500 ms), so its checks cost at most two reads per second per server. Liveness (/healthcheck) does not depend on the verifier. Alert onom_credential_sweep_last_complete_age_secondsinstead.Compatibility.
BotTokenCache.getToken,BotTokenCache.EMPTY_STRING,UserTokenCache.invalidateToken,JwtFilter.validateJwtAndGetClaimsandJwtFilter.checkValidationsForTokenkeep their signatures for other distributions.checkValidationsForTokennow also runs the stored-credential check on the token it is given. Collate's main branch uses nothing this PR removes.503when a server cannot read the database, and bot clients while the secrets manager cannot decrypt their secret. Clients that retry on 5xx need no change.Configuration. A new optional
credentialVerification:block inconf/openmetadata.yaml, withCREDENTIAL_VERIFICATION_*environment variables. Metrics areom_credential_*on/prometheus.Alternatives. A Redis read per request keeps a network round trip on every call and needs another path without Redis. A short local cache lifetime makes database reads grow with request volume instead of with the number of active principals.
Decision record. Stored tokens are checked by a per-server verifier.
Follow-ups, not in this PR.
AsyncLoadingCache.503from the hybrid ingestion runner's WebSocket handshake as temporary rather than as an authentication failure.verifySignatureOnlybeforecheckValidationsForTokeninBearerTokenUserResolver, so each request counts one stored-credential check.Tests:
Use cases covered
maxStaleness, with and without Redis. A bot token rotated through the first server is replaced on the second, and the PAT of a principal soft-deleted through the first is rejected by the second. A PAT created through one server is accepted by the other on a single request oncemissRefreshMinAgehas passed since the other last read its owner. Before then it gets503withRetry-After.openmetadata-ops regenerate-bot-tokensreach requests withinmaxStaleness.503withRetry-After, while session tokens keep working. Bot and PAT requests succeed again within a sweep interval of the database returning.401.Unit tests
bbc2a236b72, after the rebase onto main atb1941043ab4. The test planner (.github/scripts/plan_local_java_tests.py) selected 139openmetadata-serviceclasses and 9openmetadata-mcpclasses, and every selected class produced a report.openmetadata-service: 2,301 tests ran, with 0 failures. That run had no Docker, so the 26 tests ofOntologyEntityDAOTestandOntologyReleaseReviewListTesterrored and the 13 tests ofRedisCacheProviderClusterTestandRedisCacheProviderUnconfirmedWritesTestwere skipped. Once Docker was up, those four classes passed at the same commit: 39 tests, 0 failures, 0 skipped.openmetadata-mcp: all 122 tests in the 9 classes passed, includingMcpServerUnconfirmedTokenTest,OAuthHttpStatelessServerTransportProviderTestand main'sMcpServerRequestContextTest.promptAuthenticatesWithTheHeaderNotTheArguments.fba8434df5a'sOpenMetadataApplicationPushFeedUpgradeTeststarts an embedded Jetty server and opens the push-feed upgrade with the JDK's WebSocket client. It gets503withRetry-After, and500with the fix reverted.bbc2a236b72adds its401and403cases, which fail when the two answers are swapped.6abe365eab8were checked the same way: against the code before that commit, its tests fail in 8 places, at least one per fix. Of the three tests thatae3c7f88eb0adds, two fail against the code at6abe365eab8. The third covers the deep-verify retry, which was already right, and fails against two broken variants of that code.UserResource, which the integration tests exercise. Coverage of thecredentialspackage was not measured at the head commit.Backend integration tests
openmetadata-integration-tests/for the new and changed behavior.Integration tests at
bbc2a236b72, after the rebase onto main atb1941043ab4. No merge-queue workflow runs the search integration tests, so they ran locally once Docker was up, on the engines the planner chose.LiveSearchSettingsChangeITpassed on MySQL + Elasticsearch (1 test), and the 26 classes the planner selected passed on PostgreSQL + OpenSearch (64 tests, 0 skipped). The three integration-test workflows were started by hand on this branch. Each failed one test,NativeOntologyImportListRulesIT.aFilePreviewAcceptsAlsoAppliesWhenItsListsAreUsable, and passed every other test it ran:That test fails the same way on main at
b1941043ab4, the commit that adds it (#35991), in main's own MySQL + Elasticsearch and PostgreSQL + OpenSearch runs. In both runs, the change set it applies gets400with "Ontology validation failed: Declare or import the referenced property". This branch changes no ontology code.Before this rebase, the workflows ran at
4f52eaf0c82, after the first rebase onto main ate32170cdf3c. Each failed one test,UserResourceIT.test_selfGeneratedToken_lifecycle, and passed every other test it ran:The counts add up every lane of each workflow. The Redis workflow has no RDF lane.
That test called
POST /users/generateTokenfor a regular user, which main's #36156 restricted to bot users, so the call got403.a50dab2b113removes it, with the matching row ofMintedTokensAreRecognizedTest. Main'stest_generateToken_forOwnRegularAccount_forbiddencovers the new rule, andtest_deleteBotUser_rejectsBotTokenandtest_deleteUser_rejectsPersonalAccessTokenstill cover account deletion.Before the first rebase, all three workflows passed at
bd9aeb7e9ad(40833236407, 40833239431, 40833242095) and at5cbe4c551ad(40527205207, 40527211134, 40527215679). At057db895fbe, the Redis workflow (40829055078) failed one search-ranking test,SearchRankingRelevanceIT.singularQueryFindsThePluralTable, which passed in every other run. Those SHAs are the commits before the first rebase.Integration tests added or changed
CredentialRevocationTwoProcessIT: cross-server revocation, rotation and soft delete, with and without Redis.RevokeWaitForPropagationIT:waitForPropagationon both revoke endpoints.OpsRegenerateBotTokensIT: tokens rotated throughopenmetadata-ops.CredentialOutOfBandRevocationIT: SQL-only changes.CredentialTwoVerifierConvergenceIT: two verifiers over one database.CredentialScaleHelpersTest: the helpers the scale-lane benchmarks use. It runs in every lane.CredentialHotPathNoIoIT,CredentialRedisNoRoundTripIT,CredentialRedisCommandStatsIT: per-request cost.CredentialRedisDownIT: requests and revokes while Redis is down.CredentialDatabaseOutageIT:503withRetry-Afterduring a database outage.CredentialSourceDaoIT,CredentialIndexMigrationIT: the credential statements and the index migration.mcp/CredentialMcpRevocationIT: the MCP transport and tool calls.UserResourceIT: new token tests.security/scale/CredentialAuthBenchIT,CredentialSweepCostIT,CredentialMemoryIT,CredentialPatBurstIT: scale-lane benchmarks.TokenRevocationRedisMultiNodeITis deleted.CredentialRevocationTwoProcessITreplaces it.ForkedServerNodeand its helpers init/bootstraprun a second server in its own JVM.Performance
On one thread, the in-memory check takes 470 to 670 ns per request (3 JVM runs). That is about 2% of the RS256 signature verification that precedes it (28 µs median). Reproduce it with
mvn -pl openmetadata-service test -Dtest=CredentialCheckBenchmark -Dsurefire.failIfNoSpecifiedTests=false. The machine was shared and loaded during these runs. The cross-server propagation and sweep-cost benchmarks have not run yet.Ingestion integration tests
Not applicable. No ingestion changes.
Playwright (UI) tests
Not applicable. No UI changes.
Manual testing performed
None. Docker was unavailable, so no local stack ran.
UI screen recording:
Not applicable. Backend-only change.
Checklist:
I have read the CONTRIBUTING document.
My PR title is
Fixes <issue-number>: <short explanation>My PR is linked to a GitHub issue via
Fixes #<issue-number>above.I have commented on my code, particularly in hard-to-understand areas.
For JSON Schema changes: not applicable, no JSON Schema changes.
For a UI feature or feature task: not applicable, no UI changes.
I have added tests (unit / integration / Playwright as applicable) and listed them above.
Self-review: I reviewed the final diff against
REVIEW.md(thepr-checklistskill runs it) and fixed every 🔴 finding, or answered it on the line below.Commit SHAs in this paragraph and the next are the commits as rebased onto main at
b1941043ab4, exceptbd27ec8d9f2, a merge of main from before the first rebase. AREVIEW.mdpass over the whole diff ran atbd27ec8d9f2, in three parts: the verifier, the wiring and the integration tests. Its blocking findings were in the integration tests and are fixed in9b2d01c7b04tob1b28c0230f. Review comment r4384148610 is fixed in553a29ef3edto374f530f96c. Independent reviews of both fix ranges, up toc4f708e57f5, found no blocking issue, and no 🔴 was answered instead of fixed.374f530f96cadds the two tests and the wording that the last review asked for. Review comment 6449704660 is answered in6abe365eab8tob0ff181cbe1, and an independent review ran after each of those commits. The reviews ofae3c7f88eb0,f1c5781699c,4fd9099bbc3andc547a0712aefound 🔴 findings, each a statement in the decision record or a Javadoc that the code did not bear out, and the next commit fixes them. The other reviews found no 🔴. Each review's 🟡 findings are addressed in the next commit. The review ofb0ff181cbe1found no 🔴.The branch was first rebased onto main at
e32170cdf3c, keeping both sides where they overlap. A review of the conflict resolutions found two 🔴 findings, both statements that main's stored MCP access tokens (fix(mcp): tool error codes and access token revoke #36498) made untrue: the decision record named only bot tokens and personal access tokens as stored, andMintedTokensAreRecognizedTestminted the MCP access token without the client id the server now adds.97ee259a195fixes both, with the review's 🟡 findings. The review of97ee259a195found one 🔴, a sentence of thecheckValidationsForTokenJavadoc that claimed more than the code does, and1952921fb49fixes it, with that review's 🟡 findings. The review of1952921fb49found no 🔴. The integration tests at1952921fb49then failed one test that fix(security): batch of backend security-hardening fixes #36156 made obsolete, anda50dab2b113removes it. Earlier drafts of that commit rewrote the test instead. Their reviews found 🔴 findings, which removing the test resolves. The review ofa50dab2b113found no 🔴. Its two 🟡 findings, the bot row's Javadoc and a helper parameter inMintedTokensAreRecognizedTest, change no test and are not addressed. Main's fix(socket): validate credentials on push-feed WebSocket handshakes under all configurations #36143 then conflicted with the branch, and a merge of main resolved it first, keeping both sides. A review of that merge and of a first draft offba8434df5afound no 🔴, andfba8434df5aaddresses the 🟡 findings that still apply. The review offba8434df5afound no 🔴, andab0b1759392fixes its two 🟡 findings: the upgrade test had no handshake timeout, and the decision record's Guard did not name the test. The review ofab0b1759392found no 🔴, andbbc2a236b72adds the401and403upgrade cases that its 🟡 finding asked for. The review ofbbc2a236b72found no 🔴. Its 🟡 finding, that the rejection test checks one status, is not addressed. The branch was then rebased onto main atb1941043ab4, which replaces the merge and again keeps both sides where they overlap. The rebase changed no line of the PR's diff against main, so no review ran on it.Decision check: this PR adds
docs/decisions/2026-10-25-stored-tokens-are-checked-by-a-per-server-verifier.md, and amendsdocs/decisions/2026-10-23-every-jetty-request-clears-per-request-thread-state.md, whose personal access token loader this PR removes.I have added tests around the new logic.
The reviewed changes appear safe to merge; no new blocking issue was confirmed.Summary
Bot JWTs and personal access tokens now use a bounded credential cache on each server.
No new actionable issue was confirmed. Tests were inspected but not executed during this review.
Previous threads were rechecked:
harshachaccepted the propagation wait’s local configuration bound because a cluster-wide bound would require membership tracking. The scope is documented.harshachdismissed the legacy Redis rolling-upgrade concern because no released version used those keys; the obsolete code was removed.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Signed token] --> B[Classify token] B -->|Session or identity provider| C[Existing checks] B -->|Bot or personal token| D[Per-server credential cache] D -->|Fresh matching token| E[Continue request] D -->|Needs a read| F[Bounded database read] F -->|Current| E F -->|Not current| G[Reject token] F -->|Cannot confirm| H[Retry response] I[Background sweep] --> D J[Committed credential change] --> K[Local refresh and optional Redis nudge] K --> DReviews (6) · Last reviewed commit: "Merge main into the credential verifier ..." · Reviewed by Greptile