Skip to content

fix: migrate preflight and autofix to session token validation#1909

Merged
ravverma merged 3 commits intomainfrom
migrate-to-session-token
Mar 5, 2026
Merged

fix: migrate preflight and autofix to session token validation#1909
ravverma merged 3 commits intomainfrom
migrate-to-session-token

Conversation

@ravverma
Copy link
Contributor

@ravverma ravverma commented Mar 5, 2026

Summary

Migrate promise token resolution from IMS-based authentication to session-based authentication across autofix and preflight flows. The x-promise-token header is now the preferred source for the promise token, with IMS token creation retained as a backward-compatible fallback.

Changes

  • src/controllers/suggestions.js — Updated autofixSuggestions to check for x-promise-token request header first. If present and non-empty, the header value is used directly. Otherwise, falls back to the existing IMS token flow. Added JSDoc for autofixSuggestions.
  • src/controllers/preflight.js — Updated createPreflightJob with the same header-first, IMS-fallback approach for promise token resolution. Added JSDoc documenting the new behavior.
  • test/controllers/suggestions.test.js — Added tests for header token usage, IMS fallback on missing header, and IMS fallback on empty header.
  • test/controllers/preflight.test.js — Added tests for header token usage, IMS fallback on missing header, and IMS fallback on empty header.

Motivation

Session-based authentication passes the promise token via the x-promise-token header from the client, removing the need for server-side IMS token creation in the new flow. The IMS fallback ensures backward compatibility for existing callers that do not yet provide the header.

Test Plan

  • Existing unit tests pass (no regressions)
  • New tests cover header-present, header-absent, and header-empty scenarios for both autofixSuggestions and createPreflightJob
  • Non-promise-based sites are unaffected — header check is gated behind authoring type validation

@codecov
Copy link

codecov bot commented Mar 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@solaris007 solaris007 added the bug Something isn't working label Mar 5, 2026
Copy link
Member

@solaris007 solaris007 left a comment

Choose a reason for hiding this comment

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

Hey @ravverma,

Thanks for reworking this from #1887 - the backward-compatible approach is exactly what was needed. No new endpoint, no code duplication, clean fallback. The structural approach is solid.

Two things need fixing before this can merge:

1. Encryption bypass (critical)

When the IMS path creates a promise token (getIMSPromiseToken -> ImsPromiseClient.getPromiseToken), it encrypts the token via AUTOFIX_CRYPT_SECRET/AUTOFIX_CRYPT_SALT before placing it on SQS. The consumer (exchangeToken) then decrypts before exchanging with IMS.

The header path bypasses this:

promiseTokenResponse = { promise_token: headerToken };

The raw token goes to SQS unencrypted. If the consumer environment has AUTOFIX_CRYPT_SECRET/AUTOFIX_CRYPT_SALT set, it will try to decrypt() a plaintext token and fail - meaning every session-token caller will break at exchange time.

Options:

  • (a) Encrypt the header token server-side before SQS (matching IMS path behavior)
  • (b) Add a flag to the SQS message (e.g., encrypted: false) so the consumer knows to skip decryption
  • (c) If encryption is intentionally not needed for session tokens, document it and ensure the consumer handles both cases

This was flagged in the #1887 review and is still unresolved.

2. Broken spy assertion in suggestions.test.js:3309

const getPromiseTokenStub = imsPromiseClient.createFrom().getPromiseToken;

createFrom() returns a new object with a new stub on every call. This captures an orphan stub that is never wired to anything. The expect(getPromiseTokenStub).to.not.have.been.called at line 3336 will always pass regardless of whether IMS was actually called - it provides zero signal. The equivalent preflight test handles this correctly by stubbing getIMSPromiseToken at the module boundary via esmock.

Other items:

  • The OpenAPI specs (docs/openapi/) should document the x-promise-token header on the affected endpoints - consumers need to discover it through the spec.
  • PR description mentions autofixSuggestionsV2 but this function doesn't exist in the codebase.
  • preflight.test.js:721 asserts siteId: mockSite.getId() but the test overrides the site to aemCsSite - passes only by coincidence (both return 'test-site-123').

On the positive side: the token shape concern from #1887 is resolved - downstream consumer only reads .promise_token, so the reduced shape is safe. The inline pattern is clean, hasText is the right guard, and the test coverage for header-present/absent/empty is the right set of cases.

@github-actions
Copy link

github-actions bot commented Mar 5, 2026

This PR will trigger a patch release when merged.

@ravverma
Copy link
Contributor Author

ravverma commented Mar 5, 2026

thanks @solaris007

Summary

Migrate promise token resolution from IMS-based authentication to session-based authentication. The x-promise-token request header is now the preferred source for the promise token, with IMS token creation retained as a backward-compatible fallback.

Changes

  • src/controllers/suggestions.js — Updated autofixSuggestions to check for x-promise-token header first; falls back to IMS when absent or empty. Added JSDoc.
  • src/controllers/preflight.js — Updated createPreflightJob with the same header-first, IMS-fallback approach. Added JSDoc.
  • docs/openapi/parameters.yaml — Added xPromiseToken optional header parameter definition.
  • docs/openapi/site-opportunities.yaml — Referenced xPromiseToken on the autofix endpoint.
  • docs/openapi/preflight-api.yaml — Referenced xPromiseToken on the preflight jobs endpoint.
  • test/controllers/suggestions.test.js — Added tests for header token usage, IMS fallback on missing header, and IMS fallback on empty header. Fixed broken spy assertion that was capturing an orphan stub instead of mocking getIMSPromiseToken at the module boundary via esmock.
  • test/controllers/preflight.test.js — Added tests for header token usage, IMS fallback on missing header, and IMS fallback on empty header. Fixed siteId assertion to reference the correct test entity (aemCsSite) instead of mockSite.

Motivation

Session-based authentication passes the promise token via the x-promise-token header from the client, removing the need for server-side IMS token creation in the new flow. The IMS fallback ensures backward compatibility for callers that do not yet provide the header. The header check is gated behind authoring type validation, so non-promise-based sites are unaffected.

Test Plan

  • Existing unit tests pass (no regressions)
  • New tests cover header-present, header-absent, and header-empty scenarios for both autofixSuggestions and createPreflightJob
  • Spy assertion in suggestions test correctly mocks getIMSPromiseToken at the module boundary
  • Non-promise-based sites are unaffected — header check is gated behind authoring type validation
  • OpenAPI specs validated (npm run docs:lint passes)

and about the Encryption bypass

  1. Encryption bypass (critical)

When the IMS path creates a promise token (getIMSPromiseToken -> ImsPromiseClient.getPromiseToken), it encrypts the token via AUTOFIX_CRYPT_SECRET/AUTOFIX_CRYPT_SALT before placing it on SQS. The consumer (exchangeToken) then decrypts before exchanging with IMS.

The header path bypasses this:

promiseTokenResponse = { promise_token: headerToken };
The raw token goes to SQS unencrypted. If the consumer environment has AUTOFIX_CRYPT_SECRET/AUTOFIX_CRYPT_SALT set, it will try to decrypt() a plaintext token and fail - meaning every session-token caller will break at exchange time.

It is little misunderstood, the token getting from UI would be obtained from /auth/promise emitter end point and there token is already encrypted.
https://github.com/adobe/spacecat-auth-service/blob/main/src/ims/promise.js#L75

@ravverma ravverma requested a review from solaris007 March 5, 2026 09:47
Copy link
Member

@solaris007 solaris007 left a comment

Choose a reason for hiding this comment

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

Hey @ravverma,

Thanks for the updates - the three issues from our previous review are all addressed:

  • OpenAPI specs now document x-promise-token on both endpoints - clean parameter definition with proper references
  • The suggestions test spy is correctly wired via esmock at the module boundary - the "not called" assertion is now meaningful
  • Preflight test variable fixed

On the encryption bypass we flagged: after deeper investigation, the LLMO controller on main (llmo.js:1276-1383) already follows the exact same pattern - reading x-promise-token from the header and passing it through without encryption. This is deployed and functional, so the pattern is established. The IMS library also documents promise token encryption as optional. No blocker here.

Two minor notes:

  • PR description mentions autofixSuggestionsV2 which doesn't exist in the codebase - just autofixSuggestions
  • The suggestions esmock stub only declares getIMSPromiseToken (relies on esmock's merge behavior), while the preflight test spreads ...utils explicitly. Both work, but the preflight pattern is more defensive. Worth harmonizing in a follow-up.

Looks good.

@ravverma ravverma merged commit 186a599 into main Mar 5, 2026
25 checks passed
@ravverma ravverma deleted the migrate-to-session-token branch March 5, 2026 11:06
solaris007 pushed a commit that referenced this pull request Mar 5, 2026
## [1.335.1](v1.335.0...v1.335.1) (2026-03-05)

### Bug Fixes

* migrate preflight and autofix to session token validation ([#1909](#1909)) ([186a599](186a599))
@solaris007
Copy link
Member

🎉 This PR is included in version 1.335.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Labels

bug Something isn't working released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants