Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-1378

Args in job details links on jobhistory.jsp are not URL encoded

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.21.0
    • Component/s: jobtracker
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The logFile argument in the job links on the JT jobhistory.jsp page is not properly URL encoded leading to links that result in 500 errors. I found the issue while working with the Cloudera distro which contained a plus ('+') in the path which is interpreted as a space character (%20) by Firefox. Here is the (trimmed) URL. Note the hadoop-0.20.1+152 directory which should be hadoop-0.20.1%2B152. I have created a patch against current ASF svn trunk but it is untested (although the jsp compiles to a class file ok).

      A job link from http://host:50030/jobhistory.jsp:
      http://host:50030/jobdetailshistory.jsp?jobid=job_201001141235_0001&logFile=file:/Users/esammer/hadoop-0.20.1+152/logs/history/done/...

        Issue Links

          Activity

          Hide
          E. Sammer added a comment -

          Untested, needs review. Compiles cleanly.

          Show
          E. Sammer added a comment - Untested, needs review. Compiles cleanly.
          Hide
          Todd Lipcon added a comment -

          +1, looks good to me

          Show
          Todd Lipcon added a comment - +1, looks good to me
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12430352/MAPREDUCE-1378.patch
          against trunk revision 899501.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/272/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/272/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/272/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/272/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12430352/MAPREDUCE-1378.patch against trunk revision 899501. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/272/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/272/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/272/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/272/console This message is automatically generated.
          Hide
          Amar Kamat added a comment -

          Can't we use JobHistory.JobInfo.decodeJobHistoryFileName() in 0.20 and something equivalent in trunk to do the encoding?

          Show
          Amar Kamat added a comment - Can't we use JobHistory.JobInfo.decodeJobHistoryFileName() in 0.20 and something equivalent in trunk to do the encoding?
          Hide
          E. Sammer added a comment -

          I only gave the code a cursory glance, but it looks as if there's a number of places in the web apps where values might not be properly URL encoded. This may be a more pervasive problem.

          Show
          E. Sammer added a comment - I only gave the code a cursory glance, but it looks as if there's a number of places in the web apps where values might not be properly URL encoded. This may be a more pervasive problem.
          Hide
          Chris Douglas added a comment -

          Would you be willing to write a more pervasive fix?

          Show
          Chris Douglas added a comment - Would you be willing to write a more pervasive fix?
          Hide
          E. Sammer added a comment -

          Chris:

          I'd be happy to do so, but because of the way the web apps are structured, there isn't a lot of layering or centralization of input / output handling. I think the best way to handle this is to systematically factor the web apps into a cleaner, layered, architecture rather than sprinkling the logic in throughout the jsps, directly. It's a slightly larger undertaking; is that desirable?

          Show
          E. Sammer added a comment - Chris: I'd be happy to do so, but because of the way the web apps are structured, there isn't a lot of layering or centralization of input / output handling. I think the best way to handle this is to systematically factor the web apps into a cleaner, layered, architecture rather than sprinkling the logic in throughout the jsps, directly. It's a slightly larger undertaking; is that desirable?
          Hide
          Chris Douglas added a comment -

          It's hard to say "no" to a cleaner, layered architecture, but the breadth of such a change probably exceeds the scope of this issue. If you're willing to undertake that work, please file a separate JIRA and provide details there. Improving the structure of the jsps would be a welcome enhancement.

          A unit test for this would be awkward to write, but an explanation of how it was tested is a minimal prerequisite to committing the fix.

          Show
          Chris Douglas added a comment - It's hard to say "no" to a cleaner, layered architecture, but the breadth of such a change probably exceeds the scope of this issue. If you're willing to undertake that work, please file a separate JIRA and provide details there. Improving the structure of the jsps would be a welcome enhancement. A unit test for this would be awkward to write, but an explanation of how it was tested is a minimal prerequisite to committing the fix.
          Hide
          E. Sammer added a comment -

          Chris:

          Makes perfect sense, re: an improved web app. The testing of this is pretty straight forward.

          Before the patch:

          1. With a '+' or other non-encoded URL character in the real system path containing Hadoop, go to http://jt:50030/jobhistory.jsp.
          2. Click on one of the job ids to view job history details.
          3.a. Get a 500 response due to a FileNotFound exception ex. java.io.FileNotFoundException: File file:/Users/esammer/Applications/hadoop-0.20.1 152/logs/history/done/ (note the '+' in the Hadoop dir is a space).

          After the patch:

          1.5. Observe the '+' is encoded as %2B in the job id link.
          3.b. Instead of 3.a., observe that the page loads successfully.

          This is the test case I followed. This patch is very specific in that it deals with this singular link. The motivation behind doing a deep dive into the web app architecture is to address other places where user input or OS file system paths are used to build links for similar reasons. Let me know if this doesn't make sense.

          Show
          E. Sammer added a comment - Chris: Makes perfect sense, re: an improved web app. The testing of this is pretty straight forward. Before the patch: 1. With a '+' or other non-encoded URL character in the real system path containing Hadoop, go to http://jt:50030/jobhistory.jsp . 2. Click on one of the job ids to view job history details. 3.a. Get a 500 response due to a FileNotFound exception ex. java.io.FileNotFoundException: File file:/Users/esammer/Applications/hadoop-0.20.1 152/logs/history/done/ (note the '+' in the Hadoop dir is a space). After the patch: 1.5. Observe the '+' is encoded as %2B in the job id link. 3.b. Instead of 3.a., observe that the page loads successfully. This is the test case I followed. This patch is very specific in that it deals with this singular link. The motivation behind doing a deep dive into the web app architecture is to address other places where user input or OS file system paths are used to build links for similar reasons. Let me know if this doesn't make sense.
          Hide
          Chris Douglas added a comment -

          The motivation behind doing a deep dive into the web app architecture is to address other places where user input or OS file system paths are used to build links for similar reasons. Let me know if this doesn't make sense.

          This makes perfect sense. If you're willing to pursue this, please file a JIRA and post your proposal.

          +1

          I committed this. Thanks E. Sammer!

          Show
          Chris Douglas added a comment - The motivation behind doing a deep dive into the web app architecture is to address other places where user input or OS file system paths are used to build links for similar reasons. Let me know if this doesn't make sense. This makes perfect sense. If you're willing to pursue this, please file a JIRA and post your proposal. +1 I committed this. Thanks E. Sammer!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #245 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/245/)
          . URL encode link in jobhistory.jsp to avoid errors caused by unescaped characters. Contributed by E. Sammer

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #245 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/245/ ) . URL encode link in jobhistory.jsp to avoid errors caused by unescaped characters. Contributed by E. Sammer
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #239 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/239/)
          . URL encode link in jobhistory.jsp to avoid errors caused by unescaped characters. Contributed by E. Sammer

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #239 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/239/ ) . URL encode link in jobhistory.jsp to avoid errors caused by unescaped characters. Contributed by E. Sammer

            People

            • Assignee:
              E. Sammer
              Reporter:
              E. Sammer
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development