Workers - Incoherent javascript behavior

Developers Workers
, ,
21

Since you’re checking headers, they always return null in the testing tab.

22

Based on the most recent code you posted

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

Regardless of any async code, if your console.log statement shows response403I as being false the following if branch should not execute. The only possible situation, I could currently imagine where this could occur, is if the variable got re-set after the output statement but before the if evaluation.

This could happen in a multi-threaded environment, but even there it would not necessarily be reproducible but rather a race-condition. Particularly in a single-threaded environment that should not be the case, even more so because the callback for forEach is called synchronously.

So, in short, the described behaviour should not happen. I’d suggest you open a support ticket. Additionally, I am tagging @KentonVarda and @harris.

@AntonySK, as sidenote, the return false in your forEach callback is not effective

23

Could the problem be that the response returned by the origin already contains the text Sorry, this media is not available (I)., and the worker is really just passing through that response from the origin? You could test this by adding a console.log() right before the return new Response to see if that branch is actually being taken.

One reason this could be happening is because of the (sort of broken) way the preview works: If you have already deployed a version of the worker to your site, and then you view the site in the preview, two copies of your worker end up being run: first the preview version, then the live version. This is because the preview actually layers over the public version of your site. The live worker is not “removed” when the preview runs. This is something we aim to fix, but it’s a complicated problem.

If that’s not the problem, then can you please try to reproduce the issue on Cloudflareworkers.com, and share with us a link to the script running there demonstrating the issue? (You can copy/paste the URL from the address bar after saving the script on Cloudflareworkers.com.) That way we can debug more easily.

3 Likes
24

Will have a try when I’ll be back next Monday. Many thanks for your answers! ;o)

25

Good find, and probably the most likely explanation, though I would have assumed the OP did verify the if branch gets executed. Well.

26

@sandro: I did not know, i always did like that… thanks for the tip. By the way, I replaced foreach asynchronous loop by a “for”, which is faster:

let response403I = false;
for(let range = 0; range < media_data.ip.length; range++){
    console.log(media_data.ip[range]);
    if (!isInRange(clientIpNumber,media_data.ip[range])) {
        response403I = true;
    }
}

@KentonVarda: I understand the problem. I will have some problems reproducing on Cloudflareworkers since kv is not operational. I’d have to inject json kv content directly in the code, I will have a try.
Question, if I purge all my scripts, then only use “update preview” and NEVER use “deploy”, will I have a coherent behavior?

@sandro @KentonVarda: the response returned by the origin is a video file ;o) The deal is to protect them with our own rules (we’re an ovp) which are offline, ip, token or referrer protected.

27

I had a try and I can confirm.

I deleted my script, created a new one. I only use “update preview”, and not “deploy” it works like a charm for now. Waiting for Kenton to confirm before closing the ticket and waiting for Cloudflare to fix the problem.

Thanks to all of you for your help :wink: