Skip to content

AIO-interface: validate Sec-Fetch-* headers - #8786

Merged
szaimen merged 3 commits into
mainfrom
sec-fetch
Oct 14, 2026
Merged

szaimen merged 3 commits into
mainfrom
sec-fetch

Conversation

@Zoey2936

@Zoey2936 Zoey2936 commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

Rules:

  • all Sec-Fetch-Site "same-origin" and "none" request are allowed
  • all GET Requests with Sec-Fetch-Mode "navigate" and Sec-Fetch-Dest "document" to /, /login and /containers without query params are allowed and to /api/auth/getlogin with query params.
  • evertyhing else is blocked

Summary

  • The PR was tested and verified that it works locally
  • Or will be tested after merge on a dedicated test instance (available for maintainers)
  • Sign-off message is added to all commits
  • Tests (playwright if possible) are included
  • Screenshots before/after for front-end changes
  • Documentation has been updated or is not required
  • Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
  • Milestone next added

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@Zoey2936
Zoey2936 requested review from pabzm and szaimen and a balanced review from Copilot October 3, 2026 13:51

This comment was marked as spam.

@Zoey2936

Zoey2936 commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator Author

From #8600:

The question is: How sensitive are GET requiests to /api/auth/getlogin, /, /containers and /login? Since they would be allowed cross origin

@Zoey2936

Zoey2936 commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator Author

The code looks a bit unreadable

@szaimen szaimen added 2. developing Work in progress enhancement New feature or request labels Oct 5, 2026
@szaimen szaimen added this to the next milestone Oct 5, 2026
@szaimen

szaimen commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

@Zoey2936 thanks a lot! Can you please adjust the code a bit so that it gets more readable? Also please fix DCO. Thanks! :)

Signed-off-by: Zoey <zoey@z0ey.de>
@Zoey2936
Zoey2936 marked this pull request as ready for review October 5, 2026 16:44
@Zoey2936

Zoey2936 commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

done

@pabzm

pabzm commented Oct 8, 2026

Copy link
Copy Markdown
Member

Cross-posting here, because I'm not sure where this discussion shall continue best:

The question is: How sensitive are GET requiests to /api/auth/getlogin, /, /containers and /login? Since they would be allowed cross origin

With #8765 merged I think the redirect problem is reduced to allow cross-site-requests to /api/auth/getlogin and /login, which I would be fine with.

@pabzm pabzm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Besides disallowing /containers for cross-origin this looks good to me! (Didn't test, though)

Comment thread php/public/index.php Outdated
@pabzm

pabzm commented Oct 8, 2026

Copy link
Copy Markdown
Member

Hm, I'm not sure if we need to allow cross-origin requests to /. No external site should link to the UI, expect for the Nextcloud instance, which only uses /api/auth/getlogin and /login, as far as I can tell.

@Zoey2936 Do you have specific requests in mind for allowing that?

Co-authored-by: Pablo Zmdl <57864086+pabzm@users.noreply.github.com>
Signed-off-by: Zoey <zoey@z0ey.de>
@Zoey2936

Zoey2936 commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Yes, this line redirects to /:

return $response->withHeader('Location', '../..')->withStatus(302);
(in theory it could maybe also directly redirect to /login

@szaimen szaimen modified the milestones: v14.3.0, next Oct 8, 2026
@pabzm

pabzm commented Oct 14, 2026

Copy link
Copy Markdown
Member

Ah yes, I see, thank you!

@pabzm pabzm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@Zoey2936

Copy link
Copy Markdown
Collaborator Author

Yes, this line redirects to /:

return $response->withHeader('Location', '../..')->withStatus(302);

(in theory it could maybe also directly redirect to /login

But maybe this could also be changed, but I'm not sure

@pabzm

pabzm commented Oct 14, 2026

Copy link
Copy Markdown
Member

But maybe this could also be changed, but I'm not sure

I think so, too, but I'm not sure if maybe other click or code paths might lead to a redirect to /, too. So I'd prefer to keep it in, in order for users to always get to see the "login not allowed" explanation instead of the bland "Forbidden" page.

Comment thread php/public/index.php
Signed-off-by: Simon L. <szaimen@e.mail.de>
@szaimen szaimen changed the title validate Sec-Fetch-* headers AIO-interface: validate Sec-Fetch-* headers Oct 14, 2026

@szaimen szaimen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Thanks a lot! :)

@szaimen
szaimen merged commit ce5025f into main Oct 14, 2026
8 checks passed
@szaimen
szaimen deleted the sec-fetch branch October 14, 2026 11:45
@szaimen

szaimen commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

This is now released with v14.4.0 Beta. Testing and feedback is welcome! See https://github.com/nextcloud/all-in-one#how-to-switch-the-channel

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

Labels

3. to review Waiting for reviews enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants