feat(geo-experiment): presign insights rawDataUrl for UI download#2844
feat(geo-experiment): presign insights rawDataUrl for UI download#2844ssilare-adobe wants to merge 5 commits into
Conversation
…URI from the response
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
GitHub Actions
Inactive
There was a problem hiding this comment.
Hey @ssilare-adobe,
Verdict: Request changes - two issues to address before merge.
Complexity: MEDIUM - small diff; API surface change (response shape modification on a GET endpoint).
Changes: Presigns each geo-experiment analysis's rawDataUrl (s3://) into a browser-fetchable HTTPS URL and strips the raw URI from the response (2 files).
Must fix before merge
- [Important] 7-day presigned URL TTL likely exceeds Lambda session token lifetime -
src/controllers/suggestions.js:184(details inline) - [Important] Missing test for the per-analysis presign failure path -
test/controllers/suggestions.test.js(details inline)
Non-blocking (3): minor issues and suggestions
- nit: Raw
s3://URI is logged at info level on presign failure, partially undermining the goal of never exposing it -src/controllers/suggestions.js:218. Considerlog.debugwith just the analysis kind. - nit:
expiresAtis computed once beforePromise.alland is an approximation of the real AWS-side expiry, not an authoritative value -src/controllers/suggestions.js:206. Worth a brief inline comment. - suggestion: Add a bucket allowlist check before presigning as defense-in-depth, so a future pipeline bug writing an unexpected bucket into the insights JSON cannot grant access to unintended S3 objects -
src/controllers/suggestions.js:210.
Note: Recommend a human read before merge - possible divergence from the documented design (heuristic, flagged by the conventions reviewer: response shape changes without corresponding OpenAPI spec update per the repo's "Specification Sync" rule).
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 44s | Cost: $3.94 | Commit: 716b447ca2a1859d6d3eeaf24bad282f8dde33ca
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| @@ -182,6 +182,51 @@ async function postPlgSuggestionSkipAlert(site, opportunity, suggestion, context | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
issue (blocking): 7-day presigned URL TTL may exceed Lambda role session token lifetime.
INSIGHTS_PRESIGN_TTL_SECONDS is 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 the expiresIn value. 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 though rawDataPresignedUrlExpiresAt claims it is still valid.
How to fix: Either:
- Verify the Lambda role's
MaxSessionDurationsupports 7 days (unlikely for most Lambda setups), or - Reduce the TTL to something achievable (e.g. 1-4 hours) and have the UI re-request the endpoint for a fresh URL on demand.
If the team has confirmed the IAM configuration supports a 7-day session, a comment noting that decision would prevent future confusion.
|
|
||
| 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.
issue (blocking): Missing test for the per-analysis presign failure path.
The catch block in presignInsightsRawData has specific behavior: it strips rawDataUrl, does NOT set rawDataPresignedUrl or rawDataPresignedUrlExpiresAt, and logs at info level. This branch is untested.
How to fix: Add a test where context.s3.getSignedUrl rejects for one analysis (e.g. the first) while succeeding for another. Assert:
- Response is still 200.
- The failing analysis has neither
rawDataUrlnorrawDataPresignedUrl. - A non-failing analysis in the same payload IS presigned normally.
This locks the best-effort contract against regressions.
MysticatBot
added
ai-reviewed
…ned URL (no extra fields)
|
This PR will trigger a minor release when merged. |
MysticatBot
left a comment
MysticatBot
left a comment
There was a problem hiding this comment.
Hey @ssilare-adobe,
Verdict: Request changes - prior blocking findings remain unresolved; one new defense-in-depth item added.
Complexity: MEDIUM - small diff; API surface change (response value transformation on a GET endpoint).
Changes: Simplifies presigning to replace rawDataUrl in place (no new fields) and preserves the original URI on failure instead of stripping it (2 files).
Note: Recommend a human read before merge - possible divergence from the documented design (heuristic, flagged by the conventions reviewer: response value format change without corresponding OpenAPI spec update per the repo's "Specification Sync" rule). The bot review is a complement to, not a replacement for, a human read here.
Note: CI checks are currently failing - resolve before merge.
Must fix before merge
- [Important] 7-day presigned URL TTL exceeds Lambda credential session lifetime -
src/controllers/suggestions.js:186(details inline) - [Important] No test for presign failure path (behavior changed this commit, still uncovered) -
test/controllers/suggestions.test.js(details inline) - [Important] No bucket allowlist before presigning - defense-in-depth against confused-deputy -
src/controllers/suggestions.js:208(details inline)
Non-blocking (2): minor issues and suggestions
- nit: Full s3:// URI logged at info level on presign failure - consider
log.debugor redacting the key to just the analysis kind -src/controllers/suggestions.js:218 - suggestion: Update OpenAPI spec examples/description for rawDataUrl if they currently reference the s3:// format -
docs/openapi/
Previously flagged, now resolved
- Response shape simplified: in-place field replacement instead of new fields (rawDataPresignedUrl + rawDataPresignedUrlExpiresAt removed)
- Failure path no longer strips rawDataUrl (preserves original s3:// URI for graceful degradation)
| } | ||
|
|
||
| // 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.
issue (blocking): 7-day presigned URL TTL will not survive Lambda credential session expiry.
INSIGHTS_PRESIGN_TTL_SECONDS is set to 604,800s (7 days). AWS presigned URLs generated using temporary credentials (Lambda execution role via STS) are capped at the remaining session-token lifetime, regardless of the expiresIn value. Lambda credentials are rotated every 1-12 hours (depending on MaxSessionDuration).
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):
- Reduce TTL to a value within the role session duration (e.g. 3600s) and have the UI re-fetch the endpoint for a fresh URL on demand.
- If 7-day validity is a hard product requirement, presign using CloudFront signed URLs with a dedicated key pair, or a lightweight proxy endpoint.
- If the team has confirmed the current behavior is acceptable (URLs are consumed immediately, not stored), add a comment documenting that the effective expiry is
min(TTL, role-session-remaining).
| 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.
issue (blocking): No bucket allowlist before presigning - confused-deputy risk.
The regex extracts any bucket from rawDataUrl and presigns against it. While the insights JSON is sourced from an internal pipeline (not user input), this creates a confused-deputy vector if the pipeline data is ever poisoned or a future code path writes attacker-influenced rawDataUrl values. The Lambda role's s3:GetObject permission becomes the blast radius.
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.
| 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.
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 getSignedUrl rejection. This behavior changed in this commit (previously stripped rawDataUrl entirely). Codecov confirms 4 lines uncovered. Without a test pinning this behavior, a future refactor can silently regress the graceful-degradation contract.
How to fix: Add a test where getSignedUrl rejects for one analysis while resolving for another:
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);
});
There was a problem hiding this comment.
Hey @ssilare-adobe,
Verdict: Request changes - prior blocking findings remain unresolved; one new defense-in-depth item added.
Complexity: MEDIUM - small diff; API surface change (response value transformation on a GET endpoint).
Changes: Simplifies presigning to replace rawDataUrl in place (no new fields) and preserves the original URI on failure instead of stripping it (2 files).
Note: Recommend a human read before merge - possible divergence from the documented design (heuristic, flagged by the conventions reviewer: response value format change without corresponding OpenAPI spec update per the repo's "Specification Sync" rule). The bot review is a complement to, not a replacement for, a human read here.
Note: CI checks are currently failing - resolve before merge.
Must fix before merge
- [Important] 7-day presigned URL TTL exceeds Lambda credential session lifetime -
src/controllers/suggestions.js:186(details inline) - [Important] No test for presign failure path (behavior changed this commit, still uncovered) -
test/controllers/suggestions.test.js(details inline) - [Important] No bucket allowlist before presigning - defense-in-depth against confused-deputy -
src/controllers/suggestions.js:208(details inline)
Non-blocking (2): minor issues and suggestions
- nit: Full s3:// URI logged at info level on presign failure - consider
log.debugor redacting the key to just the analysis kind -src/controllers/suggestions.js:218 - suggestion: Update OpenAPI spec examples/description for rawDataUrl if they currently reference the s3:// format -
docs/openapi/
Previously flagged, now resolved
- Response shape simplified: in-place field replacement instead of new fields (rawDataPresignedUrl + rawDataPresignedUrlExpiresAt removed)
- Failure path no longer strips rawDataUrl (preserves original s3:// URI for graceful degradation)
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 9s | Cost: $5.21 | Commit: 9e1d5063ce24ffd0de8a2f9d265c2669d74cbe02
If this code review was useful, please react with 👍. Otherwise, react with 👎.
GitHub Actions
Inactive
There was a problem hiding this comment.
Hey @ssilare-adobe,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Approve - clean implementation, well-tested, follows existing patterns.
Complexity: MEDIUM - borderline-small diff; API surface response value transformation.
Changes: Replaces each geo-experiment analysis's s3:// rawDataUrl with a presigned HTTPS URL so the UI can download detail blobs directly (2 files).
Non-blocking (2): minor issues and suggestions
- suggestion: Move the
presignInsightsRawDatacall outside the existing S3 fetch try/catch (or into its own try/catch) so a structural failure during presigning is not misattributed as "Could not fetch insights" in the outer catch -src/controllers/suggestions.js:2474 - suggestion: Update the OpenAPI spec for the geo-experiment endpoint to document that
rawDataUrlvalues ininsights.analyses[]are presigned HTTPS URLs (7-day TTL, best-effort) rather than raw s3:// URIs -docs/openapi/
Previously flagged, now resolved
- Presign failure test added (graceful fallback path covered)
- Response shape simplified to in-place field replacement (no new fields)
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 10s | Cost: $4.11 | Commit: 2b73bd71e1e4a31355285ee867aa4d691ccbd123
If this code review was useful, please react with 👍. Otherwise, react with 👎.
MysticatBot
removed
the
needs-human-review
2 participants
What
On
GET /sites/:siteId/geo-experiments/:id?includeInsights=true, replace each impact-measurementanalysis's
rawDataUrlin place with a presigned HTTPS URL so the UI can download the per-analysisdetail blobs from S3. Same field name — the
s3://bucket/keyvalue is simply swapped for abrowser-fetchable presigned URL.
Builds on the existing
includeInsightssupport (already onmain), which inlines the insightsobject read from
insightsLocation.How
New helper
presignInsightsRawData(insights, s3Ctx, log)insuggestions.js:analyses[].rawDataUrl(s3://bucket/key), parses bucket+key and presigns viacontext.s3.getSignedUrl(7-day TTL), against the URL's own bucket (the Mystique assetsbucket), not the api-service bucket.
rawDataUrlin place with the presigned URL — no new fields.the original
rawDataUrlis left as-is. Analyses without arawDataUrlare untouched.Response shape (per analysis with detail):
{ "kind": "cited_text", "summary": { ... }, "rawDataUrl": "https://…presigned…" } // was s3://…, now a presigned HTTPS URLWhy
The UI reads the experiment detail and needs a browser-fetchable URL for the per-analysis detail
(e.g. prompt_performance "download CSV", cited_text / url_presence detail).
rawDataUrlis ans3://URI the browser can't fetch; presigning converts it to an HTTPS URL in place.Tests
test/controllers/suggestions.test.js— asserts eachrawDataUrlis replaced with the presigned URL(no extra fields), analyses without a
rawDataUrlare untouched, and the presign targets the URL'sown bucket/key. 15/15 getGeoExperiment tests pass; eslint clean.
The api-service Lambda role needs
s3:GetObjecton the Mystique assets bucket (whererawDataUrlpoints) for the presigned URLs to resolve when the UI fetches them. Without that grantthe presigned URL 403s.