fix(sdk): stop the legacy rewrap fallback from masking real KAS errors (DSPX-3397) - #989
Draft
dmihalcik-virtru wants to merge 1 commit into
Draft
fix(sdk): stop the legacy rewrap fallback from masking real KAS errors (DSPX-3397)#989dmihalcik-virtru wants to merge 1 commit into
dmihalcik-virtru wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dmihalcik-virtru
force-pushed
the
dspx-3397-3-access-errors
branch
from
August 10, 2026 17:47
2f79465 to
823cce3
Compare
dmihalcik-virtru
force-pushed
the
dspx-3397-2-der-hardening
branch
from
August 10, 2026 17:47
4f53ae5 to
5722f0b
Compare
|
X-Test Failure Report |
…s (DSPX-3397) fetchWrappedKey tries the Connect-RPC rewrap and falls back to the legacy REST rewrap for older platforms. Because the fallback ran on *every* RPC failure, a definitive answer from KAS -- 401 unauthenticated, 403 permission denied, 400 bad request -- was thrown away and replaced by the legacy endpoint's "404 Not Found" NetworkError on Connect-only platforms. Callers could not tell a bad token from an unreachable server. tryRpcThenLegacy now short-circuits on auth/validation errors and surfaces them as-is; only network faults and missing-endpoint errors still fall back. On a double failure it throws the RPC error (the meaningful one) and logs the legacy failure so the fallback path stays debuggable. Also: - fetchKasBasePubKey: a platform config missing its BaseKey is a ConfigurationError, not a NetworkError, and is re-thrown unwrapped. - Log error summaries, not raw error objects. A ConnectError carries response metadata (on the auth path, that includes DPoP nonces), which should not be dumped to the console. Drops a stray unconditional console.log from handleRpcRewrapError. Tests updated: the four rewrap failure cases in encrypt-decrypt.spec.ts now assert the specific error type they were always meant to. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
dspx-3397-3-access-errors
branch
from
August 11, 2026 17:54
823cce3 to
e74922c
Compare
dmihalcik-virtru
changed the base branch from
dspx-3397-2-der-hardening
to
dspx-3397-1-jws-signatures
August 11, 2026 20:35
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.
Stack 2/6, split out of #939. Base: #987.
What
fetchWrappedKeytries the Connect-RPC rewrap and falls back to the legacy REST rewrap for older platforms. The fallback ran on every RPC failure, so a definitive answer from KAS — 401 unauthenticated, 403 permission denied, 400 bad request — was discarded and replaced by the legacy endpoint's404 Not FoundNetworkErroron Connect-only platforms.The practical effect: a caller with a bad token, a caller lacking entitlement, and a caller pointed at an unreachable server all got the same
NetworkError: ... 404 Not Found.Changes
tryPromisesUntilFirstSuccess→tryRpcThenLegacy, which short-circuits on auth/validation errors (UnauthenticatedError,PermissionDeniedError,InvalidFileError) and surfaces them as-is. Only network faults and missing-endpoint errors still fall back.fetchKasBasePubKey: a platform config missing itsBaseKeyis aConfigurationError, not aNetworkError, and is re-thrown unwrapped instead of being masked behind a[PublicKey]network-error banner.errBrief, never raw error objects. AConnectErrorcarries response metadata — on the auth path that includes DPoP nonces — which should not be dumped to the console.errBriefwraps the existingextractRpcErrorMessageand adds the Connect code.console.log('Error is a ConnectError with code:', ...)fromhandleRpcRewrapError.Behavior change
This changes the error type callers see on rewrap failures. Four cases in
encrypt-decrypt.spec.tsare updated to assert the specific error they were always meant to:NetworkErrorUnauthenticatedErrorNetworkErrorPermissionDeniedErrorNetworkErrorServiceErrorNetworkError(404 Not Found)ServiceErrorWorth a look from anyone depending on catching
NetworkErrorbroadly.How to test
cd lib && npm test