-
-
Notifications
You must be signed in to change notification settings - Fork 391
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
validate chargeBoxId for WS connections (#1526) #1527
Conversation
src/main/java/de/rwth/idsg/steve/ocpp/ws/OcppWebSocketHandshakeHandler.java
Outdated
Show resolved
Hide resolved
223ca79
to
e9ab4ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the new regex is too restrictive and will break instances.
src/test/java/de/rwth/idsg/steve/web/validation/ChargeBoxIdValidatorTest.java
Outdated
Show resolved
Hide resolved
src/main/java/de/rwth/idsg/steve/web/validation/ChargeBoxIdValidator.java
Outdated
Show resolved
Hide resolved
fair enough, and i was fearing this as well. new suggestion: instead of whitelisting what is allowed in the regex, i could blacklist what should not appear in the regex. i am thinking something like |
The blacklist solution sounds better. But even if I'll be surprised that a charger uses one of this chars somewhere you need a way to block the upgrade if a charger uses one of them in the database. |
i wonder how big that percentage is. if we are about to lose a minority (for ex 0.1%) of installations, i would be okay to make that sacrifice. i can put the new logic behind a config, but i then i would like to have it enabled by default. installations that have problems can still opt out, while accepting the risks. |
It can be enabled by default but I think it must be possible to disable it or block the upgrade if one station doesn't respect the convention. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except the ui fix which should be extended everywhere a displayed data comes from a station (maybe in another pr?).
yes, there will be another issue and another PR. |
No description provided.