-
Notifications
You must be signed in to change notification settings - Fork 16
feat(geo-experiment): presign insights rawDataUrl for UI download #2844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
716b447
9e1d506
02ee173
dbde77b
2b73bd7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,6 +182,48 @@ 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; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (blocking): 7-day presigned URL TTL will not survive Lambda credential session expiry.
Result: the presigned URL silently becomes invalid (403 ExpiredToken) well before 7 days. The UI caller has no indication the URL expired early. How to fix (pick one):
ssilare-adobe marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * 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<object>} a new insights object with presigned rawDataUrls. | ||
| */ | ||
| async function presignInsightsRawData(insights, s3Ctx, log) { | ||
| const analyses = insights?.analyses; | ||
| if (!Array.isArray(analyses)) { | ||
| return insights; | ||
| } | ||
| const { s3Client, getSignedUrl, GetObjectCommand } = s3Ctx; | ||
| const presignedAnalyses = await Promise.all(analyses.map(async (analysis) => { | ||
| const match = /^s3:\/\/([^/]+)\/(.+)$/.exec(analysis?.rawDataUrl || ''); | ||
| if (!match) { | ||
| return analysis; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (blocking): No bucket allowlist before presigning - confused-deputy risk. The regex extracts any bucket from How to fix: Add a bucket allowlist check (one constant, trivial cost): const ALLOWED_PRESIGN_BUCKETS = new Set([process.env.MYSTIQUE_ASSETS_BUCKET]);
if (!ALLOWED_PRESIGN_BUCKETS.has(bucket)) {
log.warn(`[geo-experiment] Refused to presign unexpected bucket: ${bucket}`);
return analysis;
}This ensures that even if the insights JSON is compromised, the presigning cannot be used to generate download URLs for unintended S3 objects.
ssilare-adobe marked this conversation as resolved.
|
||
| } | ||
| const [, bucket, key] = match; | ||
| try { | ||
| // 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 }, | ||
| ); | ||
| return { ...analysis, rawDataUrl }; | ||
| } catch (e) { | ||
| log.info(`[geo-experiment] Could not presign rawDataUrl ${analysis.rawDataUrl}: ${e.message}`); | ||
| return analysis; | ||
| } | ||
| })); | ||
| return { ...insights, analyses: presignedAnalyses }; | ||
| } | ||
|
|
||
| /** | ||
| * Suggestions controller. | ||
| * @param {object} ctx - Context of the request. | ||
|
|
@@ -2430,6 +2472,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}`); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,86 @@ describe('Suggestions Controller', () => { | |
| expect(body.insights).to.deep.equal(insightsPayload); | ||
| }); | ||
|
|
||
| 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: [ | ||
| { 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 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('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'); | ||
| expect(signedCommand.Key).to.equal('geo-experiments/x/cited_text.json'); | ||
| }); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (blocking): Missing test for the per-analysis presign failure path. The catch block (line ~218 in suggestions.js) now preserves the original analysis unchanged on How to fix: Add a test where it('preserves original rawDataUrl when presigning fails', async () => {
// ... setup with two analyses ...
context.s3.getSignedUrl = sandbox.stub()
.onFirstCall().rejects(new Error('AccessDenied'))
.onSecondCall().resolves('https://signed.example/second.json');
const response = await suggestionsController.getGeoExperiment(...);
const [failing, succeeding] = body.insights.analyses;
// Failing analysis retains original s3:// URL
expect(failing.rawDataUrl).to.equal('s3://bucket/failing-key');
// Succeeding analysis gets the presigned URL
expect(succeeding.rawDataUrl).to.equal('https://signed.example/second.json');
expect(response.status).to.equal(200);
});
ssilare-adobe marked this conversation as resolved.
|
||
| 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; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (blocking): Missing test for the per-analysis presign failure path. The How to fix: Add a test where
This locks the best-effort contract against regressions. |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (blocking): 7-day presigned URL TTL may exceed Lambda role session token lifetime.
INSIGHTS_PRESIGN_TTL_SECONDSis set to 604,800s (7 days). AWS presigned URLs generated using temporary credentials (which Lambda roles use) are capped at the remaining session token lifetime, regardless of theexpiresInvalue. Lambda credentials typically last 6-15 hours. If the role session is shorter than 7 days, users revisiting the UI after that window will get silent 403s from the presigned URL - even thoughrawDataPresignedUrlExpiresAtclaims it is still valid.How to fix: Either:
MaxSessionDurationsupports 7 days (unlikely for most Lambda setups), orIf the team has confirmed the IAM configuration supports a 7-day session, a comment noting that decision would prevent future confusion.