Skip to content

AUT-1496 Re-authenticate instead of refresh-looping on session-capped token #149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PavelLoparev merged 2 commits into from
Jul 17, 2026
+60 −5
Merged

AUT-1496 Re-authenticate instead of refresh-looping on session-capped token #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35ae5dc
AUT-1496 Re-authenticate instead of refresh-looping on session-capped…
PavelLoparev Jul 16, 2026
a405df8
AUT-1496 Address review: offline tokens and misleading fallback log
PavelLoparev Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions api/auth/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,30 @@ export class SmartlingAuthApi extends SmartlingBaseApi implements AccessTokenPro

this.logger.debug(`Refresh token with: ${JSON.stringify(this.response, SmartlingBaseApi.sensitiveReplacer)}`);

return await this.makeRequest(
const refreshedToken: AccessTokenDto = await this.makeRequest(
"post",
`${this.entrypoint}/authenticate/refresh`,
JSON.stringify({
refreshToken: this.response.refreshToken
})
);

if (!this.isSessionCapped(refreshedToken)) {
return refreshedToken;

@maescomua maescomua Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion (non-blocking): this debug line still reads Can't refresh, doing re-auth with: on the new session-capped fallthrough path — but on that path the refresh call succeeded; it was discarded for being session-capped, not because refresh failed. During an incident, seeing Refreshed token has a session-capped lifetime (5s); re-authenticating. immediately followed by Can't refresh, doing re-auth with: {...} is contradictory and could send an on-call engineer down the wrong path (backend outage vs. expected Keycloak session-cap behavior).

Suggested change
return refreshedToken;
this.logger.debug(`Falling back to re-auth with: ${JSON.stringify(this.response, SmartlingBaseApi.sensitiveReplacer)}`);

(Wording is just a suggestion — the point is decoupling this log from the specific "can't refresh" claim so it reads correctly on both the throw-based and session-capped fallback paths.)

@PavelLoparev PavelLoparev Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed - changed to "Falling back to re-auth with:" so it reads correctly on both the throw-based and session-capped fallback paths. In a405df8.

}

this.logger.debug(`Refreshed token has a session-capped lifetime (${refreshedToken.refreshExpiresIn}s); re-authenticating.`);
}

this.logger.debug(`Can't refresh, doing re-auth with: ${JSON.stringify(this.response, SmartlingBaseApi.sensitiveReplacer)}`);
this.logger.debug(`Falling back to re-auth with: ${JSON.stringify(this.response, SmartlingBaseApi.sensitiveReplacer)}`);

return await this.authenticate();
}

isSessionCapped(response: AccessTokenDto): boolean {
return response.refreshExpiresIn > 0 && response.refreshExpiresIn < this.ttlCorrectionSec;
}

@maescomua maescomua Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion (non-blocking): isSessionCapped fails open if refreshExpiresIn is ever missing or non-numeric on a successful refresh response — undefined < this.ttlCorrectionSec is false, so a malformed response is treated as "not capped" and returned as-is.

In practice this self-heals: the same missing field makes tokenCanBeRenewed()'s arithmetic evaluate to NaN, so the very next refreshToken() call is forced into a full authenticate() anyway. And there's no runtime DTO validation anywhere else in this file (makeRequest returns any from a bare JSON.parse), so adding a guard here would be the only validated field in an otherwise-unvalidated contract.

Flagging as a judgment call rather than a fix: is the one-cycle exposure worth a typeof/Number.isFinite guard, or is matching this file's existing no-validation convention preferable? Either answer seems defensible.

@dimitrystd dimitrystd Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

refreshExpiresIn - I learned yesterday that it can be 0 for offline token type. So probably we should address this case too (because MCP server will use offline tokens).

@PavelLoparev PavelLoparev Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed by combining with dimitrystd's offline-token point below: refreshExpiresIn > 0 && refreshExpiresIn < this.ttlCorrectionSec. The > 0 guard also makes undefined/NaN safely non-capped (same as before), so no separate typeof/Number.isFinite check needed. In a405df8.

@PavelLoparev PavelLoparev Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed - Keycloak sets refresh_expires_in: 0 for offline-type tokens as a sentinel (governed by Offline Session Idle/Max, not the normal refresh TTL), not "already expired". Fixed with a refreshExpiresIn > 0 guard in isSessionCapped, covered by a new test (offline-type refresh returns as non-capped). In a405df8.

Note: tokenCanBeRenewed() has the same refreshExpiresIn-arithmetic issue for the currently held token (computes requestTimestamp + 0 - ttlCorrectionSec, always in the past) - meaning offline-token clients today never even reach the refresh branch, they always full-authenticate(). That predates this PR and isn't in AUT-1496's scope, but worth a follow-up ticket if the MCP server work depends on offline tokens actually being refreshed rather than always re-authenticated.

resetRequestTimeStamp(): void {
this.requestTimestamp = this.time();
}
Expand Down
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "smartling-api-sdk-nodejs",
"version": "2.35.0",
"version": "2.35.1",
"description": "Package for Smartling API",
"main": "built/index.js",
"engines": {
Expand Down
45 changes: 45 additions & 0 deletions test/auth.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ describe("Auth class tests.", () => {
authTokenExistsStub.returns(true);
authTokenCanBeRenewedStub.returns(true);
auth.response = { refreshToken: "test_refresh_token" };
authMakeRequestStub.returns({ accessToken: "refreshed_token", refreshExpiresIn: 3600 });

await auth.refreshToken();

Expand All @@ -90,6 +91,50 @@ describe("Auth class tests.", () => {
sinon.assert.notCalled(authAuthenticateStub);
});

it("Token exists, can be renewed, and refresh returns a normal (non-capped) token: refreshed token is returned as-is.", async () => {
authTokenExistsStub.returns(true);
authTokenCanBeRenewedStub.returns(true);
auth.response = { refreshToken: "test_refresh_token" };

const refreshedResponse = { accessToken: "refreshed_token", refreshExpiresIn: 3600 };
authMakeRequestStub.returns(refreshedResponse);

const result = await auth.refreshToken();

sinon.assert.notCalled(authAuthenticateStub);
assert.deepEqual(result, refreshedResponse);
});

it("Token exists, can be renewed, but refresh returns a session-capped token: falls through to authenticate.", async () => {
authTokenExistsStub.returns(true);
authTokenCanBeRenewedStub.returns(true);
auth.response = { refreshToken: "test_refresh_token" };

const cappedResponse = { accessToken: "capped_refreshed_token", refreshExpiresIn: 5 };
authMakeRequestStub.returns(cappedResponse);
authAuthenticateStub.returns({ accessToken: "reauthed_token" });

const result = await auth.refreshToken();

sinon.assert.calledOnce(authMakeRequestStub);
sinon.assert.calledOnce(authAuthenticateStub);
assert.deepEqual(result, { accessToken: "reauthed_token" });
});

it("Token exists, can be renewed, and refresh returns an offline-type token (refreshExpiresIn: 0): treated as non-capped, not re-authenticated.", async () => {
authTokenExistsStub.returns(true);
authTokenCanBeRenewedStub.returns(true);
auth.response = { refreshToken: "test_refresh_token" };

const offlineResponse = { accessToken: "offline_refreshed_token", refreshExpiresIn: 0 };
authMakeRequestStub.returns(offlineResponse);

const result = await auth.refreshToken();

sinon.assert.notCalled(authAuthenticateStub);
assert.deepEqual(result, offlineResponse);
});

it("Token exists but can't be renewed.", async () => {
authTokenExistsStub.returns(true);
authTokenCanBeRenewedStub.returns(false);
Expand Down