test(example): claim fragment and form_post conformance on linux/windows - #447
Open
ahmednfwela wants to merge 3 commits into
Open
test(example): claim fragment and form_post conformance on linux/windows#447ahmednfwela wants to merge 3 commits into
ahmednfwela wants to merge 3 commits into
Conversation
oidc_loopback_listener 1.1.0 and oidc_desktop 1.1.0 are released, so the two capability gates that scored five profiles as "cannot be delivered on this platform" are now stale. This flips them, which puts Hybrid, Implicit, and the three Form Post plans in front of the live suite on linux and windows for the first time. canReceiveFragmentResponse is deleted, not flipped: the listener's relay page recovers a fragment on every platform now, and a universally-true gate is dead code. isFragmentResponsePlan goes with it -- the skip block was its only production caller, and the classification it encoded (formpost overrides the hybrid/implicit fragment default) lives on, tested, in oidc_desktop's responseArrivesInFragment. canReceiveFormPost stays, with a platform parameter it previously lacked. The web page and the desktop loopback listener are both http(s), so the URI alone cannot tell "a listener that reads POST bodies" from "a static page whose script cannot see the request body". The old single-argument form was only answerable because the answer was false everywhere. The skip message for the remaining form_post exclusions now states the actual reason per case (custom scheme: not an HTTP endpoint; web: a page script cannot read a POST body) instead of citing the redirect scheme, which stopped being an explanation the moment an https page became one of the excluded transports. Example suite 77 -> 74 (four gate tests deleted, one capability test added), analyzer clean, and the flow-timeout budget test still holds. What no local test can prove is the live half: the browser executing the relay page against certification.openid.net, and the suite's form POST landing in the listener. That is exactly what the linux and windows CI jobs on this branch exist to show, and a red there is a finding, not a rollback trigger. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M6bj7Qhk5FXAkvy4HJbCER
Contributor
📝 WalkthroughWalkthroughThe conformance runner now maps platforms to OIDC application types and evaluates form-post support by platform and redirect transport. Fragment responses are no longer gated. The loopback listener processes POST responses without fragment relay handling. ChangesConformance transport handling
Loopback POST handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Contributor
🔥 Firebase Hosting previews
Channels expire 7 days after their last deploy. |
The first live run of the flipped gates produced 36 suite-side rejections on linux, one per fragment-mode module, before any browser was involved: [FAILURE] redirect_uri is one of the registered uris but uses http scheme which is not allowed when application_type is web and response type is not code The harness registered no application_type at all, and OIDC Registration 1.0 section 2 defaults an omitted value to `web` -- so the suite judged a desktop RP with a loopback redirect by web-client rules, whose implicit-grant clause this exact shape violates. The same section sanctions it verbatim for native clients: "Native Clients MUST only register redirect_uris using custom URI schemes or loopback URLs using the http scheme; loopback URLs use localhost or the IP loopback literals". Every non-web platform in this harness is a native client, so the fix is sending the metadata that was always true and simply never sent: applicationTypeForPlatform maps web -> web and everything else -> native, and prepareTestPlanRequest forwards it. Omitted still means absent -- the suite owns the default, and the harness does not invent one silently. This is the same defect the web job had, one layer over: web violated the web-client rules it was correctly subject to, and desktop was subjected to web-client rules it was never under. Both ends of the section 2 sentence have now been hit from both sides. macos/ios/android previously passed while implicitly defaulted to web only because the https check ignores non-http schemes; they now declare native, and their redirect shape is the one the native clause names. Their jobs on this PR double as the regression check for that. Example suite 74 -> 77. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M6bj7Qhk5FXAkvy4HJbCER
…onse Round 2 of the desktop conformance run answered one question and asked this one: the fragment relay works (Hybrid and Implicit pass via the relay on windows), Form Post Basic passes (POST capture works) -- but every form_post hybrid/implicit module died with "Couldn't resolve the response mode, make sure the key (state) exists in the Uri". The client had not explicitly requested response_mode=form_post, so oidc_desktop's responseArrivesInFragment saw `code id_token` and armed the relay. The listener then folded the POST body into the Uri correctly -- and the relay branch discarded it and served the relay page. The page's re-request carries location.search and location.hash; a request BODY has no representation there, so code, id_token and state all vanished, and response mode resolution failed on the missing state. A fragment can only ride a GET navigation. A form POST's response is in its body, already captured by the time the relay decision is made, so relaying a POST destroys the very response the relay exists to recover. The relay now fires for GET only; everything else completes immediately. Test written first and red: captureFragment on + a form POST must complete on the first request with the body's parameters intact. 19 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M6bj7Qhk5FXAkvy4HJbCER
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/oidc/example/test/conformance_plan_variant_test.dart (1)
186-227: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the native HTTPS branch.
canReceiveFormPostnow accepts HTTP(S) redirects for non-web platforms. The tests cover onlyhttp://localhostfor Linux and Windows. Add a native HTTPS assertion, or narrow the contract to HTTP loopback if HTTPS is not supported.Suggested regression assertion
expect( canReceiveFormPost(Uri.parse('http://localhost:22434'), 'windows'), isTrue, ); + expect( + canReceiveFormPost(Uri.parse('https://localhost:22434'), 'windows'), + isTrue, + );🤖 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 `@packages/oidc/example/test/conformance_plan_variant_test.dart` around lines 186 - 227, Extend the native-platform coverage in the form_post tests around canReceiveFormPost to assert the intended behavior for an HTTPS redirect, using a non-web platform such as Linux or Windows. If HTTPS is not supported, instead update canReceiveFormPost to restrict acceptance to HTTP loopback redirects and keep the tests aligned with that contract.
🤖 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 `@packages/oidc/example/test/conformance_plan_variant_test.dart`:
- Around line 186-227: Extend the native-platform coverage in the form_post
tests around canReceiveFormPost to assert the intended behavior for an HTTPS
redirect, using a non-web platform such as Linux or Windows. If HTTPS is not
supported, instead update canReceiveFormPost to restrict acceptance to HTTP
loopback redirects and keep the tests aligned with that contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2446d10-fbcf-4c29-9c77-35c472edfcaf
📒 Files selected for processing (5)
packages/oidc/example/integration_test/conformance/api.dartpackages/oidc/example/integration_test/shared_e2e.dartpackages/oidc/example/test/conformance_plan_variant_test.dartpackages/oidc_loopback_listener/lib/src/oidc_loopback_listener.dartpackages/oidc_loopback_listener/test/oidc_loopback_listener_test.dart
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.
What
Flips the last two desktop conformance gates, putting Hybrid RP, Implicit RP, and the three Form Post plans in front of the live suite on linux and windows for the first time. Five profiles that were scored "cannot be delivered on this platform" become claimable, because the claim stopped being true when
oidc_loopback_listener1.1.0 andoidc_desktop1.1.0 shipped (#445):application/x-www-form-urlencodedPOST body and folds it into the returnedUri— OAuth 2.0 Form Post Response Mode is deliverable to a loopback redirectlocation.hashpromoted to the query string), andoidc_desktopenables it exactly when the flow's response mode needs itThe two gates went different ways
canReceiveFragmentResponseis deleted, not flipped. Every platform can receive a fragment now, and a universally-true gate is dead code.isFragmentResponsePlangoes with it — the skip block was its only production caller, and the formpost-overrides-fragment classification it encoded lives on, tested, inoidc_desktop.responseArrivesInFragment.canReceiveFormPoststays, and gains aplatformparameter. The web page and the desktop listener are both http(s), so the URI alone cannot tell "a listener that reads POST bodies" from "a static page whose script cannot see the request body that delivered it". The old single-argument signature was only answerable because the answer used to be false everywhere. Custom schemes and web remain excluded, and the skip message now states the real reason per case instead of citing the redirect scheme.What CI on this branch is for
Local tests prove the predicates; they cannot prove the live half — a real browser executing the relay page against
certification.openid.net, and the suite's form POST landing in the listener. The linux and windows jobs here are that proof. A red on them is a finding about the relay or the listener, not a reason to re-gate: the gates were removed because the capability claim is released library behaviour, and if the live run contradicts it, the library (not the harness) is what needs fixing.Expected cost: hybrid adds 48 modules and implicit 27 per desktop job, plus the three form-post plans. The flow-timeout budget test still holds locally; if a desktop job hits its wall-clock limit, that is a legible red with an obvious remedy.
Verification
🤖 Generated with Claude Code
https://claude.ai/code/session_01M6bj7Qhk5FXAkvy4HJbCER
Summary by CodeRabbit
New Features
Bug Fixes
Tests