diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index d585ae7..354536a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -5,8 +5,7 @@ on: branches: [main] pull_request: -permissions: - contents: read +permissions: {} jobs: test: @@ -15,8 +14,17 @@ jobs: matrix: node: ['22', '24'] steps: - - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 - - uses: actions/setup-node@249970729cb0ef3589644e2896645e5dc5ba9c38 - with: - node-version: ${{ matrix.node }} - - run: node --test test/Code.test.cjs + - name: Check out the commit under test + run: | + git init --quiet + git fetch --no-tags --depth=1 "$GITHUB_SERVER_URL/$GITHUB_REPOSITORY.git" "$GITHUB_SHA" + git checkout --quiet --detach "$GITHUB_SHA" + - name: Run formula tests + env: + NODE_MAJOR: ${{ matrix.node }} + run: | + source "$HOME/.nvm/nvm.sh" --no-use + nvm install "$NODE_MAJOR" + test "$(node -p 'process.versions.node.split(".")[0]')" = "$NODE_MAJOR" + node --version + node --test test/Code.test.cjs diff --git a/Code.gs b/Code.gs index 036bc70..ac71b4b 100644 --- a/Code.gs +++ b/Code.gs @@ -161,11 +161,6 @@ function IPGEO_BULK(ipRange, field, include) { } const results = fetchIpGeoBulk_(ips, include); - - if (!Array.isArray(results) || results.length !== ips.length) { - throw new Error('Invalid bulk response: expected one result per IP address.'); - } - let resultIndex = 0; return inputs.map(function (ip) { return [ip === '' ? '' : getNestedValue_(results[resultIndex++], field)]; @@ -240,19 +235,37 @@ function fetchIpGeoBulk_(ips, include) { contentType: 'application/json', payload: JSON.stringify({ ips: ips }), muteHttpExceptions: true - }); + }, ips.length); } -function fetchJsonWithCache_(url, options) { +function prepareBulkResponse_(rows, expectedRows, requestKey) { + if (!Array.isArray(rows) || rows.length !== expectedRows || + rows.some(row => row === null || typeof row !== 'object' || Array.isArray(row))) { + throw new Error('Invalid bulk response: expected one result per IP address.'); + } + let changed = false; + for (const row of rows) { + if (typeof row.message === 'string' && !Object.prototype.hasOwnProperty.call(row, 'ip')) { + const message = redactErrorMessage_(row, requestKey); + if (message !== row.message) { + row.message = message; + changed = true; + } + } + } + return changed; +} + +function fetchJsonWithCache_(url, options, expectedRows) { options = options || { method: 'get', muteHttpExceptions: true }; const cacheKey = makeCacheKey_(options.method.toUpperCase() + ':' + url + ':' + (options.payload || '')); - const cached = getCachedJson_(cacheKey); + const requestKey = decodeURIComponent((url.match(/[?&]apiKey=([^&]*)/i) || [])[1] || ''); + const cached = getCachedJson_(cacheKey, expectedRows, requestKey); if (cached !== null) { return cached; } - const requestKey = decodeURIComponent((url.match(/[?&]apiKey=([^&]*)/i) || [])[1] || ''); let data; try { const response = UrlFetchApp.fetch(url, options); @@ -264,6 +277,9 @@ function fetchJsonWithCache_(url, options) { } data = JSON.parse(body); + if (expectedRows !== undefined) { + prepareBulkResponse_(data, expectedRows, requestKey); + } } catch (error) { throw new Error(redactErrorMessage_(error, requestKey)); } @@ -272,13 +288,24 @@ function fetchJsonWithCache_(url, options) { return data; } -function getCachedJson_(cacheKey) { +function getCachedJson_(cacheKey, expectedRows, requestKey) { + let cleaned = null; try { - const cached = CacheService.getScriptCache().get(cacheKey); - return cached ? JSON.parse(cached) : null; + const cache = CacheService.getScriptCache(); + const cached = cache.get(cacheKey); + if (!cached) return null; + try { + const data = JSON.parse(cached); + if (expectedRows === undefined || !prepareBulkResponse_(data, expectedRows, requestKey)) return data; + cleaned = data; + } catch (error) { + cleaned = null; + } + cache.remove(cacheKey); } catch (error) { - return null; + return cleaned; } + return cleaned; } function cacheJson_(cacheKey, data) { diff --git a/test/Code.test.cjs b/test/Code.test.cjs index 1bf5152..feb23e4 100644 --- a/test/Code.test.cjs +++ b/test/Code.test.cjs @@ -36,6 +36,10 @@ function loadScript(settings = {}) { assert.ok(Buffer.byteLength(value, 'utf8') <= 100000); writes.push({ key, value, ttl }); entries.set(key, value); + }, + remove: key => { + if (settings.removeError) throw new Error('Cache unavailable'); + entries.delete(key); } }; const context = vm.createContext({ @@ -75,7 +79,7 @@ function loadScript(settings = {}) { } }); vm.runInContext(source, context, { filename: 'Code.gs' }); - return { context, properties, requests, writes }; + return { context, properties, requests, writes, entries }; } const plain = value => JSON.parse(JSON.stringify(value)); @@ -141,15 +145,22 @@ test('bulk validates the nonblank request count and response length', () => { assert.match(loadScript({ data: {} }).context.IPGEO_BULK(['8.8.8.8'], 'location.city')[0][0], /bulk response/i); }); -test('bulk accepts 50000 nonblank inputs while retaining extra blank positions', () => { +test('bulk keeps all 50000 mixed results in order with extra blank positions', () => { const ips = Array(50000).fill('8.8.8.8'); - const { context: c, requests } = loadScript({ data: ips.map(() => ({ city: 'A' })) }); - const values = c.IPGEO_BULK(['', ...ips, ''], 'city'); + const key = 'large-batch-fixture'; + const data = ips.map((ip, i) => i % 5 === 0 ? { message: 'Invalid: ' + key } : { ip, index: i }); + const { context: c, requests, writes } = loadScript({ key, data }); + const values = c.IPGEO_BULK(['', ...ips, ''], ''); assert.equal(values.length, 50002); assert.equal(values[0][0], ''); - assert.equal(values[1][0], 'A'); - assert.equal(values[50000][0], 'A'); assert.equal(values[50001][0], ''); + for (let i = 0; i < ips.length; i++) { + const row = JSON.parse(values[i + 1][0]); + if (i % 5 === 0) assert.equal(row.message, 'Invalid: [REDACTED]'); + else assert.deepEqual(row, data[i]); + } + assert.equal(requests.length, 1); + assert.equal(writes.length, 0); assert.equal(JSON.parse(requests[0].options.payload).ips.length, 50000); }); @@ -160,6 +171,99 @@ test('per-entry API errors do not shift later bulk results', () => { assert.deepEqual(plain(c.IPGEO_BULK(['8.8.8.8', 'invalid', '', '1.1.1.1'], 'city')), [['A'], [''], [''], ['B']]); }); +test('bulk masks keys in error rows before displaying or caching them', () => { + const key = 'fixture+key/%'; + const good = { ip: '8.8.8.8', message: 'Public result: ' + key, city: 'A' }; + const error = { message: 'Failed: ' + key + ' or ' + encodeURIComponent(key) }; + const app = loadScript({ key, data: [good, error] }); + const inputs = ['8.8.8.8', '', 'invalid']; + const masked = 'Failed: [REDACTED] or [REDACTED]'; + assert.deepEqual(plain(app.context.IPGEO_BULK(inputs, 'message')), [[good.message], [''], [masked]]); + const raw = app.context.IPGEO_BULK(inputs, ''); + assert.deepEqual(JSON.parse(raw[0][0]), good); + assert.deepEqual(JSON.parse(raw[2][0]), { message: masked }); + assert.deepEqual(JSON.parse(app.writes[0].value), [good, { message: masked }]); + assert.equal(app.requests.length, 1); + assert.equal(app.writes.length, 1); +}); + +test('older cached error rows are safe to display even if cache removal fails', () => { + for (const removeError of [false, true]) { + const key = 'fixture-old-key'; + const app = loadScript({ key, removeError, data: [{ message: 'Invalid address' }] }); + app.context.IPGEO_BULK(['invalid'], 'message'); + const cacheKey = app.writes[0].key; + app.entries.set(cacheKey, JSON.stringify([{ message: 'Failed: ' + key }])); + assert.equal(JSON.parse(app.context.IPGEO_BULK(['invalid'], '')[0][0]).message, 'Failed: [REDACTED]'); + assert.equal(app.requests.length, 1); + if (!removeError) assert.ok(!(app.entries.get(cacheKey) || '').includes(key)); + } +}); + +test('bulk masks the key used for the request if the saved key changes', () => { + const key = 'original-fixture'; + const app = loadScript({ + key, + get data() { + app.properties.set('IPGEOLOCATION_API_KEY', 'replacement-fixture'); + return [{ message: 'Invalid: ' + key }]; + } + }); + assert.deepEqual(plain(app.context.IPGEO_BULK(['invalid'], 'message')), [['Invalid: [REDACTED]']]); + assert.ok(!app.writes[0].value.includes(key)); +}); + +test('an incomplete live response does not prevent the next lookup from succeeding', () => { + const good = [{ city: 'A' }, { city: 'B' }]; + const inputs = ['8.8.8.8', '1.1.1.1']; + for (const invalid of [[good[0]], [...good, good[0]], {}, null, + [good[0], null], [good[0], 42], [good[0], 'invalid'], [good[0], []]]) { + const settings = { data: invalid }; + const app = loadScript(settings); + assert.match(app.context.IPGEO_BULK(inputs, 'city')[0][0], /expected one result per IP address/); + assert.equal(app.writes.length, 0); + settings.data = good; + assert.deepEqual(plain(app.context.IPGEO_BULK(inputs, 'city')), [['A'], ['B']]); + assert.deepEqual(plain(app.context.IPGEO_BULK(inputs, 'city')), [['A'], ['B']]); + assert.equal(app.requests.length, 2); + assert.equal(app.writes.length, 1); + } +}); + +test('a wrong-sized cache entry triggers one fresh request per lookup', () => { + const inputs = ['8.8.8.8', '1.1.1.1']; + for (const data of [[{ city: 'A' }, { city: 'B' }], []]) { + const app = loadScript({ cached: '[{"city":"stale"}]', data }); + const result = plain(app.context.IPGEO_BULK(inputs, 'city')); + if (data.length) assert.deepEqual(result, [['A'], ['B']]); + else assert.match(result[0][0], /expected one result per IP address/); + assert.equal(app.requests.length, 1); + assert.equal(app.writes.length, data.length ? 1 : 0); + } +}); + +test('invalid cache entries are removed even when the fresh reply cannot be cached', () => { + const key = 'fixture-cache-key'; + const inputs = ['8.8.8.8', '1.1.1.1']; + const good = [{ city: 'A' }, { city: 'B' }]; + for (const status of [200, 503]) { + for (const stale of ['{broken', JSON.stringify([{ message: key }]), JSON.stringify([null, { message: key }])]) { + const settings = { key, data: good }; + const app = loadScript(settings); + app.context.IPGEO_BULK(inputs, 'city'); + const cacheKey = app.writes[0].key; + app.entries.set(cacheKey, stale); + settings.status = status; + settings.data = status === 200 ? [{ city: 'x'.repeat(100001) }, good[1]] : { message: key }; + const result = app.context.IPGEO_BULK(inputs, 'city'); + if (status === 200) assert.equal(result[1][0], 'B'); + else assert.match(result[0][0], /503.*\[REDACTED\]/); + assert.equal(app.requests.length, 2); + assert.equal(app.entries.has(cacheKey), false); + } + } +}); + for (const setting of ['readError', 'writeError', 'cacheError']) { test(`${setting} does not discard successful GET or POST results`, () => { const single = loadScript({ [setting]: true });