Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(status): Avoid session creation for status requests #49406

Merged
merged 1 commit into from
Nov 22, 2024
Merged

fix(status): Avoid session creation for status requests #49406

merged 1 commit into from
Nov 22, 2024

Conversation

solracsf
Copy link
Member

@solracsf solracsf commented Nov 20, 2024 •
edited
Loading

Checklist

@solracsf solracsf added the 3. to review Waiting for reviews label Nov 20, 2024
@solracsf solracsf added this to the Nextcloud 31 milestone Nov 20, 2024
@solracsf solracsf self-assigned this Nov 20, 2024
szaimen
szaimen reviewed Nov 20, 2024
View reviewed changes
@solracsf solracsf force-pushed the avoidStatusSessions branch from 59d4d1d to 6140788 Compare November 20, 2024 19:10
@solracsf solracsf requested a review from szaimen November 20, 2024 19:10
szaimen
szaimen reviewed Nov 20, 2024
View reviewed changes
@solracsf solracsf force-pushed the avoidStatusSessions branch from 6140788 to 7395bfd Compare November 21, 2024 06:27
szaimen
szaimen approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but didnt test

nickvergessen
nickvergessen requested changes Nov 21, 2024
View reviewed changes
@solracsf
fix(status): Avoid session creation for status requests
33d7019 
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@solracsf solracsf force-pushed the avoidStatusSessions branch from 85ccc8e to 33d7019 Compare November 21, 2024 14:00
nickvergessen
nickvergessen approved these changes Nov 21, 2024
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But since we have a valid request class in the place already, it should be fine

@nickvergessen nickvergessen merged commit c9bd003 into master Nov 22, 2024
187 checks passed
@nickvergessen nickvergessen deleted the avoidStatusSessions branch November 22, 2024 09:41
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Merged
@szaimen
Copy link
Contributor

szaimen commented Mar 1, 2025

Hey, it looks like this broke the security scanner. See https://help.nextcloud.com/t/update-nextcloud-to-31-0-0-now-scaner-showing-rating-a/218485/13. So revert?

@nickvergessen
Copy link
Member

Requests from scanner 1/month or 1/year
Requests from sync clients which don't need this 1.000.000 * x

We can add a force header to do the session or we scan a second endpoint.

@szaimen
Copy link
Contributor

szaimen commented Mar 3, 2025

Resolved by #51184

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

@nickvergessen nickvergessen nickvergessen approved these changes

@juliusknorr juliusknorr juliusknorr approved these changes

@szaimen szaimen szaimen approved these changes

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees

@solracsf solracsf

Labels
3. to review Waiting for reviews enhancement performance 🚀
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.

Don't open PHP session for status.php
4 participants
@solracsf @szaimen @nickvergessen @juliusknorr