Workers - Incoherent javascript behavior

Hi,

We’re facing a behavior we cannot understand in one of our workers. Here it is.

THE CODE:

// Check if media is blacklisted by IP rules
let response403I = false;
Object.values(media_data.ip).forEach(range => {
    if(response403I) {return false; }
    console.log('plop1');
    if (!isInRange(clientIpNumber,range)) {
      console.log('plop2');
      response403I = true;
    }
});
console.log('response403I=', response403I);
console.log('typeofresponse403I=', typeof(response403I));

if(response403I === true) {
    return new Response('Sorry, this media is not available (I).', { status: 403, statusText: 'Forbidden' })
}

return response;

}

function isInRange(ip, range) {
return (range[0] <= ip && ip <= range[1])
}

THE CONSOLE:
worker.js:59 plop1
worker.js:65 response403I= false
worker.js:66 typeofresponse403I= boolean

THE RESULT:
Sorry, this media is not available (I).

THE PROBLEM:
In my case, isInRange() returns true and response403I var is not used anywhere else:

  • we should see in console “plop2” -> this is not the case
  • response403I is false (console) -> we should never enter in last if condition “if(response403I === true)” and we should not see this result page 403 error

Would someone have the start of the beginning of an idea of the problem? :o/
We don’t…

Thanks in advance for your answers,
Antony

So the problem is, the execution enters your if branch even though its condition evaluates to false?

That should not happen. Is that really the full code or is it some trimmed down version?

Yes, this is one problem.
The second one being that “response403I = true” is parsed (if I comment line, we do not enter the problematic “if” statement), but not “console.log(‘plop2’)”

This is not the full code. I do not think the rest can interfere. But I can publish a light misfunctionning version it if you want.

Can you share the whole code?

Here it is:

addEventListener('fetch', event => {
    event.respondWith(handleRequest(event.request))
})

async function handleRequest(request) {
    let parts = request.url.split('/');
    let media_id = parts[6];
    let asset = parts[7];

    const response = await fetch(request);

    if (asset !== 'files' && asset !== 'mosaic') {
        return response;
    }

    let clientIp = request.headers.get('CF-Connecting-IP');
    let clientIpNumber = ip2int(clientIp);
    let clientReferrer = request.headers.get('Referer');
    let clientUserAgent = request.headers.get('User-Agent');

    const media_data = await xxxxxxxxxxxx.get(media_id, 'json');
    if (media_data === null) {
      return response;
    }

    if (media_data.off === true) {
        return new Response('Sorry, this media is not available (O).', { status: 403, statusText: 'Forbidden' })
    }

    if (clientReferrer.includes(media_data.ref)) {
        return new Response('Sorry, this media is not available (R).', { status: 403, statusText: 'Forbidden' })
    }

    let response403I = false;
    Object.values(media_data.ip).forEach(range => {
        if(response403I) {return false; }
        console.log('plop1');
        if (!isInRange(clientIpNumber,range)) {
          console.log('plop2');
          response403I = true;
        }
    });

    console.log('response403I=', response403I);
    console.log('typeofresponse403I=', typeof(response403I));

    if(response403I === true) {
        return new Response('Sorry, this media is not available (I).', { status: 403, statusText: 'Forbidden' })
    }

    return response;
}

function ip2int(ip) {
    return ip.split('.').reduce(function(ipInt, octet) {
        return (ipInt<<8) + parseInt(octet, 10)
    }, 0) >>> 0;
}

function isInRange(ip, range) {
  return (range[0] <= ip && ip <= range[1])
}

Does await streamlike_cdn_media_security.get(media_id, 'json') return json ?
Then where you are parsing that json?

When getting KV values using .get, the worker parses the json into an object. No need to do parsing.

Thanks

I’m thinking that, maybe he needs to “await” the headers, I’ve seen issues with that.

Yes, it returns something like (in media_data which is parsed several times with success in the code):
{
“off”: false,
“ip”: [ [1514141737,1514141737] ],
“ref”: ["_dash.cloudflare.com"],
“tok”: null
}

1 Like

he already has full access to request object.

Thenip key has an array of arrays. Why are you using Object.values for iterating? You don’t need Object.values.

Ah, right. Yeah, I don’t see why it would return true, considering logging shows false… Makes no sense.

If response403I is fulse which is the case, this that if statement body should not be excuted:

if (response403I === true) {
        return new Response('Sorry, this media is not available (I).', { status: 403, statusText: 'Forbidden' })
    }

Then only thing I can think of is, you are debugging the code in the wrong way (mixing results of different requests) and that’s a general difficulty with debugging asynchronous code.

1 Like

I updated code with

media_data.ip.forEach(range => {
    if(response403I) {return false; }
    console.log('plop1');
    if (!isInRange(clientIpNumber,range)) {
      console.log('plop2');
      response403I = true;
    }
});

And the result is still the same.

I don’t understand what should be done to debug in the “right” way @Xaq . An idea?

I’d try abstracting the response handling to a function.

I don’t have experience with workers personally (tried in a CF app recently but seems it is not available to testers anymore). How you test this code? How you test branches (Does CF allow you to mock whatever you need i.e. IP, Country…). If testing is not sanboxed what happens if another request is made by a user at the time of testing? Without answers to these questions one cannot rely on results.

I actually do three types of tests:

  1. Unit tests on the code
  2. Function testing using https://github.com/dollarshaveclub/cloudworker
  3. External testing including stress-testing.

Despite these, yeah, it’s difficult and unreliable.

1 Like

Hi @AntonySK,

ip2int() doesn’t seem to handle IPv6 addresses correctly. ip2int("2001:0db8:85a3:0000:0000:8a2e:0370:7334") returns 2001, for example. Addresses that begin with a hex digit a-f would parse as NaN. I expect values such as these would cause isInRange() to return false.

To debug something like this, I would set about capturing the input that causes isInRange() to return false. In this case, the inputs you would need to capture are the IP range from KV and the original CF-Connecting-IP header. If you have access to a client which exhibits the buggy behavior, you could return those two values in a header:

if (!isInRange(clientIpNumber,range)) {
    return new Response('Sorry, this media is not available (I).', {
        status: 403,
        statusText: 'Forbidden',
        headers: { 'X-Debug': `${range} - ${clientIp}` },
    })
}

If you don’t have access to a client that exhibits the buggy behavior or have concerns about the security of exposing the header, you may have to log the input by sending a POST request to a logging service, which could be as simple as a requestbin.

Once you have the input, then you can test and debug locally.

I realize the debugging process is much less than ideal at the moment – we are working to improve the development experience.

Harris

1 Like

Thanks to all of you for your answers.

I don’t think ip2int may be the problem (ok, it does not handle ipv6 adresses, but we only deal with internal ipv4 addr yet), and if we reduce the function to basically return true, it does not solve the problem. And we do not need to debug client side yet by returning messages into the headers. My tests are made via Cloudflare test/debug interface with my own fixed ipv4 address.

I think our problem is due to Cloudflare interface we use to tests these scripts. Results are not the same if we “deploy” or “update preview” and how long we wait to launch test. At the same time, results can be different into the test interface and in a direct call (ok in debugger, error 500 on the direct call).

Another surprise is to see that (while preview and behavior are ok) testing tab content is not:

Try to understand some more inconsistencies and will have a call with support next week when our contract will be signed.