You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/11/05 19:14:10 UTC
[GitHub] [pulsar] rdhabalia opened a new pull request #8454: [pulsar-broker] Security-Recommendation: Remove verbose error message with system info for admin-api
rdhabalia opened a new pull request #8454:
URL: https://github.com/apache/pulsar/pull/8454
### Motivation
one of the security-recommendation report has listed system internal info in error-response which should be fixed.
**url:** `curl -X DELETE -H 'Content-Type: application/json' http://localhost:8080/admin/namespaces/sample/standalone/ns1/maxConsumerPerSubscription`
**Error-response:**
```
--- An unexpected error occurred in the server ---
Message: Invalid bundle range
Stacktrace:
java.lang.IllegalArgumentException: Invalid bundle range
at com.google.common.base.Preconditions.checkArgument(Preconditions.java:141)
at org.apache.pulsar.broker.web.PulsarWebResource.validateNamespaceBundleRange(PulsarWebResource.java:480)
at org.apache.pulsar.broker.web.PulsarWebResource.validateNamespaceBundleOwnership(PulsarWebResource.java:522)
at org.apache.pulsar.broker.admin.impl.NamespacesBase.internalDeleteNamespaceBundle(NamespacesBase.java:541)
at org.apache.pulsar.broker.admin.impl.NamespacesBase.internalDeleteNamespaceBundle(NamespacesBase.java:488)
at org.apache.pulsar.broker.admin.v1.Namespaces.deleteNamespaceBundle(Namespaces.java:229)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
```
### Modification
Return informative response without system info
### Result
```
curl -X DELETE -H 'Content-Type: application/json' http://localhost:8080/admin/namespaces/sample/standalone/ns1/my-range
{"reason":"Invalid bundle range: my-range"}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #8454: [pulsar-broker] Security-Recommendation: Remove verbose error message with system info for admin-api
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #8454:
URL: https://github.com/apache/pulsar/pull/8454
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8454: [pulsar-broker] Security-Recommendation: Remove verbose error message with system info for admin-api
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8454:
URL: https://github.com/apache/pulsar/pull/8454#issuecomment-722699735
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8454: [pulsar-broker] Security-Recommendation: Remove verbose error message with system info for admin-api
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8454:
URL: https://github.com/apache/pulsar/pull/8454#issuecomment-723187354
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8454: [pulsar-broker] Security-Recommendation: Remove verbose error message with system info for admin-api
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8454:
URL: https://github.com/apache/pulsar/pull/8454#issuecomment-722609070
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8454: [pulsar-broker] Security-Recommendation: Remove verbose error message with system info for admin-api
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8454:
URL: https://github.com/apache/pulsar/pull/8454#issuecomment-722683073
@eolivelli the change and validation are to improve error-message so, I didn't add unit-test as we didn't have any functionality change. however, there is no harm in adding test. I have added one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #8454: [pulsar-broker] Security-Recommendation: Remove verbose error message with system info for admin-api
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #8454:
URL: https://github.com/apache/pulsar/pull/8454#discussion_r518574819
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest2.java
##########
@@ -1338,6 +1338,16 @@ public void testMaxNamespacesPerTenant() throws Exception {
}
+ @Test
+ public void testInvalidBundleErrorResponse() throws Exception {
+ try {
+ admin.namespaces().deleteNamespaceBundle("prop-xyz/ns1", "invalid-bundle");
+ fail("should have failed due to invalid bundle");
+ } catch (PreconditionFailedException e) {
+ assertTrue(e.getMessage().startsWith("Invalid bundle range"));
Review comment:
thank you for adding the test @rdhabalia
I asked for a test, because IIUC the issue here is that we want a new message for security reasons,
messages in exceptions sometimes change due to refactors and it is probable that in the future someone will change the error message if there is no test case.
the implementation is that we are now referring to the bundleRange in the message
IMHO the test should test that actually the message contains that value.
Also I would add here a comment that states that it is better to not change the message for security reasons (with a minimal explanation, not just "for security reasons")
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8454: [pulsar-broker] Security-Recommendation: Remove verbose error message with system info for admin-api
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8454:
URL: https://github.com/apache/pulsar/pull/8454#issuecomment-722591032
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia removed a comment on pull request #8454: [pulsar-broker] Security-Recommendation: Remove verbose error message with system info for admin-api
Posted by GitBox <gi...@apache.org>.
rdhabalia removed a comment on pull request #8454:
URL: https://github.com/apache/pulsar/pull/8454#issuecomment-723187354
/pulsarbot run-failure-checks
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org