Skip to content

Commit d32fc36

Browse files
committed
feat: guard secret revocation against cutting off the app, verify first deliveries
Backend: revoking an app secret is refused while the app has not demonstrably moved to a replacement — no other live secret has been used yet. That catches the ordinary mistake of revoking the old secret before the app picked up the new one. `?force=true` overrides it — the kill switch for a leaked secret, where cutting the app off now is the point. First use of a secret is now stamped synchronously so the guard cannot be raced by an async write. SDK: a first delivery signs with the secret it carries, which proves nothing, so the receiver now confirms the delivered credentials against the CONFIGURED Tolgee (never a URL from the payload) before trusting them, and rate-limits those checks so a flood of forged deliveries cannot turn the app into an outbound amplifier. Verified deliveries are trusted; unverified ones are rejected. Opt out with verifyCredentials:false to fall back to trust-on-first-use. Frontend: the revoke dialog offers "revoke anyway" when the guard fires.
1 parent 705c84e commit d32fc36

11 files changed

Lines changed: 316 additions & 36 deletions

File tree

apps/tolgee-apps-sdk/src/server/lifecycle/lifecycle.test.ts

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,9 @@ const deliver = async (
7878
stateDir,
7979
seenSignatures,
8080
now: () => NOW,
81+
// These tests exercise the receiver offline (signature, replay, storage,
82+
// dispatch); the verify-by-use network check has its own block below.
83+
verifyCredentials: false,
8184
...rest,
8285
rawBody,
8386
signatureHeader: signatureHeader ?? sign(rawBody, secret, timestamp),
@@ -233,6 +236,85 @@ describe('first delivery', () => {
233236
})
234237
})
235238

239+
describe('verify by use (first delivery, no secret held)', () => {
240+
const originalFetch = globalThis.fetch
241+
242+
afterEach(() => {
243+
globalThis.fetch = originalFetch
244+
})
245+
246+
/** Stubs the credential-verification call to the configured Tolgee. */
247+
const stubVerify = (ok: boolean): string[] => {
248+
const calls: string[] = []
249+
globalThis.fetch = (async (input: RequestInfo | URL) => {
250+
calls.push(String(input))
251+
return new Response(ok ? '{"_embedded":{}}' : '', {
252+
status: ok ? 200 : 401,
253+
})
254+
}) as typeof fetch
255+
return calls
256+
}
257+
258+
const deliverVerified = (
259+
ok: boolean,
260+
options: Parameters<typeof deliver>[1] = {}
261+
) => {
262+
const calls = stubVerify(ok)
263+
return { calls, result: deliver(registered(), options) }
264+
}
265+
266+
it('trusts and stores a first delivery Tolgee vouches for', async () => {
267+
const { calls, result } = deliverVerified(true, {
268+
verifyCredentials: true,
269+
})
270+
const event = assertAccepted(await result)
271+
272+
assert.equal(event.trusted, true)
273+
assert.equal(
274+
readStoredApp(TOLGEE_URL, { stateDir })?.clientSecret,
275+
'tgpubs_app-secret'
276+
)
277+
// Verified against the configured URL, not anything from the payload.
278+
assert.ok(
279+
calls[0].startsWith(`${TOLGEE_URL}/v2/public/apps/app-secrets/list`)
280+
)
281+
})
282+
283+
it('rejects and stores nothing when Tolgee does not accept the credentials', async () => {
284+
const { result } = deliverVerified(false, { verifyCredentials: true })
285+
const rejected = assertRejected(await result)
286+
287+
assert.equal(rejected.rejection, 'unverified-credentials')
288+
assert.equal(rejected.status, 401)
289+
assert.equal(readStoredApp(TOLGEE_URL, { stateDir }), null)
290+
})
291+
292+
it('rate-limits a flood of unverified first deliveries', async () => {
293+
stubVerify(false)
294+
// A URL of its own so the per-instance verify bucket is not shared with the
295+
// other tests in this block.
296+
const floodUrl = 'http://localhost:19876'
297+
const body = JSON.stringify(registered())
298+
let rateLimited = false
299+
for (let i = 0; i < 20; i++) {
300+
const result = await receiveTolgeeDelivery({
301+
tolgeeUrl: floodUrl,
302+
stateDir,
303+
seenSignatures: new Map(),
304+
now: () => NOW + i,
305+
verifyCredentials: true,
306+
rawBody: body,
307+
signatureHeader: sign(body),
308+
})
309+
if (!result.accepted && result.rejection === 'rate-limited') {
310+
rateLimited = true
311+
break
312+
}
313+
}
314+
assert.equal(rateLimited, true)
315+
})
316+
})
317+
236318
describe('a delivery signed with the held secret', () => {
237319
beforeEach(async () => {
238320
assertAccepted(await deliver(registered()))
@@ -445,6 +527,8 @@ describe('the HTTP handler', () => {
445527
stateDir,
446528
seenSignatures,
447529
now: () => NOW,
530+
// These tests cover HTTP framing, not the verify-by-use network call.
531+
verifyCredentials: false,
448532
...options,
449533
})(request, response)
450534
})

apps/tolgee-apps-sdk/src/server/lifecycle/receiveDelivery.ts

Lines changed: 95 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ export type DeliveryRejection =
2727
| 'unreadable-body'
2828
| 'unknown-event'
2929
| 'unverifiable'
30+
| 'unverified-credentials'
31+
| 'rate-limited'
3032
| 'credentials-already-held'
3133

3234
/** Called for a delivery that verified; throwing makes Tolgee retry it. */
@@ -63,6 +65,13 @@ export type TolgeeLifecycleOptions = AppInstallStoreOptions & {
6365
* secret is injected — there is no first delivery to trust.
6466
*/
6567
requireKnownSecret?: boolean
68+
/**
69+
* On a first delivery (no secret held yet), confirm the delivered app
70+
* credentials against the configured Tolgee before trusting them. Defaults to
71+
* true. Set false to fall back to trust-on-first-use — accept the self-signed
72+
* delivery as-is, marked `trusted: false`.
73+
*/
74+
verifyCredentials?: boolean
6675
/** Set false to dispatch without writing anything to the state file. */
6776
persist?: boolean
6877
on?: TolgeeLifecycleListeners
@@ -102,9 +111,55 @@ const REJECTION_STATUS: Record<DeliveryRejection, number> = {
102111
'unreadable-body': 400,
103112
'unknown-event': 400,
104113
unverifiable: 401,
114+
'unverified-credentials': 401,
115+
'rate-limited': 429,
105116
'credentials-already-held': 409,
106117
}
107118

119+
/** How many first-contact verifications the SDK makes to one Tolgee per window. */
120+
const VERIFY_LIMIT = 10
121+
const VERIFY_WINDOW_MS = 60_000
122+
const verifyTimestamps = new Map<string, number[]>()
123+
124+
/**
125+
* A first delivery signs with the secret it carries, which proves nothing, so the SDK confirms the
126+
* delivered credentials against the **configured** Tolgee before trusting them — the URL is always
127+
* the one this app was configured with, never anything from the payload. The call is rate-limited so
128+
* a flood of forged first deliveries cannot turn this app into an outbound amplifier against Tolgee.
129+
*/
130+
const allowVerify = (
131+
tolgeeUrl: string,
132+
now: number
133+
): boolean => {
134+
const recent = (verifyTimestamps.get(tolgeeUrl) ?? []).filter(
135+
(t) => t > now - VERIFY_WINDOW_MS
136+
)
137+
if (recent.length >= VERIFY_LIMIT) {
138+
verifyTimestamps.set(tolgeeUrl, recent)
139+
return false
140+
}
141+
recent.push(now)
142+
verifyTimestamps.set(tolgeeUrl, recent)
143+
return true
144+
}
145+
146+
const verifyDeliveredCredentials = async (
147+
tolgeeUrl: string,
148+
clientId: string,
149+
clientSecret: string
150+
): Promise<boolean> => {
151+
try {
152+
const response = await fetch(`${tolgeeUrl}/v2/public/apps/app-secrets/list`, {
153+
method: 'POST',
154+
headers: { 'Content-Type': 'application/json' },
155+
body: JSON.stringify({ client_id: clientId, client_secret: clientSecret }),
156+
})
157+
return response.ok
158+
} catch {
159+
return false
160+
}
161+
}
162+
108163
const processSeenSignatures = new Map<string, number>()
109164

110165
/**
@@ -147,7 +202,7 @@ export const receiveTolgeeDelivery = async (
147202
)
148203
}
149204

150-
const verified = verify(input, parsed, tolgeeUrl, storeOptions)
205+
const verified = await verify(input, parsed, tolgeeUrl, storeOptions)
151206
if ('rejection' in verified) return verified
152207

153208
const seen = input.seenSignatures ?? processSeenSignatures
@@ -180,12 +235,12 @@ export const receiveTolgeeDelivery = async (
180235

181236
type Verified = { envelope: TolgeeSignatureEnvelope; trusted: boolean }
182237

183-
const verify = (
238+
const verify = async (
184239
input: DeliveryInput,
185240
parsed: ParsedDelivery,
186241
tolgeeUrl: string,
187242
storeOptions: AppInstallStoreOptions
188-
): Verified | Extract<DeliveryResult, { accepted: false }> => {
243+
): Promise<Verified | Extract<DeliveryResult, { accepted: false }>> => {
189244
const known =
190245
input.webhookSecret ??
191246
process.env.TOLGEE_APP_WEBHOOK_SECRET ??
@@ -235,8 +290,8 @@ const verify = (
235290
}
236291

237292
// The signature was made with a key the body handed over, so it proves only
238-
// that the body is internally consistent. Trust it once, and only while there
239-
// is nothing to lose.
293+
// that the body is internally consistent. Never overwrite live credentials on
294+
// the strength of that.
240295
if (hasStoredCredentials(tolgeeUrl, storeOptions)) {
241296
return reject(
242297
'credentials-already-held',
@@ -247,7 +302,41 @@ const verify = (
247302
)
248303
}
249304

250-
return { envelope, trusted: false }
305+
// Verify by use: confirm the delivered credentials against the CONFIGURED Tolgee before trusting
306+
// them. Skipping the check falls back to trust-on-first-use — accepting the self-signed delivery
307+
// as-is, marked untrusted.
308+
if (input.verifyCredentials === false) {
309+
return { envelope, trusted: false }
310+
}
311+
312+
const clientId = parsed.app?.clientId
313+
const clientSecret = parsed.app?.clientSecret
314+
if (!clientId || !clientSecret) {
315+
return reject(
316+
'unverifiable',
317+
'This app holds no credentials for this Tolgee instance and the first delivery carries no ' +
318+
'app credentials to verify against it.'
319+
)
320+
}
321+
322+
const now = input.now?.() ?? Date.now()
323+
if (!allowVerify(tolgeeUrl, now)) {
324+
return reject(
325+
'rate-limited',
326+
`Too many unverified first deliveries for ${tolgeeUrl} — refusing to check more this minute.`
327+
)
328+
}
329+
330+
if (!(await verifyDeliveredCredentials(tolgeeUrl, clientId, clientSecret))) {
331+
return reject(
332+
'unverified-credentials',
333+
`${tolgeeUrl} did not accept the credentials this delivery carried, so it did not come from ` +
334+
'Tolgee. Ignoring it.'
335+
)
336+
}
337+
338+
// Tolgee vouched for the credentials, so this first delivery is genuinely trusted.
339+
return { envelope, trusted: true }
251340
}
252341

253342
const rejectSignature = (

backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/apps/AppSelfAppSecretsController.kt

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,7 @@ class AppSelfAppSecretsController(
6262
description =
6363
"Mints a fresh app-level secret and returns it — the only place it is ever disclosed. The " +
6464
"secret this call authenticated with keeps working, so the app can store the new one and " +
65-
"only then revoke the old one. The new secret is also pushed to the app's base URL over " +
66-
"the lifecycle channel.",
65+
"only then revoke the old one.",
6766
)
6867
fun issue(
6968
@RequestBody @Valid body: AppSecretRotationRequest,
@@ -78,16 +77,16 @@ class AppSelfAppSecretsController(
7877
@Operation(
7978
summary = "Revoke one of the calling app's own app-level secrets",
8079
description =
81-
"The secret stops authenticating immediately. Revoking the app's last live secret is refused " +
82-
"here — the app authenticates with a secret, so it would lock itself out of this very " +
83-
"endpoint. Issue the replacement first. Idempotent.",
80+
"The secret stops authenticating immediately. Refused while the app has not moved to a " +
81+
"replacement — the last live secret, or one issued but never used yet — so an app cannot " +
82+
"lock itself out of this very endpoint. Issue the replacement and use it first. Idempotent.",
8483
)
8584
fun revoke(
8685
@RequestBody @Valid body: AppSecretRotationRequest,
8786
): AppSecretModel {
8887
val app = authenticate(body)
8988
val secretId = body.secretId ?: throw BadRequestException(Message.APP_SECRET_NOT_FOUND)
90-
return appSecretModelAssembler.toModel(appSecretService.revoke(app.id, secretId, allowRevokingLast = false))
89+
return appSecretModelAssembler.toModel(appSecretService.revoke(app.id, secretId, force = false))
9190
}
9291

9392
private fun authenticate(body: AppSecretRotationRequest): App =

backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/organization/OrganizationOwnedAppsController.kt

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import org.springframework.web.bind.annotation.GetMapping
2020
import org.springframework.web.bind.annotation.PathVariable
2121
import org.springframework.web.bind.annotation.PostMapping
2222
import org.springframework.web.bind.annotation.RequestMapping
23+
import org.springframework.web.bind.annotation.RequestParam
2324
import org.springframework.web.bind.annotation.RestController
2425

2526
/**
@@ -114,16 +115,19 @@ class OrganizationOwnedAppsController(
114115
summary = "Revoke an app-level client secret",
115116
description =
116117
"Phase two of a rotation: the secret stops authenticating immediately and every other one is " +
117-
"untouched. Revoking the last live one is allowed — it is how a leaked credential is cut " +
118-
"off before a replacement exists. Idempotent.",
118+
"untouched. Refused while the app has not demonstrably moved to a replacement (no other " +
119+
"live secret has been used yet), so an ordinary rotation cannot cut the app off by mistake. " +
120+
"Pass `force=true` to override — the kill switch for a leaked secret, where cutting the app " +
121+
"off now is the point. Idempotent.",
119122
)
120123
fun revokeSecret(
121124
@PathVariable organizationId: Long,
122125
@PathVariable appId: Long,
123126
@PathVariable secretId: Long,
127+
@RequestParam(required = false, defaultValue = "false") force: Boolean,
124128
): AppSecretModel {
125129
val app = appService.getOwned(organizationId, appId)
126-
return appSecretModelAssembler.toModel(appSecretService.revoke(app.id, secretId, allowRevokingLast = true))
130+
return appSecretModelAssembler.toModel(appSecretService.revoke(app.id, secretId, force = force))
127131
}
128132

129133
@DeleteMapping("/{appId}")

backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/apps/AppCredentialTokenTest.kt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -242,8 +242,10 @@ class AppCredentialTokenTest : AuthorizedControllerTest() {
242242

243243
private fun revokeSecret(secretId: Long) {
244244
loginAsUser()
245+
// force: these cases test the cutoff itself, not the replacement-unused guard, so they revoke
246+
// the old secret before anything authenticates with the new one.
245247
performAuthDelete(
246-
"/v2/organizations/${testData.organization.id}/owned-apps/$appEntityId/secrets/$secretId",
248+
"/v2/organizations/${testData.organization.id}/owned-apps/$appEntityId/secrets/$secretId?force=true",
247249
).andIsOk
248250
}
249251

backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/apps/AppSecretRotationTest.kt

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,8 @@ class AppSecretRotationTest : AuthorizedControllerTest() {
102102
fun `a revoked app secret stops authenticating and the others keep working`() {
103103
val issued = issueAsOwner()
104104
val originalId = liveSecretIds().first { it != issued.get("id").asLong() }
105+
// The app moves to the new secret; only then may the old one be revoked without forcing.
106+
appSelfList(issued.get("secret").asText()).andIsOk
105107

106108
userAccount = testData.user
107109
performAuthDelete("${ownedAppsUrl()}/$appEntityId/secrets/$originalId").andIsOk
@@ -138,6 +140,7 @@ class AppSecretRotationTest : AuthorizedControllerTest() {
138140
fun `an app-level rotation leaves the install and its enablements alone`() {
139141
val issued = issueAsOwner()
140142
val originalId = liveSecretIds().first { it != issued.get("id").asLong() }
143+
appSelfList(issued.get("secret").asText()).andIsOk
141144
userAccount = testData.user
142145
performAuthDelete("${ownedAppsUrl()}/$appEntityId/secrets/$originalId").andIsOk
143146

@@ -194,8 +197,27 @@ class AppSecretRotationTest : AuthorizedControllerTest() {
194197
val secretId = liveSecretIds().single()
195198

196199
userAccount = testData.user
197-
performAuthDelete("${ownedAppsUrl()}/$appEntityId/secrets/$secretId").andIsOk
200+
performAuthDelete("${ownedAppsUrl()}/$appEntityId/secrets/$secretId?force=true").andIsOk
201+
202+
appSelfList(appClientSecret).andIsUnauthorized
203+
}
204+
205+
/** The guard: an owner must not revoke the old secret before the app has moved to the new one. */
206+
@Test
207+
fun `revoking a secret before the app used its replacement is refused, and forced through`() {
208+
val issued = issueAsOwner()
209+
val originalId = liveSecretIds().first { it != issued.get("id").asLong() }
210+
211+
// Neither secret has been used yet, so an ordinary revoke of the original is refused.
212+
userAccount = testData.user
213+
performAuthDelete("${ownedAppsUrl()}/$appEntityId/secrets/$originalId")
214+
.andIsBadRequest
215+
.andHasErrorMessage(Message.APP_SECRET_REPLACEMENT_UNUSED)
216+
appSelfList(appClientSecret).andIsOk
198217

218+
// Force overrides the guard — the kill switch for a leaked secret.
219+
userAccount = testData.user
220+
performAuthDelete("${ownedAppsUrl()}/$appEntityId/secrets/$originalId?force=true").andIsOk
199221
appSelfList(appClientSecret).andIsUnauthorized
200222
}
201223

backend/data/src/main/kotlin/io/tolgee/constants/Message.kt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -377,6 +377,7 @@ enum class Message {
377377
APP_INSTALL_SECRET_NOT_FOUND,
378378
APP_TOO_MANY_LIVE_SECRETS,
379379
APP_CANNOT_REVOKE_LAST_SECRET,
380+
APP_SECRET_REPLACEMENT_UNUSED,
380381
APP_NOT_REGISTERED,
381382
APP_ALREADY_REGISTERED,
382383
APP_NOT_FOUND,

0 commit comments

Comments
 (0)