fix: CRL disabled flow and signing error UX#7402
Merged
vitormattos merged 28 commits intomainfrom Apr 4, 2026
Merged
Conversation
When the admin disables external CRL validation (crl_external_validation_enabled=false), an empty CRL distribution-point list was still returning NO_URLS instead of DISABLED, causing signing to be blocked even though the admin intended to bypass validation. Move the toggle check before the empty-URL guard so that an empty list is treated the same as all points being intentionally skipped. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
…abled CrlValidationStatus::MISSING is assigned in AEngineHandler before the CRL checker runs (certificate has no CDP extension), so the crl_external_validation_enabled toggle was never consulted for this case. Check the toggle explicitly in validateCertificateRevocation so that certificates without any revocation information follow the same policy as all other non-revoked statuses when the admin disables external CRL validation. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Update the data-provider case that previously asserted NO_URLS regardless of the toggle setting, and add a dedicated test confirming that empty URL lists still return NO_URLS when external validation is enabled. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
…isabled Stub getAppConfig on the identifyService mock used by the CRL data-provider tests so the new toggle check path is covered. Add an isolated test that verifies signing is not blocked when the admin disables external CRL validation and the certificate has no CDP extension (MISSING status). Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
When a certificate has no crlDistributionPoints extension, the handler now maps the status to DISABLED when external CRL validation is turned off, instead of always setting MISSING. This makes the source status align with the configured policy before signature validation runs. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
After moving the missing-CDP policy decision to AEngineHandler, Password no longer needs to re-check app config for MISSING. Keep only status-based validation in validateCertificateRevocation. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Remove the test scenario that depended on Password reading app config for MISSING status. Policy derivation now happens in AEngineHandler, so Password tests focus on status handling only. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Refactor the non-retriable signing error spec to avoid ambiguous strict text matching and assert the current blocked-UI behavior explicitly (hidden sign CTA + retry button). Add a second scenario that returns success from the sign endpoint and verifies that the blocking warning is not shown when no non-retriable error is returned. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Keep PdfEditor visible for generic/signing errors and only hide it when the error was produced by PDF/envelope loading. Tag load-time errors with scope=pdfLoad and gate PdfEditor rendering using that scope. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Add unit tests proving PdfEditor is hidden only for pdfLoad-scoped errors and remains visible for non-pdfLoad errors. Update PdfEditor mock to expose a stable DOM marker for assertions. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
vitormattos
changed the title
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
- Add @return array{status: CrlValidationStatus, revoked_at?: string} docblocks - Applied to validate(), validateFromUrlsWithDetails(), downloadAndValidateWithDetails() - Applied to checkCertificateInCrlWithDetails() - Ensures Psalm understands status is always CrlValidationStatus enum, never string - Provides strong typing guarantee at the source (producer) level - Allows simplified, trusting consumption without defensive checks Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
- Remove normalizeRevocationStatus() helper method as no longer needed - Remove defensive mixed type handling and instanceof checks - Trust CrlRevocationChecker type declarations that status is always enum - logRevocationBlockedSigning() now accepts CrlValidationStatus directly - Remove test cases with serialized string data (legacy compatibility no longer needed) - Simplified code expresses contract: CRL validation status is always enum from source Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
- getRevocationErrorMessage() now accepts non-nullable CrlValidationStatus - Status is always present and typed as enum (never null) from CrlRevocationChecker - Reflects contract that certificate data always has validated CRL status Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
- Document context for i18n translators on each CRL status error message - Clarify technical meaning: inaccessible URLs, validation errors, missing CRL - Helps translators understand certificate revocation domain terminology Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Member
Author
|
/backport to stable33 |
Member
Author
|
/backport to stable32 |
There was a problem hiding this comment.
Pull request overview
This PR adjusts LibreSign’s CRL validation behavior to avoid blocking signing when external CRL checks are disabled, and updates the signing UI to treat non-retriable signing failures (HTTP 422) as a “blocked signing” state with an explicit retry path, while keeping the PDF visible for signing-time errors.
Changes:
- Backend: align CRL parsing/validation so missing/empty CRL distribution points don’t block signing when external validation is disabled, while preserving fail-closed behavior when enabled.
- Frontend: introduce “blocking sign error” UX for 422 failures (close password modal, disable sign CTA, show retry action), and only hide the PDF viewer for PDF-load-scoped errors.
- Tests: extend PHP unit coverage for CRL-disabled behavior, add frontend unit assertions for PDF visibility + blocked UX, and add a Playwright E2E flow for the non-retriable password error.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/php/Unit/Service/IdentifyMethod/PasswordTest.php | Updates certificate validation test cases for revised CRL status handling and logging needs. |
| tests/php/Unit/Service/Crl/CrlRevocationCheckerTest.php | Adjusts expectations for empty URL list when external validation is disabled; adds explicit “setting on” test. |
| lib/Service/IdentifyMethod/SignatureMethod/Password.php | Adds CRL-blocking logging and more specific translated error messages for fail-closed statuses. |
| lib/Service/Crl/CrlRevocationChecker.php | Treats empty URL list as DISABLED when external validation is off; tightens return type docs. |
| lib/Handler/CertificateEngine/AEngineHandler.php | Sets CRL status to DISABLED (instead of MISSING) when CRL DP info is absent and external validation is off. |
| src/views/SignPDF/SignPDF.vue | Scopes PDF-load errors so the PDF viewer hides only for PDF-loading failures. |
| src/views/SignPDF/_partials/Sign.vue | Adds blocked-signing UI for 422 errors, closes password modal on non-retriable failures, and surfaces errors in the main CTA area. |
| src/tests/views/SignPDF/SignPDF.spec.ts | Adds unit tests ensuring PDF visibility depends on error scope. |
| src/tests/views/SignPDF/Sign.spec.ts | Adds unit tests for closing password modal on 422 and for blocked CTA + retry action UX. |
| playwright/e2e/sign-password-non-retriable-error.spec.ts | Adds E2E test covering blocked-to-normal flow when toggling CRL external validation. |
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
github-project-automation
Bot
moved this from 0. Needs triage
to 4. to release
in Roadmap
This was referenced Apr 4, 2026
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
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.
Context
This PR fixes regressions around certificate revocation (CRL) behavior when external validation is disabled, and aligns signing error UX with the new backend behavior.
What changed
Validation