-
Notifications
You must be signed in to change notification settings - Fork 910
[PM-28157] Revert "Add string extension to prefix URIs with www" #6192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 7b491d3.
|
Claude finished @SaintPatrck's task —— View job Code Review CompleteOverall Assessment: APPROVE This is a clean, well-justified revert that correctly removes the www-prefixing functionality. The revert is necessary because the original implementation violated FIDO2 and Digital Asset Link specifications. Change ClassificationLow-Risk Change (Revert) This PR cleanly reverts PR #6183, which introduced automatic www-prefixing of relying party IDs during Digital Asset Link verification. The revert is well-justified: per FIDO2 and DAL specs, the RP ID must map directly to the host serving Revert Completeness✅ Clean Revert Verified
✅ No Remnants Left Behind
Architecture & SecurityCorrect Architectural Decision The revert is architecturally sound and security-positive:
Code QualityThe revert is executed properly:
Test CoverageAppropriate test cleanup:
RecommendationAPPROVE - This revert should be merged. The revert correctly addresses a specification violation in the original implementation. The original PR attempted to improve compatibility with www subdomains, but did so in a way that violated FIDO2/DAL specifications by modifying the RP ID before validation. The proper solution (for future consideration) would be for relying parties to:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
vvolkgang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏾
One of the QR related tests failed, triggered a merge from main as it may be related to have been fixed by @david-livefront's camera PR.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6192 +/- ##
==========================================
- Coverage 85.41% 85.41% -0.01%
==========================================
Files 755 755
Lines 54114 54105 -9
Branches 7803 7798 -5
==========================================
- Hits 46220 46212 -8
Misses 5180 5180
+ Partials 2714 2713 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|

🎟️ Tracking
PM-28157
Reverts #6183
Relates to #6164
📔 Objective
Revert modification of RP ID during DAL verification. Per FIDO2 and Digital Asset Link specs, redirects are not allowed and the RP ID must map directly to the
assetlink.jsonhost.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes