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

Increase max regex length in SpEL expressions #30265

Closed
agolovenko opened this issue Apr 2, 2023 · 7 comments
Closed

Increase max regex length in SpEL expressions #30265

agolovenko opened this issue Apr 2, 2023 · 7 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: regression A bug that is also a regression
Milestone

Comments

@agolovenko
Copy link

agolovenko commented Apr 2, 2023

In version 6.0.7 of spring-expression there was a method added called checkRegexLength:

private void checkRegexLength(String regex) {
if (regex.length() > MAX_REGEX_LENGTH) {
throw new SpelEvaluationException(getStartPosition(),
SpelMessage.MAX_REGEX_LENGTH_EXCEEDED, MAX_REGEX_LENGTH);
}
}

It is adding a 256 char limit check on a length of a regex in a SpEL expression. We actually use longer regexes. Why would you put this limit? Seems quite random.

Please remove this check or at least make that max regex length value configurable.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 2, 2023
@quaff
Copy link
Contributor

quaff commented Apr 3, 2023

@agolovenko
Copy link
Author

agolovenko commented Apr 3, 2023

@quaff I understand the security concerns but then I'd suggest to make this max regex length value configurable: we do have some lengthy regexes that fail with the new version

@agolovenko agolovenko changed the title Spring EL added a limit to regex length Spring EL make max regex length value configurable Apr 3, 2023
@sbrannen
Copy link
Member

sbrannen commented Apr 3, 2023

Hi @agolovenko,

Thanks for reporting the issue.

We may consider increasing the max size restriction, but before we do that could you please answer the following questions?

  1. Can you provide an example of one of the regular expressions you are using that are longer than 256 characters?
  2. In what contexts are the affected SpEL expressions used?
    • For example: configuring bean properties via @Value, in Spring Integration pipelines, provided by external users via the web, etc.
  3. If we were to change the max size, what is the smallest "max size" that would be suitable for your application?

@sbrannen sbrannen self-assigned this Apr 3, 2023
@sbrannen sbrannen added type: regression A bug that is also a regression in: core Issues in core modules (aop, beans, core, context, expression) labels Apr 3, 2023
@sbrannen sbrannen changed the title Spring EL make max regex length value configurable SpEL: make max regex length value configurable Apr 3, 2023
@sbrannen sbrannen added the status: waiting-for-feedback We need additional information before we can continue label Apr 3, 2023
@agolovenko
Copy link
Author

Hi @sbrannen ,

  1. Here's the expression that fails at the moment, it's a generated expression and regex length is ~500 chars.
{'140','141','142','143','144','145','146','147','148','149','150','151','152','153','650','651','652','653','654','655','656','657','658','659','660','661','662','663','664','665','666','667','668','669','670','671','672','673','674','675','676','677','678','679','680','681'}.contains( $target.category_id:string ) and !( $target.brand:string matches '(?i).*(ABUS|ARLO|Artemide|AwoX|Axo\sLight|B\-Leuchten|Bankamp|Baulmann|Blaupunkt|David\sTrubridge|Decor\sWalther|Eglo\sconnect|Engel|Escale|EVE|Fatboy|Flos|Fontana\sArte|Foscarini|FRANDSEN|GUBI|Herzblut|Holtkötter|Homematic\sIP|Idual|Ingo\sMauerer|Innr\sLighting|Jieldé|Knapstein|LE\sKLINT|Ledvance\sSmart|LIFX|LiquidBeam|Louis\sPoulsen|Luke\sRoberts|LZF\sLamps|Marchetti|Masiero|Menu|Nanoleaf|NEMO|Ozonos|Philips|Philips\sHue|Q\-Smart\-Home|Rademacher|Schellenberg|Senic|Steinel|Tado|Tecnolumen|Tint|UMAGE|Wiz).*' ) and $target.price gt 17000
  1. We're creating SpELs using new SpelExpressionParser().parseExpression(expr)
  2. I guess 1024 would be acceptable. But here's the thing: what's OK for us might not be OK for some other user. I didn't ask to increase the limit on purpose: whichever value you end up with would be subjective. Hardcoding it is what makes this implementation weak. I suggest giving users a way of configuring this value. Then you could leave 256 as a default one.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 3, 2023
@quaff
Copy link
Contributor

quaff commented Apr 4, 2023

I suggest you extract this into a static method, then call method in SpEL like this T(com.acme.Util).validate(#input)

@sbrannen
Copy link
Member

sbrannen commented Apr 4, 2023

Thanks for providing feedback, @agolovenko.

Out of curiosity, what do you mean by "generated"?

Is the expression generated by a custom tool that is internal to your application?

In other words, is the generator a trusted source?

Since you state that you are manually creating the SpelExpressionParser, that implies that you are likely evaluating those expressions against a StandardEvaluationContext. So, if that's the case then you are actually not limited by the recent max size restriction.

When evaluating against a StandardEvaluationContext, you can replace expressions like 'foo' matches 'regex' with 'foo'.matches('regex').

In addition, you can move more complex business logic to a static utility function that is invoked via the T() operator -- as pointed out by @quaff -- or to a method in a bean in the ApplicationContext that is invoked via the bean reference notation @myBean.myMethod(...) (if your expressions are evaluated against an ApplicationContext).

⚠️ Please note that the onus is on you to ensure that you are not evaluating expressions from untrusted sources when using a StandardEvaluationContext.

We do not plan to make any restrictions on SpEL operators configurable. In light of that, please let us know if either of the aforementioned alternatives works for you.

Thanks

@sbrannen sbrannen added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Apr 4, 2023
@sbrannen
Copy link
Member

sbrannen commented Apr 6, 2023

As mentioned previously, we do not have plans to make the max regex length configurable; however, we are willing to increase it from 256 to 1024 in order to support use cases where a regex may be rather lengthy due to inclusion of several options in an OR clause (such as (a|b|c|d|e|f|...)) or similar scenarios that result in a longer regex without necessarily increasing the complexity of the regex.

In light of that, I am repurposing this issue to address that.

@sbrannen sbrannen removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 6, 2023
@sbrannen sbrannen added this to the 6.0.8 milestone Apr 6, 2023
@sbrannen sbrannen changed the title SpEL: make max regex length value configurable Increase max regex length in SpEL Apr 6, 2023
@sbrannen sbrannen changed the title Increase max regex length in SpEL Increase max regex length in SpEL expressions Apr 6, 2023
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.3.x labels Apr 6, 2023
bclozel pushed a commit that referenced this issue Apr 13, 2023
This commit changes the max regex length in SpEL expressions from 1024
to 1000 in order to consistently use "round" numbers for recently
introduced limits.

See gh-30265
bclozel pushed a commit that referenced this issue Apr 13, 2023
This commit changes the max regex length in SpEL expressions from 1024
to 1000 in order to consistently use "round" numbers for recently
introduced limits.

See gh-30265
bclozel pushed a commit that referenced this issue Apr 13, 2023
This commit changes the max regex length in SpEL expressions from 1024
to 1000 in order to consistently use "round" numbers for recently
introduced limits.

See gh-30265
cesarhernandezgt added a commit to cesarhernandezgt/spring-framework that referenced this issue Apr 21, 2023
This commit changes the max regex length in SpEL expressions from 1024 to 1000 in order to consistently use round numbers for recently introduced limits. See spring-projectsgh-30265
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

4 participants