search

facebook / facebook-java-business-sdk

Java SDK for Meta Marketing APIs
https://developers.facebook.com/docs/business-sdk
Other
402 stars 332 forks source link

NPE from readResponse catch block on SSL cert issue instead of APIException throwable #330

Closed galacticgumshoe closed 2 years ago

galacticgumshoe commented 3 years ago

Which SDK version are you using?

9.0.0

What's the issue?

A NullPointerException is thrown when parsing the results while invoking the Business.APIRequestGetOwnedAdAccounts() method. This is in a Spring Boot application running on JVM 1.8.

Steps/Sample code to reproduce the issue

Service class:

package com.business.facebook.sdk.ingestion.pipelines;

import com.facebook.ads.sdk.*;
import com.facebook.ads.sdk.Business.APIRequestGetOwnedAdAccounts;

import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.stereotype.Service;

import java.io.PrintStream;
import java.util.ArrayList;
import java.util.List;

@Slf4j
@Service("facebookIngestion")
public class FacebookIngestionPipeline {

    @Value("xxx")
    String ACCESS_TOKEN;

    @Value("xxx")
    String APP_SECRET;

    public void execute(String ACCOUNT_ID) throws RuntimeException {
        APIContext context = new APIContext(ACCESS_TOKEN, APP_SECRET, ACCOUNT_ID);
        context.enableDebug(true);
        log.info("App ID: "+context.getAppID());
        log.info("Secret: "+context.getAppSecret());
        log.info("Token: "+context.getAccessToken());
        log.info("Debugger: "+ context.isDebug());
        try {
            APIRequestGetOwnedAdAccounts business = new APIRequestGetOwnedAdAccounts(ACCOUNT_ID, context);
            List<String> ownedAdAccountFieldList = new ArrayList<>();
            ownedAdAccountFieldList.add("id");
            ownedAdAccountFieldList.add("account_id");

            APINodeList<AdAccount> result = business.execute().withAutoPaginationIterator(true);
        } catch (NullPointerException|APIException e) {
            log.error(e.toString());
            e.printStackTrace();
            throw new RuntimeException(e.getMessage(), e.getCause());
        }
    }
}
Controller:
package com.business.facebook.sdk.ingestion.controllers;

import com.business.facebook.sdk.ingestion.pipelines.FacebookIngestionPipeline;
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestAttribute;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

@RestController
@Slf4j
public class IngestionController {

    @Autowired
    FacebookIngestionPipeline facebookIngestion;

    @GetMapping("ingest")
    public void ingest(@RequestParam(value = "source") String streamProcess, @RequestParam(name = "account_id") String accountId) {
        log.info("streamProcess::"+streamProcess);

        try {
            facebookIngestion.execute(accountId);
        } catch (RuntimeException e) {
            e.printStackTrace();
            throw new RuntimeException(e.getMessage(), e.getCause());
        }
    }
}
Sample curl:

curl -G http://localhost:8080/ingest\?source\=facebook\&account_id\="xxx"

Observed Results:

galacticgumshoe commented 3 years ago

I believe the NPE is being thrown and not handled properly due to the APIRequest.executeInternal call, which lines up to what I'm seeing in my logs:

protected ResponseWrapper executeInternal(Map<String, Object> extraParams) throws APIException {
    // extraParams are one-time params for this call,
    // so that the APIRequest can be reused later on.
    ResponseWrapper response = null;
    try {
      context.log("========Start of API Call========");
      response = executor.execute(method, getApiUrl(), getAllParams(extraParams), context);
      context.log("Response:");
      context.log(response.getBody());
      context.log("========End of API Call========");
    } catch(IOException e) {
      throw new APIException.FailedRequestException(e);
    }
    return response;
  }

As pointed out in the logs I can see the "========Start of API Call========" getting logged, followed by the METHOD and URL (assuming coming from the executor.execute... line), but nothing after that point. Since this function is only catching IOExceptions, it's causing NPE to pass unchecked. I'm a bit dubious about why it's initializing also the response as null here too, which could further complicate callers to this method in situations where maybe they usually get responses, but could have a network hiccup in between that could cause no response from their call and theoretically another NPE thrown.

galacticgumshoe commented 3 years ago

In reading through this: https://docs.oracle.com/javase/7/docs/api/java/net/URLConnection.html and lining up the logic happening in the stack trace between these different methods:

        at com.facebook.ads.sdk.APIRequest.readResponse(APIRequest.java:298)
        at com.facebook.ads.sdk.APIRequest.access$100(APIRequest.java:53)
        at com.facebook.ads.sdk.APIRequest$DefaultRequestExecutor.sendGet(APIRequest.java:533)
        at com.facebook.ads.sdk.APIRequest$DefaultRequestExecutor.execute(APIRequest.java:517)
        at com.facebook.ads.sdk.APIRequest.executeInternal(APIRequest.java:198)

I don't see where the connect() method on the HttpsURLConnection object is being invoked. In the APIRequest.sendGet() method, I can see the openConnection() method is being invoked. Shortly after this is where in my logs I get the next two lines showing "Request:" and "Get: " + url.toString(). But then from there after decorating the HttpsURLConnection object with its method and a couple of headers, it gets passed to the APIRequest.readResponse() method:

    public ResponseWrapper sendGet(String apiUrl, Map<String, Object> allParams, APIContext context) throws APIException, IOException {
      URL url = new URL(RequestHelper.constructUrlString(apiUrl, allParams));
      context.log("Request:");
      context.log("GET: " + url.toString());
      HttpsURLConnection con = (HttpsURLConnection) url.openConnection();

      con.setRequestMethod("GET");
      con.setRequestProperty("User-Agent", USER_AGENT);
      con.setRequestProperty("Content-Type","application/x-www-form-urlencoded");

      return readResponse(con);
    }

The first step in the try block of readResponse does a getResponseCode() on the HttpsURLConnection object. If the API docs are to be believed, there needs to be a connect() call made first before we can get the headers, but I'm not seeing it here. I guess that could cause a NPE to get thrown, but in here that would be caught (since it's catching all Exceptions). 

  private static ResponseWrapper readResponse(HttpsURLConnection con) throws APIException, IOException {
    try {
      int responseCode = con.getResponseCode();

      String header = convertToString(con.getHeaderFields());
      BufferedReader in = new BufferedReader(new InputStreamReader(con.getInputStream()));
      String inputLine;
      StringBuilder response = new StringBuilder();

      while ((inputLine = in.readLine()) != null) {
        response.append(inputLine);
      }
      in.close();
      return new ResponseWrapper(response.toString(), header);
    } catch(Exception e) {
      BufferedReader in = new BufferedReader(new InputStreamReader(con.getErrorStream()));
      String inputLine;
      StringBuilder response = new StringBuilder();

      while ((inputLine = in.readLine()) != null) {
        response.append(inputLine);
      }
      in.close();
      throw new APIException.FailedRequestException(
        convertToString(con.getHeaderFields()), response.toString(), e
      );
    }
  }

So that would mean the connection is never actually being made from this sendGet() method, always resulting in the getResponseCode() failing and forcing it to treat the call as a failure.

galacticgumshoe commented 3 years ago

I can see now the NPE is not being thrown in the try block, it is actually coming from the catch block in the readResponse method. The line in particular that's causing the NPE to get thrown is this:

BufferedReader in = new BufferedReader(new InputStreamReader(con.getErrorStream()));

Which means the NPE has swallowed up whatever the original Exception that was thrown in the try block and isn't throwing a new catchable exception since it's over before the throw new APIException.FailedRequestException... can be thrown. So I tried commenting out everything in the catch block:

catch(Exception e) {
            //BufferedReader in = new BufferedReader(new InputStreamReader(con.getErrorStream()));
            //String inputLine;
            //StringBuilder response = new StringBuilder();

            //while ((inputLine = in.readLine()) != null) {
                //response.append(inputLine);
            //}
            //in.close();
            throw new APIException.FailedRequestException(
                    convertToString(con.getHeaderFields()), con.getResponseMessage(), e
            );
        }

That only throws a new APIException, and that successfully showed me what the real problem was:

javax.net.ssl.SSLHandshakeException: sun.security.validator.ValidatorException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target

I fixed my certificates in my local and now it's working. So the original problem is resolved, but this brings up a new issue, which is that this catch block needs to be fixed so in situations where there is no response from the network resource (and the failure happens locally) that it properly handles an NPE. Possibly embed another try/catch block around the lines I have commented above to make sure that a secondary exception doesn't prevent the proper throw.

stale[bot] commented 3 years ago

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 2 years ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.