fix: eliminate the shard-8 sweep-test 500s on feature/ghcr-packages-integration - #95
Open
hongwei1 wants to merge 9 commits into
Open
fix: eliminate the shard-8 sweep-test 500s on feature/ghcr-packages-integration#95hongwei1 wants to merge 9 commits into
hongwei1 wants to merge 9 commits into
Conversation
Views.views.vend.systemView/customView both return Empty for a nonexistent view id, and openOrThrowException throws a raw NullPointerException that escapes as a 500 instead of the intended 4xx. Route it through unboxFullOrFail so a missing view answers 400 with ViewNotFound, matching every other lookup failure in this handler.
secretLink.toLong ran directly against the path segment with no format check, so a non-numeric secret_link threw an unhandled NumberFormatException that surfaced as a 500. Validate it first and fail with the existing InvalidNumber error instead.
getMyApiCollectionEndpoint and getApiCollectionEndpoints both call userAuthenticationMessage(false) in their description while also listing AuthenticatedUserIsRequired in errorResponseBodies and requiring a user in the handler. ResourceDoc's constructor treats a description containing the "optional" wording as authoritative when roles are empty, so it silently strips AuthenticatedUserIsRequired from the error list and the endpoint gets classified as public. Runtime auth enforcement (withUser) was never affected -- this only fixes the doc's self-reported classification, which is what tooling built on ResourceDoc (including the endpoint sweep) reads. Both sibling endpoints (getMyApiCollectionEndpoints, getMyApiCollectionEndpointsById) already use the correct true value.
getSignalChannelInfo threw a bare RuntimeException when the channel had no entry in Redis, which escaped as a 500. Fail through booleanToFuture with the new SignalChannelNotFound error instead so a missing channel answers 404.
…y enforce it getConnectorTraces and getConfigProps call withUser but never listed AuthenticatedUserIsRequired in their ResourceDoc, so the doc read as public while the handler actually demanded a user. getAllApiProductsV600 and getAllProductsV600 have the same gap by a different route: their description still uses userAuthenticationMessage(!getApiProductsIsPublic / !getProductsIsPublic), a conditional Lift carried over from APIMethods600.scala, but the http4s handlers always call withUser and never branch on the prop -- the same simplification already noted for the rest of the api-products bucket in this file. Fixed the description and error list to match what the handler does, and documented the drift from the Lift source-of-truth at the call site. None of this changes runtime behaviour; it only corrects the doc's self-reported classification, which the endpoint sweep (and any other tooling built on ResourceDoc) relies on.
createConsumerDynamicRegistration passed pem.getOrElse("") straight
into JwtUtil.verifyJwt, which throws JOSEException ("No PEM-encoded
keys found") when there is no key material to parse, escaping as a
500. Lift's commented-out original has the same gap. Check the
header is present first and fail with the existing X509GeneralError
(400) instead.
callableMethods is declared directly on the Connector trait with a real default body, so ConnectorProxy.isInheritedMember (which checks declaringClass != classOf[Connector]) does not recognise it as a non-routable member. The interceptor was therefore treating it as an ordinary connector call: looking up a MethodRouting entry for "callableMethods" and forwarding to method.invoke with a null args array (ByteBuddy passes null, not empty, for a no-arg method), which NPEs inside the zip used to build the routing lookup key. InternalConnector already special-cases this exact method for the same reason; give StarConnector the same treatment and answer it from the empty stub connector instead of routing it.
AuthSweepTest's public-endpoint check treated any anonymous 401 as a violation, but OBP-20200 (application/consumer authentication) is a legitimate way to refuse an anonymous caller distinct from OBP-20001 (user authentication) -- createConsentRequest, getConsentRequest and createVRPConsentRequest all authenticate the calling TPP via Client Credentials rather than a logged-in user, exactly as their own docs say. Treat an ApplicationNotIdentified-prefixed 401 as the doc being right rather than the sweep. Also add a signed-off expectedAuthDeviation list for two endpoints whose behaviour is deliberate and already explained in their own source comments: verifyRequestSignResponse authenticates by JWS request signature (a third mechanism ResourceDoc's authMode has no way to declare) and answers 401 with a different message than plain user auth; createTransactionRequestFreeForm intentionally skips the upfront role check and lets the connector's checkAuthorisationToCreateTransactionRequest decide, answering 400 rather than 403.
CHANNEL_NAME is a placeholder to EndpointCatalog.isPlaceholder (it matches the _NAME suffix) but not to hasNoPathVariable's narrower regex, so getSignalChannelInfo lands in SuccessSweepTest's no-setup-required bucket even though its path names an entity nothing creates. A nonexistent channel correctly answers 404; add it to expectedNon2xx with the reason rather than widening the regex for one endpoint.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the shard 8 failures from this run on
feature/ghcr-packages-integration. The triggering commit (08d68f5f2, GitHub Packages publishing) is unrelated CI/build config; these are pre-existing application bugs the endpoint sweep tests (AuthSweepTest,FailureSweepTest,SuccessSweepTest) happened to catch.Each fix is its own commit with the reasoning in the message. In order:
createTransactionRequest(v4.0.0): a missing view id threw a rawNullPointerException(Lift'sopenOrThrowExceptionconvention) instead of a 4xx — routed throughunboxFullOrFail.getUserInvitation(v4.0.0): an unguardedsecretLink.toLongthrewNumberFormatExceptionon a non-numeric secret link — added a format check.getMyApiCollectionEndpoint/getApiCollectionEndpoints(v4.0.0): both declaredAuthenticatedUserIsRequiredin their error list but also embeddeduserAuthenticationMessage(false)in the description, which madeResourceDoc's own constructor silently strip that requirement back out — so the doc reported itself as public even though the handler (and the rest of the error list) disagreed. Runtime auth enforcement was never affected; this only corrects the doc's self-reported classification.getSignalChannelInfo(v6.0.0): a barethrow new RuntimeExceptionfor an unknown channel — now a proper 404.getConnectorTraces/getConfigProps/getAllApiProductsV600/getAllProductsV600(v6.0.0): same doc/handler mismatch as above — handlers callwithUserunconditionally but the docs didn't declare it (two of them still carried Lift's prop-conditional public-access wording, which the http4s handlers never implemented — documented as intentional drift at the call site, matching the pattern already used for the rest of the api-products bucket).createConsumerDynamicRegistration(v5.1.0): a missingPSD2-CERTheader reachedJwtUtil.verifyJwtunguarded, which throwsJOSEExceptionon empty PEM input — now rejected upfront with the existingX509GeneralError.StarConnector/getConnectorMethodNames(v6.0.0):callableMethodsis declared directly on theConnectortrait with a real default body, so the ByteBuddy proxy'sisInheritedMembercheck (introduced by the cglib→ByteBuddy swap for the Scala 2.13/JDK25 migration) didn't recognise it as non-routable — it was being treated as an ordinary connector call and NPEing on a null args array.InternalConnectoralready special-cases this exact method for the same reason; gaveStarConnectorthe same treatment.OBP-20200) 401s as distinct from user-auth 401s inAuthSweepTest, a signed-off deviation list for two endpoints whose behaviour is deliberate (verifyRequestSignResponse's JWS auth,createTransactionRequestFreeForm's 400-not-403), and documentinggetSignalChannelInfoas an expected non-2xx inSuccessSweepTest(itsCHANNEL_NAMEplaceholder isn't caught by that suite's narrower regex).Test plan
AuthSweepTest,FailureSweepTest,SuccessSweepTest,SweepCoverageTest— 56/56 passing (previously 44 failures)run_tests_parallel.sh, 4 shards) — 3579 tests, 0 failures, 0 errors