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.

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

SplitHTTP server: Add global CORS headers for browser dialer #3830

Merged
merged 7 commits into from
Sep 20, 2024

Conversation

PoneyClairDeLune
Copy link
Contributor

Spotted a potential oversight sifting through the browser dialer section of SplitHTTP. If there are complications regarding CORS, this PR should fix them.

@mmmray
Copy link
Collaborator

mmmray commented Sep 19, 2024

I typically add them with nginx or cloudflare rules and never connect to the core directly. However, I recognize that this should be supported. I think splithttp in particular has already too many response header settings (noSSEheader), is it time for a generic responseHeaders setting?

Or actually, the headers can be added unconditionally if the path is validated, so no option is needed.

I also see you only set them for the GET branch, I think it's better to have those headers in h.config.WriteResponseHeaders so they get set for POST as well.

@Fangliding
Copy link
Member

I think set it default should be enough, no need to add a new field

@PoneyClairDeLune
Copy link
Contributor Author

@mmmray @Fangliding What about merging both into noDefaultHeaders?

@PoneyClairDeLune PoneyClairDeLune changed the title Adds CORS headers allowing browser dialers to function SplitHTTP: Add CORS headers allowing browser dialers to function Sep 19, 2024
@mmmray
Copy link
Collaborator

mmmray commented Sep 19, 2024

the reason one would want to disable SSE header is not related to this, so it feels awkward to couple them. I am now convinced that it's best to send them unconditionally.

@PoneyClairDeLune
Copy link
Contributor Author

I'll remove the config option then.

@PoneyClairDeLune PoneyClairDeLune changed the title SplitHTTP: Add CORS headers allowing browser dialers to function SplitHTTP: Add global CORS headers for browser dialers Sep 19, 2024
@mmmray mmmray changed the title SplitHTTP: Add global CORS headers for browser dialers SplitHTTP server: Add global CORS headers for browser dialer Sep 20, 2024
@mmmray mmmray merged commit acbf36e into XTLS:main Sep 20, 2024
36 checks passed
@mmmray
Copy link
Collaborator

mmmray commented Sep 20, 2024

I think this will work. Thanks!

@RPRX
Copy link
Member

RPRX commented Sep 20, 2024

@mmmray 话说 xmux 好像支持不了 browser dialer,因为 chrome 自动控制复用且没提供控制它的 API?

@mmmray
Copy link
Collaborator

mmmray commented Sep 20, 2024

Yes, I think that's correct. I can't control h2mux behavior or even the HTTP version at all. I guess one could redesign browser dialer so that xray is the one launching and controlling chromium (and then, it can launch many processes or tweak some about:config to force un-muxing), or maybe un-muxing can be done by opening many incognito windows, but it all feels very strange.

@RPRX
Copy link
Member

RPRX commented Sep 20, 2024

@mmmray 写一下 #3823 (comment) ,这个月就它了

@mmmray
Copy link
Collaborator

mmmray commented Sep 20, 2024

I've seen the post, can't promise anything by end of month

@RPRX
Copy link
Member

RPRX commented Sep 20, 2024

I've seen the post, can't promise anything by end of month

写起来绝对很简单,而且正好周末

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants