From 986bf8f3aa71c9f1fdf2504913a192d307667ef1 Mon Sep 17 00:00:00 2001 From: benjdlambert Date: Tue, 18 Nov 2025 11:03:39 +0100 Subject: [PATCH 1/5] feat: set state column to text instead of varchar Signed-off-by: benjdlambert --- .../20251118120000_oauth_state_text.js | 35 ++++++++++++ plugins/auth-backend/src/migrations.test.ts | 55 +++++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 plugins/auth-backend/migrations/20251118120000_oauth_state_text.js diff --git a/plugins/auth-backend/migrations/20251118120000_oauth_state_text.js b/plugins/auth-backend/migrations/20251118120000_oauth_state_text.js new file mode 100644 index 0000000000..f082b8b9f0 --- /dev/null +++ b/plugins/auth-backend/migrations/20251118120000_oauth_state_text.js @@ -0,0 +1,35 @@ +/* + * Copyright 2025 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// @ts-check + +/** + * @param {import('knex').Knex} knex + */ +exports.up = async function up(knex) { + await knex.schema.alterTable('oauth_authorization_sessions', table => { + table.text('state').nullable().alter(); + }); +}; + +/** + * @param {import('knex').Knex} knex + */ +exports.down = async function down(knex) { + await knex.schema.alterTable('oauth_authorization_sessions', table => { + table.string('state').nullable().alter(); + }); +}; diff --git a/plugins/auth-backend/src/migrations.test.ts b/plugins/auth-backend/src/migrations.test.ts index df7063351a..89def54b71 100644 --- a/plugins/auth-backend/src/migrations.test.ts +++ b/plugins/auth-backend/src/migrations.test.ts @@ -304,4 +304,59 @@ describe('migrations', () => { await knex.destroy(); }, ); + + it.each(databases.eachSupportedId())( + '20251118120000_oauth_state_text.js, %p', + async databaseId => { + const knex = await databases.init(databaseId); + + await migrateUntilBefore(knex, '20251118120000_oauth_state_text.js'); + + // First create a client for the foreign key constraint + await knex + .insert({ + client_id: 'test-client-id', + client_secret: 'test-client-secret', + client_name: 'Test Client', + response_types: JSON.stringify(['code']), + grant_types: JSON.stringify(['authorization_code']), + redirect_uris: JSON.stringify(['https://example.com/callback']), + }) + .into('oidc_clients'); + + // Apply the migration that changes state to TEXT + await migrateUpOnce(knex); + + // Test inserting a state parameter longer than 255 characters + // This is based on the real-world example from the issue + const longState = 'a'.repeat(280); + + await knex + .insert({ + id: 'test-long-state-session', + client_id: 'test-client-id', + redirect_uri: 'https://example.com/callback', + state: longState, + response_type: 'code', + status: 'pending', + expires_at: new Date(Date.now() + 3600000), + }) + .into('oauth_authorization_sessions'); + + await expect( + knex('oauth_authorization_sessions') + .where('id', 'test-long-state-session') + .first(), + ).resolves.toEqual( + expect.objectContaining({ + id: 'test-long-state-session', + state: longState, + }), + ); + + await migrateDownOnce(knex); + + await knex.destroy(); + }, + ); }); From a9315d0f77b2d1cb7b41178bec692a48ca90ff0b Mon Sep 17 00:00:00 2001 From: benjdlambert Date: Tue, 18 Nov 2025 11:04:45 +0100 Subject: [PATCH 2/5] chore: add changeset Signed-off-by: benjdlambert --- .changeset/tiny-chefs-visit.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tiny-chefs-visit.md diff --git a/.changeset/tiny-chefs-visit.md b/.changeset/tiny-chefs-visit.md new file mode 100644 index 0000000000..2a17de343e --- /dev/null +++ b/.changeset/tiny-chefs-visit.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-auth-backend': patch +--- + +Change internal `state` column to `text` to support state of over 255 characters From eb279cbe7b18b44a9c5c00bad796cfd12bb82fe5 Mon Sep 17 00:00:00 2001 From: benjdlambert Date: Tue, 18 Nov 2025 11:12:37 +0100 Subject: [PATCH 3/5] chore: codereview comments Signed-off-by: benjdlambert --- .../20251118120000_oauth_state_text.js | 2 +- plugins/auth-backend/src/migrations.test.ts | 26 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/plugins/auth-backend/migrations/20251118120000_oauth_state_text.js b/plugins/auth-backend/migrations/20251118120000_oauth_state_text.js index f082b8b9f0..3dbef647d4 100644 --- a/plugins/auth-backend/migrations/20251118120000_oauth_state_text.js +++ b/plugins/auth-backend/migrations/20251118120000_oauth_state_text.js @@ -21,7 +21,7 @@ */ exports.up = async function up(knex) { await knex.schema.alterTable('oauth_authorization_sessions', table => { - table.text('state').nullable().alter(); + table.text('state', 'longtext').nullable().alter(); }); }; diff --git a/plugins/auth-backend/src/migrations.test.ts b/plugins/auth-backend/src/migrations.test.ts index 89def54b71..1f6a3a5dc9 100644 --- a/plugins/auth-backend/src/migrations.test.ts +++ b/plugins/auth-backend/src/migrations.test.ts @@ -324,9 +324,35 @@ describe('migrations', () => { }) .into('oidc_clients'); + // Insert a session with state before migration + const existingState = 'existing-short-state'; + await knex + .insert({ + id: 'test-existing-session', + client_id: 'test-client-id', + redirect_uri: 'https://example.com/callback', + state: existingState, + response_type: 'code', + status: 'pending', + expires_at: new Date(Date.now() + 3600000), + }) + .into('oauth_authorization_sessions'); + // Apply the migration that changes state to TEXT await migrateUpOnce(knex); + // Verify existing state persists after migration + await expect( + knex('oauth_authorization_sessions') + .where('id', 'test-existing-session') + .first(), + ).resolves.toEqual( + expect.objectContaining({ + id: 'test-existing-session', + state: existingState, + }), + ); + // Test inserting a state parameter longer than 255 characters // This is based on the real-world example from the issue const longState = 'a'.repeat(280); From 064471c87fa7fe99b2f58b40048651abe1390356 Mon Sep 17 00:00:00 2001 From: benjdlambert Date: Tue, 18 Nov 2025 11:32:15 +0100 Subject: [PATCH 4/5] chore: update sql reports Signed-off-by: benjdlambert --- plugins/auth-backend/report.sql.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/auth-backend/report.sql.md b/plugins/auth-backend/report.sql.md index 7622a5750e..c2fa13c743 100644 --- a/plugins/auth-backend/report.sql.md +++ b/plugins/auth-backend/report.sql.md @@ -18,7 +18,7 @@ | `redirect_uri` | `text` | false | - | - | | `response_type` | `character varying` | false | 255 | - | | `scope` | `text` | true | - | - | -| `state` | `character varying` | true | 255 | - | +| `state` | `text` | true | - | - | | `status` | `text` | true | - | `'pending'::text` | | `user_entity_ref` | `character varying` | true | 255 | - | From 3511ba4431ccf948ee91d29317d7cb41868b7f20 Mon Sep 17 00:00:00 2001 From: benjdlambert Date: Tue, 18 Nov 2025 12:29:24 +0100 Subject: [PATCH 5/5] chore: fix down migration Signed-off-by: benjdlambert --- .../migrations/20251118120000_oauth_state_text.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/plugins/auth-backend/migrations/20251118120000_oauth_state_text.js b/plugins/auth-backend/migrations/20251118120000_oauth_state_text.js index 3dbef647d4..41e128ec20 100644 --- a/plugins/auth-backend/migrations/20251118120000_oauth_state_text.js +++ b/plugins/auth-backend/migrations/20251118120000_oauth_state_text.js @@ -29,6 +29,12 @@ exports.up = async function up(knex) { * @param {import('knex').Knex} knex */ exports.down = async function down(knex) { + // Delete sessions with state > 255 chars since they won't fit in varchar(255) + // These sessions would be unusable with truncated state anyway + await knex('oauth_authorization_sessions') + .whereRaw('LENGTH(state) > 255') + .delete(); + await knex.schema.alterTable('oauth_authorization_sessions', table => { table.string('state').nullable().alter(); });