feat: Only pass state if it was set #2183
Conversation
|
|
|
Thank you for your contribution! Could you please provide a reference to the specification where this is defined? |
jaylinski
changed the title
@aeneasr It is specified here: https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout |
jaylinski
changed the title
|
Looking good, thank you! Could you maybe also add a test to prevent regressions here? |
Using `state` in the logout flow is optional, so `state` can be empty. In order to avoid an ugly `/post-logout-redirect-uri?state=` URI, the state should only be appended if it is not empty.
| subject: "logout-subject-7", | ||
| expectBody: "redirected to default servercustom", | ||
| expectRequestURI: "/custom", | ||
| }, |
There was a problem hiding this comment.
An alternative would be to remove the state value from expectBody and instead add the expectRequestURI to all test-cases.
|
|
||
| if tc.expectRequestURI != "" { | ||
| assert.EqualValues(t, tc.expectRequestURI, resp.Request.URL.RequestURI(), "%s\n%s", resp.Request.URL.String(), out) | ||
| } |
There was a problem hiding this comment.
The if could be removed if expectRequestURI would be added to all test-cases.
@aeneasr I added a test case. Please review. |
|
@aeneasr Thanks for this nice piece of software! 🙌 Greetings from Linz. |
Related issue
I didn't open an issue because I prefer to directly show the problem by submitting a PoC.
Proposed changes
Using
statein the logout flow is optional, sostatecan beempty. In order to avoid an ugly
/post-logout-redirect-uri?state=URI, the state should only be appended if it is not empty.
Relevant specification: https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout
Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further comments
If you are willing to accept this change, I'll add tests (and fix the typo in the commit).