Skip to content

Use HTTP POST for logout#7489

Merged
Alkarex merged 6 commits intoFreshRSS:edgefrom
Alkarex:post-logout
Apr 5, 2025
Merged

Use HTTP POST for logout#7489
Alkarex merged 6 commits intoFreshRSS:edgefrom
Alkarex:post-logout

Conversation

@Alkarex
Copy link
Member

@Alkarex Alkarex commented Apr 1, 2025

To avoid potential CSRF risks

To avoid potential CSRF risks
@Alkarex Alkarex added this to the 1.26.2 milestone Apr 1, 2025
@Alkarex Alkarex requested a review from math-GH April 1, 2025 22:13
@Alkarex
Copy link
Member Author

Alkarex commented Apr 1, 2025

The font size for Logout in that menu is smaller than the rest. I am not sure why. Help welcome
image

@Alkarex
Copy link
Member Author

Alkarex commented Apr 1, 2025

In this PR, the logout is now a POST instead of GET to address a security issue, but I have only tested with Web form and not OpenID Connect.

@aaronschif if you are around, help welcome to test with ODIC the logout function you did in #5351

Ping a few others for test, as you may have different setups: @Hani-K, @UncleArya, @jasonajack, @mpalatsi, @ShaddyDC, @pando85 ... Thanks

P.S. To test this branch, if using Docker, you can modify your Docker Compose like so:

  freshrss:
    image: freshrss/freshrss:7489
    build:
      context: https://github.com/Alkarex/FreshRSS.git#post-logout
      dockerfile: Docker/Dockerfile
    ...

@Alkarex Alkarex mentioned this pull request Apr 1, 2025
6 tasks
@UncleArya
Copy link
Contributor

In this PR, the logout is now a POST instead of GET to address a security issue, but I have only tested with Web form and not OpenID Connect.

@aaronschif if you are around, help welcome to test with ODIC the logout function you did in #5351

Ping a few others for test, as you may have different setups: @Hani-K, @UncleArya, @jasonajack, @mpalatsi, @ShaddyDC, @pando85 ... Thanks

P.S. To test this branch, if using Docker, you can modify your Docker Compose like so:

  freshrss:
    image: freshrss/freshrss:7489
    build:
      context: https://github.com/Alkarex/FreshRSS.git#post-logout
      dockerfile: Docker/Dockerfile
    ...

Just tested with the branch you mentioned. Logout worked as expected with my OIDC setup, no issues to report. Hope that helps.

@Alkarex
Copy link
Member Author

Alkarex commented Apr 2, 2025

Just tested with the branch you mentioned. Logout worked as expected with my OIDC setup, no issues to report. Hope that helps.

Thanks for the test @UncleArya . Do you know whether your logout endpoint has any protection against XSRF, for instance through a secret? This would be to avoid for instance an image coming from a feed, which address is the same than your OIDC logout

@Alkarex
Copy link
Member Author

Alkarex commented Apr 2, 2025

mod_auth_openidclogout documentation https://github.com/OpenIDC/mod_auth_openidc/wiki#9-how-do-i-logout-users

@Alkarex
Copy link
Member Author

Alkarex commented Apr 2, 2025

@UncleArya What happens when you click logout in your case? Are you immediately logged out, or do you have one more action to do?

@Inverle
Copy link
Member

Inverle commented Apr 2, 2025

@Alkarex

The font size for Logout in that menu is smaller than the rest. I am not sure why. Help welcome

try adding:

button {
	font-family: OpenSans, Cantarell, Helvetica, Arial, sans-serif;
}

the hover animation isn't present anymore either so this is needed too:

button:hover .icon {
	filter: brightness(1.5);
	transition: 0.1s linear;
}

(changing a:hover .icon to button:hover .icon)

@Frenzie
Copy link
Member

Frenzie commented Apr 2, 2025

Don't forget about :focus with buttons. You generally want to get that in there the same as :hover.

@Alkarex
Copy link
Member Author

Alkarex commented Apr 3, 2025

Edits to the PR welcome 😉

@math-GH math-GH mentioned this pull request Apr 3, 2025
@math-GH
Copy link
Contributor

math-GH commented Apr 3, 2025

The font size for Logout in that menu is smaller than the rest. I am not sure why. Help welcome image

I cannot reproduce it

Alkarex pushed a commit that referenced this pull request Apr 3, 2025
Frenzie added a commit to Frenzie/FreshRSS that referenced this pull request Apr 3, 2025
So you can see keyboard focus.

In reply to <FreshRSS#7489 (comment)>.
@Alkarex
Copy link
Member Author

Alkarex commented Apr 3, 2025

I cannot reproduce it

In the menu on the left-hand side, not the one on the right-hand side

Alkarex pushed a commit that referenced this pull request Apr 5, 2025
So you can see keyboard focus.

In reply to <#7489 (comment)>.
@Alkarex Alkarex merged commit d858053 into FreshRSS:edge Apr 5, 2025
1 check passed
@Alkarex Alkarex deleted the post-logout branch April 5, 2025 21:15
@Inverle
Copy link
Member

Inverle commented Apr 6, 2025

Is the logout link supposed to be like this?
Only the text is clickable and it's not like the other links

logout

@Alkarex
Copy link
Member Author

Alkarex commented Apr 26, 2025

@Inverle Could you please try again making sure to clear the browser cache? I cannot seem to reproduce the issue

@Inverle
Copy link
Member

Inverle commented Apr 27, 2025

@Alkarex

Yes, it's still happening.
There is also this on the Swage theme (black text in logout button):

swage

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Apr 27, 2025
@Alkarex Alkarex mentioned this pull request Apr 27, 2025
@Alkarex
Copy link
Member Author

Alkarex commented Apr 27, 2025

@Inverle Thanks. I was looking at another menu. Should be fixed by #7526
Tests welcome

Alkarex added a commit that referenced this pull request Apr 27, 2025
* Themes fix CSS .as-link
Add missing rules.
fix #7489 (comment)

* More fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants