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.

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

Fix a bug where CORS headers are not injected when a ServerErrorHandler is set #5939

Merged
merged 6 commits into from
Nov 7, 2024
Merged

Fix a bug where CORS headers are not injected when a ServerErrorHandler is set #5939

merged 6 commits into from
Nov 7, 2024
+170 −79

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Oct 14, 2024 •
edited
Loading

Motivation:

ServerErrorHandler.renderStatus() is used in DefaultServerErrorHandler and may not be called in a custom ServerErrorHandler. Attempting to inject CORS headers via CorsServerErrorHandler.renderStatus() may not work.

Modifications:

  • Use ctx.mutateAdditionalResponseHeaders() to set CORS headers instead of mutating the response headers.
  • Store whether a CORS is set by CorsService to ctx.attr().
    • The value is used in CorsServerErrorHandler to set if missing

Result:

@ikhoon ikhoon added the defect label Oct 14, 2024
@ikhoon ikhoon added this to the 1.31.0 milestone Oct 14, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 14, 2024 •
edited
Loading

🔍 Build Scan® (commit: 8065402)

Job name Status Build Scan®
build-ubicloud-standard-8-jdk-8 ✅ https://ge.armeria.dev/s/fazkthe3ihwae
build-ubicloud-standard-8-jdk-21-snapshot-blockhound ✅ https://ge.armeria.dev/s/67szeu7b6bgwk
build-ubicloud-standard-8-jdk-17-min-java-17-coverage ✅ https://ge.armeria.dev/s/byss46mekt4qg
build-ubicloud-standard-8-jdk-17-min-java-11 ✅ https://ge.armeria.dev/s/yc5gh2v74f4lw
build-ubicloud-standard-8-jdk-17-leak ✅ https://ge.armeria.dev/s/ulwpjvcbybspa
build-ubicloud-standard-8-jdk-11 ✅ https://ge.armeria.dev/s/6d4wvjvp2njtm
build-macos-12-jdk-21 ✅ https://ge.armeria.dev/s/dwxkaqdngnqqg

@ikhoon ikhoon marked this pull request as ready for review October 15, 2024 03:37
jrhee17
jrhee17 approved these changes Oct 17, 2024
View reviewed changes
core/src/main/java/com/linecorp/armeria/internal/server/CorsHeaderUtil.java Outdated
@@ -47,6 +50,8 @@ public final class CorsHeaderUtil {
public static final String DELIMITER = ",";
private static final Joiner HEADER_JOINER = Joiner.on(DELIMITER);

private static AttributeKey<Boolean> IS_CORS_SET = AttributeKey.valueOf(CorsService.class, "IS_CORS_SET");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static AttributeKey<Boolean> IS_CORS_SET = AttributeKey.valueOf(CorsService.class, "IS_CORS_SET");
private static final AttributeKey<Boolean> IS_CORS_SET = AttributeKey.valueOf(CorsService.class, "IS_CORS_SET");

core/src/main/java/com/linecorp/armeria/internal/server/CorsHeaderUtil.java Outdated
@@ -47,6 +50,8 @@ public final class CorsHeaderUtil {
public static final String DELIMITER = ",";
private static final Joiner HEADER_JOINER = Joiner.on(DELIMITER);

private static AttributeKey<Boolean> IS_CORS_SET = AttributeKey.valueOf(CorsService.class, "IS_CORS_SET");

public static ResponseHeaders addCorsHeaders(ServiceRequestContext ctx, CorsConfig corsConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this method be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch!

core/src/main/java/com/linecorp/armeria/server/CorsServerErrorHandler.java
return oldRes.recover(HttpResponseException.class, ex -> {
return ex.httpResponse()
.mapHeaders(oldHeaders -> addCorsHeaders(ctx, corsService.config(), oldHeaders));
final CorsService corsService = ctx.findService(CorsService.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this was a problem before also, but preflight requests won't be handled correctly if an exception occurs before reaching CorsService.

Copy link
Contributor Author

@ikhoon ikhoon Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've changed to apply CORS headers only for simple requests or main requests.

If there is an error in processing preflight requests, we may not need to set CORS headers.

core/src/main/java/com/linecorp/armeria/server/CorsServerErrorHandler.java
return oldRes.recover(HttpResponseException.class, ex -> {
return ex.httpResponse()
.mapHeaders(oldHeaders -> addCorsHeaders(ctx, corsService.config(), oldHeaders));
final CorsService corsService = ctx.findService(CorsService.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This is also a similar issue, but a debug level log will be printed every time an exception is thrown for an Origin which doesn't have a policy defined:

armeria/core/src/main/java/com/linecorp/armeria/internal/server/CorsHeaderUtil.java

Line 127 in 683bd74

logger.debug(

Normally, this log wouldn't be printed if a request goes through CorsService

minwoox
minwoox approved these changes Nov 7, 2024
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@ikhoon ikhoon merged commit 90258fe into line:main Nov 7, 2024
14 checks passed
@ikhoon ikhoon deleted the cors-bug branch November 7, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minwoox minwoox minwoox approved these changes

@jrhee17 jrhee17 jrhee17 approved these changes

@trustin trustin Awaiting requested review from trustin trustin is a code owner

Assignees
No one assigned
Labels
defect
Projects
None yet
Milestone
1.31.0
Development

Successfully merging this pull request may close these issues.

CorsService does not inject CORS headers to error responses
3 participants
@ikhoon @jrhee17 @minwoox