Reverting printableCharacters SCEP validation - #49758
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/agentic_review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughCustom SCEP proxy challenge PrintableString validation was removed from certificate authority creation, updates, batch processing, and the frontend form. Backend validation retains existing empty-value and SCEP URL checks. Challenge-specific frontend error typing and rendering were removed, along with tests covering rejected characters and unchanged masked challenges. Release notes document the reverted validation and separate Windows enrollment fix. Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ee/server/service/certificate_authorities.go (1)
395-409: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the reverted challenge behavior.
Cover
_and@in create, update, and batch requests so the intentionally removed PrintableString restriction cannot be reintroduced.Also applies to: 793-795, 1476-1480
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/server/service/certificate_authorities.go` around lines 395 - 409, The validation tests for custom SCEP proxy challenges need regression coverage for underscores and at-signs. Extend the create, update, and batch request tests covering validateCustomSCEPProxy to submit challenge values containing “_” and “@” and assert they are accepted, preserving the removed PrintableString restriction behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ee/server/service/certificate_authorities.go`:
- Around line 395-409: The validation tests for custom SCEP proxy challenges
need regression coverage for underscores and at-signs. Extend the create,
update, and batch request tests covering validateCustomSCEPProxy to submit
challenge values containing “_” and “@” and assert they are accepted, preserving
the removed PrintableString restriction behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a811bb6-5dd8-44de-a3e3-d5b37e8ec0f9
📒 Files selected for processing (6)
changes/49756-revert-scep-challenge-printable-validationee/server/service/certificate_authorities.goee/server/service/certificate_authorities_test.gofrontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tests.tsxfrontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tsxfrontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/helpers.ts
💤 Files with no reviewable changes (3)
- frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tsx
- frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tests.tsx
- ee/server/service/certificate_authorities_test.go
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Reverts the printable-character (ASN.1 PrintableString) validation for Custom SCEP proxy challenges across the UI and backend, aligning Fleet behavior with pre-4.89.0 and unblocking Apple enrollments that use base64url-style challenge secrets (e.g., containing _).
Changes:
- Removed Custom SCEP challenge “printable characters” validation from the CustomSCEP form validation helper and UI field error display.
- Removed backend validation that rejected Custom SCEP challenges containing non-PrintableString characters (create/update + GitOps/batch path).
- Removed the associated frontend and backend tests that asserted the previous rejection behavior.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/helpers.ts | Drops printable-character validation; adjusts validation typings. |
| frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tsx | Removes challenge field error display tied to the removed validation. |
| frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tests.tsx | Removes UI tests that asserted challenge rejection for underscores / masked challenge behavior. |
| ee/server/service/certificate_authorities.go | Removes backend printable-character challenge validation for Custom SCEP proxy CAs. |
| ee/server/service/certificate_authorities_test.go | Removes backend tests that asserted challenge rejection for non-PrintableString characters. |
Files excluded by content exclusion policy (1)
- changes/49756-revert-scep-challenge-printable-validation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/49756-revert-scep-challenge-printable-validation
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #49756 # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit * **Bug Fixes** * Custom SCEP proxy challenges can again include characters such as underscores. * Apple device enrollment works again with these challenges. * Removed the overly strict printable-character validation from the Custom SCEP configuration form. * The Challenge field now only enforces the required-value rule and no longer shows printable-character validation errors. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 95b535a)
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #49756 # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit * **Bug Fixes** * Custom SCEP proxy challenges can again include characters such as underscores. * Apple device enrollment works again with these challenges. * Removed the overly strict printable-character validation from the Custom SCEP configuration form. * The Challenge field now only enforces the required-value rule and no longer shows printable-character validation errors. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 95b535a)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #49758 +/- ##
==========================================
+ Coverage 67.88% 67.90% +0.01%
==========================================
Files 3896 3896
Lines 248688 248808 +120
Branches 13224 13062 -162
==========================================
+ Hits 168820 168950 +130
+ Misses 64649 64645 -4
+ Partials 15219 15213 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Related issue: Resolves #49756
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
Summary by CodeRabbit