From 716b447ca2a1859d6d3eeaf24bad282f8dde33ca Mon Sep 17 00:00:00 2001 From: Sahil Silare Date: Fri, 17 Jul 2026 11:50:31 +0530 Subject: [PATCH 1/3] feat(geo-experiment): presign insights rawDataUrl and drop the s3:// URI from the response --- src/controllers/suggestions.js | 47 ++++++++++++++++++++++++++++ test/controllers/suggestions.test.js | 37 ++++++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/src/controllers/suggestions.js b/src/controllers/suggestions.js index 8f3e775e43..211228e25c 100644 --- a/src/controllers/suggestions.js +++ b/src/controllers/suggestions.js @@ -182,6 +182,51 @@ async function postPlgSuggestionSkipAlert(site, opportunity, suggestion, context } } +// TTL for presigned impact-measurement raw-data URLs (7 days). +const INSIGHTS_PRESIGN_TTL_SECONDS = 60 * 60 * 24 * 7; + +/** + * Replace each analysis's `rawDataUrl` (an `s3://bucket/key` URI) with a presigned HTTPS URL so the + * UI can download the per-analysis detail blobs. The raw `s3://` URI is dropped from the response + * (not exposed); a presigned analysis carries `rawDataPresignedUrl` + expiry. + * Best-effort per analysis — on a presign failure the `s3://` URI is still dropped and the analysis + * simply has no download URL. + * + * @param {object} insights - ExperimentInsights object. + * @param {object} s3Ctx - context.s3 ({ s3Client, getSignedUrl, GetObjectCommand }). + * @param {object} log - logger. + * @returns {Promise} a new insights object with presigned analyses. + */ +async function presignInsightsRawData(insights, s3Ctx, log) { + const analyses = insights?.analyses; + if (!Array.isArray(analyses)) { + return insights; + } + const { s3Client, getSignedUrl, GetObjectCommand } = s3Ctx; + const expiresAt = new Date(Date.now() + (INSIGHTS_PRESIGN_TTL_SECONDS * 1000)).toISOString(); + const presignedAnalyses = await Promise.all(analyses.map(async (analysis) => { + const match = /^s3:\/\/([^/]+)\/(.+)$/.exec(analysis?.rawDataUrl || ''); + if (!match) { + return analysis; + } + const [, bucket, key] = match; + const result = { ...analysis }; + delete result.rawDataUrl; // never expose the raw s3:// URI + try { + result.rawDataPresignedUrl = await getSignedUrl( + s3Client, + new GetObjectCommand({ Bucket: bucket, Key: key }), + { expiresIn: INSIGHTS_PRESIGN_TTL_SECONDS }, + ); + result.rawDataPresignedUrlExpiresAt = expiresAt; + } catch (e) { + log.info(`[geo-experiment] Could not presign rawDataUrl ${analysis.rawDataUrl}: ${e.message}`); + } + return result; + })); + return { ...insights, analyses: presignedAnalyses }; +} + /** * Suggestions controller. * @param {object} ctx - Context of the request. @@ -2430,6 +2475,8 @@ function SuggestionsController(ctx, sqs, env) { ); const body = await response.Body.transformToString(); insights = JSON.parse(body); + // Presign each analysis's rawDataUrl so the UI can download the S3 detail blobs directly. + insights = await presignInsightsRawData(insights, context.s3, context.log); } catch (s3Error) { // Insights may not exist yet (e.g. impact measurement not yet complete) context.log.info(`[geo-experiment] Could not fetch insights for ${geoExperimentId}: ${s3Error.message}`); diff --git a/test/controllers/suggestions.test.js b/test/controllers/suggestions.test.js index 34ac0a3f7a..386c8faf92 100644 --- a/test/controllers/suggestions.test.js +++ b/test/controllers/suggestions.test.js @@ -9990,6 +9990,7 @@ describe('Suggestions Controller', () => { s3Client: { send: sandbox.stub() }, s3Bucket: 'test-bucket', GetObjectCommand: StubGetObjectCommand, + getSignedUrl: sandbox.stub().resolves('https://presigned.example/blob'), }; sandbox.stub(AccessControlUtil.prototype, 'hasAccess').resolves(true); @@ -10165,6 +10166,42 @@ describe('Suggestions Controller', () => { expect(body.insights).to.deep.equal(insightsPayload); }); + it('presigns each analysis rawDataUrl when includeInsights=true', async () => { + const insightsKey = `geo-experiments/${SITE_ID}/${GEO_EXP_ID}-insights.json`; + const insightsPayload = { + analyses: [ + { kind: 'cited_text', rawDataUrl: 's3://mystique-bucket/geo-experiments/x/cited_text.json' }, + { kind: 'url_presence' }, // no rawDataUrl → left untouched + ], + }; + mockGeoExperiment.getInsightsLocation = () => insightsKey; + context.s3.getSignedUrl = sandbox.stub().resolves('https://signed.example/cited_text.json'); + context.s3.s3Client.send.callsFake((command) => { + const payload = command.Key === insightsKey ? insightsPayload : []; + return Promise.resolve({ + Body: { transformToString: sandbox.stub().resolves(JSON.stringify(payload)) }, + }); + }); + const response = await suggestionsController.getGeoExperiment({ + ...context, + data: { includeInsights: 'true' }, + params: { siteId: SITE_ID, geoExperimentId: GEO_EXP_ID }, + }); + expect(response.status).to.equal(200); + const body = await response.json(); + const [cited, urlPresence] = body.insights.analyses; + // rawDataUrl presigned into an HTTPS URL; the raw s3:// URI is dropped from the response. + expect(cited.rawDataPresignedUrl).to.equal('https://signed.example/cited_text.json'); + expect(cited.rawDataPresignedUrlExpiresAt).to.be.a('string'); + expect(cited).to.not.have.property('rawDataUrl'); + // analysis without rawDataUrl is untouched. + expect(urlPresence).to.not.have.property('rawDataPresignedUrl'); + // presigned against the URL's OWN bucket/key (Mystique bucket), not the api-service bucket. + const signedCommand = context.s3.getSignedUrl.firstCall.args[1]; + expect(signedCommand.Bucket).to.equal('mystique-bucket'); + expect(signedCommand.Key).to.equal('geo-experiments/x/cited_text.json'); + }); + it('returns null insights and logs when insights S3 fetch fails', async () => { const insightsKey = `geo-experiments/${SITE_ID}/${GEO_EXP_ID}-insights.json`; mockGeoExperiment.getInsightsLocation = () => insightsKey; From 9e1d5063ce24ffd0de8a2f9d265c2669d74cbe02 Mon Sep 17 00:00:00 2001 From: Sahil Silare Date: Fri, 17 Jul 2026 12:07:51 +0530 Subject: [PATCH 2/3] refactor(geo-experiment): replace rawDataUrl in place with the presigned URL (no extra fields) --- src/controllers/suggestions.js | 21 +++++++++------------ test/controllers/suggestions.test.js | 11 +++++------ 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/src/controllers/suggestions.js b/src/controllers/suggestions.js index 211228e25c..1caee79405 100644 --- a/src/controllers/suggestions.js +++ b/src/controllers/suggestions.js @@ -186,16 +186,15 @@ async function postPlgSuggestionSkipAlert(site, opportunity, suggestion, context const INSIGHTS_PRESIGN_TTL_SECONDS = 60 * 60 * 24 * 7; /** - * Replace each analysis's `rawDataUrl` (an `s3://bucket/key` URI) with a presigned HTTPS URL so the - * UI can download the per-analysis detail blobs. The raw `s3://` URI is dropped from the response - * (not exposed); a presigned analysis carries `rawDataPresignedUrl` + expiry. - * Best-effort per analysis — on a presign failure the `s3://` URI is still dropped and the analysis - * simply has no download URL. + * Replace each analysis's `rawDataUrl` (an `s3://bucket/key` URI) in place with a presigned HTTPS URL + * so the UI can download the per-analysis detail blobs. Same field name — the `s3://` value is simply + * swapped for the presigned URL. Best-effort per analysis — on a presign failure the original + * `rawDataUrl` is left as-is. Analyses without a `rawDataUrl` are untouched. * * @param {object} insights - ExperimentInsights object. * @param {object} s3Ctx - context.s3 ({ s3Client, getSignedUrl, GetObjectCommand }). * @param {object} log - logger. - * @returns {Promise} a new insights object with presigned analyses. + * @returns {Promise} a new insights object with presigned rawDataUrls. */ async function presignInsightsRawData(insights, s3Ctx, log) { const analyses = insights?.analyses; @@ -203,26 +202,24 @@ async function presignInsightsRawData(insights, s3Ctx, log) { return insights; } const { s3Client, getSignedUrl, GetObjectCommand } = s3Ctx; - const expiresAt = new Date(Date.now() + (INSIGHTS_PRESIGN_TTL_SECONDS * 1000)).toISOString(); const presignedAnalyses = await Promise.all(analyses.map(async (analysis) => { const match = /^s3:\/\/([^/]+)\/(.+)$/.exec(analysis?.rawDataUrl || ''); if (!match) { return analysis; } const [, bucket, key] = match; - const result = { ...analysis }; - delete result.rawDataUrl; // never expose the raw s3:// URI try { - result.rawDataPresignedUrl = await getSignedUrl( + // Replace the s3:// rawDataUrl in place with a presigned HTTPS URL the browser can fetch. + const rawDataUrl = await getSignedUrl( s3Client, new GetObjectCommand({ Bucket: bucket, Key: key }), { expiresIn: INSIGHTS_PRESIGN_TTL_SECONDS }, ); - result.rawDataPresignedUrlExpiresAt = expiresAt; + return { ...analysis, rawDataUrl }; } catch (e) { log.info(`[geo-experiment] Could not presign rawDataUrl ${analysis.rawDataUrl}: ${e.message}`); + return analysis; } - return result; })); return { ...insights, analyses: presignedAnalyses }; } diff --git a/test/controllers/suggestions.test.js b/test/controllers/suggestions.test.js index 386c8faf92..46e28fef0b 100644 --- a/test/controllers/suggestions.test.js +++ b/test/controllers/suggestions.test.js @@ -10166,7 +10166,7 @@ describe('Suggestions Controller', () => { expect(body.insights).to.deep.equal(insightsPayload); }); - it('presigns each analysis rawDataUrl when includeInsights=true', async () => { + it('replaces each analysis rawDataUrl with a presigned URL when includeInsights=true', async () => { const insightsKey = `geo-experiments/${SITE_ID}/${GEO_EXP_ID}-insights.json`; const insightsPayload = { analyses: [ @@ -10190,12 +10190,11 @@ describe('Suggestions Controller', () => { expect(response.status).to.equal(200); const body = await response.json(); const [cited, urlPresence] = body.insights.analyses; - // rawDataUrl presigned into an HTTPS URL; the raw s3:// URI is dropped from the response. - expect(cited.rawDataPresignedUrl).to.equal('https://signed.example/cited_text.json'); - expect(cited.rawDataPresignedUrlExpiresAt).to.be.a('string'); - expect(cited).to.not.have.property('rawDataUrl'); + // rawDataUrl is replaced in place with the presigned HTTPS URL (no new fields, no s3://). + expect(cited.rawDataUrl).to.equal('https://signed.example/cited_text.json'); + expect(cited).to.not.have.property('rawDataPresignedUrl'); // analysis without rawDataUrl is untouched. - expect(urlPresence).to.not.have.property('rawDataPresignedUrl'); + expect(urlPresence).to.not.have.property('rawDataUrl'); // presigned against the URL's OWN bucket/key (Mystique bucket), not the api-service bucket. const signedCommand = context.s3.getSignedUrl.firstCall.args[1]; expect(signedCommand.Bucket).to.equal('mystique-bucket'); From 02ee17347a064f6cfa01daa1e9d92bfddfcaf55e Mon Sep 17 00:00:00 2001 From: Sahil Silare Date: Fri, 17 Jul 2026 12:21:32 +0530 Subject: [PATCH 3/3] test(geo-experiment): cover no-analyses and presign-failure branches --- test/controllers/suggestions.test.js | 45 ++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/test/controllers/suggestions.test.js b/test/controllers/suggestions.test.js index 46e28fef0b..6ad0a9b216 100644 --- a/test/controllers/suggestions.test.js +++ b/test/controllers/suggestions.test.js @@ -10201,6 +10201,51 @@ describe('Suggestions Controller', () => { expect(signedCommand.Key).to.equal('geo-experiments/x/cited_text.json'); }); + it('returns insights unchanged when it has no analyses array', async () => { + const insightsKey = `geo-experiments/${SITE_ID}/${GEO_EXP_ID}-insights.json`; + const insightsPayload = { version: '1' }; // no analyses -> presign is a no-op + mockGeoExperiment.getInsightsLocation = () => insightsKey; + context.s3.s3Client.send.callsFake((command) => { + const payload = command.Key === insightsKey ? insightsPayload : []; + return Promise.resolve({ + Body: { transformToString: sandbox.stub().resolves(JSON.stringify(payload)) }, + }); + }); + const response = await suggestionsController.getGeoExperiment({ + ...context, + data: { includeInsights: 'true' }, + params: { siteId: SITE_ID, geoExperimentId: GEO_EXP_ID }, + }); + expect(response.status).to.equal(200); + const body = await response.json(); + expect(body.insights).to.deep.equal(insightsPayload); + expect(context.s3.getSignedUrl.called).to.equal(false); + }); + + it('leaves rawDataUrl unchanged and logs when presigning fails', async () => { + const insightsKey = `geo-experiments/${SITE_ID}/${GEO_EXP_ID}-insights.json`; + const rawUri = 's3://mystique-bucket/geo-experiments/x/cited_text.json'; + const insightsPayload = { analyses: [{ kind: 'cited_text', rawDataUrl: rawUri }] }; + mockGeoExperiment.getInsightsLocation = () => insightsKey; + context.s3.getSignedUrl = sandbox.stub().rejects(new Error('presign boom')); + context.s3.s3Client.send.callsFake((command) => { + const payload = command.Key === insightsKey ? insightsPayload : []; + return Promise.resolve({ + Body: { transformToString: sandbox.stub().resolves(JSON.stringify(payload)) }, + }); + }); + const response = await suggestionsController.getGeoExperiment({ + ...context, + data: { includeInsights: 'true' }, + params: { siteId: SITE_ID, geoExperimentId: GEO_EXP_ID }, + }); + expect(response.status).to.equal(200); + const body = await response.json(); + // presign failed → original s3:// rawDataUrl left untouched, and the failure is logged. + expect(body.insights.analyses[0].rawDataUrl).to.equal(rawUri); + expect(context.log.info.calledWithMatch(/Could not presign rawDataUrl/)).to.equal(true); + }); + it('returns null insights and logs when insights S3 fetch fails', async () => { const insightsKey = `geo-experiments/${SITE_ID}/${GEO_EXP_ID}-insights.json`; mockGeoExperiment.getInsightsLocation = () => insightsKey;