Conversation
|
This PR will trigger a minor release when merged. |
3c2b123 to
be0c98c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Rename PR title to trigger release. |
PR #2197 Review:
|
| Area | Verdict |
|---|---|
| Code correctness | |
| Security | |
| Test coverage | Partial — missing opportunities controller tests |
| Style/conventions | Minor — empty PR description |
Recommendation: The core logic of isViewAsTrialRequest is clean and well-tested, but the blast radius of short-circuiting getIsSummitPlgEnabled globally needs to be scoped down to avoid unintended writes and to add an authorization check.
alinarublea
left a comment
There was a problem hiding this comment.
See review comment above.
PR Review Comments Addressed
|
alinarublea
left a comment
There was a problem hiding this comment.
Re-review — PR #2197: feat: view trial toggle
Previous review: Changes requested (alinarublea, 2026-04-16)
Author response: Tests added for opportunities controller, fragile test fixed, grant writes justified as intentional
Issues Addressed Since Last Review
Issue #3 (Missing opportunities controller tests) — Resolved. Three tests added covering getByID PLG paths.
Issue #6 (Fragile test coupling) — Resolved. The author now mocks getIsSummitPlgEnabled explicitly via esmock.
Outstanding Critical Issues
1. [Blocking] x-view-as-trial is not in the CORS Access-Control-Allow-Headers list
src/index.js:128 and src/index.js:178 define the allowed headers for CORS preflight:
Content-Type, Authorization, x-api-key, x-ims-org-id, x-client-type, x-import-api-key, x-trigger-audits, x-requested-with, origin, accept
x-view-as-trial is absent. Since the API at spacecat.experiencecloud.live is cross-origin from the sites-optimizer-ui, browsers will reject the preflight and block the request entirely. This header will never reach the server from the UI. This is a functional blocker — the feature cannot work without updating the CORS config.
2. [Security] No server-side authorization gate — unchanged
The author's response did not address this. Any authenticated user (customers, not just Adobe internal) can send x-view-as-trial: true via non-browser clients (curl, Postman, scripts). This:
- Short-circuits
getIsSummitPlgEnabledtotrue, bypassing Configuration and Entitlement checks - Triggers
grantSuggestionsForOpportunitywhich writes real grant records and consumes token capacity - Applies PLG-style filtering to a site that may not be enrolled in PLG
The x-client-type check is not a security boundary — it's an unverified header. This needs a server-side gate such as accessControlUtil.hasAdminAccess().
3. [Design] Grant writes from a simulation toggle — partially addressed
The author explains: "Grant writes in the read path are intentional — without them filterByGrantStatus returns empty results, breaking the simulation."
This is a valid functional argument, but the consequence is that a "view" toggle mutates production state:
grantSuggestionsForOpportunitycreatesSuggestionGrantandTokenrecords- These records persist after the toggle is turned off
- If the site is later genuinely enrolled in PLG, these pre-existing grants could interfere with the real token cycle (
handleExistingTokenCycleingrant-suggestions-handler.js:359revokes and re-grants based on existing token state)
The auto-deploy concern — the author says "deploy button is disabled in the UI when isViewAsTrial is true." This is a client-side-only defense. The server still accepts x-view-as-trial: true + a deploy request and returns 403. If a power user or API consumer isn't using the UI, they get a confusing 403 with no indication it's caused by a simulation header. At minimum, the 403 message at suggestions.js:1105 should mention the trial simulation if isViewAsTrialRequest is active.
Recommendation
Before merging:
- Add
x-view-as-trialto CORS allowed headers insrc/index.js(lines 128 and 178) — otherwise the feature is dead on arrival - Gate the toggle behind admin access or at minimum verify the user is internal (IMS org check, admin scope, etc.)
- Consider a cleanup mechanism for grant records created during simulation, or document that simulation grants are permanent
The test improvements are solid, and the core utility function is well-structured. The remaining concerns are about security boundaries and operational safety, not code quality.
🤖 Generated with Claude Code
PR Review Comments Addressed
|
alinarublea
left a comment
There was a problem hiding this comment.
Re-review #3 — PR #2197: feat: view trial toggle
Previous reviews: Changes requested (2x), citing CORS blocker, security gate, write side-effects
Latest commit: f9b9986 — "Fix PR review comments"
Issues Addressed Since Last Review
CORS blocker — Resolved. x-view-as-trial added to both Access-Control-Allow-Headers locations in src/index.js (lines 128 and 191). Test updated in test/index.test.js. The feature will now work from the browser.
403 message clarity — Resolved. suggestions.js:1105-1108 now appends (trial simulation mode is active - disable the View as Trial toggle to deploy) when isViewAsTrialRequest is true. Real PLG users are unaffected since they don't send the header. Test added to verify both cases (with and without the hint).
Test coverage — Solid. 24 new test assertions across suggestions, opportunities, and utils covering:
- Grant triggering and filtering with trial header
- End-to-end grant + filter flow
getByStatuswith trial header- Trial hint in 403 message (positive and negative)
isViewAsTrialRequestedge cases (wrong client, absent header, false value, undefined context)
Re-assessment of Security Gate Concern
On reflection, my previous security concern was overstated. Tracing every call site where getIsSummitPlgEnabled returns true:
| Call site | Effect | More or less access? |
|---|---|---|
filterForSummitPlg (opportunities) |
Filters to only broken-backlinks, cwv, alt-text |
Less — hides opportunities |
filterByGrantStatus (suggestions) |
Filters to only granted suggestions | Less — hides suggestions |
grantSuggestionsForOpportunity |
Creates grant/token records | Write side-effect (see below) |
| Auto-deploy guard (suggestions:1102) | Blocks deploy of non-granted suggestions | Less — blocks actions |
getByID guard (suggestions:531) |
Returns 404 for non-granted suggestions | Less — hides data |
A malicious user sending x-view-as-trial: true would see less data and have fewer capabilities — not more. This is not a privilege escalation vector. The only effect is data pollution from grant writes, which the author has justified as necessary for the simulation to work.
The author's response that the SitesInternal restriction in the ASO UI already gates who can access the toggle is a reasonable layered defense argument. Combined with the fact that the header is self-penalizing (restricts, doesn't expand), the risk is acceptably low.
One Remaining Observation (Non-blocking)
Grant record persistence — When a user toggles "View as Trial" on and then off, the SuggestionGrant and Token records created during the simulation remain in the database. If the site is later genuinely enrolled in PLG, handleExistingTokenCycle in grant-suggestions-handler.js will encounter these pre-existing records. This should be fine (the function revokes and re-grants), but it's worth documenting in the feature's runbook that simulation-created grants are permanent and will be reconciled on real enrollment.
Summary
| Category | Status |
|---|---|
| CORS | Fixed |
| 403 UX | Fixed |
| Security | Acceptable risk — header is self-penalizing |
| Test coverage | Comprehensive |
| CI | All checks passing |
Recommendation: Ready to merge. Clean up the PR description and squash the 31 commits before merging.
🤖 Generated with Claude Code
Issues addressed — CORS fix, 403 message improvement, security risk re-assessed as acceptable.
alinarublea
left a comment
There was a problem hiding this comment.
All previously raised issues have been addressed:
- CORS:
x-view-as-trialadded to allowed headers ✅ - 403 UX: Trial simulation hint added to the error message ✅
- Security: Re-assessed — the header is self-penalizing (restricts access, doesn't expand it), making the risk acceptably low ✅
- Tests: Comprehensive coverage across all affected controllers ✅
- CI: All checks passing ✅
See detailed re-review comment above. Recommend squashing the 31 commits before merge.
🤖 Generated with Claude Code
# [1.446.0](v1.445.3...v1.446.0) (2026-04-17) ### Features * view trial toggle ([#2197](#2197)) ([c99aea7](c99aea7))
|
🎉 This PR is included in version 1.446.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Please ensure your pull request adheres to the following guidelines:
WHAT CHANGED
Screen.Recording.2026-04-13.at.8.00.17.PM.mov
describe here the problem you're solving.
If the PR is changing the API specification:
yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
If the PR is changing the API implementation or an entity exposed through the API:
If the PR is introducing a new audit type:
Related Issues
Thanks for contributing!