fix: migrate preflight and autofix to session token validation#1909
fix: migrate preflight and autofix to session token validation#1909
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
solaris007
left a comment
There was a problem hiding this comment.
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 thex-promise-tokenheader on the affected endpoints - consumers need to discover it through the spec. - PR description mentions
autofixSuggestionsV2but this function doesn't exist in the codebase. preflight.test.js:721assertssiteId: mockSite.getId()but the test overrides the site toaemCsSite- 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.
|
This PR will trigger a patch release when merged. |
|
thanks @solaris007 SummaryMigrate promise token resolution from IMS-based authentication to session-based authentication. The Changes
MotivationSession-based authentication passes the promise token via the Test Plan
and about the Encryption bypass
It is little misunderstood, the token getting from UI would be obtained from |
solaris007
left a comment
There was a problem hiding this comment.
Hey @ravverma,
Thanks for the updates - the three issues from our previous review are all addressed:
- OpenAPI specs now document
x-promise-tokenon 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
autofixSuggestionsV2which doesn't exist in the codebase - justautofixSuggestions - The suggestions esmock stub only declares
getIMSPromiseToken(relies on esmock's merge behavior), while the preflight test spreads...utilsexplicitly. Both work, but the preflight pattern is more defensive. Worth harmonizing in a follow-up.
Looks good.
## [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))
|
🎉 This PR is included in version 1.335.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Migrate promise token resolution from IMS-based authentication to session-based authentication across autofix and preflight flows. The
x-promise-tokenheader is now the preferred source for the promise token, with IMS token creation retained as a backward-compatible fallback.Changes
src/controllers/suggestions.js— UpdatedautofixSuggestionsto check forx-promise-tokenrequest header first. If present and non-empty, the header value is used directly. Otherwise, falls back to the existing IMS token flow. Added JSDoc forautofixSuggestions.src/controllers/preflight.js— UpdatedcreatePreflightJobwith 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-tokenheader 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
autofixSuggestionsandcreatePreflightJob