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

Bad regex in CORS settings should throw a nicer error #34035

Merged
merged 4 commits into from
Sep 27, 2018

Conversation

romseygeek
Copy link
Contributor

Currently a bad regex in CORS settings throws a PatternSyntaxException, which
then bubbles up through the bootstrap code, meaning users have to parse a
stack trace to work out where the problem is. We should instead catch this
exception and rethrow with a more useful error message.

Currently a bad regex in CORS settings throws a PatternSyntaxException, which
then bubbles up through the bootstrap code, meaning users have to parse a
stack trace to work out where the problem is.  We should instead catch this
exception and rethrow with a more useful error message.
@romseygeek romseygeek added >enhancement :Distributed/Network Http and internode communication implementations v7.0.0 v6.5.0 labels Sep 25, 2018
@romseygeek romseygeek self-assigned this Sep 25, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Looks fine but I left some suggestions.

builder = Netty4CorsConfigBuilder.forPattern(p);
}
}
catch (PatternSyntaxException e) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: catch should be on the same line as the closing brace of the try block.

}
}
catch (PatternSyntaxException e) {
throw new SettingsException("Bad regex in " + SETTING_CORS_ALLOW_ORIGIN.getKey() + ": " + origin, e);
Copy link
Member

Choose a reason for hiding this comment

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

Can you please enclose the parameters in square brackets? i.e.:

throw new SettingsException("Bad regex in [" + SETTING_CORS_ALLOW_ORIGIN.getKey() + "]: [" + origin + "]", e);

builder = NioCorsConfigBuilder.forPattern(p);
}
}
catch (PatternSyntaxException e) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: catch should be on the same line as the closing brace of the try block.

}
}
catch (PatternSyntaxException e) {
throw new SettingsException("Bad regex in " + SETTING_CORS_ALLOW_ORIGIN.getKey() + ": " + origin, e);
Copy link
Member

Choose a reason for hiding this comment

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

Can you please enclose the parameters in square brackets?

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thanks @romseygeek! LGTM

@romseygeek
Copy link
Contributor Author

retest this please

@romseygeek romseygeek merged commit 1cc53b5 into elastic:master Sep 27, 2018
@romseygeek romseygeek deleted the cors-regex-error branch September 27, 2018 09:02
romseygeek added a commit that referenced this pull request Sep 27, 2018
Currently a bad regex in CORS settings throws a PatternSyntaxException, which
then bubbles up through the bootstrap code, meaning users have to parse a
stack trace to work out where the problem is.  We should instead catch this
exception and rethrow with a more useful error message.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 27, 2018
* master: (25 commits)
  [DOCS] Synchronize location of Breaking Changes (elastic#33588)
  [DOCS] Synchronizes captialization in top-level titles (elastic#33605)
  [SQL] Clean up LogicalPlanBuilder#doJoin (elastic#34048)
  Fix remote cluster seeds fallback (elastic#34090)
  [ML][HLRC] Replace REST-based ML test cleanup with the ML client (elastic#34109)
  Handle MatchNoDocsQuery in span query wrappers (elastic#34106)
  Update MovAvgIT AwaitsFix bug url
  Bad regex in CORS settings should throw a nicer error (elastic#34035)
  [HLRC] Support for role mapper expression dsl (elastic#33745)
  Watcher: Reduce script cache churn by checking for mustache tags (elastic#33978)
  Fold EngineSearcher into Engine.Searcher (elastic#34082)
  Mute SpanMultiTermQueryBuilderTests#testToQuery
  TESTS: Enable DEBUG Logging in Flaky Test (elastic#34091)
  TEST: Add engine is closed as expected failure msg
  Adjust bwc version for max_seq_no_of_updates
  Build DocStats from SegmentInfos in ReadOnlyEngine (elastic#34079)
  When creating wildcard queries, use MatchNoDocsQuery when the field type doesn't exist. (elastic#34093)
  [DOCS] Moves graph to docs folder (elastic#33472)
  Mute MovAvgIT#testHoltWintersNotEnoughData
  Security: use default scroll keepalive (elastic#33639)
  ...
kcm pushed a commit that referenced this pull request Oct 30, 2018
Currently a bad regex in CORS settings throws a PatternSyntaxException, which
then bubbles up through the bootstrap code, meaning users have to parse a
stack trace to work out where the problem is.  We should instead catch this
exception and rethrow with a more useful error message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Network Http and internode communication implementations >enhancement v6.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants