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

Restrict automatic CORS configuration to UrlBasedCorsConfigurationSource #15444

Open
wants to merge 1 commit into
base: 6.2.x
Choose a base branch
from

Conversation

baezzys
Copy link
Contributor

@baezzys baezzys commented Jul 21, 2024

Closes gh-15378

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 21, 2024
@baezzys baezzys force-pushed the fix/vary-header-issue-15378 branch from e6995e4 to 0f3c2c2 Compare July 23, 2024 01:27
@marcusdacoregio marcusdacoregio added in: web An issue in web modules (web, webmvc) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 24, 2024
@marcusdacoregio marcusdacoregio added this to the 6.2.6 milestone Jul 24, 2024
@marcusdacoregio
Copy link
Contributor

Hi @baezzys, thanks for the PR. Can you please target the 6.2.x branch in your PR and rebase your branch with it?

Map<String, CorsConfigurationSource> corsConfigurationSources = this.context
.getBeansOfType(CorsConfigurationSource.class);

boolean hasUrlBasedCorsConfigurationSource = corsConfigurationSources.values()
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you please adopt another strategy instead of using Stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have refactored the code to avoid using Streams. PTAL. Thanks.

@baezzys baezzys changed the base branch from main to 6.2.x July 24, 2024 15:20
@baezzys baezzys force-pushed the fix/vary-header-issue-15378 branch from 0f3c2c2 to cdda121 Compare July 24, 2024 15:24
@baezzys
Copy link
Contributor Author

baezzys commented Jul 24, 2024

Hi @marcusdacoregio, I have completed the rebase and retargeted the PR to the 6.2.x branch.

@baezzys baezzys force-pushed the fix/vary-header-issue-15378 branch from cdda121 to 66e4c96 Compare July 24, 2024 15:33
…onSource

- Update CORS configuration logic to automatically enable .cors() only if a UrlBasedCorsConfigurationSource bean is present.
- Modify applyCorsIfAvailable method to check for UrlBasedCorsConfigurationSource instances.
@baezzys baezzys force-pushed the fix/vary-header-issue-15378 branch from 66e4c96 to 1d41596 Compare July 24, 2024 15:39
Copy link
Contributor

@marcusdacoregio marcusdacoregio left a comment

Choose a reason for hiding this comment

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

Thanks @baezzys, I've left some feedback inline.


for (CorsConfigurationSource source : corsConfigurationSources.values()) {
if (source instanceof UrlBasedCorsConfigurationSource) {
http.cors(withDefaults());
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should check if the instance is UrlBasedCorsConfigurationSource and if the bean name is corsConfigurationSource, since this is the bean name used by the CorsConfigurer, to avoid picking up the wrong CorsConfigurationSource.


this.mockMvc.perform(formLogin()).andExpect(header().doesNotExist("Access-Control-Allow-Origin"));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a test that verifies if the header Vary is not present? In summary, simulate the problem reported in #15378 and assert that it is fixed.

You can add the issue number in the test, like so:

// gh-15378
@Test
void ...() {
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: bug A general bug
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Response header Vary since Spring Security 6.2.0
3 participants