[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
GitHub user glitch opened a pull request:

    https://github.com/apache/accumulo/pull/289

    ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor

    I took a swing at this one.  I wasn't fully sure what the bounds on some of the params should be (i.e. should minutes be bound between (0-59]  )?  What level of sanitization is sufficient, UrlEncoder kind  of stuff?  Take a look at the ParameterValidator class first if you're interested as it is the piece that's reused across the rest endpoints.
   
    If you had something else in mind, feel free to decline or provide feedback and I can update.  Regards!
    Note: Still running a local build and need to add further unit tests etc. if you're interested in this submission.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/glitch/accumulo ACCUMULO-4677

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/accumulo/pull/289.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #289
   
----
commit fbe7bed5516890f89876053693b98c9e4f610987
Author: glitch <[hidden email]>
Date:   2017-08-02T00:56:04Z

    ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based...

ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/289
 
    Oh, nice! Thanks for working on this @glitch ; I took a brief look and I think this is basically what we need. I didn't do a thorough review, though. I'll try to get to that soon, if nobody else does.
   
    I think the main concern is that we don't allow input to be put back into the returned page in a way that poses a security risk. If a table can't be found, or a range of minutes doesn't work because it was input incorrectly, that's not so much a big deal, as long as it only breaks that particular HTTP request, and not Accumulo itself or the monitor state.
   
    The main concern is probably being able to click a link to the monitor which causes the monitor to start executing javascript or something which was in the query or path parameters.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r131427513
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tservers/TabletServerResource.java ---
    @@ -141,28 +146,27 @@ public TabletServersRecovery getTserverRecovery() {
       /**
        * Generates details for the selected tserver
        *
    -   * @param tserverAddr
    +   * @param tserverAddress
        *          TServer name
        * @return TServer details
        */
       @Path("{address}")
       @GET
    -  public TabletServerSummary getTserverDetails(@PathParam("address") String tserverAddr) throws Exception {
    -
    -    String tserverAddress = tserverAddr;
    +  public TabletServerSummary getTserverDetails(@PathParam("address") String tserverAddress) throws Exception {
    --- End diff --
   
    +1 I prefer full english words for parameter variable names (especially with Strings).  Unless you are using VIM as your IDE, abbreviation doesn't help anyone and only leads to confusion.  Granted, this example 99% of developers will know what "addr" means but who knows... there could be another variable to come along with the name tserverAdder or something.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r131424909
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java ---
    @@ -84,6 +86,10 @@
       @GET
       public RecentTracesList getTraces(@DefaultValue("10") @PathParam("minutes") int minutes) throws Exception {
     
    +    if (minutes >= 60 || minutes <= 0) {
    --- End diff --
   
    These should be MIN and MAX constants in ParameterValidator.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r131426801
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java ---
    @@ -258,8 +278,8 @@ protected Range getRangeForTrace(long minutesSince) {
         long startTime = endTime - millisSince;
     
         String startHexTime = Long.toHexString(startTime), endHexTime = Long.toHexString(endTime);
    -    while (startHexTime.length() < endHexTime.length()) {
    -      startHexTime = "0" + startHexTime;
    +    if (startHexTime.length() < endHexTime.length()) {
    +      StringUtils.leftPad(startHexTime, endHexTime.length(), '0');
    --- End diff --
   
    Nice improvement!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r131425860
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java ---
    @@ -176,12 +193,15 @@ public Void run() {
       @Path("show/{id}")
       @GET
       public TraceList getTracesType(@PathParam("id") String id) throws Exception {
    -    TraceList traces = new TraceList(id);
     
    -    if (id == null) {
    +    if (StringUtils.isEmpty(id)) {
    --- End diff --
   
    I don't know what type of ID this is referring to but if it is a table or namespace, should create ID type objects.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r131425948
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java ---
    @@ -119,16 +125,27 @@ public Void run() {
       /**
        * Generates a list of traces filtered by type and range of minutes
        *
    -   * @param type
    +   * @param typeParameter
        *          Type of the trace
        * @param minutes
        *          Range of minutes
        * @return List of traces filtered by type and range
        */
       @Path("listType/{type}/{minutes}")
       @GET
    -  public TraceType getTracesType(@PathParam("type") String type, @PathParam("minutes") int minutes) throws Exception {
    +  public TraceType getTracesType(@PathParam("type") String typeParameter, @PathParam("minutes") int minutes) throws Exception {
    +
    +    if (StringUtils.isEmpty(typeParameter)) {
    +      throw new Exception("Specified type was empty");
    +    }
     
    +    // Need finalized value for use in anonymous function below.
    +    final String type = ParameterValidator.sanitizeParameter(typeParameter);
    +    
    +    if (minutes >= 60 || minutes < 0) {
    +      throw new Exception("minutes was not of range [0-59], was " + String.valueOf(minutes));
    --- End diff --
   
    Should also use constants here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based...

ctubbsii
In reply to this post by ctubbsii
Github user glitch commented on the issue:

    https://github.com/apache/accumulo/pull/289
 
    @milleruntime What's your feeling on using javax.validation.constraints right in the method signature?  Doing that I can pretty much eliminate the ParameterValidator class altogether.
    i.e.
    `@Path("show/{id}")
      @GET
      public TraceList getTracesType(@PathParam("id") @NotNull @Pattern(regexp = "\\w+") String id) throws Exception {
    `
    For int params:
    `@DefaultValue("10") @PathParam("minutes") @NotNull @Min(0) @Max(2592000) int minutes`
   
    It loses the single class encapsulation, but on the other hand you can really fine tune each method and it makes it self documenting.  It looks like support is built directly into Jersey for it.
   
    Thoughts?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r132054185
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java ---
    @@ -77,12 +81,12 @@
        * Generates a trace summary
        *
        * @param minutes
    -   *          Range of minutes to filter traces
    +   *          Range of minutes to filter traces Min of 0 minutes, Max of 30 days
        * @return Trace summary in specified range
        */
       @Path("summary/{minutes}")
       @GET
    -  public RecentTracesList getTraces(@DefaultValue("10") @PathParam("minutes") int minutes) throws Exception {
    +  public RecentTracesList getTraces(@DefaultValue("10") @PathParam("minutes") @Min(0) @Max(2592000) int minutes) throws Exception {
    --- End diff --
   
    Very interested in how this is enforced. Any insight?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r132053160
 
    --- Diff: server/monitor/pom.xml ---
    @@ -45,6 +45,10 @@
           <artifactId>javax.servlet-api</artifactId>
         </dependency>
         <dependency>
    +      <groupId>javax.validation</groupId>
    +      <artifactId>validation-api</artifactId>
    --- End diff --
   
    This is interesting. What does the enforcement of these annotations?
   
    Also, if this is used, it should be added to the binary tarball (`assemble/src/main/assemblies/component.xml` and `assemble/pom.xml`), as well as the root `pom.xml`'s `<dependencyManagement>` section with a `<version>`. It's AL2 licensed, and no NOTICE file, and has no dependencies, so IP is pretty simple to deal with if we go this direction.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r132053484
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/rest/problems/ProblemsResource.java ---
    @@ -96,9 +98,13 @@ public ProblemSummary getSummary() {
       public void clearTableProblems(@QueryParam("s") String tableID) {
         Logger log = LoggerFactory.getLogger(Monitor.class);
         try {
    +      tableID = ParameterValidator.sanitizeParameter(tableID);
    +      if (StringUtils.isEmpty(tableID)) {
    --- End diff --
   
    Probably not worth bringing in StringUtils for simple `s == null || s.isEmpty()` checks. More use of commons-lang means more possible points of breakage with transitive dependencies.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r132053915
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tables/TablesResource.java ---
    @@ -65,6 +69,7 @@
     public class TablesResource {
     
       private static final TabletServerStatus NO_STATUS = new TabletServerStatus();
    +  private static final Pattern COMMA = Pattern.compile("(,|%2[cC])");
    --- End diff --
   
    Should not be doing any URL decoding here. This should already be URL-decoded by the framework. This is double-decoding. Although that's probably okay here (because namespaces can't contain commas), it's a bad precedent to set.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r132054933
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/util/ParameterValidator.java ---
    @@ -0,0 +1,48 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.monitor.util;
    +
    +import java.io.UnsupportedEncodingException;
    +import java.net.URLEncoder;
    +
    +import org.apache.commons.lang.StringUtils;
    +
    +/**
    + * Simple utility class to validate Accumulo Monitor Query and Path parameters
    + */
    +public class ParameterValidator {
    +
    +  /**
    +   * @param s
    +   *          String parameter to sanitize. Common usages are tableId, type, tserverAddress
    +   * @return URLEncoder encoded version of the passed in String parameter
    +   * @throws UnsupportedEncodingException
    +   *           if the string cannot be encoded to UTF-8
    +   */
    +  public static String sanitizeParameter(String s) throws UnsupportedEncodingException {
    +    return StringUtils.isEmpty(s) ? StringUtils.EMPTY : URLEncoder.encode(s, "UTF-8").trim();
    --- End diff --
   
    I don't think this should be returning a URL-encoded version of the string. In most cases, we actually need to use the value after it has been decoded by Jersey. The only time we'd want to re-encode it is before display... but this method doesn't seem to be limited to only being used prior to display in the view.
   
    It should also not throw an UnsupportedEncodingException. UTF-8 is always supported. This exception should be suppressed inside the method, instead of leaking out.
   
    Also, `StandardCharsets.UTF_8.name()` should be used instead of `"UTF-8"`, whenever the String version of the charset is required.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r132055156
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/util/ParameterValidator.java ---
    @@ -0,0 +1,48 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.monitor.util;
    +
    +import java.io.UnsupportedEncodingException;
    +import java.net.URLEncoder;
    +
    +import org.apache.commons.lang.StringUtils;
    +
    +/**
    + * Simple utility class to validate Accumulo Monitor Query and Path parameters
    + */
    +public class ParameterValidator {
    +
    +  /**
    +   * @param s
    +   *          String parameter to sanitize. Common usages are tableId, type, tserverAddress
    +   * @return URLEncoder encoded version of the passed in String parameter
    +   * @throws UnsupportedEncodingException
    +   *           if the string cannot be encoded to UTF-8
    +   */
    +  public static String sanitizeParameter(String s) throws UnsupportedEncodingException {
    +    return StringUtils.isEmpty(s) ? StringUtils.EMPTY : URLEncoder.encode(s, "UTF-8").trim();
    +  }
    +
    +  public static String sanitizeParameter(String s, String defaultValue) throws UnsupportedEncodingException {
    --- End diff --
   
    Is this method needed? It seems redundant with the default value annotation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user glitch commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r132062403
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/util/ParameterValidator.java ---
    @@ -0,0 +1,48 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.monitor.util;
    +
    +import java.io.UnsupportedEncodingException;
    +import java.net.URLEncoder;
    +
    +import org.apache.commons.lang.StringUtils;
    +
    +/**
    + * Simple utility class to validate Accumulo Monitor Query and Path parameters
    + */
    +public class ParameterValidator {
    +
    +  /**
    +   * @param s
    +   *          String parameter to sanitize. Common usages are tableId, type, tserverAddress
    +   * @return URLEncoder encoded version of the passed in String parameter
    +   * @throws UnsupportedEncodingException
    +   *           if the string cannot be encoded to UTF-8
    +   */
    +  public static String sanitizeParameter(String s) throws UnsupportedEncodingException {
    +    return StringUtils.isEmpty(s) ? StringUtils.EMPTY : URLEncoder.encode(s, "UTF-8").trim();
    --- End diff --
   
    @ctubbsii I've been researching the annotation route using the javax.validation api which would remove the necessity of this class & methods altogether.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user glitch commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r132064599
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java ---
    @@ -77,12 +81,12 @@
        * Generates a trace summary
        *
        * @param minutes
    -   *          Range of minutes to filter traces
    +   *          Range of minutes to filter traces Min of 0 minutes, Max of 30 days
        * @return Trace summary in specified range
        */
       @Path("summary/{minutes}")
       @GET
    -  public RecentTracesList getTraces(@DefaultValue("10") @PathParam("minutes") int minutes) throws Exception {
    +  public RecentTracesList getTraces(@DefaultValue("10") @PathParam("minutes") @Min(0) @Max(2592000) int minutes) throws Exception {
    --- End diff --
   
    The annotations are part of the javax.validation api and Jersey 2.0+ has built in support for them.  According to the documentation the auto-discovery service will auto-enable them on the server without the need of manual configuration.  The reference implementation is the hibernate validator which will need to be explicitly added to the pom based on their documentation:
   
    https://jersey.github.io/documentation/latest/bean-validation.html
   
    I'm still reading up on it and it sounds like it could negate the need for the ParameterValidator class in this PR (which I'll end up removing).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based...

ctubbsii
In reply to this post by ctubbsii
Github user glitch commented on the issue:

    https://github.com/apache/accumulo/pull/289
 
    Here is the link for the Jersey documentation surrounding parameter annotations.
    https://jersey.github.io/documentation/latest/bean-validation.html
   
    It's part of the JSR-303 spec and the reference implementation for the actual validation code is from hibernate-validator (org.hibernate) which looks to be pulled in as a transitive dependency.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r132070673
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java ---
    @@ -77,12 +81,12 @@
        * Generates a trace summary
        *
        * @param minutes
    -   *          Range of minutes to filter traces
    +   *          Range of minutes to filter traces Min of 0 minutes, Max of 30 days
        * @return Trace summary in specified range
        */
       @Path("summary/{minutes}")
       @GET
    -  public RecentTracesList getTraces(@DefaultValue("10") @PathParam("minutes") int minutes) throws Exception {
    +  public RecentTracesList getTraces(@DefaultValue("10") @PathParam("minutes") @Min(0) @Max(2592000) int minutes) throws Exception {
    --- End diff --
   
    Ah, interesting. So, it looks like there's a jersey extension which implements the validation for this API. Without the jersey-bean-validation, these are nothing more than ignored annotations. I'm curious what dependencies that might bring in. I don't think it'll be too bad, but need to be careful to include whatever we need in the assembly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on the issue:

    https://github.com/apache/accumulo/pull/289
 
    > @milleruntime What's your feeling on using javax.validation.constraints right in the method signature?
   
    I am on the fence with using external validation.  One one hand, it doesn't make sense to bring in a slew of dependencies just for a few annotations.  I have seen full web apps plagued by this in the past.  And since the monitor isn't the heart of Accumulo, I am more hesitant to include something like this.  On the other hand, if it is only one transitive dependency, I can definitely see the benefits it brings through custom code minimization and enhanced end point security, since this type of validation is quite common with web apps.
   
    I am curious to see what type of errors are thrown by the validation.  I have seen frameworks like hibernate obscure error handling, especially at the front end.  Although, I don't think we have any error handling on the REST endpoints yet so perhaps that could be a follow on task.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #289: ACCUMULO-4677 Sanitizing PathParam values in RES...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/289#discussion_r132199113
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java ---
    @@ -77,12 +81,12 @@
        * Generates a trace summary
        *
        * @param minutes
    -   *          Range of minutes to filter traces
    +   *          Range of minutes to filter traces Min of 0 minutes, Max of 30 days
        * @return Trace summary in specified range
        */
       @Path("summary/{minutes}")
       @GET
    -  public RecentTracesList getTraces(@DefaultValue("10") @PathParam("minutes") int minutes) throws Exception {
    +  public RecentTracesList getTraces(@DefaultValue("10") @PathParam("minutes") @Min(0) @Max(2592000) int minutes) throws Exception {
    --- End diff --
   
    Wouldn't defaulting to ignored annotation introduce a vulnerability in the validation? Say if an admin doesn't setup the monitor correctly or disables the auto-discovery.  I am wondering why we would ever want to disable the validation and can we prevent it from being disabled?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
12
Loading...