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

#306 Send CORS headers so vscode can use the API #319

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

dustydecapod
Copy link
Contributor

@dustydecapod dustydecapod commented Aug 21, 2021

Explicitly instructs Spring to send CORS headers allowing any origin.

I haven't tested this, I'm not really a Java developer and am not set up to test this project. However I believe according to the Spring documentation that this will fix the issue at-hand.

Fixes #306

I went ahead and set up a test environment, and as noted by daiyam below my original fix was insufficient as Spring already defaulted to all origins. On further investigation I found that the extensionquery endpoint was not covered by the pre-existing CORS mappings as defined in WebConfig.java, due to existing outside of the /api/ endpoint root. I've added two lines to add this endpoint to the CORS mappings, which in my quick test solved the issue.

@jotoho
Copy link

jotoho commented Aug 21, 2021

#306 currently only links to the commit and not this PR. Hopefully my mentioning it in a comment will fix that, so it is easier for people to find this PR.

Edit: Seems to have worked.

@dustydecapod
Copy link
Contributor Author

Based on https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/cors/CorsConfiguration.html#applyPermitDefaultValues-- and https://howtodoinjava.com/spring-boot2/spring-cors-configuration/, * is set by default.

Yup you're right, the issue was instead that the endpoint was not covered by existing CORS mapping patterns. I updated my branch to fix this.

@dustydecapod
Copy link
Contributor Author

dustydecapod commented Aug 21, 2021

Looks like this is a combination issue of misconfigured servers as noted by @daiyam in #306, and what this code change fixes. While this fixes things in my test environment, other API endpoints that should be passing CORS headers in the production environment are not in-fact passing CORS headers.

@daiyam
Copy link

daiyam commented Aug 21, 2021

@berzoidberg Wow! Great find!
But, sadly, I don't think it will be enough because CORS aren't passed on /api urls:

(function() {
const xhr = new XMLHttpRequest();

xhr.open('POST', 'https://open-vsx.org/api/-/query', true);

xhr.setRequestHeader('Content-Type', 'application/json');
xhr.setRequestHeader('Accept', 'application/json');

xhr.onload = (e) => {
	console.log(xhr.response);
};

xhr.send('{"extensionName": "bar"}');
})();

from server/src/test/java/org/eclipse/openvsx/RegistryAPITest.java, testQueryExtensionName()

@@ -46,6 +46,8 @@ public void addCorsMappings(CorsRegistry registry) {
.allowedOrigins("*");
registry.addMapping("/api/**")
.allowedOrigins("*");
registry.addMapping("/vscode/gallery/extensionquery")
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the contribution! Shall we extend this to all endpoints in the vscode adapter? That would also cover the endpoints for fetching files.

Suggested change
registry.addMapping("/vscode/gallery/extensionquery")
registry.addMapping("/vscode/**")

Copy link
Contributor Author

@dustydecapod dustydecapod Aug 23, 2021

Choose a reason for hiding this comment

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

Probably a good idea. I didn't look to see what other endpoints might need covered, only addressed what was in the bug.

Upon closer inspection it looks like there are potentially other endpoints also not covered by the @CrossOrigin annotation. These are:

/vscode/item
/vscode/gallery/publishers/{namespace}/vsextensions/{extension}/{version}/vspackage

I've added a new commit covering your suggested change, as well as adding @CrossOrigin to the two other endpoints under /vscode/.

Please note that it seems there is also an infrastructure issue blocking CORS headers from being transmitted from the production deployment to clients, as noted by daiyam.

Copy link
Member

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

Thanks! What I don't understand though is why the WebConfig entry is needed although we already have @CrossOrigin annotations. Do you know more about that?

@spoenemann
Copy link
Member

Please note that it seems there is also an infrastructure issue blocking CORS headers from being transmitted from the production deployment to clients, as noted by daiyam.

The Eclipse Foundation is responsible for the infrastructure. Please create an issue at https://github.com/EclipseFdn/open-vsx.org (CC @mbarbero)

@spoenemann
Copy link
Member

@berzoidberg could you squash and sign-off your commits, and make sure you have signed the Eclipse Contributor Agreement (https://www.eclipse.org/legal/ECA.php)?

@dustydecapod
Copy link
Contributor Author

Thanks! What I don't understand though is why the WebConfig entry is needed although we already have @CrossOrigin annotations. Do you know more about that?

Unfortunately, I do not. The documentation indicates that @CrossOrigin /should/ work out of the box, however in my experimentation locally this proved not to be the case. My hunch is that using a combination of the "new" @CrossOrigin annotation and the "old" addCorsMapping method causes some funky behavior. I haven't looked much more deeply than basic experimentation. My hope is that perhaps someone with more Spring experience can chime in, as I'm not an experienced Java developer.

@crossorigin annotation too remaining vscode adapter endpoints.

Signed-off-by: Ber Zoidberg <ber.zoidberg@gmail.com>
@dustydecapod
Copy link
Contributor Author

@berzoidberg could you squash and sign-off your commits, and make sure you have signed the Eclipse Contributor Agreement (https://www.eclipse.org/legal/ECA.php)?

Done. Took me a second to figure out how to sign-off on a squash :P

@spoenemann spoenemann merged commit 02e07b4 into eclipse:master Aug 23, 2021
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.

error: CORS request did not succeed
4 participants