Skip to content

fix: edge routing api - add llmo admin check#1861

Open
dipratap wants to merge 5 commits intomainfrom
cdn-header
Open

fix: edge routing api - add llmo admin check#1861
dipratap wants to merge 5 commits intomainfrom
cdn-header

Conversation

@dipratap
Copy link
Contributor

Please ensure your pull request adheres to the following guidelines:

  • make sure to link the related issues in this description. Or if there's no issue created, make sure you
    describe here the problem you're solving.
  • when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

If the PR is changing the API specification:

  • make sure you add a "Not implemented yet" note the endpoint description, if the implementation is not ready
    yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
  • make sure you add at least one example of the request and response.

If the PR is changing the API implementation or an entity exposed through the API:

  • make sure you update the API specification and the examples to reflect the changes.

If the PR is introducing a new audit type:

  • make sure you update the API specification with the type, schema of the audit result and an example

Related Issues

Thanks for contributing!

@github-actions
Copy link

This PR will trigger a patch release when merged.

@dipratap dipratap requested a review from ravverma February 25, 2026 10:57
@dipratap dipratap changed the title fix: edge routing api - send promise token as cookie fix: edge routing api - add llmo admin check Feb 25, 2026
@dipratap dipratap requested a review from nit23uec February 25, 2026 10:58
@dipratap
Copy link
Contributor Author

Created https://jira.corp.adobe.com/browse/LLMO-3333 for second iteration to shift sending promise token from header to cookie

@ravverma
Copy link
Contributor

Hey @dipratap , Please find review

Review – PR #1861


MUST (Critical) – Changes in this PR

1. JSDoc out of sync with implementation
File: src/controllers/llmo/llmo.js, line 1268

JSDoc still says "context.request for headers" but the code now uses context.pathInfo.headers. Update the JSDoc to match.

Suggested fix: Change to: @param {object} context - Request context (context.pathInfo.headers for headers)

2. Null-safe header access
File: src/controllers/llmo/llmo.js, line 1276

Switching from context.request?.headers?.get?.('x-promise-token') to context.pathInfo?.headers?.['x-promise-token'] is null-safe. Confirm pathInfo.headers is populated by the middleware and that header keys are normalized (e.g. lowercased) so 'x-promise-token' matches.


SHOULD (Recommended) – Changes in this PR

3. LLMO admin check
The new isLLMOAdministrator() check runs before the promise token check. That ordering is appropriate for fail-fast and avoids leaking token validation details.

4. Error message
"Only LLMO administrators can update edge optimize routing" is generic and does not expose internal details.

5. Cookie vs header (LLMO-3333)
For LLMO-3333 (cookie migration): prefer an HttpOnly cookie over a header for browser-originated requests (CSRF/XSS protection, no Referer leakage). Ensure the cookie is HttpOnly, Secure, and SameSite.


MUST (Critical) – Pre-existing ( not changed in this PR)

6. Sanitize error messages before returning to client
File: src/controllers/llmo/llmo.js, lines 1349, 1363

probeError.message and hostError.message are returned directly to the client and can leak internal details (hostnames, DNS errors, etc.). Use cleanupHeaderValue() from @adobe/helix-shared-utils before returning.

7. Avoid exposing environment name in client responses
File: src/controllers/llmo/llmo.js, lines 1279–1283

Returning env?.ENV (e.g. prod, stage, dev) in the response can help attackers infer deployment topology. Use a generic message such as "API is not available in this environment" instead.


SHOULD (Recommended) – Pre-existing / Out of scope

8. Token exchange error handling
The generic message "Authentication failed with upstream IMS service" is appropriate and does not leak internal implementation details.

Copy link
Contributor

@ravverma ravverma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes requested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants