Skip to content

2541 cors plugin implementation #2552

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
Merged
amadio merged 1 commit into from
Sep 9, 2025
Merged

2541 cors plugin implementation #2552

amadio merged 1 commit into from
Sep 9, 2025

Conversation

@ccaffy
Copy link
Contributor

@ccaffy ccaffy commented Jun 24, 2025 •
edited
Loading

This is draft and not properly tested. Despite having unit tests, I plan to add system test as well!

Please have a look so I can get early feedback and correct things once I'm back.

Thanks,
Cheers,
Cedric

This is the config file:

  http.cors libXrdCors.so
  cors.origin https://test.cern.ch
# AND ONE CAN PUT EVERYTHING IN ONE LINE IF NEEDED
  cors.origin https://test2.cern.ch https://test.ch

This is the effect on a PUT request:

> PUT /tmp/fic.txt_dest? HTTP/1.1
> Host: xrootd.cern.ch:1096
> User-Agent: curl/7.76.1
> Accept: */*
> Repr-Digest: adler=:RXJyb3IK=:, sha256=:test:
> Origin: https://test.cern.ch
> Content-Length: 5242880
> Expect: 100-continue
> 

< HTTP/1.1 100 Continue
< Connection: Close
< Server: XrootD/v5.7.1-22-g82a3fc0e8
< Access-Control-Allow-Origin: https://test.cern.ch

@ccaffy ccaffy requested review from abh3 and amadio June 24, 2025 15:23
@ccaffy ccaffy force-pushed the 2541-cors-plugin branch from 56d9597 to e5b6dd7 Compare June 24, 2025 15:25
@amadio amadio linked an issue Jul 9, 2025 that may be closed by this pull request
[XrdHttp] Add CORS Origin header handling #2541
Closed
@ccaffy ccaffy force-pushed the 2541-cors-plugin branch 3 times, most recently from 26dbf43 to 6e9c8c9 Compare July 16, 2025 07:08
abh3
abh3 requested changes Jul 19, 2025
View reviewed changes
Copy link
Member

@abh3 abh3 left a comment

Choose a reason for hiding this comment

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

Some round I changes needed here. Not too bad at all.

@ccaffy ccaffy force-pushed the 2541-cors-plugin branch 4 times, most recently from d35b2a5 to 65886c8 Compare July 21, 2025 09:37
abh3
abh3 requested changes Jul 23, 2025
View reviewed changes
Copy link
Member

@abh3 abh3 left a comment

Choose a reason for hiding this comment

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

Thank you for renaming the files! There are still a couple of changes outstanding from the last review. I have noted them in this review.

@ccaffy ccaffy force-pushed the 2541-cors-plugin branch from 65886c8 to cc55039 Compare July 23, 2025 09:34
abh3
abh3 requested changes Jul 24, 2025
View reviewed changes
Copy link
Member

@abh3 abh3 left a comment

Choose a reason for hiding this comment

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

OK, so some more changes needed here. Nothing dramatic.

@ccaffy ccaffy force-pushed the 2541-cors-plugin branch from cc55039 to bfffa72 Compare July 24, 2025 06:49
abh3
abh3 approved these changes Jul 25, 2025
View reviewed changes
Copy link
Member

@abh3 abh3 left a comment

Choose a reason for hiding this comment

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

Looks OK, thanks!

chrisburr
chrisburr reviewed Jul 30, 2025
View reviewed changes
@ccaffy ccaffy force-pushed the 2541-cors-plugin branch 5 times, most recently from 264cb93 to b36e082 Compare August 13, 2025 10:06
amadio
amadio requested changes Aug 14, 2025
View reviewed changes
Copy link
Member

@amadio amadio left a comment

Choose a reason for hiding this comment

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

I made some comments/suggestions, nothing major.

@abh3 Please take a quick look at them and let us know if you agree/disagree or if you have other suggestions.

In any case, I think this is in general in good shape to go into XRootD 5.9 after some small adjustments.

@ccaffy ccaffy force-pushed the 2541-cors-plugin branch 4 times, most recently from 2671e5e to 834e013 Compare August 18, 2025 12:50
@amadio
Copy link
Member

amadio commented Sep 8, 2025

@ccaffy Can you please rebase on the current master branch? Thank you!

@ccaffy
Copy link
Contributor Author

ccaffy commented Sep 9, 2025

Yup, done! Do you want me to change the target branch of this PR @amadio?

@amadio
Copy link
Member

amadio commented Sep 9, 2025

Yes, better to target master, if you plan to have it in XRootD 5.9, as I already picked from devel the things that should go into that version.

@ccaffy
Copy link
Contributor Author

ccaffy commented Sep 9, 2025

OK I'll target master as I need this for 5.9 definitely

@ccaffy ccaffy changed the base branch from devel to master September 9, 2025 07:19
@ccaffy ccaffy force-pushed the 2541-cors-plugin branch 2 times, most recently from fa40eaa to f7f27f8 Compare September 9, 2025 07:23
@amadio amadio added this to the 5.9.0 milestone Sep 9, 2025
@amadio
Copy link
Member

amadio commented Sep 9, 2025

Could you please add a simple README.md with documentation for this plugin?

@ccaffy
[XrdCors] Add CORS plugin to XrdHttp
63b44cf 
This plugin currently supports the 'Access-Control-Allow-Origin'
functionality.

Fixes xrootd#2541
@ccaffy
Copy link
Contributor Author

ccaffy commented Sep 9, 2025

Yes done :) Tell me if that is enough :)

@amadio
Copy link
Member

amadio commented Sep 9, 2025

Yes done :) Tell me if that is enough :)

That's good, thanks! I guess this is ready to merge now, unless you're still working on it.

@amadio amadio merged commit 368aa92 into xrootd:master Sep 9, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amadio amadio amadio requested changes

@abh3 abh3 abh3 approved these changes

+1 more reviewer

@chrisburr chrisburr chrisburr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Release Planning
Status: Done

Milestone

5.9.0

Development

Successfully merging this pull request may close these issues.

[XrdHttp] Add CORS Origin header handling

4 participants

@ccaffy @amadio @abh3 @chrisburr