Workers - Incoherent javascript behavior

AntonySK · May 27, 2019, 1:27pm

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

sandro · May 27, 2019, 1:35pm

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?

AntonySK · May 27, 2019, 1:41pm

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.

Xaq · May 27, 2019, 1:53pm

Can you share the whole code?

AntonySK · May 27, 2019, 2:00pm

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])
}

Xaq · May 27, 2019, 2:13pm

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

thomas4 · May 27, 2019, 2:15pm

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

Xaq · May 27, 2019, 2:17pm

Thanks

thomas4 · May 27, 2019, 2:17pm

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

AntonySK · May 27, 2019, 2:19pm

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
}

Xaq · May 27, 2019, 2:20pm

he already has full access to request object.

Xaq · May 27, 2019, 2:26pm

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

thomas4 · May 27, 2019, 2:27pm

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

Xaq · May 27, 2019, 2:47pm

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.

AntonySK · May 28, 2019, 10:29am

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?

thomas4 · May 28, 2019, 10:51am

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

Xaq · May 28, 2019, 11:12am

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.

thomas4 · May 28, 2019, 11:18am

I actually do three types of tests:

Unit tests on the code
Function testing using GitHub - dollarshaveclub/cloudworker: Run Cloudflare Worker scripts locally
External testing including stress-testing.

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

harris · May 29, 2019, 3:22am

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

AntonySK · May 29, 2019, 6:46am

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.

Topic		Replies	Views
Worker returning HTTP 429 Workers	0	2235	May 20, 2020
Redirecting worker not redirecting :-( Workers dash-errors , dash-troubleshooting	5	7282	May 31, 2018
Debugging workers specific javascript errors Workers dash-troubleshooting	6	8002	January 23, 2019
Issue With Worker-To-Worker HTTPS request Workers dash-errors , dash-troubleshooting	39	20069	November 26, 2021
Very simple worker Workers	3	3983	October 2, 2020

Workers - Incoherent javascript behavior

Related Topics