Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: Upcoming Release
    • Component/s: accounting
    • Labels:
      None
    • Flags:
      Patch

      Description

      PcChargeApi.java:81: 82, MS_PKGPROTECT

      • MS: org.apache.ofbiz.accounting.thirdparty.gosoftware.PcChargeApi.validOut should be package protected
        A mutable static field could be changed by malicious code or by accident. The field could be made package protected to avoid this vulnerability.

      PcChargeApi.java:189, DM_DEFAULT_ENCODING

      • Dm: Found reliance on default encoding in org.apache.ofbiz.accounting.thirdparty.gosoftware.PcChargeApi.send(): new java.io.PrintStream(OutputStream)
        Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behaviour to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.

      PcChargeApi.java:198, DM_DEFAULT_ENCODING

      • Dm: Found reliance on default encoding in org.apache.ofbiz.accounting.thirdparty.gosoftware.PcChargeApi.send(): new String(byte[], int, int)

      PcChargeServices.java:94: 180: 246: 306, RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE

      • RCN: Redundant nullcheck of out, which is known to be non-null in org.apache.ofbiz.accounting.thirdparty.gosoftware.PcChargeServices
        This method contains a redundant check of a known non-null value against the constant null.

      RitaApi.java:80, MS_PKGPROTECT

      • MS: org.apache.ofbiz.accounting.thirdparty.gosoftware.RitaApi.validOut should be package protected
        A mutable static field could be changed by malicious code or by accident. The field could be made package protected to avoid this vulnerability.

      RitaApi.java:84, MS_PKGPROTECT

      • MS: org.apache.ofbiz.accounting.thirdparty.gosoftware.RitaApi.validIn should be package protected
        A mutable static field could be changed by malicious code or by accident. The field could be made package protected to avoid this vulnerability.

      RitaServices.java:61: 98: 164: 184: 233: 260: 301: 329, RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE

      • RCN: Redundant nullcheck of api, which is known to be non-null in org.apache.ofbiz.accounting.thirdparty.gosoftware.RitaServices
        This method contains a redundant check of a known non-null value against the constant null.

        Activity

        Hide
        mbrohl Michael Brohl added a comment -

        Thanks Kyra,

        your patch is in trunk r1805460.

        Show
        mbrohl Michael Brohl added a comment - Thanks Kyra, your patch is in trunk r1805460.
        Hide
        jacques.le.roux Jacques Le Roux added a comment - - edited

        Hi Kyra,

        Right: https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html class private is better in this case

        Show
        jacques.le.roux Jacques Le Roux added a comment - - edited Hi Kyra, Right: https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html class private is better in this case
        Hide
        Kyra Pritzel-Hentley Kyra Pritzel-Hentley added a comment -

        Hi Jacques,

        It was no mistake. I removed the "protected" modifier on purpose. The FindBugs report is pointing out that subclasses could access this variable wrongfully. "Package protected" here means no explicit modifier should be used (which is the same as "package private").

        But anyway, "private" is the best Thanks for the suggestion!

        Show
        Kyra Pritzel-Hentley Kyra Pritzel-Hentley added a comment - Hi Jacques, It was no mistake. I removed the "protected" modifier on purpose. The FindBugs report is pointing out that subclasses could access this variable wrongfully. "Package protected" here means no explicit modifier should be used (which is the same as "package private"). But anyway, "private" is the best Thanks for the suggestion!
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Kyra,

        I guess it was a mistake, actually in your 1st patch you removed proctected But anyway indeed private was the way to go, thanks!

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Kyra, I guess it was a mistake, actually in your 1st patch you removed proctected But anyway indeed private was the way to go, thanks!
        Hide
        Kyra Pritzel-Hentley Kyra Pritzel-Hentley added a comment -

        I have added the private modifier to the validIn and validOut variables in RitaApi and PcChargeApi

        Show
        Kyra Pritzel-Hentley Kyra Pritzel-Hentley added a comment - I have added the private modifier to the validIn and validOut variables in RitaApi and PcChargeApi
        Hide
        Kyra Pritzel-Hentley Kyra Pritzel-Hentley added a comment -

        Hello Jacques,
        I removed the modifier because FindBugs suggested to make the variables package protected. But it would make a lot of sense as well to go one step further, as you say, and make them private. Then nothing can have influence on these variables from the outside.

        Show
        Kyra Pritzel-Hentley Kyra Pritzel-Hentley added a comment - Hello Jacques, I removed the modifier because FindBugs suggested to make the variables package protected. But it would make a lot of sense as well to go one step further, as you say, and make them private. Then nothing can have influence on these variables from the outside.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Same for RitaApi.

        Show
        jacques.le.roux Jacques Le Roux added a comment - Same for RitaApi.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Kyra,

        I did not review all, but why did you remove protected for validOut and validIn in PcChargeApi class?

        BTW they are only used in PcChargeApi class, so I'd rather make them private.

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Kyra, I did not review all, but why did you remove protected for validOut and validIn in PcChargeApi class? BTW they are only used in PcChargeApi class, so I'd rather make them private.
        Hide
        Kyra Pritzel-Hentley Kyra Pritzel-Hentley added a comment -

        This patch resolves the bugs found in package org.apache.ofbiz.accounting.thirdparty.gosoftware.
        The Nullchecks in RitaServices at Lines 61, 164, 233, 301 are not redundant and were not fixed since the methods buildPccProperties(context, delegator) and getApi(props, "CREDIT") in lines 59 and 60 respectively may return null.

        Show
        Kyra Pritzel-Hentley Kyra Pritzel-Hentley added a comment - This patch resolves the bugs found in package org.apache.ofbiz.accounting.thirdparty.gosoftware. The Nullchecks in RitaServices at Lines 61, 164, 233, 301 are not redundant and were not fixed since the methods buildPccProperties(context, delegator) and getApi(props, "CREDIT") in lines 59 and 60 respectively may return null.

          People

          • Assignee:
            mbrohl Michael Brohl
            Reporter:
            Kyra Pritzel-Hentley Kyra Pritzel-Hentley
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development